Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Bring back deprecated proto fields to v1beta1 #9534

Merged
merged 23 commits into from
Jun 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact, this field is problematic. User simply can't use it. We need to be clear in the changelog about it - because in other circumstance this should require a proto version update in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, I updated the existing changelog entry, do you think it's enough? A mention in the release notes would be useful too.


// 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