fix: simapp and upgrade configuration for e2e v7 upgrade #3136

damiannolan · 2023-02-10T09:33:35Z

Description

ref: cosmos/cosmos-sdk#14992
closes: #3131

Commit Message / Changelog Entry

NA

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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 and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

damiannolan · 2023-02-13T13:58:16Z

.github/workflows/e2e-upgrade.yaml

@@ -24,7 +24,7 @@ jobs:
            chain-binary: simd
            chain-a-tag: v6.1.0
            chain-b-tag: v6.1.0
-            chain-upgrade-tag: v7.0.0-rc0
+            chain-upgrade-tag: pr-3136


This will essentially need to be v7.0.0-rc1 when this PR is merged, backported and tagged in rc1.

damiannolan · 2023-02-13T14:03:02Z

testing/simapp/app.go

+				consensusparamtypes.StoreKey,
+				crisistypes.StoreKey,


Adds store keys for x/consensus and x/crisis.

The new x/consensus module maintains consensus parameters previously maintained by the base app subspace.
The x/crisis module previously existed but without a store key. With the deprecation of x/params, the crisis module now uses its own dedicated store to maintain params.

Looks like this logic is repeated for every new module. If we need to do it again for a new module, it'd probably make sense to turn into a helper function. Looks great to me as is

Yeah I agree. We could possible adopt the osmosis upgrade structure, but may require some refactoring of app.go.

damiannolan · 2023-02-13T14:09:48Z

testing/simapp/app.go

+	paramsKeeper.Subspace(authtypes.ModuleName).WithKeyTable(authtypes.ParamKeyTable())
+	paramsKeeper.Subspace(banktypes.ModuleName).WithKeyTable(banktypes.ParamKeyTable())
+	paramsKeeper.Subspace(stakingtypes.ModuleName).WithKeyTable(stakingtypes.ParamKeyTable())
+	paramsKeeper.Subspace(minttypes.ModuleName).WithKeyTable(minttypes.ParamKeyTable())
+	paramsKeeper.Subspace(distrtypes.ModuleName).WithKeyTable(distrtypes.ParamKeyTable())
+	paramsKeeper.Subspace(slashingtypes.ModuleName).WithKeyTable(slashingtypes.ParamKeyTable())
 	paramsKeeper.Subspace(govtypes.ModuleName).WithKeyTable(govv1.ParamKeyTable())
-	paramsKeeper.Subspace(crisistypes.ModuleName)
+	paramsKeeper.Subspace(crisistypes.ModuleName).WithKeyTable(crisistypes.ParamKeyTable())


Adding WithKeyTable(<params_key_table>) for each module here in order to register param key-value types in x/param for migration.

This registration was previously done in each NewKeeper function of the corresponding module. E.g.

if !paramSpace.HasKeyTable() { paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable()) }

The param set pair types need to be registered in order to perform the migrations for modules maintaining their own params, otherwise we fail here. Essentially, the key-value attributes are maintained in memory in this map.

Nice find. Will this be removable in the future?

Yeah we can remove this post v7. We will have to do something similar when we migrate params for ibc-go modules.

Yeah we can remove this post v7.

Should we have an issue for this?

Created #3141

damiannolan · 2023-02-13T14:11:54Z

testing/simapp/app.go

@@ -305,7 +309,7 @@ func NewSimApp(
 	app.ParamsKeeper = initParamsKeeper(appCodec, legacyAmino, keys[paramstypes.StoreKey], tkeys[paramstypes.TStoreKey])

 	// set the BaseApp's parameter store
-	app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, keys[upgradetypes.StoreKey], authtypes.NewModuleAddress(govtypes.ModuleName).String())
+	app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, keys[consensusparamtypes.StoreKey], authtypes.NewModuleAddress(govtypes.ModuleName).String())


This was a misconfiguration in #2672

colin-axner

Wow! Excellent work debugging these test failures! Each of these issues seem quite difficult to dig up. 🙏 ❤️

colin-axner · 2023-02-13T15:14:39Z

testing/simapp/app.go

+	paramsKeeper.Subspace(authtypes.ModuleName).WithKeyTable(authtypes.ParamKeyTable())
+	paramsKeeper.Subspace(banktypes.ModuleName).WithKeyTable(banktypes.ParamKeyTable())
+	paramsKeeper.Subspace(stakingtypes.ModuleName).WithKeyTable(stakingtypes.ParamKeyTable())
+	paramsKeeper.Subspace(minttypes.ModuleName).WithKeyTable(minttypes.ParamKeyTable())
+	paramsKeeper.Subspace(distrtypes.ModuleName).WithKeyTable(distrtypes.ParamKeyTable())
+	paramsKeeper.Subspace(slashingtypes.ModuleName).WithKeyTable(slashingtypes.ParamKeyTable())
 	paramsKeeper.Subspace(govtypes.ModuleName).WithKeyTable(govv1.ParamKeyTable())
-	paramsKeeper.Subspace(crisistypes.ModuleName)
+	paramsKeeper.Subspace(crisistypes.ModuleName).WithKeyTable(crisistypes.ParamKeyTable())


Nice find. Will this be removable in the future?

colin-axner · 2023-02-13T15:17:14Z

testing/simapp/app.go

+				consensusparamtypes.StoreKey,
+				crisistypes.StoreKey,


Looks like this logic is repeated for every new module. If we need to do it again for a new module, it'd probably make sense to turn into a helper function. Looks great to me as is

crodriguezvega

Amazing work, @damiannolan. Another rabbit hole you managed to get out of sound and safe! :)

These changes in app.go, do we need them in the app.go of the interchain-account-demo repo?

damiannolan · 2023-02-14T10:26:59Z

These changes in app.go, do we need them in the app.go of the interchain-account-demo repo?

Yeah, I think it would be good to include the changes from #2672 and this PR. The upgrade specific stuff is probably fine to leave out, unless you plan to try using icad for upgrades.

chatton

Nice work with the investigation and fixes for these issues ❤️

* Register required types for upgrade E2E tests * removed temporary function update * registering additional types and specifying rc0 tag in upgrade test * updated workflow tag * bump version to 6.1.0 * adding keytables to params subspaces in app.go * replace with pr docker build for testing * adding more wait for blocks * temporarily add new grpc services * removing last addition * configure store loaders for upgrade * adding consensus params migration from baseapp * testing without baseapp param migration * readd baseapp params migration * commiting updates, autocli and reflection svc * fix in run e2e script * adding crisis storekey to store upgrades * removing additional wait for blocks --------- Co-authored-by: Cian Hatton <[email protected]> (cherry picked from commit 80f162c) # Conflicts: # .github/workflows/e2e-upgrade.yaml # e2e/scripts/run-e2e.sh

