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

Add consensus params into Simulation #6002

Merged
merged 25 commits into from
Apr 21, 2020
Merged

Conversation

jgimeno
Copy link
Contributor

@jgimeno jgimeno commented Apr 16, 2020

Closes: #5988

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Apr 17, 2020

Codecov Report

Merging #6002 into master will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #6002      +/-   ##
==========================================
- Coverage   54.71%   54.65%   -0.06%     
==========================================
  Files         424      424              
  Lines       25794    25820      +26     
==========================================
  Hits        14113    14113              
- Misses      10710    10736      +26     
  Partials      971      971              

client/cmd_test.go Outdated Show resolved Hide resolved
x/simulation/params.go Outdated Show resolved Hide resolved
x/simulation/params.go Outdated Show resolved Hide resolved
@jgimeno jgimeno self-assigned this Apr 21, 2020
@jgimeno jgimeno marked this pull request as ready for review April 21, 2020 13:36
client/cmd_test.go Outdated Show resolved Hide resolved
client/cmd_test.go Outdated Show resolved Hide resolved
client/cmd_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @jgimeno, I left a few bits of feedback.

x/simulation/params.go Outdated Show resolved Hide resolved
x/simulation/params.go Outdated Show resolved Hide resolved

// GetGenesisStateFromAppState returns x/staking GenesisState given raw application
// genesis state.
func GetGenesisStateFromAppState(cdc *codec.Codec, appState map[string]json.RawMessage) GenesisState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this even called/used? I'd opt to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah in x/simulation/params.go line 182

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so you removed the other, now removed private helper function 👍

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM 🎉


// GetGenesisStateFromAppState returns x/staking GenesisState given raw application
// genesis state.
func GetGenesisStateFromAppState(cdc *codec.Codec, appState map[string]json.RawMessage) GenesisState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so you removed the other, now removed private helper function 👍

@alexanderbez alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 21, 2020
@mergify mergify bot merged commit 6504868 into master Apr 21, 2020
@mergify mergify bot deleted the jonathan/5988-sim-params-consensus branch April 21, 2020 18:28
@fedekunze
Copy link
Collaborator

test-sim-import-export failed

@jgimeno
Copy link
Contributor Author

jgimeno commented Apr 21, 2020

Checking

@fedekunze
Copy link
Collaborator

fedekunze commented Apr 21, 2020

so the problem is that newApp calls mm.InitGenesis which doesn't populate the consensus params:

        // TestAppImportExport L86
	appState, _, cp, err := app.ExportAppStateAndValidators(false, []string{}) // return ConsensusParams
	require.NoError(t, err)
	require.NotNil(t, cp)

	fmt.Printf("importing genesis...\n")

	_, newDB, newDir, _, _, err := SetupSimulation("leveldb-app-sim-2", "Simulation-2")
	require.NoError(t, err, "simulation setup failed")

	defer func() {
		newDB.Close()
		require.NoError(t, os.RemoveAll(newDir))
	}()

	newApp := NewSimApp(log.NewNopLogger(), newDB, nil, true, map[int64]bool{}, DefaultNodeHome, FlagPeriodValue, fauxMerkleModeOpt)
	require.Equal(t, "SimApp", newApp.Name())

	var genesisState GenesisState
	err = app.Codec().UnmarshalJSON(appState, &genesisState)
	require.NoError(t, err)
	genesisState["consensus_params"] = app.Codec().MustMarshalJSON(cp) // add consensus params

	ctxA := app.NewContext(true, abci.Header{Height: app.LastBlockHeight()})
	ctxB := newApp.NewContext(true, abci.Header{Height: app.LastBlockHeight()})
	newApp.mm.InitGenesis(ctxB, app.Codec(), genesisState) // will fail because consensus params are not part of any module

@fedekunze fedekunze mentioned this pull request Apr 21, 2020
11 tasks
@jgimeno
Copy link
Contributor Author

jgimeno commented Apr 21, 2020

Ok thanks! Got it, can we revert this branch meanwhile?

alexanderbez added a commit that referenced this pull request Apr 21, 2020
mergify bot pushed a commit that referenced this pull request Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Simulations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simulation: Use Consensus Params
4 participants