refactor(contracts): add natspec across contracts and cleanup

contracts/contracts/MACI.sol

@@ -1,22 +1,25 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.10;
 
-import { Poll, PollFactory } from "./Poll.sol";
+import { Poll } from "./Poll.sol";
+import { PollFactory } from "./PollFactory.sol";
 import { InitialVoiceCreditProxy } from "./initialVoiceCreditProxy/InitialVoiceCreditProxy.sol";
 import { SignUpGatekeeper } from "./gatekeepers/SignUpGatekeeper.sol";
 import { AccQueue, AccQueueQuinaryBlankSl } from "./trees/AccQueue.sol";
-import { IMACI } from "./IMACI.sol";
-import { Params } from "./Params.sol";
-import { DomainObjs } from "./DomainObjs.sol";
+import { IMACI } from "./interfaces/IMACI.sol";
+import { Params } from "./utilities/Params.sol";
+import { DomainObjs } from "./utilities/DomainObjs.sol";
 import { VkRegistry } from "./VkRegistry.sol";
 import { TopupCredit } from "./TopupCredit.sol";
 import { SnarkCommon } from "./crypto/SnarkCommon.sol";
 import { SnarkConstants } from "./crypto/SnarkConstants.sol";
-
+import { Hasher } from "./crypto/Hasher.sol";
+import { Utilities } from "./utilities/Utilities.sol";
 import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
 
 /// @title MACI - Minimum Anti-Collusion Infrastructure Version 1
