fix: Refactor Arbitrum_Adapter #214
Conversation
We have noticed in practice that `outboundTransferCustomRefund` seems to be crediting the `to` address with the gas refund rather than the `l2RefundAddress`. While `outboundTransfer` at least refunds the caller's (aliased) address which means funds are recoverable
| @@ -64,10 +54,6 @@ contract Arbitrum_Adapter is AdapterInterface { | |||
| // L2 Gas price bid for immediate L2 execution attempt (queryable via standard eth*gasPrice RPC) | |||
| uint256 public immutable l2GasPrice = 5e9; // 5 gWei | |||
|
|
|||
| // Gas limit for immediate L2 execution attempt (can be estimated via NodeInterface.estimateRetryableTicket). | |||
| // NodeInterface precompile interface exists at L2 address 0x00000000000000000000000000000000000000C8 | |||
| uint32 public immutable l2GasLimit = 2_000_000; | |||
There was a problem hiding this comment.
Gas limit for sending tokens should be a lot lower than for sending messages
There was a problem hiding this comment.
nit: any reason not to make these both constants rather than hardcoding them?
There was a problem hiding this comment.
you mean instead of uint256 public immutable do uint256 public constant? Is that a gas savings?
There was a problem hiding this comment.
Sorry, yeah, that too (constant is better than immutable!), but I just meant giving them names, which you did!
| @@ -64,10 +54,6 @@ contract Arbitrum_Adapter is AdapterInterface { | |||
| // L2 Gas price bid for immediate L2 execution attempt (queryable via standard eth*gasPrice RPC) | |||
| uint256 public immutable l2GasPrice = 5e9; // 5 gWei | |||
|
|
|||
| // Gas limit for immediate L2 execution attempt (can be estimated via NodeInterface.estimateRetryableTicket). | |||
| // NodeInterface precompile interface exists at L2 address 0x00000000000000000000000000000000000000C8 | |||
| uint32 public immutable l2GasLimit = 2_000_000; | |||
There was a problem hiding this comment.
nit: any reason not to make these both constants rather than hardcoding them?
| // refund address. This means that the excess ETH to pay for the L2 transaction will be sent to the aliased | ||
| // contract address on L2, which we'd have to retrieve via a custom adapter | ||
| // (i.e. the Arbitrum_RescueAdapter). | ||
| l1ERC20GatewayRouter.outboundTransfer{ value: requiredL1CallValue }( |
There was a problem hiding this comment.
Just double checking -- we have a confirmed DAI transfer where we see the refund working correctly (i.e. going to the hub pool alias address rather than the spoke pool)?
There was a problem hiding this comment.
yes i confirmed it by checking the hub pool alias ETH balance the blockbefore and after the DAI transfer
| data | ||
| ); | ||
| } | ||
| // Excess funds will be sent to this contract's aliased arbitrum address. |
There was a problem hiding this comment.
Is this logic identical to the logic that was here in the initial version of this adapter? I want to make sure we're really tight on what's being changed in these adapters since this code would be running in the HubPool context.
There was a problem hiding this comment.
yep exactly the same
There was a problem hiding this comment.
ive checked against on-chain verified contract code here
There was a problem hiding this comment.
LGTM! As with the prev comment: #214 (review), I think we should wait to hear back before moving forward with any modifications.
| @@ -64,10 +54,6 @@ contract Arbitrum_Adapter is AdapterInterface { | |||
| // L2 Gas price bid for immediate L2 execution attempt (queryable via standard eth*gasPrice RPC) | |||
| uint256 public immutable l2GasPrice = 5e9; // 5 gWei | |||
|
|
|||
| // Gas limit for immediate L2 execution attempt (can be estimated via NodeInterface.estimateRetryableTicket). | |||
| // NodeInterface precompile interface exists at L2 address 0x00000000000000000000000000000000000000C8 | |||
| uint32 public immutable l2GasLimit = 2_000_000; | |||
There was a problem hiding this comment.
Sorry, yeah, that too (constant is better than immutable!), but I just meant giving them names, which you did!
nicholaspai
changed the title
We realized that we haven't updated the arbitrum adapter to use
outboundTransferCustomRefundso this cleans up the adapter before updating it