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

feat(x/gov): limit deposited coins to accepted proposal denom #18189

Merged
merged 8 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<appd> 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.
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/bank/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
40 changes: 29 additions & 11 deletions x/gov/keeper/deposit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -266,3 +267,20 @@ func (keeper Keeper) validateInitialDeposit(ctx context.Context, initialDeposit
}
return nil
}

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 {
Copy link
Member

Choose a reason for hiding this comment

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

can we add a godoc and a mention in the readme about this feature.

If a user sends a valid token and invalid token the tx will be rejected, etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 👍🏾

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
}
7 changes: 6 additions & 1 deletion x/gov/keeper/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved

return k.validateInitialDeposit(ctx, params, initialDeposit, expedited)
}
11 changes: 10 additions & 1 deletion x/gov/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved

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
}

Expand Down
48 changes: 48 additions & 0 deletions x/gov/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions x/gov/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
Loading