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): implement a minimum amount per deposit #18146

Merged
merged 25 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from 4 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 @@ -174,6 +174,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

* (x/gov) [#18146](https://github.com/cosmos/cosmos-sdk/pull/18146) Add denom check to reject denoms outside of those listed in `MinDeposit`. A new `MinDepositRatio` param is added (with a default value of `0.001`) and now deposits are required to be at least `MinDepositRatio*MinDeposit` to be accepted.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry correctly summarizes the changes made in the PR. It mentions the addition of the MinDepositRatio parameter and the new requirement for deposits. However, it might be helpful to also mention the new error constant ErrInvalidDepositDenom that was added for handling invalid deposit denominations.

- * (x/gov) [#18146](https://github.com/cosmos/cosmos-sdk/pull/18146) Add denom check to reject denoms outside of those listed in `MinDeposit`. A new `MinDepositRatio` param is added (with a default value of `0.001`) and now deposits are required to be at least `MinDepositRatio*MinDeposit` to be accepted.
+ * (x/gov) [#18146](https://github.com/cosmos/cosmos-sdk/pull/18146) Add denom check to reject denoms outside of those listed in `MinDeposit`. A new `MinDepositRatio` param is added (with a default value of `0.001`) and now deposits are required to be at least `MinDepositRatio*MinDeposit` to be accepted. Added a new error constant `ErrInvalidDepositDenom` for handling invalid deposit denominations.

Commitable suggestion (Beta)
Suggested change
* (x/gov) [#18146](https://github.com/cosmos/cosmos-sdk/pull/18146) Add denom check to reject denoms outside of those listed in `MinDeposit`. A new `MinDepositRatio` param is added (with a default value of `0.001`) and now deposits are required to be at least `MinDepositRatio*MinDeposit` to be accepted.
* (x/gov) [#18146](https://github.com/cosmos/cosmos-sdk/pull/18146) Add denom check to reject denoms outside of those listed in `MinDeposit`. A new `MinDepositRatio` param is added (with a default value of `0.001`) and now deposits are required to be at least `MinDepositRatio*MinDeposit` to be accepted. Added a new error constant `ErrInvalidDepositDenom` for handling invalid deposit denominations.

julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
* (x/distribution) [#17657](https://github.com/cosmos/cosmos-sdk/pull/17657) Migrate community pool funds from x/distribution to x/protocolpool.
* (x/distribution) [#17115](https://github.com/cosmos/cosmos-sdk/pull/17115) Migrate `PreviousProposer` to collections.
* (x/upgrade) [#16244](https://github.com/cosmos/cosmos-sdk/pull/16244) upgrade module no longer stores the app version but gets and sets the app version stored in the `ParamStore` of baseapp.
Expand Down
4 changes: 4 additions & 0 deletions api/cosmos/feegrant/v1beta1/tx.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions api/cosmos/feegrant/v1beta1/tx_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

151 changes: 117 additions & 34 deletions api/cosmos/gov/v1/gov.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions proto/cosmos/gov/v1/gov.proto
Original file line number Diff line number Diff line change
Expand Up @@ -261,4 +261,11 @@ message Params {

// burn deposits if quorum with vote type no_veto is met
bool burn_vote_veto = 15;

// The ratio representing the proportion of the deposit value minimum that must be met when making a deposit.
// Default value: 0.001. Meaning that for a chain with a min_deposit of 1000stake, a deposit of 1stake would be
Copy link
Member

Choose a reason for hiding this comment

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

Default looks quite low, shouldn't we do 1% instead of 0.1%.
This way we iterate at worse over 100 deposits instead of 1000.

// required.
//
// Since: cosmos-sdk 0.50
string min_deposit_ratio = 16 [(cosmos_proto.scalar) = "cosmos.Dec"];
}
12 changes: 10 additions & 2 deletions x/feegrant/tx.pb.go
Copy link
Member

Choose a reason for hiding this comment

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

Fee grant changes looks out of place. Could you once you merge main in this PR, re-run make proto-gen?

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading