feat(ica/host)!: migrate ICA host params to be self managed #3520

aleem1314 · 2023-04-25T10:16:48Z

Description

With this PR:

The ica/host submodule now self-manages its parameters.
Unit and integration tests have been added for the ica/host submodule's parameters.
Migration code has been implemented for moving parameters from x/params' subspace to the ica/host submodule's keeper.
Migration unit and integration tests have been added.

closes: #3504

addresses: #2010

Commit Message / Changelog Entry

feat(ica/host)!: migrate ica/host to params to be self managed

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.

codecov-commenter · 2023-04-25T10:24:28Z

Codecov Report

Merging #3520 (2eb440c) into main (ba62612) will decrease coverage by 0.05%.
The diff coverage is 64.42%.

❗ Current head 2eb440c differs from pull request most recent head 8586e2e. Consider uploading reports for the commit 8586e2e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3520      +/-   ##
==========================================
- Coverage   78.80%   78.75%   -0.05%     
==========================================
  Files         182      186       +4     
  Lines       12689    12744      +55     
==========================================
+ Hits         9999    10036      +37     
- Misses       2258     2276      +18     
  Partials      432      432

Impacted Files	Coverage Δ
...es/apps/27-interchain-accounts/host/types/codec.go	`0.00% <0.00%> (ø)`
...les/apps/27-interchain-accounts/host/types/keys.go	`0.00% <ø> (ø)`
...27-interchain-accounts/host/types/params_legacy.go	`0.00% <0.00%> (ø)`
modules/apps/27-interchain-accounts/module.go	`53.68% <50.00%> (-0.87%)`	⬇️
...s/27-interchain-accounts/host/keeper/migrations.go	`78.57% <78.57%> (ø)`
...les/apps/27-interchain-accounts/host/types/msgs.go	`81.81% <81.81%> (ø)`
...les/apps/27-interchain-accounts/host/ibc_module.go	`91.11% <100.00%> (ø)`
...apps/27-interchain-accounts/host/keeper/genesis.go	`90.47% <100.00%> (+1.00%)`	⬆️
.../apps/27-interchain-accounts/host/keeper/keeper.go	`87.39% <100.00%> (+1.81%)`	⬆️
...s/27-interchain-accounts/host/keeper/msg_server.go	`100.00% <100.00%> (ø)`
... and 2 more

modules/apps/27-interchain-accounts/host/migrations/v8/migrations.go

…ons.go

srdtrk · 2023-05-10T09:55:03Z

modules/apps/27-interchain-accounts/host/types/params_legacy.go

+func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
+	return paramtypes.ParamSetPairs{
+		paramtypes.NewParamSetPair(KeyHostEnabled, p.HostEnabled, validateEnabledType),
+		paramtypes.NewParamSetPair(KeyAllowMessages, p.AllowMessages, validateAllowlistLegacy),


Suggested change

paramtypes.NewParamSetPair(KeyAllowMessages, p.AllowMessages, validateAllowlistLegacy),

paramtypes.NewParamSetPair(KeyAllowMessages, &p.AllowMessages, validateAllowlistLegacy),

This code should fail during migration unless you use referencing & here. I know because I did the same mistake before. The reason why we both made this mistake was because the original was missing the references.

You would've ran into this error if you wrote migration tests (they would've failed). You should write migration tests. You can use this as an example.

I'm surprised that the e2e tests are passing though as you should be needing to use the new governance proposal to have them update. Did these tests not exist before?

modules/apps/27-interchain-accounts/host/types/params_legacy.go

srdtrk · 2023-05-10T10:07:48Z

modules/apps/27-interchain-accounts/host/keeper/params.go

Imo, it makes more sense for these functions to be in keeper.go instead.

Remove the IsHostEnabled function all together, imo, it is wrong to return false if there is a problem unmarshalling. This case should be handled in GetParams, it seems like you're re-implementing GetParams, unnecessary code duplication

srdtrk · 2023-05-10T10:10:05Z

proto/ibc/applications/interchain_accounts/host/v1/tx.proto

You should register the new message in types/codec.go (which you need to create)

srdtrk · 2023-05-10T10:18:00Z

testing/simapp/app.go

You should also initialise the key table for migration in line 915. See example here

…d' and 'GetAllowMessages' functions

colin-axner

Thank you @aleem1314! Really cool that you picked up this work for us 🙏 I left various comments and only had time to review half the pr, I will hopefully pick up my review soon

CHANGELOG.md

modules/apps/27-interchain-accounts/exported/exported.go

modules/apps/27-interchain-accounts/host/keeper/keeper.go

modules/apps/27-interchain-accounts/host/types/keys.go

modules/apps/27-interchain-accounts/module.go

… ValidateBasic

crodriguezvega

Great work, @aleem1314! I just left a few nits.

modules/apps/27-interchain-accounts/module.go

crodriguezvega · 2023-05-22T21:29:09Z

modules/apps/27-interchain-accounts/module.go

+
+	hostm := hostkeeper.NewMigrator(am.hostKeeper)
+	if err := cfg.RegisterMigration(types.ModuleName, 2, hostm.MigrateParams); err != nil {
+		panic(fmt.Sprintf("failed to migrate interchainaccounts host params from version 2 to 3: %v", err))


Since we will probably migrate together both controller and host params, maybe we can do something here to bump the version only once. But maybe we can deal with that in a later PR after merging also the migration of the controller params.

Suggested change

panic(fmt.Sprintf("failed to migrate interchainaccounts host params from version 2 to 3: %v", err))

panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 2 to 3: %v", err))

done, decided to increase the version now, I assumed the two would be done together but this is safer

Yep, the migration handler can be configured like this with an anonymous function

if err := cfg.RegisterMigration(types.ModuleName, 2, func(ctx sdk.Context) error { if err := hostMigrator.MigrateParams(ctx); err != nil { return err } if err := controllerMigrator.MigrateParams(ctx); err != nil { return err } }); err != nil { panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 2 to 3: %v", err)) }

this can be done later, lgtm

modules/apps/27-interchain-accounts/host/keeper/msg_server_test.go

modules/apps/27-interchain-accounts/host/keeper/keeper_test.go

modules/apps/27-interchain-accounts/host/keeper/genesis_test.go

damiannolan

Thanks @aleem1314 for your work on this, and @srdtrk for seeing it to completion

colin-axner · 2023-05-23T14:30:02Z

I almost forgot, we need a note in the migration doc for the breaking change in app.go. It should be pretty short with a small diff noting that the authority needs to be added

modules/apps/27-interchain-accounts/host/types/msgs.go

DimitrisJim · 2023-05-23T15:13:52Z

proto/ibc/applications/interchain_accounts/host/v1/tx.proto

+}
+
+// MsgUpdateParams defines the payload for Msg/UpdateParams
+message MsgUpdateParams {


ah, also, since this is a new file, adding the message signer option or else we'll totally miss it after #3627

~~Shouldn't their PRs take care of these when their PR is merging. If I implement it now, it can be confusing. Even if they have to manually resolve the merge conflict. I think it should be fine?~~

Edit: I misunderstood. I think I will make these additions as commits to that PR, not here. I will be mindful of this in future PRs after their PRs are merged.

DimitrisJim · 2023-05-23T15:17:52Z

proto/ibc/applications/interchain_accounts/host/v1/tx.proto

+import "ibc/applications/interchain_accounts/host/v1/host.proto";
+
+// Msg defines the 27-interchain-accounts/host Msg service.
+service Msg {


and the equivalent one for services #3630

~~Shouldn't their PRs take care of these when their PR is merging. If I implement it now, it can be confusing. Even if they have to manually resolve the merge conflict. I think it should be fine?~~

Edit: I misunderstood. I think I will make these additions as commits to that PR, not here

feat: add proto msg

4ac0e7b

aleem1314 added 5 commits April 25, 2023 16:50

feat: add msg-server implementation

3e8354b

wip: add migrations

c17fdf2

fix failing tests

dc97796

cleanup

2140a3e

cleanup

c9cd005

aleem1314 changed the title ~~feat: migrate ICA host params to be self managed~~ feat!: migrate ICA host params to be self managed May 3, 2023

aleem1314 marked this pull request as ready for review May 3, 2023 15:12

aleem1314 requested review from colin-axner, AdityaSripal, damiannolan, crodriguezvega, charleenfei, chatton and DimitrisJim as code owners May 3, 2023 15:12

aleem1314 commented May 3, 2023

View reviewed changes

modules/apps/27-interchain-accounts/host/migrations/v8/migrations.go Outdated Show resolved Hide resolved

Update modules/apps/27-interchain-accounts/host/migrations/v8/migrati…

98f213c

…ons.go

srdtrk requested changes May 10, 2023

View reviewed changes

srdtrk and others added 7 commits May 15, 2023 16:57

fix(transfer): 'p.AllowMessages' missing '&' in 'NewParamSetPair'

9c53d61

fix(transfer): 'p.HostEnabled' missing '&' in 'NewParamSetPair'

b02d03b

refactor(ica/host): reduce code duplication by removing 'IsHostEnable…

6269d16

…d' and 'GetAllowMessages' functions

refactor(ica/host): moved getter and setters to 'keeper.go'

b9f0817

refactor(ica/host): moved getter and setters to 'keeper.go'

f5c9fea

fix(ica/host): handling the SetParam errors now

d9a12a8

imp(ica/host): added failure cases to TestValidateParams

3c97fba

colin-axner reviewed May 16, 2023

View reviewed changes

srdtrk added 3 commits May 16, 2023 14:47

feat(ica/host): added codec for msg

a5f4b75

fix(ica/host): added host's RegisterInterfaces to module.go

09da84f

fix(changelog): removed manual addition

926ffdd

srdtrk added 13 commits May 22, 2023 14:49

style(ica/host): updated nil check to be more consistent

df22e88

style(transfer): renamed errors to errorsmod to avoid shadowing

708d5c8

imp(ica/host/test): made test shorter

72bd14d

imp(ica/host/test): removed unneeded comment

2b0aaae

style(ica/host/test): removed test case field names

d804902

refactor(ica/host, fee/test): refactored Validate out of SetParams

ba74181

style(ica/host): ran gofumpt

c806a23

imp(ica/types): switched back to 'bz == nil'

c0f74e0

style(ica/host/test): looks better

e19914f

merge: remote-tracking branch 'origin' into aleem/3504-host-params

212945a

style(ica/host): changed panic message

27c9029

imp(ica/host): removed redundant Validate from msg server, handled in…

a0884ca

… ValidateBasic

style(ica/host): added code comment

0bc9281

crodriguezvega reviewed May 22, 2023

View reviewed changes

srdtrk added 3 commits May 23, 2023 00:46

fix(ica/host): increase consensus version

ce98d84

style(ica/host/test): moved success case to top

e84d975

style(ica/host/test): improve test styling

32ebf33

damiannolan approved these changes May 23, 2023

View reviewed changes

merge: remote-tracking branch 'origin' into aleem/3504-host-params

db33a85

DimitrisJim reviewed May 23, 2023

View reviewed changes

modules/apps/27-interchain-accounts/host/types/msgs.go Outdated Show resolved Hide resolved

DimitrisJim reviewed May 23, 2023

View reviewed changes

srdtrk added 3 commits May 23, 2023 22:13

docs(migration): added changes in app.go

61fee14

imp(ica/host): improved the error message

2eb440c

merge: remote-tracking branch 'origin' into aleem/3504-host-params

8586e2e

srdtrk merged commit df918d2 into cosmos:main May 23, 2023

This was referenced May 24, 2023

feat(client): migrate client params #3640

Merged

feat(ica/controller)!: migrate ica/controller parameters to be self managed #3590

Merged

crodriguezvega mentioned this pull request May 31, 2023

Add e2e test for ics27 host params #2118

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ica/host)!: migrate ICA host params to be self managed #3520

feat(ica/host)!: migrate ICA host params to be self managed #3520

aleem1314 commented Apr 25, 2023 •

edited by srdtrk

Loading

codecov-commenter commented Apr 25, 2023 •

edited

Loading

srdtrk May 10, 2023

srdtrk May 10, 2023

srdtrk May 10, 2023

srdtrk May 10, 2023

colin-axner left a comment

crodriguezvega left a comment

crodriguezvega May 22, 2023

srdtrk May 22, 2023

damiannolan May 23, 2023

damiannolan May 23, 2023

damiannolan left a comment

colin-axner commented May 23, 2023

DimitrisJim May 23, 2023 •

edited

Loading

srdtrk May 23, 2023 •

edited

Loading

DimitrisJim May 23, 2023

srdtrk May 23, 2023 •

edited

Loading

	paramtypes.NewParamSetPair(KeyAllowMessages, p.AllowMessages, validateAllowlistLegacy),
	paramtypes.NewParamSetPair(KeyAllowMessages, &p.AllowMessages, validateAllowlistLegacy),

	panic(fmt.Sprintf("failed to migrate interchainaccounts host params from version 2 to 3: %v", err))
	panic(fmt.Sprintf("failed to migrate interchainaccounts app from version 2 to 3: %v", err))

feat(ica/host)!: migrate ICA host params to be self managed #3520

feat(ica/host)!: migrate ICA host params to be self managed #3520

Conversation

aleem1314 commented Apr 25, 2023 • edited by srdtrk Loading

Description

Commit Message / Changelog Entry

codecov-commenter commented Apr 25, 2023 • edited Loading

Codecov Report

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

crodriguezvega left a 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

damiannolan left a comment

Choose a reason for hiding this comment

colin-axner commented May 23, 2023

DimitrisJim May 23, 2023 • edited Loading

Choose a reason for hiding this comment

srdtrk May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk May 23, 2023 • edited Loading

Choose a reason for hiding this comment

aleem1314 commented Apr 25, 2023 •

edited by srdtrk

Loading

codecov-commenter commented Apr 25, 2023 •

edited

Loading

DimitrisJim May 23, 2023 •

edited

Loading

srdtrk May 23, 2023 •

edited

Loading

srdtrk May 23, 2023 •

edited

Loading