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

Inject (nullifier, commitment) tuple. #869

Merged
merged 18 commits into from
Jul 20, 2023
Merged

Inject (nullifier, commitment) tuple. #869

merged 18 commits into from
Jul 20, 2023

Conversation

suyash67
Copy link
Contributor

@suyash67 suyash67 commented Jun 16, 2023 •
edited
Loading

Description

resolves #863

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.

@suyash67 suyash67 force-pushed the sb/null-comm-tuple branch 2 times, most recently from 14cd454 to 3b6f240 Compare June 22, 2023 11:22
@suyash67 suyash67 force-pushed the sb/null-comm-tuple branch 2 times, most recently from 9dced87 to 3a96fb7 Compare July 18, 2023 18:52
@suyash67 suyash67 marked this pull request as ready for review July 18, 2023 21:01
dbanks12
dbanks12 previously requested changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@dbanks12 dbanks12 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! Just responded to your comments so once you update / remove those TODOs it's good to merge

yarn-project/noir-contracts/src/libs/custom-notes/src/transparent_note.nr Outdated Show resolved Hide resolved
yarn-project/noir-contracts/src/libs/noir-aztec/src/state_vars/immutable_singleton.nr Outdated Show resolved Hide resolved
suyash67 and others added 15 commits July 20, 2023 07:14
@suyash67 suyash67 requested a review from jeanmon July 20, 2023 07:46
iAmMichaelConnor
iAmMichaelConnor reviewed Jul 20, 2023
View reviewed changes
yarn-project/noir-libs/noir-aztec/src/note/lifecycle.nr Outdated

// We also need the note commitment corresponding to the "nullifier"
let compute_note_hash = note_interface.compute_note_hash;
let note_hash = compute_note_hash(note);
Copy link
Contributor

Choose a reason for hiding this comment

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

By this point, we'll have already called compute_note_hash when we called get*, so it would be inefficient to call it again here.

@LeilaWang perhaps the Note needs to contain a member note_hash: Option<Field>, so that it can be retrieved by functions such as this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also nullifier: Option<Field> too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was aware that we're recomputing the note commitment. Didn't find a way to retrieve it while computing the nullifier. One way could be to change the compute_nullifier interface to return the pair (nullifier, commitment).

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've added a TODO which we'll take care of as a separate PR.

jeanmon
jeanmon approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@jeanmon jeanmon left a comment

Choose a reason for hiding this comment

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

LGTM. My comments are very minor.

circuits/cpp/src/aztec3/constants.hpp Outdated Show resolved Hide resolved
yarn-project/circuits.js/src/structs/private_circuit_public_inputs.ts Outdated Show resolved Hide resolved
@jeanmon jeanmon requested a review from dbanks12 July 20, 2023 08:59
@jeanmon jeanmon dismissed dbanks12’s stale review July 20, 2023 09:29

We need this one in master and comments have been addressed AFAIK

@jeanmon jeanmon merged commit 15c73f1 into master Jul 20, 2023
@jeanmon jeanmon deleted the sb/null-comm-tuple branch July 20, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iAmMichaelConnor iAmMichaelConnor iAmMichaelConnor left review comments

@jeanmon jeanmon jeanmon approved these changes

@dbanks12 dbanks12 Awaiting requested review from dbanks12

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

Successfully merging this pull request may close these issues.

[Noir, TS, C++] App circuit should output the commitment that a nullifier corresponds to
4 participants
@suyash67 @iAmMichaelConnor @dbanks12 @jeanmon