ADR 17 Implementation: Historical Module #5380
Conversation
AdityaSripal
requested review from
alessio,
alexanderbez,
fedekunze,
jackzampolin and
rigelrozanski
as code owners
Codecov Report
@@ Coverage Diff @@
## master #5380 +/- ##
=========================================
- Coverage 54.73% 54.6% -0.14%
=========================================
Files 312 315 +3
Lines 18789 18919 +130
=========================================
+ Hits 10285 10330 +45
- Misses 7726 7808 +82
- Partials 778 781 +3
|
There was a problem hiding this comment.
Performed an initial review. Great job @AdityaSripal. In addition to the review, let's be sure to
- Add any necessary type/var/const to
alias.go-- I noticed this file wasn't modified and typically should on such a feature. - Update the spec -- can be a simple section about what historical info is and how its persisted.
| @@ -147,6 +147,9 @@ that allows for arbitrary vesting periods. | |||
| * `ValidateSigCountDecorator`: Validate the number of signatures in tx based on app-parameters. | |||
| * `IncrementSequenceDecorator`: Increments the account sequence for each signer to prevent replay attacks. | |||
| * (cli) [\#5223](https://github.com/cosmos/cosmos-sdk/issues/5223) Cosmos Ledger App v2.0.0 is now supported. The changes are backwards compatible and App v1.5.x is still supported. | |||
| * (x/staking) [\#5380](https://github.com/cosmos/cosmos-sdk/pull/5380) Introduced ability to store historical info entries in staking keeper, allows applications to introspect specified number of past headers and validator sets | |||
There was a problem hiding this comment.
A single changelog entry should be made with bullet points if need be.
x/staking/abci.go
Outdated
| // BeginBlocker will persist the current header and validator set as a historical entry | ||
| // and prune the oldest entry based on the HistoricalEntries parameter | ||
| func BeginBlocker(ctx sdk.Context, k Keeper) { | ||
| entryNum := k.HistoricalEntries(ctx) |
There was a problem hiding this comment.
Best to move this entire block into a method on the keeper.
- This keeps the
BeginBlockerabstraction clean. - Improves testability.
There was a problem hiding this comment.
Open to doing this here, but what i have follows the pattern of BeginBlockers in x/slashing, x/supply, and x/upgrade
There was a problem hiding this comment.
They should be updated as well.
There was a problem hiding this comment.
What about the EndBlocker? Since that isn't a method on the keeper either?
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
…into aditya/history
|
@AdityaSripal mind resolving the conflicts? |
| } | ||
|
|
||
| // ValidateBasic will ensure HistoricalInfo is not nil and sorted | ||
| func ValidateBasic(hi HistoricalInfo) error { |
There was a problem hiding this comment.
this doesn't check if the header is empty
There was a problem hiding this comment.
I noticed too that this function is not used at all in this PR
There was a problem hiding this comment.
Yes, doesn't need to be used in the code since the historicalinfo created is correct-by-construction.
I use the method in the tests to check if NewHistoricalInfo works correctly, and to keep the method available for users of the staking module who might find it useful
There was a problem hiding this comment.
ACK -- great job @AdityaSripal. I think BeginBlocker should have the logic moved to a method. I'll be happy to merge after that.
…into aditya/history
Implements historical module inside
x/stakingAddress #4647 via design in #5340
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)