refactor!: move v1beta2 gov types into types dir by cmwaters · Pull Request #10763 · cosmos/cosmos-sdk

cmwaters · 2021-12-13T13:48:31Z

To be merged after #10748

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)

…o callum/gov-v1beta2

cmwaters · 2021-12-15T16:33:16Z

proto/cosmos/gov/v1beta2/gov.proto

 message TallyParams {
  //  Minimum percentage of total stake needed to vote for a result to be
  //  considered valid.
-  bytes quorum = 1;
+  string quorum = 1 [(cosmos_proto.scalar) = "cosmos.Dec"];

  //  Minimum proportion of Yes votes for proposal to pass. Default value: 0.5.
-  bytes threshold = 2;
+  string threshold = 2 [(cosmos_proto.scalar) = "cosmos.Dec"];

  //  Minimum value of Veto votes to Total votes ratio for proposal to be
  //  vetoed. Default value: 1/3.
-  bytes veto_threshold = 3;
+  string veto_threshold = 3 [(cosmos_proto.scalar) = "cosmos.Dec"];
 }


To reviewers: This is one of the major changes made in this PR

This might need a migration, we can add one in another PR

Yup. I plan to write up a migration for proposals so I can couple it with this as well

x/gov/types/msgs.go

amaury1093

LGTM overall, have some questions that don't need to be answered in this PR.

My biggest concern is if it's worth generating coins into []*sdk.Coins. I prefer to keep []sdk.Coins even though that means we still use gogoproto in v1beta2.

proto/cosmos/gov/v1beta2/genesis.proto

amaury1093 · 2021-12-15T16:57:45Z

proto/cosmos/gov/v1beta2/gov.proto

 message TallyParams {
  //  Minimum percentage of total stake needed to vote for a result to be
  //  considered valid.
-  bytes quorum = 1;
+  string quorum = 1 [(cosmos_proto.scalar) = "cosmos.Dec"];

  //  Minimum proportion of Yes votes for proposal to pass. Default value: 0.5.
-  bytes threshold = 2;
+  string threshold = 2 [(cosmos_proto.scalar) = "cosmos.Dec"];

  //  Minimum value of Veto votes to Total votes ratio for proposal to be
  //  vetoed. Default value: 1/3.
-  bytes veto_threshold = 3;
+  string veto_threshold = 3 [(cosmos_proto.scalar) = "cosmos.Dec"];
 }


This might need a migration, we can add one in another PR

x/gov/migrations/v043/json_test.go

x/gov/types/msgs.go

proto/cosmos/gov/v1beta2/gov.proto

x/gov/types/deposit.go

amaury1093

lgtm, thanks!

proto/cosmos/gov/v1beta2/gov.proto

atheeshp

lgtm!

x/gov/types/proposal.go

amaury1093 · 2021-12-16T14:23:21Z

--- FAIL: TestMsgDepositGetSignBytes (0.00s)
    msgs_test.go:31: 
        	Error Trace:	msgs_test.go:31
        	Error:      	Not equal: 
        	            	expected: "{\"type\":\"cosmos-sdk/MsgDeposit\",\"value\":{\"amount\":[{\"amount\":\"1000\",\"denom\":\"stake\"}],\"depositor\":\"cosmos1v9jxgu33kfsgr5\",\"proposal_id\":\"0\"}}"
        	            	actual  : "{\"type\":\"cosmos-sdk/MsgDeposit\",\"value\":{\"amount\":[{\"amount\":\"1000\",\"denom\":\"stake\"}],\"depositor\":\"cosmos1v9jxgu33kfsgr5\"}}"

This error is a bit concerning, as it touches signing (so consensus). I think it's due to some json tags changes (maybe omitempty?).

I believe there's no risk of simply updating the v1beta2 test (as long as the v1beta1 test doesn't change).

cmwaters · 2021-12-16T17:03:05Z

Yeah it seems the failing test is only when proposal id is 0. When set to non zero it works fine. I'm not sure if there are any tags I need to change. I've added a validity check in genesis that proposal id can never be zero to avoid this case

amaury1093

let's actually keep 0! With 1 we wouldn't have caught this bug.

x/gov/types/msgs_test.go

