-
Notifications
You must be signed in to change notification settings - Fork 40
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: add weth -> eth/weth bridge functionality #1115
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: bennett <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
referrer, | ||
integratorId, | ||
}: AcrossDepositArgs, | ||
spokePool: SpokePool, | ||
spokePoolVerifier: SpokePoolVerifier, | ||
onNetworkMismatch?: NetworkMismatchHandler | ||
): Promise<ethers.providers.TransactionResponse> { | ||
/* We cannot send WETH to the recipient on the destination chain with the spoke pool verifier until |
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.
Marking this to delete in case we want to merge this PR before ETH -> WETH support
(isDestinationChainPolygon || isReceiverContract) | ||
) { | ||
return "WETH"; | ||
} | ||
|
||
if ( |
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.
I'm pretty sure this is OK to remove, since we now want to say that it's possible to bridge from WETH -> ETH/WETH unless the destination is a contract or Polygon, in which case we enforce sending WETH.
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.
Yes, that should be fine
]; | ||
} | ||
/* | ||
* TODO: Handle ETH -> ETH/WETH |
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.
Marking to remove if we merge this before ETH -> WETH/ETH support.
I've so far checked WETH -> WETH transfers from Arbitrum -> All other chains and then Optimism -> Arbitrum. |
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 looks very good! I left some smaller remarks though. And another issue we should fix is captured in the screenshot below:
The tooltip is technically not wrong but redundant. We should remove it by adding the condition
inputToken.symbol === "WETH"
to
inputToken.symbol === "USDC" || |
import { getProvider } from "./providers"; | ||
import { getDeployedAddress } from "@across-protocol/contracts"; |
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 line unfortunately increases the FE bundle size by ~30%... This is due to the @across-protocol/contracts
package being non-tree shakeable which is a known issue for some time. Can we for now just hardcode the MulticallHandler
addresses in https://github.com/across-protocol/frontend/pull/1115/files#diff-ca62a13994aae3c545ad2d796d272f3ee8c0ab33b83bb3d5c023dc553b55f984R29-R36?
I think we'd need to populate the addresses into the routes*.json
file during the build time ideally. But we can tackle that in a separate PR
// If a user is interacting with the front end, assume that they are an EOA and not | ||
// a contract. Since they are an EOA, by default they will receive ETH on the destination | ||
// (except for Polygon). Therefore, we only need to use the multicall handler when the EOA | ||
// wants to receive WETH after an ETH or WETH deposit. |
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.
I think we need to actually check whether the recipient
is an EOA using isContractDeployedToAddress
. And only if it is not a contracts, use the multicall handler?
(isDestinationChainPolygon || isReceiverContract) | ||
) { | ||
return "WETH"; | ||
} | ||
|
||
if ( |
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.
Yes, that should be fine
We want to be able to support WETH/ETH -> WETH/ETH deposits. Since the contracts will always unwrap to an EOA, we need to use the multicall handler to perform the weth wraps and unwraps. This commit should inject a message into the transaction request which performs these unwraps (or weth transfers) when a deposit condition is set.