-
Notifications
You must be signed in to change notification settings - Fork 152
Remove CoW AMM indexer from driver #3680
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
Conversation
|
Reminder: Please consider backward compatibility when modifying the API specification.
Resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Removes the CoW AMM indexer from the driver by having autopilot send CoW AMM to helper address mappings directly in auction data. This eliminates the need for driver to index CoW AMM deployment events from blockchain data.
- Replaces old
Vec<Address>CoW AMM list withHashMap<Address, Vec<Address>>mapping helpers to AMMs - Removes CoW AMM configuration and indexing infrastructure from driver
- Creates in-memory cache for CoW AMM instances to avoid re-indexing on restarts
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/e2e/src/setup/deploy.rs | Removes CoW AMM helper contract deployment and configuration |
| crates/e2e/src/setup/colocation.rs | Removes CoW AMM configuration generation for test setup |
| crates/driver/src/tests/setup/solver.rs | Updates test setup to use new helper-to-owners mapping structure |
| crates/driver/src/tests/setup/mod.rs | Changes surplus capturing owner storage from Vec to HashMap by helper |
| crates/driver/src/tests/setup/driver.rs | Updates API request field name for new data structure |
| crates/driver/src/tests/cases/jit_orders.rs | Updates test case to use new helper-based owner mapping |
| crates/driver/src/run.rs | Removes archive node URL parameter from Ethereum initialization |
| crates/driver/src/infra/solver/mod.rs | Updates method call to use new helper-based owner mapping |
| crates/driver/src/infra/solver/dto/solution.rs | Updates method call for surplus capturing owner access |
| crates/driver/src/infra/solver/dto/auction.rs | Changes surplus owner extraction to use HashMap keys |
| crates/driver/src/infra/observe/mod.rs | Updates function signature to accept HashMap instead of HashSet |
| crates/driver/src/infra/config/mod.rs | Removes archive node URL and CoW AMM configuration fields |
| crates/driver/src/infra/config/file/mod.rs | Removes CoW AMM configuration structures and archive node field |
| crates/driver/src/infra/config/file/load.rs | Removes CoW AMM and archive node configuration loading |
| crates/driver/src/infra/blockchain/mod.rs | Removes archive node parameter from Ethereum constructor |
| crates/driver/src/infra/blockchain/contracts.rs | Removes CoW AMM registry and related indexing infrastructure |
| crates/driver/src/infra/api/routes/solve/dto/solve_request.rs | Updates API to accept HashMap and convert to owner-to-helper mapping |
| crates/driver/src/domain/quote.rs | Updates method calls to use new mapping structure |
| crates/driver/src/domain/mod.rs | Adds cow_amm module to domain |
| crates/driver/src/domain/cow_amm.rs | Implements new CoW AMM cache for on-demand AMM instance creation |
| crates/driver/src/domain/competition/solution/settlement.rs | Updates scoring method signature for new mapping |
| crates/driver/src/domain/competition/solution/mod.rs | Updates solution methods to use HashMap instead of HashSet |
| crates/driver/src/domain/competition/pre_processing.rs | Integrates CoW AMM cache and updates AMM order generation |
| crates/driver/src/domain/competition/mod.rs | Updates solution filtering and scoring calls |
| crates/driver/src/domain/competition/auction.rs | Changes surplus capturing owner storage to HashMap |
| crates/driver/openapi.yml | Updates API schema to reflect new HashMap structure |
| crates/driver/example.toml | Removes CoW AMM configuration example |
| crates/cow-amm/src/amm.rs | Makes Amm constructor public and adds helper address accessor |
| crates/autopilot/src/solvable_orders.rs | Groups CoW AMMs by helper address and updates fee calculation |
| crates/autopilot/src/run_loop.rs | Updates auction data structure usage throughout |
| crates/autopilot/src/infra/solvers/dto/solve.rs | Sends both old and new data structures for backward compatibility |
| crates/autopilot/src/infra/persistence/mod.rs | Updates persistence to handle new HashMap structure |
| crates/autopilot/src/infra/persistence/dto/auction.rs | Updates auction DTO to use HashMap for surplus owners |
| crates/autopilot/src/domain/fee/mod.rs | Updates fee calculation to work with new HashMap structure |
| crates/autopilot/src/domain/competition/winner_selection.rs | Updates winner selection to extract owners from HashMap |
| crates/autopilot/src/domain/auction/mod.rs | Changes auction data structure to use HashMap |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| .into_iter() | ||
| // Only generate orders for cow amms the auction told us about. | ||
| // Otherwise the solver would expect the order to get surplus but | ||
| // the autopilot would actually not count it. | ||
| .filter(|amm| auction.surplus_capturing_jit_order_owners.contains(ð::Address(*amm.address()))) | ||
| // Only generate orders where the auction provided the required | ||
| // reference prices. Otherwise there will be an error during the | ||
| // surplus calculation which will also result in 0 surplus for |
Copilot
AI
Sep 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment starting at line 358 appears to be incomplete or cut off. The comment should be completed to explain what happens when reference prices are not provided.
# Conflicts: # crates/e2e/src/setup/deploy.rs
|
You can avoid sending any additional data in the auction by looking up each cow amms factory address with |
Sorry, which factory? The proposal fully drops the driver's cow amm config. |
# Conflicts: # crates/driver/src/domain/competition/pre_processing.rs # crates/driver/src/infra/api/routes/solve/dto/solve_request.rs # crates/e2e/src/setup/deploy.rs
|
The cow amm contracts have a public member variable |
# Conflicts: # crates/contracts/src/alloy.rs # crates/driver/src/infra/blockchain/contracts.rs # crates/driver/src/tests/setup/solver.rs # crates/e2e/src/setup/deploy.rs
# Conflicts: # crates/driver/src/domain/competition/pre_processing.rs
jmg-duarte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left some nits and one point for discussion before approval
# Conflicts: # crates/driver/src/infra/blockchain/mod.rs # crates/driver/src/run.rs # crates/driver/src/tests/setup/solver.rs
| /// Archive node URL used to index CoW AMM | ||
| archive_node_url: Option<Url>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you already thought of removing this. 👍
crates/e2e/tests/e2e/cow_amm.rs
Outdated
| // This keeps the helper's template order around 30% of the pool balance. | ||
| // This imbalance shouldn't exceed 50%, since such an order will be rejected by | ||
| // the SC: <https://github.com/balancer/cow-amm/blob/84750b705a02dd600766c5e6a9dd4370386cf0f1/src/contracts/BPool.sol#L250-L252> | ||
| let usdc_imbalance_amount = U256::from(15_097_988_166_u64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you prefer using a hardcoded value here instead of computing it using information queried on the pool?
If someone were to update the WETH we send to the pool they would also have to redo this math.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for flip flopping on this maybe I didn't fully understand the need to make the imbalance a specific value. Judging by the comment it seems like as long as the imbalance on the pool is less than 50% we are good. If we don't actually have any test asserting properties on the order prices based on that imbalance we could also just imbalance the pool using some hardcoded amount and add an assertion + comment to make sure the pool is less than 50% imbalanced (whatever that means).
Maybe you could elaborate a bit more why it's now useful to unbalance the pool to a very specific amount. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could elaborate a bit more why it's now useful to unbalance the pool to a very specific amount. 🤔
Maybe i need to rephrase the code comment. The idea is to imbalance by 30% or so and that is all. I just funded it with 10 WETH, then we need to calculate a proper USDC value to imbalance it enough, so the driver can create a CoW AMM order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time of finalization, the AMM it empty if I didn't miss anything, so we need at least something there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the block you have configured for the forked test the pool had 1925 USDC and 0.77 WETH in a 50/50 weighting. That means the amount you imbalance the pool with is relatively small (originally 2500 USDC / WETH and now with 2551 USDC / WETH). I think simply doing a small one sided deposit should be sufficient and avoid a bunch of math and details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true, i somehow missed the correct balances. Simplified the test.
| addr!("5afe3855358e112b5647b952709e6165e1c1eeee"), // SAFE | ||
| ]; | ||
|
|
||
| wait_for_condition(TIMEOUT, || async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update this part to check that the driver was actually able to generate a cow amm order for the desired pool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, the amounts are pretty weird. Sell/buy amounts are about $10...
Order {
uid: [81, 95, 167, 62, 197, 127, 127, 51, 77, 255, 175, 153, 213, 48, 132, 162, 19, 231, 229, 129, 4, 104, 182, 49, 246, 123, 172, 231, 142, 82, 222, 32, 240, 141, 77, 234, 54, 156, 69, 109, 38, 163, 22, 143, 240, 2, 75, 144, 79, 45, 139, 145, 102, 179, 104, 168],
sell_token: 0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2,
buy_token: 0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48,
sell_amount: 4365551272787746,
full_sell_amount: 4365551272787746,
buy_amount: 10800057,
full_buy_amount: 10800057,
fee_policies: None,
valid_to: 1723033768,
kind: Sell,
receiver: Some(0x0000000000000000000000000000000000000000),
owner: 0xf08d4dea369c456d26a3168ff0024b904f2d8b91,
partially_fillable: true,
pre_interactions: [InteractionData {
target: 0xf08d4dea369c456d26a3168ff0024b904f2d8b91,
value: 0,
call_data: [241, 79, 203, 200, 81, 95, 167, 62, 197, 127, 127, 51, 77, 255, 175, 153, 213, 48, 132, 162, 19, 231, 229, 129, 4, 104, 182, 49, 246, 123, 172, 231, 142, 82, 222, 32]
}],
post_interactions: [],
sell_token_source: Erc20,
buy_token_destination: Erc20,
class: Limit,
app_data: 0x362e5182440b52aa8fffe70a251550fbbcbca424740fe5a14f59bf0c1b06fe1d,
flashloan_hint: None,
signing_scheme: Eip1271,
signature: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 192, 42, 170, 57, 178, 35, 254, 141, 10, 14, 92, 79, 39, 234, 217, 8, 60, 117, 108, 194, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 160, 184, 105, 145, 198, 33, 139, 54, 193, 209, 157, 74, 46, 158, 176, 206, 54, 6, 235, 72, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 15, 130, 114, 28, 220, 255, 34, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 164, 203, 185, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 102, 179, 104, 168, 54, 46, 81, 130, 68, 11, 82, 170, 143, 255, 231, 10, 37, 21, 80, 251, 188, 188, 164, 36, 116, 15, 229, 161, 79, 89, 191, 12, 27, 6, 254, 29, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 243, 178, 119, 114, 139, 63, 238, 116, 148, 129, 235, 62, 11, 59, 72, 152, 13, 187, 171, 120, 101, 143, 196, 25, 2, 92, 177, 110, 238, 52, 103, 117, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 90, 40, 233, 54, 59, 185, 66, 182, 57, 39, 0, 98, 170, 107, 178, 149, 244, 52, 188, 223, 196, 44, 151, 38, 123, 240, 3, 242, 114, 6, 13, 201, 90, 40, 233, 54, 59, 185, 66, 182, 57, 39, 0, 98, 170, 107, 178, 149, 244, 52, 188, 223, 196, 44, 151, 38, 123, 240, 3, 242, 114, 6, 13, 201]
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the native price for USDC in the test? The helper contract computes a suggested order based on the price difference between native price and the pool price. If they are close together the order would be small.
# Conflicts: # crates/driver/src/infra/blockchain/contracts.rs # crates/e2e/src/setup/deploy.rs
MartinquaXD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic change looks solid to me. The remaining open questions are mostly about how to unbalance the pool to make sense of this. But given that we don't want to test the logic of the helper contract and already have an assertion that we are generating SOME suggested order I'm fine with merging this.
Although simplifying the unbalancing pool to do a single one sided deposit might be nicer.
# Conflicts: # crates/e2e/src/setup/deploy.rs
Description
Currently, both autopilot and driver use the same CoW AMM indexer functionality in parallel to serve different purposes: autopilot needs to fill out surplus capturing jit order owners in the auction and driver uses it to fetch tradable tokens and then prepare CoW AMM template orders. Since autopilot already sends CoW AMM addresses within each auction, the only missing part is the CoW AMM helper SC address. It can be received by fetching the CoW AMM factory address and then using a config mapping to find the helper address. This should also reduce the RPC traffic.
Changes
FACTORYfunction. Since we can now completely deprecate legacy CoW AMMs, this logic is simplified. I also used a simple abi to create rust bindings for this interface. The fetched data is stored in the memory cache.How to test
Adjusted forked e2e test.