feat!: note type ids#4500
Merged
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2820. Additional context and design discussion can be found here.
This PR introduces the notion of Note Type IDs, identifiers for each note type that are unique within a contract. Different contracts may share the same IDs, and even use the same ID for notes of different types. Note Type IDs are only meaningful when referring to a single contract.
I considered two ways to implement this. The first was to add a new field to
NoteHeaderand leave ID allocation up to the contract in a similar way storage slots are determined. The call tonewfor each state variable would populate this field and make it available down the call stack. This results in a number of issues, including creating a dependency on the note type id in places where we need the header but don't care about this new value (e.g. when hashing), and forcing us to change thenew()signature to have an extra field (so that the macro will work nicely on all state variables), despite it being entirely unused in some contexts (such as public state variables, which require no note type id as there's no hashing/nullification).An alternative approach (the one followed in this PR) is to make the note type id a property of the note's themselves via a trait function
get_note_type_id()which returns a constant. This results in a massively smaller diff as we don't need to change anything about state variables, theirnewfunction nor note headers: we simply fetch the note type deep in the call stack right before calling the oracle, avoiding a lot of passing values around (I'd imagine this also leads to a smaller circuit, but I haven't actually measured it).The only problem here is that due to the orphan rule, we can't have a contract implement an external trait (the one for
get_note_type_id()) for an external type (the note): the implementation must be in either of these two modules. It is possible to alter the note module via macros (something like aderivemacro, which we already want to automatically serialize and deserialize notes), but the current macros are not yet suitable to do this. @vezenovm is working on a related feature that lays some of the foundation to what we want to achieve here.Eventually we'll be able to produce all of the impls for
get_note_type_idby keeping track of a per-contract counter (essentially enumerating all imported note types - see #4519). For now, I simply provided unique hardcoded values which correspond to the name of the note type. Once this is done, implementing #2918 should be trivial. Other future work includes exporting these type ids as part of the contract artifact (#4518).Something that is potentially missing here is an example of a contract that uses two maps with different underlying note types. I'd leave that for a separate PR to not pollute this one with a bunch of new examples and associated tests, since it's already relatively large and touches a large number of files.
Minor curiosity: the PXE service uses
ExtendedNoteboth to add and return notes. For simplicity, I added the type id to this type, meaning we end up storing the type id in the local note database. This is not strictly required, but the impact is negligible and it results in easier debugging and a (much!) smaller diff.