feat(hubpool): Add Initial initiateRelayerRefund implementation #10

chrismaree · 2022-01-27T18:14:54Z

Adds initial initiateRelayerRefund PR.

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

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

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

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

chrismaree · 2022-01-27T18:17:00Z

contracts/MerkleLib.sol

 // SPDX-License-Identifier: GPL-3.0-only
 pragma solidity ^0.8.0;

 import "@openzeppelin/contracts/utils/cryptography/MerkleProof.sol";


changes in this file were due to running the linter config.

nicholaspai · 2022-01-27T19:02:25Z

contracts/BridgeAdmin.sol

+import "./interfaces/BridgeAdminInterface.sol";
+
+
+contract BridgeAdmin is BridgeAdminInterface {


why not just import this from uma/core?

Ah I see that this would be a brand new contract

nicholaspai · 2022-01-27T19:12:50Z

contracts/HubPool.sol

    mapping(address => LPToken) public lpTokens; // Mapping of L1TokenAddress to the associated LPToken.

+    // Address of L1Weth. Enable LPs to deposit/receive ETH, if they choose, when adding/removing liquidity.
+    WETH9Like public l1Weth;


in general do we prefer storing contracts as addresses or as interfaces? I stored weth as an address in the SpokePool for example but we should just standardize

ye, good point. my main question which should inform this decision is is this more expensive to store? is this more expensive to use?

my main reason for doing it this way is it means you dont need to do any type casting when using the interface which makes things a bit easier.

nicholaspai · 2022-01-27T19:13:27Z

contracts/HubPool.sol

+    IERC20 public bondToken;
+
+    // The bondToken's final fee from the UMA Store is scaled by this number to increase the bonding amount.
+    uint256 public bondTokenFinalFeeMultiplier;


Can we make this a uint64?

for context, I use uint64 for all percentage values in spokepool

what? you dont want a 2^256-1 * final fee?

but ye we can. I'll change the type to uint64

nicholaspai · 2022-01-27T19:14:11Z

contracts/HubPool.sol

+        address _bridgeAdmin,
+        address _bondToken,
+        uint256 _bondTokenFinalFeeMultiplier,
+        address _timerAddress


nit: group similar types together

nicholaspai · 2022-01-27T19:14:25Z

contracts/HubPool.sol

     *************************************************/

+    function setBondToken(address _bondToken) public onlyOwner {
+        bondToken = IERC20(_bondToken);


These should emit events since they change state

nicholaspai · 2022-01-27T19:14:42Z

contracts/HubPool.sol

+                status: RefundRequestStatus.Pending
+            })
+        );
+        uint256 relayerRefundId = relayerRefundRequests.length - 1;


int: should the ID be a smaller type like uint64?

nicholaspai

LGTM i agree with the structure but have nits on types

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

mrice32

LGTM! A few minor comments

mrice32 · 2022-01-27T20:31:47Z

contracts/HubPool.sol

+        emit BondTokenSet(newBondToken);
+    }
+
+    function setBondTokenFinalFeeMultiplier(uint64 newBondTokenFinalFeeMultiplier) public onlyOwner {


This is interesting. I totally get the rationale: it's easier to just set the final fees and have this move around as a result since they're kinda a valuation anchor, but do you think this is weird in that it relies on internal UMA system variables for setting its own security bounds? Like if the UMA voting system changed and spam were less of a concern, so final fees were dropped by 95%, we probably wouldn't want this value to change as a result, right?

It's also not super hard to manage this since it's only one bond and one token for the entire Across v2 system.

As a side note: this also means that we need to be sure that the bond token is an approved UMA collateral currency.

this is all true! would you prefer it to simply have one number that is set within the contract and we don't pull anything from the store? my logic was that the store is a reasnoble bond size that can be scaled. we can easily just define one number in this contract that sets the bond for everything

OK I've changed this. we now have a bondAmount that the owner can set. much cleaner

agreed with this, its cleaner and in the same spirit of the UMA final fee

mrice32 · 2022-01-27T20:33:52Z

contracts/HubPool.sol

+        );
+
+        // Pull bonds from from the caller.
+        bondToken.safeTransferFrom(msg.sender, address(this), bondAmount);


This is annoying, but how do we deal with a changing bond amount mid-refund? I know this code is to pull the bond, but do you think we need to store the bond amount for each refund request?

just dont change it if there is a pending refund I guess. if we set the bond as a variable directly (not using the store) this becomes easier.

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

This was renamed in all other events and function prototypes, but it appears to have been missed here. Having it named relayer rather than exclusiveRelayer creates friction when unpacking the event in the sdk. This PR has already been merged into the public contracts-v2; I'm cherry-picking it here for consistency.

mrice32 and others added 6 commits January 25, 2022 16:15

feat: add a first draft of the proof library

fc7df12

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

WIP

df84090

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

Apply suggestions from code review

7eb63c7

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

WIP

68005b4

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

Merge branch 'proof_stub' into chrismaree/first-pass-repayment-claim

6fac5b2

feat(hubpool): Add Initial initiateRelayerRefund implementation

888b9e9

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

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

fixed merge

4eb788b

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

chrismaree commented Jan 27, 2022

View reviewed changes

chrismaree changed the title ~~chrismaree/first pass repayment claim~~ feat(hubpool): Add Initial initiateRelayerRefund implementation Jan 27, 2022

nicholaspai reviewed Jan 27, 2022

View reviewed changes

nicholaspai approved these changes Jan 27, 2022

View reviewed changes

nit

8770d3b

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

mrice32 approved these changes Jan 27, 2022

View reviewed changes

chrismaree added 4 commits January 27, 2022 15:31

nit

2236c03

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

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

3d7ab5f

nit

cf74acd

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

nit

17f2121

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

chrismaree merged commit 448680b into master Jan 27, 2022

chrismaree deleted the chrismaree/first-pass-repayment-claim branch January 27, 2022 23:41

		import "./interfaces/BridgeAdminInterface.sol";


		contract BridgeAdmin is BridgeAdminInterface {

feat(hubpool): Add Initial initiateRelayerRefund implementation #10

feat(hubpool): Add Initial initiateRelayerRefund implementation #10

Uh oh!

Conversation

chrismaree commented Jan 27, 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!

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!

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!

chrismaree Jan 27, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrismaree Jan 27, 2022 •

edited

Loading