-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: indexed address within interface #99
Conversation
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 interface change is important for the Balancer team, we should notify them before they redeploy their contract.
The interface shared with the balancer team already had this value |
# Description Our implementation of the cow amm helper contract needed to generate rebalancing orders was very slightly different from the agreed upon interface. This difference prevented us from indexing balancer pools correctly. To support the legacy and balancer pools at the same time both factory implementations need to adhere to the exact same interface (actually just the emitted events). This PR updates the interface and all related deployment info based on cowprotocol/cow-amm#99. # Changes Also the new helper contract returns a slightly different signature which already includes the signer address (expected by the settlement contract). This should generally be nicer for people using the helper but in our code base we expect the signer address to not already be part of the signature bytes so we needed to introduce a small workaround (with comment explaining why). Note that the helper contract for the balancer [implementation](https://etherscan.io/address/0xE50481D88f147B8b4aaCdf9a1B7b7bA44F87823f#code) has the same behavior so everything should work out. I also removed the deployment information for `sepolia` since the helper contract actually supports 0 pools there so we might as well drop it altogether on `sepolia`. ## How to test e2e tests still work
# Description Our implementation of the cow amm helper contract needed to generate rebalancing orders was very slightly different from the agreed upon interface. This difference prevented us from indexing balancer pools correctly. To support the legacy and balancer pools at the same time both factory implementations need to adhere to the exact same interface (actually just the emitted events). This PR updates the interface and all related deployment info based on cowprotocol/cow-amm#99. # Changes Also the new helper contract returns a slightly different signature which already includes the signer address (expected by the settlement contract). This should generally be nicer for people using the helper but in our code base we expect the signer address to not already be part of the signature bytes so we needed to introduce a small workaround (with comment explaining why). Note that the helper contract for the balancer [implementation](https://etherscan.io/address/0xE50481D88f147B8b4aaCdf9a1B7b7bA44F87823f#code) has the same behavior so everything should work out. I also removed the deployment information for `sepolia` since the helper contract actually supports 0 pools there so we might as well drop it altogether on `sepolia`. ## How to test e2e tests still work
This PR:
COWAMMPoolCreated
complies (i.e. the address isindexed
).