feat: Implement _initiateRelayerRefund and begin Optimism-specific SpokePool #20

nicholaspai · 2022-02-03T18:15:36Z

No description provided.

…art-contracts-v2 into npai/relay-refund-spoke-pool

contracts/SpokePool.sol

nicholaspai · 2022-02-03T19:09:30Z

@mrice32 @chrismaree not sure why but I the yarn.lock wasn't regenerating after I added something to the package.json, so i removed and then re-added and it caused a big diff

yarn.lock

mrice32

Looks good! Only minor questions/comments

contracts/Optimism_SpokePool.sol

mrice32 · 2022-02-03T19:38:44Z

contracts/Optimism_SpokePool.sol

+     *         INTERNAL FUNCTIONS         *
+     **************************************/
+
+    function _setCrossDomainAdmin(address newCrossDomainAdmin) internal {


Is there any way to call this function after construction? Is this something we'd like to be able to change? (genuine question, not suggesting a change)

there is a setCrossDomainAdmin that can call this after construction enabling us to change it. I think we'd want to change it in the event something broke within the L1<->L2 logic. one problem, though, that I can see with this pattern is it assumes that the cross domain owner implements sufficient methods to call setCrossDomainAdmin over the bridge. this should be doable as this is going to be sitting within the "Adapter" logic that I've created for the L1->L2 calls. We can create a base contract (CrossDomainAdminBase) or something equivalent that these adapters will inherit from. can think more on this in the next few PRs.

Yeah i added in this function without telling you. We should allow this to be changed from L1

Sorry, I'm a little confused. Are there two functions that allow you to setCrossDomainAdmin? If so, can we consolidate them into a single function?

No there's only one, implemented in this contract. setCrossDomainAdmin is included in the SpokePoolInterface

mrice32 · 2022-02-03T19:40:56Z

contracts/Optimism_SpokePool.sol

+    }
+
+    // TODO:
+    function setDepositQuoteTimeBuffer(uint64 buffer) public onlyFromCrossDomainAccount(crossDomainAdmin) {


Is there any interface or anything that ensures that these functions match across all SpokePool implementations?

Great point I'll add an interface

do you think all L2 spoke pools will immplement the notion of a crossDomainAdmin? @chrismaree

chrismaree · 2022-02-04T02:13:42Z

test/constants.ts


 export const zeroRawValue = { rawValue: "0" };
+
+export const spokePoolRelayerRefundRoot = createRandomBytes32();


I have a const for this in my other PR called mockDestinationDistributionRoot.

Will merge that in

…art-contracts-v2 into npai/relay-refund-spoke-pool

nicholaspai · 2022-02-04T20:44:05Z

contracts/Optimism_SpokePool.sol

+
+contract Optimism_SpokePool is CrossDomainEnabled, SpokePoolInterface, SpokePool {
+    // Address of the L1 contract that acts as the owner of this SpokePool.
+    address public override crossDomainAdmin;


@chrismaree @mrice32 should crossDomainAdmin be in the SpokePool or do you think this notion of a cross domain admin will be different for each. L2?

What is the cross-domain admin? The optimism bridge address on L2? Or is it the BridgeAdmin address on L1?

Its the contract that calls relayMessage on the canonical bridge, so its either the BridgeAdmin or the HubPool

for now HubPool me thinks. we might want to change this later but I think lets leave it there optimistically at the moment.

mrice32

LGTM!

mrice32 · 2022-02-04T21:46:15Z

contracts/Optimism_SpokePool.sol

+
+contract Optimism_SpokePool is CrossDomainEnabled, SpokePoolInterface, SpokePool {
+    // Address of the L1 contract that acts as the owner of this SpokePool.
+    address public override crossDomainAdmin;


What is the cross-domain admin? The optimism bridge address on L2? Or is it the BridgeAdmin address on L1?

contracts/SpokePool.sol

mrice32 · 2022-02-04T21:58:48Z

contracts/SpokePool.sol


+    struct RelayerRefund {
+        // Merkle root of relayer refunds.
+        bytes32 distributionRoot;


One interesting tradeoff about doing claims via merkle proofs one-by-one is that this will result in increased calldata costs on L2 over passing in all the leaves and storing (or maybe even executing) in a single call. This is because you will have to submit each leaf individually with another bytes32 element for each layer in the tree. This means that your overall calldata might be significantly larger than just passing in the leaves alone. I'm not sure how significant this would end up being, but I wonder how hard it would be for us to take in all the leaves and generate the proofs internally on-chain to show that they roll up into the root.

Not suggesting a change, just wanted to mention this.

Hm so you think this could work by the HubPool relaying all leafs to the SpokePool here and storing them on-chain, as opposed to just relaying the root. @chrismaree wdyt?

I dont think @mrice32 is proposing the HubPool passing the leafs; it cant do that in a efficient way. I think @mrice32 is proposing a technique where you can execute multiple leafs at once in a compressed structure and prove they are within the provided root. @mrice32 what you describe would fit into the execution stage of the process, right?

How do you imagine we improve on just making the contract multicall and assuming distributeRelayerRefund is called multiple times in one batched txn?

I wanna be super clear that I don't think we should do this now, and I think before we add this complexity, we should try to better understand the amount of calldata/gas savings we'd see from something like this with different tree sizes.

Agree @chrismaree. I'm not suggesting the HubPool do anything different since any added data anywhere on mainnet is going to be expensive.

If you did multicall, you would effectively be passing:

[{distributionStruct1, proofArray1}, {distributionStruct2, proofArray2}...]

What I'm suggesting is an additional method that allows you to pass all leaves this way:

[{distriubtionStruct1}, {distributionStruct2}, ...]

If we get the logic right in the contract, the leaf data could be used to generate the proofs so you wouldn't need to pass them in, thereby reducing calldata.

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

test/SpokePool.RelayerRefund.ts

chrismaree

this looks great to me! keen to get the next bit of functionality in with the proofs.

nicholaspai · 2022-02-06T17:16:00Z

Just giving @mrice32 a chance to comment here before merging

…ary Calldata (#20)

nicholaspai added 5 commits February 2, 2022 08:05

improve: Destructure RelayData params in FilledRelay event

011278a

use fillAmount not amountToSend in FillRelay event

edecc6f

Remove exclusive test

be559ea

feat: Implement _initiateRelayerRefund and Optimism-specific adapter

3897440

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

1066d32

…art-contracts-v2 into npai/relay-refund-spoke-pool

nicholaspai marked this pull request as ready for review February 3, 2022 18:17

chrismaree reviewed Feb 3, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

Update SpokePool.sol

3543a9f

chrismaree reviewed Feb 3, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

nicholaspai added 3 commits February 3, 2022 10:38

update comment + bump eth-optimism version

e6c247c

gas opt

7481c58

Add tests

a666d86

nicholaspai requested review from chrismaree and mrice32 February 3, 2022 19:08

mrice32 reviewed Feb 3, 2022

View reviewed changes

yarn.lock Show resolved Hide resolved

mrice32 approved these changes Feb 3, 2022

View reviewed changes

nicholaspai added 2 commits February 3, 2022 11:55

Add setCrossDomain func

35d9d87

remove exclsuve test

22ad18a

chrismaree mentioned this pull request Feb 3, 2022

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

Merged

chrismaree reviewed Feb 4, 2022

View reviewed changes

nicholaspai added 4 commits February 4, 2022 07:38

Add interface

eb505e9

Add comemnts and change interface

8fb750a

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

2079344

…art-contracts-v2 into npai/relay-refund-spoke-pool

rebase master

67ef9d8

nicholaspai commented Feb 4, 2022

View reviewed changes

nicholaspai requested review from chrismaree and mrice32 February 4, 2022 20:44

mrice32 approved these changes Feb 4, 2022

View reviewed changes

Update SpokePool.sol

ce78455

Update contracts/SpokePool.sol

37ef6bb

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

nicholaspai requested a review from mrice32 February 4, 2022 23:51

chrismaree reviewed Feb 5, 2022

View reviewed changes

test/SpokePool.RelayerRefund.ts Show resolved Hide resolved

add todo

a61f777

chrismaree approved these changes Feb 5, 2022

View reviewed changes

rename file

2d417e9

mrice32 approved these changes Feb 7, 2022

View reviewed changes

nicholaspai merged commit 9efe41b into master Feb 7, 2022

nicholaspai deleted the npai/relay-refund-spoke-pool branch February 7, 2022 17:24

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

improve: L05 Low Level Call to External Exchange Contract With Arbitr…

2c68c2e

…ary Calldata (#20)


		export const zeroRawValue = { rawValue: "0" };

		export const spokePoolRelayerRefundRoot = createRandomBytes32();

feat: Implement _initiateRelayerRefund and begin Optimism-specific SpokePool #20

feat: Implement _initiateRelayerRefund and begin Optimism-specific SpokePool #20

Uh oh!

Conversation

nicholaspai commented Feb 3, 2022

Uh oh!

Uh oh!

Uh oh!

nicholaspai commented Feb 3, 2022

Uh oh!

Uh oh!

mrice32 left a comment

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

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!

Uh oh!

chrismaree left a comment

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Feb 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants