feat: add Arbitrum forwarder contracts #610

bmzig · 2024-09-17T23:58:57Z

This contains L2 forwarder contracts based on the interface #609, under the assumption that the L2 on which it is deployed is Arbitrum. It mostly follows the same format as the Arbitrum_CustomGasToken_Adapter and Arbitrum_Adapter contracts.

Signed-off-by: bennett <[email protected]>

bmzig · 2024-09-18T14:33:30Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+        address _l3SpokePool,
+        address _crossDomainAdmin
+    )
+        CircleCCTPAdapter(_l2Usdc, _cctpTokenMessenger, CircleDomainIds.UNINITIALIZED)


This is speculation on how CCTP would work on L3s. We may just want to remove this on the forwarders.

I don't see a reason it would work differently on L3s, so it seems reasonable just to leave this here as uninitialized and use it if/when we need it.

Maybe we've talked about this before, but is there a reason we shouldn't allow the domain id to be passed in rather than hardcoded, so it can be enabled without changing the code?

Signed-off-by: bennett <[email protected]>

bmzig · 2024-09-18T19:13:46Z

There are some refactors I need to do to hopefully cut down a lot on the code duplication, but I think this is the general idea.

mrice32

Looks great! A few comments

mrice32 · 2024-09-19T04:58:29Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+        address _l3SpokePool,
+        address _crossDomainAdmin
+    )
+        CircleCCTPAdapter(_l2Usdc, _cctpTokenMessenger, CircleDomainIds.UNINITIALIZED)


I don't see a reason it would work differently on L3s, so it seems reasonable just to leave this here as uninitialized and use it if/when we need it.

Maybe we've talked about this before, but is there a reason we shouldn't allow the domain id to be passed in rather than hardcoded, so it can be enabled without changing the code?

