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 13 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 @@ -90,7 +90,7 @@ if input key is empty, or input data contains empty key.
* (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 @@ -125,7 +125,7 @@ if input key is empty, or input data contains empty key.

* (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. If this field is set, the VoteOption will be converted into a WeightedVoteOption with weight 1. |
| `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. If this field is set,
// the VoteOption will be converted into a WeightedVoteOption with
// weight 1.
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
5 changes: 4 additions & 1 deletion x/gov/keeper/querier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,10 @@ func TestQueries(t *testing.T) {
require.Equal(t, vote1, votes[0])

vote := getQueriedVote(t, ctx, legacyQuerierCdc, querier, proposal2.ProposalId, TestAddrs[0])
require.Equal(t, vote1, vote)
// vote and vote1 only differ by the fact that vote has its `Option` field populated thanks to graceful fallback in GetVote
require.Equal(t, vote1.Options, vote.Options)
require.Equal(t, vote1.Voter, vote.Voter)
require.Equal(t, vote1.ProposalId, vote.ProposalId)

// Test query votes on types.Proposal 3
votes = getQueriedVotes(t, ctx, legacyQuerierCdc, querier, proposal3.ProposalId, 1, 0)
Expand Down
12 changes: 12 additions & 0 deletions x/gov/keeper/vote.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,23 @@ func (keeper Keeper) GetVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.A
}

keeper.cdc.MustUnmarshal(bz, &vote)

// Graceful fallback of deprecated `Option` field, in case there's only 1
// VoteOption.
if len(vote.Options) == 1 && vote.Options[0].Weight.Equal(sdk.MustNewDecFromStr("1.0")) {
vote.Option = vote.Options[0].Option //nolint staticcheck // Deprecated field
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we remove the entries in Options?

vote.Options = nil

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should we do the same transformation (setting vote.Option in GetVotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, should we do the same transformation (setting vote.Option in GetVotes?

Yes, fixed.

should we remove the entries in Options? vote.Options = nil

I'm not sure. We should for sure populate Vote.Option to keep backwards-compatibility, but I would also populate Vote.Options too, so that clients know what to use instead of the deprecated field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, Vote.Option is marked as deprecated, so Vote.Options should be used in principal.


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 staticcheck // Deprecated field
panic(fmt.Errorf("expected empty vote.Option, got %s", vote.Option)) //nolint staticcheck // Deprecated field
}
Copy link
Contributor Author

@amaury1093 amaury1093 Jun 21, 2021

Choose a reason for hiding this comment

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

@robert-zaremba

In Keeper.AddVote, shall we check that Vote.Option and Vote.Options are exclusive (on can't set both parameters)?

AddVote takes options as argument, I don't think there's anything to change.

I added this piece of code in keeper.SetVote, so that in state, we always only store the weighted option, does that seem okay to you?


store := ctx.KVStore(keeper.storeKey)
bz := keeper.cdc.MustMarshal(&vote)
addr, err := sdk.AccAddressFromBech32(vote.Voter)
Expand Down
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