Adding docs for gov v1 migration #4628

chatton · 2023-09-12T10:29:12Z

Description

closes: #4544

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

Thank you @chatton

damiannolan · 2023-09-12T10:55:40Z

docs/migrations/v7-to-v8.md

+```diff
+	govRouter := govv1beta1.NewRouter()
+	govRouter.AddRoute(govtypes.RouterKey, govv1beta1.ProposalHandler).
+		AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)).


Are we also able to remove this from app.go now? for param change proposals?

charleenfei · 2023-09-12T12:18:45Z

docs/migrations/v7-to-v8.md

@@ -88,6 +88,37 @@ Each module has a corresponding `MsgUpdateParams` message with a `Params` which

 Legacy params subspaces must still be initialised in app.go in order to successfully migrate from x/params to the new self-contained approach. See reference: https://github.com/cosmos/ibc-go/blob/main/testing/simapp/app.go#L1001-L1006

+### Governance V1 migration


should we say something about making sure the correct authority is set in the ibc module keeper on app.go, and that it will default to gov if not?

This reverts commit a0c8b8d.

charleenfei

thank you @chatton!

crodriguezvega

Thank you, @chatton. I left some comments and one question about something that, if needed, can be addressed in a separate PR.

crodriguezvega · 2023-09-13T08:38:40Z

docs/migrations/v7-to-v8.md

@@ -88,6 +88,39 @@ Each module has a corresponding `MsgUpdateParams` message with a `Params` which

 Legacy params subspaces must still be initialised in app.go in order to successfully migrate from x/params to the new self-contained approach. See reference: https://github.com/cosmos/ibc-go/blob/main/testing/simapp/app.go#L1001-L1006

+### Governance V1 migration
+
+Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620).


Suggested change

Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620).

Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620). The proposal `ClientUpdateProposal` has been removed and replaced with `MsgRecoverClient` and the proposal `UpgradeProposal` has been removed and replaced with MsgIBCSoftwareUpgrade`.

crodriguezvega · 2023-09-13T08:46:42Z

docs/migrations/v7-to-v8.md

+
+Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620).
+
+Ensure that the correct authority field is provided to the ibc keeper. The default authority will be `gov` if not specified.


I might be wrong, but I think we don't default to the gov module address in our keepers? We just accept whatever is passed as argument, which now makes me think if we should validate the authority to make sure is a valid SDK address?

yes good call, we don't actually default to the gov, address. (maybe we should 🤔 )

Yeah, also valid solution.

We do explicit checking of GetAuthority() != msg.Signer and msg.Signer should be validated in ValidateBasic of all the messages. So it is already indirectly validated.

However you are correct that we don't default to gov! I can remove that section.

crodriguezvega · 2023-09-13T08:48:55Z

docs/migrations/v7-to-v8.md

+
+Remove legacy proposal registration from app.go ref: [#4602](https://github.com/cosmos/ibc-go/pull/4602).
+
+Remove the ibcclient ProposalHandler from the govRouter.


Suggested change

Remove the ibcclient ProposalHandler from the govRouter.

Remove the 02-client proposal handler from the `govRouter`.

crodriguezvega · 2023-09-13T08:50:01Z

docs/migrations/v7-to-v8.md

+    govtypes.ModuleName: gov.NewAppModuleBasic(
+      []govclient.ProposalHandler{
+      paramsclient.ProposalHandler,
+-     ibcclientclient.UpgradeProposalHandler,


Also the line with ibcclientclient.UpdateClientProposalHandler, should be removed, right?

I believe there was one handler that handled both cases

colin-axner

LGTM, thanks @chatton!

docs: adding docs for gov v1 migration

cd37676

chatton marked this pull request as ready for review September 12, 2023 10:29

chatton requested review from colin-axner, AdityaSripal, crodriguezvega, charleenfei, damiannolan, DimitrisJim and srdtrk as code owners September 12, 2023 10:29

damiannolan approved these changes Sep 12, 2023

View reviewed changes

charleenfei reviewed Sep 12, 2023

View reviewed changes

chatton and others added 4 commits September 12, 2023 12:40

Merge branch 'main' into cian/issue#4544-migration-docs-for-v8-govv1

f897bfc

chore: adding note about authority

a8d1d71

chore: run upgrade tests when upgrade_test.go changes

a0c8b8d

Revert "chore: run upgrade tests when upgrade_test.go changes"

4cfca8b

This reverts commit a0c8b8d.

charleenfei approved these changes Sep 12, 2023

View reviewed changes

chatton and others added 2 commits September 12, 2023 15:03

Merge branch 'main' into cian/issue#4544-migration-docs-for-v8-govv1

1f3de91

formatting of code

43d9ae6

crodriguezvega reviewed Sep 13, 2023

View reviewed changes

chatton mentioned this pull request Sep 13, 2023

feat: migration to gov v1 #4620

Merged

9 tasks

colin-axner added the v8 label Sep 13, 2023

chatton and others added 3 commits September 14, 2023 11:48

chore: addressing PR feedback

3fde33e

Merge branch 'main' into cian/issue#4544-migration-docs-for-v8-govv1

89bdefb

Merge branch 'main' into cian/issue#4544-migration-docs-for-v8-govv1

1408613

colin-axner approved these changes Sep 18, 2023

View reviewed changes

Merge branch 'main' into cian/issue#4544-migration-docs-for-v8-govv1

62d9e58

chatton enabled auto-merge (squash) September 18, 2023 15:23

chatton merged commit 762cebf into main Sep 18, 2023
19 of 20 checks passed

chatton deleted the cian/issue#4544-migration-docs-for-v8-govv1 branch September 18, 2023 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding docs for gov v1 migration #4628

Adding docs for gov v1 migration #4628

chatton commented Sep 12, 2023

damiannolan left a comment

damiannolan Sep 12, 2023

charleenfei Sep 12, 2023

chatton Sep 12, 2023

charleenfei left a comment

crodriguezvega left a comment

crodriguezvega Sep 13, 2023

crodriguezvega Sep 13, 2023

chatton Sep 13, 2023

crodriguezvega Sep 13, 2023

chatton Sep 14, 2023 •

edited

Loading

crodriguezvega Sep 13, 2023

crodriguezvega Sep 13, 2023

colin-axner Sep 18, 2023

colin-axner left a comment

		@@ -88,6 +88,37 @@ Each module has a corresponding `MsgUpdateParams` message with a `Params` which

		Legacy params subspaces must still be initialised in app.go in order to successfully migrate from x/params to the new self-contained approach. See reference: https://github.com/cosmos/ibc-go/blob/main/testing/simapp/app.go#L1001-L1006

		### Governance V1 migration

	Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620).
	Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620). The proposal `ClientUpdateProposal` has been removed and replaced with `MsgRecoverClient` and the proposal `UpgradeProposal` has been removed and replaced with MsgIBCSoftwareUpgrade`.


		Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620).

		Ensure that the correct authority field is provided to the ibc keeper. The default authority will be `gov` if not specified.


		Remove legacy proposal registration from app.go ref: [#4602](https://github.com/cosmos/ibc-go/pull/4602).

		Remove the ibcclient ProposalHandler from the govRouter.

	Remove the ibcclient ProposalHandler from the govRouter.
	Remove the 02-client proposal handler from the `govRouter`.

Adding docs for gov v1 migration #4628

Adding docs for gov v1 migration #4628

Conversation

chatton commented Sep 12, 2023

Description

damiannolan 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

charleenfei 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

chatton Sep 14, 2023 • edited Loading

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

chatton Sep 14, 2023 •

edited

Loading