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.

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(nonqv): optimize tally votes non qv circuit and contracts #1174

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

ctrlc03
Copy link
Collaborator

@ctrlc03 ctrlc03 commented Feb 10, 2024

Description

Reduce the number of constraints for the tallyVotesNonQv circuit, and amend the repo code to support the changes. This includes a new Tally contract with reduced logic (no need for perVOSpentVoiceCredits anymore due to quadratic voting being removed).

Relevant code to deploy either factory version of tally has been added, and as far as MACI is concerned, they implement the same interface so no changes were needed on the MACI contract.

With ceremony params (6,2,3) - from 398474 to 371675 constraints

Confirmation

Copy link

netlify bot commented Feb 10, 2024

Deploy Preview for maci-typedoc ready!

Name Link
🔨 Latest commit ea632a9
🔍 Latest deploy log https://app.netlify.com/sites/maci-typedoc/deploys/65cd05f11413060008a94638
😎 Deploy Preview https://deploy-preview-1174--maci-typedoc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Slither found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@ctrlc03 ctrlc03 force-pushed the refactor/tally-votes-non-qv branch from 1273880 to 9ef7e30 Compare February 10, 2024 16:51
@ctrlc03 ctrlc03 self-assigned this Feb 10, 2024
@ctrlc03 ctrlc03 force-pushed the refactor/tally-votes-non-qv branch from 9ef7e30 to 9060662 Compare February 10, 2024 17:11
@ctrlc03 ctrlc03 marked this pull request as ready for review February 12, 2024 09:29
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 thanks, just few comments, and can you add TallyNonQv contract and factory to contracts/tasks/deploy?

circuits/circom/tallyVotesNonQv.circom Show resolved Hide resolved
cli/ts/commands/genProofs.ts Outdated Show resolved Hide resolved
cli/ts/commands/verify.ts Outdated Show resolved Hide resolved
contracts/contracts/TallyNonQv.sol Show resolved Hide resolved
contracts/contracts/TallyNonQv.sol Show resolved Hide resolved
contracts/contracts/TallyNonQv.sol Outdated Show resolved Hide resolved
contracts/contracts/TallyNonQvFactory.sol Show resolved Hide resolved
contracts/tests/TallyNonQv.test.ts Outdated Show resolved Hide resolved
@ctrlc03 ctrlc03 force-pushed the refactor/tally-votes-non-qv branch 2 times, most recently from 1a2aa96 to d60e452 Compare February 13, 2024 15:34
@ctrlc03 ctrlc03 requested a review from 0xmad February 13, 2024 15:50
@0xmad
Copy link
Collaborator

0xmad commented Feb 13, 2024

There are some missing parts:

  1. Updates from this file cli/ts/commands/genProofs.ts are needed for contracts/tasks
  2. There are now TallyNonQv and TallyNonQvFactory added for deploys (you can take as an example Tally and TallyFactory). You will need to add these contract names to EContract enum and see them if deployed-contracts.json after deploying with non-qv option.

@ctrlc03 ctrlc03 force-pushed the refactor/tally-votes-non-qv branch from d60e452 to 08ca900 Compare February 14, 2024 13:42
@ctrlc03
Copy link
Collaborator Author

ctrlc03 commented Feb 14, 2024

There are some missing parts:

  1. Updates from this file cli/ts/commands/genProofs.ts are needed for contracts/tasks
  2. There are now TallyNonQv and TallyNonQvFactory added for deploys (you can take as an example Tally and TallyFactory). You will need to add these contract names to EContract enum and see them if deployed-contracts.json after deploying with non-qv option.

Added!

contracts/tasks/deploy/maci/08-tallyFactory.ts Outdated Show resolved Hide resolved
contracts/tasks/helpers/Deployment.ts Outdated Show resolved Hide resolved
contracts/tasks/runner/deployFull.ts Outdated Show resolved Hide resolved
contracts/tasks/deploy/poll/01-poll.ts Show resolved Hide resolved
reduce the number of constraints for the tally votes non qv circuit, and amend the repo code to
support the changes. This includes a new Tally contract with reduced logic which has been removed
from the circuit.
@ctrlc03 ctrlc03 force-pushed the refactor/tally-votes-non-qv branch from 08ca900 to ea632a9 Compare February 14, 2024 18:26
@ctrlc03 ctrlc03 requested a review from 0xmad February 14, 2024 18:27
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 thanks!

@ctrlc03
Copy link
Collaborator Author

ctrlc03 commented Feb 14, 2024

@ctrlc03 thanks!

thanks for your patience with this review 🙂

@ctrlc03 ctrlc03 enabled auto-merge February 14, 2024 18:32
@ctrlc03 ctrlc03 merged commit e2e1031 into dev Feb 14, 2024
19 checks passed
@ctrlc03 ctrlc03 deleted the refactor/tally-votes-non-qv branch February 14, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants