Merge pull request #1008 from privacy-scaling-explorations/refactor/c…

…ontracts-optimizations

refactor(contracts): optimize code

.github/scripts/ceremony-param-tests-c-witness.sh

@@ -71,4 +71,4 @@ HARDHAT_CONFIG=./build/hardhat.config.js node build/ts/index.js verify \
     --poll-id 0 \
     --subsidy-enabled false \
     --tally-file tally.json \
-    -q true
+    -q true

contracts/contracts/MACI.sol

@@ -39,41 +39,37 @@ contract MACI is IMACI, Params, Utilities, Ownable {
   mapping(uint256 => address) 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;
 
   /// @notice ERC20 contract that hold topup credits
-  TopupCredit public topupCredit;
+  TopupCredit public immutable topupCredit;
 
   /// @notice Factory contract that deploy a Poll contract
-  IPollFactory public pollFactory;
+  IPollFactory public immutable pollFactory;
 
   /// @notice Factory contract that deploy a MessageProcessor contract
-  IMessageProcessorFactory public messageProcessorFactory;
+  IMessageProcessorFactory public immutable messageProcessorFactory;
 
   /// @notice Factory contract that deploy a Tally contract
-  ITallySubsidyFactory public tallyFactory;
+  ITallySubsidyFactory public immutable tallyFactory;
 
   /// @notice Factory contract that deploy a Subsidy contract
-  ITallySubsidyFactory public subsidyFactory;
+  ITallySubsidyFactory public immutable subsidyFactory;
 
   /// @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);
@@ -131,8 +127,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

@@ -42,9 +42,9 @@ contract MessageProcessor is Ownable, SnarkCommon, Hasher, CommonUtilities, IMes
   /// @inheritdoc IMessageProcessor
   uint256 public sbCommitment;
 
-  IPoll public poll;
-  IVerifier public verifier;
-  IVkRegistry public vkRegistry;
+  IPoll public immutable poll;
+  IVerifier public immutable verifier;
+  IVkRegistry public immutable vkRegistry;
 
   /// @notice Create a new instance
   /// @param _verifier The Verifier contract address

contracts/contracts/Poll.sol

@@ -19,19 +19,20 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
   using SafeERC20 for ERC20;
 
   bool internal isInit;
+
   // The coordinator's public key
   PubKey public coordinatorPubKey;
 
   /// @notice Hash of the coordinator's public key
-  uint256 public coordinatorPubKeyHash;
+  uint256 public immutable coordinatorPubKeyHash;
 
   uint256 public mergedStateRoot;
 
   // 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;
 
   /// @notice Whether the MACI contract's stateAq has been merged by this contract
   bool public stateAqMerged;
@@ -55,6 +56,8 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
 
   /// @notice Batch sizes for processing
   BatchSizes public batchSizes;
+  /// @notice The contracts used by the Poll
+  ExtContracts public extContracts;
 
   error VotingPeriodOver();
   error VotingPeriodNotOver();
@@ -71,9 +74,6 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
   event MergeMessageAqSubRoots(uint256 _numSrQueueOps);
   event MergeMessageAq(uint256 _messageRoot);
 
-  /// @notice External contracts
-  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
@@ -91,14 +91,13 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
     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;
   }
@@ -146,10 +145,12 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
     // 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;
@@ -170,11 +171,12 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
       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);
@@ -187,9 +189,8 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
     // 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);
   }
@@ -202,6 +203,7 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots, IPol
 
     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,12 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
 
   uint8 internal constant TREE_ARITY = 5;
 
-  // Error codes
+  IVerifier public immutable verifier;
+  IVkRegistry public immutable vkRegistry;
+  IPoll public immutable poll;
+  IMessageProcessor public immutable mp;
+
+  // Custom errors
   error ProcessingNotComplete();
   error InvalidSubsidyProof();
   error AllSubsidyCalculated();
@@ -36,11 +41,6 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
   error RbiTooLarge();
   error CbiTooLarge();
 
-  IVerifier public verifier;
-  IVkRegistry public vkRegistry;
-  IPoll public poll;
-  IMessageProcessor public mp;
-
   /// @notice Create a new Subsidy contract
   /// @param _verifier The Verifier contract
   /// @param _vkRegistry The VkRegistry contract
@@ -60,6 +60,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,10 +36,18 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
   // The final commitment to the state and ballot roots
   uint256 public sbCommitment;
 
-  IVerifier public verifier;
-  IVkRegistry public vkRegistry;
-  IPoll public poll;
-  IMessageProcessor public mp;
+  IVerifier public immutable verifier;
+  IVkRegistry public immutable vkRegistry;
+  IPoll public immutable poll;
+  IMessageProcessor public immutable mp;
+
+  /// @notice 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
@@ -105,6 +105,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,