refactor!: improve the aztec::history module#19956
Conversation
nventuro
changed the title
aztec::history moduleaztec::history module
| @@ -1,2 +1,2 @@ | |||
| --- | |||
| title: Proving Historic State | |||
There was a problem hiding this comment.
@critesjosh @ciaranightingale this is an example of a docs page that is more apiref than anything else. This PR improves the structure of the history module but does not yet make the docs great (in fact only two functions have docs). We'll quickly add these, and at that point this page would likely become superfluous, at least given its current content.
I'm curious if you'd then delete this, keep a redundant copy, or change the content into something else (e.g. more high level explanations? examples of patterns? idk).
There was a problem hiding this comment.
I think we should keep some version of the page, but likely trim out the code references. Having a docs page is useful for discovery, people can find it via the sidebar or with the docs search. On that page we can link to the apiref docs for the details.
There was a problem hiding this comment.
We'd be keeping a worse copy of what already exists, and forcing people to go through that, in the hope that once they click through and go deeper the docs improve (which is not intuitive). I'd try to find a better solution.
benesjan
left a comment
benesjan
left a comment
There was a problem hiding this comment.
Brilliant!
So satisfying to see the bad naming choices from the past get eliminated.
Good call dropping the traits 👍
| where | ||
| Note: NoteHash, | ||
| { | ||
| let confirmed_note = assert_note_existed_by(block_header, hinted_note); |
There was a problem hiding this comment.
Such a cute transformation 🥰
| Note: NoteHash, | ||
| { | ||
| let note_hash_for_nullification = compute_confirmed_note_hash_for_nullification(confirmed_note); | ||
| let inner_nullifier = confirmed_note.note.compute_nullifier( |
There was a problem hiding this comment.
Unrelated to your PR but what do you think about the nullifier naming?
Here we have compute_nullifier that returns inner_nullifier and then we also have the siloed nullifier thingy which goes into the tree but in the case of notes it's the unique note hash that goes into tree.
Feels like it would help the protocol legibility a lot if we had fixed terms for the "thing that goes to the tree" and for the thing that's computed based just on the "initial input data".
There was a problem hiding this comment.
My current thinking is that compute_nullifier might need to be renamed to compute_inner_nullifier.
|
Since my suggestions were I believe non-controversial I applied them and added this to merge queue. |
Long overdue. This clears up the `history` mod, removing the unnecessary traits and exposing functions instead, fixing lots of bad naming that confused people, addressing some clarifications re. siloed vs unsiloed params, and adding some docs where it was most needed. Fixes F-240. Built with Claude 🤖 (the docs are mine) Co-authored-by: Jan Beneš <janbenes1234@gmail.com> Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
AztecBot
force-pushed
the
nv/history-revamp
branch
from
ab580e8 to
c2ef6e8
Compare
4 participants
Long overdue. This clears up the
historymod, removing the unnecessary traits and exposing functions instead, fixing lots of bad naming that confused people, addressing some clarifications re. siloed vs unsiloed params, and adding some docs where it was most needed.Fixes F-240.
Built with Claude 🤖 (the docs are mine)