…3143) * simapp and upgrade configuration for e2e v7 upgrade (#3136) * Register required types for upgrade E2E tests * removed temporary function update * registering additional types and specifying rc0 tag in upgrade test * updated workflow tag * bump version to 6.1.0 * adding keytables to params subspaces in app.go * replace with pr docker build for testing * adding more wait for blocks * temporarily add new grpc services * removing last addition * configure store loaders for upgrade * adding consensus params migration from baseapp * testing without baseapp param migration * readd baseapp params migration * commiting updates, autocli and reflection svc * fix in run e2e script * adding crisis storekey to store upgrades * removing additional wait for blocks --------- Co-authored-by: Cian Hatton <[email protected]> (cherry picked from commit 80f162c) # Conflicts: # .github/workflows/e2e-upgrade.yaml # e2e/scripts/run-e2e.sh * rm -rf e2e * rm e2e-upgrade.yaml --------- Co-authored-by: Damian Nolan <[email protected]>

* Register required types for upgrade E2E tests * removed temporary function update * registering additional types and specifying rc0 tag in upgrade test * updated workflow tag * bump version to 6.1.0 * adding keytables to params subspaces in app.go * replace with pr docker build for testing * adding more wait for blocks * temporarily add new grpc services * removing last addition * configure store loaders for upgrade * adding consensus params migration from baseapp * testing without baseapp param migration * readd baseapp params migration * commiting updates, autocli and reflection svc * fix in run e2e script * adding crisis storekey to store upgrades * removing additional wait for blocks --------- Co-authored-by: Cian Hatton <[email protected]>

chatton and others added 12 commits February 9, 2023 13:11

Register required types for upgrade E2E tests

8262756

removed temporary function update

2442711

registering additional types and specifying rc0 tag in upgrade test

47d6f74

updated workflow tag

6ed71d8

bump version to 6.1.0

ae4ceb1

adding keytables to params subspaces in app.go

b665d3f

replace with pr docker build for testing

44c7ece

adding more wait for blocks

4d1cf73

temporarily add new grpc services

d8e3e05

removing last addition

889f0c1

configure store loaders for upgrade

e7203d0

adding consensus params migration from baseapp

306a935

damiannolan changed the title ~~fix: adding x/param keytable registration to allow migrations to proceed~~ fix: simapp and upgrade configuration for e2e v7 upgrade Feb 10, 2023

damiannolan added 6 commits February 10, 2023 20:53

testing without baseapp param migration

fe6ad84

readd baseapp params migration

14f19d7

commiting updates, autocli and reflection svc

82594a1

fix in run e2e script

dc9907d

adding crisis storekey to store upgrades

9f0565a

removing additional wait for blocks

641b25e

damiannolan commented Feb 13, 2023

View reviewed changes

damiannolan marked this pull request as ready for review February 13, 2023 14:12

damiannolan requested review from colin-axner, AdityaSripal, crodriguezvega, charleenfei and chatton as code owners February 13, 2023 14:12

colin-axner approved these changes Feb 13, 2023

View reviewed changes

crodriguezvega reviewed Feb 13, 2023

View reviewed changes

chatton approved these changes Feb 14, 2023

View reviewed changes

Base automatically changed from cian/issue#3131-e2e-upgrade-tests-are-failing-after-broadcast-mode-changes to main February 14, 2023 10:32

Merge branch 'main' into damian/add-params-key-tables

287c9dd

damiannolan added the backport-to-v7.0.x label Feb 14, 2023

damiannolan merged commit 80f162c into main Feb 14, 2023

damiannolan deleted the damian/add-params-key-tables branch February 14, 2023 11:26

mergify bot mentioned this pull request Feb 14, 2023

fix: simapp and upgrade configuration for e2e v7 upgrade (backport #3136) #3143

Merged

faddat mentioned this pull request Jun 23, 2023

v7.2.0 for sdk 50 #3948

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: simapp and upgrade configuration for e2e v7 upgrade #3136

fix: simapp and upgrade configuration for e2e v7 upgrade #3136

damiannolan commented Feb 10, 2023 •

edited

Loading

damiannolan Feb 13, 2023

damiannolan Feb 13, 2023

colin-axner Feb 13, 2023

damiannolan Feb 13, 2023

damiannolan Feb 13, 2023

colin-axner Feb 13, 2023

damiannolan Feb 13, 2023

crodriguezvega Feb 13, 2023

damiannolan Feb 14, 2023

damiannolan Feb 13, 2023

colin-axner left a comment

colin-axner Feb 13, 2023

colin-axner Feb 13, 2023

crodriguezvega left a comment

damiannolan commented Feb 14, 2023

chatton left a comment

fix: simapp and upgrade configuration for e2e v7 upgrade #3136

fix: simapp and upgrade configuration for e2e v7 upgrade #3136

Conversation

damiannolan commented Feb 10, 2023 • edited Loading

Description

Commit Message / Changelog Entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

damiannolan commented Feb 14, 2023

chatton left a comment

Choose a reason for hiding this comment

damiannolan commented Feb 10, 2023 •

edited

Loading