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 2 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
44 changes: 33 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,24 @@ 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)
}

if len(depositAmount) > len(params.MinDeposit) {
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
return errors.Wrapf(types.ErrInvalidDepositDenom, "deposited %s, but gov accepts only the following denom(s): %v", depositAmount, denoms)
}

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
}
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 20, 2023

Choose a reason for hiding this comment

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

The new function validateDepositDenom is introduced to validate the deposit denomination against the accepted denominations specified in the governance parameters. This is a good addition as it ensures that only valid denominations are accepted for deposit. However, the error message in line 280 and 285 could be more informative by specifying which denomination(s) are invalid.

- return errors.Wrapf(types.ErrInvalidDepositDenom, "deposited %s, but gov accepts only the following denom(s): %v", depositAmount, denoms)
+ return errors.Wrapf(types.ErrInvalidDepositDenom, "deposited %s, but gov accepts only the following denom(s): %v. Invalid denom(s): %v", depositAmount, denoms, invalidDenoms)

[USER_EXPERIENCE]

Copy link
Member Author

Choose a reason for hiding this comment

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

won't do, as it requires looping through the whole depositAmount, which could be very big.

Copy link
Contributor

Choose a reason for hiding this comment

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

@julienrbrt I understand your concern about the potential performance impact of looping through a large depositAmount. However, it's important to note that the loop is already present in the current implementation to validate each coin in the depositAmount against the acceptedDenoms.

The suggestion was to enhance the error message by including the invalid denominations, which would require storing these invalid denominations during the existing loop. The additional cost would be the storage of these invalid denominations, which is relatively small compared to the cost of the loop itself.

That being said, if the size of depositAmount is expected to be significantly large, and performance is a critical concern, it might be better to stick with the current implementation. The trade-off here is between a more informative error message and the potential performance impact.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow, you can respond. Amazing.
Yes, performance is a concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

@julienrbrt I understand. Performance is indeed a critical aspect to consider, especially in systems handling large amounts of data. In this case, maintaining the current implementation without the additional storage and processing for invalid denominations is a reasonable decision. It's always important to balance the trade-offs between user experience and system performance. Thank you for your clarification.

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("error getting gov params: %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 100stake,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 10000000stake,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