Skip to content

Commit

Permalink
refactor: Bring back deprecated proto fields to v1beta1 (#9534)
Browse files Browse the repository at this point in the history
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: #9446 

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
  • Loading branch information
amaury1093 committed Jun 23, 2021
1 parent 1c77766 commit 29c7a46
Show file tree
Hide file tree
Showing 24 changed files with 376 additions and 1,136 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (client/keys) [\#8500](https://github.com/cosmos/cosmos-sdk/pull/8500) `InfoImporter` interface is removed from legacy keybase.
* (x/staking) [\#8505](https://github.com/cosmos/cosmos-sdk/pull/8505) `sdk.PowerReduction` has been renamed to `sdk.DefaultPowerReduction`, and most staking functions relying on power reduction take a new function argument, instead of relying on that global variable.
* [\#8629](https://github.com/cosmos/cosmos-sdk/pull/8629) Deprecated `SetFullFundraiserPath` from `Config` in favor of `SetPurpose` and `SetCoinType`.
* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove IBC logic from x/upgrade. Deprecates IBC fields in an Upgrade Plan. IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added.
* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove IBC logic from x/upgrade. Deprecates IBC fields in an Upgrade Plan, an error will be thrown if they are set. IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added.
* (x/bank) [\#8517](https://github.com/cosmos/cosmos-sdk/pull/8517) `SupplyI` interface and `Supply` are removed and uses `sdk.Coins` for supply tracking
* (x/upgrade) [\#8743](https://github.com/cosmos/cosmos-sdk/pull/8743) `UpgradeHandler` includes a new argument `VersionMap` which helps facilitate in-place migrations.
* (x/auth) [\#8129](https://github.com/cosmos/cosmos-sdk/pull/8828) Updated `SigVerifiableTx.GetPubKeys` method signature to return error.
Expand Down Expand Up @@ -121,7 +121,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

* (x/{bank,distrib,gov,slashing,staking}) [\#8363](https://github.com/cosmos/cosmos-sdk/issues/8363) Store keys have been modified to allow for variable-length addresses.
* (x/evidence) [\#8502](https://github.com/cosmos/cosmos-sdk/pull/8502) `HandleEquivocationEvidence` persists the evidence to state.
* (x/gov) [\#7733](https://github.com/cosmos/cosmos-sdk/pull/7733) ADR 037 Implementation: Governance Split Votes
* (x/gov) [\#7733](https://github.com/cosmos/cosmos-sdk/pull/7733) ADR 037 Implementation: Governance Split Votes, use `MsgWeightedVote` to send a split vote. Sending a regular `MsgVote` will convert the underlying vote option into a weighted vote with weight 1.
* (x/bank) [\#8656](https://github.com/cosmos/cosmos-sdk/pull/8656) balance and supply are now correctly tracked via `coin_spent`, `coin_received`, `coinbase` and `burn` events.
* (x/bank) [\#8517](https://github.com/cosmos/cosmos-sdk/pull/8517) Supply is now stored and tracked as `sdk.Coins`
* (store) [\#8790](https://github.com/cosmos/cosmos-sdk/pull/8790) Reduce gas costs by 10x for transient store operations.
Expand Down
2 changes: 0 additions & 2 deletions buf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ lint:
breaking:
use:
- FILE
except:
- FIELD_NO_DELETE
ignore:
- tendermint
- gogoproto
Expand Down
3 changes: 3 additions & 0 deletions docs/core/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -4987,6 +4987,7 @@ A Vote consists of a proposal ID, the voter, and the vote option.
| ----- | ---- | ----- | ----------- |
| `proposal_id` | [uint64](#uint64) | | |
| `voter` | [string](#string) | | |
| `option` | [VoteOption](#cosmos.gov.v1beta1.VoteOption) | | **Deprecated.** Deprecated: Prefer to use `options` instead. This field is set in queries if an only if `len(options) == 1` and that option has weight 1. In all other cases, this field will default to OptionEmpty. |
| `options` | [WeightedVoteOption](#cosmos.gov.v1beta1.WeightedVoteOption) | repeated | |


Expand Down Expand Up @@ -7873,8 +7874,10 @@ Plan specifies information about a planned upgrade and when it should occur.
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `name` | [string](#string) | | Sets the name for the upgrade. This name will be used by the upgraded version of the software to apply any special "on-upgrade" commands during the first BeginBlock method after the upgrade is applied. It is also used to detect whether a software version can handle a given upgrade. If no upgrade handler with this name has been set in the software, it will be assumed that the software is out-of-date when the upgrade Time or Height is reached and the software will exit. |
| `time` | [google.protobuf.Timestamp](#google.protobuf.Timestamp) | | **Deprecated.** Deprecated: Time based upgrades have been deprecated. Time based upgrade logic has been removed from the SDK. If this field is not empty, an error will be thrown. |
| `height` | [int64](#int64) | | The height at which the upgrade must be performed. Only used if Time is not set. |
| `info` | [string](#string) | | Any application specific upgrade info to be included on-chain such as a git commit that validators could automatically upgrade to |
| `upgraded_client_state` | [google.protobuf.Any](#google.protobuf.Any) | | **Deprecated.** Deprecated: UpgradedClientState field has been deprecated. IBC upgrade logic has been moved to the IBC module in the sub module 02-client. If this field is not empty, an error will be thrown. |



Expand Down
6 changes: 4 additions & 2 deletions proto/cosmos/gov/v1beta1/gov.proto
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,10 @@ message Vote {

uint64 proposal_id = 1 [(gogoproto.moretags) = "yaml:\"proposal_id\""];
string voter = 2;
reserved 3;
reserved "option";
// Deprecated: Prefer to use `options` instead. This field is set in queries
// if and only if `len(options) == 1` and that option has weight 1. In all
// other cases, this field will default to OptionEmpty.
VoteOption option = 3 [deprecated = true];
repeated WeightedVoteOption options = 4 [(gogoproto.nullable) = false];
}

Expand Down
13 changes: 7 additions & 6 deletions proto/cosmos/upgrade/v1beta1/upgrade.proto
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ message Plan {
// reached and the software will exit.
string name = 1;

// Time based upgrades have been deprecated. Time based upgrade logic
// Deprecated: Time based upgrades have been deprecated. Time based upgrade logic
// has been removed from the SDK.
reserved 2;
reserved "time";
// If this field is not empty, an error will be thrown.
google.protobuf.Timestamp time = 2 [deprecated = true, (gogoproto.stdtime) = true, (gogoproto.nullable) = false];

// The height at which the upgrade must be performed.
// Only used if Time is not set.
Expand All @@ -35,10 +35,11 @@ message Plan {
// such as a git commit that validators could automatically upgrade to
string info = 4;

// UpgradedClientState field has been deprecated. IBC upgrade logic has been
// Deprecated: 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";
// If this field is not empty, an error will be thrown.
google.protobuf.Any upgraded_client_state = 5
[deprecated = true, (gogoproto.moretags) = "yaml:\"upgraded_client_state\""];
}

// SoftwareUpgradeProposal is a gov Content type for initiating a software
Expand Down
3 changes: 2 additions & 1 deletion x/genutil/legacy/v043/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ import (
"github.com/cosmos/cosmos-sdk/x/genutil/types"
v040gov "github.com/cosmos/cosmos-sdk/x/gov/legacy/v040"
v043gov "github.com/cosmos/cosmos-sdk/x/gov/legacy/v043"
gov "github.com/cosmos/cosmos-sdk/x/gov/types"
)

// Migrate migrates exported state from v0.40 to a v0.43 genesis state.
func Migrate(appState types.AppMap, clientCtx client.Context) types.AppMap {
// Migrate x/gov.
if appState[v040gov.ModuleName] != nil {
// unmarshal relative source genesis application state
var oldGovState v040gov.GenesisState
var oldGovState gov.GenesisState
clientCtx.Codec.MustUnmarshalJSON(appState[v040gov.ModuleName], &oldGovState)

// delete deprecated x/gov genesis state
Expand Down
6 changes: 3 additions & 3 deletions x/gov/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryVote() {
Voter: addrs[0].String(),
}

expRes = &types.QueryVoteResponse{Vote: types.NewVote(proposal.ProposalId, addrs[0], types.NewNonSplitVoteOption(types.OptionAbstain))}
expRes = &types.QueryVoteResponse{Vote: types.Vote{ProposalId: proposal.ProposalId, Voter: addrs[0].String(), Option: types.OptionAbstain, Options: []types.WeightedVoteOption{{Option: types.OptionAbstain, Weight: sdk.MustNewDecFromStr("1.0")}}}}
},
true,
},
Expand Down Expand Up @@ -395,8 +395,8 @@ func (suite *KeeperTestSuite) TestGRPCQueryVotes() {
app.GovKeeper.SetProposal(ctx, proposal)

votes = []types.Vote{
{proposal.ProposalId, addrs[0].String(), types.NewNonSplitVoteOption(types.OptionAbstain)},
{proposal.ProposalId, addrs[1].String(), types.NewNonSplitVoteOption(types.OptionYes)},
{ProposalId: proposal.ProposalId, Voter: addrs[0].String(), Options: types.NewNonSplitVoteOption(types.OptionAbstain)},
{ProposalId: proposal.ProposalId, Voter: addrs[1].String(), Options: types.NewNonSplitVoteOption(types.OptionYes)},
}
accAddr1, err1 := sdk.AccAddressFromBech32(votes[0].Voter)
accAddr2, err2 := sdk.AccAddressFromBech32(votes[1].Voter)
Expand Down
19 changes: 15 additions & 4 deletions x/gov/keeper/querier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,16 +268,16 @@ func TestQueries(t *testing.T) {
// Test query votes on types.Proposal 2
votes := getQueriedVotes(t, ctx, legacyQuerierCdc, querier, proposal2.ProposalId, 1, 0)
require.Len(t, votes, 1)
require.Equal(t, vote1, votes[0])
checkEqualVotes(t, vote1, votes[0])

vote := getQueriedVote(t, ctx, legacyQuerierCdc, querier, proposal2.ProposalId, TestAddrs[0])
require.Equal(t, vote1, vote)
checkEqualVotes(t, vote1, vote)

// Test query votes on types.Proposal 3
votes = getQueriedVotes(t, ctx, legacyQuerierCdc, querier, proposal3.ProposalId, 1, 0)
require.Len(t, votes, 2)
require.Equal(t, vote2, votes[0])
require.Equal(t, vote3, votes[1])
checkEqualVotes(t, vote2, votes[0])
checkEqualVotes(t, vote3, votes[1])

// Test query all proposals
proposals = getQueriedProposals(t, ctx, legacyQuerierCdc, querier, nil, nil, types.StatusNil, 1, 0)
Expand Down Expand Up @@ -384,3 +384,14 @@ func TestPaginatedVotesQuery(t *testing.T) {
})
}
}

// checkEqualVotes checks that two votes are equal, without taking into account
// graceful fallback for `Option`.
// When querying, the keeper populates the `vote.Option` field when there's
// only 1 vote, this function checks equality of structs while skipping that
// field.
func checkEqualVotes(t *testing.T, vote1, vote2 types.Vote) {
require.Equal(t, vote1.Options, vote2.Options)
require.Equal(t, vote1.Voter, vote2.Voter)
require.Equal(t, vote1.ProposalId, vote2.ProposalId)
}
19 changes: 19 additions & 0 deletions x/gov/keeper/vote.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func (keeper Keeper) AddVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.A
// GetAllVotes returns all the votes from the store
func (keeper Keeper) GetAllVotes(ctx sdk.Context) (votes types.Votes) {
keeper.IterateAllVotes(ctx, func(vote types.Vote) bool {
populateLegacyOption(&vote)
votes = append(votes, vote)
return false
})
Expand All @@ -53,6 +54,7 @@ func (keeper Keeper) GetAllVotes(ctx sdk.Context) (votes types.Votes) {
// GetVotes returns all the votes from a proposal
func (keeper Keeper) GetVotes(ctx sdk.Context, proposalID uint64) (votes types.Votes) {
keeper.IterateVotes(ctx, proposalID, func(vote types.Vote) bool {
populateLegacyOption(&vote)
votes = append(votes, vote)
return false
})
Expand All @@ -68,11 +70,18 @@ func (keeper Keeper) GetVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.A
}

keeper.cdc.MustUnmarshal(bz, &vote)
populateLegacyOption(&vote)

return vote, true
}

// SetVote sets a Vote to the gov store
func (keeper Keeper) SetVote(ctx sdk.Context, vote types.Vote) {
// vote.Option is a deprecated field, we don't set it in state
if vote.Option != types.OptionEmpty { //nolint
vote.Option = types.OptionEmpty //nolint
}

store := ctx.KVStore(keeper.storeKey)
bz := keeper.cdc.MustMarshal(&vote)
addr, err := sdk.AccAddressFromBech32(vote.Voter)
Expand All @@ -91,6 +100,7 @@ func (keeper Keeper) IterateAllVotes(ctx sdk.Context, cb func(vote types.Vote) (
for ; iterator.Valid(); iterator.Next() {
var vote types.Vote
keeper.cdc.MustUnmarshal(iterator.Value(), &vote)
populateLegacyOption(&vote)

if cb(vote) {
break
Expand All @@ -107,6 +117,7 @@ func (keeper Keeper) IterateVotes(ctx sdk.Context, proposalID uint64, cb func(vo
for ; iterator.Valid(); iterator.Next() {
var vote types.Vote
keeper.cdc.MustUnmarshal(iterator.Value(), &vote)
populateLegacyOption(&vote)

if cb(vote) {
break
Expand All @@ -119,3 +130,11 @@ func (keeper Keeper) deleteVote(ctx sdk.Context, proposalID uint64, voterAddr sd
store := ctx.KVStore(keeper.storeKey)
store.Delete(types.VoteKey(proposalID, voterAddr))
}

// populateLegacyOption adds graceful fallback of deprecated `Option` field, in case
// there's only 1 VoteOption.
func populateLegacyOption(vote *types.Vote) {
if len(vote.Options) == 1 && vote.Options[0].Weight.Equal(sdk.MustNewDecFromStr("1.0")) {
vote.Option = vote.Options[0].Option //nolint
}
}
4 changes: 4 additions & 0 deletions x/gov/keeper/vote_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func TestVotes(t *testing.T) {
require.Equal(t, proposalID, vote.ProposalId)
require.True(t, len(vote.Options) == 1)
require.Equal(t, types.OptionAbstain, vote.Options[0].Option)
require.Equal(t, types.OptionAbstain, vote.Option)

// Test change of vote
require.NoError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[0], types.NewNonSplitVoteOption(types.OptionYes)))
Expand All @@ -49,6 +50,7 @@ func TestVotes(t *testing.T) {
require.Equal(t, proposalID, vote.ProposalId)
require.True(t, len(vote.Options) == 1)
require.Equal(t, types.OptionYes, vote.Options[0].Option)
require.Equal(t, types.OptionYes, vote.Option)

// Test second vote
require.NoError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[1], types.WeightedVoteOptions{
Expand All @@ -70,6 +72,7 @@ func TestVotes(t *testing.T) {
require.True(t, vote.Options[1].Weight.Equal(sdk.NewDecWithPrec(30, 2)))
require.True(t, vote.Options[2].Weight.Equal(sdk.NewDecWithPrec(5, 2)))
require.True(t, vote.Options[3].Weight.Equal(sdk.NewDecWithPrec(5, 2)))
require.Equal(t, types.OptionEmpty, vote.Option)

// Test vote iterator
// NOTE order of deposits is determined by the addresses
Expand All @@ -87,4 +90,5 @@ func TestVotes(t *testing.T) {
require.True(t, votes[1].Options[1].Weight.Equal(sdk.NewDecWithPrec(30, 2)))
require.True(t, votes[1].Options[2].Weight.Equal(sdk.NewDecWithPrec(5, 2)))
require.True(t, votes[1].Options[3].Weight.Equal(sdk.NewDecWithPrec(5, 2)))
require.Equal(t, types.OptionEmpty, vote.Option)
}
Loading

0 comments on commit 29c7a46

Please sign in to comment.