feat(HubPool): Add initial L1->L2 call plumbing #19

chrismaree · 2022-02-03T00:24:22Z

nit
nit
nit
Update contracts/HubPool.sol
review nit
review nit
feat(hubpool): Refactor hub pool to use 1D bitmap integer
nit
WIP
review nit
review nit
nit
nit
nit
nit
nit
nit
WIP
nit
nit

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree · 2022-02-03T00:35:47Z

contracts/HubPool.sol

+        _sendTokensToTargetChain(poolRebalance, refundRequestId);
+        _executeRelayerRefundOnTargetChain(poolRebalance, refundRequestId);


all the logic of sending tokens to the chains and sending the L1->L2 calls is wrapped into these two functions :)

On L2, what happens if the relayer refund method on the spoke pool is executed before tokens arrive there?

it'll revert as there is not enough money to pay everyone.

Let me be sure I have the right understanding here:

This will just send over a verified root that will be saved on the L2. No funds get automatically distributed.

If funds haven't arrived and someone tried to distribute on the L2, their transaction will revert.

Is my understanding correct? In no situation should the L2 side of the _executeRelayerRefundOnTargetChain transaction revert, right?

this is exactly right @mrice32

chrismaree · 2022-02-03T00:36:09Z

contracts/HubPool.sol

+        if (refundRequest.unclaimedPoolRebalanceLeafCount == 0)
            bondToken.safeTransfer(refundRequest.proposer, bondAmount);
-        }


it's better to only pay the relayer back their bond one all bundles are executed.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nicholaspai · 2022-02-03T17:44:54Z

contracts/HubPool.sol

+        AdapterInterface adapter = crossChainContracts[poolRebalance.chainId].adapter;
+        adapter.relayMessage(
+            address(adapter),
+            abi.encodeWithSignature("initializeRelayerRefund(address)", refundRequest.destinationDistributionRoot)


This should be "bytes32" not "address" right?

it should! fixed.

contracts/HubPool.sol

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree · 2022-02-03T23:45:26Z

contracts/SpokePool.sol

        emit FilledRelay(relayHash, relayFills[relayHash], repaymentChain, amountToSend, msg.sender, relayData);
    }

-    function initializeRelayerRefund(bytes32 relayerRepaymentDistributionProof) public {}


nit to give me the required interface. @nicholaspai 's PR implements this actual logic.

chrismaree · 2022-02-03T23:48:43Z

test/HubPool.RefundExecution.ts

@@ -0,0 +1,81 @@
+import { expect } from "chai";


I have split the HubPool rebalance unit tests into 3 separate files: Initialization,Execution and Dispute.

chrismaree · 2022-02-03T23:49:24Z

test/HubPool.RefundExecution.ts

