feat(x/gov): limit deposited coins to accepted proposal denom #18189
Conversation
WalkthroughThis update brings significant improvements to the governance module, enhancing the handling of proposals and deposits. It introduces validation for deposit denominations, optimizes the Changes
TipsChat with CodeRabbit Bot (
|
julienrbrt
added
the
backport/v0.50.x
![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: 5
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- x/gov/keeper/deposit.go (4 hunks)
- x/gov/keeper/export_test.go (1 hunks)
- x/gov/keeper/msg_server.go (1 hunks)
- x/gov/keeper/msg_server_test.go (2 hunks)
- x/gov/types/errors.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/gov/types/errors.go
Additional comments (Suppressed): 8
x/gov/keeper/export_test.go (1)
- 13-13: The function signature of
validateInitialDepositseems to have changed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.x/gov/keeper/msg_server.go (2)
83-85: The
validateInitialDepositfunction now takes an additionalparamsargument. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.87-89: The
validateDepositDenomfunction is a new addition and it's good to see that error handling is properly done here. However, it would be helpful to add a comment explaining what this function does and why it's necessary.x/gov/keeper/deposit.go (2)
96-102: The retrieval of governance parameters has been removed from this section of the code. Ensure that the parameters are being retrieved before this check and are up-to-date. If the parameters are not retrieved or are outdated, the check for transitioning the proposal into the voting period might not work as expected.
243-249: The function
validateInitialDepositnow takesparamsas an argument instead of retrieving it within the function. This is a good practice as it reduces the number of database or network calls, improving performance. However, ensure that theparamspassed to this function are up-to-date and valid.x/gov/keeper/msg_server_test.go (3)
210-224: The test case "invalid deposited coin" checks if the system correctly handles a deposit with an invalid coin denomination. The expected error message indicates that the deposited coin's denomination is not accepted by the governance module. This is a good test case to ensure that the system correctly validates the deposited coins.
225-239: The test case "invalid deposited coin (multiple)" checks if the system correctly handles a deposit with multiple coins, one of which has an invalid denomination. The expected error message indicates that one of the deposited coins' denomination is not accepted by the governance module. This is a good test case to ensure that the system correctly validates each coin in a deposit with multiple coins.
825-842: The test cases "invalid deposited coin" and "invalid deposited coin (multiple)" in the
TestMsgDepositfunction are similar to the test cases in theTestMsgSubmitProposalfunction. They check if the system correctly handles a deposit with an invalid coin denomination and a deposit with multiple coins, one of which has an invalid denomination. The expected error messages are similar to the ones in theTestMsgSubmitProposalfunction. These are good test cases to ensure that the system correctly validates the deposited coins in theMsgDepositmessage handler.
x/gov/keeper/deposit.go
Outdated
| 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) { | ||
| return errors.Wrapf(types.ErrInvalidDepositDenom, "deposited %s, but gov accepts only the following denom(s): %v", depositAmount, denoms) | ||
| } | ||
|
|
||
| for _, coin := range depositAmount { | ||
| 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 | ||
| } |
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
won't do, as it requires looping through the whole depositAmount, which could be very big.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Oh wow, you can respond. Amazing.
Yes, performance is a concern.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments (Suppressed): 1
CHANGELOG.md (1)
- 58-58: The change log entry correctly summarizes the changes made in the PR. It mentions the new feature of limiting the accepted deposit coins for a proposal to the minimum proposal deposit denoms.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- tests/integration/bank/app_test.go (1 hunks)
- x/gov/keeper/msg_server.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/integration/bank/app_test.go
Additional comments (Suppressed): 2
x/gov/keeper/msg_server.go (2)
83-85: The validation of the initial deposit is a good practice. It ensures that the deposit meets the necessary requirements before proceeding. This can prevent potential issues down the line.
87-89: The addition of a validation step for the deposit denomination is a good practice. It ensures that the denomination of the deposit matches the accepted denominations specified in the governance parameters. This can prevent potential issues and confusion.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- x/gov/keeper/deposit.go (4 hunks)
- x/gov/keeper/msg_server_test.go (2 hunks)
Additional comments (Suppressed): 7
x/gov/keeper/msg_server_test.go (3)
210-224: The test case "invalid deposited coin" is checking if the function correctly handles a scenario where an invalid coin is deposited. The expected error message is correctly defined. This test case is important to ensure that the function correctly validates the deposited coins against the accepted denominations.
225-239: The test case "invalid deposited coin (multiple)" is checking if the function correctly handles a scenario where multiple coins are deposited, and one of them is invalid. The expected error message is correctly defined. This test case is important to ensure that the function correctly validates each deposited coin against the accepted denominations.
825-842: The test cases "invalid deposited coin" and "invalid deposited coin (multiple)" are similar to the ones in the previous hunk. They are checking if the function correctly handles scenarios where an invalid coin or multiple coins with an invalid one are deposited. The expected error messages are correctly defined. These test cases are important to ensure that the function correctly validates the deposited coins against the accepted denominations.
x/gov/keeper/deposit.go (4)
70-85: The new hunk introduces a check to validate if the coins to be deposited match the proposal's deposit parameters. This is a good addition as it ensures that only valid denominations are accepted for deposit. However, the retrieval of parameters from the
keeper.Paramscould potentially be a performance bottleneck if it involves a database or network call. Consider passing the parameters as an argument to the function if they are already available in the calling context.
[PERFORMANCE]96-102: The new hunk checks if the deposit has provided sufficient total funds to transition the proposal into the voting period. This is a good practice as it ensures that the proposal can only proceed to the voting period if it has met the minimum deposit requirement. However, the retrieval of parameters from the
keeper.Paramshas been removed in this hunk. Ensure that theparamsvariable is still correctly initialized before this point.243-249: The new hunk modifies the
validateInitialDepositfunction to takeparamsas an argument instead of retrieving it within the function. This is a good practice as it reduces the number of potentially expensive operations. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.271-286: The new hunk introduces a function
validateDepositDenomto 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 creation of theacceptedDenomsmap and thedenomsslice could potentially be a performance bottleneck if the number of denominations is large. Consider optimizing this by creating these data structures once and reusing them.
| denoms = append(denoms, coin.Denom) | ||
| } | ||
|
|
||
| for _, coin := range depositAmount { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- x/gov/README.md (4 hunks)
- x/gov/keeper/deposit.go (4 hunks)
Additional comments (Suppressed): 8
CHANGELOG.md (1)
- 58-58: The changelog entry correctly summarizes the changes made in the PR, specifically the addition of a feature to limit the deposited coins for a proposal to the accepted denominations specified in the governance parameters.
x/gov/README.md (3)
521-521: The new requirement for
initialDepositto conform to the accepted denom of theMinDepositparam is introduced. Ensure that this change is propagated throughout the codebase and any potential impacts are considered.535-542: The conditions for accepting a deposit have been updated to include a check for the deposited coins to conform to the accepted denom from the
MinDepositparam. This is a significant change and should be thoroughly tested.554-556: The pseudocode for handling
MsgDeposittransactions has been removed. Ensure that the logic is still correctly implemented in the actual code.x/gov/keeper/deposit.go (4)
73-85: The retrieval of parameters from
keeper.Paramscould potentially be a performance bottleneck if it involves a database or network call. Consider passing the parameters as an argument to the function if they are already available in the calling context. This would reduce the number of potentially expensive operations and improve performance. However, if the parameters are not frequently changing, the current implementation is efficient.
[PERFORMANCE]96-102: The new hunk introduces a check to validate if the deposit has provided sufficient total funds to transition the proposal into the voting period. This is a good addition as it ensures that only valid denominations are accepted for deposit. However, the retrieval of parameters from the
keeper.Paramscould potentially be a performance bottleneck if it involves a database or network call. Consider passing the parameters as an argument to the function if they are already available in the calling context. This would reduce the number of potentially expensive operations and improve performance. However, if the parameters are not frequently changing, the current implementation is efficient.
[PERFORMANCE]243-249: The new hunk introduces a check to validate if the initial deposit is greater than or equal to the minimum required at the time of proposal submission. This is a good addition as it ensures that only valid denominations are accepted for deposit. However, the retrieval of parameters from the
keeper.Paramscould potentially be a performance bottleneck if it involves a database or network call. Consider passing the parameters as an argument to the function if they are already available in the calling context. This would reduce the number of potentially expensive operations and improve performance. However, if the parameters are not frequently changing, the current implementation is efficient.
[PERFORMANCE]271-287: The new function
validateDepositDenomis a good addition as it validates the denomination of the deposit against the accepted denominations specified in the governance parameters. This ensures that only valid denominations are accepted for deposit. The use of a map foracceptedDenomsis a good choice as it allows for efficient lookup of denominations. However, consider using a set data structure if available, as it would be more semantically appropriate for this use case.
(cherry picked from commit 2fbc547) # Conflicts: # CHANGELOG.md
Description
A-10
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.mdmake lintandmake testReviewers 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
Slashfunction to return early if there are zero tokens to burn, enhancing system efficiency.q gov proposercommand to directly query a proposal, increasing command reliability.ErrInvalidDepositDenomfor better error tracking.