Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observe multiple contracts for ETH flow #3249

Merged
merged 17 commits into from
Jan 30, 2025
Merged

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Jan 21, 2025

Description

Implements the idea explained here: #3248 (comment)

The Ethflow contract of interest: https://etherscan.io/address/0x40a50cf069e992aa4536211b23f286ef88752187#code (probably useful to better review and understand refunder code changes)

This solution has a limitation: we can't specify a block number to start indexing the new contract. Upon autopilot restart, indexing resumes from the latest block recorded in the database. This approach assumes synchronized and atomic insertion of events for both the old and new contracts.
As a result, new contract events will only be indexed from the point when the new contract address is added to the autopilot configuration.

This is acceptable under the following assumptions:

  • The configuration to read from the new contract is applied first.
  • The new contract is officially published, and orders are created afterward.

Changes

  • EthFlowRefundRetriever and CoWSwapOnchainOrdersContract are now observing multiple ethflow contracts at the same time.
  • Refunder service adjusted to also detect the ethflow contract for which the refund should be done (so far this was irrelevant since there was only one contract)
  • Updated E2E test for refunder.

How to test

Existing e2e tests.
Updated refunder e2e test to simulate two orders being created on two different ethflow contracts and being properly refunded.
I also manually verified (through observing logs on e2e tests) that indexing from multiple contracts works properly.

@sunce86 sunce86 self-assigned this Jan 27, 2025
@sunce86 sunce86 marked this pull request as ready for review January 29, 2025 10:09
@sunce86 sunce86 requested a review from a team as a code owner January 29, 2025 10:09
@sunce86 sunce86 changed the title [DRAFT] Observe multiple contracts for ETH flow Observe multiple contracts for ETH flow Jan 29, 2025
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks like it should work but I think the code in the refunder could be simplified quite a lot.

crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
.map(|contract| {
contract
.orders(ethcontract::tokens::Bytes(order_hash))
.batch_call(&mut batch)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refunder uses our auto batching logic under the hood. If make use of this this function would probably end up being a lot easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what are you referring to exactly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying HttpTransport automatically batches individual RPC calls together. So instead of manually calling .batch_call() you can simple do things like join_all(rpc_calls) and it will do the same thing while being a lot easier to read.

crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
@@ -98,63 +93,55 @@ impl RefundService {
async fn identify_uids_refunding_status_via_web3_calls(
&self,
refundable_order_uids: Vec<EthOrderPlacement>,
) -> Result<Vec<OrderUid>> {
) -> Vec<(OrderUid, Vec<CoWSwapEthFlowAddress>)> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that an OrderUid can only belong to a single contract we can ditch the vector. And I would suggest to simply copy the contract type instead of messing around with the address to make the code easier to read. The refunder is doing barely any work anyway so I think we should optimize for readability over performance here.

Suggested change
) -> Vec<(OrderUid, Vec<CoWSwapEthFlowAddress>)> {
) -> Vec<(OrderUid, CoWSwapEthFlow)> {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using contract instead of address is not convenient since PartialEq and Hash are not implemented for CoWSwapEthFlow

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming all the comments above will be addressed.

@sunce86 sunce86 requested a review from MartinquaXD January 30, 2025 15:16
@MartinquaXD
Copy link
Contributor

Pushed some changes directly:

  • turned 1 error log into warn because it will become a very noisy alert when we consider the transition period over but people still submit orders via the old contract
  • also logged the UID in ☝️
  • avoided clunky batch call logic and relied on autobatching of underlying HttpTransport
  • grouped ethflow orders in first function instead of second to streamline things
  • use Vec::truncate() instead of .into_iter().take(n).collect()

@sunce86
Copy link
Contributor Author

sunce86 commented Jan 30, 2025

LG.

Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went quickly through the code and the changes look reasonable to me, everything is consistent with my understanding of the contract and the refunder.

@sunce86 sunce86 enabled auto-merge (squash) January 30, 2025 16:55
@sunce86 sunce86 merged commit 6b7f719 into main Jan 30, 2025
11 checks passed
@sunce86 sunce86 deleted the multiple-ethflow-contracts-2 branch January 30, 2025 17:00
@github-actions github-actions bot locked and limited conversation to collaborators Jan 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants