feat: migration to gov v1 #4620

colin-axner · 2023-09-11T10:20:33Z

Description

closes: #1282

Commit Message / Changelog Entry

refactor!: migrate to gov v1 via the additions of `MsgRecoverClient` and `MsgIBCSoftwareUpgrade`. The legacy proposal types `ClientUpdateProposal` and `UpgradeClientProposal` have been removed.

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.

* Add proto message for Recover client, implement sdk.Message interface. * Update modules/core/02-client/types/msgs.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Apply suggestions from code review Co-authored-by: Charly <charly@interchain.io> * Remove gogoproto false for cmp, lint, move ibctesting address inline. --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io>

…ce proposals (#4436) * add protos and keeper function placeholder * add keeper functions/tests, msgs and tests

* Add 02-client implementation for Recover client. * Partially address feedback. * Docu RecoverClient, add label, re-use error.

* Add message server handler for recovering a client * Don't assign to deprecated attrs, clean up unused fields. * Further clean-up, remove declaration of unmutated vars.

* Add cmd for submitting a recover client prop. * Bump cosmossdk in e2e. * Use govtypes.ModuleName, rename old govtypes to govv1beta1 * Update modules/core/02-client/client/cli/tx.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Add auth flag. --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>

chore: bump feat/govv1

Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>

* docs: recover client update * Update docs/ibc/proposals.md * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <carlos@interchain.io> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* refactor: remove legacy client update proposal * e2e: swap from ClientUpdateProposal e2e to RecoverClient * refactor: remove unused events

Co-authored-by: colin axner <25233464+colin-axner@users.noreply.github.com>

…ed client (#4594) * add new event type * update event name

modules/core/02-client/types/events.go

damiannolan

Dropping a green check! All in all, pretty clean and easy to review! I only left some nits.
I can ack any follow up commits if needs be but looks good to me! 🚀

docs/ibc/proposals.md

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

modules/apps/transfer/module.go

modules/core/02-client/keeper/client_test.go

modules/core/module.go

proto/ibc/core/client/v1/tx.proto

* wip e2e test * query proposal * update upgrade height in plan * rm unnecessary wait/authority * rm test artifact from merge * add checks for scheduled plan * hook up upgrade query client * plan height * pr fixes * update test * import space * update newchainID value * update clientID upgrade * linter * gci * rm unnecessary event --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Sishir Giri <sis1001@berkeley.edu> Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: sontrinh16 <trinhleson2000@gmail.com> Co-authored-by: catShaark <gundamaster5@gmail.com> Co-authored-by: khanh-notional <50263489+catShaark@users.noreply.github.com>

* chore: update docs for UpgradeProposal -> MsgIBCSoftwareUpgrade * chore: anticipate link change * fix event docs

modules/core/02-client/keeper/events.go

colin-axner · 2023-09-11T13:58:00Z

I noticed the ordering of the msgs is inconsistent, I will update as follows:

CreateClient
UpdateClient
UpgradeClient
SubmitMisbehaviour
RecoverClient
IBCSoftwareUpgrade
UpdateParams

…aviour, recover, ibcsoftwareupgrade, update params

codecov-commenter · 2023-09-11T14:22:49Z

Codecov Report

Merging #4620 (fd6bfd1) into main (e192899) will increase coverage by 0.16%.
Report is 4 commits behind head on main.
The diff coverage is 93.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4620      +/-   ##
==========================================
+ Coverage   79.27%   79.44%   +0.16%     
==========================================
  Files         188      188              
  Lines       13046    13109      +63     
==========================================
+ Hits        10342    10414      +72     
+ Misses       2270     2265       -5     
+ Partials      434      430       -4

Files Changed	Coverage Δ
modules/apps/27-interchain-accounts/module.go	`47.52% <0.00%> (-0.97%)`	⬇️
modules/apps/29-fee/keeper/msg_server.go	`97.14% <ø> (ø)`
modules/apps/transfer/module.go	`46.55% <0.00%> (-1.67%)`	⬇️
modules/core/module.go	`39.02% <0.00%> (-0.98%)`	⬇️
modules/core/02-client/keeper/keeper.go	`81.42% <62.50%> (-1.15%)`	⬇️
modules/core/02-client/types/msgs.go	`78.44% <93.75%> (+3.99%)`	⬆️
...pps/27-interchain-accounts/simulation/proposals.go	`100.00% <100.00%> (ø)`
modules/apps/transfer/simulation/proposals.go	`100.00% <100.00%> (ø)`
modules/core/02-client/keeper/client.go	`94.07% <100.00%> (+2.18%)`	⬆️
modules/core/02-client/keeper/events.go	`88.46% <100.00%> (ø)`
... and 3 more

damiannolan

ACK latest 👍

Thanks @colin-axner! And thank you to @charleenfei and @DimitrisJim for their work on the feat branch! ❤️

charleenfei

thanks for opening this and fixing up conflicts @colin-axner !

docs/architecture/adr-026-ibc-client-recovery-mechanisms.md

docs/ibc/upgrades/quick-guide.md

docs/ibc/events.md

Co-authored-by: Charly <charly@interchain.io>

colin-axner · 2023-09-11T16:02:15Z

self ACK on the changes

# Conflicts: # e2e/tests/core/02-client/client_test.go

crodriguezvega

Great work everyone getting this migration done.

Bye bye, auf Wiedersehen, adios v1beta1 proposals, we will miss you... not! 💔

chatton

LGTM! Most of the comments I left are nits and can be safely ignored, there are just a few questions.

Great work on this everyone ❤️

e2e/tests/core/02-client/client_test.go

e2e/testsuite/grpc_query.go

e2e/tests/core/02-client/client_test.go

e2e/testsuite/grpc_query.go

chatton · 2023-09-12T15:23:30Z

modules/core/02-client/client/cli/tx.go

@@ -242,97 +245,145 @@ func NewUpgradeClientCmd() *cobra.Command {
 	return cmd
 }

-// NewCmdSubmitUpdateClientProposal implements a command handler for submitting an update IBC client proposal transaction.
-func NewCmdSubmitUpdateClientProposal() *cobra.Command {
+// newSubmitRecoverClientProposalCmd defines the command to recover an IBC light client


Suggested change

// newSubmitRecoverClientProposalCmd defines the command to recover an IBC light client

// newSubmitRecoverClientProposalCmd defines the command to recover an IBC light client.

modules/core/02-client/client/cli/tx.go

chatton · 2023-09-12T15:29:33Z

testing/simapp/app.go

-		AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)).
-		AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientProposalHandler(app.IBCKeeper.ClientKeeper))
-
+		AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper))


@damiannolan brought this up in the docs review, is the NewParamChangeProposalHandler still required?

Probably not. maybe we can remove in a followup since it should have been removed with param migration?

can hold off on merging #4628 until then I think

chatton · 2023-09-12T15:30:45Z

modules/core/keeper/msg_server.go

+	}
+
+	if err = k.ClientKeeper.ScheduleIBCSoftwareUpgrade(ctx, msg.Plan, upgradedClientState); err != nil {
+		return nil, errorsmod.Wrapf(err, "cannot schedule IBC client upgrade")


Suggested change

return nil, errorsmod.Wrapf(err, "cannot schedule IBC client upgrade")

return nil, errorsmod.Wrap(err, "cannot schedule IBC client upgrade")

I believe Wrapf expects format args

modules/core/02-client/keeper/client.go

DimitrisJim and others added 20 commits August 29, 2023 12:39

add protos, msgs, keeper handler to upgrade clients using v1 governan…

35909f6

…ce proposals (#4436) * add protos and keeper function placeholder * add keeper functions/tests, msgs and tests

update to 0.47.5 release branch

3b528db

Move signer as last proto field. (#4510)

e30e1bd

msg server function and tests for MsgScheduleIBCClientUpgrade (#4442)

d9f6200

Add 02-client implementation for Recover client. (#4499)

9c5499e

* Add 02-client implementation for Recover client. * Partially address feedback. * Docu RecoverClient, add label, re-use error.

Add implementation for recover client on message server. (#4503)

016e092

* Add message server handler for recovering a client * Don't assign to deprecated attrs, clean up unused fields. * Further clean-up, remove declaration of unmutated vars.

rename command

0bf6708

docs: fixed broken links (#4571)

8a372bd

Merge branch 'main' of github.com:cosmos/ibc-go into feat/govv1

a9325bb

Merge pull request #4577 from cosmos/colin/bump-feat-govv1

3d68136

chore: bump feat/govv1

Add e2e test for recovering a client. (#4543)

611e69a

Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>

feat: add unpacket inerfaces message assertion (#4588)

7f3dd42

add cli for MsgIBCSoftwareUpgrade (#4558)

a374631

docs: use MsgRecoverClient in docs (#4580)

ffcb06d

* docs: recover client update * Update docs/ibc/proposals.md * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <carlos@interchain.io> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

refactor: remove legacy client update proposal (#4581)

40b727a

* refactor: remove legacy client update proposal * e2e: swap from ClientUpdateProposal e2e to RecoverClient * refactor: remove unused events

feat: add proposal simulator interface function and tests (#4466)

c4f947e

Co-authored-by: colin axner <25233464+colin-axner@users.noreply.github.com>

refactor!: remove UpgradeProposal type (#4602)

386ea01

create separate event emission for ibc software upgrades vs an upgrad…

f2e7b0d

…ed client (#4594) * add new event type * update event name

colin-axner commented Sep 11, 2023

View reviewed changes

modules/core/02-client/types/events.go Outdated Show resolved Hide resolved

colin-axner self-assigned this Sep 11, 2023

colin-axner added 2 commits September 11, 2023 12:47

Merge branch 'main' of github.com:cosmos/ibc-go into feat/govv1

e90fddf

fix build

1f6911f

colin-axner mentioned this pull request Sep 11, 2023

Remove legacy governance v1beta1 proposal handlers #3674

Closed

2 tasks

remove: legacy event emissions

14e5593

damiannolan approved these changes Sep 11, 2023

View reviewed changes

colin-axner and others added 3 commits September 11, 2023 13:20

remove: unnecessary assignments, apply suggestion from code review

b2a6b2b

chore: docs for MsgIBCSoftwareUpgrade (#4601)

1bf1b17

* chore: update docs for UpgradeProposal -> MsgIBCSoftwareUpgrade * chore: anticipate link change * fix event docs

colin-axner commented Sep 11, 2023

View reviewed changes

modules/core/02-client/keeper/events.go Outdated Show resolved Hide resolved

colin-axner added 2 commits September 11, 2023 16:01

refactor: ordering, order as follows: create, update, upgrade, misbhe…

2cd4fe7

…aviour, recover, ibcsoftwareupgrade, update params

refactor: simplify ibc software upgrade emitted event

d3a526c

colin-axner mentioned this pull request Sep 11, 2023

Add message and rpc handler to upgrade clients using v1 governance proposals #3673

Closed

5 tasks

lint lint lint

b6ff18b

damiannolan approved these changes Sep 11, 2023

View reviewed changes

charleenfei reviewed Sep 11, 2023

View reviewed changes

docs/architecture/adr-026-ibc-client-recovery-mechanisms.md Outdated Show resolved Hide resolved

docs/ibc/upgrades/quick-guide.md Outdated Show resolved Hide resolved

colin-axner commented Sep 11, 2023

View reviewed changes

docs/ibc/events.md Outdated Show resolved Hide resolved

Apply suggestions from code review

46648c5

Co-authored-by: Charly <charly@interchain.io>

colin-axner added the v8 label Sep 11, 2023

Merge branch 'main' into feat/govv1

fd6bfd1

colin-axner and others added 3 commits September 12, 2023 09:31

Merge branch 'main' into feat/govv1

2e96bb7

review of feat/govv1

33a4ce4

Merge branch 'main' into feat/govv1

808613d

# Conflicts: # e2e/tests/core/02-client/client_test.go

crodriguezvega approved these changes Sep 12, 2023

View reviewed changes

crodriguezvega added 2 commits September 12, 2023 16:01

Merge branch 'main' into feat/govv1

2dc0d36

Merge branch 'main' into feat/govv1

017f21f

chatton approved these changes Sep 12, 2023

View reviewed changes

charleenfei and others added 4 commits September 12, 2023 18:46

Merge branch 'main' into feat/govv1

a9b7f1f

pr nits

cd97b1d

fix tests for error wrapping

c63ed18

Merge branch 'main' into feat/govv1

448d48d

colin-axner merged commit 26f7fe3 into main Sep 13, 2023
55 checks passed

colin-axner deleted the feat/govv1 branch September 13, 2023 11:22

srdtrk mentioned this pull request Sep 13, 2023

fix(callbacks/simapp): removed 'NewClientProposalHandler' #4656

Merged

9 tasks

nullpointer0x00 mentioned this pull request Oct 13, 2023

Gov proposal support for IBC client update provenance-io/provenance#1670

Closed

4 tasks

ttl33 mentioned this pull request Dec 19, 2023

[CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 dydxprotocol/v4-chain#867

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migration to gov v1 #4620

feat: migration to gov v1 #4620

colin-axner commented Sep 11, 2023 •

edited

Loading

damiannolan left a comment

colin-axner commented Sep 11, 2023

codecov-commenter commented Sep 11, 2023 •

edited

Loading

damiannolan left a comment

charleenfei left a comment

colin-axner commented Sep 11, 2023

crodriguezvega left a comment

chatton left a comment

chatton Sep 12, 2023

chatton Sep 12, 2023

colin-axner Sep 13, 2023

chatton Sep 13, 2023

chatton Sep 12, 2023

	// newSubmitRecoverClientProposalCmd defines the command to recover an IBC light client
	// newSubmitRecoverClientProposalCmd defines the command to recover an IBC light client.

	return nil, errorsmod.Wrapf(err, "cannot schedule IBC client upgrade")
	return nil, errorsmod.Wrap(err, "cannot schedule IBC client upgrade")

feat: migration to gov v1 #4620

feat: migration to gov v1 #4620

Conversation

colin-axner commented Sep 11, 2023 • edited Loading

Description

Commit Message / Changelog Entry

damiannolan left a comment

Choose a reason for hiding this comment

colin-axner commented Sep 11, 2023

codecov-commenter commented Sep 11, 2023 • edited Loading

Codecov Report

damiannolan left a comment

Choose a reason for hiding this comment

charleenfei left a comment

Choose a reason for hiding this comment

colin-axner commented Sep 11, 2023

crodriguezvega left a comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

chatton Sep 12, 2023

Choose a reason for hiding this comment

chatton Sep 12, 2023

Choose a reason for hiding this comment

colin-axner Sep 13, 2023

Choose a reason for hiding this comment

chatton Sep 13, 2023

Choose a reason for hiding this comment

chatton Sep 12, 2023

Choose a reason for hiding this comment

colin-axner commented Sep 11, 2023 •

edited

Loading

codecov-commenter commented Sep 11, 2023 •

edited

Loading