amaury1093 · 2021-12-16T17:27:45Z

Yeah it seems the failing test is only when proposal id is 0. When set to non zero it works fine. I'm not sure if there are any tags I need to change.

I think it's fine like this. I just reverted the test back to use 0 and fixed it, so we're more resilient to future edge cases (if they happen again).

cmwaters added 6 commits December 13, 2021 09:44

move legacy gov to v1beta1

448e30e

fix tests

7584048

Merge branch 'master' into callum/gov-v1beta1

d21a843

Merge branch 'master' into callum/gov-v1beta1

17d6c75

add changelog entry

324de6e

refactor: move v1beta2 gov types into types dir

096d429

cmwaters requested review from aaronc and alexanderbez as code owners December 13, 2021 13:48

github-actions bot added C:CLI C:Simulations C:x/auth C:x/authz C:x/distribution distribution module related C:x/feegrant C:x/gov C:x/params C:x/upgrade labels Dec 13, 2021

Merge branch 'master' into callum/gov-v1beta2

4b1e3a8

github-actions bot removed C:x/upgrade C:x/auth C:x/authz C:x/feegrant C:CLI C:Simulations C:x/distribution distribution module related C:x/params labels Dec 13, 2021

blushi marked this pull request as draft December 14, 2021 16:21

cmwaters added 2 commits December 15, 2021 16:56

fix compiling errors

48eecc7

Merge branch 'callum/gov-v1beta2' of github.com:cosmos/cosmos-sdk int…

ee5b895

…o callum/gov-v1beta2

github-actions bot added the C:CLI label Dec 15, 2021

cmwaters marked this pull request as ready for review December 15, 2021 16:30

cmwaters commented Dec 15, 2021

View reviewed changes

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

amaury1093 reviewed Dec 15, 2021

View reviewed changes

amaury1093 mentioned this pull request Dec 16, 2021

Gov: generate proto types into types/v1beta2 instead of types #10793

Closed

4 tasks

cmwaters added 2 commits December 16, 2021 11:54

restore gogoproto for non nullable sdk Coins

95803d9

Merge branch 'master' into callum/gov-v1beta2

61052dd

cmwaters requested a review from amaury1093 December 16, 2021 10:55

amaury1093 approved these changes Dec 16, 2021

View reviewed changes

amaury1093 requested a review from atheeshp December 16, 2021 11:26

amaury1093 reviewed Dec 16, 2021

View reviewed changes

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

amaury1093 added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: State Machine Breaking State machine breaking changes (impacts consensus). labels Dec 16, 2021

amaury1093 changed the title ~~refactor: move v1beta2 gov types into types dir~~ refactor!: move v1beta2 gov types into types dir Dec 16, 2021

update changelog

3ba024d

atheeshp approved these changes Dec 16, 2021

View reviewed changes

x/gov/types/proposal.go Outdated Show resolved Hide resolved

cmwaters added 2 commits December 16, 2021 17:58

fix test

ad8e6ef

remove empty const

38c6c52

Merge branch 'master' into callum/gov-v1beta2

bb153d6

cmwaters added the automerge label Dec 16, 2021

amaury1093 reviewed Dec 16, 2021

View reviewed changes

x/gov/types/msgs_test.go Outdated Show resolved Hide resolved

x/gov/types/msgs_test.go Outdated Show resolved Hide resolved

amaury1093 removed the automerge label Dec 16, 2021

amaury1093 added 2 commits December 16, 2021 18:27

Update x/gov/types/msgs_test.go

b04b9bd

Update x/gov/types/msgs_test.go

47b177e

amaury1093 added the automerge label Dec 16, 2021

mergify bot merged commit 350f916 into master Dec 16, 2021

mergify bot deleted the callum/gov-v1beta2 branch December 16, 2021 18:23

Conversation

cmwaters commented Dec 13, 2021 • edited by amaury1093 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Author Checklist

Reviewers Checklist

Uh oh!

cmwaters Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

amaury1093 Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

cmwaters Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaury1093 Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atheeshp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaury1093 commented Dec 16, 2021

Uh oh!

cmwaters commented Dec 16, 2021

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amaury1093 commented Dec 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cmwaters commented Dec 13, 2021 •

edited by amaury1093

Loading