Skip to content

Conversation

bmzig
Copy link
Contributor

@bmzig bmzig commented Sep 27, 2024

This implements the newer version of ForwarderBase and does not make any refactors to the adapters, since the forwarders now assume that the appropriate adapters have been deployed to LX.

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]>
address target
) external payable override onlyAdmin {
address remoteToken = remoteTokens[target][currentLayerToken] == address(0)
? arbitrumGatewayRouter.calculateL2TokenAddress(currentLayerToken)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gateway router should return the correct address if the token in question is actually routed through an Arbitrum gateway. The most notable exception we know of now is USDC and CCTP, which is an example of when we would instead need to set remoteTokens. I think if we go with this, we will need to be careful and ensure that the remote token we are attempting to bridge to is indeed created through a bridgeMint call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would cctp work? cctp from L1 to L2, and then canonical bridge from there? can we support that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, there was this comment #610 (comment) on the previous PR which inherited the adapter code, and the idea was that there may be a CCTP bridge from L2-L3 which bridges native to native. This should work fine if we use CCTP for L1 -> L2, have a native on L2, and then bridge native on L2 -> bridged on L3 via the canonical bridge, since that uses an Arbitrum gateway. TLDR CCTP is just speculation. It's mainly an example if there was another non-arbitrum bridge from L2-L3, and also calculateL2TokenAddress should return the remote token which is minted on L2 given the input L1 token.

@@ -54,7 +55,7 @@ contract Arbitrum_SpokePool is SpokePool, CircleCCTPAdapter {
}

modifier onlyFromCrossDomainAdmin() {
require(msg.sender == _applyL1ToL2Alias(crossDomainAdmin), "ONLY_COUNTERPART_GATEWAY");
require(msg.sender == AddressUtils._applyL1ToL2Alias(crossDomainAdmin), "ONLY_COUNTERPART_GATEWAY");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this library should be called CrossDomainAddressUtils as AddressUtils overlaps with the openzeppelin utility file that has utilities for any arbitrary EVM. This aliasing is very OVM/AVM specific

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know its in draft mode so just left some comments upon first pass

@bmzig bmzig marked this pull request as ready for review October 1, 2024 17:50
Base automatically changed from bz/l2ForwarderInterface to master October 2, 2024 13:24
// L1 addresses are transformed during l1->l2 calls.
// This cannot be pulled directly from Arbitrum contracts because their contracts are not 0.8.X compatible and
// this operation takes advantage of overflows, whose behavior changed in 0.8.0.
function _applyL1ToL2Alias(address l1Address) internal pure returns (address l2Address) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I prefer not having an underscore prefix on library functions even if they are internal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here dfee0a1

Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmzig bmzig requested review from nicholaspai and pxrl October 2, 2024 18:40
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

// L1 addresses are transformed during l1->l2 calls.
// See https://github.com/matter-labs/era-contracts/blob/main/docs/Overview.md#mailboxfacet for more information.
// Another source: https://github.com/matter-labs/era-contracts/blob/41c25aa16d182f757c3fed1463c78a81896f65e6/ethereum/contracts/vendor/AddressAliasHelper.sol#L28
function _applyL1ToL2Alias(address l1Address) internal pure returns (address l2Address) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to triple check, arbitrum and zksync do this the exact same way, right? Code looks the same, but just want to sanity check that I read this right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. (Just to enumerate) Arbitrum and ZkSync both have the same aliasing method, and all other chains query the messenger contract.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect.

@bmzig bmzig merged commit 3d61c1b into master Oct 3, 2024
9 checks passed
@bmzig bmzig deleted the bz/customForwarders branch October 3, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants