Skip to content
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

Dump all state to JSON #901

Merged
merged 22 commits into from
Apr 28, 2018
Merged

Dump all state to JSON #901

merged 22 commits into from
Apr 28, 2018

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Apr 23, 2018

Closes #131

@cwgoes cwgoes requested a review from ebuchman as a code owner April 23, 2018 12:52
@codecov
Copy link

codecov bot commented Apr 23, 2018

Codecov Report

Merging #901 into develop will decrease coverage by 2.18%.
The diff coverage is 30.18%.

@@             Coverage Diff             @@
##           develop     #901      +/-   ##
===========================================
- Coverage    62.61%   60.42%   -2.19%     
===========================================
  Files           65       72       +7     
  Lines         3314     3500     +186     
===========================================
+ Hits          2075     2115      +40     
- Misses        1072     1215     +143     
- Partials       167      170       +3

@cwgoes cwgoes force-pushed the cwgoes/dump-state-to-json branch 2 times, most recently from 00d1b15 to 744a1e9 Compare April 26, 2018 15:53
@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 26, 2018

Swapped to general export command, so usage would be like:

gaiad export > ~/.gaiad/config/genesis.json
# modify some consensus rules, rebuild
gaiad start

@cwgoes cwgoes changed the title WIP: Dump all state to JSON Dump all state to JSON Apr 26, 2018
@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 26, 2018

Ready for review, the code coverage isn't very accurate & this isn't consensus-level anyways.

Thoughts on this approach / alternative suggestions welcome.

@@ -371,6 +371,30 @@ func (k Keeper) GetDelegatorBond(ctx sdk.Context,
return bond, true
}

// load all bonds
func (k Keeper) GetBonds(ctx sdk.Context, maxRetrieve int16) (bonds []DelegatorBond) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be exposed? I don't think we need to use this outside of specialized stake functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, now unexposed.

@@ -49,9 +49,29 @@ func NewEndBlocker(k Keeper) sdk.EndBlocker {
//_____________________________________________________________________

// InitGenesis - store genesis parameters
func InitGenesis(ctx sdk.Context, k Keeper, data GenesisState) {
func (k Keeper) InitGenesis(ctx sdk.Context, data GenesisState) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitGenesis (and WriteGenesis now) should not be functions of the keeper. It's subtle, but a while back Jae and I came the conclusion that handler ought to not be a function of keeper, by extension I'd put genesis functionality in the same category

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

@@ -149,7 +149,15 @@ func (app *GaiaApp) initChainer(ctx sdk.Context, req abci.RequestInitChain) abci
}

// load the initial stake information
stake.InitGenesis(ctx, app.stakeKeeper, genesisState.StakeData)
stake.InitGenesis(app.stakeKeeper, ctx, genesisState.StakeData)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you switch it's standard that the context is always the first function input

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, updated all instances.

if err != nil {
return nil, nil, err
}
bapp := app.NewGaiaApp(log.NewNopLogger(), db)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bapp -> gapp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}
bapp := app.NewGaiaApp(log.NewNopLogger(), db)
if err != nil {
return nil, nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error check is not checking any new errors :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, too much vim-copy-search-replace fu. Fixed.

@@ -39,3 +40,16 @@ func generateApp(rootDir string, logger log.Logger) (abci.Application, error) {
bapp := app.NewGaiaApp(logger, db)
return bapp, nil
}

func exportApp(rootDir string, logger log.Logger) (interface{}, *wire.Codec, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just reviewed all the instances of exportApp in this PR and I think that we should be able to move it's functionality to baseapp - we should probably really limit/group what is actually being passed into server.AddCommands that list is growing fast, I think we just pass in the app which extends baseapp and therefore should have access to exportApp if we build it into their. - We could do this refactors as in this PR or in a separate PR I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I'll do this in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked generateApp and baseApp to share common functionality, let me know what you think.

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good - I think we should refactor to bring exportApp into baseapp as I mentioned in one of my comments

@cwgoes cwgoes changed the title Dump all state to JSON WIP: Dump all state to JSON Apr 26, 2018
@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 27, 2018

Addressed PR comments and added an iterator to export accounts properly. Testsuite presently fails, I think because the account mapper no longer uses the main store.

@cwgoes cwgoes dismissed rigelrozanski’s stale review April 27, 2018 15:16

Addressed, see comment

@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 27, 2018

Testcases fixed, just needed to change some store names.

Curious if there was a reason accounts were using the main store, or if it was just unintentional?

edit: Per conversation, seems to be unintentional & using a separate store is the right approach.

@cwgoes cwgoes changed the title WIP: Dump all state to JSON Dump all state to JSON Apr 27, 2018
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

@rigelrozanski rigelrozanski merged commit 71dccd4 into develop Apr 28, 2018
@rigelrozanski rigelrozanski deleted the cwgoes/dump-state-to-json branch April 28, 2018 00:57
@cwgoes cwgoes mentioned this pull request May 1, 2018
chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
* Add Cosmos Hub Delta upgrade (Gravity DEX) documentation

* Adding FAQ section

* Add FAQ section to TOC

* Stylistic changes to bullets in FAQ

* Add quicksync link for snapshots

* Update docs/migration/cosmoshub-4-delta-upgrade.md

Co-authored-by: billy rennekamp <[email protected]>

* Update docs/migration/cosmoshub-4-delta-upgrade.md

Co-authored-by: billy rennekamp <[email protected]>

* Corrections to panic message and skip upgrade parameter

* Added self-healing infra to faq

* Add DR step between binaries

* Add auto halt requirement and backup cmd

* Add peer check cmd

* Add consensus state cmd

Co-authored-by: billy rennekamp <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants