feat: a custom AdminRelayTokensAdapter that allows admin to call relayTokens via an admin action
#1072
Conversation
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
| // We are ok with this low-level call since the adapter address is set by the admin and we've | ||
| // already checked that its not the zero address. | ||
| // solhint-disable-next-line avoid-low-level-calls | ||
| (bool success, ) = targetAdapter.delegatecall( |
There was a problem hiding this comment.
This method is payable, should we revert if there is any msg.value?
There was a problem hiding this comment.
I don't think you can attach any value with a delegatecall, and that's the only way the HubPool should be able to call this contract, so I don't think there's any risk in not checking. What do you think?
There was a problem hiding this comment.
Honestly, I'll just add it
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
| * @param message Abi-encoded arguments params for the `relayTokens` function call: (address l1Token, | ||
| * address l2Token, uint256 amount, address spokePool) | ||
| */ | ||
| function relayMessage(address target, bytes calldata message) external payable { |
There was a problem hiding this comment.
does this function need some kind of guard to check if caller is admin?
There was a problem hiding this comment.
It doesn't because this function is being delegatecalled into, basically acting as a library that HubPool will use.
Without HubPools state (e.g. token balances) this function call is not meaningful.
Basically the following flow will happen:
- admin calls HubPool.relaySpokePoolAdminFunction
- (now we're executing in the context of HubPool) HubPool delegatecalls
AdminRelayTokensAdapter.relayMessage - (still in the context of HubPool because of delegatecall) HubPool* delegatecalls
UNDERLYING_ADAPTER.relayTokens-- this step is exactly the same as what HubPool does when sending tokens to Spokes normally during a Rebalance leaf execution (ifUNDERLYING_ADAPTERis, say,Arbitrum_Adapter)
Signed-off-by: Ihor Farion <[email protected]>
| // `deletagecall`ed into, and delegatecall does not allow for attaching the `value` | ||
| require(msg.value == 0, "value transfer not supported"); | ||
|
|
||
| (address l1Token, address l2Token, uint256 amount, address spokePool) = abi.decode( |
There was a problem hiding this comment.
Why encode the spokePool address at all? Why not just use the target?
There was a problem hiding this comment.
My thinking is, this adapter is going to be set as an ad-hoc adapter for non-existing chainId, so by default the spokePool for that is just zero address, but we could e.g. do:
- setCrossChainContracts(thisadapter, arbitrum-spoke-addr)
and then HubPool would send in the arbitrum-spoke-addr as target, just felt easier to pass in as part of encoded arguments
There was a problem hiding this comment.
If we used it that way, wouldn't the check that requires target == spokePool below fail?
| function relayMessage(address target, bytes calldata message) external payable { | ||
| // This adapter is not intended to accept any value as a part of `relayMessage`. It's intended usecase is to be | ||
| // `deletagecall`ed into, and delegatecall does not allow for attaching the `value` | ||
| require(msg.value == 0, "value transfer not supported"); |
There was a problem hiding this comment.
I'm a nit and I don't like require statements. To me, if we are going to make this contract (and hopefully not modify it ever), we might as well use the ever-so-slightly more gas-optimal custom errors.
On another note, we probably don't need this check either way. None of our other adapters check for empty msg.value since (as far as I know) nothing happens when you attach msg.value to these txns (except the eth just goes to the hub pool). Also, letting this be payable might be useful if we do want to check that this adapter works for e.g. the arbitrum or zksync adapters since we can call this adapter via an EOA and attach value to the txn.
There was a problem hiding this comment.
Will add custom errors.
I agree on the msg.value, mentioned it here too
@nicholaspai I think we should be able to skip this msg.value == 0 check as Bennett mentions above
Closes ACX-4313