Sepolia fix for Cow AMMs #3110
Sepolia fix for Cow AMMs #3110
Conversation
| @@ -86,6 +90,26 @@ impl Amm { | |||
| let raw_signature = signature.0.into_iter().skip(20).collect(); | |||
| let signature = Signature::Eip1271(raw_signature); | |||
|
|
|||
| // Cow AMM pools can have specific requirements for the signature validity. | |||
There was a problem hiding this comment.
nit: The underlying issue is not that specific cow amms might have special requirements but rather that the helper contract might produce invalid signatures at all.
There was a problem hiding this comment.
But it's still Cow AMM specific EIP1271 signature check implementation causing the invalidity right... I mean, the order is good and signature is good, it's only bad in context of using that signature/order on cow amm pool.
Anyway, your statement is true as well. I think git blame can be used here to give more context in not clear.
There was a problem hiding this comment.
But it's still Cow AMM specific EIP1271 signature check implementation causing the invalidity right.
The meaning of the signature is entirely implementation specific. If it doesn't work for the only thing it's supposed to work it's not really a good signature is it? 😅
The correct behavior would be that the helper contract refuses to create the signature in the first place because it's supposed to be aware of the implementation details of the cow amms it's supposed to work for.
It's not the case that there is a catch all implementation of the helper contract which happens to be incompatible with the balancer cow amm. The helper was specifically written for the balancer version so we should be able to expect that it works.
But it's also true that by now there is enough context on the matter to be found with git blame.
There was a problem hiding this comment.
Thanks for taking a stab at it. I'd be a bit more comfortable if we could better test this (test plan reads a bit like 🤞), but I guess adding an e2e test is more work than testing in staging.
For this however, we need to disable cow amms in sepolia prod and enable it in staging to see it fixes the issue.
| @@ -48,12 +51,13 @@ impl Amm { | |||
| let (order, pre_interactions, post_interactions, signature) = | |||
| self.helper.order(self.address, prices).call().await?; | |||
| self.convert_orders_reponse(order, signature, pre_interactions, post_interactions) | |||
| .await | |||
There was a problem hiding this comment.
nit: check validity would make sense between fetching the order and converting it (ie only convert "valid" orders).
There was a problem hiding this comment.
True, but the whole convert_orders_reponse functionality is already needed to check the signature (order hash building and signature building), so it makes it really convenient to be part of it.
Given that this issue is easily reproducable on sepolia at the moment we could just create a forked e2e test for the problematic pool and assert that we are not able to generate the bad order. |
| if !self | ||
| .helper | ||
| .is_legacy(self.contract.address()) | ||
| .call() | ||
| .await? |
There was a problem hiding this comment.
Is there a reason to only check the non-legacy pools?
IMO this is the correct thing to do anyway and we don't really gain anything from omitting very very few RPC requests with this check.
Also I think the actual helper interface doesn't even have this function.
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
Description
Signature verification is now part of order creation for cow amm orders.
Changes
How to test
Cow amm e2e test should confirm no new issues are introduced.
Fixes #
#3102