-
Notifications
You must be signed in to change notification settings - Fork 261
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
QGB State machine error handling #550
QGB State machine error handling #550
Conversation
Codecov Report
@@ Coverage Diff @@
## qgb-integration #550 +/- ##
==================================================
- Coverage 9.68% 9.63% -0.06%
==================================================
Files 55 55
Lines 9379 9408 +29
==================================================
- Hits 908 906 -2
- Misses 8395 8423 +28
- Partials 76 79 +3
|
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.
nice! I like this a lot. Really good work as per usual
// Too heavy, shouldn't mainly be used. | ||
// Returns empty array if no element is found. |
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.
do you think we might be able to get rid of this in the future? We inherited this from the gravity bridge module's code iirc, and I think we might be able to design around it.
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.
yes, definitely, it has to do with indexing. If we start double indexing data using multiple keys, we can define iterators with more specific store ranges, and it would be way faster
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.
We should investigate however the storage cost if it is worth it compared to the gained performance
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.
opened issue: #552
x/qgb/keeper/keeper_valset.go
Outdated
if !found { | ||
panic(sdkerrors.Wrap( | ||
types.ErrNilAttestation, | ||
fmt.Sprintf("stumbeled upon nil attestation for nonce %d", i), |
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.
fmt.Sprintf("stumbeled upon nil attestation for nonce %d", i), | |
fmt.Sprintf("found nil attestation for nonce %d", i), |
alternatively this
Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
Improves error handling in state machine. It follows the following thinking:
LastAttestationNonce
.(value, found, error)
in other cases to know if element doesn't exist or some error occurred.(value, found, error)
to know if an element doesn't exist or some error occurred.RequestResponse{ element: nil }, nil
if the element doesn't exit and uses will need to do their checks.Closes: #437