Remove CoW AMMs#4624
Conversation
|
Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable).
Caused by: |
|
Claude encountered an error after 4m 1s —— View job PR Review: Remove CoW AMMs
The removal looks clean and consistent: the I flagged two rollout-coordination concerns as inline comments — neither is a logic bug, but both can break a deploy:
One behavioral note (not flagged, appears intentional): surplus-capturing JIT owners are no longer filtered by native-price availability in Given the PR is explicitly not meant to merge yet, these are mainly things to sequence correctly when you do execute. |
| @@ -0,0 +1 @@ | |||
| DROP TABLE IF EXISTS cow_amms; | |||
There was a problem hiding this comment.
Rollout ordering: dropping cow_amms in the same release that removes the indexer is risky.
Per the DB rollout note, during a deploy k8s starts the new autopilot, runs the Flyway migration (this DROP TABLE), and only then shuts down the old pod. During that overlap the old autopilot is still running its cow-amm indexer, which reads/writes cow_amms. In the current (old) code that indexer runs inside run_essential_maintenance's tokio::try_join! — so once the table is gone, every essential-maintenance cycle on the old pod will error, which can stall auction building until the old pod is finally terminated.
Suggest splitting into two releases: this PR removes all code that touches cow_amms (keep the table), then ship DROP TABLE cow_amms in a follow-up release once no running version references it.
Also worth noting for the DB README/checklist: DROP TABLE is not reversible (data loss) — intentional here, but should be called out.
There was a problem hiding this comment.
Claude is correct and if the drop takes a while it might break deployment too
There was a problem hiding this comment.
The drop itself would be super fast but the new autopilot takes a while to become ready. During that time the maintenance of the cow amms would fail.
When dropping the table, please also delete its documentation in the readme file.
| /// Allowlist of owner addresses whose JIT orders are allowed to capture | ||
| /// surplus in the auction. | ||
| #[serde(default)] | ||
| pub cow_amm: CowAmmGroupConfig, | ||
| pub surplus_capturing_jit_order_owners: Vec<Address>, |
There was a problem hiding this comment.
Configuration is annotated with #[serde(rename_all = "kebab-case", deny_unknown_fields)], and this PR removes the cow-amm field. Any deployed autopilot config that still contains a cow-amm = { ... } section will now fail to deserialize and the process won't start.
The same applies to the driver's ContractsConfig (also deny_unknown_fields), which drops the cow-amms list.
Make sure the infra config for every environment has the cow-amm / cow-amms sections removed as part of (or before) this rollout so the new binaries can boot.
| @@ -0,0 +1 @@ | |||
| DROP TABLE IF EXISTS cow_amms; | |||
There was a problem hiding this comment.
Claude is correct and if the drop takes a while it might break deployment too
| let base_auction = tasks.auction.clone().await; | ||
|
|
||
| let auction = Auction { | ||
| id: base_auction.id, | ||
| orders: base_auction | ||
| .orders | ||
| .iter() | ||
| .cloned() | ||
| .chain(cow_amm_orders.iter().cloned()) | ||
| .collect(), | ||
| orders: base_auction.orders.clone(), |
There was a problem hiding this comment.
unless im missing something can't we do
let BaseAuction { elements go here } = tasks.auction.clone().await;
and remove the later clones?
There was a problem hiding this comment.
No, the auction returned from the future is Arced. But since orders are now Arced as well we can consider unArcing the auction to directly return an owned auction.
That should not be addressed in this PR, though. Might make sense to look into this during the 1 block per auction speedup investigations.
| /// surplus in the auction. | ||
| #[serde(default)] | ||
| pub cow_amm: CowAmmGroupConfig, | ||
| pub surplus_capturing_jit_order_owners: Vec<Address>, |
There was a problem hiding this comment.
the infra pr removes the fields and doesnt add any, do we actually need this or could we also remove this field and remove even more things?
MartinquaXD
left a comment
There was a problem hiding this comment.
🧹
Not sure if we actually need to keep the surplus capturing jit order owner concept but removing this might also be a bit more involved. Starting by ripping out only the cow amm specific stuff is definitely the right call.
Description
As we are stopping support for CoW AMMs, this PR removes all occurrences of the concept in our backend code base. It leaves the underlying protocol concept of "surplus capturing jit orders" (the idea that the protocol can define certain accounts, for which the solver can create arbitrary limit orders during the competition and execute them with surplus that counts towards their score), as this is relatively little overhead to maintain and potentially a useful concept down the line.
Note, I'm not intending to merge this PR right away (mainly wanted to get feedback and a feel if I'm missing anything). We still need to talk to solvers and the one remaining LP before we can execute on this.
Changes
How to test
CI
Related Issues
https://linear.app/cowswap/issue/BE-99/remove-cow-amm-support