-
Notifications
You must be signed in to change notification settings - Fork 200
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
fix!: Change serialization of struct field order to match the user defined order #1166
Conversation
Using a fixed serialisation pattern, like the alphabetical order for instance is more reliable and avoids user errors based on field struct ordering. I wonder if we could not find a way to keep a fix ordering? |
Reassigning this to @kevaundray for the remaining errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments from when I was looking through this earlier.
crates/nargo_cli/tests/test_data/poseidonperm_x5_254/Nargo.toml
Outdated
Show resolved
Hide resolved
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
For more context: I added another example with a simple struct which also fails: use dep::std;
struct myStruct {
foo: u32,
bar: Field,
}
fn main(y : pub myStruct) {
constrain y.foo == 5;
constrain y.bar == 7;
} But if we re-order the struct to be in alphabetical order, it will pass like so: struct myStruct {
bar: Field,
foo: u32,
} |
There must be a case I missed regarding the ordering. Perhaps during verification |
Yeah looking to see if we forgot to change something from a BTreeMap |
* master: chore: update flake version to match current release (#1204) feat!: Switch to aztec_backend that uses upstream BB & UltraPlonk (#1114) chore(ssa refactor): Add Context structs and start ssa gen pass (#1196) chore(ssa): Replace JmpIf with BrIf (#1193) chore(noir): Release 0.4.1 (#1164) chore(ssa refactor): Add DenseMap and SparseMap types (#1184) feat: bump noir-source-resolver version (#1182) chore(deps): bump h2 from 0.3.16 to 0.3.18 (#1186) fix(nargo): restore `nargo codegen-verifier` functionality (#1185) chore: simplify setup code in `noir_integration` test (#1180) feat: Add Poseidon-BN254 hash functions (#1176)
You haven't adjusted the ABI encoding code to use this new ordering logic. This means when constructing the initial witness you're reversing the two fields. noir/crates/noirc_abi/src/lib.rs Lines 289 to 293 in 6fe27e7
|
* master: (66 commits) feat(nargo)!: retire print-acir in favour of flag (#1328) chore(ssa): enable cse for assert (#1350) chore(ssa refactor): Add basic instruction simplification (#1329) chore(noir): Release 0.6.0 (#1279) feat: enable to_radix for any field element (#1343) chore(ssa refactor): Simplify inlining pass and fix inlining failure (#1337) chore!: Update to acvm 0.11.0 (#1322) feat: Add ECDSA secp256k1 builtin test (#1294) chore: add support for encoding/decoding inputs from JSON (#1325) feat: Issue an error when attempting to use a `return` expression (#1330) chore(ssa refactor): Fix inlining bug (#1335) fix: to-bits and to-radix for > 128 bits (#1312) chore(parser): Parser error optimisation (#1292) chore(ssa refactor): Implement function inlining (#1293) chore: fix installation link in readme (#1326) chore: fix installation link in readme (#1326) feat(stdlib): Add keccak (#1249) fix: Parsing nested generics (#1319) chore(ssa refactor): Document some SSA-gen functions (#1321) fix: Assigning to tuple fields (#1318) ...
Should be good to go now. |
Just noticed the panic in CI. Looking into it now |
This may take some larger refactoring. It's pretty gross. |
Could you open up an issue re the hack and the larger refactoring needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment re tracking the issues/hacks we have added, so we can fix them in the future
Update struct serialization to match noir-lang/noir#1166
Related issue(s)
Resolves #1110
Description
Summary of changes
When serializing contracts, struct fields were previously serialized alphabetically. This change will serialize them in the order defined by users when declaring the struct instead.
Dependency additions / changes
Test additions / changes
Checklist
cargo fmt
with default settings.Documentation needs
Additional context