From 36e9ab2b0559e0305462e1768a783fd31c78c521 Mon Sep 17 00:00:00 2001 From: chrismaree Date: Wed, 16 Mar 2022 07:59:38 +0200 Subject: [PATCH 1/5] fix[N04] Add additional documentation Signed-off-by: chrismaree --- contracts/HubPool.sol | 2 ++ contracts/HubPoolInterface.sol | 10 +++++----- contracts/MerkleLib.sol | 6 +++++- contracts/SpokePoolInterface.sol | 4 ++-- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c970e0fd2..be83f3c5b 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -124,6 +124,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // the full amount of fees entitled to LPs in ~ 7.72 days, just over the standard L2 7 day liveness. uint256 public lpFeeRatePerSecond = 1500000000000; + // Mapping of l1TokenAddress to cumulative unclaimed protocol tokens that the sent to the protocolFeeCaptureAddress + // at any time. This enables the protocol to capture some percentage of LP fees which can be allocated elsewhere. mapping(address => uint256) public unclaimedAccumulatedProtocolFees; // Address that captures protocol fees. Accumulated protocol fees can be claimed by this address. diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index 692ac40d2..273290db1 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -16,11 +16,11 @@ interface HubPoolInterface { uint256[] bundleLpFees; // This array is grouped with the two above, and it represents the amount to send or request back from the // SpokePool. If positive, the pool will pay the SpokePool. If negative the SpokePool will pay the HubPool. - // There can be arbitrarily complex rebalancing rules defined offchain. This number is only nonzero - // when the rules indicate that a rebalancing action should occur. When a rebalance does not occur, - // runningBalances for this token should change by the total relays - deposits in this bundle. When a rebalance - // does occur, runningBalances should be set to zero for this token and the netSendAmounts should be set to the - // previous runningBalances + relays - deposits in this bundle. + // There can be arbitrarily complex rebalancing rules defined offchain. This number is only nonzero when the + // rules indicate that a rebalancing action should occur. When a rebalance does occur, runningBalances should be + // set to zero for this token and the netSendAmounts should be set to the previous runningBalances + relays - + // deposits in this bundle. If non-zero then it must be set on the SpokePool's RelayerRefundLeaf amountToReturn + // as -1 * this value to indicate if funds are being sent from or to the SpokePool. int256[] netSendAmounts; // This is only here to be emitted in an event to track a running unpaid balance between the L2 pool and the L1 pool. // A positive number indicates that the HubPool owes the SpokePool funds. A negative number indicates that the diff --git a/contracts/MerkleLib.sol b/contracts/MerkleLib.sol index 85666ca8b..1686c4808 100644 --- a/contracts/MerkleLib.sol +++ b/contracts/MerkleLib.sol @@ -14,6 +14,7 @@ library MerkleLib { * @param root the merkle root. * @param rebalance the rebalance struct. * @param proof the merkle proof. + * @return bool to signal if the pool rebalance proof correctly shows inclusion of the rebalance within the tree. */ function verifyPoolRebalance( bytes32 root, @@ -28,6 +29,7 @@ library MerkleLib { * @param root the merkle root. * @param refund the refund struct. * @param proof the merkle proof. + * @return bool to signal if the relayer refund proof correctly shows inclusion of the refund within the tree. */ function verifyRelayerRefund( bytes32 root, @@ -40,8 +42,9 @@ library MerkleLib { /** * @notice Verifies that a distribution is contained within a merkle root. * @param root the merkle root. - * @param slowRelayFulfillment the relayData fulfullment struct. + * @param slowRelayFulfillment the relayData fulfillment struct. * @param proof the merkle proof. + * @return bool to signal if the slow rely's proof correctly shows inclusion of the slow relay within the tree. */ function verifySlowRelayFulfillment( bytes32 root, @@ -95,6 +98,7 @@ library MerkleLib { * @param claimedBitMap a simple uint256 mapping in storage used as a bitmap. Uint8 type enforces that index * can't be > 255. * @param index the index to mark in the bitmap. + * @param uint256 representing the modified input claimedBitMap with the index set to true. */ function setClaimed1D(uint256 claimedBitMap, uint8 index) internal pure returns (uint256) { return claimedBitMap | (1 << index % 256); diff --git a/contracts/SpokePoolInterface.sol b/contracts/SpokePoolInterface.sol index 5f49b0ca8..f5ab96584 100644 --- a/contracts/SpokePoolInterface.sol +++ b/contracts/SpokePoolInterface.sol @@ -7,8 +7,8 @@ pragma solidity ^0.8.0; interface SpokePoolInterface { // This leaf is meant to be decoded in the SpokePool to pay out successful relayers. struct RelayerRefundLeaf { - // This is the amount to return to the HubPool. This occurs when there is a PoolRebalanceLeaf netSendAmount that is - // negative. This is just that value inverted. + // This is the amount to return to the HubPool. This occurs when there is a PoolRebalanceLeaf netSendAmount that + // is negative. This is just that value inverted. uint256 amountToReturn; // Used to verify that this is being executed on the correct destination chainId. uint256 chainId; From 081a59dc0462b32815b066b86544aeedda7a9bc2 Mon Sep 17 00:00:00 2001 From: chrismaree Date: Wed, 16 Mar 2022 08:21:35 +0200 Subject: [PATCH 2/5] nit Signed-off-by: chrismaree --- contracts/HubPool.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c970e0fd2..da28d939c 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -818,7 +818,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { /** * @notice Conveniently queries whether an origin chain + token => destination chain ID is whitelisted and returns - * the whitelisted destination token. + * the whitelisted destination token. If not whitelisted returns address(0x). * @param originChainId Deposit chain. * @param originToken Deposited token. * @param destinationChainId Where depositor can receive funds. From 033d8ee15b4854909417347f95fd2423b8a967ab Mon Sep 17 00:00:00 2001 From: chrismaree Date: Wed, 16 Mar 2022 08:36:37 +0200 Subject: [PATCH 3/5] fix[N06] Fix missleading comments Signed-off-by: chrismaree --- contracts/HubPool.sol | 6 +++--- contracts/HubPoolInterface.sol | 20 ++++++++++---------- contracts/SpokePool.sol | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index e079b72d5..b532c90b0 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -121,7 +121,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { bytes32 public identifier = "IS_ACROSS_V2_BUNDLE_VALID"; // Interest rate payment that scales the amount of pending fees per second paid to LPs. 0.0000015e18 will pay out - // the full amount of fees entitled to LPs in ~ 7.72 days, just over the standard L2 7 day liveness. + // the full amount of fees entitled to LPs in ~ 7.72 days assuming no contract interactions. If someone interacts + // with the contract then the LP rewards are smeared sublinearly over the window. uint256 public lpFeeRatePerSecond = 1500000000000; // Mapping of l1TokenAddress to cumulative unclaimed protocol tokens that the sent to the protocolFeeCaptureAddress @@ -574,7 +575,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint32 requestExpirationTimestamp = uint32(getCurrentTime()) + liveness; - delete rootBundleProposal; // Only one bundle of roots can be executed at a time. + delete rootBundleProposal; // Only one bundle of roots can be executed at a time. Delete the previous bundle. rootBundleProposal.requestExpirationTimestamp = requestExpirationTimestamp; rootBundleProposal.unclaimedPoolRebalanceLeafCount = poolRebalanceLeafCount; @@ -602,7 +603,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * from this contract to the SpokePool designated in the leaf, and will also publish relayer refund and slow * relay roots to the SpokePool on the network specified in the leaf. * @dev In some cases, will instruct spokePool to send funds back to L1. - * @notice Deletes the published root bundle if this is the last leaf to be executed in the root bundle. * @param chainId ChainId number of the target spoke pool on which the bundle is executed. * @param groupIndex If set to 0, then relay roots to SpokePool via cross chain bridge. Used by off-chain validator * to organize leafs with the same chain ID and also set which leaves should result in relayed messages. diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index 273290db1..3d0521697 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -14,17 +14,17 @@ interface HubPoolInterface { uint256 chainId; // Total LP fee amount per token in this bundle, encompassing all associated bundled relays. uint256[] bundleLpFees; - // This array is grouped with the two above, and it represents the amount to send or request back from the - // SpokePool. If positive, the pool will pay the SpokePool. If negative the SpokePool will pay the HubPool. - // There can be arbitrarily complex rebalancing rules defined offchain. This number is only nonzero when the - // rules indicate that a rebalancing action should occur. When a rebalance does occur, runningBalances should be - // set to zero for this token and the netSendAmounts should be set to the previous runningBalances + relays - - // deposits in this bundle. If non-zero then it must be set on the SpokePool's RelayerRefundLeaf amountToReturn - // as -1 * this value to indicate if funds are being sent from or to the SpokePool. + // This array is represents the amount to send to or request back from the SpokePool. If positive, the pool will + // pay the SpokePool. If negative the SpokePool will pay the HubPool. There can be arbitrarily rebalancing rules + // defined offchain. This number is only nonzero when the rules indicate that a rebalancing action should occur. + // When a rebalance does occur, runningBalances should be set to zero for this token and the netSendAmounts + // should be set to the previous runningBalances + relays - deposits in this bundle. If non-zero then it must be + // set on the SpokePool's RelayerRefundLeaf amountToReturn as -1 * this value to indicate if funds are + // being sent to or from the associated SpokePool. int256[] netSendAmounts; - // This is only here to be emitted in an event to track a running unpaid balance between the L2 pool and the L1 pool. - // A positive number indicates that the HubPool owes the SpokePool funds. A negative number indicates that the - // SpokePool owes the HubPool funds. See the comment above for the dynamics of this and netSendAmounts + // This is only here to be emitted in an event to track a running unpaid balance between the L2 pool and the L1 + // pool. A positive number indicates that the HubPool owes the SpokePool funds. A negative number indicates that + // the SpokePool owes the HubPool funds. See the comment above for the dynamics of this and netSendAmounts. int256[] runningBalances; // Used by data worker to mark which leaves should relay roots to SpokePools, and to otherwise organize leaves. // For example, each leaf should contain all the rebalance information for a single chain, but in the case where diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index c42cec6f8..78b023220 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -61,7 +61,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // Merkle root of relayer refunds for successful relays. bytes32 relayerRefundRoot; // This is a 2D bitmap tracking which leafs in the relayer refund root have been claimed, with max size of - // 256x256 leaves per root. + // 256x(2^248) leaves per root. mapping(uint256 => uint256) claimedBitmap; } From ce2d5cb05478389ecca5bfa40327001161c3be5b Mon Sep 17 00:00:00 2001 From: Chris Maree Date: Fri, 18 Mar 2022 14:26:32 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com> --- contracts/SpokePool.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 34cb9b83e..2a97430ae 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -58,7 +58,8 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // Merkle root of relayer refunds for successful relays. bytes32 relayerRefundRoot; // This is a 2D bitmap tracking which leaves in the relayer refund root have been claimed, with max size of - // 256x(2^248) leaves per root. + // 256x(2^248) leaves per root. Due to the indexing scheme in MerkleLib, there are a maximum of + // 2^248 different values of claimedWordIndex with 256 different claimedBitIndexes. mapping(uint256 => uint256) claimedBitmap; } From 3f168429d44b84b1e3f0945e5cc014326252751f Mon Sep 17 00:00:00 2001 From: chrismaree Date: Fri, 18 Mar 2022 17:51:26 +0200 Subject: [PATCH 5/5] nit Signed-off-by: chrismaree --- contracts/HubPool.sol | 3 ++- contracts/HubPoolInterface.sol | 19 +++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 4801c1db2..1a06bed57 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -126,7 +126,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Interest rate payment that scales the amount of pending fees per second paid to LPs. 0.0000015e18 will pay out // the full amount of fees entitled to LPs in ~ 7.72 days assuming no contract interactions. If someone interacts - // with the contract then the LP rewards are smeared sublinearly over the window. + // with the contract then the LP rewards are smeared sublinearly over the window (i.e spread over the remaining + // period for each interaction which approximates a decreasing exponential function). uint256 public lpFeeRatePerSecond = 1500000000000; // Mapping of l1TokenAddress to cumulative unclaimed protocol tokens that can be sent to the protocolFeeCaptureAddress diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index 66977d1a6..ea8c3ed4c 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -15,13 +15,12 @@ interface HubPoolInterface { uint256 chainId; // Total LP fee amount per token in this bundle, encompassing all associated bundled relays. uint256[] bundleLpFees; - // This array is grouped with the two above, and it represents the amount to send or request back from the - // SpokePool. If positive, the pool will pay the SpokePool. If negative the SpokePool will pay the HubPool. - // There can be arbitrarily complex rebalancing rules defined offchain. This number is only nonzero when the - // rules indicate that a rebalancing action should occur. When a rebalance does occur, runningBalances should be - // set to zero for this token and the netSendAmounts should be set to the previous runningBalances + relays - - // deposits in this bundle. If non-zero then it must be set on the SpokePool's RelayerRefundLeaf amountToReturn - // as -1 * this value to indicate if funds are being sent from or to the SpokePool. + // Represents the amount to push to or pull from the SpokePool. If +, the pool pays the SpokePool. If negative + // the SpokePool pays the HubPool. There can be arbitrarily complex rebalancing rules defined offchain. This + // number is only nonzero when the rules indicate that a rebalancing action should occur. When a rebalance does + // occur, runningBalances must be set to zero for this token and netSendAmounts should be set to the previous + // runningBalances + relays - deposits in this bundle. If non-zero then it must be set on the SpokePool's + // RelayerRefundLeaf amountToReturn as -1 * this value to show if funds are being sent from or to the SpokePool. int256[] netSendAmounts; // This is only here to be emitted in an event to track a running unpaid balance between the L2 pool and the L1 // pool. A positive number indicates that the HubPool owes the SpokePool funds. A negative number indicates that @@ -36,9 +35,9 @@ interface HubPoolInterface { uint256 groupIndex; // Used as the index in the bitmap to track whether this leaf has been executed or not. uint8 leafId; - // The following arrays are required to be the same length. They are parallel arrays for the given chainId and - // should be ordered by the l1Tokens field. All whitelisted tokens with nonzero relays on this chain in this - // bundle in the order of whitelisting. + // The bundleLpFees, netSendAmounts, and runningBalances are required to be the same length. They are parallel + // arrays for the given chainId and should be ordered by the l1Tokens field. All whitelisted tokens with nonzero + // relays on this chain in this bundle in the order of whitelisting. address[] l1Tokens; }