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

chore: add bn254 attribute when needed in the stdlib #3208

Merged
merged 5 commits into from
Oct 26, 2023
Merged

chore: add bn254 attribute when needed in the stdlib #3208

merged 5 commits into from
Oct 26, 2023

Conversation

guipublic
Copy link
Contributor

Description

Use the field attribute to restrict some functions in stdlib to bn254 field

Problem*

Some stdlib functions require that noir native field is the one from bn254 curve, so we use the new field attribute so that these functions cannot be used if the field is different.

Summary*

Add the attribute #[field(bn254)] to a few functions in stdlib, such as:
poseidon, babyjubjub and mimc.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@kevaundray kevaundray requested a review from vezenovm October 19, 2023 23:21
jfecher
jfecher approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM. Pinging @vezenovm and @kevaundray for re-review just in case.

@vezenovm
Copy link
Contributor

I'm fine with merging this right now, but it does feel that the attributes in noir_stdlib/src/hash/poseidon/bn254/perm.nr are unneeded as they should be encapsulated by the bn254 module instead.

@guipublic
Copy link
Contributor Author

I'm fine with merging this right now, but it does feel that the attributes in noir_stdlib/src/hash/poseidon/bn254/perm.nr are unneeded as they should be encapsulated by the bn254 module instead.

But we do not have attribute for modules?

@vezenovm
Copy link
Contributor

vezenovm commented Oct 25, 2023 •
edited
Loading

But we do not have attribute for modules?

Yeah we do not, but each method calls permute from the bn254 module which has the attribute on it. It would be nice to be able to specify on the module itself as well, but I think this is good for now.

@guipublic guipublic mentioned this pull request Oct 26, 2023
Open
@kevaundray kevaundray added this pull request to the merge queue Oct 26, 2023
@kevaundray
Copy link
Contributor

I'm fine with merging this right now, but it does feel that the attributes in noir_stdlib/src/hash/poseidon/bn254/perm.nr are unneeded as they should be encapsulated by the bn254 module instead.

But we do not have attribute for modules?

Yeah it would need to be introduced in a separate PR

Merged via the queue into master with commit cb81192 Oct 26, 2023
26 checks passed
@kevaundray kevaundray deleted the gd/bn254-attribute branch October 26, 2023 22:05
guipublic added a commit that referenced this pull request Oct 27, 2023
@guipublic
chore: add bn254 attribute when needed in the stdlib (#3208)
a1d05cb
guipublic added a commit that referenced this pull request Oct 27, 2023
@guipublic
chore: add bn254 attribute when needed in the stdlib (#3208)
fb4c8b7
guipublic added a commit that referenced this pull request Oct 30, 2023
@guipublic
chore: add bn254 attribute when needed in the stdlib (#3208)
ffacd52
TomAFrench added a commit that referenced this pull request Oct 30, 2023
@TomAFrench
Merge branch 'master' into kh-update-nix
025a8c0 
* master: (242 commits)
  chore(docs): Update Windows installation  (#3326)
  chore!: change stdlib function `pedersen` to `pedersen_commitment` (#3341)
  chore: add back algolia recrawler (#3332)
  chore: comment out algolia recrawler workflow as its failing master (#3331)
  chore: Modify single line if-else expression width threshold (#3329)
  feat: Expand trait impl overlap check to cover generic types (#3320)
  feat: Implement where clauses on impls (#3324)
  chore: builtin wrapping shift left (#3270)
  chore: format integration tests  (#3257)
  fix(3300): cache warnings into debug artefacts (#3313)
  fix(3275): activate brillig modulo test with negative integers (#3318)
  feat: Add check for overlapping generic traits (#3307)
  feat: Refactor debugger and separate core from UI (#3308)
  chore: recrawl docs on merge (#3306)
  chore: add bn254 attribute when needed in the stdlib (#3208)
  feat: add exports of JS black box solvers to noirJS (#3295)
  chore: upload acir artifacts as a github artifact (#3288)
  chore: bump bb version to 0.12.0 (#3304)
  chore(docs): Supplement descriptions for defaulting loop indices to be `u64` (#3237)
  chore: create publish-docs.yml (#3298)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevaundray kevaundray kevaundray left review comments

@vezenovm vezenovm vezenovm approved these changes

@jfecher jfecher jfecher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guipublic @vezenovm @kevaundray @jfecher