Repository: thehubbleproject/hubble-contracts (ts)

Issue:
Make Batch a mapping
We currently define batch as an array in storage, this comes with some disadvantages:

- Need to keep track of `batches.length`
- Can't simply delete a finalized batch



Pull request:
Make batches a mapping
Fix #338 

Patch:
diff --git a/contracts/rollup/BatchManager.sol b/contracts/rollup/BatchManager.sol
index 92eef9e..7bfaa80 100644
--- a/contracts/rollup/BatchManager.sol
+++ b/contracts/rollup/BatchManager.sol
@@ -14,7 +14,10 @@ contract BatchManager is Parameters {
     // External contracts
     IDepositManager public depositManager;
 
-    Types.Batch[] public batches;
+    // batchID -> Batch
+    mapping(uint256 => Types.Batch) public batches;
+    // nextBatchID also represents how many batches in `batches`
+    uint256 public nextBatchID = 0;
 
     // batchID -> depositSubtreeRoot
     mapping(uint256 => bytes32) public deposits;
@@ -54,17 +57,13 @@ contract BatchManager is Parameters {
         _;
     }
 
-    function numOfBatchesSubmitted() public view returns (uint256) {
-        return batches.length;
-    }
-
     function getBatch(uint256 batchID)
         external
         view
         returns (Types.Batch memory batch)
     {
         require(
-            batches.length - 1 >= batchID,
+            batches[batchID].meta != bytes32(0),
             "Batch id greater than total number of batches, invalid batch id"
         );
         batch = batches[batchID];
@@ -79,13 +78,12 @@ contract BatchManager is Parameters {
         bytes32 depositSubTreeRoot;
         uint256 totalSlashings = 0;
         for (
-            uint256 batchID = batches.length - 1;
+            uint256 batchID = nextBatchID - 1;
             batchID >= invalidBatchMarker;
             batchID--
         ) {
             if (gasleft() <= paramMinGasLeft) break;
 
-            Bitmap.setClaimed(batchID, withdrawalBitmap);
             delete batches[batchID];
 
             depositSubTreeRoot = deposits[batchID];
@@ -93,11 +91,11 @@ contract BatchManager is Parameters {
                 depositManager.reenqueue(depositSubTreeRoot);
 
             totalSlashings++;
-            batches.length--;
+            nextBatchID--;
 
             emit BatchRollback(batchID);
         }
-        if (batches.length == invalidBatchMarker) invalidBatchMarker = 0;
+        if (nextBatchID == invalidBatchMarker) invalidBatchMarker = 0;
 
         uint256 slashedAmount = totalSlashings.mul(paramStakeAmount);
         uint256 reward = slashedAmount.mul(2).div(3);
@@ -118,7 +116,7 @@ contract BatchManager is Parameters {
         Types.Usage batchType
     ) internal {
         require(msg.value >= paramStakeAmount, "Rollup: wrong stake amount");
-        Types.Batch memory newBatch = Types.Batch({
+        batches[nextBatchID] = Types.Batch({
             commitmentRoot: commitmentRoot,
             meta: Types.encodeMeta(
                 uint256(batchType),
@@ -127,8 +125,8 @@ contract BatchManager is Parameters {
                 block.number + paramBlocksToFinalise
             )
         });
-        batches.push(newBatch);
-        emit NewBatch(msg.sender, batches.length - 1, batchType);
+        emit NewBatch(msg.sender, nextBatchID, batchType);
+        nextBatchID++;
     }
 
     /**
diff --git a/contracts/rollup/Rollup.sol b/contracts/rollup/Rollup.sol
index 6104aee..760b3b5 100644
--- a/contracts/rollup/Rollup.sol
+++ b/contracts/rollup/Rollup.sol
@@ -207,7 +207,7 @@ contract RollupCore is BatchManager {
         Types.CommitmentInclusionProof memory previous,
         Types.SubtreeVacancyProof memory vacant
     ) public payable onlyCoordinator isNotRollingBack {
-        uint256 preBatchID = batches.length - 1;
+        uint256 preBatchID = nextBatchID - 1;
         require(
             previous.path == batches[preBatchID].size() - 1,
             "previous commitment has wrong path"
@@ -390,7 +390,7 @@ contract Rollup is RollupCore {
         bytes32 commitmentRoot = keccak256(
             abi.encode(genesisCommitment, ZERO_BYTES32)
         );
-        Types.Batch memory newBatch = Types.Batch({
+        batches[nextBatchID] = Types.Batch({
             commitmentRoot: commitmentRoot,
             meta: Types.encodeMeta(
                 uint256(Types.Usage.Genesis),
@@ -399,8 +399,8 @@ contract Rollup is RollupCore {
                 block.number // genesis finalise instantly
             )
         });
-        batches.push(newBatch);
-        emit NewBatch(msg.sender, batches.length - 1, Types.Usage.Genesis);
+        emit NewBatch(msg.sender, nextBatchID, Types.Usage.Genesis);
+        nextBatchID++;
         appID = keccak256(abi.encodePacked(address(this)));
     }
 }


Test patch:
diff --git a/contracts/test/TestRollup.sol b/contracts/test/TestRollup.sol
index 3099add..3c540a2 100644
--- a/contracts/test/TestRollup.sol
+++ b/contracts/test/TestRollup.sol
@@ -35,7 +35,7 @@ contract TestRollup is BatchManager {
     }
 
     function submitDeposits(bytes32 depositSubTreeRoot) external payable {
-        deposits[batches.length] = depositSubTreeRoot;
+        deposits[nextBatchID] = depositSubTreeRoot;
         submitBatch(bytes32(0), 0, Types.Usage.Transfer);
     }
 
diff --git a/test/MassMigration.test.ts b/test/MassMigration.test.ts
index 2f23793..68fcf16 100644
--- a/test/MassMigration.test.ts
+++ b/test/MassMigration.test.ts
@@ -114,7 +114,7 @@ describe("Mass Migrations", async function() {
 
         await targetBatch.submit(rollup, TESTING_PARAMS.STAKE_AMOUNT);
 
-        const batchId = Number(await rollup.numOfBatchesSubmitted()) - 1;
+        const batchId = Number(await rollup.nextBatchID()) - 1;
         const rootOnchain = await registry.registry.root();
         assert.equal(root, rootOnchain, "mismatch pubkey tree root");
         const batch = await rollup.getBatch(batchId);
@@ -179,7 +179,7 @@ describe("Mass Migrations", async function() {
         const batch = commitment.toBatch();
         await batch.submit(rollup, TESTING_PARAMS.STAKE_AMOUNT);
 
-        const batchId = Number(await rollup.numOfBatchesSubmitted()) - 1;
+        const batchId = Number(await rollup.nextBatchID()) - 1;
 
         await expectRevert(
             withdrawManager.processWithdrawCommitment(batchId, batch.proof(0)),
diff --git a/test/rollback.test.ts b/test/rollback.test.ts
index 86d696a..1bafaa4 100644
--- a/test/rollback.test.ts
+++ b/test/rollback.test.ts
@@ -16,7 +16,7 @@ describe("Rollback", function() {
     const numOfBatches = 250;
 
     async function getTipBatchID() {
-        return Number(await rollup.numOfBatchesSubmitted()) - 1;
+        return Number(await rollup.nextBatchID()) - 1;
     }
 
     beforeEach(async function() {
diff --git a/test/rollup.test.ts b/test/rollup.test.ts
index 9c999cd..b1206d8 100644
--- a/test/rollup.test.ts
+++ b/test/rollup.test.ts
@@ -106,7 +106,7 @@ describe("Rollup", async function() {
             await (await _txSubmit.wait()).gasUsed.toNumber()
         );
 
-        const batchId = Number(await rollup.numOfBatchesSubmitted()) - 1;
+        const batchId = Number(await rollup.nextBatchID()) - 1;
         const batch = await rollup.getBatch(batchId);
 
         assert.equal(