refactor!: nuking NoteHeader#11942
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
benesjan
force-pushed
the
02-12-refactor_removing_noteheader_out_of_note
branch
from
5bc7d98 to
d2bafea
Compare
Changes to public function bytecode sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
benesjan
changed the title
NoteHeader out of note
benesjan
force-pushed
the
02-12-refactor_removing_noteheader_out_of_note
branch
2 times, most recently
from
8a8bb1f to
a722a93
Compare
benesjan
changed the title
NoteHeader out of noteNoteHeader
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
benesjan
force-pushed
the
02-12-refactor_removing_noteheader_out_of_note
branch
from
aafc224 to
14eb329
Compare
There was a problem hiding this comment.
Thanks for tackling this! Left a bunch of comments on questions asked, vague thoughts on future things we'll do, and some observations.
In general please do try to keep things a bit tidier and split across saner changesets. The number of files involved here is massive, as is the linecount, but the vast majority of the changes are trivial, e.g. renaming note to retrieved_note, or T to NOTE in balance_set. However the changes that are not trivial are hidden among those and require careful thinking - it's quite easy to miss these important changes given the noise.
I suggest you go through the diff yourself when submitting as if you were the reviewer, and trying to think if it's easy to spot the things you want for the reader to look at and comment on, or if there's things that could be clarified or removed.
| /// A container of a note and its metadata. Metadata is needed to constrain the note when reading the note from | ||
| /// an unconstrained context and to compute the nullifier when destroying a note. | ||
| pub struct RetrievedNote<NOTE> { | ||
| pub note: NOTE, | ||
| pub contract_address: AztecAddress, | ||
| pub nonce: Field, | ||
| pub note_hash_counter: u32, // a note_hash_counter of 0 means non-transient | ||
| } |
There was a problem hiding this comment.
I find this name very confusing - at first I thought RetrievedNote meant that you had actually applied constraints and checked that the note exists, but this is just the old note + header struct. This makes sense in terms of the approach I suggested in #11860 (what I called NoteWithData), but the name is quite inapropriate given the direction we're taking (e.g. the other items mentioned in that issue, plus #8589).
I imagine it'd be unlikely for a user to run into this type - the oracle might return this, but there's then two layers of encapsulation:
- aztec.nr/notes dealing with constraining and returning a proper struct that indicates the note exists
- the state variable doing things with the read note (e.g. replacing it, removing it, unwrapping it)
Given this I think it's fine to have a descriptive un-sexy name, like NoteWithMetadata or whatever.
There was a problem hiding this comment.
Your docs actually start with A container of a note and its metadata. 😛
There was a problem hiding this comment.
Given this I think it's fine to have a descriptive un-sexy name, like NoteWithMetadata or whatever.
I don't really see a difference between RetrievedNote and NoteWithMetadata. Both don't tell me much and I need to open the struct definition to see what that is. Can we just merge this and come up with a nicer name later? I see both as only a temporary placeholder.
There was a problem hiding this comment.
The difference I had in mind was one between a user-facing name and something more internal, but I wasn't actually suggesting that we rename this now, more rambling and gathering my thoughts as I went through the review. I agree the rename can wait, sorry I did not clarify that earlier.
| // TODO: For non-transient notes, the contract address appears to be implicitly checked since we return a siloed | ||
| // note hash from the `compute_note_hash_for_read_request(...)` function. Investigate whether this check is needed | ||
| // for transient notes and if not, remove it. | ||
| assert( | ||
| retrieved_note.contract_address.eq(context.this_address()), | ||
| "Note contract address mismatch.", | ||
| ); |
There was a problem hiding this comment.
This check is almost free anyway, it'd cost more constraints to test if the note is transient and then conditionally do it.
There was a problem hiding this comment.
I just wanted a verification from the reviewer that the check is here really only for transient notes. Based on your reaction I see it's the case and I've updated the comment in this commit. LMK if it looks good to you. (will try to not mess the commit link with graphite 😝).
There was a problem hiding this comment.
The new comment is fantastic thanks!
benesjan
force-pushed
the
02-12-refactor_removing_noteheader_out_of_note
branch
from
edd3cac to
b6aa786
Compare
* master: (245 commits) chore: Fix unbound CI variable on release image bootstrap (#12095) fix: dry run on grind (#12088) fix(spartan): eth-execution logging (#12094) fix: aws_handle_evict recovery & termination (#12086) chore: Use native acvm when available on orchestrator tests (#11560) refactor: function macros cleanup (#12066) refactor: remove `addNullifiedNote` from pxe (#11822) fix: `#[aztec]` macro warnings (#12038) refactor!: Notes implementing `Packable<N>` (#12004) chore(ops): add gcloud cli into devbox base image (#12082) fix: hotfix grinding fix: L1 deployment on reth (#12060) fix: Add missing bootstrap fast aliases (#12078) fix: hash_str caching (#12074) fix: Naive attempt to fix nightly deployments (#12079) fix: kind smoke (#12084) refactor!: nuking `NoteHeader` (#11942) fix: inject dockerhub creds (#12072) feat(docs): Note discovery concepts page (#11760) chore: cleanup libp2p logger (#12058) ...
Follow up to AztecProtocol/aztec-packages#11942. This improves the readability and management of code that reads and destroys notes by introducing the `NoteMetadata` struct, which handles the different combinations of note hash counter and nonce internally. This is a stepping stone towards #8589.
3 participants
Fixes #11860
Fixes #11861