-
Notifications
You must be signed in to change notification settings - Fork 71
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!: separate element decoding error from verification error #16
Conversation
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.
LGTM - not sure if this is a breaking change or not with the switch to Result from Option
@drcapybara You are right! It is a breaking change since it changes error messages when certain non-verification errors occur. |
- add `QueryError::MiscellaneousDecodingError`, `QueryError::InvalidIndexes` and `QueryError::MiscellaneousEvaluationError` to account for decoding errors beyond overflows and invalid strings - if decoding errors happen we will use them instead of VerificationError
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.
LGTM
🎉 This PR is included in version 0.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Rationale for this change
For arithmetic (such as #14) to work it is necessary to separate element decoding error from verification error. There are already
QueryError::Overflow
andQueryError::InvalidString
that handle some decoding errors. However currently in practice if overflows do take place we getVerificationError
. This PR will fix this issue.What changes are included in this PR?
QueryError::MiscellaneousDecodingError
,QueryError::InvalidIndexes
andQueryError::MiscellaneousEvaluationError
to account for decoding errors beyond overflows and invalid stringsVerificationError
crates/proof-of-sql/src/sql/proof/provable_query_result_test.rs
Are these changes tested?
Yes.