-contract MACI is IMACI, DomainObjs, Params, SnarkCommon, Ownable {
+/// @notice A contract which allows users to sign up, and deploy new polls
+contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {
   /// @notice The state tree depth is fixed. As such it should be as large as feasible
   /// so that there can be as many users as possible.  i.e. 5 ** 10 = 9765625
   /// this should also match the parameter of the circom circuits.
@@ -59,7 +62,7 @@ contract MACI is IMACI, DomainObjs, Params, SnarkCommon, Ownable {
   PollFactory public pollFactory;
 
   /// @notice The state AccQueue. Represents a mapping between each user's public key
-  /// @notice and their voice credit balance.
+  /// and their voice credit balance.
   AccQueue public override stateAq;
 
   /// @notice Whether the init() function has been successfully executed yet.
@@ -111,6 +114,11 @@ contract MACI is IMACI, DomainObjs, Params, SnarkCommon, Ownable {
   error PreviousPollNotCompleted(uint256 pollId);
   error PollDoesNotExist(uint256 pollId);
 
+  /// @notice Create a new instance of the MACI contract.
+  /// @param _pollFactory The PollFactory contract
+  /// @param _signUpGatekeeper The SignUpGatekeeper contract
+  /// @param _initialVoiceCreditProxy The InitialVoiceCreditProxy contract
+  /// @param _stateTreeDepth The depth of the state tree
   constructor(
     PollFactory _pollFactory,
     SignUpGatekeeper _signUpGatekeeper,
@@ -199,14 +207,16 @@ contract MACI is IMACI, DomainObjs, Params, SnarkCommon, Ownable {
 
   /// @notice Deploy a new Poll contract.
   /// @param _duration How long should the Poll last for
+  /// @param _maxValues The maximum number of vote options, and messages
   /// @param _treeDepths The depth of the Merkle trees
-  /// @return a new Poll contract address
+  /// @param _coordinatorPubKey The coordinator's public key
+  /// @return pollAddr a new Poll contract address
   function deployPoll(
     uint256 _duration,
     MaxValues memory _maxValues,
     TreeDepths memory _treeDepths,
     PubKey memory _coordinatorPubKey
-  ) public afterInit onlyOwner returns (address) {
+  ) public afterInit onlyOwner returns (address pollAddr) {
     uint256 pollId = nextPollId;
 
     // Increment the poll ID for the next poll
@@ -240,9 +250,9 @@ contract MACI is IMACI, DomainObjs, Params, SnarkCommon, Ownable {
 
     polls[pollId] = p;
 
-    emit DeployPoll(pollId, address(p), _coordinatorPubKey);
+    pollAddr = address(p);
 
-    return address(p);
+    emit DeployPoll(pollId, pollAddr, _coordinatorPubKey);
   }
 
   /// @notice Allow Poll contracts to merge the state subroots
@@ -256,26 +266,24 @@ contract MACI is IMACI, DomainObjs, Params, SnarkCommon, Ownable {
 
   /// @notice Allow Poll contracts to merge the state root
   /// @param _pollId The active Poll ID
-  /// @return uint256 The calculated Merkle root
-  function mergeStateAq(uint256 _pollId) public override onlyPoll(_pollId) afterInit returns (uint256) {
-    uint256 root = stateAq.merge(stateTreeDepth);
+  /// @return root The calculated Merkle root
+  function mergeStateAq(uint256 _pollId) public override onlyPoll(_pollId) afterInit returns (uint256 root) {
+    root = stateAq.merge(stateTreeDepth);
 
     emit MergeStateAq(_pollId);
-
-    return root;
   }
 
   /// @notice Return the main root of the StateAq contract
-  /// @return uint256 The Merkle root
-  function getStateAqRoot() public view override returns (uint256) {
-    return stateAq.getMainRoot(stateTreeDepth);
+  /// @return root The Merkle root
+  function getStateAqRoot() public view override returns (uint256 root) {
+    root = stateAq.getMainRoot(stateTreeDepth);
   }
 
   /// @notice Get the Poll details
   /// @param _pollId The identifier of the Poll to retrieve
-  /// @return Poll The Poll data
-  function getPoll(uint256 _pollId) public view returns (Poll) {
+  /// @return poll The Poll contract object
+  function getPoll(uint256 _pollId) public view returns (Poll poll) {
     if (_pollId >= nextPollId) revert PollDoesNotExist(_pollId);
-    return polls[_pollId];
+    poll = polls[_pollId];
   }
 }

contracts/contracts/MessageProcessor.sol

@@ -2,20 +2,21 @@
 pragma solidity ^0.8.10;
 
 import { AccQueue } from "./trees/AccQueue.sol";
-import { IMACI } from "./IMACI.sol";
+import { IMACI } from "./interfaces/IMACI.sol";
 import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
 import { Poll } from "./Poll.sol";
 import { SnarkCommon } from "./crypto/SnarkCommon.sol";
 import { Hasher } from "./crypto/Hasher.sol";
-import { CommonUtilities } from "./utilities/Utility.sol";
+import { CommonUtilities } from "./utilities/Utilities.sol";
 import { Verifier } from "./crypto/Verifier.sol";
 import { VkRegistry } from "./VkRegistry.sol";
 
 /// @title MessageProcessor
-/// @dev MessageProcessor is used to process messages published by signup users
-/// it will process message by batch due to large size of messages
-/// after it finishes processing, the sbCommitment will be used for Tally and Subsidy contracts
+/// @dev MessageProcessor is used to process messages published by signup users.
+/// It will process message by batch due to large size of messages.
+/// After it finishes processing, the sbCommitment will be used for Tally and Subsidy contracts.
 contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
+  /// @notice custom errors
   error NoMoreMessages();
   error StateAqNotMerged();
   error MessageAqNotMerged();
@@ -39,6 +40,8 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
 
   Verifier public verifier;
 
+  /// @notice Create a new instance
+  /// @param _verifier The Verifier contract address
   constructor(Verifier _verifier) {
     verifier = _verifier;
   }
@@ -124,14 +127,23 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
     }
   }
 
+  /// @notice Verify the proof for processMessage
+  /// @dev used to update the sbCommitment
+  /// @param _poll The Poll contract address
+  /// @param _currentMessageBatchIndex The batch index of current message batch
+  /// @param _messageRoot The message tree root
+  /// @param _currentSbCommitment The current sbCommitment (state and ballot)
+  /// @param _newSbCommitment The new sbCommitment after we update this message batch
+  /// @param _proof The zk-SNARK proof
+  /// @return isValid Whether the proof is valid
   function verifyProcessProof(
     Poll _poll,
     uint256 _currentMessageBatchIndex,
     uint256 _messageRoot,
     uint256 _currentSbCommitment,
     uint256 _newSbCommitment,
     uint256[8] memory _proof
-  ) internal view returns (bool) {
+  ) internal view returns (bool isValid) {
     (, , uint8 messageTreeDepth, uint8 voteOptionTreeDepth) = _poll.treeDepths();
     (uint256 messageBatchSize, , ) = _poll.batchSizes();
     (uint256 numSignUps, ) = _poll.numSignUpsAndMessages();
@@ -159,7 +171,7 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
       messageBatchSize
     );
 
-    return verifier.verify(_proof, vk, publicInputHash);
+    isValid = verifier.verify(_proof, vk, publicInputHash);
   }
 
   /// @notice Returns the SHA256 hash of the packed values (see
@@ -168,21 +180,21 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
   /// ballot root. By passing the SHA256 hash of these values to the circuit
   /// as a single public input and the preimage as private inputs, we reduce
   /// its verification gas cost though the number of constraints will be
-  /// higher and proving time will be higher.
-  /// @param _poll: contract address
-  /// @param _currentMessageBatchIndex: batch index of current message batch
-  /// @param _numSignUps: number of users that signup
-  /// @param _currentSbCommitment: current sbCommitment
-  /// @param _newSbCommitment: new sbCommitment after we update this message batch
-  /// @return returns the SHA256 hash of the packed values
+  /// higher and proving time will be longer.
+  /// @param _poll The Poll contract address
+  /// @param _currentMessageBatchIndex The batch index of current message batch
+  /// @param _numSignUps The number of users that signup
+  /// @param _currentSbCommitment The current sbCommitment (state and ballot root)
+  /// @param _newSbCommitment The new sbCommitment after we update this message batch
+  /// @return inputHash Returns the SHA256 hash of the packed values
   function genProcessMessagesPublicInputHash(
     Poll _poll,
     uint256 _currentMessageBatchIndex,
     uint256 _messageRoot,
     uint256 _numSignUps,
     uint256 _currentSbCommitment,
     uint256 _newSbCommitment
-  ) public view returns (uint256) {
+  ) public view returns (uint256 inputHash) {
     uint256 coordinatorPubKeyHash = _poll.coordinatorPubKeyHash();
 
     uint256 packedVals = genProcessMessagesPackedVals(_poll, _currentMessageBatchIndex, _numSignUps);
@@ -196,24 +208,23 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
     input[3] = _currentSbCommitment;
     input[4] = _newSbCommitment;
     input[5] = deployTime + duration;
-    uint256 inputHash = sha256Hash(input);
-
-    return inputHash;
+    inputHash = sha256Hash(input);
   }
 
   /// @notice One of the inputs to the ProcessMessages circuit is a 250-bit
   /// representation of four 50-bit values. This function generates this
   /// 250-bit value, which consists of the maximum number of vote options, the
   /// number of signups, the current message batch index, and the end index of
   /// the current batch.
-  /// @param _poll: the poll contract
-  /// @param _currentMessageBatchIndex: batch index of current message batch
-  /// @param _numSignUps: number of users that signup
+  /// @param _poll the poll contract
+  /// @param _currentMessageBatchIndex batch index of current message batch
+  /// @param _numSignUps number of users that signup
+  /// @return result The packed value
   function genProcessMessagesPackedVals(
     Poll _poll,
     uint256 _currentMessageBatchIndex,
     uint256 _numSignUps
-  ) public view returns (uint256) {
+  ) public view returns (uint256 result) {
     (, uint256 maxVoteOptions) = _poll.maxValues();
     (, uint256 numMessages) = _poll.numSignUpsAndMessages();
     (uint24 mbs, , ) = _poll.batchSizes();
@@ -229,15 +240,13 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
     if (_currentMessageBatchIndex >= 2 ** 50) revert CurrentMessageBatchIndexTooLarge();
     if (batchEndIndex >= 2 ** 50) revert BatchEndIndexTooLarge();
 
-    uint256 result = maxVoteOptions + (_numSignUps << 50) + (_currentMessageBatchIndex << 100) + (batchEndIndex << 150);
-
-    return result;
+    result = maxVoteOptions + (_numSignUps << 50) + (_currentMessageBatchIndex << 100) + (batchEndIndex << 150);
   }
 
   /// @notice update message processing state variables
-  /// @param _newSbCommitment: sbCommitment to be updated
-  /// @param _currentMessageBatchIndex: currentMessageBatchIndex to be updated
-  /// @param _processingComplete: update flag that indicate processing is finished or not
+  /// @param _newSbCommitment sbCommitment to be updated
+  /// @param _currentMessageBatchIndex currentMessageBatchIndex to be updated
+  /// @param _processingComplete update flag that indicate processing is finished or not
   function updateMessageProcessingData(
     uint256 _newSbCommitment,
     uint256 _currentMessageBatchIndex,