chore(noir-libs): TransparentNote rework #1412
Conversation
Co-authored-by: Michael Connor <[email protected]>
…c-packages into db/1210-rework-nonces
…col/aztec-packages into db/1210-rework-nonces
| // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386): should use | ||
| // `compute_note_hash_for_read_or_nullify` once public functions inject nonce! | ||
| let siloed_note_hash = compute_siloed_note_hash(TransparentNoteMethods, self); | ||
| pedersen([self.secret, siloed_note_hash, self.leafIndex])[0] |
There was a problem hiding this comment.
in this construction will leaf index ever not be 0?
There was a problem hiding this comment.
Good catch. Realistically we should just get rid of this leafIndex. Once we have nonces all working for public, the commitments themselves will be unique and this won't be necessary. I'll just remove leafIndex here for now and will link the ticket to add nonces here.
| note: &mut Note, | ||
| note_interface: NoteInterface<Note, N>, | ||
| ) { | ||
| let mut inner_note_hash = 0; |
There was a problem hiding this comment.
This does not need to be initialised up here, it can just be assigned on line 46
There was a problem hiding this comment.
Good catch. I'll fix create_note too.
| context: &mut Context, | ||
| storage_slot: Field, | ||
| note_interface: NoteInterface<Note, N>, | ||
| note: &mut Note, |
There was a problem hiding this comment.
I dont think note needs to be mutable here?
There was a problem hiding this comment.
The note ends up getting its header populated although the existing NonNativeToken contract does not actually use the updated note. But this mimics the ensure_note_exists function above it.
| // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386): rename to | ||
| // `assert_contains` and replace function above ^ once public kernel injects | ||
| // nonces to note hashes. | ||
| fn assert_contains_note_hash(self, context: &mut Context, note: &mut Note) { |
There was a problem hiding this comment.
assert_note_hash is maybe enough?
There was a problem hiding this comment.
🤔 maybe but I'm mimicking assert the above function assert_contains. Eventually we can replace assert_contains with this implementation. Right now assert_contains_note_hash only works for notes created in public.
We are planning to rename commitment to noteHash just about everywhere. So I have been using the term noteHash in all new instances in the meantime. It is already being used often in Noir, although I agree that it is very confusing that we are using both terms. |
dbanks12
force-pushed
the
db/1194-transparentnote-rework
branch
from
a3e4bd3 to
44b67e1
Compare
Closes #1194
Closes #1363
Will likely rename getCommitment alongside all instances of "commitment" as a part of this other issue: #1408
Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.