Allow easy emptying of the queue (#34)

* Bump package version * Allow easy emptying of the queue * Simplify appendQueueBatch logic * Fix lint errors * Address feedback
ethereum-optimism · Oct 26, 2020 · 98fd7e7 · 98fd7e7
1 parent 753b309
commit 98fd7e7
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 13 deletions.
diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/chain/OVM_CanonicalTransactionChain.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/chain/OVM_CanonicalTransactionChain.sol
@@ -14,6 +14,17 @@ import { iOVM_CanonicalTransactionChain } from "../../iOVM/chain/iOVM_CanonicalT
 /* Contract Imports */
 import { OVM_ExecutionManager } from "../execution/OVM_ExecutionManager.sol";
 
+
+library Math {
+    function min(uint x, uint y) internal pure returns (uint z) {
+        if (x < y) {
+            return x;
+        }
+        return y;
+    }
+}
+
+
 /**
  * @title OVM_CanonicalTransactionChain
  */
@@ -237,14 +248,24 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, Lib_Ad
         override
         public
     {
+        uint40 nextQueueIndex = getNextQueueIndex();
+        uint40 queueLength = _getQueueLength();
+        _numQueuedTransactions = Math.min(_numQueuedTransactions, queueLength - nextQueueIndex);
         require(
             _numQueuedTransactions > 0,
             "Must append more than zero transactions."
         );
 
-        uint40 nextQueueIndex = getNextQueueIndex();
         bytes32[] memory leaves = new bytes32[](_numQueuedTransactions);
+
         for (uint256 i = 0; i < _numQueuedTransactions; i++) {
+            if (msg.sender != sequencer) {
+                Lib_OVMCodec.QueueElement memory el = getQueueElement(nextQueueIndex);
+                require(
+                    el.timestamp + forceInclusionPeriodSeconds < block.timestamp,
+                    "Queue transactions cannot be submitted during the sequencer inclusion period."
+                );
+            }
             leaves[i] = _getQueueLeafHash(nextQueueIndex);
             nextQueueIndex++;
         }
@@ -313,6 +334,7 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, Lib_Ad
         uint32 transactionIndex = 0;
         uint32 numSequencerTransactionsProcessed = 0;
         uint40 nextQueueIndex = getNextQueueIndex();
+        uint40 queueLength = _getQueueLength();
 
         for (uint32 i = 0; i < numContexts; i++) {
             BatchContext memory context = _getBatchContext(i);
@@ -331,6 +353,7 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, Lib_Ad
             }
 
             for (uint32 j = 0; j < context.numSubsequentQueueTransactions; j++) {
+                require(nextQueueIndex < queueLength, "Not enough queued transactions to append.");
                 leaves[transactionIndex] = _getQueueLeafHash(nextQueueIndex);
                 nextQueueIndex++;
                 transactionIndex++;
@@ -502,14 +525,6 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, Lib_Ad
             bytes32
         )
     {
-        Lib_OVMCodec.QueueElement memory element = getQueueElement(_index);
-
-        require(
-            msg.sender == sequencer
-            || element.timestamp + forceInclusionPeriodSeconds <= block.timestamp,
-            "Queue transactions cannot be submitted during the sequencer inclusion period."
-        );
-
         return _hashTransactionChainElement(
             Lib_OVMCodec.TransactionChainElement({
                 isSequenced: false,
@@ -521,6 +536,23 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, Lib_Ad
         );
     }
 
+    /**
+     * Retrieves the length of the queue.
+     * @return Length of the queue.
+     */
+    function _getQueueLength()
+        internal
+        view
+        returns (
+            uint40
+        )
+    {
+        // The underlying queue data structure stores 2 elements
+        // per insertion, so to get the real queue length we need
+        // to divide by 2. See the usage of `push2(..)`.
+        return queue.getLength() / 2;
+    }
+
     /**
      * Retrieves the hash of a sequencer element.
      * @param _context Batch context for the given element.

diff --git a/packages/contracts/package.json b/packages/contracts/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@eth-optimism/contracts",
-  "version": "0.0.2-alpha.1",
+  "version": "0.0.2-alpha.3",
   "main": "build/src/index.js",
   "files": [
     "build/**/*.js",

diff --git a/packages/contracts/test/contracts/OVM/chain/OVM_CanonicalTransactionChain.spec.ts b/packages/contracts/test/contracts/OVM/chain/OVM_CanonicalTransactionChain.spec.ts
@@ -423,7 +423,7 @@ describe('OVM_CanonicalTransactionChain', () => {
     it('should revert if the queue is empty', async () => {
       await expect(
         OVM_CanonicalTransactionChain.appendQueueBatch(1)
-      ).to.be.revertedWith('Index out of bounds.')
+      ).to.be.revertedWith('Must append more than zero transactions.')
     })
 
     describe('when the queue is not empty', () => {
@@ -485,10 +485,12 @@ describe('OVM_CanonicalTransactionChain', () => {
                 .withArgs(0, size, size)
             })
 
-            it(`should revert if appending ${size} + 1 elements`, async () => {
+            it(`should be able to append ${size} elements even if attempting to append ${size} + 1 elements`, async () => {
               await expect(
                 OVM_CanonicalTransactionChain.appendQueueBatch(size + 1)
-              ).to.be.revertedWith('Index out of bounds.')
+              )
+                .to.emit(OVM_CanonicalTransactionChain, 'QueueBatchAppended')
+                .withArgs(0, size, size)
             })
           })
         })