feat: add slow relay functionality #28

mrice32 · 2022-02-08T20:47:46Z

This doesn't include tests or some of the meat of the verification and fill logic just to allow us to agree on the interfaces before moving forward.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree · 2022-02-08T20:51:20Z

contracts/HubPool.sol

        uint64 unclaimedPoolRebalanceLeafCount;
        bytes32 poolRebalanceRoot;
        bytes32 destinationDistributionRoot;
+        bytes32 slowRelayDataRoot;


you'll need to add this to initiateRelayerRefund. I can do that in a separate PR though, If you'd like as this will break a bunch of existing tests.

I dont like this variable name. can it be SlowRelayFulfilmentRoot?

Oh, I thought I added it everywhere, will do another pass to better understand where I missed.

On the naming, sure!

Done on both.

chrismaree · 2022-02-08T20:52:18Z

contracts/MerkleLib.sol

    }

+    // TODO: should these structs be in this file?
+    struct RelayData {


would a better name be SlowRelayFulfilment? then use this through out, such as verifySlowRelayFufilment

Well, it's tricky here because this is the exact data structure the SpokePool uses internally to represent a relay. I don't know if we want to rename that struct.

move this to SpokePool interface?

hhhmm. ye I see your point. not ideal.

if we move it to the SpokePool interface then we'll get the MerkleLib to import from this interface, yes? equally, we can pull the Hub's Merkle structs to a HubPool interface and import them into the MerkleLib in the same way.

contracts/SpokePool.sol

chrismaree · 2022-02-08T20:55:06Z

contracts/SpokePool.sol

+        address depositor,
+        address recipient,
+        address destinationToken,
+        uint64 realizedLpFeePct,
+        uint64 relayerFeePct,
+        uint64 depositId,
+        uint256 originChainId,
+        uint256 totalRelayAmount,


all these props can be taken directly from the MerkleLib.RelayData, no? would it be cleaner to pass in one struct and then the extra fields that are not in the struct?

I was trying to mirror the fill method, as I think we will effectively have 3 fill methods that will have mostly the same parameters. Would like these to be as consistent as possible, so if we use a struct here, maybe it makes sense to use a struct in the others?

ye, Ideally we pass around structs in all these methods methinks

The only issue is the interpretability on etherscan :/

we do this on v1 and you can decode them I think.

I take that back! looks like Etherscan has removed this for some reason :/

this is a settlement tx

we might need to re-evaluate the interface in the hub pool as well to spread the props, like you do it here.

Yeah, this definitely used to work :((

i spread props in other external spokepool methods for thi very reason

I'll make this change in the hub :(((((

nicholaspai

This skeleton makes total sense +1

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree

this is awesome! good work on re-factoring the interfaces like this.

nicholaspai · 2022-02-10T18:18:58Z

contracts/HubPool.sol

        uint64 unclaimedPoolRebalanceLeafCount;
        bytes32 poolRebalanceRoot;
        bytes32 destinationDistributionRoot;
+        bytes32 slowRelayFulfilmentRoot;


nit: I hate to be that grammar guy but fulfillment has two L's in the "fill" part

Thank you!

I don't mean to throw @chrismaree under the bus, but I just copy-pasted this from his suggestion haha

nicholaspai · 2022-02-10T18:21:30Z

contracts/HubPoolInterface.sol

+import "./chain-adapters/AdapterInterface.sol";
+
+interface HubPoolInterface {
+    struct PoolRebalance {


nit: Should we add "Leaf" to the end of PoolRebalance and DestiationDistribution to make it clear that they are to be used as merkle root structures? @chrismaree wdyt, i'm not strongly opinionated and its not a big deal but i think its more readable

Agree, will try to make this change. Hopefully will just be a simple find and replace.

contracts/MerkleLib.sol

nicholaspai · 2022-02-10T18:22:29Z

contracts/MerkleLib.sol

 * @notice Library to help with merkle roots, proofs, and claims.
 */
 library MerkleLib {
-    // TODO: some of these data structures can be moved out if convenient.


+1 moving these to the spoke/hub pool interface i think that makes a lot of sense

contracts/SpokePool.sol

nicholaspai · 2022-02-10T18:26:02Z

contracts/SpokePool.sol

            // If relay token is weth then unwrap and send eth.
            if (relayData.destinationToken == address(weth)) {
-                IERC20(relayData.destinationToken).safeTransferFrom(msg.sender, address(this), amountToSend);
+                if (!isSlowRelay)


Can you add a comment explaining that the source of funds to pay the recipient is either the caller+relayer or this contract depending on whether this is a slow relay

nicholaspai · 2022-02-10T18:27:40Z

contracts/SpokePool.sol

+        );
+
+        bytes32 relayHash = _getRelayHash(relayData);
+        uint256 fillAmountPreFees = _fillRelay(relayHash, relayData, relayerFeePct, type(uint256).max / 1e18, true);


You could also set maxAmountToSend == totalRelayAmount which might make it more readable?

Interesting idea -- what you're saying is that relayAmount >= total send, so this will always result in the relay being completely filled. I like this. Will add an explanatory comment too. Always in favor of less math.

exactly yeah, if you try to relay the full relayAmount you'll always end up sending the max

nicholaspai · 2022-02-10T18:28:02Z

contracts/SpokePoolInterface.sol

-
 interface SpokePoolInterface {
+    // This leaf is meant to be decoded in the SpokePool in order to pay out individual relayers for this bundle.
+    struct DestinationDistribution {


nit: same as PoolRebalance, maybe add "Leaf" to end of this name

nicholaspai

Thanks for merging in my SpokePool changes! I left mostly nits on naming + comments and one suggestion for which variable to pass into the internal _fillRelay method. I also appreciate you not adding tests yet so we can re-review them in a second pass or a follow up PR. I'll be sure to follow the same commit pattern on my PR's!

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

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai · 2022-02-11T14:22:35Z

contracts/MerkleLib.sol

     * @param proof the merkle proof.
     */
-    function verifyPoolRebalance(
+    function verifyPoolRebalanceLeaf(


why rename this function to end with "Leaf"? I think its not neccessary since you take in a param called "leaf", but if you do go this route then I think you should end verifyRelayData and verifyRelayerDistribution also with "Leaf"

Perhaps verifyRelayData is more appropriately named verifyRelayDataHashLeaf or verifyRelayHashLeaf

Okay, fixed on both! I reverted the Leaf change here (it was a find-and-replace issue I think)

nicholaspai · 2022-02-11T14:24:12Z

This LGTM but just be consistent on using "leaf" in the "verify-" function names

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree · 2022-02-11T21:33:39Z

contracts/SpokePool.sol

 import "@uma/core/contracts/common/implementation/MultiCaller.sol";
 import "./MerkleLib.sol";
 import "./SpokePoolInterface.sol";
+import "hardhat/console.sol";


@mrice32 nit

Thanks, will remove!

chrismaree · 2022-02-11T21:48:38Z

contracts/test/MockSpokePool.sol

+        public
+        override
+    {
+        _initializeRelayerRefund(relayerRepaymentDistributionRoot, slowRelayFulfillmentRoot);


I know you've changed this through the play but do we think that changing these variable names would make our linting a bit easier? relayerRepaymentDistributionRoot is a very long variable name. would relayerRepaymentRoot be better? and slowRelayFulfillmentRoot to slowRelayRoot? My goal is that we have a lot of functions and variables that are wrapping onto multiple lines and would perhaps not wrap if they were a bit shorter.

dont worry about this for now but just something to think about. IMHO as much as posible having code on one line will make things a lot easier to read and generally makes the code look much cleaner.

Yeah, I'll leave for a follow-up. We've had a lot of back and forth on naming in this PR, and just to avoid doing a bunch more find-and-replace, we can do this in one shot in a follow up if that's cool.

chrismaree · 2022-02-11T21:48:47Z

package.json

  },
+  "engines": {
+    "node": ">=14.0.0"
+  },


I ran some command that failed saying it was incompatible with node version 8, and adding this fixed it. I can't remember what command it was, and I can't seem to find it in my local history, so I'll remove.

chrismaree · 2022-02-11T21:49:04Z

test/HubPool.Fixture.ts

 import { TokenRolesEnum } from "@uma/common";
 import { getContractFactory, randomAddress, toBN, fromWei } from "./utils";

-import { bondAmount, refundProposalLiveness, finalFee, identifier, repaymentChainId } from "./constants";


we should lint for unused variables.

Agree! We should turn on eslint, as it will catch stuff like this. I think these are all used, though.

chrismaree · 2022-02-11T21:49:52Z

test/HubPool.Fixture.ts

 import { Contract, Signer } from "ethers";
 import hre from "hardhat";


both of these lines can be imported from utils. dont worry about it now, though. I'll do a clean up PR later.

Fixed for this file. Haven't checked other files tho.

chrismaree · 2022-02-11T21:50:11Z

test/HubPool.PooledTokenSynchronization.ts

-    await hubPool.connect(dataWorker).initiateRelayerRefund([3117], 1, tree.getHexRoot(), consts.mockTreeRoot);
+    await hubPool
+      .connect(dataWorker)
+      .initiateRelayerRefund([3117], 1, tree.getHexRoot(), consts.mockTreeRoot, consts.mockSlowRelayFulfillmentRoot);


here is another example of the long ass variable names and how it makes the code ugly.

Yeah, we can update these when we update the other names. I agree that shorter names would be better! So let's decide offline what these should be called and do a big naming PR.

chrismaree

This is great! I left a few nits but overall this is excellent work! Feel free to merge once you've implemented the nits.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

Restoring this for backwards compatibility with the v2 ABI. Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

feat: add slow relay functionality

976af4b

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested review from chrismaree and nicholaspai February 8, 2022 20:47

chrismaree reviewed Feb 8, 2022

View reviewed changes

contracts/SpokePool.sol Show resolved Hide resolved

chrismaree reviewed Feb 8, 2022

View reviewed changes

nicholaspai approved these changes Feb 8, 2022

View reviewed changes

mrice32 added 2 commits February 8, 2022 16:59

WIP

1ffad81

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

abeee00

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree approved these changes Feb 8, 2022

View reviewed changes

mrice32 added 5 commits February 8, 2022 21:19

WIP

13472b8

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

3fae4b0

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

Merge branch 'master' into slow_relay

4ebec18

fixes

4485b80

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

2e2ce87

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree approved these changes Feb 10, 2022

View reviewed changes

nicholaspai reviewed Feb 10, 2022

View reviewed changes

contracts/MerkleLib.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Feb 10, 2022

View reviewed changes

contracts/MerkleLib.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Feb 10, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Feb 10, 2022

View reviewed changes

nicholaspai approved these changes Feb 10, 2022

View reviewed changes

mrice32 and others added 3 commits February 10, 2022 17:05

Apply suggestions from code review

ed1cb93

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

WIP

c760af8

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

Merge branch 'master' into slow_relay

204f134

mrice32 added 3 commits February 10, 2022 21:45

update names

acbfbd7

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

correct find and replace

84d05a3

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

385e0b5

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai reviewed Feb 11, 2022

View reviewed changes

WIP

42565d4

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested review from chrismaree and nicholaspai February 11, 2022 18:46

WIP

e3aaf76

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree reviewed Feb 11, 2022

View reviewed changes

chrismaree approved these changes Feb 11, 2022

View reviewed changes

WIP

cbc55f1

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 merged commit 271f16a into master Feb 11, 2022

mrice32 deleted the slow_relay branch February 11, 2022 22:27

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

fix: Restore v2 FundsDeposited event (#28)

94c4f3d

Restoring this for backwards compatibility with the v2 ABI. Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

		import { Contract, Signer } from "ethers";
		import hre from "hardhat";

feat: add slow relay functionality #28

feat: add slow relay functionality #28

Uh oh!

Conversation

mrice32 commented Feb 8, 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!

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!

nicholaspai left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

mrice32 Feb 10, 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!

mrice32 Feb 10, 2022 •

edited

Loading