-
Notifications
You must be signed in to change notification settings - Fork 108
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
feat: use new deterministic ethflow contracts #5334
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
9ec74e6
to
696a633
Compare
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've retested cancellation expiration, successful cases on all chains when the flag is ON and OFF: no issues found except a know one: #4153
52fb6a3
to
9ba727a
Compare
@@ -72,7 +80,7 @@ export async function ethFlow( | |||
throw new Error('Quote expired. Please refresh.') | |||
} | |||
|
|||
if (contract.address !== COWSWAP_ETHFLOW_CONTRACT_ADDRESS_MAP[chainId as SupportedChainId]) { | |||
if (contract.address !== getEthFlowContractAddresses(ethFlowEnv, useNewEthFlowContracts, chainId)) { |
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.
Should we add .toLowerCase() here?
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 I saw this before merging. It didn't have if before, but it may be wise. I can address in develop (as this works without the change, if something, is in case a future library of ethers breaks this logic)
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.
Summary
This PR is in preparation for the ETH FLOW migration. We will replace BARN and PROD eth flow contracts with some new version deployed in deterministic address (same address to all networks).
This is done to minimize the risk of sending ETH to the wrong address in case of a Dapp or Wallet bug.
Context
Tracker for the changes https://www.notion.so/cownation/tracker-ETH-Flow-Contract-Replacements-1828da5f04ca8023af57e76cf6d064d9?pvs=4
See the new deployment and more context:
cowprotocol/ethflowcontract#59
Test
Test old contracts
With he feature flag off, make sure it uses the old barn contracts:
Test the new flag
If we enable the flag, it should use
0x04501b9b1d52e67f6862d157e00d13419d2d6e95
no matter which network.Test new contracts
We could set the flag to
true
, but because multiple teams are using this PR, we might be affecting each other#5358
Details on what to test
Test flows:
How to change the flag: