Make IDepositManager.DepositQueued.depositID zero based

Add docstrings to DepositManager.sol Fix docstring in Rollup.sol Co-authored-by: Chih Cheng Liang <[email protected]>
thehubbleproject · Aug 5, 2021 · 2e7cee9 · 2e7cee9
1 parent df8c8c3
commit 2e7cee9
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 15 deletions.
diff --git a/contracts/DepositManager.sol b/contracts/DepositManager.sol
@@ -10,26 +10,55 @@ 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,
         uint256 l2Amount,
         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
@@ -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
@@ -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
@@ -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,