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

Jump to bottom

fix: organize std packages hints registrations #1043

Merged
merged 10 commits into from
Feb 8, 2024
Merged

fix: organize std packages hints registrations #1043

merged 10 commits into from
Feb 8, 2024

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Feb 8, 2024

Description

This PR refactors used in 2-chain and emulated curves, adding methods for getting all the hints used in the pacakges, registering them in one place. I also added the registration into std/ package, for which I added init method, allowing to do only

import _ "github.com/consensys/gnark/std"

which should register all hints automatically.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

  • CI tests
  • downstream tests

How has this been benchmarked?

Not anticipating any changes.

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

ivokub added 9 commits February 8, 2024 12:05
@ivokub
refactor: bls12377 hints
48c8795
@ivokub
chore: remove ineffective code
3bcc026
@ivokub
refactor: bls24315 hints
41a12fb
@ivokub
refactor: sw_bls12377 hints
468320a
@ivokub
refactor: sw_bls24315 hints
74417f1
@ivokub
fix: include square torus hints in registration
e9a7835
@ivokub
fix: register package hints
eed6aa0
@ivokub
feat: add side-effect std hint registration
6819378
@ivokub
refactor: make all the hints private
a3c3cae 
The preferred way to get hints would be to use GetHints() methods in packages.
In that way, the users cannot depend on individual hints and makes refactoring
easier (and avoids any kind of assumptions of the existence of hints).
@ivokub ivokub added the bug Something isn't working label Feb 8, 2024
@ivokub ivokub added this to the v0.10.0 milestone Feb 8, 2024
@ivokub ivokub requested a review from yelhousni February 8, 2024 11:36
@ivokub ivokub self-assigned this Feb 8, 2024
yelhousni
yelhousni approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

Looks clean 👍 only remark is squareTorusHint is redundant

@yelhousni yelhousni self-requested a review February 8, 2024 11:46
yelhousni
yelhousni requested changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

remove redundant squareTorusHint

@ivokub ivokub requested a review from yelhousni February 8, 2024 11:49
@ivokub
Copy link
Collaborator Author

ivokub commented Feb 8, 2024

remove redundant squareTorusHint

Done, could you re-review?

@ivokub ivokub merged commit 9b8efda into master Feb 8, 2024
7 checks passed
@ivokub ivokub deleted the fix/std-hint-registration branch February 8, 2024 11:59
@ivokub ivokub mentioned this pull request Feb 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yelhousni yelhousni yelhousni approved these changes

Assignees

@ivokub ivokub

Labels
bug Something isn't working
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ivokub @yelhousni