From f72933674f1253155887936cb40fe833ea219cf4 Mon Sep 17 00:00:00 2001 From: ctrlc03 <93448202+ctrlc03@users.noreply.github.com> Date: Mon, 19 Feb 2024 15:05:56 +0000 Subject: [PATCH] refactor(contracts): revisit functions visibility and inheritance Ensure contracts integrating MACI can override certain functions, as well as use public for functions which are likely to be called from within an inheriting contract. Also remove private for constants in favour of internal. --- contracts/contracts/MACI.sol | 10 +++++++++- contracts/contracts/MessageProcessor.sol | 2 +- contracts/contracts/PollFactory.sol | 10 +++++++--- contracts/contracts/Tally.sol | 2 +- contracts/contracts/TallyFactory.sol | 2 +- contracts/contracts/TallyNonQv.sol | 4 ++-- contracts/contracts/TallyNonQvFactory.sol | 2 +- contracts/contracts/interfaces/IPollFactory.sol | 3 +-- 8 files changed, 23 insertions(+), 12 deletions(-) diff --git a/contracts/contracts/MACI.sol b/contracts/contracts/MACI.sol index df0c61ac3c..c00223c5a1 100644 --- a/contracts/contracts/MACI.sol +++ b/contracts/contracts/MACI.sol @@ -218,7 +218,15 @@ contract MACI is IMACI, Params, Utilities, Ownable { address _owner = owner(); - address p = pollFactory.deploy(_duration, maxValues, _treeDepths, _coordinatorPubKey, this, topupCredit, _owner); + address p = pollFactory.deploy( + _duration, + maxValues, + _treeDepths, + _coordinatorPubKey, + address(this), + topupCredit, + _owner + ); address mp = messageProcessorFactory.deploy(_verifier, _vkRegistry, p, _owner); address tally = tallyFactory.deploy(_verifier, _vkRegistry, p, mp, _owner); diff --git a/contracts/contracts/MessageProcessor.sol b/contracts/contracts/MessageProcessor.sol index 1ab10fbcf3..2ecbf031ac 100644 --- a/contracts/contracts/MessageProcessor.sol +++ b/contracts/contracts/MessageProcessor.sol @@ -29,7 +29,7 @@ contract MessageProcessor is Ownable, SnarkCommon, Hasher, CommonUtilities, IMes error BatchEndIndexTooLarge(); // the number of children per node in the merkle trees - uint256 private constant TREE_ARITY = 5; + uint256 internal constant TREE_ARITY = 5; /// @inheritdoc IMessageProcessor bool public processingComplete; diff --git a/contracts/contracts/PollFactory.sol b/contracts/contracts/PollFactory.sol index 43bed664f0..ae62a1b761 100644 --- a/contracts/contracts/PollFactory.sol +++ b/contracts/contracts/PollFactory.sol @@ -30,10 +30,10 @@ contract PollFactory is Params, DomainObjs, IPollFactory { MaxValues calldata _maxValues, TreeDepths calldata _treeDepths, PubKey calldata _coordinatorPubKey, - IMACI _maci, + address _maci, TopupCredit _topupCredit, address _pollOwner - ) public returns (address pollAddr) { + ) public virtual returns (address pollAddr) { /// @notice Validate _maxValues /// maxVoteOptions must be less than 2 ** 50 due to circuit limitations; /// it will be packed as a 50-bit value along with other values as one @@ -46,7 +46,11 @@ contract PollFactory is Params, DomainObjs, IPollFactory { AccQueue messageAq = new AccQueueQuinaryMaci(_treeDepths.messageTreeSubDepth); /// @notice the smart contracts that a Poll would interact with - ExtContracts memory extContracts = ExtContracts({ maci: _maci, messageAq: messageAq, topupCredit: _topupCredit }); + ExtContracts memory extContracts = ExtContracts({ + maci: IMACI(_maci), + messageAq: messageAq, + topupCredit: _topupCredit + }); // deploy the poll Poll poll = new Poll(_duration, _maxValues, _treeDepths, _coordinatorPubKey, extContracts); diff --git a/contracts/contracts/Tally.sol b/contracts/contracts/Tally.sol index 8bbaf83b7f..e49d587c88 100644 --- a/contracts/contracts/Tally.sol +++ b/contracts/contracts/Tally.sol @@ -15,7 +15,7 @@ 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 { - uint256 private constant TREE_ARITY = 5; + uint256 internal constant TREE_ARITY = 5; /// @notice The commitment to the tally results. Its initial value is 0, but after /// the tally of each batch is proven on-chain via a zk-SNARK, it should be diff --git a/contracts/contracts/TallyFactory.sol b/contracts/contracts/TallyFactory.sol index f269f16ff4..bbf0746928 100644 --- a/contracts/contracts/TallyFactory.sol +++ b/contracts/contracts/TallyFactory.sol @@ -14,7 +14,7 @@ contract TallyFactory is ITallySubsidyFactory { address _poll, address _messageProcessor, address _owner - ) public returns (address tallyAddr) { + ) public virtual returns (address tallyAddr) { // deploy Tally for this Poll Tally tally = new Tally(_verifier, _vkRegistry, _poll, _messageProcessor); tally.transferOwnership(_owner); diff --git a/contracts/contracts/TallyNonQv.sol b/contracts/contracts/TallyNonQv.sol index 4a6bf1d197..96c8af1dff 100644 --- a/contracts/contracts/TallyNonQv.sol +++ b/contracts/contracts/TallyNonQv.sol @@ -15,7 +15,7 @@ import { CommonUtilities } from "./utilities/CommonUtilities.sol"; /// @notice The TallyNonQv contract is used during votes tallying /// and by users to verify the tally results. contract TallyNonQv is Ownable, SnarkCommon, CommonUtilities, Hasher { - uint256 private constant TREE_ARITY = 5; + uint256 internal constant TREE_ARITY = 5; /// @notice The commitment to the tally results. Its initial value is 0, but after /// the tally of each batch is proven on-chain via a zk-SNARK, it should be @@ -79,7 +79,7 @@ contract TallyNonQv is Ownable, SnarkCommon, CommonUtilities, Hasher { /// @notice Check if all ballots are tallied /// @return tallied whether all ballots are tallied - function isTallied() external view returns (bool tallied) { + function isTallied() public view returns (bool tallied) { (uint8 intStateTreeDepth, , , ) = poll.treeDepths(); (uint256 numSignUps, ) = poll.numSignUpsAndMessages(); diff --git a/contracts/contracts/TallyNonQvFactory.sol b/contracts/contracts/TallyNonQvFactory.sol index 3d48f958f3..099fe14058 100644 --- a/contracts/contracts/TallyNonQvFactory.sol +++ b/contracts/contracts/TallyNonQvFactory.sol @@ -14,7 +14,7 @@ contract TallyNonQvFactory is ITallySubsidyFactory { address _poll, address _messageProcessor, address _owner - ) public returns (address tallyAddr) { + ) public virtual returns (address tallyAddr) { // deploy Tally for this Poll TallyNonQv tally = new TallyNonQv(_verifier, _vkRegistry, _poll, _messageProcessor); tally.transferOwnership(_owner); diff --git a/contracts/contracts/interfaces/IPollFactory.sol b/contracts/contracts/interfaces/IPollFactory.sol index 0080be622d..ee903f3a8e 100644 --- a/contracts/contracts/interfaces/IPollFactory.sol +++ b/contracts/contracts/interfaces/IPollFactory.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.10; -import { IMACI } from "./IMACI.sol"; import { TopupCredit } from "../TopupCredit.sol"; import { Params } from "../utilities/Params.sol"; import { DomainObjs } from "../utilities/DomainObjs.sol"; @@ -23,7 +22,7 @@ interface IPollFactory { Params.MaxValues memory _maxValues, Params.TreeDepths memory _treeDepths, DomainObjs.PubKey memory _coordinatorPubKey, - IMACI _maci, + address _maci, TopupCredit _topupCredit, address _pollOwner ) external returns (address);