From d39d3844690749b396ba66226224789b19af522d Mon Sep 17 00:00:00 2001 From: ctrlc03 <93448202+ctrlc03@users.noreply.github.com> Date: Thu, 11 Jan 2024 23:08:29 +0000 Subject: [PATCH] refactor(contracts): optimize code by using immutable variables are removing redundant code --- .../scripts/ceremony-param-tests-c-witness.sh | 2 +- contracts/contracts/MACI.sol | 24 +++++++---------- contracts/contracts/MessageProcessor.sol | 6 ++--- contracts/contracts/Poll.sol | 26 ++++++++++--------- contracts/contracts/Subsidy.sol | 14 +++++----- contracts/contracts/Tally.sol | 25 +++++++++--------- .../versioned_docs/version-v1.x/contracts.md | 2 -- 7 files changed, 48 insertions(+), 51 deletions(-) diff --git a/.github/scripts/ceremony-param-tests-c-witness.sh b/.github/scripts/ceremony-param-tests-c-witness.sh index 5434611899..b4e7fc9ef5 100755 --- a/.github/scripts/ceremony-param-tests-c-witness.sh +++ b/.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 \ No newline at end of file + -q true diff --git a/contracts/contracts/MACI.sol b/contracts/contracts/MACI.sol index 2c70bf7f26..e923eff37e 100644 --- a/contracts/contracts/MACI.sol +++ b/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(); } diff --git a/contracts/contracts/MessageProcessor.sol b/contracts/contracts/MessageProcessor.sol index 42a2d5fcc3..c03729d011 100644 --- a/contracts/contracts/MessageProcessor.sol +++ b/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 diff --git a/contracts/contracts/Poll.sol b/contracts/contracts/Poll.sol index 23740e7ad4..c9107cec85 100644 --- a/contracts/contracts/Poll.sol +++ b/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); diff --git a/contracts/contracts/Subsidy.sol b/contracts/contracts/Subsidy.sol index d46035502f..81a2887329 100644 --- a/contracts/contracts/Subsidy.sol +++ b/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(); } diff --git a/contracts/contracts/Tally.sol b/contracts/contracts/Tally.sol index 37b92708c8..c58f38db44 100644 --- a/contracts/contracts/Tally.sol +++ b/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(); } diff --git a/website/versioned_docs/version-v1.x/contracts.md b/website/versioned_docs/version-v1.x/contracts.md index f185cb8b9a..cc2d48a411 100644 --- a/website/versioned_docs/version-v1.x/contracts.md +++ b/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,