feat(spoke-pool): Distribute relayer refund #24

nicholaspai · 2022-02-07T21:24:29Z

No description provided.

mrice32

Looks great! A few minor comments.

mrice32 · 2022-02-08T06:52:24Z

contracts/Optimism_SpokePool.sol

+            IL2ERC20Bridge(Lib_PredeployAddresses.L2_STANDARD_BRIDGE).withdrawTo(
+                distributionLeaf.l2TokenAddress, // _l2Token. Address of the L2 token to bridge over.
+                hubPool, // _to. Withdraw, over the bridge, to the l1 pool contract.
+                distributionLeaf.amountToReturn, // _amount. Send the full balance of the deposit box to bridge.
+                6_000_000, // _l1Gas. Unused, but included for potential forward compatibility considerations
+                "" // _data. We don't need to send any data for the bridging action.
+            );


nit: should we just put this bit of special logic in a virtual internal method that's called by the base class? That way the more generic logic could all live in the base class and this dispatch bit would be the only part that's custom per chain?

Great idea, done: 833d320

mrice32 · 2022-02-08T06:54:44Z

contracts/SpokePool.sol

+        // TODO: Should we be checking that distributionLeaf.refundAddresses.length == distributionLeaf.refundAmounts.length
+        // or can we assume that the `initiateRelayerRefund` on the HubPool would have been disputed on L1?


As long as it's fairly cheap to check (which I think it is), I think this check makes sense to avoid any unexpected behavior below.

mrice32 · 2022-02-08T06:55:32Z

contracts/SpokePool.sol

+
+        // TODO: Similar question to above: should we assume that the L2 token is not address(0) or any other address
+        // that would have been disputed on L1?
+        require(distributionLeaf.l2TokenAddress != address(0), "invalid leaf");


I wouldn't worry about this unless there's something particularly bad that would happen if it were address 0.

Yeah this would revert anyways if l2 token was address 0, and also should be disputed on L1

mrice32 · 2022-02-08T06:57:27Z

contracts/Optimism_SpokePool.sol

