-
Notifications
You must be signed in to change notification settings - Fork 144
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
Implement error codes for the mtree_verify
instructions
#1328
base: next
Are you sure you want to change the base?
Conversation
cb28df9
to
53fe0b4
Compare
c53b821
to
69eb6f8
Compare
69eb6f8
to
41d8342
Compare
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.
Looks good! Just one small nit, but your call on that one.
assembly/src/parser/grammar.lalrpop
Outdated
@@ -397,7 +397,7 @@ MacroInst: SmallOpsVec = { | |||
#[inline] | |||
Inst: Instruction = { | |||
AdviceInjector, | |||
Assert, | |||
MaybeWithError, |
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.
Nit: I would name this something a bit more precise, perhaps InstWithErrorCode
? Most other instances of Maybe*
are macros which can be applied to a non-terminal, but that isn't the case here - the others are more precise about what is being parsed, so less ambiguous at first glance.
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.
Looks good! Thank you!
One outstanding thing, probably for a different PR, is figuring out how we can treat MerklePathVerificationFailed
error in a similar way as FailedAssertion
. Specifically, for FailedAssertion
, we have a dedicated host handler which can process assertion errors in a custom way (i.e., by mapping them to user-friendly error messages). Should we do something similar for the MerklePathVerificationFailed
?
If we do add a special handler to the Host
for MerklePathVerificationFailed
- should it cover only mtree_verify
instruction, or other Merkle-tree related instructions as well?
As I mentioned, this is probably for a different PR - but I think we should have a good understanding of what we want to do before merging this PR.
We can add a Host handler for Merkle tree errors exactly the same as we do it for By the way, I noticed that we call |
This PR implements the ability to specify error codes for mtree_verify instruction the same way it is done for assert instructions.
TODO:
Related issue: #1264