feat(txe): auto-generate oracle serialization roundtrip tests - #24138
Conversation
A separate solution to reduce macro declarations, we could probably simplify the test macro into a single module macro: Should be able to call into the same macros written here just for oracle functions within a module. |
| storage_write_opcode(storage_slot, value); | ||
| } | ||
|
|
||
| #[auto_serialization_test] |
There was a problem hiding this comment.
we should be able to have a single macro on the module. I see we missed some tests not being marked here like aztec_avm_transactionFee
There was a problem hiding this comment.
Actually, that is intended in this case. TXE doesn't implement all oracle AVM oracles because they are not all needed. So I can't (and don't need to) test all of them
But I didn't know I could do that, TIL. Will look more into it
There was a problem hiding this comment.
Perhaps we could filter by attribute name as well (e.g., w/ a hard coded excluded txe oracles). We could then still contain all oracle test generation from a single macro.
There was a problem hiding this comment.
this actually is aligned with a discussion I had with @nventuro somewhere else: maybe we should restructure the codebase so that we have feature/oracles instead of oracle/feature.nr and feature/ importing those.
if we had avm/oracles.nr or avm/oracles/mod.nr making auto_serialization_test a mod attribute would fit nicely without further work
There was a problem hiding this comment.
I decided to go with @vezenovm 's suggestion with a list of excluded oracles. Since this is only the first step, there are many oracles we are not implementing yet, simply because we can't. Didn't want to move them around just for that reason, made sense to simply exclude them. Once we are done with all oracles, we can revisit this
| interface Scenario { | ||
| value: unknown; | ||
| name?: string; | ||
| } | ||
|
|
||
| function unnamed(value: unknown): Scenario { | ||
| return { value }; | ||
| } | ||
| function named(value: unknown, name: string): Scenario { | ||
| return { value, name }; | ||
| } |
There was a problem hiding this comment.
maybe making name mandatory and defaulting to default could remove some complexity?
There was a problem hiding this comment.
I don't think so. It would be pretty much the same
mverzilli
left a comment
mverzilli
left a comment
There was a problem hiding this comment.
Super useful! Two main objections, in order of personal importance:
- Comments are excessive, in most cases reading them actively confused me, where reading code would have sufficed. I was battling hard against my brain just filtering them out.
- I second Maxim, would be nice to only have to write the autoserialize attribute once per module. If we want to leave avm oracles out of this without incurring in a huge change, maybe we can start by segregating avm oracles to their own module?
| // TODO: implement once we support more complex types | ||
| quote { emit_public_log_opcode }, | ||
| quote { returndata_copy_opcode }, | ||
| // TODO: requires fix on registry |
There was a problem hiding this comment.
These were uncovered by the tests, so they work! The registry implements a return type different from the actual Noir oracle functions. I will fix that in another PR, didn't want to make the PR bigger
Flakey Tests🤖 says: This CI run detected 4 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
BEGIN_COMMIT_OVERRIDE feat(txe): auto-generate oracle serialization roundtrip tests (AztecProtocol#24138) END_COMMIT_OVERRIDE
4 participants
Motivation
#23537 introduced oracle tests that verify serialization across Noir and TS. They were manual, so we had to write one for every oracle. Worse, when two oracles used the same types we ended up testing the same serialization twice, by hand.
Our change
This PR adds a
#[auto_serialization_test]macro that generates those tests automatically, one per oracle. Instead of writing a test each time, we define which values to test for a given type once, and every oracle using that type is covered. Types with more than one serialization shape are handled too: anOption, for example, is tested in both itssomeandnoneforms.Notes
I first tried folding this into a single macro that would emit both the oracle function and its test, instead of pairing
#[auto_serialization_test]with#[oracle(...)]. It didn't work:#[oracle(...)]is a special built-in Noir attribute rather than a regular comptime macro, so I run into different issues.Next steps
This PR only covers simple types. Follow-ups will add the harder ones until every oracle is tested, at which point CI can start failing when an oracle isn't. That will let us drop the serialization tests we currently add by hand, in both Noir and TS, for each new oracle type.