feat(x/gov): implement a minimum amount per deposit

[facundomedica]

Description

This PR adds a new param to x/gov called MinDepositRatio which dictates the least amount of tokens that a deposit must have to be accepted (given by MinDepositRatio*MinDeposit).

Also only denoms that are present in MinDeposit will be accepted.

Lastly, when sending a proposal the deposit that this includes must also meet the conditions of a normal deposit (meaning there's a minimum amount to deposit along with a proposal submission).

Examples:

MinDepositRatio = 0.001
MinDeposit = 1000stake, 500atom

Meaning that only deposits with at least 1stake or 0.5atom will be accepted.


Valid deposits:
- 1stake, 0atom
- 0stake, 1atom
- 1stake, 0.1atom
- 5stake, 0atom
- ...

Invalid deposits:
- 0stake, 0atom
- 0.5stake, 0.4atom
- 100stake, 1atom, 1osmo // this is invalid because it contains 1 invalid denom
- 0stake, 0atom, 10000osmo

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• run make lint and make test
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

Summary by CodeRabbit

New Features:

• Added a new parameter MinDepositRatio in the gov module to enforce minimum deposit requirements, enhancing the governance system's security.
• Migrated community pool funds from x/distribution to x/protocolpool in the distribution module, improving fund management.
• Migrated PreviousProposer to collections in the distribution module, optimizing data handling.
• Removed the storage of app version in the upgrade module, instead retrieving it from the ParamStore of baseapp, simplifying version management.

Refactor:

• Consolidated deposit validation checks in the gov module, simplifying the code and improving maintainability.

Tests:

• Adjusted deposit amounts in various test cases to align with new deposit requirements.
• Added a new test function TestDepositAmount to validate deposit amount logic, ensuring robustness of the new feature.

Chores:

• Removed unused test function TestQueryDepositsWithoutInitialDeposit from the codebase, improving code cleanliness.

[alexanderbez]

Which issue does this close? Where can I read more on the rationale for this?

[coderabbitai]

Walkthrough

The changes mainly revolve around the cosmos-sdk repository, introducing new validation checks for deposit amounts and ratios in the gov module. The distribution module sees community pool funds migration, and the upgrade module now retrieves the app version from ParamStore. New message types for pruning expired fee allowances are introduced, and various test functions and cases are updated accordingly.

Changes

File(s)	Summary
CHANGELOG.md	Notable changes in different modules, including new validation checks in gov module, funds migration in distribution module, and app version retrieval from ParamStore in upgrade module.
api/cosmos/feegrant/v1beta1/tx.pulsar.go, api/cosmos/gov/v1/gov.pulsar.go, proto/cosmos/gov/v1/gov.proto	Introduction of new message types for pruning expired fee allowances and a new field MinDepositRatio to the Params struct.
tests/e2e/gov/..., tests/integration/bank/app_test.go, x/gov/abci_test.go, x/gov/keeper/deposit_test.go, x/gov/keeper/msg_server_test.go	Various test functions and cases updated to reflect the new validation checks and parameters.
x/gov/keeper/deposit.go, x/gov/keeper/msg_server.go, x/gov/migrations/v5/store.go, x/gov/simulation/genesis.go, x/gov/simulation/operations.go, x/gov/types/errors.go, x/gov/types/v1/params.go	Implementation of new validation checks for deposit amounts and ratios, consolidation of validation logic, and introduction of new error constant and default variable.

🐇🍂
As the leaves fall, so do the old ways,
In the world of code, a new day sways.
With checks and balances, we refine,
In the autumn's glow, our code does shine.
Celebrate the change, embrace the new,
For in this season, we've much to do! 🍁🎉

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.
• @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

[coderabbitai]

Review Status

Actionable comments generated: 1

Commits

Files that changed from the base of the PR and between 93770f5 and cf4e715.

Files ignored due to filter (3)

• api/cosmos/feegrant/v1beta1/tx_grpc.pb.go
• x/feegrant/tx.pb.go
• x/gov/types/v1/gov.pb.go

Files selected for processing (17)

• CHANGELOG.md (1 hunks)
• api/cosmos/feegrant/v1beta1/tx.pulsar.go (2 hunks)
• api/cosmos/gov/v1/gov.pulsar.go (16 hunks)
• proto/cosmos/gov/v1/gov.proto (1 hunks)
• tests/e2e/gov/deposits.go (2 hunks)
• tests/e2e/gov/tx.go (5 hunks)
• tests/integration/bank/app_test.go (1 hunks)
• x/gov/abci_test.go (5 hunks)
• x/gov/keeper/deposit.go (4 hunks)
• x/gov/keeper/deposit_test.go (1 hunks)
• x/gov/keeper/msg_server.go (3 hunks)
• x/gov/keeper/msg_server_test.go (9 hunks)
• x/gov/migrations/v5/store.go (1 hunks)
• x/gov/simulation/genesis.go (3 hunks)
• x/gov/simulation/operations.go (2 hunks)
• x/gov/types/errors.go (1 hunks)
• x/gov/types/v1/params.go (4 hunks)

Files skipped from review due to trivial changes (7)

• tests/e2e/gov/deposits.go
• tests/integration/bank/app_test.go
• x/gov/keeper/deposit_test.go
• x/gov/keeper/msg_server.go
• x/gov/keeper/msg_server_test.go
• x/gov/migrations/v5/store.go
• x/gov/types/errors.go

Additional comments (Suppressed): 44

api/cosmos/feegrant/v1beta1/tx.pulsar.go (2)

• 2725-2730: The new hunk seems to be adding a comment indicating the version of the cosmos-sdk where MsgPruneAllowances was introduced. Ensure that this versioning comment is accurate and necessary for the context of your project. If it is, the change is approved.

• 2764-2769: Similar to the previous comment, this hunk is adding a versioning comment for MsgPruneAllowancesResponse. Again, verify the accuracy and necessity of this comment for your project. If it is, the change is approved.

x/gov/types/v1/params.go (4)

• 33-33: The new DefaultMinDepositRatio constant is introduced with a default value of "0.001". Ensure that this default value is appropriate for all use cases. If not, consider making it configurable.

• 60-65: The NewParams function signature has been updated to include a new parameter minDepositRatio. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

• 82-82: The MinDepositRatio field is added to the Params struct. This change is consistent with the new parameter added to the NewParams function.

• 104-104: The DefaultMinDepositRatio is used in the DefaultParams function. This is consistent with the addition of the MinDepositRatio field to the Params struct.

x/gov/simulation/operations.go (3)

• 573-578: The new code introduces a minimum deposit ratio and calculates a threshold for the deposit amount. Ensure that the GetMinDepositRatio() function is correctly implemented and returns a valid ratio. Also, verify that the ToLegacyDec() and Mul() functions are correctly converting and multiplying the values. The TruncateInt() function should correctly truncate the decimal part of the result.

• 580-582: The MinInitialDepositRatio parameter is used here. Ensure that this parameter is correctly set and that the LegacyNewDecFromStr() function correctly converts it to a decimal.

• 596-598: The new code checks if the deposit amount is less than the calculated threshold. This is a new condition that wasn't present in the old code. Ensure that this condition is correctly implemented and that it doesn't introduce any unexpected behavior.

x/gov/simulation/genesis.go (4)

• 28-31: The new constant MinDepositRatio has been added to the list of simulation parameters. This constant represents the minimum deposit ratio for proposals in the governance module.

• 98-102: The GenMinDepositRatio function has been added to generate a random value for the MinDepositRatio parameter. Currently, it returns a fixed value of 0.001. The commented-out code suggests that the function was intended to return a random value between 0.001 and 0.05, but this has been temporarily disabled. Please ensure that the function is updated to generate a random value as intended before merging the PR.

-    return sdkmath.LegacyMustNewDecFromStr("0.001")
+    return sdkmath.LegacyNewDec(int64(simulation.RandIntBetween(r, 1, 50))).Quo(sdkmath.LegacyNewDec(1000))

• 141-142: The MinDepositRatio parameter is being generated or retrieved from the AppParams map. This is consistent with the handling of other simulation parameters.

• 146-146: The MinDepositRatio parameter is being included in the Params struct for the GenesisState. Ensure that the Params struct and any functions that use it have been updated to handle this new field.

x/gov/keeper/deposit.go (4)

• 6-6: The import of the strings package is new. Ensure that it is used in the code and is not an unnecessary import.

• 75-122: The logic for validating the deposit amount and ratio has been significantly updated. The new logic checks if the deposit contains only valid denominations, and if the MinDepositRatio is set, it checks if the deposit is equal to or greater than MinDepositAmount * MinDepositRatio for at least one denomination. If these conditions are not met, appropriate errors are returned. This is a significant change and should be thoroughly tested to ensure it works as expected.

• 132-137: The logic for checking if the deposit has provided sufficient total funds to transition the proposal into the voting period has been simplified. The retrieval of the params and minDepositAmount has been removed from this section, presumably because it has been handled earlier in the code. Ensure that this change does not affect the functionality of this section.

• 276-285: The function validateInitialDeposit now checks if the initialDeposit is valid and not negative before proceeding with the rest of the validation. This is a good practice as it prevents further processing of invalid data.

x/gov/abci_test.go (5)

• 98-104: The deposit amount for the proposal has been increased from 5 to 10000. Ensure that this change aligns with the new MinDepositRatio parameter and that the tests are still valid with this new deposit amount.

• 149-155: The deposit amount for the proposal has been increased from 5 to 10000. Ensure that this change aligns with the new MinDepositRatio parameter and that the tests are still valid with this new deposit amount.

• 172-178: The deposit amount for the proposal has been increased from 5 to 10000. Ensure that this change aligns with the new MinDepositRatio parameter and that the tests are still valid with this new deposit amount.

• 218-224: The deposit amount for the proposal has been increased from 5 to 10000. Ensure that this change aligns with the new MinDepositRatio parameter and that the tests are still valid with this new deposit amount.

• 241-247: The deposit amount for the proposal has been increased from 5 to 10000. Ensure that this change aligns with the new MinDepositRatio parameter and that the tests are still valid with this new deposit amount.

proto/cosmos/gov/v1/gov.proto (2)

• 262-263: The burn_vote_veto field has been retained in the Params message. No changes have been made to this field.

• 265-270: A new field min_deposit_ratio has been added to the Params message. This field represents the minimum deposit ratio that must be met when making a deposit. The field is of type string and uses the cosmos.Dec scalar. Ensure that the new field is properly handled in the codebase where the Params message is used.

+  // 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
+  // required.
+  //
+  // Since: cosmos-sdk 0.50
+  string min_deposit_ratio = 16 [(cosmos_proto.scalar) = "cosmos.Dec"];

api/cosmos/gov/v1/gov.pulsar.go (15)

• 5547-5550: The new field fd_Params_min_deposit_ratio of type protoreflect.FieldDescriptor has been added. Ensure that this field is correctly initialized and used throughout the codebase.

• 5568-5571: The new field fd_Params_min_deposit_ratio is being initialized here. Ensure that the field name "min_deposit_ratio" exists in md_Params.

• 5726-5734: The new field MinDepositRatio is being checked and its value is being processed here. Ensure that the function f can handle the value correctly.

• 5777-5781: The new field MinDepositRatio is being checked here. Ensure that the comparison x.MinDepositRatio != "" is the intended behavior.

• 5825-5829: The new field MinDepositRatio is being reset here. Ensure that setting x.MinDepositRatio to an empty string is the intended behavior for reset.

• 5894-5899: The new field MinDepositRatio is being processed here. Ensure that the function protoreflect.ValueOfString can handle the value correctly.

• 5951-5955: The new field MinDepositRatio is being set here. Ensure that the value is correctly cast to a string.

• 6020-6024: The new field MinDepositRatio is being checked here. Ensure that the panic message is correct and that the field is indeed not mutable.

• 6070-6074: The new field MinDepositRatio is being checked here. Ensure that returning an empty string is the intended behavior when the field is not set.

• 6202-6208: The new field MinDepositRatio is being processed here. Ensure that the length of x.MinDepositRatio is correctly added to n.

• 6235-6246: The new field MinDepositRatio is being processed here. Ensure that the field is correctly copied into dAtA and that the correct bytes are being set.

• 6906-6941: The new field MinDepositRatio is being unmarshalled here. Ensure that the unmarshalling process is correct and that the field is correctly set in x.

• 7725-7733: The new field MinDepositRatio has been added to the Params struct. Ensure that the protobuf tag is correct and that the JSON tag matches the intended JSON key.

• 7861-7866: The new getter GetMinDepositRatio has been added. Ensure that it returns the correct value and that it is used correctly throughout the codebase.

• 8003-8009: The new field MinDepositRatio is being processed here. Ensure that the protobuf tag and the bytes are correct.

tests/e2e/gov/tx.go (5)

• 59-70: The test case now includes a proposal with a minimum deposit calculated based on the DefaultMinDepositTokens and DefaultMinDepositRatio. Ensure that the new deposit calculation logic is correctly implemented and that the test case is correctly testing the new deposit validation logic.

• 130-133: The deposit amount in the proposal has been changed from 5431 to 10000. Ensure that this change aligns with the new deposit validation logic and that it does not affect other parts of the test suite.

• 198-201: The deposit amount in the proposal has been changed from 5431 to 15431. Ensure that this change aligns with the new deposit validation logic and that it does not affect other parts of the test suite.

• 224-230: The deposit amount in the proposal has been changed from 5431 to 10000. Ensure that this change aligns with the new deposit validation logic and that it does not affect other parts of the test suite.

• 249-255: The deposit amount in the proposal has been changed from 5431 to 10000. Ensure that this change aligns with the new deposit validation logic and that it does not affect other parts of the test suite.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 63c003d and e4b06f9.

Files selected for processing (1)

• proto/cosmos/gov/v1/gov.proto (1 hunks)

Files skipped from review due to trivial changes (1)

• proto/cosmos/gov/v1/gov.proto

[facundomedica]

DNM: I still need to solve simulations
Ready to merge, fixed simulations with a hack until we figure out #18317

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e4b06f9 and 05159a7.

Files selected for processing (1)

• x/gov/simulation/operations.go (2 hunks)

Additional comments: 1

x/gov/simulation/operations.go (1)

• 570-581: The minDepositRatio is parsed from a string value obtained from params.GetMinDepositRatio(). Ensure that the value of params.GetMinDepositRatio() is always a valid decimal string. If it's not guaranteed, consider adding error handling or validation for this value.

595:
The amount variable is multiplied by 3. This is indicated as a hack to set it to three times the required amount. If this is a temporary solution, consider revisiting this to implement a more robust solution.

597:
The condition for returning an error is modified to include the case where amount is less than threshold. Ensure that this change doesn't introduce unexpected behavior in the rest of the codebase.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 05159a7 and cbb97e6.

Files selected for processing (2)

• Makefile (1 hunks)
• x/gov/simulation/operations_test.go (3 hunks)

Files skipped from review due to trivial changes (1)

• Makefile

Additional comments: 3

x/gov/simulation/operations_test.go (3)

• 155-155: The initial deposit value has been updated. Ensure that this new value is consistent with the changes made in the x/gov module and that it meets the new minimum deposit limit.

• 188-188: The initial deposit value has been updated. Ensure that this new value is consistent with the changes made in the x/gov module and that it meets the new minimum deposit limit.

• 279-279: The deposit amount has been updated. Ensure that this new value is consistent with the changes made in the x/gov module and that it meets the new minimum deposit limit.

[julienrbrt]

Can we get a comment on the reason of hard-coding the seed?
This makes running that sim not more random anymore right?

[alexanderbez]

yeah we should not hard code a seed

[facundomedica]

sorry, I was testing and forgot to remove it!