diff --git a/CHANGELOG.md b/CHANGELOG.md index a532fa34d600..59055fe238ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (x/gov) [#18189](https://github.com/cosmos/cosmos-sdk/pull/18189) Limit the accepted deposit coins for a proposal to the minimum proposal deposit denoms. * (x/gov) [#18025](https://github.com/cosmos/cosmos-sdk/pull/18025) Improve ` q gov proposer` by querying directly a proposal instead of tx events. It is an alias of `q gov proposal` as the proposer is a field of the proposal. * (x/staking/keeper) [#18049](https://github.com/cosmos/cosmos-sdk/pull/18049) return early if Slash encounters zero tokens to burn. * (x/staking/keeper) [#18035](https://github.com/cosmos/cosmos-sdk/pull/18035) Hoisted out of the redelegation loop, the non-changing validator and delegator addresses parsing. diff --git a/tests/integration/bank/app_test.go b/tests/integration/bank/app_test.go index 3a664016d04e..2a69407862e9 100644 --- a/tests/integration/bank/app_test.go +++ b/tests/integration/bank/app_test.go @@ -373,14 +373,14 @@ func TestMsgSetSendEnabled(t *testing.T) { s := createTestSuite(t, genAccs) ctx := s.App.BaseApp.NewContext(false) - require.NoError(t, testutil.FundAccount(ctx, s.BankKeeper, addr1, sdk.NewCoins(sdk.NewInt64Coin("foocoin", 101)))) + require.NoError(t, testutil.FundAccount(ctx, s.BankKeeper, addr1, sdk.NewCoins(sdk.NewInt64Coin("stake", 101)))) addr1Str := addr1.String() govAddr := s.BankKeeper.GetAuthority() goodGovProp, err := govv1.NewMsgSubmitProposal( []sdk.Msg{ types.NewMsgSetSendEnabled(govAddr, nil, nil), }, - sdk.Coins{{Denom: "foocoin", Amount: sdkmath.NewInt(5)}}, + sdk.Coins{{Denom: "stake", Amount: sdkmath.NewInt(5)}}, addr1Str, "set default send enabled to true", "Change send enabled", diff --git a/x/gov/README.md b/x/gov/README.md index e5fc5b8b849e..87b2fc5fad6c 100644 --- a/x/gov/README.md +++ b/x/gov/README.md @@ -518,6 +518,7 @@ All `sdk.Msgs` passed into the `messages` field of a `MsgSubmitProposal` message must be registered in the app's `MsgServiceRouter`. Each of these messages must have one signer, namely the gov module account. And finally, the metadata length must not be larger than the `maxMetadataLen` config passed into the gov keeper. +The `initialDeposit` must be strictly positive and conform to the accepted denom of the `MinDeposit` param. **State modifications:** @@ -529,58 +530,17 @@ must not be larger than the `maxMetadataLen` config passed into the gov keeper. * Push `proposalID` in `ProposalProcessingQueue` * Transfer `InitialDeposit` from the `Proposer` to the governance `ModuleAccount` -A `MsgSubmitProposal` transaction can be handled according to the following -pseudocode. - -```go -// PSEUDOCODE // -// Check if MsgSubmitProposal is valid. If it is, create proposal // - -upon receiving txGovSubmitProposal from sender do - - if !correctlyFormatted(txGovSubmitProposal) - // check if proposal is correctly formatted and the messages have routes to other modules. Includes fee payment. - // check if all messages' unique Signer is the gov acct. - // check if the metadata is not too long. - throw - - initialDeposit = txGovSubmitProposal.InitialDeposit - if (initialDeposit.Atoms <= 0) OR (sender.AtomBalance < initialDeposit.Atoms) - // InitialDeposit is negative or null OR sender has insufficient funds - throw - - if (txGovSubmitProposal.Type != ProposalTypePlainText) OR (txGovSubmitProposal.Type != ProposalTypeSoftwareUpgrade) - - sender.AtomBalance -= initialDeposit.Atoms - - depositParam = load(GlobalParams, 'DepositParam') - - proposalID = generate new proposalID - proposal = NewProposal() - - proposal.Messages = txGovSubmitProposal.Messages - proposal.Metadata = txGovSubmitProposal.Metadata - proposal.TotalDeposit = initialDeposit - proposal.SubmitTime = - proposal.DepositEndTime = .Add(depositParam.MaxDepositPeriod) - proposal.Deposits.append({initialDeposit, sender}) - proposal.Submitter = sender - proposal.YesVotes = 0 - proposal.NoVotes = 0 - proposal.NoWithVetoVotes = 0 - proposal.AbstainVotes = 0 - proposal.CurrentStatus = ProposalStatusOpen - - store(Proposals, , proposal) // Store proposal in Proposals mapping - return proposalID -``` - ### Deposit -Once a proposal is submitted, if -`Proposal.TotalDeposit < ActiveParam.MinDeposit`, Atom holders can send +Once a proposal is submitted, if `Proposal.TotalDeposit < ActiveParam.MinDeposit`, Atom holders can send `MsgDeposit` transactions to increase the proposal's deposit. +A deposit is accepted iff: + +* The proposal exists +* The proposal is not in the voting period +* The deposited coins are conform to the accepted denom from the `MinDeposit` param + ```protobuf reference https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/gov/v1/tx.proto#L134-L147 ``` @@ -594,55 +554,6 @@ https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/gov/v1/tx.pro * Push `proposalID` in `ProposalProcessingQueueEnd` * Transfer `Deposit` from the `proposer` to the governance `ModuleAccount` -A `MsgDeposit` transaction has to go through a number of checks to be valid. -These checks are outlined in the following pseudocode. - -```go -// PSEUDOCODE // -// Check if MsgDeposit is valid. If it is, increase deposit and check if MinDeposit is reached - -upon receiving txGovDeposit from sender do - // check if proposal is correctly formatted. Includes fee payment. - - if !correctlyFormatted(txGovDeposit) - throw - - proposal = load(Proposals, ) // proposal is a const key, proposalID is variable - - if (proposal == nil) - // There is no proposal for this proposalID - throw - - if (txGovDeposit.Deposit.Atoms <= 0) OR (sender.AtomBalance < txGovDeposit.Deposit.Atoms) OR (proposal.CurrentStatus != ProposalStatusOpen) - - // deposit is negative or null - // OR sender has insufficient funds - // OR proposal is not open for deposit anymore - - throw - - depositParam = load(GlobalParams, 'DepositParam') - - if (CurrentBlock >= proposal.SubmitBlock + depositParam.MaxDepositPeriod) - proposal.CurrentStatus = ProposalStatusClosed - - else - // sender can deposit - sender.AtomBalance -= txGovDeposit.Deposit.Atoms - - proposal.Deposits.append({txGovVote.Deposit, sender}) - proposal.TotalDeposit.Plus(txGovDeposit.Deposit) - - if (proposal.TotalDeposit >= depositParam.MinDeposit) - // MinDeposit is reached, vote opens - - proposal.VotingStartBlock = CurrentBlock - proposal.CurrentStatus = ProposalStatusActive - ProposalProcessingQueue.push(txGovDeposit.ProposalID) - - store(Proposals, , proposal) -``` - ### Vote Once `ActiveParam.MinDeposit` is reached, voting period starts. From there, @@ -661,35 +572,6 @@ https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/gov/v1/tx.pro Gas cost for this message has to take into account the future tallying of the vote in EndBlocker. ::: -Next is a pseudocode outline of the way `MsgVote` transactions are handled: - -```go - // PSEUDOCODE // - // Check if MsgVote is valid. If it is, count vote// - - upon receiving txGovVote from sender do - // check if proposal is correctly formatted. Includes fee payment. - - if !correctlyFormatted(txGovDeposit) - throw - - proposal = load(Proposals, ) - - if (proposal == nil) - // There is no proposal for this proposalID - throw - - - if (proposal.CurrentStatus == ProposalStatusActive) - - - // Sender can vote if - // Proposal is active - // Sender has some bonds - - store(Governance, , txGovVote.Vote) // Voters can vote multiple times. Re-voting overrides previous vote. This is ok because tallying is done once at the end. -``` - ## Events The governance module emits the following events: diff --git a/x/gov/keeper/deposit.go b/x/gov/keeper/deposit.go index a0af97427dff..3da0fd5a9563 100644 --- a/x/gov/keeper/deposit.go +++ b/x/gov/keeper/deposit.go @@ -70,6 +70,16 @@ func (keeper Keeper) AddDeposit(ctx context.Context, proposalID uint64, deposito return false, errors.Wrapf(types.ErrInactiveProposal, "%d", proposalID) } + // Check coins to be deposited match the proposal's deposit params + params, err := keeper.Params.Get(ctx) + if err != nil { + return false, err + } + + if err := keeper.validateDepositDenom(ctx, params, depositAmount); err != nil { + return false, err + } + // update the governance module's account coins pool err = keeper.bankKeeper.SendCoinsFromAccountToModule(ctx, depositorAddr, types.ModuleName, depositAmount) if err != nil { @@ -84,13 +94,9 @@ func (keeper Keeper) AddDeposit(ctx context.Context, proposalID uint64, deposito } // Check if deposit has provided sufficient total funds to transition the proposal into the voting period - activatedVotingPeriod := false - params, err := keeper.Params.Get(ctx) - if err != nil { - return false, err - } minDepositAmount := proposal.GetMinDepositFromParams(params) + activatedVotingPeriod := false if proposal.Status == v1.StatusDepositPeriod && sdk.NewCoins(proposal.TotalDeposit...).IsAllGTE(minDepositAmount) { err = keeper.ActivateVotingPeriod(ctx, proposal) if err != nil { @@ -237,12 +243,7 @@ func (keeper Keeper) RefundAndDeleteDeposits(ctx context.Context, proposalID uin // validateInitialDeposit validates if initial deposit is greater than or equal to the minimum // required at the time of proposal submission. This threshold amount is determined by // the deposit parameters. Returns nil on success, error otherwise. -func (keeper Keeper) validateInitialDeposit(ctx context.Context, initialDeposit sdk.Coins, expedited bool) error { - params, err := keeper.Params.Get(ctx) - if err != nil { - return err - } - +func (keeper Keeper) validateInitialDeposit(ctx context.Context, params v1.Params, initialDeposit sdk.Coins, expedited bool) error { minInitialDepositRatio, err := sdkmath.LegacyNewDecFromStr(params.MinInitialDepositRatio) if err != nil { return err @@ -266,3 +267,21 @@ func (keeper Keeper) validateInitialDeposit(ctx context.Context, initialDeposit } return nil } + +// validateDepositDenom validates if the deposit denom is accepted by the governance module. +func (keeper Keeper) validateDepositDenom(ctx context.Context, params v1.Params, depositAmount sdk.Coins) error { + denoms := []string{} + acceptedDenoms := make(map[string]bool, len(params.MinDeposit)) + for _, coin := range params.MinDeposit { + acceptedDenoms[coin.Denom] = true + denoms = append(denoms, coin.Denom) + } + + for _, coin := range depositAmount { + if _, ok := acceptedDenoms[coin.Denom]; !ok { + return errors.Wrapf(types.ErrInvalidDepositDenom, "deposited %s, but gov accepts only the following denom(s): %v", coin, denoms) + } + } + + return nil +} diff --git a/x/gov/keeper/export_test.go b/x/gov/keeper/export_test.go index f56e25c34c0e..f9db25240adb 100644 --- a/x/gov/keeper/export_test.go +++ b/x/gov/keeper/export_test.go @@ -5,5 +5,10 @@ import sdk "github.com/cosmos/cosmos-sdk/types" // ValidateInitialDeposit is a helper function used only in deposit tests which returns the same // functionality of validateInitialDeposit private function. func (k Keeper) ValidateInitialDeposit(ctx sdk.Context, initialDeposit sdk.Coins, expedited bool) error { - return k.validateInitialDeposit(ctx, initialDeposit, expedited) + params, err := k.Params.Get(ctx) + if err != nil { + return err + } + + return k.validateInitialDeposit(ctx, params, initialDeposit, expedited) } diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index bd624b61fc7d..6af92409740c 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -75,7 +75,16 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos ctx := sdk.UnwrapSDKContext(goCtx) initialDeposit := msg.GetInitialDeposit() - if err := k.validateInitialDeposit(ctx, initialDeposit, msg.Expedited); err != nil { + params, err := k.Params.Get(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get governance parameters: %w", err) + } + + if err := k.validateInitialDeposit(ctx, params, initialDeposit, msg.Expedited); err != nil { + return nil, err + } + + if err := k.validateDepositDenom(ctx, params, initialDeposit); err != nil { return nil, err } diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index 1a79dbe59043..1fd9e3b2dd2e 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -207,6 +207,36 @@ func (suite *KeeperTestSuite) TestMsgSubmitProposal() { expErr: true, expErrMsg: "proposal message not recognized by router", }, + "invalid deposited coin": { + preRun: func() (*v1.MsgSubmitProposal, error) { + return v1.NewMsgSubmitProposal( + []sdk.Msg{bankMsg}, + []sdk.Coin{sdk.NewCoin("invalid", sdkmath.NewInt(100))}, + proposer.String(), + "", + "Proposal", + "description of proposal", + false, + ) + }, + expErr: true, + expErrMsg: "deposited 100invalid, but gov accepts only the following denom(s): [stake]: invalid deposit denom", + }, + "invalid deposited coin (multiple)": { + preRun: func() (*v1.MsgSubmitProposal, error) { + return v1.NewMsgSubmitProposal( + []sdk.Msg{bankMsg}, + append(initialDeposit, sdk.NewCoin("invalid", sdkmath.NewInt(100))), + proposer.String(), + "", + "Proposal", + "description of proposal", + false, + ) + }, + expErr: true, + expErrMsg: "deposited 100invalid, but gov accepts only the following denom(s): [stake]: invalid deposit denom", + }, "all good": { preRun: func() (*v1.MsgSubmitProposal, error) { return v1.NewMsgSubmitProposal( @@ -792,6 +822,24 @@ func (suite *KeeperTestSuite) TestMsgDeposit() { expErr: true, expErrMsg: "invalid depositor address", }, + "invalid deposited coin ": { + preRun: func() uint64 { + return pID + }, + depositor: proposer, + deposit: []sdk.Coin{sdk.NewCoin("ibc/badcoin", sdkmath.NewInt(1000))}, + expErr: true, + expErrMsg: "deposited 1000ibc/badcoin, but gov accepts only the following denom(s): [stake]", + }, + "invalid deposited coin (multiple)": { + preRun: func() uint64 { + return pID + }, + depositor: proposer, + deposit: append(minDeposit, sdk.NewCoin("ibc/badcoin", sdkmath.NewInt(1000))), + expErr: true, + expErrMsg: "deposited 1000ibc/badcoin, but gov accepts only the following denom(s): [stake]", + }, "all good": { preRun: func() uint64 { return pID diff --git a/x/gov/types/errors.go b/x/gov/types/errors.go index a5d8675b82f9..4ee8a17257bc 100644 --- a/x/gov/types/errors.go +++ b/x/gov/types/errors.go @@ -24,4 +24,5 @@ var ( ErrVotingPeriodEnded = errors.Register(ModuleName, 20, "voting period already ended") ErrInvalidProposal = errors.Register(ModuleName, 21, "invalid proposal") ErrSummaryTooLong = errors.Register(ModuleName, 22, "summary too long") + ErrInvalidDepositDenom = errors.Register(ModuleName, 23, "invalid deposit denom") )