Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(contracts): smart contracts optimizations #919

Merged
merged 1 commit into from
Jan 5, 2024
Merged

refactor(contracts): smart contracts optimizations #919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions contracts/contracts/MACI.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";

/// @title MACI - Minimum Anti-Collusion Infrastructure Version 1
/// @notice A contract which allows users to sign up, and deploy new polls
contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {
contract MACI is IMACI, 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.
Expand All @@ -28,8 +28,7 @@ contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {
/// in contracts/ts/genEmptyBallotRootsContract.ts file
/// if we change the state tree depth!
uint8 internal constant STATE_TREE_SUBDEPTH = 2;
uint8 internal constant STATE_TREE_ARITY = 5;
uint8 internal constant MESSAGE_TREE_ARITY = 5;
uint8 internal constant TREE_ARITY = 5;

/// @notice The hash of a blank state leaf
uint256 internal constant BLANK_STATE_LEAF_HASH =
Expand Down Expand Up @@ -78,6 +77,7 @@ contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {
_;
}

/// @notice custom errors
error CallerMustBePoll(address _caller);
error PoseidonHashLibrariesNotLinked();
error TooManySignups();
Expand All @@ -96,7 +96,7 @@ contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {
InitialVoiceCreditProxy _initialVoiceCreditProxy,
TopupCredit _topupCredit,
uint8 _stateTreeDepth
) {
) payable {
kittybest marked this conversation as resolved.
Show resolved Hide resolved
// Deploy the state AccQueue
stateAq = new AccQueueQuinaryBlankSl(STATE_TREE_SUBDEPTH);
stateAq.enqueue(BLANK_STATE_LEAF_HASH);
Expand Down Expand Up @@ -137,14 +137,15 @@ contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {
bytes memory _initialVoiceCreditProxyData
) public {
// ensure we do not have more signups than what the circuits support
if (numSignUps == uint256(STATE_TREE_ARITY) ** uint256(stateTreeDepth)) revert TooManySignups();
if (numSignUps == uint256(TREE_ARITY) ** uint256(stateTreeDepth)) revert TooManySignups();
kittybest marked this conversation as resolved.
Show resolved Hide resolved

if (_pubKey.x >= SNARK_SCALAR_FIELD || _pubKey.y >= SNARK_SCALAR_FIELD) {
revert MaciPubKeyLargerThanSnarkFieldSize();
}

// Increment the number of signups
// cannot overflow as numSignUps < 5 ** 10 -1
// cannot overflow with realistic stateTreeDepth
// values as numSignUps < 5 ** stateTreeDepth -1
unchecked {
numSignUps++;
}
Expand Down Expand Up @@ -176,6 +177,7 @@ contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {
TreeDepths memory _treeDepths,
PubKey memory _coordinatorPubKey
) public onlyOwner returns (address pollAddr) {
// cache the poll to a local variable so we can increment it
uint256 pollId = nextPollId;

// Increment the poll ID for the next poll
Expand All @@ -190,9 +192,9 @@ contract MACI is IMACI, DomainObjs, Params, Utilities, Ownable {

// The message batch size and the tally batch size
BatchSizes memory batchSizes = BatchSizes(
uint24(MESSAGE_TREE_ARITY) ** _treeDepths.messageTreeSubDepth,
uint24(STATE_TREE_ARITY) ** _treeDepths.intStateTreeDepth,
uint24(STATE_TREE_ARITY) ** _treeDepths.intStateTreeDepth
uint24(TREE_ARITY) ** _treeDepths.messageTreeSubDepth,
uint24(TREE_ARITY) ** _treeDepths.intStateTreeDepth,
uint24(TREE_ARITY) ** _treeDepths.intStateTreeDepth
);

Poll p = pollFactory.deploy(
Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/MessageProcessor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ contract MessageProcessor is Ownable, SnarkCommon, CommonUtilities, Hasher {
/// @notice Create a new instance
/// @param _verifier The Verifier contract address
/// @param _vkRegistry The VkRegistry contract address
constructor(Verifier _verifier, VkRegistry _vkRegistry) {
constructor(Verifier _verifier, VkRegistry _vkRegistry) payable {
verifier = _verifier;
vkRegistry = _vkRegistry;
}
Expand Down
30 changes: 17 additions & 13 deletions contracts/contracts/Poll.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,11 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {

uint256 public numMessages;

/// @notice The number of messages which have been processed and the number of
/// signups
/// @return numSignups The number of signups
/// @return numMsgs The number of messages sent by voters
function numSignUpsAndMessages() public view returns (uint256 numSignups, uint256 numMsgs) {
numSignups = extContracts.maci.numSignUps();
numMsgs = numMessages;
}

MaxValues public maxValues;
TreeDepths public treeDepths;
BatchSizes public batchSizes;

// errors
/// @notice custom errors
error VotingPeriodOver();
error VotingPeriodNotOver();
error PollAlreadyInit();
Expand Down Expand Up @@ -95,7 +86,7 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
BatchSizes memory _batchSizes,
PubKey memory _coordinatorPubKey,
ExtContracts memory _extContracts
) {
) payable {
extContracts = _extContracts;

coordinatorPubKey = _coordinatorPubKey;
Expand Down Expand Up @@ -151,7 +142,8 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
/// @param stateIndex The index of user in the state queue
/// @param amount The amount of credits to topup
function topup(uint256 stateIndex, uint256 amount) public isWithinVotingDeadline {
if (numMessages > maxValues.maxMessages) revert TooManyMessages();
// we check that we do not exceed the max number of messages
if (numMessages == maxValues.maxMessages) revert TooManyMessages();
ctrlc03 marked this conversation as resolved.
Show resolved Hide resolved

unchecked {
numMessages++;
Expand All @@ -173,9 +165,11 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
/// @param _encPubKey An epheremal public key which can be combined with the
/// coordinator's private key to generate an ECDH shared key with which
/// to encrypt the message.
function publishMessage(Message memory _message, PubKey memory _encPubKey) public isWithinVotingDeadline {
function publishMessage(Message memory _message, PubKey calldata _encPubKey) public isWithinVotingDeadline {
// we check that we do not exceed the max number of messages
if (numMessages == maxValues.maxMessages) revert TooManyMessages();
ctrlc03 marked this conversation as resolved.
Show resolved Hide resolved

// validate that the public key is valid
if (_encPubKey.x >= SNARK_SCALAR_FIELD || _encPubKey.y >= SNARK_SCALAR_FIELD) {
revert MaciPubKeyLargerThanSnarkFieldSize();
}
Expand All @@ -184,6 +178,7 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
numMessages++;
}

// force the message to have type 1
_message.msgType = 1;
uint256 messageLeaf = hashMessageAndEncPubKey(_message, _encPubKey);
extContracts.messageAq.enqueue(messageLeaf);
Expand Down Expand Up @@ -252,4 +247,13 @@ contract Poll is Params, Utilities, SnarkCommon, Ownable, EmptyBallotRoots {
_deployTime = deployTime;
_duration = duration;
}

/// @notice The number of messages which have been processed and the number of
/// signups
/// @return numSignups The number of signups
/// @return numMsgs The number of messages sent by voters
function numSignUpsAndMessages() public view returns (uint256 numSignups, uint256 numMsgs) {
numSignups = extContracts.maci.numSignUps();
numMsgs = numMessages;
}
}
14 changes: 9 additions & 5 deletions contracts/contracts/PollFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,14 @@ import { Poll } from "./Poll.sol";
/// size to stay within the limit set by EIP-170.
contract PollFactory is Params, DomainObjs {
// The number of children each node in the message tree has
uint256 public constant TREE_ARITY = 5;
uint256 internal constant TREE_ARITY = 5;

// custom error
error InvalidMaxValues();

/// @notice The PollFactory constructor
constructor() payable {}

/// @notice Deploy a new Poll contract and AccQueue contract for messages.
/// @param _duration The duration of the poll
/// @param _maxValues The max values for the poll
Expand All @@ -29,10 +33,10 @@ contract PollFactory is Params, DomainObjs {
/// @return poll The deployed Poll contract
function deploy(
uint256 _duration,
MaxValues memory _maxValues,
TreeDepths memory _treeDepths,
BatchSizes memory _batchSizes,
PubKey memory _coordinatorPubKey,
MaxValues calldata _maxValues,
TreeDepths calldata _treeDepths,
BatchSizes calldata _batchSizes,
PubKey calldata _coordinatorPubKey,
IMACI _maci,
TopupCredit _topupCredit,
address _pollOwner
Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/SignUpToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import "@openzeppelin/contracts/access/Ownable.sol";
/// can be used to allow users to sign up for a poll.
contract SignUpToken is ERC721, Ownable {
/// @notice The constructor which calls the ERC721 constructor
constructor() ERC721("SignUpToken", "SignUpToken") {}
constructor() payable ERC721("SignUpToken", "SignUpToken") {}

/// @notice Gives an ERC721 token to an address
/// @param to The address to give the token to
Expand Down
17 changes: 10 additions & 7 deletions contracts/contracts/Subsidy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,16 @@ import { VkRegistry } from "./VkRegistry.sol";
/// are correct. It is also used to update the subsidy commitment if the
/// proof is valid.
contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
uint256 public rbi; // row batch index
uint256 public cbi; // column batch index
// row batch index
uint256 public rbi;
// column batch index
uint256 public cbi;

// The final commitment to the state and ballot roots
uint256 public sbCommitment;
uint256 public subsidyCommitment;

uint8 public constant treeArity = 5;
uint8 internal constant TREE_ARITY = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about that TREE_ARITY is called so many times in different contracts, and they should keep the same. Should we put it in somewhere and import it in these contracts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm maybe we could put it inside another contract shared by all of them though I think that would bloat things up a little bit 🤔 was looking and couldn't find a contract that was already in use by all of them (MACI, Poll, MP, Tally, etc) where we could add this constant..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a good idea to have a contract holding the constants? or we just keep status quo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm we have SnarkConstants holding some constants, though I'm not sure if it's a widely used pattern.. to avoid bloating contracts even more I would just keep this one constant inside each contract, at least for now

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then when it comes to deployment, if a user wants to alter the TREE_ARITY, they currently need to navigate through every contract file to modify the value. What if we convert it into an environmental variable for the sake of convenience during deployment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, though they would also need to change the type of AccQueue in use, which currently is hardcoded to a quinary tree everywhere, mm 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useful discussion here, there's definitely some more simplifying we can do given we always use quinary trees

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by converting to env variable, you mean that it should be a value passed in the constructor when deploying the contracts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes... but not sure if this is a good idea, just wanna reduce the risk that if some users deploy it and miss to change one of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's write this down and discuss together at the next team meeting? from next week we'll have a smart contract developer to ask all SC questions to 😈


// Error codes
error ProcessingNotComplete();
Expand All @@ -39,7 +42,7 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
/// @notice Create a new Subsidy contract
/// @param _verifier The Verifier contract
/// @param _vkRegistry The VkRegistry contract
constructor(Verifier _verifier, VkRegistry _vkRegistry) {
constructor(Verifier _verifier, VkRegistry _vkRegistry) payable {
verifier = _verifier;
vkRegistry = _vkRegistry;
}
Expand Down Expand Up @@ -93,14 +96,14 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
Poll _poll,
MessageProcessor _mp,
uint256 _newSubsidyCommitment,
uint256[8] memory _proof
uint256[8] calldata _proof
) external onlyOwner {
_votingPeriodOver(_poll);
updateSbCommitment(_mp);

(uint8 intStateTreeDepth, , , ) = _poll.treeDepths();

uint256 subsidyBatchSize = uint256(treeArity) ** intStateTreeDepth;
uint256 subsidyBatchSize = uint256(TREE_ARITY) ** intStateTreeDepth;

(uint256 numSignUps, ) = _poll.numSignUpsAndMessages();

Expand Down Expand Up @@ -140,7 +143,7 @@ contract Subsidy is Ownable, CommonUtilities, Hasher, SnarkCommon {
/// @return isValid True if the proof is valid
function verifySubsidyProof(
Poll _poll,
uint256[8] memory _proof,
uint256[8] calldata _proof,
uint256 _numSignUps,
uint256 _newSubsidyCommitment
) public view returns (bool isValid) {
Expand Down
24 changes: 12 additions & 12 deletions contracts/contracts/Tally.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
error BatchStartIndexTooLarge();
error TallyBatchSizeTooLarge();

uint8 private constant LEAVES_PER_NODE = 5;
uint8 private 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
Expand All @@ -51,7 +51,7 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
/// @notice Create a new Tally contract
/// @param _verifier The Verifier contract
/// @param _vkRegistry The VkRegistry contract
constructor(Verifier _verifier, VkRegistry _vkRegistry) {
constructor(Verifier _verifier, VkRegistry _vkRegistry) payable {
verifier = _verifier;
vkRegistry = _vkRegistry;
}
Expand Down Expand Up @@ -115,7 +115,7 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
Poll _poll,
MessageProcessor _mp,
uint256 _newTallyCommitment,
uint256[8] memory _proof
uint256[8] calldata _proof
) public onlyOwner {
_votingPeriodOver(_poll);
updateSbCommitment(_mp);
Expand Down Expand Up @@ -149,7 +149,7 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
/// @return isValid whether the proof is valid
function verifyTallyProof(
Poll _poll,
uint256[8] memory _proof,
uint256[8] calldata _proof,
uint256 _numSignUps,
uint256 _batchStartIndex,
uint256 _tallyBatchSize,
Expand Down Expand Up @@ -185,16 +185,16 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
uint8 _depth,
uint256 _index,
uint256 _leaf,
uint256[][] memory _pathElements
uint256[][] calldata _pathElements
) internal pure returns (uint256 current) {
uint256 pos = _index % LEAVES_PER_NODE;
uint256 pos = _index % TREE_ARITY;
current = _leaf;
uint8 k;

uint256[LEAVES_PER_NODE] memory level;
uint256[TREE_ARITY] memory level;

for (uint8 i = 0; i < _depth; ++i) {
for (uint8 j = 0; j < LEAVES_PER_NODE; ++j) {
for (uint8 j = 0; j < TREE_ARITY; ++j) {
if (j == pos) {
level[j] = current;
} else {
Expand All @@ -207,8 +207,8 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
}
}

_index /= LEAVES_PER_NODE;
pos = _index % LEAVES_PER_NODE;
_index /= TREE_ARITY;
pos = _index % TREE_ARITY;
current = hash5(level);
}
}
Expand Down Expand Up @@ -246,7 +246,7 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
function verifyPerVOSpentVoiceCredits(
uint256 _voteOptionIndex,
uint256 _spent,
uint256[][] memory _spentProof,
uint256[][] calldata _spentProof,
uint256 _spentSalt,
uint8 _voteOptionTreeDepth,
uint256 _spentVoiceCreditsHash,
Expand Down Expand Up @@ -275,7 +275,7 @@ contract Tally is Ownable, SnarkCommon, CommonUtilities, Hasher {
function verifyTallyResult(
uint256 _voteOptionIndex,
uint256 _tallyResult,
uint256[][] memory _tallyResultProof,
uint256[][] calldata _tallyResultProof,
uint256 _tallyResultSalt,
uint8 _voteOptionTreeDepth,
uint256 _spentVoiceCreditsHash,
Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/TopupCredit.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ contract TopupCredit is ERC20, Ownable {
uint256 public constant MAXIMUM_AIRDROP_AMOUNT = 100000 * 10 ** _decimals;

/// @notice create a new TopupCredit token
constructor() ERC20("TopupCredit", "TopupCredit") {}
constructor() payable ERC20("TopupCredit", "TopupCredit") {}

/// @notice mint tokens to an account
/// @param account the account to mint tokens to
Expand Down
9 changes: 6 additions & 3 deletions contracts/contracts/VkRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ contract VkRegistry is Ownable, SnarkCommon {
error TallyVkNotSet();
error SubsidyVkNotSet();

/// @notice Create a new instance of the VkRegistry contract
constructor() payable {}

/// @notice Check if the process verifying key is set
/// @param _sig The signature
/// @return isSet whether the verifying key is set
Expand Down Expand Up @@ -106,8 +109,8 @@ contract VkRegistry is Ownable, SnarkCommon {
uint256 _messageTreeDepth,
uint256 _voteOptionTreeDepth,
uint256 _messageBatchSize,
VerifyingKey memory _processVk,
VerifyingKey memory _tallyVk
VerifyingKey calldata _processVk,
VerifyingKey calldata _tallyVk
) public onlyOwner {
uint256 processVkSig = genProcessVkSig(_stateTreeDepth, _messageTreeDepth, _voteOptionTreeDepth, _messageBatchSize);

Expand Down Expand Up @@ -152,7 +155,7 @@ contract VkRegistry is Ownable, SnarkCommon {
uint256 _stateTreeDepth,
uint256 _intStateTreeDepth,
uint256 _voteOptionTreeDepth,
VerifyingKey memory _subsidyVk
VerifyingKey calldata _subsidyVk
) public onlyOwner {
uint256 subsidyVkSig = genSubsidyVkSig(_stateTreeDepth, _intStateTreeDepth, _voteOptionTreeDepth);

Expand Down
7 changes: 7 additions & 0 deletions contracts/contracts/gatekeepers/FreeForAllSignUpGatekeeper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ import { MACI } from "../MACI.sol";
/// @title FreeForAllGatekeeper
/// @notice A SignUpGatekeeper which allows anyone to sign up.
contract FreeForAllGatekeeper is SignUpGatekeeper {
/// @notice Create a new instance of FreeForAllGatekeeper
constructor() payable {}
ctrlc03 marked this conversation as resolved.
Show resolved Hide resolved

/// @notice setMaciInstance does nothing in this gatekeeper
/// @param _maci The MACI contract
function setMaciInstance(MACI _maci) public override {}

/// @notice Registers the user without any restrictions.
/// @param _address The address of the user
/// @param _data memory additional data
Expand Down
Loading
Loading