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: generating Noir constants #1066

Merged
merged 9 commits into from
Jul 14, 2023
Merged

Conversation

benesjan
Copy link
Contributor

Description

Fixes #1065

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@benesjan benesjan marked this pull request as draft July 14, 2023 08:25
@benesjan benesjan force-pushed the janb/generating-noir-constants branch from ce96a86 to 5b6e57c Compare July 14, 2023 12:18
@benesjan benesjan marked this pull request as ready for review July 14, 2023 12:19
@benesjan benesjan requested a review from ludamad July 14, 2023 12:19
@benesjan benesjan enabled auto-merge (squash) July 14, 2023 12:19
global ARGS_LENGTH: comptime Field = 16;
global RETURN_VALUES_LENGTH: comptime Field = 4;
global READ_REQUESTS_LENGTH: comptime Field = 4;
global MAX_NEW_COMMITMENTS_PER_CALL: comptime Field = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about some of these GENERATOR__ names but looks good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt like prefixing it with enum name is the best solution given that there are no native enums in Noir. Kev told me that enums can be approximated with structs but that seems like unnecessary complexity to me.

Or would you prefer some other prefix?

@@ -50,7 +50,7 @@ impl<Note, N, S, P> NoteGetterOptions<Note, N, S, P> {
}

fn with_filter(
filter: fn ([Note; MAX_READ_REQUESTS], P) -> [Note; S],
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, this seems like it should still have MAX in the name, not sure how to reconcile that

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe if some of the constants went into a limits crate and we had limits::READ_REQUESTS_LENGTH

@benesjan benesjan merged commit 26a02f7 into master Jul 14, 2023
@benesjan benesjan deleted the janb/generating-noir-constants branch July 14, 2023 12:54
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.

Generate Noir constants from C++
2 participants