Add weth support for Optimism Spoke pool. #34
Conversation
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
nicholaspai
left a comment
nicholaspai
left a comment
There was a problem hiding this comment.
Looks good left some questions.
| // TODO: Should relay message to L2 for destinationChainId and call setEnableRoute(originToken, destinationChainId, true) | ||
| // Note that this method makes no L1->L1 call to whitelist the route. The assumption is that the origin chain's | ||
| // SpokePool Owner will call setEnableRoute to enable the route. This removes the need for an L1->L2 call. | ||
| whitelistedRoutes[originToken][destinationChainId] = destinationToken; |
There was a problem hiding this comment.
This comment no longer applies as my outstanding PR #33 will make the L1-->L2 call
contracts/Optimism_SpokePool.sol
Outdated
| contract Optimism_SpokePool is CrossDomainEnabled, SpokePoolInterface, SpokePool, Ownable { | ||
| // "l1Gas" parameter used in call to bridge tokens from this contract back to L1 via `IL2ERC20Bridge`. | ||
| uint32 l1Gas = 6_000_000; | ||
| uint32 public l1Gas = 1_900_000; |
There was a problem hiding this comment.
This is L1 gas. Are you sure this should be set to 1.9mil? I thought only the L2 gas param in the Optimism Adapter (which calls L1--> L2 txns) should use this value
There was a problem hiding this comment.
I'm not exactly sure. tbh I'm not exactly sure how it works going from L2->L1, only really know what the costing is from L1->L2.
There was a problem hiding this comment.
Hmmm i believe this value is unused as per the comments but let's double check
There was a problem hiding this comment.
I think you can leave this at 6mil, but +1 on making it public
contracts/Optimism_SpokePool.sol
Outdated
| if (distributionLeaf.l2TokenAddress == address(weth)) { | ||
| WETH9(l2Token).withdraw(distributionLeaf.amountToReturn); // Unwrap ETH. | ||
| l2Token = l2Eth; // Set the bridged token to the Optimism ETH address. | ||
| l1Destination = l1EthWrapper; // Set the destination to the L1 ETH wrapper. |
There was a problem hiding this comment.
Why is the destination the l1 eth wrapper, how does thhis wrapper contract know who the end recipient is?
There was a problem hiding this comment.
this will be this contract: https://github.com/across-protocol/across-smart-contracts/blob/master/contracts/insured-bridge/ovm/Optimism_Wrapper.sol which simply receives eth, wraps it and sends it to the hupPool (the destination). I'll add this contract to this PR.
There was a problem hiding this comment.
I've added a modified version of this contract to this PR.
There was a problem hiding this comment.
@nicholaspai I've pretty drastically refactored this.
| * and https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol. | ||
| */ | ||
| contract Lockable { | ||
| bool internal _notEntered; |
There was a problem hiding this comment.
I had to copy this to change it to be internal so the HubPool can read from it.
contracts/HubPool.sol
Outdated
| // contract. In this case, deposit the ETH into WETH. If true then this was set as a result of unwinding LP tokens, | ||
| // with the intention of sending ETH to the LP. In this case, do nothing as we intend on sending the ETH to the LP. | ||
| function depositEthToWeth() public payable { | ||
| if (_notEntered) weth.deposit{ value: address(this).balance }(); |
There was a problem hiding this comment.
As discussed IRL, let's expose _notEntered via a descriptive public function and call that here
Originally authored by Nick & Matt. Signed-off-by: nicholaspai npai.nyc@gmail.com Signed-off-by: Matt Rice matthewcrice32@gmail.com Co-authored-by: Matt Rice matthewcrice32@gmail.com
No description provided.