tests, more docs, minor refactoring

contracts/ERC20Mixer.sol

@@ -25,21 +25,21 @@ contract ERC20Mixer is Mixer {
     uint256 _emptyElement,
     address payable _operator,
     address _token,
-    uint256 _mixDenomination
+    uint256 _denomination
-  ) Mixer(_verifier, _mixDenomination, _merkleTreeHeight, _emptyElement, _operator) public {
+  ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public {
     token = _token;
     userEther = _userEther;
   }
 
   function _processDeposit() internal {
     require(msg.value == userEther, "Please send `userEther` ETH along with transaction");
-    safeErc20TransferFrom(msg.sender, address(this), mixDenomination);
+    safeErc20TransferFrom(msg.sender, address(this), denomination);
   }
 
   function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal {
     _receiver.transfer(userEther);
 
-    safeErc20Transfer(_receiver, mixDenomination - _fee);
+    safeErc20Transfer(_receiver, denomination - _fee);
     if (_fee > 0) {
       safeErc20Transfer(_relayer, _fee);
     }

contracts/ETHMixer.sol

@@ -16,21 +16,21 @@ import "./Mixer.sol";
 contract ETHMixer is Mixer {
   constructor(
     address _verifier,
-    uint256 _mixDenomination,
+    uint256 _denomination,
     uint8 _merkleTreeHeight,
     uint256 _emptyElement,
     address payable _operator
-  ) Mixer(_verifier, _mixDenomination, _merkleTreeHeight, _emptyElement, _operator) public {
+  ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public {
   }
 
   function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal {
-    _receiver.transfer(mixDenomination - _fee);
+    _receiver.transfer(denomination - _fee);
     if (_fee > 0) {
       _relayer.transfer(_fee);
     }
   }
 
   function _processDeposit() internal {
-    require(msg.value == mixDenomination, "Please send `mixDenomination` ETH along with transaction");
+    require(msg.value == denomination, "Please send `mixDenomination` ETH along with transaction");
   }
 }

contracts/Mixer.sol

@@ -18,16 +18,23 @@ contract IVerifier {
 }
 
 contract Mixer is MerkleTreeWithHistory {
-  bool public isDepositsEnabled = true;
+  uint256 public denomination;
-  bool public isVerifierUpdateAllowed = true;
-  // operator can disable new deposits in case of emergency
-  // it also receives a relayer fee
-  address payable public operator;
   mapping(uint256 => bool) public nullifierHashes;
   // we store all commitments just to prevent accidental deposits with the same commitment
   mapping(uint256 => bool) public commitments;
   IVerifier public verifier;
-  uint256 public mixDenomination;
+
+  // operator can
+  //  - receive a relayer fee
+  //  - disable new deposits in case of emergency
+  //  - update snark verification key until this ability is permanently disabled
+  address payable public operator;
+  bool public isDepositsEnabled = true;
+  bool public isVerifierUpdateAllowed = true;
+  modifier onlyOperator {
+    require(msg.sender == operator, "Only operator can call this function.");
+    _;
+  }
 
   event Deposit(uint256 indexed commitment, uint256 leafIndex, uint256 timestamp);
   event Withdraw(address to, uint256 nullifierHash, address indexed relayer, uint256 fee);
@@ -41,26 +48,22 @@ contract Mixer is MerkleTreeWithHistory {
   */
   constructor(
     address _verifier,
-    uint256 _mixDenomination,
+    uint256 _denomination,
     uint8 _merkleTreeHeight,
     uint256 _emptyElement,
     address payable _operator
   ) MerkleTreeWithHistory(_merkleTreeHeight, _emptyElement) public {
     verifier = IVerifier(_verifier);
     operator = _operator;
-    mixDenomination = _mixDenomination;
+    denomination = _denomination;
   }
+
   /**
-    @dev Deposit funds into mixer. The caller must send value equal to `mixDenomination` of this mixer.
+    @dev Deposit funds into mixer. The caller must send (for ETH) or approve (for ERC20) value equal to or `denomination` of this mixer.
-    @param commitment the note commitment, which is PedersenHash(nullifier + secret)
-  */
-  /**
-    @dev Deposit funds into the mixer. The caller must send ETH value equal to `userEther` of this mixer.
-    The caller also has to have at least `mixDenomination` amount approved for the mixer.
     @param commitment the note commitment, which is PedersenHash(nullifier + secret)
   */
   function deposit(uint256 commitment) public payable {
-    require(isDepositsEnabled, "deposits disabled");
+    require(isDepositsEnabled, "deposits are disabled");
     require(!commitments[commitment], "The commitment has been submitted");
     _processDeposit();
     _insert(commitment);
@@ -68,6 +71,10 @@ contract Mixer is MerkleTreeWithHistory {
 
     emit Deposit(commitment, next_index - 1, block.timestamp);
   }
+
+  /** @dev this function is defined in a child contract */
+  function _processDeposit() internal {}
+
   /**
     @dev Withdraw deposit from the mixer. `a`, `b`, and `c` are zkSNARK proof data, and input is an array of circuit public inputs
     `input` array consists of:
@@ -82,7 +89,7 @@ contract Mixer is MerkleTreeWithHistory {
     address payable receiver = address(input[2]);
     address payable relayer = address(input[3]);
     uint256 fee = input[4];
-    require(fee < mixDenomination, "Fee exceeds transfer value");
+    require(fee < denomination, "Fee exceeds transfer value");
     require(!nullifierHashes[nullifierHash], "The note has been already spent");
 
     require(isKnownRoot(root), "Cannot find your merkle root"); // Make sure to use a recent one
@@ -92,32 +99,41 @@ contract Mixer is MerkleTreeWithHistory {
     emit Withdraw(receiver, nullifierHash, relayer, fee);
   }
 
-  function toggleDeposits() external {
+  /** @dev this function is defined in a child contract */
-    require(msg.sender == operator, "unauthorized");
+  function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal {}
-    isDepositsEnabled = !isDepositsEnabled;
-  }
-
-  function updateVerifier(address newVerifier) external {
-    require(isVerifierUpdateAllowed, "verifier updates are disabled");
-    require(msg.sender == operator, "unauthorized");
-    verifier = IVerifier(newVerifier);
-  }
-
-  function disableVerifierUpdate() external {
-    require(msg.sender == operator, "unauthorized");
-    isVerifierUpdateAllowed = false;
-  }
-
-  function changeOperator(address payable _newAccount) external {
-    require(msg.sender == operator, "unauthorized");
-    operator = _newAccount;
-  }
 
+  /** @dev whether a note is already spent */
   function isSpent(uint256 nullifier) public view returns(bool) {
     return nullifierHashes[nullifier];
   }
 
-  function _processDeposit() internal {}
+  /**
-  function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal {}
+    @dev Allow operator to temporarily disable new deposits. This is needed to protect users funds in case a vulnerability is discovered.
+    It does not affect existing deposits.
+  */
+  function toggleDeposits() external onlyOperator {
+    isDepositsEnabled = !isDepositsEnabled;
+  }
 
+  /**
+    @dev allow operator to update SNARK verification keys. This is needed to update keys after the final trusted setup ceremony is held.
+    After that operator is supposed to permanently disable this ability.
+  */
+  function updateVerifier(address newVerifier) external onlyOperator {
+    require(isVerifierUpdateAllowed, "Verifier updates have been disabled.");
+    verifier = IVerifier(newVerifier);
+  }
+
+  /**
+    @dev an option for operator to permanently disable verification keys update ability.
+    This is supposed to be called after the final trusted setup ceremony is held.
+  */
+  function disableVerifierUpdate() external onlyOperator {
+    isVerifierUpdateAllowed = false;
+  }
+
+  /** @dev operator can change his address */
+  function changeOperator(address payable _newAccount) external onlyOperator {
+    operator = _newAccount;
+  }
 }

test/ETHMixer.test.js

@@ -90,7 +90,7 @@ contract('ETHMixer', accounts => {
 
   describe('#constructor', () => {
     it('should initialize', async () => {
-      const etherDenomination = await mixer.mixDenomination()
+      const etherDenomination = await mixer.denomination()
       etherDenomination.should.be.eq.BN(toBN(value))
     })
   })
@@ -116,11 +116,11 @@ contract('ETHMixer', accounts => {
       let commitment = 42;
       (await mixer.isDepositsEnabled()).should.be.equal(true)
       const err = await mixer.toggleDeposits({ from: accounts[1] }).should.be.rejected
-      err.reason.should.be.equal('unauthorized')
+      err.reason.should.be.equal('Only operator can call this function.')
       await mixer.toggleDeposits({ from: sender });
       (await mixer.isDepositsEnabled()).should.be.equal(false)
       let error = await mixer.deposit(commitment, { value, from: sender }).should.be.rejected
-      error.reason.should.be.equal('deposits disabled')
+      error.reason.should.be.equal('deposits are disabled')
     })
 
     it('should throw if there is a such commitment', async () => {
@@ -416,11 +416,66 @@ contract('ETHMixer', accounts => {
 
       const newOperator = accounts[7]
       const error = await mixer.changeOperator(newOperator, { from:  accounts[7] }).should.be.rejected
-      error.reason.should.be.equal('unauthorized')
+      error.reason.should.be.equal('Only operator can call this function.')
 
     })
   })
 
+  describe('#updateVerifier', () => {
+    it('should work', async () => {
+      let operator = await mixer.operator()
+      operator.should.be.equal(sender)
+
+      const newVerifier = accounts[7]
+      await mixer.updateVerifier(newVerifier).should.be.fulfilled
+
+      verifier = await mixer.verifier()
+      verifier.should.be.equal(newVerifier)
+    })
+
+    it('cannot change from different address', async () => {
+      let operator = await mixer.operator()
+      operator.should.be.equal(sender)
+
+      const newVerifier = accounts[7]
+      const error = await mixer.updateVerifier(newVerifier, { from:  accounts[7] }).should.be.rejected
+      error.reason.should.be.equal('Only operator can call this function.')
+
+    })
+  })
+
+  describe('#disableVerifierUpdate', () => {
+    it('should work', async () => {
+      let operator = await mixer.operator()
+      operator.should.be.equal(sender)
+
+      let isVerifierUpdateAllowed = await mixer.isVerifierUpdateAllowed()
+      isVerifierUpdateAllowed.should.be.equal(true)
+
+      await mixer.disableVerifierUpdate().should.be.fulfilled
+
+      newValue = await mixer.isVerifierUpdateAllowed()
+      newValue.should.be.equal(false)
+    })
+
+    it('cannot update verifier after this function is called', async () => {
+      let operator = await mixer.operator()
+      operator.should.be.equal(sender)
+
+      let isVerifierUpdateAllowed = await mixer.isVerifierUpdateAllowed()
+      isVerifierUpdateAllowed.should.be.equal(true)
+
+      await mixer.disableVerifierUpdate().should.be.fulfilled
+
+      newValue = await mixer.isVerifierUpdateAllowed()
+      newValue.should.be.equal(false)
+
+      const newVerifier = accounts[7]
+      const error = await mixer.updateVerifier(newVerifier).should.be.rejected
+      error.reason.should.be.equal('Verifier updates have been disabled.')
+    })
+  })
+
   afterEach(async () => {
     await revertSnapshot(snapshotId.result)
     // eslint-disable-next-line require-atomic-updates