refactor!: Use v1 for `gov` and `upgrade` proto packages #9492

amaury1093 · 2021-06-10T10:52:21Z

Description

Closes: #9446

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

amaury1093 · 2021-06-10T12:49:27Z

proto/cosmos/gov/v1beta1/genesis.proto

@@ -5,7 +5,7 @@ package cosmos.gov.v1beta1;
 import "gogoproto/gogo.proto";
 import "cosmos/gov/v1beta1/gov.proto";

-option go_package = "github.com/cosmos/cosmos-sdk/x/gov/types";
+option go_package = "github.com/cosmos/cosmos-sdk/x/gov/legacy/v040";


We need gov@v1beta1 still for migrations. I changed their generated path to the legacy folder.

ah, we need it to deserialize Anys?

Yeah, old proposals content have e.g. cosmos.gov.v1beta1.Textproposal

codecov · 2021-06-10T14:08:50Z

Codecov Report

Merging #9492 (6b16239) into master (0027111) will increase coverage by 25.03%.
The diff coverage is 62.83%.

❗ Current head 6b16239 differs from pull request most recent head b2456e3. Consider uploading reports for the commit b2456e3 to get more accurate results

@@             Coverage Diff             @@
##           master    #9492       +/-   ##
===========================================
+ Coverage   35.48%   60.52%   +25.03%     
===========================================
  Files         332      591      +259     
  Lines       32620    37327     +4707     
===========================================
+ Hits        11575    22591    +11016     
+ Misses      19819    12787     -7032     
- Partials     1226     1949      +723

Impacted Files	Coverage Δ
client/keys/types.go	`100.00% <ø> (+100.00%)`	⬆️
client/keys/utils.go	`42.85% <ø> (+2.50%)`	⬆️
client/query.go	`16.98% <ø> (ø)`
client/rpc/block.go	`10.00% <ø> (ø)`
client/rpc/routes.go	`100.00% <ø> (ø)`
client/rpc/status.go	`47.72% <ø> (ø)`
client/rpc/validators.go	`0.00% <ø> (ø)`
client/test_helpers.go	`0.00% <ø> (ø)`
client/tx/factory.go	`28.20% <ø> (ø)`
client/tx/legacy.go	`68.42% <ø> (ø)`
... and 702 more

ryanchristo

I cross-referenced that latest v0.42.x proto files and manually tested an upgrade. Overall it looks good. A couple minor observations. Still trying to understand some of the migration changes.

proto/cosmos/gov/v1beta1/gov.proto

proto/cosmos/upgrade/v1beta1/upgrade.proto

robert-zaremba

utACK. Left 2 comments.

CHANGELOG.md

likhita-809

lgtm

…ov-upgrade-v1

amaury1093 · 2021-06-14T14:54:33Z

One thing to note is that to query txs by event containing gov Msgs, clients need to query for both versions

colin-axner · 2021-06-15T08:42:15Z

proto/cosmos/upgrade/v1/upgrade.proto

+  // UpgradedClientState field has been deprecated. IBC upgrade logic has been
+  // moved to the IBC module in the sub module 02-client.
+  reserved 5;
+  reserved "option";


Is this still needed? Since it is a new version? Any legacy IBC upgrade mechanisms will use v1beta1

Sure, we can do that. We'll need to add a in-place store migration for x/upgrade.

Isn't the plan deleted after (or even before) the migration?

looks like it would happen right after

I don't have strong opinions on removing/leaving the reserved field. Whatever y'all think is best. Just now seems like a time to do so

colin-axner · 2021-06-15T08:50:28Z

One thing to note is that to query txs by event containing gov Msgs, clients need to query for both versions

They also need to register the legacy Msgs onto their client codec right? I assume only v1 will be registered by default. Maybe add a note of this in the changelog?

colin-axner · 2021-06-15T13:42:24Z

x/gov/legacy/v043/json.go

+	for i, oldProp := range oldProposals {
+		newProps[i] = types.Proposal{
+			ProposalId:       oldProp.ProposalId,
+			Content:          oldProp.Content,


I think you need to migrate the content as well:

Error: error validating genesis file /home/bartleby/.gm/network2/config/genesis.json: failed to unmarshal gov genesis state: unable to resolve type URL /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal

This is from testing genesis export/import and migration using commit b2456e3a2a29

I think you just need to check if oldProp.Content.Type == /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal and then unmarshal/copy/marshal. Do the store migrations fix this as well? I ran a x/upgrade without this pr, but querying proposals doesn't work now (since the proposal wasn't migrated ofc):

gaiad q gov proposals --node http://localhost:27000 Error: rpc error: code = Unknown desc = rpc error: code = Internal desc = rpc error: code = Internal desc = no concrete type registered for type URL /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal against interface *types.Content: unknown request

Edit:
Thinking about this some more, what happens if a module outside the SDK changes their proposal proto type. Should the gov module add some sort of callback to allow contents to migrate themselves? I ran into a similar issue on IBC since we heavily use Anys and it seems changing the Any Type is very problematic

Maybe some sort of registry could work. I register my type url and a function which performs a migration and returns a new Any. So in this case I'd register a function to migrate /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal to /cosmos.upgrade.v1.SoftwareUpgradeProposal, then the migration code could just call the registry to perform the migration, if a migration func is registered

Obviously out of scope of this pr, but seems likely the usage of Any in migrations needs some sort of handling/design

Hmm, good point, I'll add logic to migrate Anys too from v1beta1 to v1

amaury1093 · 2021-06-17T15:54:11Z

superseded by #9534, as per #9446 (comment)

Use v1 for gov and update

f49ada2

github-actions bot added C:CLI C:x/gov C:x/upgrade T:Docs Changes and features related to documentation. labels Jun 10, 2021

Merge branch 'master' into am/gov-upgrade-v1

405ea76

amaury1093 changed the title ~~refactor: Use v1 for gov and update proto packages~~ refactor: Use v1 for gov and upgrade proto packages Jun 10, 2021

Add changelog

9d81f9f

amaury1093 changed the title ~~refactor: Use v1 for gov and upgrade proto packages~~ refactor!: Use v1 for gov and upgrade proto packages Jun 10, 2021

amaury1093 added 2 commits June 10, 2021 13:29

Fix tests

5387f22

Fix migration

0fa1d4a

github-actions bot added C:Simulations C:x/authz C:x/feegrant labels Jun 10, 2021

amaury1093 commented Jun 10, 2021

View reviewed changes

amaury1093 added 2 commits June 10, 2021 15:29

commit random file

a38aca5

Fix migration

bea146c

github-actions bot added the C:x/genutil genutil module issues label Jun 10, 2021

amaury1093 marked this pull request as ready for review June 10, 2021 14:23

amaury1093 requested review from aaronc and alexanderbez as code owners June 10, 2021 14:23

amaury1093 requested review from robert-zaremba, clevinson and ryanchristo June 10, 2021 14:24

blushi assigned blushi and likhita-809 Jun 10, 2021

ryanchristo reviewed Jun 10, 2021

View reviewed changes

proto/cosmos/gov/v1beta1/gov.proto Show resolved Hide resolved

proto/cosmos/upgrade/v1beta1/upgrade.proto Show resolved Hide resolved

robert-zaremba approved these changes Jun 11, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

likhita-809 approved these changes Jun 11, 2021

View reviewed changes

amaury1093 added 4 commits June 11, 2021 16:12

Create proto breaking changes section

396a408

Not proto breaking

c2fef96

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am/g…

6b16239

…ov-upgrade-v1

Merge branch 'master' into am/gov-upgrade-v1

b2456e3

colin-axner reviewed Jun 15, 2021

View reviewed changes

amaury1093 marked this pull request as draft June 16, 2021 14:59

amaury1093 closed this Jun 17, 2021

amaury1093 deleted the am/gov-upgrade-v1 branch July 27, 2021 10:08

blushi mentioned this pull request Jul 30, 2021

feat: add array of messages to a proposal #9810

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Use v1 for `gov` and `upgrade` proto packages #9492

refactor!: Use v1 for `gov` and `upgrade` proto packages #9492

amaury1093 commented Jun 10, 2021 •

edited

Loading

amaury1093 Jun 10, 2021

robert-zaremba Jun 11, 2021

amaury1093 Jun 11, 2021

codecov bot commented Jun 10, 2021 •

edited

Loading

ryanchristo left a comment

robert-zaremba left a comment

likhita-809 left a comment

amaury1093 commented Jun 14, 2021

colin-axner Jun 15, 2021

amaury1093 Jun 16, 2021

colin-axner Jun 16, 2021

colin-axner Jun 16, 2021

colin-axner commented Jun 15, 2021

colin-axner Jun 15, 2021 •

edited

Loading

colin-axner Jun 16, 2021

amaury1093 Jun 16, 2021

amaury1093 commented Jun 17, 2021

refactor!: Use v1 for gov and upgrade proto packages #9492

refactor!: Use v1 for gov and upgrade proto packages #9492

Conversation

amaury1093 commented Jun 10, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2021 • edited Loading

Codecov Report

ryanchristo left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

amaury1093 commented Jun 14, 2021

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 commented Jun 15, 2021

colin-axner Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Jun 17, 2021

refactor!: Use v1 for `gov` and `upgrade` proto packages #9492

refactor!: Use v1 for `gov` and `upgrade` proto packages #9492

amaury1093 commented Jun 10, 2021 •

edited

Loading

codecov bot commented Jun 10, 2021 •

edited

Loading

colin-axner Jun 15, 2021 •

edited

Loading