feat: relay logic #9

nicholaspai · 2022-01-26T22:36:32Z

Signed-off-by: Nick Pai npai.nyc@gmail.com

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

…ill to pull from relayer

contracts/SpokePool.sol

chrismaree

This looks great to me! I think you should add a bit more to events and perhaps consider changing how you are appending to arrays but other than this it looks great!

contracts/SpokePool.sol

chrismaree · 2022-01-27T19:28:10Z

contracts/SpokePool.sol

+        uint256 amountNetFees = amount - _getAmountFromPct(relay.realizedLpFeePct + relay.relayerFeePct, amount);
+        IERC20(relay.destinationToken).safeTransferFrom(msg.sender, relay.recipient, amountNetFees);
+
+        // If relay token is weth then unwrap and send eth.


do you think that we should try let the user choose to receive WETH or ETH? it's kinda strange that a user sending WETH or ETH always gets ETH? (obs not something for now but I'm curious what you think).

Hm I think this could be useful, how do you propose the user lets the relayer know?

the depositor would need an extra bool, which is kind of a pain 🤔 . might not be worth it tbh

chrismaree

This is excellent! nice and simple, well commented and good code. good work

mrice32

Looks great! A few high-level comments

test/SpokePool.Relay.ts

contracts/SpokePool.sol

mrice32

This looks awesome! Thanks for adding the hashing logic. +1. I have one high-level comment

contracts/SpokePool.sol

mrice32

Only remaining comment: #9 (comment).

chrismaree · 2022-02-01T16:14:42Z

contracts/SpokePool.sol

    // Address of WETH contract for this network. If an origin token matches this, then the caller can optionally
    // instruct this contract to wrap ETH when depositing.
-    address public wethAddress;
+    WETH9Like public weth;


nice. I prefer this pattern.

chrismaree · 2022-02-01T16:15:16Z

contracts/SpokePool.sol

        address timerAddress,
        address _wethAddress,
        uint64 _depositQuoteTimeBuffer
    ) Testable(timerAddress) {


in the constructor params might it be better to put the timer last? we always do this in all the other UMA contracts. wdyt?

chrismaree · 2022-02-01T16:16:59Z

test/SpokePool.Relay.ts

+      spokePool
+        .connect(relayer)
+        .fillRelay(
+            relayData[0],


can you not just use the spread operator here? ...relayData?

I see in a few of these cases you are overriding the data within relayData. this could be solved by returning an object from getRelayhash then overriding the named props using {...relayData, namedPropYouWantToOverride} or something like this so we dont need to define these long lists of indexes.

wdyt, my goal is fewer lines here

mrice32

LGTM! Two minor comments on the fill math, but this looks really good!

mrice32 · 2022-02-01T16:44:11Z

contracts/SpokePool.sol

+        // that we'll add to the `relayFills` counter, and we do this math here in the contract for the user's
+        // convenience so that they don't have to do this math before calling this function. The user can simply
+        // pass in `maxTokensToSend` and assume that the contract will pull exactly that amount of tokens (or revert).
+        uint256 fillAmountPreFees = (maxTokensToSend / (1e18 - (realizedLpFeePct + relayerFeePct))) * 1e18;


Would swapping the order of the multiplication and division mean less rounding?

mrice32 · 2022-02-01T16:48:57Z

contracts/SpokePool.sol

+        // to the amount filled so far for a particular `relayHash`, so this will start at 0 and increment with each
+        // fill.
+        require(
+            maxTokensToSend > 0 && totalRelayAmount >= relayFills[relayHash] + fillAmountPreFees,


I think this would fail if the user passes in a number larger than the amount needed to fill the remainder of the relay. Should we do some something like min(totalRelayAmount - relayFills[relayHash], fillAmountPreFees) to determine how much they will actually fill?

Ah yes i forgot to add this logic, will do

Do you think this logic is too convoluted and are there gas savings? c6970ae

mrice32 · 2022-02-01T18:38:22Z

contracts/SpokePool.sol

+        // we'll pull exactly enough tokens to complete the relay.
+        uint256 amountToSend;
+        if (totalRelayAmount - relayFills[relayHash] < fillAmountPreFees) {
+            amountToSend = totalRelayAmount - relayFills[relayHash];


Won't this cause them to send the amount including fees. Don't we need to multiply this by (1e18 - (realizedLpFeePct + relayerFeePct)) / 1e18?

Ah yes good call, fixed

mrice32 · 2022-02-01T19:23:41Z

contracts/SpokePool.sol

+    }
+
+    function _computeAmountPostFees(uint256 amount, uint256 feesPct) private pure returns (uint256) {
+        return (amount * feesPct) / 1e18;


nit: should this also be (1e18 - feesPct) rather than just feesPct?

shoot yeah, the test case where the fees are equally set to 0.25 and 0.25 is confusing because 1e18 - totalFeesPct == totalFeesPct. I changed the test values to 0.1 and 0.2 and changed this solidity code to 1e18 - feesPct

mrice32

Everything looks good, just that one nit: #9 (comment)

…art-contracts-v2 into npai/relay-flow

mrice32

LGTM!

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

Initially submitted by James. Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu> Co-authored-by: James Morris, MS <96435344+james-a-morris@users.noreply.github.com>

feat: Initiate and fill relay logic

ccd67bd

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

nicholaspai requested review from chrismaree and mrice32 January 26, 2022 22:36

nicholaspai added 2 commits January 27, 2022 08:47

remove fill ID, separate deposit and relay nonces, update amount of f…

0514c3b

…ill to pull from relayer

Fix

35320bb

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

nicholaspai added 4 commits January 27, 2022 10:58

Change destinationToken name and add relayer address to events

634bbfb

add event param

a140097

Use relay length as nonce

0a4b55e

Update SpokePool.sol

8de2332

nicholaspai requested a review from chrismaree January 27, 2022 19:11

lint

38e7c16

chrismaree reviewed Jan 27, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

chrismaree reviewed Jan 27, 2022

View reviewed changes

chrismaree approved these changes Jan 27, 2022

View reviewed changes

mrice32 reviewed Jan 27, 2022

View reviewed changes

test/SpokePool.Relay.ts Outdated Show resolved Hide resolved

contracts/SpokePool.sol Outdated Show resolved Hide resolved

contracts/SpokePool.sol Outdated Show resolved Hide resolved

contracts/SpokePool.sol Outdated Show resolved Hide resolved

nicholaspai added 2 commits January 28, 2022 06:20

Map relays by deposit hash

27eeff4

key by relayHash instead of depositHash

977a9a4

nicholaspai requested review from chrismaree and mrice32 January 28, 2022 15:18

mrice32 reviewed Jan 28, 2022

View reviewed changes

contracts/SpokePool.sol Show resolved Hide resolved

chrismaree reviewed Jan 28, 2022

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

Key relays by hash, combine relay funcs into one method

c74b6fc

add indexed event params and rename relays => relayFills

f6341f4

mrice32 reviewed Jan 31, 2022

View reviewed changes

Add tests

380f3d1

nicholaspai requested a review from mrice32 January 31, 2022 22:52

chrismaree reviewed Feb 1, 2022

View reviewed changes

nicholaspai added 3 commits February 1, 2022 08:33

Add tests

977ffa9

use spread operator and move timer to last param

84a89dc

lint

20ce355

nicholaspai requested a review from chrismaree February 1, 2022 16:43

mrice32 approved these changes Feb 1, 2022

View reviewed changes

Pull remaining relay amount from relayer if their max > amountRemaining

c6970ae

nicholaspai requested a review from mrice32 February 1, 2022 17:42

mrice32 approved these changes Feb 1, 2022

View reviewed changes

pull tokens net fees even in case

a3a3de6

nicholaspai requested a review from mrice32 February 1, 2022 19:21

lint

f78353a

mrice32 reviewed Feb 1, 2022

View reviewed changes

mrice32 approved these changes Feb 1, 2022

View reviewed changes

Fix fees calc

f0c23b3

nicholaspai requested a review from mrice32 February 1, 2022 19:39

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

e99e1e7

…art-contracts-v2 into npai/relay-flow

mrice32 approved these changes Feb 1, 2022

View reviewed changes

nicholaspai merged commit a44eeb2 into master Feb 1, 2022

nicholaspai deleted the npai/relay-flow branch February 1, 2022 19:59

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

feat(testnet): add sepolia deployments (#9)

156f0b7

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

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

feat(testnet): add sepolia deployments (#9)

80cf678

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

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

feat(testnet): add sepolia deployments (#9)

2ed3975

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

feat: relay logic #9

feat: relay logic #9

Uh oh!

Conversation

nicholaspai commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

chrismaree left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

nicholaspai commented Jan 26, 2022 •

edited

Loading