refactor(contracts): optimize code by using immutable variables are r…

…emoving redundant code

contracts/contracts/MACI.sol

@@ -38,31 +38,27 @@ contract MACI is IMACI, Params, Utilities, Ownable {
   mapping(uint256 => Poll) public polls;
 
   /// @notice The number of signups
-  uint256 public override numSignUps;
+  uint256 public numSignUps;
 
   /// @notice A mapping of block timestamps to the number of state leaves
   mapping(uint256 => uint256) public numStateLeaves;
 
   // ERC20 contract that hold topup credits
-  TopupCredit public topupCredit;
+  TopupCredit public immutable topupCredit;
 
-  PollFactory public pollFactory;
+  PollFactory public immutable pollFactory;
 
   /// @notice The state AccQueue. Represents a mapping between each user's public key
   /// and their voice credit balance.
-  AccQueue public override stateAq;
+  AccQueue public immutable stateAq;
 
   /// @notice Address of the SignUpGatekeeper, a contract which determines whether a
   /// user may sign up to vote
-  SignUpGatekeeper public signUpGatekeeper;
+  SignUpGatekeeper public immutable signUpGatekeeper;
 
   /// @notice  The contract which provides the values of the initial voice credit
   /// balance per user
-  InitialVoiceCreditProxy public initialVoiceCreditProxy;
-
-  /// @notice  When the contract was deployed. We assume that the signup period starts
-  /// immediately upon deployment.
-  uint256 public signUpTimestamp;
+  InitialVoiceCreditProxy public immutable initialVoiceCreditProxy;
 
   // Events
   event SignUp(uint256 _stateIndex, PubKey _userPubKey, uint256 _voiceCreditBalance, uint256 _timestamp);
@@ -110,8 +106,6 @@ contract MACI is IMACI, Params, Utilities, Ownable {
     initialVoiceCreditProxy = _initialVoiceCreditProxy;
     STATE_TREE_DEPTH = _stateTreeDepth;
 
-    signUpTimestamp = block.timestamp;
-
     // Verify linked poseidon libraries
     if (hash2([uint256(1), uint256(1)]) == 0) revert PoseidonHashLibrariesNotLinked();
   }

contracts/contracts/MessageProcessor.sol

@@ -38,8 +38,8 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
   /// @notice The commitment to the state and ballot roots
   uint256 public sbCommitment;
 
-  Verifier public verifier;
-  VkRegistry public vkRegistry;
+  Verifier public immutable verifier;
+  VkRegistry public immutable vkRegistry;
 
   /// @notice Create a new instance
   /// @param _verifier The Verifier contract address

contracts/contracts/Poll.sol

@@ -18,17 +18,18 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
   using SafeERC20 for ERC20;
 
   bool internal isInit;
+
   // The coordinator's public key
   PubKey public coordinatorPubKey;
 
   uint256 public mergedStateRoot;
-  uint256 public coordinatorPubKeyHash;
+  uint256 public immutable coordinatorPubKeyHash;
 
   // The timestamp of the block at which the Poll was deployed
-  uint256 internal deployTime;
+  uint256 internal immutable deployTime;
 
   // The duration of the polling period, in seconds
-  uint256 internal duration;
+  uint256 internal immutable duration;
 
   // Whether the MACI contract's stateAq has been merged by this contract
   bool public stateAqMerged;
@@ -47,6 +48,7 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
   MaxValues public maxValues;
   TreeDepths public treeDepths;
   BatchSizes public batchSizes;
+  ExtContracts public extContracts;
 
   /// @notice custom errors
   error VotingPeriodOver();
@@ -64,8 +66,6 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
   event MergeMessageAqSubRoots(uint256 _numSrQueueOps);
   event MergeMessageAq(uint256 _messageRoot);
 
-  ExtContracts public extContracts;
-
   /// @notice Each MACI instance can have multiple Polls.
   /// When a Poll is deployed, its voting period starts immediately.
   /// @param _duration The duration of the voting period, in seconds
@@ -83,14 +83,13 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
     ExtContracts memory _extContracts
   ) payable {
     extContracts = _extContracts;
-
     coordinatorPubKey = _coordinatorPubKey;
+    // we hash it ourselves to ensure we record the correct value
     coordinatorPubKeyHash = hashLeftRight(_coordinatorPubKey.x, _coordinatorPubKey.y);
     duration = _duration;
     maxValues = _maxValues;
     batchSizes = _batchSizes;
     treeDepths = _treeDepths;
-
     // Record the current timestamp
     deployTime = block.timestamp;
   }
