refactor(Notes): Rename Singleton.getNote to convey "update"
#3123
Comments
|
throughout aztec-nr, we use Maybe we shoukd just refactor the name altogether in aztec-nr to what mike suggested. What do you think? |
rahul-kothari
added
the
S-needs-discussion
|
@rahul-kothari I think Mike's names are definitely better but I can also appreciate the consistency of view_note() naming with view() call. The issue here is that if you don't know the API that well it's easy to do the mistake of using get_notes in unconstrained context. I think probably the best solution would be to improve the error message. Currently we show someting like "brillig trap hit". If we instead manage to show "calling a constrained function get_note() from an unconstrained context is prohibited" and it pointed to the line there would be no issue. Even a much simpler message of "calling a constrained function from an unconstrained context is prohibited" would suffice although it would make sense to invest time in this because I think this will be quite a common issue. @sirasistant do you know if it would be tricky to improve the error message here and point to the line where the constrained function was called? |
|
The error itself should have a noir callstack showing all the calls that lead to the blow up, If that's not the case, there is a bug :( pub fn get_note(self) -> Note {
let context = self.context.unwrap();
let storage_slot = self.storage_slot;
get_note(context, storage_slot, self.note_interface)
}to check if is_some() and if not assert with an appropiate error message? |
|
@sirasistant this is what the error looks like when I incorrectly use the get_note method: so it correctly shows the callstack. As you propose, we could either add there the assert with a nice error message or maybe the best solution of all would be implementing this issue Santiago proposed. |
|
Potential name for |
|
Tested on current version. Tx with:
The immutable What do people think about On the part of naming. Anyone know how come we are using @benesjan you ok with me altering the name of your issue to be singleton instead since Related, but the immutable here is also just application-enforced and not protocol-enforced. |
|
@LHerskind Sure, feel free to update the issue as necessary. |
LHerskind
changed the title
ImmutableSingleton.getNoteSingleton.getNote to convey "update"
|
Another task in the issue can be to throw error when someone calls
@iAmMichaelConnor: what are your thoughts on the proposed names? |
|
Going over all of this it seems like there's maybe two issues being conflated: the unhelpful error message when calling The first item (@benesjan's original issue) is perhaps better tackled by simply improving the error message. Note that this is not just about singletons: sets also feature a pair of get/set functions which can result the same frustrating debugging scenario. As suggested here we can test the The second item has to do with the safety guarantees that
|
Actually @kevaundray is working on it rn! |
|
We decided that |
LHerskind
changed the title
Singleton.getNote to convey "update"Singleton.getNote to convey "update"
|
@LHerskind is this sitll blocked on Noir or anything? |
|
Closes #5886. This removes the `Context` struct and makes all state variables generic over a new `Context` type parameter, with each state variable providing implementations for `PrivateContext`, `PublicContext` or `()` (used to marked `unconstarined` - more on this later). The end result is that we get compile-time errors when calling functions that are unavailable in the current context, reduced wrapping and unwrapping, and no obscure `explicit trap hit in brilling 'self._is_some'` runtime errors, such as in #3123. ## How The implementation is prety much as described in #5886, except instead of using traits we specialize the type directly for the contexts we know. ```rust struct MyStateVar<Context> { context: Context } impl MyStateVar<&mut PrivateContext> { fn private_read() { } } ``` This works because there's only a couple of them, and users are not expected to extend them. The macros were altered so that instead of wrapping the context object in `Context` and then passing that, we simply pass the raw object to the `Storage::init` function. This means that `Storage` itself is now also generic, resulting in some unfortunate new boilerplate in the struct declaration. All instances of `self.context.private.unwrap()` and friends were removed: each function is now available under the corresponding `impl` block that is specialized for the corresponding context. We could even rename some since `read_public` and `read_private` is no longer required: both impls can have `read` functions since they are effectively methods for different types, so the shared name is a non-issue. ## Oddities This change revelead a number of small bugs in the codebase, in the form of uncallable functions. These were undetected since no test called them. I'll do a pass over the entire PR and leave comments where relevant. ## Top-level unconstrained This PR continues the formalization of what I've been calling 'top-level unconstrained' (i.e. an unconstrained contract function) as a fundamental Aztec.nr concept and third execution environment, alongside private and public execution. So far we've been accessing oracles in these unconstrained functions without much care (sometimes for perfomance reasons - see #5911), but the new stricter compile-time checks force us to be a bit more careful. In my mind, we are arriving at the following model: - public execution: done by the sequencer, can be simulated locally with old data, not unlike the evm - private execution: able to produce valid private kernel proofs with side effects collected in the context - top-level unconstrained execution: local computation using both private and old public data, with certain restrictions from private exec lifted (e.g. unbounded loops), unable to produce any kind of proofs or reason about state changes. only useful for computing values doing arbitrary computation over both private and public state, with zero validation and guarantees of correctness Private execution requires a context object a it needs to collect side effects, but public notably does not - it simply calls oracles and gets them to do things. In this sense, the `PublicContext` type is acting as a marker of the current execution environment in order to prevent developers from accidentally doing things that are invalid in public, which would otherwise result in either transpilation error or inability to create public kernel proofs. This means that we may want a third `UnconstrainedContext` to act as a similar marker for this third type (where we can e.g. call `view_notes`, read old public state, etc.). It currently doesn't exist: we simply have `Context::none()`, and it is defined as the absense of one of the other contexts. Because of this, I chose to temporarily use the unit type (`()`) to mark this environment. Note that in some cases the different execution environments share code paths: `view_notes` is simply `get_notes` without any constraints, and public storage reads are performed by calling the same oracles in both public and unconstrained. I imagine small differences will arise in the future, specially as work on the AVM continues. --------- Co-authored-by: esau <152162806+sklppy88@users.noreply.github.com> Co-authored-by: thunkar <gregojquiros@gmail.com>
Closes #5886. This removes the `Context` struct and makes all state variables generic over a new `Context` type parameter, with each state variable providing implementations for `PrivateContext`, `PublicContext` or `()` (used to marked `unconstarined` - more on this later). The end result is that we get compile-time errors when calling functions that are unavailable in the current context, reduced wrapping and unwrapping, and no obscure `explicit trap hit in brilling 'self._is_some'` runtime errors, such as in AztecProtocol/aztec-packages#3123. ## How The implementation is prety much as described in #5886, except instead of using traits we specialize the type directly for the contexts we know. ```rust struct MyStateVar<Context> { context: Context } impl MyStateVar<&mut PrivateContext> { fn private_read() { } } ``` This works because there's only a couple of them, and users are not expected to extend them. The macros were altered so that instead of wrapping the context object in `Context` and then passing that, we simply pass the raw object to the `Storage::init` function. This means that `Storage` itself is now also generic, resulting in some unfortunate new boilerplate in the struct declaration. All instances of `self.context.private.unwrap()` and friends were removed: each function is now available under the corresponding `impl` block that is specialized for the corresponding context. We could even rename some since `read_public` and `read_private` is no longer required: both impls can have `read` functions since they are effectively methods for different types, so the shared name is a non-issue. ## Oddities This change revelead a number of small bugs in the codebase, in the form of uncallable functions. These were undetected since no test called them. I'll do a pass over the entire PR and leave comments where relevant. ## Top-level unconstrained This PR continues the formalization of what I've been calling 'top-level unconstrained' (i.e. an unconstrained contract function) as a fundamental Aztec.nr concept and third execution environment, alongside private and public execution. So far we've been accessing oracles in these unconstrained functions without much care (sometimes for perfomance reasons - see AztecProtocol/aztec-packages#5911), but the new stricter compile-time checks force us to be a bit more careful. In my mind, we are arriving at the following model: - public execution: done by the sequencer, can be simulated locally with old data, not unlike the evm - private execution: able to produce valid private kernel proofs with side effects collected in the context - top-level unconstrained execution: local computation using both private and old public data, with certain restrictions from private exec lifted (e.g. unbounded loops), unable to produce any kind of proofs or reason about state changes. only useful for computing values doing arbitrary computation over both private and public state, with zero validation and guarantees of correctness Private execution requires a context object a it needs to collect side effects, but public notably does not - it simply calls oracles and gets them to do things. In this sense, the `PublicContext` type is acting as a marker of the current execution environment in order to prevent developers from accidentally doing things that are invalid in public, which would otherwise result in either transpilation error or inability to create public kernel proofs. This means that we may want a third `UnconstrainedContext` to act as a similar marker for this third type (where we can e.g. call `view_notes`, read old public state, etc.). It currently doesn't exist: we simply have `Context::none()`, and it is defined as the absense of one of the other contexts. Because of this, I chose to temporarily use the unit type (`()`) to mark this environment. Note that in some cases the different execution environments share code paths: `view_notes` is simply `get_notes` without any constraints, and public storage reads are performed by calling the same oracles in both public and unconstrained. I imagine small differences will arise in the future, specially as work on the AVM continues. --------- Co-authored-by: esau <152162806+sklppy88@users.noreply.github.com> Co-authored-by: thunkar <gregojquiros@gmail.com>
|
Outdated issue |
ImmutableSingleton.getNotemodifies state and I assumed it doesn't which made me use it unconstrained context and waste time debugging it. We should either rename it to clarify that it modifies state, throw an error message showing that calling the function from unconstrained context is prohibited or, as Santiago proposed here, allow for calling of constrained function from unconstrained context.The text was updated successfully, but these errors were encountered: