-
Notifications
You must be signed in to change notification settings - Fork 191
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
public-vm: rethink/add serialization information in Instruction class #4218
Labels
C-avm
Component: AVM related tickets (aka public VM)
Milestone
Comments
Different instructions have different "flags", and certain flags are It might make sense to group all "flags" in the code, or maybe we just treat "tags" and "indirect" separately. But either way, it might be good to consider that we'll need |
fcarreiro
changed the title
Rethink/add serialization information in Instruction class
public-vm: rethink/add serialization information in Instruction class
Jan 31, 2024
fcarreiro
added a commit
that referenced
this issue
Feb 1, 2024
This PR implements serialization for all existing operations. It also adds `indirect` and other missing parameters to the operations (but does not use them). It's a big PR but the most interesting changes are * The `serialization/` folder. * Things that are not tests. What I like * Easy to add types and modify existing wire formats. * Very explicit tests that will tell you exactly what we expect (too repetitive? see [DRY vs DAMP in tests](https://abseil.io/resources/swe-book/html/ch12.html#tests_and_code_sharing_dampcomma_not_dr)). * Well-defined interfaces. * Instruction serialization from object is also supported! This made creating bytecode a breeze. What I don't like * ~~Due to limitations of TypeScript, there is more boilerplate in each operation than I would like. Time permitting I will try to simplify things further.~~ Thanks to @Maddiaa0 for coming up with a boilerplate-less version. * Some typing is not as strict (uses of `any`). What's still missing * External calls cannot be added to the instruction set, because they create a dependency cycle. This stems from the context using `decodeFromBytecode` or, to paraphrase, from an instruction using an interpreter (this was not introduced in this PR). It might be fixable but not in this PR. Closes #4218. --------- Co-authored-by: Maddiaa0 <[email protected]>
michaelelliot
pushed a commit
to Swoir/noir_rs
that referenced
this issue
Feb 28, 2024
…Protocol#4338) This PR implements serialization for all existing operations. It also adds `indirect` and other missing parameters to the operations (but does not use them). It's a big PR but the most interesting changes are * The `serialization/` folder. * Things that are not tests. What I like * Easy to add types and modify existing wire formats. * Very explicit tests that will tell you exactly what we expect (too repetitive? see [DRY vs DAMP in tests](https://abseil.io/resources/swe-book/html/ch12.html#tests_and_code_sharing_dampcomma_not_dr)). * Well-defined interfaces. * Instruction serialization from object is also supported! This made creating bytecode a breeze. What I don't like * ~~Due to limitations of TypeScript, there is more boilerplate in each operation than I would like. Time permitting I will try to simplify things further.~~ Thanks to @Maddiaa0 for coming up with a boilerplate-less version. * Some typing is not as strict (uses of `any`). What's still missing * External calls cannot be added to the instruction set, because they create a dependency cycle. This stems from the context using `decodeFromBytecode` or, to paraphrase, from an instruction using an interpreter (this was not introduced in this PR). It might be fixable but not in this PR. Closes AztecProtocol#4218. --------- Co-authored-by: Maddiaa0 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The AVM instructions (in typescript) currently include information like
static numberOfOperands = 3;
and we also would needhasTag
, etc. This information is currently only needed for (de)serialization. As a short term solution we could just addhasTag
but maybe it's worth thinking if we are not mixing serialization with other concerns.The text was updated successfully, but these errors were encountered: