From 2e7cee934d13f1ddc3d53d1557193578c68cc13c Mon Sep 17 00:00:00 2001 From: Jacob Caban-Tomski Date: Tue, 3 Aug 2021 16:24:11 -0600 Subject: [PATCH] Make IDepositManager.DepositQueued.depositID zero based Add docstrings to DepositManager.sol Fix docstring in Rollup.sol Co-authored-by: Chih Cheng Liang --- contracts/DepositManager.sol | 48 +++++++++++++++++-- contracts/rollup/Rollup.sol | 11 ++--- test/fast/deposit.test.ts | 6 +-- .../services/events/depositsFinalised.ts | 3 +- 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/contracts/DepositManager.sol b/contracts/DepositManager.sol index 12321661..c631e55a 100644 --- a/contracts/DepositManager.sol +++ b/contracts/DepositManager.sol @@ -10,7 +10,19 @@ import { ImmutableOwnable } from "./libs/ImmutableOwnable.sol"; import { Rollup } from "./rollup/Rollup.sol"; import { ITokenRegistry } from "./TokenRegistry.sol"; +/** + * @notice Interface for a contract acting as a manager of deposits for a Hubble network + */ interface IDepositManager { + /** + * @notice Event when a deposit has been enqueued + * for eventual submission to the rollup + * @param pubkeyID Registered public key ID + * @param tokenID Registered token ID + * @param l2Amount UserState.balance of deposit + * @param subtreeID Subtree this deposit will be part of (1 ... n) + * @param depositID Deposit number in the subtree (0 ... 1 << maxSubtreeDepth) + */ event DepositQueued( uint256 pubkeyID, uint256 tokenID, @@ -18,18 +30,35 @@ interface IDepositManager { uint256 subtreeID, uint256 depositID ); + /** + * @notice Event when a deposit subtree is ready + * to be submitted to the rollup + * @param subtreeID Subtree ID of deposits (1 ... n) + * @param subtreeRoot Merklized root of subtree + */ event DepositSubTreeReady(uint256 subtreeID, bytes32 subtreeRoot); + /** + * @notice Max subtree depth for queued deposits + */ function paramMaxSubtreeDepth() external returns (uint256); + /** + * @notice Dequeues a deposit subtree for submission to the rollup + */ function dequeueToSubmit() external returns (uint256 subtreeID, bytes32 subtreeRoot); + /** + * @notice Re-enqueues a deposit subtree. + * @param subtreeRoot Merklized root of subtree + */ function reenqueue(bytes32 subtreeRoot) external; } /** + * @notice Contract which is a queue for deposit subtrees * @dev subtreeID starts at 1 */ contract SubtreeQueue { @@ -59,6 +88,10 @@ contract SubtreeQueue { } } +/** + * @notice Contract which merges deposits into subtrees + * @dev subtreeID starts at 1 + */ contract DepositCore is SubtreeQueue { // An element is a deposit tree root of any depth. // It could be just a leaf of a new deposit or @@ -78,9 +111,10 @@ contract DepositCore is SubtreeQueue { internal returns (uint256 subtreeID, uint256 depositID) { - depositID = depositCount + 1; - uint256 i = depositID; + depositID = depositCount; + uint256 numDeposits = depositID + 1; + uint256 i = numDeposits; uint256 len = babyTreesLength; babyTrees[len] = depositLeaf; len++; @@ -97,7 +131,7 @@ contract DepositCore is SubtreeQueue { babyTreesLength = len; // Subtree is ready, send to SubtreeQueue - if (depositID == paramMaxSubtreeSize) { + if (numDeposits == paramMaxSubtreeSize) { subtreeID = enqueue(babyTrees[0]); // reset babyTreesLength = 0; @@ -106,10 +140,16 @@ contract DepositCore is SubtreeQueue { } subtreeID = back + 1; - depositCount = depositID; + depositCount = numDeposits; } } +/** + * @notice Contract which manages deposits. Most functions are + * restricted only for the rollup's use. Most Hubble users need + * only call depositFor. + * @dev subtreeID starts at 1 + */ contract DepositManager is DepositCore, IDepositManager, diff --git a/contracts/rollup/Rollup.sol b/contracts/rollup/Rollup.sol index 3772ee65..a051bb5f 100644 --- a/contracts/rollup/Rollup.sol +++ b/contracts/rollup/Rollup.sol @@ -107,14 +107,13 @@ contract Rollup is BatchManager, EIP712, IEIP712 { _; } - /* - * When running multiple batch submissions in a single transaction, + /** + * @dev When running multiple batch submissions in a single transaction, * if one of the earlier batch submissions fails the latter submission * will still proceed, be invalid since it depended on the earlier - * submission's state, and be slashed. - * - * To prevent this scenario, verify the expected batchID matches - * nextBatchID when a submission function is executed. + * submission's state, and be slashed. To prevent this scenario, + * verify the expected batchID matches nextBatchID when a + * submission function is executed. * * @param batchID expected batch ID */ diff --git a/test/fast/deposit.test.ts b/test/fast/deposit.test.ts index 1664927a..b5e4af5c 100644 --- a/test/fast/deposit.test.ts +++ b/test/fast/deposit.test.ts @@ -176,7 +176,7 @@ describe("DepositManager", async function() { const event0State = State.fromDepositQueuedEvent(event0); assert.equal(event0State.hash(), deposit0.hash()); assert.equal(event0.args.subtreeID.toNumber(), 1); - assert.equal(event0.args.depositID.toNumber(), 1); + assert.equal(event0.args.depositID.toNumber(), 0); const txDeposit1 = await depositManager.depositFor( 1, @@ -195,7 +195,7 @@ describe("DepositManager", async function() { const event1State = State.fromDepositQueuedEvent(event1); assert.equal(event1State.hash(), deposit1.hash()); assert.equal(event1.args.subtreeID.toNumber(), 1); - assert.equal(event1.args.depositID.toNumber(), 2); + assert.equal(event1.args.depositID.toNumber(), 1); const [eventReady] = await depositManager.queryFilter( depositManager.filters.DepositSubTreeReady(), @@ -222,7 +222,7 @@ describe("DepositManager", async function() { const event2State = State.fromDepositQueuedEvent(event2); assert.equal(event2State.hash(), deposit2.hash()); assert.equal(event2.args.subtreeID.toNumber(), 2); - assert.equal(event2.args.depositID.toNumber(), 1); + assert.equal(event2.args.depositID.toNumber(), 0); }); it("submit a deposit Batch to rollup", async function() { diff --git a/ts/client/services/events/depositsFinalised.ts b/ts/client/services/events/depositsFinalised.ts index 9866c982..a761beaa 100644 --- a/ts/client/services/events/depositsFinalised.ts +++ b/ts/client/services/events/depositsFinalised.ts @@ -78,8 +78,7 @@ export class DepositsFinalisedEventSyncer extends ContractEventSyncer { for (const pd of pendingDeposits) { const stateID = pathToSubTree .mul(2 ** this.maxDepositSubtreeDepth) - .add(pd.depositID) - .sub(1); + .add(pd.depositID); const tokenIDStr = pd.tokenID.toString(); this.feeReceivers.push({ tokenID: tokenIDStr,