@@ -140,10 +139,12 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
     // we check that we do not exceed the max number of messages
     if (numMessages == maxValues.maxMessages) revert TooManyMessages();
 
+    // cannot realistically overflow
     unchecked {
       numMessages++;
     }
 
+    /// @notice topupCredit is a trusted token contract which reverts if the transfer fails
     extContracts.topupCredit.transferFrom(msg.sender, address(this), amount);
     uint256[2] memory dat;
     dat[0] = stateIndex;
@@ -169,11 +170,12 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
       revert MaciPubKeyLargerThanSnarkFieldSize();
     }
 
+    // cannot realistically overflow
     unchecked {
       numMessages++;
     }
 
-    // force the message to have type 1
+    // force the message to have type 1 just to be safe
     _message.msgType = 1;
     uint256 messageLeaf = hashMessageAndEncPubKey(_message, _encPubKey);
     extContracts.messageAq.enqueue(messageLeaf);
@@ -188,9 +190,8 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
     // This function cannot be called after the stateAq was merged
     if (stateAqMerged) revert StateAqAlreadyMerged();
 
-    if (!extContracts.maci.stateAq().subTreesMerged()) {
-      extContracts.maci.mergeStateAqSubRoots(_numSrQueueOps, _pollId);
-    }
+    // merge subroots
+    extContracts.maci.mergeStateAqSubRoots(_numSrQueueOps, _pollId);
 
     emit MergeMaciStateAqSubRoots(_numSrQueueOps);
   }
@@ -206,6 +207,7 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
 
     stateAqMerged = true;
 
+    // the subtrees must have been merged first
     if (!extContracts.maci.stateAq().subTreesMerged()) revert StateAqSubtreesNeedMerge();
 
     mergedStateRoot = extContracts.maci.mergeStateAq(_pollId);

contracts/contracts/Subsidy.sol

@@ -27,7 +27,10 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
 
   uint8 internal constant TREE_ARITY = 5;
 
-  // Error codes
+  Verifier public immutable verifier;
+  VkRegistry public immutable vkRegistry;
+
+  // Custom errors
   error ProcessingNotComplete();
   error InvalidSubsidyProof();
   error AllSubsidyCalculated();
@@ -36,9 +39,6 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
   error RbiTooLarge();
   error CbiTooLarge();
 
-  Verifier public verifier;
-  VkRegistry public vkRegistry;
-
   /// @notice Create a new Subsidy contract
   /// @param _verifier The Verifier contract
   /// @param _vkRegistry The VkRegistry contract
@@ -55,6 +55,8 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
     if (!_mp.processingComplete()) {
       revert ProcessingNotComplete();
     }
+
+    // only update it once
     if (sbCommitment == 0) {
       sbCommitment = _mp.sbCommitment();
     }

contracts/contracts/Tally.sol

@@ -15,14 +15,6 @@ import { CommonUtilities } from "./utilities/CommonUtilities.sol";
 /// @notice The Tally contract is used during votes tallying
 /// and by users to verify the tally results.
 contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
-  // custom errors
-  error ProcessingNotComplete();
-  error InvalidTallyVotesProof();
-  error AllBallotsTallied();
-  error NumSignUpsTooLarge();
-  error BatchStartIndexTooLarge();
-  error TallyBatchSizeTooLarge();
-
   uint8 private constant TREE_ARITY = 5;
 
   /// @notice The commitment to the tally results. Its initial value is 0, but after
@@ -44,8 +36,16 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
   // The final commitment to the state and ballot roots
   uint256 public sbCommitment;
 
-  Verifier public verifier;
-  VkRegistry public vkRegistry;
+  Verifier public immutable verifier;
+  VkRegistry public immutable vkRegistry;
+
+  // custom errors
+  error ProcessingNotComplete();
+  error InvalidTallyVotesProof();
+  error AllBallotsTallied();
+  error NumSignUpsTooLarge();
+  error BatchStartIndexTooLarge();
+  error TallyBatchSizeTooLarge();
 
   /// @notice Create a new Tally contract
   /// @param _verifier The Verifier contract
@@ -100,6 +100,7 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
     if (!_mp.processingComplete()) {
       revert ProcessingNotComplete();
     }
+
     if (sbCommitment == 0) {
       sbCommitment = _mp.sbCommitment();
     }

website/versioned_docs/version-v1.x/contracts.md

@@ -31,8 +31,6 @@ constructor(
     signUpGatekeeper = _signUpGatekeeper;
     initialVoiceCreditProxy = _initialVoiceCreditProxy;
 
-    signUpTimestamp = block.timestamp;
-
     // Verify linked poseidon libraries
     require(
         hash2([uint256(1), uint256(1)]) != 0,