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

Create a macro that injects compute_note_hash_and_nullifier #2918

Closed
Tracked by #4126
benesjan opened this issue Oct 18, 2023 · 4 comments · Fixed by #4610
Closed
Tracked by #4126

Create a macro that injects compute_note_hash_and_nullifier #2918

benesjan opened this issue Oct 18, 2023 · 4 comments · Fixed by #4610
Assignees

Comments

@benesjan
Copy link
Contributor

benesjan commented Oct 18, 2023

compute_note_hash_and_nullifier is a boilerplate and we should inject it with macros. We want to give devs the ability to define the function on their own. This is straightforward to do because we can inject it only if it doesn't already exist.

To generate compute_note_hash_and_nullifier we need:

  1. names of the NoteInterface (e.g. AddressNoteMethods) and its import path (it might not be imported in the contract because the interface could be used in a custom type and not directly passed to e.g. Set in Storage struct),
  2. we need to somehow extract the storage slot of the notes defined in the Storage struct.

To make it more complicated we need to extract the info above only when the Notes are used in a private context and the information about the NoteInterface might be nested deep in custom type definitions like is done for example in our Token contract:
image

This is the function definition in the Token contract:
image
Highlighted is the info we need to obtain.

@Maddiaa0 pointed out that we might need to annotate each the storage struct with state visibility of each type (e.g. with #[storage(private)]) but I am not sure if it helps us with extracting the NoteInterface name.

@sirasistant @Maddiaa0 any ideas how to solve this?

Side note: With Sean's help today I've already implemented the compute_note_hash_and_nullifier presence check in this PR. The issue with that PR is that it forces users to define the function even when they work with public state only. Since solving that issue is also quite complicated I decided to bring up this one because that is the end-game.

@spalladino
Copy link
Collaborator

FWIW this relates to having a more declarative view of the storage layout, which also popped up in #2806, since that'd allow type-safe access to storage via get(Public|Private)StorageAt calls.

@nventuro
Copy link
Contributor

nventuro commented Feb 8, 2024

With #4500 this is now relatively simple to do. We'd find all implementations of NoteInterface, and then for each of them we produce a new case in the switch statement, checking against its note type id and calling the deserialize function on a match. This would also be a good opportunity to implement #4520.

Note that to find these impls we'll need to traverse the typed AST, which is passed to transform_hir and not transform. Since we're first checking for the existence of compute_note_hash_and_nullifier on the untyped AST, we'd need to move this check until later on the compilation pipeline (i.e. inside transform_hir), potentially making it so that we only provide the function if it is not already user-defined.

@nventuro
Copy link
Contributor

nventuro commented Feb 9, 2024

We also need to pick an array size that is large enough to fit all of the different types of notes in a contract. This can be done by looking at the global len values (e.g. VALUE_NOTE_LEN), or ideally by finding the generic value over which the NoteInterface trait is implemented.

github-merge-queue bot pushed a commit that referenced this issue Feb 19, 2024
Closes #2918.

This adds a new macro function that processes the unresolved trait impls
and injects a new function before name resolution takes place. This lets
us leverage the parser and write the function in Noir instead of having
to deal with more complicated processed objects.

In order to find all of the note types we look for impls of the
`NoteInterface` trait. This is a bit more involved than it seems, since
other crates may also have structs that impl this trait, and those will
have already been processed. We rely on the fact that the contract crate
is processed last, and search for external crate impls via the
NodeInterner and internal ones in the unresolved functions. This method
is robust as long as we do the contract crate after all of its imports,
which holds because the imports are recursively collected from the root
crate.

I also added a small escape hatch mechanism by skipping any code
generation if the function is already defined by the user. This could
use some polishing since it is possible for the user to e.g. provide the
function but get the arity or parameter types wrong, in which case
they'd get a duplicate definition error. Diagnosis and error messages
can be improved here
(#4647), but I
think a simple mechanism is sufficient for now.

---

### Minor issues

- One of the function arguments is a fixed-size array, which should be
as big as the largest note length. We are not yet pulling the note
length (#4649), so
I'm passing a hardcoded value for now. 12 Fields ought to be enough for
anyone.

- Doing this introduces an implicit dependency on `AztecAddress` on all
contracts. This won't be an issue once
#4496 is in, but I
did have to manually add it to some of the account contracts for now.

- I created a new macro function that is called on each crate after
collection but before resolution. Due to some internal compilers
shenanigans (mostly who holds mut references to what) I chose to
specialize this function so that for now it only passes the collected
unresolved functions, making it a bit niche for the current use case.
@vezenovm and I are planning to generalize this down the road
(#4653).

- I'm now importing in the macro from some places that were not
previously used, notably the HIR and Noir errors. I am not sure if we
might want to pull those differently - I simply imported what I needed.

- I also introduced some getters to provide mutable access to private
fields of the HIR Context and CrateDefMap. This is because we need to
modify the contract module in the def map by declaring the new function
(which is how we get things like duplicate definition detection). We're
already mutating the HIR Context in the macros, so also mutating its
members doesn't feel like a stretch.

- Finally, I don't know enough about how Noir errors work to know how to
produce a useful `Location` value for the new function, if indeed that
can be done. Providing an empty span seems to at least not cause weird
errors on the LSP plugin, so that's how I left it for now.
@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Feb 19, 2024
@TomAFrench
Copy link
Member

// TODO: remove this placeholder once https://github.com/AztecProtocol/aztec-packages/issues/2918 is implemented

This should now be able to be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants