-
Notifications
You must be signed in to change notification settings - Fork 7
Support multiple networks #61
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
ce27bbf to
184cdb3
Compare
| - id: yarn-cache | ||
| run: echo "::set-output name=dir::$(yarn cache dir)" | ||
| - uses: actions/cache@v2 | ||
| - uses: actions/cache@v4 |
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.
CI was complaining it was too old version of it
script/ValidatedAddress.sol
Outdated
| _wrappedNativeToken = 0x4200000000000000000000000000000000000006; | ||
| require(eq(WithSymbol(_wrappedNativeToken).symbol(), "WETH")); | ||
| } else if (chainId() == CHAINID_POLYGON) { | ||
| _wrappedNativeToken = 0x0000000000000000000000000000000000001010; |
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.
This is weird. I doubt the eth-flow contract can work with this because there's no fallback to call (required here). There isn't even a deposit() function, it seems, just a deposit(address, amount), and it's not payable? This won't work unfortunately. 0x0d500b1d8e8ef31e21c99d1db9a6444d3adf1270 looks like a better candidate, but I'm not sure how "official" it is.
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.
@fedgiac would it be fine for the ethflow contract to have a different native token than the official one?
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 unofficial version has 5 times as many holders according to etherscan so I think we are not the only ones who prefer the other token. 😅
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.
As long as it's compatible with the WETH ABI (not like this) and has some liquidity to convert it to the "official" wrapped version, it should be ok (but it would also mean that native token trades become more inefficient/expensive that they should because of the extra hops on average).
Unfortunately, this contract isn't ABI-compatible and so in the best case it doesn't work, in the worst case user funds are lost. 😓
Maybe we should add a test for WETH compatibility?
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.
Can confirm having worked with POL native asset before that 0x0d500b1d8e8ef31e21c99d1db9a6444d3adf1270 is indeed the correct contract to use. I had some lengthy discussions with the Zerion Team about this back in January, however I can't seem to find their discord server anymore.
fedgiac
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.
As a "popularity test," I compared the wrapped native token addresses here with those presented on the Uniswap interface by typing "wrapped" and selecting the native token in the corresponding chain.
I got that all addresses match except for Polygon, where it's 0x0d500b1d8e8ef31e21c99d1db9a6444d3adf1270 instead.
If that address is changed, then the PR looks good.
Addresses
Optimism
0x4200000000000000000000000000000000000006
BNB
0xbb4CdB9CBd36B01bD1cBaEBF2De08d9173bc095c
Polygon
0x0d500b1d8e8ef31e21c99d1db9a6444d3adf1270
Avalanche
0xb31f66aa3c1e785363f0875a1b74e27b85fd66c7
fedgiac
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.
Wrong button pressed, this is a requested change.
|
@fedgiac thanks a lot! done! |
57ee572 to
d66b157
Compare
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
It is ready to go now. |
fedgiac
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.
Looks good, thanks for the work!
Details on how I verified the deployments
I checked that:
- The code on all new networks looks consistent with that on Sepolia (with some expected changes for WETH and the domain separator).
- They use the expected settlement contract.
- They use the WETH address specified in the script.
Specifically, I used the following commands:
To get the code:
net=polygon; cast code --rpc-url $net 0xba3cb449bd2b4adddbc894d8697f5170800eadec > $net.0xba.txtTo get the settlement contract:
for net in sepolia avalanche bnb optimism polygon; do for addr in 0x04501b9b1d52e67f6862d157e00d13419d2d6e95 0xba3cb449bd2b4adddbc894d8697f5170800eadec; do cast abi-decode --input "f(address)" "$(cast call --rpc-url "$net" "$addr" "cowSwapSettlement()")"; done; doneTo get the WETH addresses:
for net in sepolia avalanche bnb optimism polygon; do for addr in 0x04501b9b1d52e67f6862d157e00d13419d2d6e95 0xba3cb449bd2b4adddbc894d8697f5170800eadec; do cast abi-decode --input "f(address)" "$(cast call --rpc-url "$net" "$addr" "wrappedNativeToken()")"; done; done
Support networks: Avalance, Optimism, BNB and Polygon.