mrice32 · 2024-09-19T04:59:57Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+ */
+
+// solhint-disable-next-line contract-name-camelcase
+contract Arbitrum_CustomGasToken_L2_Forwarder is ArbitrumForwarderInterface, CircleCCTPAdapter {


Is the code in this contract identical to the custom gas arbitrum adapter?

Thoughts on sharing the code via a library or inheritance?

There was a big refactor here ea2b6ca which should share code via inheritance.

mrice32 · 2024-09-19T05:00:29Z

contracts/chain-adapters/Arbitrum_L2_Forwarder.sol

+ */
+
+// solhint-disable-next-line contract-name-camelcase
+contract Arbitrum_L2_Forwarder is ArbitrumForwarderInterface, CircleCCTPAdapter {


Same question here, but for the regular arbitrum adapter

mrice32 · 2024-09-19T05:01:40Z

contracts/chain-adapters/Ovm_CustomGasToken_L2_Forwarder.sol

+ */
+
+// solhint-disable-next-line contract-name-camelcase
+contract Ovm_CustomGasToken_L2_Forwarder is Arbitrum_CustomGasToken_L2_Forwarder {


+1, great job on keeping these super simple

Signed-off-by: bennett <[email protected]>

pxrl · 2024-09-24T12:08:56Z

contracts/chain-adapters/Arbitrum_Adapter.sol

@@ -7,7 +7,8 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import "../external/interfaces/CCTPInterfaces.sol";
 import "../libraries/CircleCCTPAdapter.sol";
-import { ArbitrumInboxLike as ArbitrumL1InboxLike, ArbitrumL1ERC20GatewayLike } from "../interfaces/ArbitrumBridgeInterfaces.sol";


This change was only just introduced back in this (not yer merged) PR: https://github.com/across-protocol/contracts/pull/609/files#diff-9986d5b9b57b18f73b5c629869f6d8a6cfdbfa978964dd76de7f4f37d4b5d561

Is it worth backporting the change to that PR, or is the intermediate step necessary?

pxrl · 2024-09-24T12:25:45Z

contracts/chain-adapters/Arbitrum_AdapterBase.sol

+            IERC20(l1Token).safeIncreaseAllowance(erc20Gateway, amount);
+
+            // `outboundTransfer` expects that the caller includes a bytes message as the last param that includes the
+            // maxSubmissionCost to use when creating an L2 retryable ticket: https://github.com/OffchainLabs/arbitrum/blob/e98d14873dd77513b569771f47b5e05b72402c5e/packages/arb-bridge-peripherals/contracts/tokenbridge/ethereum/gateway/L1GatewayRouter.sol#L232


This seems like a dead link now. I think the correct reference for that contract is: https://github.com/OffchainLabs/arbitrum-classic/blob/551a39b381dcea81e03e7599fcb01fddff4fe96c/packages/arb-bridge-peripherals/contracts/tokenbridge/ethereum/gateway/L1GatewayRouter.sol#L240

We are however referencing their legacy contracts though. They seem to be maintaining compatibility moving forward but I wonder if we should try to reference the current implementations instead.

Nice catch 05dfc6e

pxrl · 2024-09-24T12:36:42Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+    function _applyL1ToL2Alias(address l1Address) internal pure returns (address l2Address) {
+        // Allows overflows as explained above.
+        unchecked {
+            l2Address = address(uint160(l1Address) + uint160(0x1111000000000000000000000000000000001111));
+        }
+    }


We have this in the SpokePool as well. Should we factor it out to an "Arbitrum Utils" library for inlining at build time?

Same comment as paul. If we add it as an internal library function

Refactored: 05dfc6e

pxrl · 2024-09-24T12:41:33Z

contracts/chain-adapters/Ovm_L2_Forwarder.sol

+ */
+
+// solhint-disable-next-line contract-name-camelcase
+contract Ovm_L2_Forwarder is Arbitrum_L2_Forwarder {


I keep getting lost in these naming conventions (i.e. L2 forwarder, l3 adapter, ...) and it gets a bit tricker when chain names are overlaid like this. Is there a concrete naming scheme to be followed? wdyt about building a lexicon that explains how to interpret the names?

I think we should stick with the Adapter{base} naming scheme:

chainId makes it clear which chain the contract should be deployed on, or L1 if omitted

Adapter already is intuitively understood by us as something that sends message to a higher level

Base contracts are meant to be inherited

nicholaspai

I really like the inheritance tree of this PR but I think the names are clunky. Can we think of a better naming scheme? There are too many underscores for one.

Signed-off-by: bennett <[email protected]>

nicholaspai · 2024-09-24T16:14:54Z

contracts/chain-adapters/Arbitrum_Adapter.sol

+            _l1Usdc,
+            _cctpTokenMessenger,
+            _circleDomainId,
+            _l2MaxSubmissionCost,


Note for future work: anytime you make changes to existing contract code where its understood that you're refactoring, you should leave a comment saying something like "I changed this code here"

nicholaspai · 2024-09-24T16:19:36Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+import { Arbitrum_CustomGasToken_AdapterBase, FunderInterface } from "./Arbitrum_CustomGasToken_AdapterBase.sol";
+
+/**
+ * @notice Contract containing logic to send messages from Arbitrum to an AVM L3.


In all contracts, its helpful to note which layer this contract is supposed to be deployed on. In this case, this contract is supposed to be deployed on L2, right?

Yes, it is L2: 05dfc6e

nicholaspai · 2024-09-24T16:20:49Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+ */
+
+// solhint-disable-next-line contract-name-camelcase
+contract Arbitrum_CustomGasToken_L2_Forwarder is Arbitrum_CustomGasToken_AdapterBase, ForwarderBase {


alternative naming option:

Arbitrum_CustomGasToken_L2Adapter

Makes it more clear this behaves like an Adapter, but its on L1. Also I think the forwarder logic maybe is implied if this extends ForwarderBase

nicholaspai · 2024-09-24T16:22:50Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+
+    /**
+     * @notice Constructs new L2 Forwarder.
+     * @dev We normally cannot define a constructor for proxies, but this is an exception since all


I would note here in a @dev note that the cross domain admin will be sit upon initilization of the ForwarderBase, which should be the HubPool

nicholaspai · 2024-09-24T16:23:08Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+/**
+ * @notice Contract containing logic to send messages from Arbitrum to an AVM L3.
+ * @dev This contract is very similar to Arbitrum_CustomGasToken_Adapter. It is meant to bridge
+ * tokens and send messages over a bridge which uses a custom gas token, except this contract assumes


I would also note that this contract is designed to be controlled by a cross domain admin which should be the HubPool

nicholaspai · 2024-09-24T16:23:44Z

contracts/chain-adapters/Arbitrum_CustomGasToken_L2_Forwarder.sol

+        address,
+        uint256 amount
+    ) external payable override {
+        _relayTokens(l2Token, address(0), amount, l3SpokePool);


you should comment why address(0) can be hardcoded here

nicholaspai · 2024-09-24T16:25:00Z

contracts/chain-adapters/Arbitrum_L2_Forwarder.sol

+ */
+
+// solhint-disable-next-line contract-name-camelcase
+contract Arbitrum_L2_Forwarder is ForwarderBase, Arbitrum_AdapterBase {


alternative name could be Arbitrum_L2Adapter for similar reasons to here

nicholaspai · 2024-09-24T16:29:41Z

contracts/chain-adapters/Ovm_CustomGasToken_L2_Forwarder.sol

+ */
+
+// solhint-disable-next-line contract-name-camelcase
+contract Ovm_CustomGasToken_L2_Forwarder is Arbitrum_CustomGasToken_L2_Forwarder {


alternative name would be Ovm_CustomGasToken_L2_Adapter

nicholaspai · 2024-09-24T16:30:37Z

contracts/chain-adapters/Ovm_CustomGasToken_L2_Forwarder.sol

+ * @notice Contract containing logic to send messages from an OVM L2 to an AVM L3.
+ * @dev This contract is very similar to Arbitrum_CustomGasToken_Adapter. It is meant to bridge
+ * tokens and send messages over a bridge which uses a custom gas token, except this contract makes
+ * the assumption that it is deployed on an OpStack L2.


Can you explain a bit more how this differs from Arbitrum_L2 equivalent? Specifically comment on how the onlyFromCrossDomainAdmin-esque function has changed

Signed-off-by: bennett <[email protected]>

bmzig · 2024-09-27T19:07:46Z

Closing in favor of #625 since we no longer wish to inherit specific adapter code, but instead delegatecall external contracts which contain bridging logic.

bmzig added 2 commits September 17, 2024 18:55

feat: add arbitrum forwarder contracts

975ff35

Signed-off-by: bennett <[email protected]>

Merge branch 'bz/l2ForwarderInterface' into bz/ArbitrumForwarders

11d28b1

bmzig commented Sep 18, 2024

View reviewed changes

add ovm forwarders

be13700

Signed-off-by: bennett <[email protected]>

mrice32 reviewed Sep 19, 2024

View reviewed changes

bmzig added 2 commits September 19, 2024 12:56

Merge branch 'bz/l2ForwarderInterface' into bz/ArbitrumForwarders

b733f14

Signed-off-by: bennett <[email protected]>

refactor

5dc9890

Signed-off-by: bennett <[email protected]>

bmzig marked this pull request as ready for review September 20, 2024 16:53

bmzig added 3 commits September 23, 2024 08:53

Merge branch 'bz/l2ForwarderInterface' into bz/ArbitrumForwarders

4a4ebb3

refactor Arbitrum adapter hierarchy

ea2b6ca

Signed-off-by: bennett <[email protected]>

Merge branch 'bz/l2ForwarderInterface' into bz/ArbitrumForwarders

15ff8f5

Signed-off-by: bennett <[email protected]>

pxrl reviewed Sep 24, 2024

View reviewed changes

bmzig marked this pull request as draft September 24, 2024 13:54

nicholaspai reviewed Sep 24, 2024

View reviewed changes

update test with constructor; update comments

4433749

Signed-off-by: bennett <[email protected]>

nicholaspai reviewed Sep 24, 2024

View reviewed changes

bmzig added 3 commits September 24, 2024 13:36

update comments and refactor aliasing

05dfc6e

Signed-off-by: bennett <[email protected]>

Merge branch 'bz/l2ForwarderInterface' into bz/ArbitrumForwarders

7cd46da

update w/ new format. still need to change comments

9cffd55

Signed-off-by: bennett <[email protected]>

bmzig mentioned this pull request Sep 25, 2024

feat: generic L2 forwarder base contract #609

Merged

Merge branch 'bz/l2ForwarderInterface' into bz/ArbitrumForwarders

253310d

factor out adapter interface

8201f86

Signed-off-by: bennett <[email protected]>

bmzig closed this Sep 27, 2024

bmzig mentioned this pull request Sep 27, 2024

feat: forwarders for AVM and OVM networks #625

Merged

feat: add Arbitrum forwarder contracts #610

feat: add Arbitrum forwarder contracts #610

Conversation

bmzig commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmzig commented Sep 18, 2024

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxrl Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmzig commented Sep 27, 2024

pxrl Sep 24, 2024 •

edited

Loading