-        require(newCrossDomainAdmin != address(0), "Bad bridge router address");
-        crossDomainAdmin = newCrossDomainAdmin;
-        emit SetXDomainAdmin(crossDomainAdmin);
+        emit TokensBridged(


Should we only emit this if we actually bridged tokens?

yup! This is now implemented here 833d320

_bridgeTokensToHubPool is only called if amountToReturn > 0, and that method should emit an event for each L2-specific implementation, like it is in the Optimism_SpokePool demo code

chrismaree · 2022-02-08T19:10:01Z

contracts/Optimism_SpokePool.sol

-        require(newCrossDomainAdmin != address(0), "Bad bridge router address");
-        crossDomainAdmin = newCrossDomainAdmin;
-        emit SetXDomainAdmin(crossDomainAdmin);
+    function _bridgeTokensToHubPool(MerkleLib.DestinationDistribution memory distributionLeaf) internal override {


this will need branching logic for WETH. can do it in a future PR.

added a TODO

chrismaree · 2022-02-08T19:10:41Z

contracts/Optimism_SpokePool.sol

+            distributionLeaf.l2TokenAddress,
+            hubPool,
+            distributionLeaf.amountToReturn,
+            6_000_000,


I think we should set this in a variable and then let the owner change it. does not need to happen in this PR though but should probs do it.

I can change in this PR. what's annoying is that this is potentially Optimism specific, but HubPool will need to implement a method that can trigger the cross chain admin function to make this call. Shold we just implement setL2GasLimit in the SpokePool and have it be useless for some L2s?

And consider other constants such as maxSubmissionCost and l2GasPrice (and refundAddress) that will be Arbitrum specific?

Perhaps in the HubPool we can allow it to call any arbitrary encoded method to the SpokePool so it can call L2-specific admin functions like setL2GasLimit for Optimism and setL2GasPrice for Arbitrum

ye, so I think that what makes the most sence is to have this be a chain specific event, like you mention. then, each SpokePool specific implementation for a given chain has these extra methods on them for those chain specific things.

Cool added a universal event emitted by SpokePool and added an Optimism specific OptimismTokenBridged event. Also added a setL1Gas onlyOwner method on the OptimismSpokePool

chrismaree · 2022-02-08T19:11:41Z

contracts/SpokePool.sol

+        require(distributionLeaf.chainId == chainId(), "Invalid chainId");
+        require(distributionLeaf.refundAddresses.length == distributionLeaf.refundAmounts.length, "invalid leaf");


I dont think any of these checks are needed. If either of these fail then the proof will fail. it's somewhat redundant, no?

I think its better to be safe in case the bots miss something, we won't get into weird state

merging this comment: #24 (comment)

chrismaree · 2022-02-08T19:12:20Z

contracts/SpokePool.sol

+        // Check that `inclusionProof` proves that `distributionLeaf` is contained within the distribution root.
+        // Note: This should revert if the `distributionRoot` is uninitialized.
+        require(
+            MerkleLib.verifyRelayerDistribution(refund.distributionRoot, distributionLeaf, inclusionProof),


nit: if you re-name the inclusionProof to proof t his might fit on one line and then it's consistant with the HubPool.

chrismaree

This is awesome! I left some nits and comments but once those are sorted we are good to go.

mrice32 · 2022-02-08T21:35:08Z

contracts/Optimism_SpokePool.sol

-    address public override crossDomainAdmin;
-
-    event SetXDomainAdmin(address indexed newAdmin);
+    event TokensBridged(


Do we need this event to be different per chain? Or could we standardize and put in the base class?

I think it should be diff per chain since each chain will use different params to bridge tokens. For example, Arbitrum uses something called a maxSubmissionCost whereas optimsim does not

Gotcha, okay. Could make sense to have a standard event that we use for all contracts, if the specific parameters aren't relevant. Just something to think about later. Not something I think we should change now.

Hmmm ill add a TODO

…art-contracts-v2 into npai/distribute-relayer-refund

mrice32

LGTM!

chrismaree · 2022-02-09T19:00:26Z

contracts/Optimism_SpokePool.sol

-        crossDomainAdmin = newCrossDomainAdmin;
-        emit SetXDomainAdmin(crossDomainAdmin);
+    function _bridgeTokensToHubPool(MerkleLib.DestinationDistribution memory distributionLeaf) internal override {
+        // TODO: Handle WETH token unwrapping


yup! this part you can copy some of the work done in the other contracts, fortunately.

chrismaree · 2022-02-09T19:00:38Z

contracts/Optimism_SpokePool.sol

+        // TODO: Handle WETH token unwrapping
+        IL2ERC20Bridge(Lib_PredeployAddresses.L2_STANDARD_BRIDGE).withdrawTo(
+            distributionLeaf.l2TokenAddress, // _l2Token. Address of the L2 token to bridge over.
+            hubPool, // _to. Withdraw, over the bridge, to the l1 pool contract.


this might need to change in the context of weth.

contracts/SpokePool.sol

…ethod to OptimismSpokePool

Co-authored-by: Chris Maree <christopher.maree@gmail.com>

…art-contracts-v2 into npai/distribute-relayer-refund

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

Original fix from Nick. Signed-off-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

nicholaspai added 3 commits February 7, 2022 11:03

IMplement functions

536fb7c

add tests

6a331a1

Update Optimism_SpokePool.ts

1a442c4

nicholaspai requested review from chrismaree and mrice32 February 7, 2022 21:24

mrice32 approved these changes Feb 8, 2022

View reviewed changes

nicholaspai added 2 commits February 8, 2022 09:39

Move bridging L2-->L1 tokens logic to virtual internal method

833d320

Respond to matts comments on require statements

660a0e4

chrismaree reviewed Feb 8, 2022

View reviewed changes

chrismaree approved these changes Feb 8, 2022

View reviewed changes

mrice32 approved these changes Feb 8, 2022

View reviewed changes

nicholaspai requested review from chrismaree and mrice32 February 8, 2022 23:41

mrice32 approved these changes Feb 9, 2022

View reviewed changes

add TODOs

5dfbbe9

nicholaspai requested a review from mrice32 February 9, 2022 04:16

nicholaspai added 2 commits February 8, 2022 20:24

Update SpokePool.sol

e1c5e8c

Merge branch 'master' of https://github.com/across-protocol/across-sm…

4e8a74e

…art-contracts-v2 into npai/distribute-relayer-refund

mrice32 approved these changes Feb 9, 2022

View reviewed changes

chrismaree reviewed Feb 9, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree approved these changes Feb 9, 2022

View reviewed changes

nicholaspai and others added 4 commits February 9, 2022 11:56

Add shared TokensBridged event to SpokePool; add setL1Gas onlyOwner m…

125e08e

…ethod to OptimismSpokePool

Update contracts/SpokePool.sol

d96a275

Co-authored-by: Chris Maree <christopher.maree@gmail.com>

Update SpokePool.sol

eb4ea53

Merge branch 'master' of https://github.com/across-protocol/across-sm…

bde66ba

…art-contracts-v2 into npai/distribute-relayer-refund

nicholaspai merged commit 3a9df25 into master Feb 9, 2022

nicholaspai deleted the npai/distribute-relayer-refund branch February 9, 2022 20:35

nicholaspai restored the npai/distribute-relayer-refund branch February 14, 2022 17:02

pxrl pushed a commit that referenced this pull request Feb 13, 2024

feat: M04 Polygon lock can be bypassed (#24)

739d919

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

pxrl added a commit that referenced this pull request Feb 13, 2024

feat: M04 Polygon lock can be bypassed (#24) (#432)

6d2a7a8

Original fix from Nick. Signed-off-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

pxrl pushed a commit that referenced this pull request Feb 20, 2024

feat: M04 Polygon lock can be bypassed (#24)

d910641

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

		// TODO: Should we be checking that distributionLeaf.refundAddresses.length == distributionLeaf.refundAmounts.length
		// or can we assume that the `initiateRelayerRefund` on the HubPool would have been disputed on L1?

		require(distributionLeaf.chainId == chainId(), "Invalid chainId");
		require(distributionLeaf.refundAddresses.length == distributionLeaf.refundAmounts.length, "invalid leaf");

feat(spoke-pool): Distribute relayer refund #24

feat(spoke-pool): Distribute relayer refund #24

Uh oh!

Conversation

nicholaspai commented Feb 7, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismaree 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants