fix: check latest attestation nonce before getting it #1638
Conversation
MSevey
requested review from
a team and
staheri14
and removed request for
a team
# Conflicts: # x/qgb/keeper/keeper_valset_test.go
| if !k.CheckLatestAttestationNonce(ctx) { | ||
| return nil, types.ErrLatestAttestationNonceStillNotInitialized | ||
| } |
There was a problem hiding this comment.
If it's just a problem with the querier, then why do all the other functions have this check. Can't we just add the check in the query methods only?
There was a problem hiding this comment.
It's an issue with the querier, yes. But, I figured to add the checks also in other places, even if I think they won't be hit, but still there might be some edge case where the value does not exist and might end up causing a panic in the state machine. Maybe after an upgrade, or a hardfork, etc.
So, I added the checks everywhere for more safety.
There was a problem hiding this comment.
Is it possible to initialise with a random value in the genesis i.e. have a nonce of 0 (which could be invalid but avoids panicking)?
There was a problem hiding this comment.
The genesis actually initializes the value with 0. However, whichever value is set at genesis, it will be incremented. So, there is no guarantees that the nonces will start at 1
There was a problem hiding this comment.
Ok, just to clarify where I'm coming from. It's okay if we want to be defensive in our programming but we should also balance that with making the code as easy to read as possible.
There was a problem hiding this comment.
Makes sense, I guess after we audit this implementation and be sure that no panic would occur, we can get rid of the extra checks. I don't want the QGB module to cause a network halt at some point or be a pain to deal with while it's still not audited :D
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #1638 +/- ##
==========================================
+ Coverage 50.98% 51.79% +0.80%
==========================================
Files 92 95 +3
Lines 5749 5976 +227
==========================================
+ Hits 2931 3095 +164
- Misses 2519 2567 +48
- Partials 299 314 +15
|
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> Closes #1637 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> <!-- Please complete the checklist to ensure that the PR is ready to be reviewed. IMPORTANT: PRs should be left in Draft until the below checklist is completed. --> - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --------- Co-authored-by: Rootul P <rootulp@gmail.com>
Overview
Closes #1637
Checklist