+    await hubPool.connect(liquidityProvider).addLiquidity(dai.address, consts.amountToLp.mul(10));
+  });
+
+  it("Execute relayer refund correctly produces the refund bundle call and sends cross-chain repayment actions", async function () {


this test covers the main logic added in this PR. please read through it to see how the leafs are created, tree constructed, refund initiated and finally executed.

chrismaree · 2022-02-04T00:07:10Z

test/MerkleLib.utils.ts

+const { defaultAbiCoder, keccak256 } = ethers.utils;
+import { BigNumber, Signer, Contract } from "ethers";
+
+export interface PoolRebalance {


these interfaces were moved to here to from test/MerkleLib.Proofs.ts.

chrismaree · 2022-02-04T00:07:52Z

test/utils.ts

+  return ethers.utils.getAddress(ethers.utils.hexlify(ethers.utils.randomBytes(20)));
+}
+
+export async function getParamType(contractName: string, functionName: string, paramName: string) {


new function to return the types of a function's parameters. used when building Merkle trees.

chrismaree · 2022-02-04T00:08:05Z

test/utils.ts


 export function randomAddress() {
-  return ethers.utils.hexlify(ethers.utils.randomBytes(20));
+  return ethers.utils.getAddress(ethers.utils.hexlify(ethers.utils.randomBytes(20)));


using the .getAddress call checksum cases the address.

chrismaree · 2022-02-04T00:08:28Z

test/HubPool.RefundInitilization.ts

@@ -0,0 +1,66 @@
+import { expect } from "chai";


nothing changed here, just moved and cut down on redundant imports/method calls.

chrismaree · 2022-02-04T00:08:54Z

test/HubPool.RelayerDispute.ts

@@ -1,91 +1,35 @@
 import { expect } from "chai";


nothing changed here, just moved and cut down on redundant imports/method calls.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

mrice32

There are a few commend threads that still need to be resolved, but otherwise, LGTM!

mrice32 · 2022-02-04T05:19:23Z

contracts/MerkleLib.sol

+        // The following arrays are required to be the same length. They are parallel arrays for the given chainId and should be ordered by the `l1Token` field.
        // All whitelisted tokens with nonzero relays on this chain in this bundle in the order of whitelisting.
-        address[] tokenAddresses;
+        address[] l1Token;


Suggested change

address[] l1Token;

address[] l1Tokens;

if we make this change should we make all the arrays plural? netSendAmounts runningBalances?

I think arrays should always be plural

Yeah, I agree. If we didn't do so before in our structs (which could be my fault), I think we should update.

ok will update accordingly on all variables in all places.

nicholaspai · 2022-02-04T14:55:52Z

contracts/HubPool.sol

-        address indexed disputer,
-        SkinnyOptimisticOracleInterface.Request ooPriceRequest,
-        RefundRequest refundRequest
+    event RelayerRefundExecuted(


are any of these events indexable?

yup. will index appropriate ones.

nicholaspai · 2022-02-04T15:01:36Z

test/HubPool.Fixture.ts

+  ).deploy(weth.address, 0, parentFixtureOutput.timer.address);
+  await hubPool.setCrossChainContracts(repaymentChainId, mockAdapter.address, mockSpoke.address);
+
+  // Deploy mock l2 tokens for each token created before and whitelist the routes.


nit: is there any benefit to running all these async funcs in parallel? I know the test network is already super fast but we could possibly get even faster?

I dont think really. I think optimising for readability is the most important thing in fixtures IMHO.

nicholaspai · 2022-02-04T15:02:06Z

test/HubPool.RefundExecution.ts

+    // two token to one chain Id with simple lpFee, netSend and running balance amounts.
+    const wethToSend = toBNWei(100);
+    const daiToSend = toBNWei(1000);
+    const leafs = buildPoolRebalanceLeafs(


Useful function +1

nicholaspai · 2022-02-04T17:54:02Z

contracts/HubPool.sol


    function executeRelayerRefund(
-        uint256 relayerRefundRequestId,
+        uint256 refundRequestId,


As discussed offline, i think we can remove refundRequestId, rename poolRebalance to poolRebalanceLeaf and rename proof to inclusionProof

nicholaspai · 2022-02-04T17:59:09Z

contracts/HubPool.sol

        bytes32 poolRebalanceRoot,
        bytes32 destinationDistributionRoot
-    ) public onlyIfNoActiveRequest {
+    ) public noActiveRequests {


As discussed IRL:

consider adding a comment that after initiateRelayerRefund is called, if the poolRebalanceRoot, destinationDistributionRoot, poolRebalanceLeafCount, and bundleEvaluationBlockNumbers set is disputable then this can be challenged. Once the challenge period passes, then the roots are no longer disputable, and only executeRelayerRefund can be called and initiateRelayerRefund can't be called again until all leafs are executed.

Examples of some characteristics of these roots that could be disputed: poolRebalanceRoot.netSendAmount != destinationDistributionRoot.amountToReturn for example

Motivation is updated comments + interface for the spoke pool distributeRelayerRefund in my PR: 8fb750a

Signed-off-by: chrismaree <christopher.maree@gmail.com>

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

chrismaree and others added 27 commits January 28, 2022 14:04

nit

ba54cb9

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

84aad83

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

e971ff7

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Update contracts/HubPool.sol

7a24fba

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

review nit

2f99c84

Signed-off-by: chrismaree <christopher.maree@gmail.com>

review nit

dcb052a

Signed-off-by: chrismaree <christopher.maree@gmail.com>

feat(hubpool): Refactor hub pool to use 1D bitmap integer

487e7ea

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

39ea052

Signed-off-by: chrismaree <christopher.maree@gmail.com>

WIP

febc4a6

Signed-off-by: chrismaree <christopher.maree@gmail.com>

review nit

1ba6779

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Merge branch 'master' into chrismaree/third-pass-replayment-claim

1886cfd

review nit

439138b

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

b7e3ae2

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

1f6fc17

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

f64bf85

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

8bdcec7

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

89e642a

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

d6bc9bd

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

8de1b8f

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

98b2b52

Signed-off-by: chrismaree <christopher.maree@gmail.com>

WIP

eefd0c0

Signed-off-by: chrismaree <christopher.maree@gmail.com>

WIP

5de87a2

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

9d75d36

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

aca98e0

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Merge branch 'master' into chrismaree/fourth-pass-repayment-claim

6307c53

nit

6d87b1d

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

b03f37e

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree commented Feb 3, 2022

View reviewed changes

nit

f45389b

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nicholaspai reviewed Feb 3, 2022

View reviewed changes

contracts/HubPool.sol Show resolved Hide resolved

nit

03c4d00

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree commented Feb 3, 2022

View reviewed changes

chrismaree commented Feb 4, 2022

View reviewed changes

nit

b6cca33

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree requested review from mrice32 and nicholaspai February 4, 2022 00:14

mrice32 approved these changes Feb 4, 2022

View reviewed changes

nicholaspai reviewed Feb 4, 2022

View reviewed changes

nicholaspai approved these changes Feb 4, 2022

View reviewed changes

nicholaspai changed the title ~~feat(HupPool): Add initial L1->L2 call plumbing~~ feat(HubPool): Add initial L1->L2 call plumbing Feb 4, 2022

nicholaspai reviewed Feb 4, 2022

View reviewed changes

chrismaree added 3 commits February 4, 2022 10:35

nit

804bb41

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

ce3f29c

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

e9c0cb9

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree merged commit 4b998f9 into master Feb 4, 2022

chrismaree deleted the chrismaree/fourth-pass-repayment-claim branch February 4, 2022 20:07

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

fix: L04 Insufficient Access Controls (#19)

fdb5b73

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

fix: L04 Insufficient Access Controls (#19) (#436)

2e203a0

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

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

fix: L04 Insufficient Access Controls (#19)

e6bb797

		_sendTokensToTargetChain(poolRebalance, refundRequestId);
		_executeRelayerRefundOnTargetChain(poolRebalance, refundRequestId);

feat(HubPool): Add initial L1->L2 call plumbing #19

feat(HubPool): Add initial L1->L2 call plumbing #19

Uh oh!

Conversation

chrismaree commented Feb 3, 2022

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismaree Feb 3, 2022 • 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!

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

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 Feb 4, 2022 • 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!

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!

nicholaspai Feb 4, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

chrismaree Feb 3, 2022 •

edited

Loading

chrismaree Feb 4, 2022 •

edited

Loading

nicholaspai Feb 4, 2022 •

edited

Loading