feat(linea): add adapter and spokepool contracts #382

dohaki · 2023-12-18T17:30:24Z

Closes ACX-1759, ACX-1760 and ACX-1768

Test Contracts

Contract	Address	Chain
`Linea_Adapter`	0x84e14730cf11d7e2a0ac448b8a88d8e02ef296fd	5
`TokensRelayer`	0x33e76207af8AeAc2072213C9Bc33eB844b0e26e4	5
`Linea_SpokePool`	0x9FC7700c4Fd1e689f0c9cd25a1BC9B82328fe0C5	59140
`Linea_MockSpokePool`	0x24fE232B87E6bba1865ddAD8AF4faA2C8B5323B4	59140

L1->L2

Method	Goerli	Linea Goerli
`relayMessage` - via `setDepositRoute`	`setDepositRoute` tx	`claimMessage` tx
`relayTokens` - WETH	`relayTokens` tx	`claimMessage` tx
`relayTokens` - USDC	`relayTokens` tx	`claimMessage` tx
`relayTokens` - USDT	`relayTokens` tx	`claimMessage` tx

L2->L1

Method	Linea Goerli	Goerli
`_bridgeTokensToHubPool` - WETH	`bridgeTokensToHubPool` tx	`claimMessage` tx
`_bridgeTokensToHubPool` - USDC	`bridgeTokensToHubPool` tx	`claimMessage` tx
`_bridgeTokensToHubPool` - USDT	`bridgeTokensToHubPool` tx	`claimMessage` tx

linear · 2023-12-18T17:30:28Z

ACX-1759 Implement Linea_Adapter.sol

mrice32

Looks great! A few comments

mrice32 · 2023-12-21T04:18:08Z

contracts/chain-adapters/Linea_Adapter.sol

