feat!(x/gov): implement a minimum amount per deposit (backport #19312) #510
Conversation
Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Julien Robert <[email protected]>
|
@mattverse can you ping when e2e test is passing and i will review |
|
@czarcas7ic this is r4r! |
czarcas7ic
force-pushed
the
osmo/v0.47.5
branch
from
8992442 to
e95dddd
Compare
|
Yeah I am fairly confident this is already implemented but just with a different name @mattverse, can you double check my claim |
|
Going to set as draft until response |
|
@czarcas7ic This is different from the initial minimum deposit we have. This state is for having a minimum deposit for each deposit tx user can make. |
WalkthroughThe update introduces a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
api/cosmos/gov/v1/gov.pulsar.gois excluded by:!**/*.pulsar.gox/gov/types/v1/gov.pb.gois excluded by:!**/*.pb.go
Files selected for processing (20)
- proto/cosmos/gov/v1/gov.proto (1 hunks)
- tests/e2e/gov/deposits.go (2 hunks)
- tests/e2e/gov/query.go (2 hunks)
- tests/e2e/gov/tx.go (6 hunks)
- x/bank/app_test.go (1 hunks)
- x/gov/abci_test.go (5 hunks)
- x/gov/keeper/deposit.go (5 hunks)
- x/gov/keeper/deposit_test.go (2 hunks)
- x/gov/keeper/export_test.go (1 hunks)
- x/gov/keeper/msg_server.go (4 hunks)
- x/gov/keeper/msg_server_test.go (10 hunks)
- x/gov/migrations/v4/json.go (1 hunks)
- x/gov/migrations/v4/json_test.go (1 hunks)
- x/gov/migrations/v4/store.go (1 hunks)
- x/gov/migrations/v4/store_test.go (1 hunks)
- x/gov/simulation/genesis.go (3 hunks)
- x/gov/simulation/operations.go (2 hunks)
- x/gov/simulation/operations_test.go (3 hunks)
- x/gov/types/errors.go (1 hunks)
- x/gov/types/v1/params.go (4 hunks)
Additional comments: 47
x/gov/keeper/export_test.go (1)
- 10-11: The update to include
params v1.Paramsin theValidateInitialDepositfunction aligns with the PR's objectives and correctly implements the necessary logic for testing deposit validation with governance parameters. Good encapsulation and reuse of existing logic.x/gov/migrations/v4/json.go (1)
- 31-32: The inclusion of
MinDepositRatioin theParamsconstructor during the migration process correctly integrates the new governance parameter. This change is essential for ensuring that theMinDepositRatioparameter is set during migrations, aligning with the PR's objectives.x/gov/types/errors.go (1)
- 26-27: The introduction of
ErrInvalidDepositDenomwith code 23 enhances error handling by providing a clear and specific error message for invalid deposit denominations. This addition improves the robustness of the governance module.x/gov/migrations/v4/json_test.go (1)
- 80-80: The inclusion of the
min_deposit_ratiofield in the JSON structure for testing the migration logic correctly reflects the addition of theMinDepositRatioparameter. This change ensures comprehensive testing of the migration logic.x/gov/migrations/v4/store.go (1)
- 43-44: The inclusion of
MinDepositRatioin themigrateParamsfunction during the migration process correctly integrates the new governance parameter. This change is essential for ensuring that theMinDepositRatioparameter is set during migrations, aligning with the PR's objectives.x/gov/migrations/v4/store_test.go (1)
- 107-107: The addition of an assertion for the
MinDepositRatioparameter in theTestMigrateStorefunction correctly tests the migration logic, ensuring theMinDepositRatioparameter is correctly handled during migrations.tests/e2e/gov/deposits.go (1)
- 125-125: The modification to error message concatenation in
TestQueryProposalAfterVotingPeriodand the removal ofTestQueryDepositsWithoutInitialDepositalign with the PR's objectives and enhance the clarity and relevance of the e2e tests.x/gov/simulation/genesis.go (1)
- 100-103: The introduction of
GenMinDepositRatioand its integration into theRandomizedGenStatefunction correctly simulates the newMinDepositRatioparameter, ensuring comprehensive simulation coverage for the governance module enhancements.x/gov/types/v1/params.go (3)
- 31-31: The addition of
DefaultMinDepositRatiowith a value ofsdk.MustNewDecFromStr("0.01")introduces a new default parameter for the minimum deposit ratio. This change aligns with the PR's objective to implement a minimum deposit amount per deposit.- 59-65: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [62-78]
The update to the
NewParamsfunction signature to includeminDepositRatioas a string parameter and its assignment within the function ensures that the new minimum deposit ratio parameter is properly integrated into the governance module's parameters. This change is necessary for the feature's implementation and follows best practices for extending functionality.
- 98-98: The adjustment in the
DefaultParamsfunction to includeDefaultMinDepositRatio.String()ensures that the new default minimum deposit ratio is correctly applied when generating default parameters. This change is consistent with the PR's objective and follows best practices for managing default configurations.x/gov/keeper/msg_server.go (3)
- 36-38: Fetching the governance parameters within the
SubmitProposalfunction ensures that the latest parameters are used for validating the initial deposit. This is a necessary step for implementing the new minimum deposit ratio feature and adheres to best practices for accessing dynamic configuration values.- 168-172: The modification in the
Depositfunction to callvalidateDepositfor validating the deposit amount is crucial for enforcing the new minimum deposit ratio requirement. This ensures that all deposits meet the specified criteria, enhancing the governance mechanism's robustness.- 198-205: The addition of the
validateDepositfunction provides a centralized way to validate deposit amounts against the governance module's requirements. This function ensures that deposits are valid and positive, aligning with the PR's objective to enforce minimum deposit ratios. It's a good practice to encapsulate validation logic in a dedicated function for maintainability and reusability.x/gov/keeper/deposit.go (3)
- 122-166: The implementation of deposit amount and ratio validation in the
AddDepositfunction, including the newminDepositRatiocheck, is crucial for enforcing the minimum deposit requirements for governance proposals. This change effectively integrates the new feature into the deposit handling logic, ensuring that deposits meet the specified criteria. The error handling and messaging provide clear feedback to users about the deposit requirements.- 230-240: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [233-261]
The addition of the
validateInitialDepositfunction to validate the initial deposit against the minimum required at the time of proposal submission is a significant enhancement. This function ensures that the initial deposit meets the specified ratio, aligning with the PR's objective to implement a minimum deposit amount per deposit. The validation logic is well-structured and adheres to best practices.
- 262-278: The
validateDepositDenomfunction ensures that the deposit denomination is accepted by the governance module, aligning with the new minimum deposit ratio feature. This validation is essential for maintaining the integrity of the governance process and ensuring that deposits are made in the correct denominations. The implementation follows best practices for validation and error handling.proto/cosmos/gov/v1/gov.proto (1)
- 256-263: The addition of the
min_deposit_ratiofield in theParamsmessage is a key part of implementing the new minimum deposit ratio feature. This change allows for the configuration of the minimum deposit ratio at the protocol level, enhancing the flexibility and robustness of the governance mechanism. The documentation and default value provide clear guidance on the purpose and usage of this new parameter.tests/e2e/gov/query.go (2)
- 26-26: The addition of the
min_deposit_ratiofield in the JSON output of theTestCmdParamsfunction correctly reflects the new governance parameter introduced in the Cosmos SDK. This change ensures that the end-to-end tests are updated to account for the new governance feature.- 50-50: Similarly, the inclusion of the
min_deposit_ratiofield in the text output of theTestCmdParamsfunction is consistent with the changes made to the governance module. This update ensures that the text output remains comprehensive and includes all relevant governance parameters.x/bank/app_test.go (2)
- 353-353: The change in funding the
addr1account fromfoocointostakecoins, and adjusting the amount to100000from5, is a significant modification. This change aligns with the governance proposal's deposit requirement in the test case, ensuring that the test reflects the actual conditions under which the governance proposal would be submitted.- 360-360: The deposit amount specified in the governance proposal (
sdk.Coins{{Denom: "stake", Amount: sdk.NewInt(100000)}}) matches the funding provided to theaddr1account. This consistency is crucial for the validity of the test case, ensuring that the governance proposal can be submitted without issues related to insufficient funds.x/gov/keeper/deposit_test.go (2)
- 145-221: The addition of
TestDepositAmountprovides comprehensive coverage for validating deposit amounts against minimum deposit ratios. It's well-structured and covers a variety of scenarios. Consider adding comments to each test case for better clarity on their specific validation purpose.- 142-225: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [222-324]
The update to
TestValidateInitialDepositis necessary for testing the new minimum deposit ratio feature. The test cases are well-designed to cover various scenarios, ensuring the functionality works as expected.tests/e2e/gov/tx.go (4)
- 57-61: Adjusting the deposit amount for proposals based on
DefaultMinDepositTokensandDefaultMinDepositRatiocorrectly tests the new minimum deposit functionality. This ensures that the governance module's updated behavior is accurately reflected in the end-to-end tests.- 116-116: The update to the deposit amount in the
TestNewCmdSubmitProposalfunction is appropriate for testing the proposal submission functionality under the new minimum deposit requirements.- 184-184: The update to the deposit amount in the
TestNewCmdSubmitLegacyProposalfunction aligns with the new minimum deposit requirements, ensuring comprehensive test coverage for legacy proposal submissions.- 308-319: Including a test case that fails due to an insufficient minimum deposit amount in the
TestNewCmdDepositfunction is crucial for validating the governance module's enforcement of the new deposit requirements.x/gov/simulation/operations.go (1)
- 458-470: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [461-487]
The implementation of the
minDepositRatiocalculation and its application in determining the deposit threshold is logically sound. However, the use ofMulRaw(3)on line 485 as a workaround to ensure the deposit amount meets certain criteria is marked as a hack with a TODO comment. This approach might not be the most accurate or flexible for simulations. It's recommended to explore more dynamic methods to calculate the deposit amount that aligns with the governance parameters, ensuring the simulation reflects realistic scenarios more closely.x/gov/simulation/operations_test.go (3)
- 155-155: The stake amount for
InitialDepositinTestSimulateMsgSubmitProposalhas been updated to"1682907stake". Ensure this value aligns with the new minimum deposit requirements introduced in the governance module. It's important to verify that this value is consistent with themin_deposit_ratioset in the governance parameters.- 185-185: The stake amount for
InitialDepositinTestSimulateMsgSubmitLegacyProposalhas been updated to"40290165stake". Similar to the previous comment, verify that this updated value is in accordance with the new minimum deposit requirements. Consistency with themin_deposit_ratioparameter is crucial for ensuring the validity of these tests.- 234-234: The stake amount for
AmountinTestSimulateMsgDeposithas been updated to"1682907stake". This change is presumably to reflect the new minimum deposit requirements for governance proposals. As with the other tests, it's important to ensure that this updated value is consistent with themin_deposit_ratioparameter in the governance module's configuration.x/gov/abci_test.go (5)
- 42-42: The deposit amount in the
TestTickExpiredDepositPeriodtest case has been updated from 5 to 100000. This change aligns with the PR's objective to enforce a minimum deposit amount for governance proposals. Ensure that this new minimum deposit amount is consistent across all relevant test cases and documentation.- 99-99: The deposit amount in the
TestTickMultipleExpiredDepositPeriodtest case for the first proposal submission has been updated to 100000. This adjustment is necessary to comply with the new minimum deposit requirements. Verify that similar updates are applied consistently in all test cases that involve proposal submissions.- 126-126: The deposit amount for the second proposal submission in the
TestTickMultipleExpiredDepositPeriodtest case has also been updated to 100000. This consistency in applying the new minimum deposit amount across multiple proposals within the same test ensures that the test accurately reflects the updated governance module behavior.- 188-188: In the
TestTickPassedDepositPeriodtest case, the deposit amount for the proposal submission has been updated to 100000. This change is part of the effort to test the governance module's behavior with the new minimum deposit amount. It's important to ensure that the test logic and assertions are still valid with this updated deposit amount.- 215-215: The deposit amount for the
TestTickPassedDepositPeriodtest case, specifically for the deposit transaction, has been updated to 100000. This update is crucial for testing the behavior of deposit transactions under the new minimum deposit requirements. Confirm that the test accurately reflects the expected behavior of the governance module with these changes.x/gov/keeper/msg_server_test.go (10)
- 21-21: The stake coin amount has been updated from 100 to 100000 in the
TestSubmitProposalReqfunction to reflect the new minimum deposit requirements. This change aligns with the PR objectives to implement a minimum deposit amount for governance proposals.- 95-124: New test cases for handling invalid deposited coins have been added to the
TestSubmitProposalReqfunction. These test cases ensure that the system correctly handles deposits with invalid denominations, both for single and multiple invalid coins. This is a crucial addition for robustness and aligns with the PR's objectives to enhance the governance mechanism.- 177-177: The stake coin amount has been updated from 100 to 100000 in the
TestVoteReqfunction. This update is necessary to match the new minimum deposit requirements and ensure consistency across test cases.- 299-299: The stake coin amount has been updated from 100 to 100000 in the
TestVoteWeightedReqfunction. This change ensures that the test cases reflect the updated governance module's requirements for minimum deposits.- 421-421: The stake coin amount has been updated from 100 to 100000 in the
TestDepositReqfunction. This adjustment is consistent with the changes made in other test functions and supports the PR's goal of implementing a minimum deposit amount for governance proposals.- 462-478: New test cases for handling invalid deposited coins have been added to the
TestDepositReqfunction. These test cases are important for verifying that the system correctly rejects deposits with invalid denominations, enhancing the governance module's robustness.- 509-509: The stake coin amount has been updated from 100 to 100000 in the
TestLegacyMsgSubmitProposalfunction. This change is part of the overall update to test cases to reflect the new minimum deposit requirements in the governance module.- 559-559: The stake coin amount has been updated from 100 to 100000 in the
TestLegacyMsgVotefunction. This update ensures that legacy voting functionality is tested with the updated minimum deposit requirements.- 671-671: The stake coin amount has been updated from 100 to 100000 in the
TestLegacyVoteWeightedfunction. This adjustment aligns with the PR's objectives and ensures that legacy weighted voting functionality is tested under the new minimum deposit conditions.- 783-783: The stake coin amount has been updated from 100 to 100000 in the
TestLegacyMsgDepositfunction. This change is consistent with updates made in other test functions and is necessary for testing the legacy deposit functionality with the new minimum deposit requirements.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/gov/keeper/deposit.go (5 hunks)
- x/gov/simulation/operations.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/gov/keeper/deposit.go
- x/gov/simulation/operations.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/gov/simulation/operations_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/gov/simulation/operations_test.go
|
@czarcas7ic another reason Im sure that these are two different things is that upstream also has these two different states. Linking the proto file below for reference. https://github.com/cosmos/cosmos-sdk/blob/main/x/gov/proto/cosmos/gov/v1/gov.proto#L296 |
x/gov/keeper/deposit.go
Outdated
| // NOTE: backported from v50 | ||
| // v47 does not have expedited proposals so we always use params.MinDeposit | ||
| minDepositAmount := params.MinDeposit | ||
| minDepositRatio, err := sdk.NewDecFromStr(params.GetMinDepositRatio()) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if err := keeper.validateDepositDenom(ctx, params, depositAmount); err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
We do have expedited proposals, so we need to include the expedited proposal logic @mattverse
There was a problem hiding this comment.
Oof good catch! Fixed in the most recent commit!
| // NOTE: backport from v50 | ||
| amount = amount.MulRaw(3) // 3x what's required // TODO: this is a hack, we need to be able to calculate the correct amount using params | ||
|
|
There was a problem hiding this comment.
This doesn't make sense to me, but I assume its just backporting what they did
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 5
CHANGELOG.md (5)
- 45-45: The description of the new feature in the governance module is clear and follows the changelog's format. However, there are several typographical and grammatical issues throughout the document that need attention for clarity and professionalism.
- 47-47: The abbreviation "IAVL" might not be immediately clear to all readers. Consider adding a brief explanation or expanding the acronym at its first occurrence.
- 42-48: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [108-108]
The term "SetMsgs" in "
MsgSubmitProposalSetMsgsmethod" might be unclear. Consider explaining or simplifying the terminology.
- 42-48: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [116-116]
The term "db_backend" in "Fix
db_backendlookup fallback fromconfig.toml." might be unclear to some readers. Consider expanding it for clarity.
- 42-48: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [120-120]
The term "msgURL" in "Do not try validate
msgURLas web URL" might be unclear. Consider explaining or simplifying the terminology.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 6
CHANGELOG.md (6)
- 44-50: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [42-42]
The term "CacheKV speedups" might be unclear to some readers. Consider rephrasing for clarity, possibly explaining briefly what "CacheKV" refers to.
- 45-45: The abbreviation "authz" is used without explanation. For clarity, consider expanding it to "authorization" or providing a brief explanation the first time it is used.
- 44-50: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [52-52]
The term "denom validation" might be unclear to some readers. Consider expanding or clarifying what "denom" refers to for better understanding.
- 44-50: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [106-106]
The version number
[v0.47.5]appears to be missing a link or reference. Ensure version numbers are consistently linked to their respective release notes or documentation for user convenience.
- 44-50: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [110-110]
The term "QueryEventForTxCmd" might be unclear to some readers. Consider adding a brief explanation or context to make it more accessible to all readers.
- 44-50: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [241-241]
The term "PutUnbondingOnHold" might be unclear to some readers. Consider adding a brief explanation or context to make it more accessible to all readers.
…cosmos#19312) (#510)" This reverts commit 106ba23.
…cosmos#19312) (#510)" This reverts commit 106ba23.
…cosmos#19312) (#510)" This reverts commit 106ba23.
Description
Backport: cosmos#19312 and cosmos#18146
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit