[Protocol3] Some minor cleanup/enhancements + More tests (#253)

* [Protocol3] Removed FINALIZED block state + misc cleanup

* [Protocol3] Improved tests

* [Protocol3] Added some tests/fixed some typos

* [Protocol3] Added some tests for overflow checking

* [Protocol3] Fixed issue after updating node version

Author: Brechtpd

packages/loopring_v3/contracts/iface/IExchange.sol

@@ -181,7 +181,7 @@ contract IExchange
     ///          depositFee +
     ///          (isAccountNew ? accountCreationFee : 0) +
     ///          (isAccountUpdated ? accountUpdateFee : 0)
-    ///      If the user sends too much ETH the surplus is immediatly sent back.
+    ///      If the user sends too much ETH the surplus is sent back immediately.
     ///
     ///      Note that after such an operation, it will take the operator some
     ///      time (no more than MAX_AGE_REQUEST_UNTIL_FORCED) to process the request
@@ -477,11 +477,11 @@ contract IExchange
     ///                - WalletSplitPercentage: 1 byte
     ///
     ///        The RING_SETTLEMENT data availability data is further transformed
-    ///        to make it more compressable:
+    ///        to make it more compressible:
     ///        - The Ring-matcher account ID, fee amount and token ID (the first 5 bytes) are
     ///          XORed with the corresponding data from the previous ring
     ///        - To group more similar data together we don't store all data
-    ///          for a ring next to eachother but group them together for all rings.
+    ///          for a ring next to each other but group them together for all rings.
     ///          For ALL rings, sequentially:
     ///             - Ring-matcher account ID + fee + Token ID
     ///             - orderA.orderID + orderB.orderID
@@ -497,7 +497,7 @@ contract IExchange
     ///            - Mode 1: An IDecompressor address (20 bytes) is stored after the mode byte.
     ///                      IDecompressor.decompress() will be called to decompress the following data.
     /// @param offchainData Arbitrary data for off-chain data-availability, i.e.,
-    ///        the multihash of the IPFS file that containts the block data.
+    ///        the multihash of the IPFS file that contains the block data.
     function commitBlock(
         uint8  blockType,
         uint16 blockSize,
@@ -507,7 +507,7 @@ contract IExchange
         )
         external;
 
-    /// @dev Submit a ZK proof onchain to verify a previouly committed block. Submitting an
+    /// @dev Submit a ZK proof onchain to verify a previously committed block. Submitting an
     ///      invalid proof will not change the state of the exchange. Note that proofs can
     ///      be submitted in a different order than the blocks themselves.
     ///
@@ -583,7 +583,7 @@ contract IExchange
     ///          depositFee +
     ///          (isAccountNew ? accountCreationFee : 0) +
     ///          (isAccountUpdated ? accountUpdateFee : 0)
-    ///      If the user sends too much ETH the surplus is immediatly sent back.
+    ///      If the user sends too much ETH the surplus is sent back immediately.
     ///
     ///      Note that after such an operation, it will take the operator some
     ///      time (no more than MAX_AGE_REQUEST_UNTIL_FORCED) to process the request
@@ -616,17 +616,17 @@ contract IExchange
     /// @dev Deposit Ether or ERC20 tokens to the sender's account.
     ///
     ///      The total fee in ETH that the user needs to pay is 'depositFee'.
-    ///      If the user sends too much ETH the surplus is immediatly sent back.
+    ///      If the user sends too much ETH the surplus is sent back immediately.
     ///
     ///      Note that after such an operation, it will take the operator some
     ///      time (no more than MAX_AGE_REQUEST_UNTIL_FORCED) to process the request
     ///      and create the deposit to the offchain account.
     ///
     ///      Warning: the DEX UI should warn their users not to deposit more than 2^96 - 1
     ///               tokens in total. If that happens, the user may lose token.
-    ///               This token balance upper limit, however, is large enought for most scenarios.
+    ///               This token balance upper limit, however, is large enough for most scenarios.
     ///
-    /// @param tokenAddress The adderss of the token, use `0x0` for Ether.
+    /// @param tokenAddress The address of the token, use `0x0` for Ether.
     /// @param amount The amount of tokens to deposit
     function deposit(
         address tokenAddress,
@@ -638,15 +638,15 @@ contract IExchange
     /// @dev Deposit Ether or ERC20 tokens to a recipient account.
     ///
     ///      The total fee in ETH that the user needs to pay is 'depositFee'.
-    ///      If the user sends too much ETH the surplus is immediatly sent back.
+    ///      If the user sends too much ETH the surplus is sent back immediately.
     ///
     ///      Note that after such an operation, it will take the operator some
     ///      time (no more than MAX_AGE_REQUEST_UNTIL_FORCED) to process the request
     ///      and create the deposit to the offchain account.
     ///
     ///      Warning: the DEX UI should warn their users not to deposit more than 2^96 - 1
     ///               tokens in total. If that happens, the user may lose token.
-    ///               This token balance upper limit, however, is large enought for most scenarios.
+    ///               This token balance upper limit, however, is large enough for most scenarios.
     ///
     /// @param recipient The address of the recipient
     /// @param tokenAddress The adderss of the token, use `0x0` for Ether.
@@ -699,7 +699,7 @@ contract IExchange
     ///      Only the owner of the account can request a withdrawal.
     ///
     ///      The total fee in ETH that the user needs to pay is 'withdrawalFee'.
-    ///      If the user sends too much ETH the surplus is immediatly sent back.
+    ///      If the user sends too much ETH the surplus is sent back immediately.
     ///
     ///      Note that after such an operation, it will take the operator some
     ///      time (no more than MAX_AGE_REQUEST_UNTIL_FORCED) to process the request
@@ -731,7 +731,7 @@ contract IExchange
         payable;
 
     /// @dev Allows an account owner to withdraw his funds using the balances stored
-    ///      in the merkle tree. The funds will be sent to the owner of the acount.
+    ///      in the merkle tree. The funds will be sent to the owner of the account.
     ///
     ///      Trading pubKey matching the offchain Merkle tree need to be provided.
     ///      The pubKey may already be reset to 0 when the exchange is shutdown.
@@ -824,7 +824,7 @@ contract IExchange
     /// @dev Allows withdrawing funds after a withdrawal request (either onchain
     ///      or offchain) was committed in a block by the operator.
     ///
-    ///      Can be called by anyone. The withdrawan tokens will be sent to
+    ///      Can be called by anyone. The withdrawn tokens will be sent to
     ///      the owner of the account they were withdrawn out.
     ///
     ///      Normally it is should not be needed for users to call this manually.
@@ -946,7 +946,7 @@ contract IExchange
     /// @dev Starts or continues maintenance mode for the specified duration.
     ///      The necessary additional downtime minutes will be purchased. The number of
     ///      downtime minutes still available for use can be checked with getRemainingDowntime().
-    ///      In maintainance mode, all onchain user requests, including account creation,
+    ///      In maintenance mode, all onchain user requests, including account creation,
     ///      account update, deposits, and withdrawal requests are disabled.
     ///
     ///      The remaining downtime time will be extended so that the exchange can stay in
@@ -955,7 +955,7 @@ contract IExchange
     ///      The exchange owner can exit maintenance mode by calling stopMaintenanceMode()
     ///      or by waiting until the remaining downtime is reduced to 0.
     ///
-    ///      Once entering the maintainance mode, the operator should still fulfill his duty
+    ///      Once entering the maintenance mode, the operator should still fulfill his duty
     ///      by submitting blocks and proofs until all pending user requests have been taken
     ///      care of within the required timeouts. In the maintenance mode, operator can no longer
     ///      submit settlement blocks.
@@ -1015,7 +1015,7 @@ contract IExchange
     ///
     ///      Note that the exchange can still enter the withdrawal mode after this function
     ///      has been invoked successfully. To prevent entering the withdrawal mode, exchange
-    ///      operators need to reset the Merkle tree to its initial state by doingwithdrawals
+    ///      operators need to reset the Merkle tree to its initial state by doing withdrawals
     ///      within MAX_TIME_IN_SHUTDOWN_BASE + (accounts.length * MAX_TIME_IN_SHUTDOWN_DELTA)
     ///      seconds.
     ///
@@ -1028,9 +1028,9 @@ contract IExchange
 
     /// @dev Get number of available/processed deposits/withdrawals.
     /// @return numDepositRequestsProcessed The num of the processed deposit requests
-    /// @return numAvailableDepositSlots The number of slots avalable for deposits
+    /// @return numAvailableDepositSlots The number of slots available for deposits
     /// @return numWithdrawalRequestsProcessed The num of processed withdrawal requests
-    /// @return numAvailableWithdrawalSlots The number of slots avalable for withdrawals
+    /// @return numAvailableWithdrawalSlots The number of slots available for withdrawals
     function getRequestStats()
         external
         view

packages/loopring_v3/contracts/iface/ILoopringV3.sol

@@ -148,7 +148,7 @@ contract ILoopringV3
     /// @param  onchainDataAvailability True if "Data Availability" is turned on for this
     ///         exchange. Note that this value can not be changed once the exchange is created.
     /// @return exchangeId The id of the exchange.
-    /// @return exchangeAddress The address of the newly depolyed exchange contract.
+    /// @return exchangeAddress The address of the newly deployed exchange contract.
     function createExchange(
         address payable operator,
         bool onchainDataAvailability

packages/loopring_v3/contracts/impl/BlockVerifier.sol

@@ -85,6 +85,7 @@ contract BlockVerifier is IBlockVerifier, Claimable
             vk[7], vk[8], vk[9], vk[10], vk[11], vk[12], vk[13]
         ];
         uint256[4] memory _vk_gammaABC = [vk[14], vk[15], vk[16], vk[17]];
+        // Maybe we should strip the highest bits of the hash so we don't have any overflow (uint256/prime field)
         uint256[1] memory publicInputs = [uint256(publicDataHash)];
 
         return Verifier.Verify(_vk, _vk_gammaABC, proof, publicInputs);

packages/loopring_v3/contracts/impl/Exchange.sol

@@ -666,6 +666,7 @@ contract Exchange is IExchange, Claimable, ReentrancyGuard
         require(blockIdx < state.blocks.length, "INVALID_BLOCK_IDX");
         ExchangeData.Block storage withdrawBlock = state.blocks[blockIdx];
         state.withdrawFromApprovedWithdrawal(
+            blockIdx,
             withdrawBlock,
             slotIdx,
             false

packages/loopring_v3/contracts/impl/libexchange/ExchangeAccounts.sol

@@ -16,25 +16,18 @@
 */
 pragma solidity 0.5.7;
 
-import "../../lib/AddressUtil.sol";
-import "../../lib/BurnableERC20.sol";
-import "../../lib/ERC20SafeTransfer.sol";
 import "../../lib/MathUint.sol";
 
-import "./ExchangeAccounts.sol";
 import "./ExchangeBalances.sol";
 import "./ExchangeData.sol";
-import "./ExchangeMode.sol";
 
 
 /// @title ExchangeAccounts.
 /// @author Daniel Wang  - <[email protected]>
 /// @author Brecht Devos - <[email protected]>
 library ExchangeAccounts
 {
-    using AddressUtil       for address payable;
     using MathUint          for uint;
-    using ExchangeMode      for ExchangeData.State;
     using ExchangeBalances  for ExchangeData.State;
 
     event AccountCreated(
@@ -70,7 +63,6 @@ library ExchangeAccounts
         pubKeyY = account.pubKeyY;
     }
 
-    // We do allow pubkeyX and/or pubkeyY to be 0.
     function createOrUpdateAccount(
         ExchangeData.State storage S,
         uint pubKeyX,
@@ -161,7 +153,7 @@ library ExchangeAccounts
         returns (uint24 accountID)
     {
         accountID = S.ownerToAccountId[owner];
-        require(accountID != 0, "SENDER_HAS_NO_ACCOUNT");
+        require(accountID != 0, "ADDRESS_HAS_NO_ACCOUNT");
 
         accountID = accountID - 1;
     }

packages/loopring_v3/contracts/impl/libexchange/ExchangeAdmins.sol

@@ -25,7 +25,7 @@ import "./ExchangeData.sol";
 import "./ExchangeMode.sol";
 
 
-/// @title ExchangeAccounts.
+/// @title ExchangeAdmins.
 /// @author Daniel Wang  - <[email protected]>
 /// @author Brecht Devos - <[email protected]>
 library ExchangeAdmins
@@ -210,8 +210,8 @@ library ExchangeAdmins
 
         // Exchange needs to be shutdown
         require(S.isShutdown(), "EXCHANGE_NOT_SHUTDOWN");
-        // Last block needs to be finalized
-        require(lastBlock.state == ExchangeData.BlockState.FINALIZED, "BLOCK_NOT_FINALIZED");
+        // All blocks needs to be finalized
+        require(S.blocks.length == S.numBlocksFinalized, "BLOCK_NOT_FINALIZED");
         // We also require that all deposit requests are processed
         require(lastBlock.numDepositRequestsCommitted == S.depositChain.length, "DEPOSITS_NOT_PROCESSED");
         // Merkle root needs to be reset to the genesis block

packages/loopring_v3/contracts/impl/libexchange/ExchangeBalances.sol

@@ -21,7 +21,7 @@ import "../../lib/MathUint.sol";
 import "../../thirdparty/MiMC.sol";
 
 
-/// @title IManagingMode.
+/// @title ExchangeBalances.
 /// @author Daniel Wang  - <[email protected]>
 /// @author Brecht Devos - <[email protected]>
 library ExchangeBalances

packages/loopring_v3/contracts/impl/libexchange/ExchangeBlocks.sol

@@ -16,9 +16,7 @@
 */
 pragma solidity 0.5.7;
 
-import "../../lib/BurnableERC20.sol";
 import "../../lib/BytesUtil.sol";
-import "../../lib/ERC20SafeTransfer.sol";
 import "../../lib/MathUint.sol";
 
 import "../../iface/IBlockVerifier.sol";
@@ -108,6 +106,7 @@ library ExchangeBlocks
             "PROOF_TOO_LATE"
         );
 
+        // Verify the proof
         require(
             S.blockVerifier.verifyProof(
                 uint8(specifiedBlock.blockType),
@@ -120,29 +119,21 @@ library ExchangeBlocks
             "INVALID_PROOF"
         );
 
-        // Update state of this block and potentially the following blocks
-        ExchangeData.Block storage previousBlock = S.blocks[blockIdx - 1];
-        if (previousBlock.state == ExchangeData.BlockState.FINALIZED) {
-            specifiedBlock.state = ExchangeData.BlockState.FINALIZED;
-            S.numBlocksFinalized = blockIdx + 1;
-            emit BlockFinalized(blockIdx);
-            // The next blocks could become finalized as well so check this now
-            // The number of blocks after the specified block index is limited
-            // by MAX_GAP_BETWEEN_FINALIZED_AND_VERIFIED_BLOCKS
-            // so we don't have to worry about running out of gas in this loop
-            uint nextBlockIdx = blockIdx + 1;
-            while (nextBlockIdx < S.blocks.length &&
-                S.blocks[nextBlockIdx].state == ExchangeData.BlockState.VERIFIED) {
-
-                S.blocks[nextBlockIdx].state = ExchangeData.BlockState.FINALIZED;
-                S.numBlocksFinalized = nextBlockIdx + 1;
-                emit BlockFinalized(nextBlockIdx);
-                nextBlockIdx++;
-            }
-        } else {
-            specifiedBlock.state = ExchangeData.BlockState.VERIFIED;
-            emit BlockVerified(blockIdx);
+        // Mark the block as verified
+        specifiedBlock.state = ExchangeData.BlockState.VERIFIED;
+        emit BlockVerified(blockIdx);
+
+        // Update the number of blocks that are finalized
+        // The number of blocks after the specified block index is limited
+        // by MAX_GAP_BETWEEN_FINALIZED_AND_VERIFIED_BLOCKS
+        // so we don't have to worry about running out of gas in this loop
+        uint idx = S.numBlocksFinalized;
+        while (idx < S.blocks.length &&
+            S.blocks[idx].state == ExchangeData.BlockState.VERIFIED) {
+            emit BlockFinalized(idx);
+            idx++;
         }
+        S.numBlocksFinalized = idx;
     }
 
     function revertBlock(
@@ -159,9 +150,8 @@ library ExchangeBlocks
         require(specifiedBlock.state == ExchangeData.BlockState.COMMITTED, "INVALID_BLOCK_STATE");
 
         // The specified block needs to be the first block not finalized
-        // (this way we always revert to a guaranteed valid block and don't need to revert multiple times)
-        ExchangeData.Block storage previousBlock = S.blocks[uint(blockIdx).sub(1)];
-        require(previousBlock.state == ExchangeData.BlockState.FINALIZED, "PREV_BLOCK_NOT_FINALIZED");
+        // (this way we always revert to a guaranteed valid block and don't revert multiple times)
+        require(blockIdx == S.numBlocksFinalized, "PREV_BLOCK_NOT_FINALIZED");
 
         // Check if this block is verified too late
         require(

packages/loopring_v3/contracts/impl/libexchange/ExchangeData.sol

@@ -23,7 +23,6 @@ import "../../iface/ILoopringV3.sol";
 /// @title ExchangeData
 /// @author Daniel Wang  - <[email protected]>
 /// @author Brecht Devos - <[email protected]>
-
 library ExchangeData
 {
     // -- Enums --
@@ -47,12 +46,8 @@ library ExchangeData
         COMMITTED,      // = 1
 
         // A valid ZK proof has been submitted for this block.
-        VERIFIED,       // = 2
-
-        // A block's state will become FINALIZED when and only when this block is VERIFIED
-        // and all previous blocks in the chain have become FINALIZED. The genesis block is
-        // FINALIZED by default.
-        FINALIZED       // = 3
+        // The genesis block is VERIFIED by default.
+        VERIFIED        // = 2
     }
 
     // -- Structs --
@@ -167,7 +162,7 @@ library ExchangeData
     }
 
     function MAX_PROOF_GENERATION_TIME_IN_SECONDS() internal pure returns (uint32) { return 1 hours; }
-    function MAX_GAP_BETWEEN_FINALIZED_AND_VERIFIED_BLOCKS() internal pure returns (uint32) { return 1000; }
+    function MAX_GAP_BETWEEN_FINALIZED_AND_VERIFIED_BLOCKS() internal pure returns (uint32) { return 2500; }
     function MAX_OPEN_DEPOSIT_REQUESTS() internal pure returns (uint16) { return 1024; }
     function MAX_OPEN_WITHDRAWAL_REQUESTS() internal pure returns (uint16) { return 1024; }
     function MAX_AGE_UNFINALIZED_BLOCK_UNTIL_WITHDRAW_MODE() internal pure returns (uint32) { return 1 days; }
@@ -221,6 +216,9 @@ library ExchangeData
         // A map from an account owner to a token to if the balance is withdrawn
         mapping (address => mapping (address => bool)) withdrawnInWithdrawMode;
 
+        // A block's state will become FINALIZED when and only when this block is VERIFIED
+        // and all previous blocks in the chain have become FINALIZED.
+        // The genesis block is FINALIZED by default.
         uint numBlocksFinalized;
 
         // Cached data for the protocol fee

packages/loopring_v3/contracts/impl/libexchange/ExchangeDeposits.sol

@@ -16,6 +16,7 @@
 */
 pragma solidity 0.5.7;
 
+import "../../lib/AddressUtil.sol";
 import "../../lib/BurnableERC20.sol";
 import "../../lib/ERC20SafeTransfer.sol";
 
@@ -25,7 +26,7 @@ import "./ExchangeMode.sol";
 import "./ExchangeTokens.sol";
 
 
-/// @title ExchangeAccounts.
+/// @title ExchangeDeposits.
 /// @author Daniel Wang  - <[email protected]>
 /// @author Brecht Devos - <[email protected]>
 library ExchangeDeposits
@@ -109,6 +110,7 @@ library ExchangeDeposits
         // Total fee to be paid by the user
         uint feeETH = additionalFeeETH.add(S.depositFeeETH);
 
+        // Transfer the tokens to this contract
         transferDeposit(
             account.owner,
             tokenAddress,