+     * @param message Data to send to target.
+     */
+    function relayMessage(address target, bytes calldata message) external payable override {
+        l1MessageService.sendMessage{ value: msg.value }(target, 0, message);


Typically, we don't require the dataworker to send in the ETH on every execution. This has a few problems:

Hard to compute this offchain.

Can't use the in-contract multicall function because it doesn't allow msg.value and all the value would be used up in the first call.

The way we typically do this is shown in the arbitrum adapter: https://github.com/across-protocol/contracts-v2/blob/866d09fcae4c3257c9759fd150c2b8c6f10034d7/contracts/chain-adapters/Arbitrum_Adapter.sol#L187-L190. We pre-load the dataworker with ETH regularly and compute the value needed in relayMessage in the contract.

Note: remember this code is run via delegatecall, so it's as if it's running within the HubPool contract. msg.value, msg.sender, etc are all the same as they are when code is executing within the HubPool.

Got it! Yes, because we need to finalize the relayed messages on Linea ourselves anyways, we are not required to send any value. So in our case, msg.value would be 0. Should I still remove the forwarding of msg.value?

Yes I think we should remove any msg.value and just use any ETH held in the HubPool account if we need any. If we don't need any, then just set to 0?

Agreed. Remember that these adapters are easy to replace if the requirements change.

Bump on this, we should drop the msg.value here

contracts/chain-adapters/Linea_Adapter.sol

mrice32 · 2023-12-21T04:21:45Z

contracts/chain-adapters/Linea_Adapter.sol

@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: BUSL-1.1


I would suggest adding a few simple tests, like we have for the other adapters.

done here 32d72c0

socket-security · 2023-12-26T18:16:33Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@consensys/linea-sdk	0.1.6	None	`+27`	11.6 MB	victorien-gauch

linear · 2023-12-26T18:18:20Z

ACX-1760 Implement Linea_SpokePool.sol

ACX-1768 Implement l1ToL2Finalizer helper script

contracts/Linea_SpokePool.sol

nicholaspai · 2023-12-27T13:18:18Z

contracts/chain-adapters/Linea_Adapter.sol

+     * @param message Data to send to target.
+     */
+    function relayMessage(address target, bytes calldata message) external payable override {
+        l1MessageService.sendMessage{ value: msg.value }(target, 0, message);


If we are required to include a msg.value to pay for the message then you should grab eth from the hub pools balance like in Arbitrum. This way the caller isn't forced to add msg.value

Because Linea (currently at least) does not support auto-claiming of messages that contain calldata, we need to finalize messages ourselves. Which means we can omit fees for now. I will set the msg.value to 0

ok and in the future if we want to omit fees we should still grab balance from address(this).balance, so that the caller doesn't have to send value on each txn

nicholaspai · 2023-12-27T13:18:57Z

contracts/chain-adapters/Linea_Adapter.sol

+        // via the Canoncial Message Service.
+        if (l1Token == address(l1Weth)) {
+            l1Weth.withdraw(amount);
+            l1MessageService.sendMessage{ value: amount }(to, 0, "");


We only have to pay amount? No extra fees to send a message?

Because Linea (currently at least) does not support auto-claiming messages containing calldata, we need to finalize most messages ourselves. In this case, though, we could set a fee because the calldata is empty. But as we need to finalize all other messages anyways, I am wondering if it makes sense to do it or not 🤔

i think its ideal if we can get someone else to finalize this for us if we can set the fee correctly

contracts/Linea_SpokePool.sol

nicholaspai · 2023-12-27T16:25:28Z

contracts/Linea_SpokePool.sol

+        // send ETH directly via the Canonical Message Service.
+        if (l2TokenAddress == address(wrappedNativeToken)) {
+            WETH9Interface(l2TokenAddress).withdraw(amountToReturn); // Unwrap into ETH.
+            IMessageService(l2MessageService).sendMessage{ value: amountToReturn }(hubPool, 0, "");


Are there no fees that have to be paid here?

Yea, auto-claiming for L2->L1 seems not to be supported yet. So again we need to do that ourselves.

nicholaspai · 2023-12-27T16:26:46Z

contracts/Linea_SpokePool.sol

+        // If the l1Token is USDC, then we need sent it via the USDC Bridge.
+        else if (l2TokenAddress == l2UsdcBridge.usdc()) {
+            IERC20(l2TokenAddress).safeIncreaseAllowance(address(l2UsdcBridge), amountToReturn);
+            l2UsdcBridge.depositTo(amountToReturn, hubPool);


This function is payable, does it need to include any fees?

nicholaspai · 2023-12-27T16:26:53Z

contracts/Linea_SpokePool.sol

+        // For other tokens, we can use the Canonical Token Bridge.
+        else {
+            IERC20(l2TokenAddress).safeIncreaseAllowance(address(l2TokenBridge), amountToReturn);
+            l2TokenBridge.bridgeToken(l2TokenAddress, amountToReturn, hubPool);


Same questions as https://github.com/across-protocol/contracts-v2/pull/382/files#r1437139413

nicholaspai · 2023-12-27T16:27:16Z

contracts/Linea_SpokePool.sol

+    }
+
+    function _requireAdminSender() internal view override {
+        require(IMessageService(l2MessageService).sender() == crossDomainAdmin, "ONLY_COUNTERPART_GATEWAY");


Have you tested that no one else can call a function protected by this internal func?

crossDomainAdmin is set to the HubPool address, right?

Have you tested that no one else can call a function protected by this internal func?

Yes, I tried to call a few functions with an EOA for example.

crossDomainAdmin is set to the HubPool address, right?

Yes, it is set to the address of the HubPool

Yes, I tried to call a few functions with an EOA for example.

Did you also test calling over the L1->L2 bridge from an address that isn't the HubPool?

Did you also test calling over the L1->L2 bridge from an address that isn't the HubPool?

yes I also tested that

nicholaspai · 2023-12-27T16:28:11Z

contracts/chain-adapters/Linea_Adapter.sol

+        // If the l1Token is USDC, then we need sent it via the USDC Bridge.
+        else if (l1Token == l1UsdcBridge.usdc()) {
+            IERC20(l1Token).safeIncreaseAllowance(address(l1UsdcBridge), amount);
+            l1UsdcBridge.depositTo(amount, to);


Do we need to include any fees here?

nicholaspai · 2023-12-27T16:28:18Z

contracts/chain-adapters/Linea_Adapter.sol

+        // For other tokens, we can use the Canonical Token Bridge.
+        else {
+            IERC20(l1Token).safeIncreaseAllowance(address(l1TokenBridge), amount);
+            l1TokenBridge.bridgeToken(l1Token, amount, to);


Same question as https://github.com/across-protocol/contracts-v2/pull/382/files#r1437140098

nicholaspai · 2023-12-27T16:33:09Z

scripts/claimLineaL2Message.ts

+
+    const uniqueTxHashes = new Set(events.map((event) => event.transactionHash));
+    for (const txHash of uniqueTxHashes) {
+      const messageSentEvents = await l1MessageService.getMessagesByTransactionHash(txHash);


Does this handle well sending multiple messages from the same txHash? In the finalizer we've sometimes had issues using the native SDK for a chain to handle the multiple message case so we implemented a helper function getUniqueLogIndex here in use in the Arbitrum finalizer to handle this case

Good question 🤔 I will take a look at the source code

I looked into the code and the implementation looks like

async getMessagesByTransactionHash(transactionHash) { const receipt = await this.provider.getTransactionReceipt(transactionHash); if (!receipt) { return null; } return receipt.logs .filter((log) => log.address === this.contractAddress && log.topics[0] === constants_1.MESSAGE_SENT_EVENT_SIGNATURE) .map((log) => this.contract.interface.parseLog(log)) .map(mappers_1.mapMessageSentEventOrLogToMessage); }

So it should correctly handle multiple MessageSent event logs in a single tx

mrice32

Looks great! Only a few minor comments/questions

contracts/Linea_SpokePool.sol

mrice32 · 2023-12-27T21:32:15Z

contracts/Linea_SpokePool.sol

+    }
+
+    function _requireAdminSender() internal view override {
+        require(IMessageService(l2MessageService).sender() == crossDomainAdmin, "ONLY_COUNTERPART_GATEWAY");


crossDomainAdmin is set to the HubPool address, right?

mrice32 · 2023-12-27T21:33:16Z

contracts/chain-adapters/Linea_Adapter.sol

+     * @param message Data to send to target.
+     */
+    function relayMessage(address target, bytes calldata message) external payable override {
+        l1MessageService.sendMessage{ value: msg.value }(target, 0, message);


Agreed. Remember that these adapters are easy to replace if the requirements change.

mrice32

Looks great! Only minor comments

mrice32 · 2023-12-28T16:57:08Z

contracts/Linea_SpokePool.sol

+    }
+
+    function _requireAdminSender() internal view override {
+        require(IMessageService(l2MessageService).sender() == crossDomainAdmin, "ONLY_COUNTERPART_GATEWAY");


Yes, I tried to call a few functions with an EOA for example.

Did you also test calling over the L1->L2 bridge from an address that isn't the HubPool?

mrice32 · 2023-12-28T16:59:02Z

contracts/chain-adapters/Linea_Adapter.sol

+     * @param message Data to send to target.
+     */
+    function relayMessage(address target, bytes calldata message) external payable override {
+        l1MessageService.sendMessage{ value: msg.value }(target, 0, message);


Bump on this, we should drop the msg.value here

nicholaspai

Some suggestions on reducing the fee we pay on L2 and making it easier for dataworker to know what to pay

nicholaspai · 2024-01-04T13:11:07Z

contracts/Linea_SpokePool.sol

+    function _bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) internal override {
+        // Linea's L2 Canonical Message Service, requires a minimum fee to be set.
+        uint256 minFee = IMessageService(l2MessageService).minimumFeeInWei();
+        require(msg.value >= minFee, "MESSAGE_FEE_TOO_LOW");


Should we require msg.value is exactly the min fee so the caller doesn't burn extra fees?

Ok yea, that makes sense.

Changed here a29b3a0

nicholaspai · 2024-01-04T13:11:43Z

contracts/Linea_SpokePool.sol

+
+    function _bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) internal override {
+        // Linea's L2 Canonical Message Service, requires a minimum fee to be set.
+        uint256 minFee = IMessageService(l2MessageService).minimumFeeInWei();


Can we add an external method that returns the minder so dataworker can more easily know how much value it should send on each call?

Sure will do!

Added here 916f600. New deployment can be tested here

nicholaspai · 2024-01-04T15:16:09Z

contracts/Linea_SpokePool.sol

+
+    function _bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) internal override {
+        // Linea's L2 Canonical Message Service, requires a minimum fee to be set.
+        uint256 minFee = IMessageService(l2MessageService).minimumFeeInWei();


nit: should we use the public minimumFeeInwei function now?

Changed here d76364c

nicholaspai · 2024-01-05T19:27:01Z

contracts/Linea_SpokePool.sol

+contract Linea_SpokePool is SpokePool {
+    using SafeERC20 for IERC20;
+
+    IMessageService public l2MessageService;


Need comments above public vars

nicholaspai · 2024-01-05T19:27:08Z

contracts/Linea_SpokePool.sol

+    event SetL2UsdcBridge(address indexed newUsdcBridge, address oldUsdcBridge);
+
+    /// @custom:oz-upgrades-unsafe-allow constructor
+    constructor(


Need Natspec on public functions

nicholaspai · 2024-01-05T19:28:13Z

contracts/Linea_SpokePool.sol

+    ITokenBridge public l2TokenBridge;
+    IUSDCBridge public l2UsdcBridge;
+
+    event LineaTokensBridged(address indexed l2Token, address target, uint256 numberOfTokensBridged);


Please add an EVENTS section, or just follow all the commenting rules James implemented in https://github.com/across-protocol/contracts-v2/pull/381/files

nicholaspai · 2024-01-05T19:29:17Z

contracts/Linea_SpokePool.sol

+    function _bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) internal override {
+        // Linea's L2 Canonical Message Service, requires a minimum fee to be set.
+        uint256 minFee = minimumFeeInWei();
+        require(msg.value == minFee, "MESSAGE_FEE_MISMATCH");


Can you add a comment here about why we require that the caller pass in msg.value versus pulling ETH out of this contract's balance which would require a separate accounting system to keep LP funds separated from system funds used to pay for L2 to L1 messages?

nicholaspai · 2024-01-05T19:29:52Z

contracts/SpokePool.sol

        USSSpokePoolInterface.USSRelayerRefundLeaf calldata relayerRefundLeaf,
        bytes32[] calldata proof
-    ) public virtual override nonReentrant {
+    ) public payable virtual override nonReentrant {


So this doesn;'t compile if we leave this change out?

yea exactly

nicholaspai

This mostly looks good to go but needs more comments

feat: add Linea_Adapter.sol and goerli deployment

516aa48

dohaki requested review from mrice32, nicholaspai and pxrl December 18, 2023 17:30

mrice32 reviewed Dec 21, 2023

View reviewed changes

mrice32 mentioned this pull request Dec 21, 2023

feat(scroll): adapter and spokepool #381

Merged

dohaki added 7 commits December 22, 2023 11:16

chore: redeploy Linear_Adapter

4ae32a5

improve: remove explicit usdc arg

c49aa88

Merge branch 'master' into dong-ha/acx-1759-implement-linea_adapter

263e0ab

feat: add Linea_SpokePool contract and linea-goerli deployment

b6812a5

test: add unit tests for linea adapter and spoke

32d72c0

fix: add linea to hh config

571c4d2

feat: helper script for claiming linea messages

f58bdf5

dohaki changed the title ~~feat: add Linea_Adapter.sol and goerli deployment~~ feat(linea): add adapter and spokepool contracts Dec 26, 2023

dohaki requested a review from mrice32 December 27, 2023 08:56

nicholaspai reviewed Dec 27, 2023

View reviewed changes

contracts/Linea_SpokePool.sol Show resolved Hide resolved

nicholaspai reviewed Dec 27, 2023

View reviewed changes

contracts/Linea_SpokePool.sol Show resolved Hide resolved

nicholaspai reviewed Dec 27, 2023

View reviewed changes

mrice32 approved these changes Dec 27, 2023

View reviewed changes

mrice32 approved these changes Dec 28, 2023

View reviewed changes

dohaki added 3 commits January 2, 2024 16:17

fix: review requests and l2->l2 fix

9aacf83

fix: make _bridgeTokensToHub payable

1a9754d

chore: new linea spoke pool deployment

bbf2953

dohaki requested a review from nicholaspai January 3, 2024 18:36

dohaki added 3 commits January 4, 2024 07:57

Merge branch 'master' into dong-ha/acx-1759-implement-linea_adapter

67535c3

fixup

f5d0212

fix: polygon spokepool compilation

69e1384

nicholaspai reviewed Jan 4, 2024

View reviewed changes

dohaki added 2 commits January 4, 2024 15:22

improve: require exact min. fee

a29b3a0

feat: add view function for min fee

916f600

dohaki requested a review from nicholaspai January 4, 2024 14:40

nicholaspai reviewed Jan 4, 2024

View reviewed changes

improve: use public minimumFeeInWei functions

d76364c

dohaki requested a review from nicholaspai January 4, 2024 15:34

nicholaspai reviewed Jan 5, 2024

View reviewed changes

nicholaspai approved these changes Jan 5, 2024

View reviewed changes

dohaki added 4 commits January 5, 2024 22:08

chore: add more comments

86adcd8

Merge branch 'master' into dong-ha/acx-1759-implement-linea_adapter

0b5bfd6

chore: use correct number prefix for linea deploy scripts

b1378e9

fixup

6399a66

dohaki merged commit 339bafa into master Jan 7, 2024

dohaki deleted the dong-ha/acx-1759-implement-linea_adapter branch January 7, 2024 11:20

feat(linea): add adapter and spokepool contracts #382

feat(linea): add adapter and spokepool contracts #382

Uh oh!

Conversation

dohaki commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Contracts

L1->L2

L2->L1

Uh oh!

linear bot commented Dec 18, 2023

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socket-security bot commented Dec 26, 2023

Uh oh!

linear bot commented Dec 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dohaki commented Dec 18, 2023 •

edited

Loading

mrice32 left a comment •

edited

Loading