Skip to content

feat(wires): implements GetWiresConstraintExact as a draft proposal #1462

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

Jump to bottom
Merged
AlexandreBelling merged 8 commits into from
Apr 9, 2025
Merged

feat(wires): implements GetWiresConstraintExact as a draft proposal #1462

AlexandreBelling merged 8 commits into from
Apr 9, 2025

Conversation

@AlexandreBelling
Copy link
Collaborator

@AlexandreBelling AlexandreBelling commented Mar 31, 2025

Description

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

  • Test A
  • Test B

How has this been benchmarked?

  • Benchmark A, on Macbook pro M1, 32GB RAM
  • Benchmark B, on x86 Intel xxx, 16GB RAM

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@AlexandreBelling AlexandreBelling self-assigned this Mar 31, 2025
@AlexandreBelling AlexandreBelling marked this pull request as ready for review April 9, 2025 08:48
@AlexandreBelling
Copy link
Collaborator Author

AlexandreBelling commented Apr 9, 2025

The PR is ready. I decided it's maybe simpler to keep the original function. I renamed it however

ivokub
ivokub approved these changes Apr 9, 2025
View reviewed changes
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.

Looks good to me. Indeed I think we could optimize that we wouldn't add constraints for every constant (if they are duplicate), but I guess it shouldn't happen too much?

Only thing I would change is the name of the existing method - the documentation says GetWireConstraintsNoDuplicate but method name is GetWireConstraintsN. But I would keep it as is, because we use it using implicit interfaces and then its easier to keep existing implementation working.

@AlexandreBelling
Copy link
Collaborator Author

AlexandreBelling commented Apr 9, 2025

Fair enough. I have applied your suggestion. For the constant, it seems easy enough to use a map to read ten times the same constants. For MiMC, the constant is essentially the "0" that we use to initialize the state of a MiMC hasher.

@AlexandreBelling
Copy link
Collaborator Author

AlexandreBelling commented Apr 9, 2025

Lemme test this on my end before merging. Will merge after. Happy to get a reapproval :)

ivokub
ivokub reviewed Apr 9, 2025
View reviewed changes
ivokub
ivokub approved these changes Apr 9, 2025
View reviewed changes
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.

Yep - now it seems better. Now only the method documentation doesn't match the implementation.

ivokub
ivokub approved these changes Apr 9, 2025
View reviewed changes
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.

LGTM now. Thanks!

@AlexandreBelling AlexandreBelling merged commit 1e252ac into master Apr 9, 2025
5 checks passed
@AlexandreBelling AlexandreBelling deleted the feat/get-wires-constraints-exact branch April 9, 2025 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivokub ivokub ivokub approved these changes

@gbotrel gbotrel Awaiting requested review from gbotrel

Assignees

@AlexandreBelling AlexandreBelling

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AlexandreBelling @ivokub