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

feat: add groth16 commitment verifier #2

Merged
merged 4 commits into from
Apr 3, 2024

Conversation

ahmetyalp
Copy link
Contributor

Testing utilities for Consensys/gnark#1063

@ivokub
Copy link
Collaborator

ivokub commented Feb 27, 2024

cc @gbotrel - I do not have maintainer rights for the repo.

@ivokub
Copy link
Collaborator

ivokub commented Feb 27, 2024

In general is good, but I'll try to think if we can merge the templates so that wouldn't have different templates for commitment and non-commitment variants. And imo we can also deduce the number of commitments from verification key for example.

I'll try to push some changes after I get maintainer rights.

@ivokub ivokub self-requested a review February 27, 2024 23:34
@ivokub
Copy link
Collaborator

ivokub commented Apr 2, 2024

I merged the templates with conditional case in case there are commitments for Groth16. Even though I'd prefer not to have --commitments arguments (as it should be deducible from the verification key or the proof itself), then there is too big mismatch between PLONK and Groth16 Solidity verifier APIs to have a nice unified implementation. I'll open another issue to try to unify the APIs (for example having in G16 Verify proof input as bytes similarly as for PLONK). I think we can go for now as is and then try to refactor/unify later in separate PRs.

@ivokub
Copy link
Collaborator

ivokub commented Apr 2, 2024

@ahmetyalp - can you have a look at my changes and let me know if looks good? I'll then go ahead with merging.

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

I think it is good for now to merge. Thanks for the contribution! I'll continue with the PR Consensys/gnark#1063

@ivokub ivokub merged commit a818a0d into Consensys:main Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants