-
Notifications
You must be signed in to change notification settings - Fork 244
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
Method calls don't apply trait constraints #4124
Comments
#[oracle(storageRead)]
fn storage_read_oracle<N>(_storage_slot: Field, _number_of_elements: Field) -> [Field; N] {} Glancing at this line, I'm guessing this certainly affects the problem. How would an oracle return a field of any size N for the same inputs? From the signature alone, We should fix this so that it doesn't panic, but I think a proper fix would make this a compiler error. |
After looking in-depth more at the program, since the result of that function is constrained with the type annotation in the During monomorphization, the function reference to |
I've found the core issue. The contents of assert(ps.read() == value); On this line, the compiler is never actually applying the |
# Description ## Problem\* Resolves #4124 Resolves #4095 ## Summary\* We were never applying trait constraints from method calls before. These have been handled for other identifiers since #4000, but not for method calls which desugar to a function identifier that is called, then type checked with its own special function. I've fixed this by removing the special function and recursively type checking the function call they desugar to instead. This way we have less code duplication and only need to fix things in one spot in the future. ## Additional Context It is a good day when you get to fix a bug by removing code. This is a draft currently because I still need: - [x] To add `&mut` implicitly where applicable to the function calls that are now checked recursively - [x] To add the test case I'm using locally ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
Closes: #3198, #2928 ~~Requires #4135, which is blocked by noir-lang/noir#4124 Automatic storage initialization via aztec macro. Full support of public and private state from `dep::aztec::state_vars::*`, including Maps (and nested Maps!) Limited support for custom types (as long as they have a single serializable generic and their constructor is `::new(context, storage_slot`). ~~Pending: better errors, code comments and some cleanup.~~ Hijacking my own [comment](#4200 (comment)) for the explanation: The idea behind this is that in 99% of cases, storage initialization (that is, the `impl` for a given `struct Storage...` is redundant, and the only need for its existence was assigning storage slots...which in turn were necessary because we didn't know how to serialize the data structures that were used in a given contract or how much space they used once serialized (relevant for the public state). After #4135 is merged, both of those things don't have to be explicitly provided since we're using traits, so the aztec macro can infer the implementation of the Storage struct just by taking hints from the definition. An example: ```rust struct Storage { // MyAwesomeStuff implements Serialize<2>, so we assign it slot 1 (and remember that it will take 2 slots due to its size) public_var: PublicState<MyAwesomeSuff>, // Right after the first one, assign it to slot: current_slot + previous_size = 3 another_public_var: PublicState<MyAwesomeSuff>, // Private and Public state don't share slots since they "live" in different trees, but keeping the slot count simplifies implementation. // Notes also implement Serialize<N>, but they only take up 1 slot anyways because of hashing, assign it slot 5 a_singleton: Singleton<ANote>, // Maps derive slots via hashing, so we can assume they only "take" 1 slot. We assign it slot 6 balances: Map<AztecAddress, Singleton<ANote>>, // Slot 7 a_set: Set<ANote>, // Slot 8 imm_singleton: ImmutableSingleton<ANote>, // Slot 9. profiles: Map<AztecAddress, Map<Singleton<ANote>>>, } ``` We have all the info we need in the AST and HIR to build this automatically: ```rust impl Storage { fn init(context: Context) -> Self { Storage { public_var: PublicState::new(context, 1), // No need for serialization methods, taken from the the trait impl another_public_var: PublicState::new(context, 3), a_singleton: Singleton::new(context, 5), // Map init lambda always takes the same form for known storage structs balances: Map::new(context, 6, |context, slot| { Singleton::new(context, slot) }), a_set: Set::new(context, 7), imm_singleton: ImmutableSingleton::new(context, 8), // A map of maps is just nesting lambdas, we can infer this too profiles: Map::new(context, 9, |context, slot| { Map::new(context, slot, |context, slot| { Singleton::new(context, slot) }) }) } } } ``` ...as long as we use "canonical" storage implementations. This means `AStoragePrimitive<SomethingSerializable>` and `Map<SomethingWithToField, AStoragePrimitive<SomethingSerializable>>`. **TLDR:** define the Storage struct, in 99% of cases the macro takes care of the implementation! Implementing custom storage will look just like it does know, the macro will skip automatic generation if it finds one. --------- Co-authored-by: sirasistant <[email protected]>
Closes: AztecProtocol/aztec-packages#3198, AztecProtocol/aztec-packages#2928 ~~Requires AztecProtocol/aztec-packages#4135, which is blocked by noir-lang/noir#4124 Automatic storage initialization via aztec macro. Full support of public and private state from `dep::aztec::state_vars::*`, including Maps (and nested Maps!) Limited support for custom types (as long as they have a single serializable generic and their constructor is `::new(context, storage_slot`). ~~Pending: better errors, code comments and some cleanup.~~ Hijacking my own [comment](AztecProtocol/aztec-packages#4200 (comment)) for the explanation: The idea behind this is that in 99% of cases, storage initialization (that is, the `impl` for a given `struct Storage...` is redundant, and the only need for its existence was assigning storage slots...which in turn were necessary because we didn't know how to serialize the data structures that were used in a given contract or how much space they used once serialized (relevant for the public state). After AztecProtocol/aztec-packages#4135 is merged, both of those things don't have to be explicitly provided since we're using traits, so the aztec macro can infer the implementation of the Storage struct just by taking hints from the definition. An example: ```rust struct Storage { // MyAwesomeStuff implements Serialize<2>, so we assign it slot 1 (and remember that it will take 2 slots due to its size) public_var: PublicState<MyAwesomeSuff>, // Right after the first one, assign it to slot: current_slot + previous_size = 3 another_public_var: PublicState<MyAwesomeSuff>, // Private and Public state don't share slots since they "live" in different trees, but keeping the slot count simplifies implementation. // Notes also implement Serialize<N>, but they only take up 1 slot anyways because of hashing, assign it slot 5 a_singleton: Singleton<ANote>, // Maps derive slots via hashing, so we can assume they only "take" 1 slot. We assign it slot 6 balances: Map<AztecAddress, Singleton<ANote>>, // Slot 7 a_set: Set<ANote>, // Slot 8 imm_singleton: ImmutableSingleton<ANote>, // Slot 9. profiles: Map<AztecAddress, Map<Singleton<ANote>>>, } ``` We have all the info we need in the AST and HIR to build this automatically: ```rust impl Storage { fn init(context: Context) -> Self { Storage { public_var: PublicState::new(context, 1), // No need for serialization methods, taken from the the trait impl another_public_var: PublicState::new(context, 3), a_singleton: Singleton::new(context, 5), // Map init lambda always takes the same form for known storage structs balances: Map::new(context, 6, |context, slot| { Singleton::new(context, slot) }), a_set: Set::new(context, 7), imm_singleton: ImmutableSingleton::new(context, 8), // A map of maps is just nesting lambdas, we can infer this too profiles: Map::new(context, 9, |context, slot| { Map::new(context, slot, |context, slot| { Singleton::new(context, slot) }) }) } } } ``` ...as long as we use "canonical" storage implementations. This means `AStoragePrimitive<SomethingSerializable>` and `Map<SomethingWithToField, AStoragePrimitive<SomethingSerializable>>`. **TLDR:** define the Storage struct, in 99% of cases the macro takes care of the implementation! Implementing custom storage will look just like it does know, the macro will skip automatic generation if it finds one. --------- Co-authored-by: sirasistant <[email protected]>
Closes: #3198, #2928 ~~Requires #4135, which is blocked by noir-lang/noir#4124 Automatic storage initialization via aztec macro. Full support of public and private state from `dep::aztec::state_vars::*`, including Maps (and nested Maps!) Limited support for custom types (as long as they have a single serializable generic and their constructor is `::new(context, storage_slot`). ~~Pending: better errors, code comments and some cleanup.~~ Hijacking my own [comment](#4200 (comment)) for the explanation: The idea behind this is that in 99% of cases, storage initialization (that is, the `impl` for a given `struct Storage...` is redundant, and the only need for its existence was assigning storage slots...which in turn were necessary because we didn't know how to serialize the data structures that were used in a given contract or how much space they used once serialized (relevant for the public state). After #4135 is merged, both of those things don't have to be explicitly provided since we're using traits, so the aztec macro can infer the implementation of the Storage struct just by taking hints from the definition. An example: ```rust struct Storage { // MyAwesomeStuff implements Serialize<2>, so we assign it slot 1 (and remember that it will take 2 slots due to its size) public_var: PublicState<MyAwesomeSuff>, // Right after the first one, assign it to slot: current_slot + previous_size = 3 another_public_var: PublicState<MyAwesomeSuff>, // Private and Public state don't share slots since they "live" in different trees, but keeping the slot count simplifies implementation. // Notes also implement Serialize<N>, but they only take up 1 slot anyways because of hashing, assign it slot 5 a_singleton: Singleton<ANote>, // Maps derive slots via hashing, so we can assume they only "take" 1 slot. We assign it slot 6 balances: Map<AztecAddress, Singleton<ANote>>, // Slot 7 a_set: Set<ANote>, // Slot 8 imm_singleton: ImmutableSingleton<ANote>, // Slot 9. profiles: Map<AztecAddress, Map<Singleton<ANote>>>, } ``` We have all the info we need in the AST and HIR to build this automatically: ```rust impl Storage { fn init(context: Context) -> Self { Storage { public_var: PublicState::new(context, 1), // No need for serialization methods, taken from the the trait impl another_public_var: PublicState::new(context, 3), a_singleton: Singleton::new(context, 5), // Map init lambda always takes the same form for known storage structs balances: Map::new(context, 6, |context, slot| { Singleton::new(context, slot) }), a_set: Set::new(context, 7), imm_singleton: ImmutableSingleton::new(context, 8), // A map of maps is just nesting lambdas, we can infer this too profiles: Map::new(context, 9, |context, slot| { Map::new(context, slot, |context, slot| { Singleton::new(context, slot) }) }) } } } ``` ...as long as we use "canonical" storage implementations. This means `AStoragePrimitive<SomethingSerializable>` and `Map<SomethingWithToField, AStoragePrimitive<SomethingSerializable>>`. **TLDR:** define the Storage struct, in 99% of cases the macro takes care of the implementation! Implementing custom storage will look just like it does know, the macro will skip automatic generation if it finds one. --------- Co-authored-by: sirasistant <[email protected]>
Closes: AztecProtocol#3198, AztecProtocol#2928 ~~Requires AztecProtocol#4135, which is blocked by noir-lang/noir#4124 Automatic storage initialization via aztec macro. Full support of public and private state from `dep::aztec::state_vars::*`, including Maps (and nested Maps!) Limited support for custom types (as long as they have a single serializable generic and their constructor is `::new(context, storage_slot`). ~~Pending: better errors, code comments and some cleanup.~~ Hijacking my own [comment](AztecProtocol#4200 (comment)) for the explanation: The idea behind this is that in 99% of cases, storage initialization (that is, the `impl` for a given `struct Storage...` is redundant, and the only need for its existence was assigning storage slots...which in turn were necessary because we didn't know how to serialize the data structures that were used in a given contract or how much space they used once serialized (relevant for the public state). After AztecProtocol#4135 is merged, both of those things don't have to be explicitly provided since we're using traits, so the aztec macro can infer the implementation of the Storage struct just by taking hints from the definition. An example: ```rust struct Storage { // MyAwesomeStuff implements Serialize<2>, so we assign it slot 1 (and remember that it will take 2 slots due to its size) public_var: PublicState<MyAwesomeSuff>, // Right after the first one, assign it to slot: current_slot + previous_size = 3 another_public_var: PublicState<MyAwesomeSuff>, // Private and Public state don't share slots since they "live" in different trees, but keeping the slot count simplifies implementation. // Notes also implement Serialize<N>, but they only take up 1 slot anyways because of hashing, assign it slot 5 a_singleton: Singleton<ANote>, // Maps derive slots via hashing, so we can assume they only "take" 1 slot. We assign it slot 6 balances: Map<AztecAddress, Singleton<ANote>>, // Slot 7 a_set: Set<ANote>, // Slot 8 imm_singleton: ImmutableSingleton<ANote>, // Slot 9. profiles: Map<AztecAddress, Map<Singleton<ANote>>>, } ``` We have all the info we need in the AST and HIR to build this automatically: ```rust impl Storage { fn init(context: Context) -> Self { Storage { public_var: PublicState::new(context, 1), // No need for serialization methods, taken from the the trait impl another_public_var: PublicState::new(context, 3), a_singleton: Singleton::new(context, 5), // Map init lambda always takes the same form for known storage structs balances: Map::new(context, 6, |context, slot| { Singleton::new(context, slot) }), a_set: Set::new(context, 7), imm_singleton: ImmutableSingleton::new(context, 8), // A map of maps is just nesting lambdas, we can infer this too profiles: Map::new(context, 9, |context, slot| { Map::new(context, slot, |context, slot| { Singleton::new(context, slot) }) }) } } } ``` ...as long as we use "canonical" storage implementations. This means `AStoragePrimitive<SomethingSerializable>` and `Map<SomethingWithToField, AStoragePrimitive<SomethingSerializable>>`. **TLDR:** define the Storage struct, in 99% of cases the macro takes care of the implementation! Implementing custom storage will look just like it does know, the macro will skip automatic generation if it finds one. --------- Co-authored-by: sirasistant <[email protected]>
Closes: AztecProtocol/aztec-packages#3198, AztecProtocol/aztec-packages#2928 ~~Requires AztecProtocol/aztec-packages#4135, which is blocked by noir-lang/noir#4124 Automatic storage initialization via aztec macro. Full support of public and private state from `dep::aztec::state_vars::*`, including Maps (and nested Maps!) Limited support for custom types (as long as they have a single serializable generic and their constructor is `::new(context, storage_slot`). ~~Pending: better errors, code comments and some cleanup.~~ Hijacking my own [comment](AztecProtocol/aztec-packages#4200 (comment)) for the explanation: The idea behind this is that in 99% of cases, storage initialization (that is, the `impl` for a given `struct Storage...` is redundant, and the only need for its existence was assigning storage slots...which in turn were necessary because we didn't know how to serialize the data structures that were used in a given contract or how much space they used once serialized (relevant for the public state). After AztecProtocol/aztec-packages#4135 is merged, both of those things don't have to be explicitly provided since we're using traits, so the aztec macro can infer the implementation of the Storage struct just by taking hints from the definition. An example: ```rust struct Storage { // MyAwesomeStuff implements Serialize<2>, so we assign it slot 1 (and remember that it will take 2 slots due to its size) public_var: PublicState<MyAwesomeSuff>, // Right after the first one, assign it to slot: current_slot + previous_size = 3 another_public_var: PublicState<MyAwesomeSuff>, // Private and Public state don't share slots since they "live" in different trees, but keeping the slot count simplifies implementation. // Notes also implement Serialize<N>, but they only take up 1 slot anyways because of hashing, assign it slot 5 a_singleton: Singleton<ANote>, // Maps derive slots via hashing, so we can assume they only "take" 1 slot. We assign it slot 6 balances: Map<AztecAddress, Singleton<ANote>>, // Slot 7 a_set: Set<ANote>, // Slot 8 imm_singleton: ImmutableSingleton<ANote>, // Slot 9. profiles: Map<AztecAddress, Map<Singleton<ANote>>>, } ``` We have all the info we need in the AST and HIR to build this automatically: ```rust impl Storage { fn init(context: Context) -> Self { Storage { public_var: PublicState::new(context, 1), // No need for serialization methods, taken from the the trait impl another_public_var: PublicState::new(context, 3), a_singleton: Singleton::new(context, 5), // Map init lambda always takes the same form for known storage structs balances: Map::new(context, 6, |context, slot| { Singleton::new(context, slot) }), a_set: Set::new(context, 7), imm_singleton: ImmutableSingleton::new(context, 8), // A map of maps is just nesting lambdas, we can infer this too profiles: Map::new(context, 9, |context, slot| { Map::new(context, slot, |context, slot| { Singleton::new(context, slot) }) }) } } } ``` ...as long as we use "canonical" storage implementations. This means `AStoragePrimitive<SomethingSerializable>` and `Map<SomethingWithToField, AStoragePrimitive<SomethingSerializable>>`. **TLDR:** define the Storage struct, in 99% of cases the macro takes care of the implementation! Implementing custom storage will look just like it does know, the macro will skip automatic generation if it finds one. --------- Co-authored-by: sirasistant <[email protected]>
Aim
Doing code like this:
Expected Behavior
Should compile!
Bug
To Reproduce
Installation Method
None
Nargo Version
No response
Additional Context
The workaround consists in passing an unused sample array with the length:
The ugly part of this workaround is that it requires to modify the serialize trait, which would affect users ):
Would you like to submit a PR for this Issue?
No
Support Needs
No response
The text was updated successfully, but these errors were encountered: