feat(x/staking): update validators min commission rate after MsgUpdateParams
#19537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julienrbrt
changed the title
MsgUpdateParamsMsgUpdateParams
WalkthroughWalkthroughThe recent update introduces a significant enhancement to the staking module by enabling dynamic updates to the Changes
Assessment against linked issues
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: 2
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/staking/CHANGELOG.md (1 hunks)
- x/staking/keeper/msg_server.go (2 hunks)
- x/staking/keeper/msg_server_test.go (9 hunks)
Additional comments: 4
x/staking/CHANGELOG.md (1)
- 30-30: The changelog entry is clear and follows the established format, correctly categorizing the change as a new feature and providing a direct link to the PR for more context.
x/staking/keeper/msg_server_test.go (3)
- 1030-1041: The creation of a validator for commission rate testing is well-implemented. However, it's crucial to ensure that the commission rates set during validator creation align with the updated
MinCommissionRatein theMsgUpdateParamstest cases. This ensures that the tests accurately reflect the intended functionality of updating minimum commission rates for validators.- 1064-1075: The test case for updating parameters with an updated minimum commission rate is well-structured. It correctly verifies that after updating the
MinCommissionRate, the commission rate of existing validators is adjusted accordingly. This aligns with the PR's objective to dynamically adjust validators' commission rates based on governance decisions. It's important to ensure that this behavior is consistent across all validators in the network.- 1181-1189: The error handling and post-check logic in the
MsgUpdateParamstest cases are correctly implemented. However, it's essential to ensure comprehensive coverage for all possible error scenarios and parameter updates. This includes testing for edge cases and validating that all network policies regarding commission rates are enforced correctly.
x/staking/keeper/msg_server.go
Outdated
Comment on lines
602
to
638
| // get previous params params | ||
| previousParams, err := k.Params.Get(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // store params | ||
| if err := k.Params.Set(ctx, msg.Params); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // when min comission rate is updated, we need to update the commission rate of all validators | ||
| if !previousParams.MinCommissionRate.Equal(msg.Params.MinCommissionRate) { | ||
| minRate := msg.Params.MinCommissionRate | ||
|
|
||
| vals, err := k.GetAllValidators(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, val := range vals { | ||
| // set the commission rate to min rate | ||
| if val.Commission.CommissionRates.Rate.LT(minRate) { | ||
| val.Commission.CommissionRates.Rate = minRate | ||
| // set the max rate to minRate if it is less than min rate | ||
| if val.Commission.CommissionRates.MaxRate.LT(minRate) { | ||
| val.Commission.CommissionRates.MaxRate = minRate | ||
| } | ||
|
|
||
| val.Commission.UpdateTime = k.environment.HeaderService.GetHeaderInfo(ctx).Time | ||
| if err := k.SetValidator(ctx, val); err != nil { | ||
| return nil, fmt.Errorf("failed to set validator after MinCommissionRate param change: %w", err) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The logic to update all validators' commission rates when the MinCommissionRate parameter changes is correctly implemented. However, there are a few considerations:
- Performance: Iterating over all validators and updating them one by one could be inefficient for a large number of validators. Consider batching updates or optimizing the database access pattern if possible.
- Error Handling: The loop continues even if setting a validator fails. This could lead to partial updates without clear visibility. Consider adding logging or a mechanism to track failed updates.
- Commission Rate Adjustment Logic: The logic correctly checks if the current rate is less than the new minimum rate and updates it accordingly. However, it also adjusts the
MaxRateto theMinRateif it's lower, which might not be intended. Confirm this behavior aligns with the business logic. - Best Practices: The use of
k.environment.HeaderService.GetHeaderInfo(ctx).Timefor setting theUpdateTimeis appropriate, ensuring that the update timestamp reflects the transaction time.
Overall, the implementation meets the objectives, but consider the above points for potential refinement.
Comment on lines
1051
to
1085
| Authority: keeper.GetAuthority(), | ||
| Params: types.DefaultParams(), | ||
| }, | ||
| expErr: false, | ||
| postCheck: func() { | ||
| // verify that the commission isn't changed | ||
| vals, err := keeper.GetAllValidators(ctx) | ||
| require.NoError(err) | ||
| require.Len(vals, 1) | ||
| require.True(vals[0].Commission.Rate.Equal(comm.Rate)) | ||
| require.True(vals[0].Commission.MaxRate.GTE(comm.MaxRate)) | ||
| }, | ||
| }, | ||
| { | ||
| name: "valid params with updated min commission rate", | ||
| input: &types.MsgUpdateParams{ | ||
| Authority: keeper.GetAuthority(), | ||
| Params: paramsWithUpdatedMinCommissionRate, | ||
| }, | ||
| postCheck: func() { | ||
| vals, err := keeper.GetAllValidators(ctx) | ||
| require.NoError(err) | ||
| require.Len(vals, 1) | ||
| require.True(vals[0].Commission.Rate.GTE(paramsWithUpdatedMinCommissionRate.MinCommissionRate)) | ||
| require.True(vals[0].Commission.MaxRate.GTE(paramsWithUpdatedMinCommissionRate.MinCommissionRate)) | ||
| }, | ||
| }, | ||
| { | ||
| name: "invalid authority", | ||
| input: &types.MsgUpdateParams{ | ||
| Authority: "invalid", | ||
| Params: types.DefaultParams(), | ||
| }, | ||
| expErr: true, | ||
| expErrMsg: "invalid authority", | ||
| }, | ||
| { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1030-1189]
While the test cases for MsgUpdateParams effectively cover scenarios with valid and invalid parameters, including the updated minimum commission rate, it would be beneficial to add more test cases that specifically target the automatic adjustment of validators' commission rates. This could include scenarios where multiple validators have commission rates below the new minimum and ensuring that their rates are updated correctly. Additionally, testing the persistence of these changes across blocks would further validate the robustness of the implementation.
// Suggested test case addition:
// Test automatic adjustment of validators' commission rates when the MinCommissionRate is updated.
func (s *KeeperTestSuite) TestAutomaticCommissionRateAdjustment() {
// Setup: Create multiple validators with varying commission rates.
// Action: Update the MinCommissionRate through MsgUpdateParams.
// Assert: All validators have their commission rates adjusted to at least the new MinCommissionRate.
}
julienrbrt
force-pushed
the
julien/staking-msgparamsupgrade
branch
from
5e2ac0a
to
9a27340
Compare
facundomedica
approved these changes
Feb 23, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- x/staking/README.md (2 hunks)
- x/staking/keeper/msg_server.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/staking/README.md
- x/staking/keeper/msg_server.go
alexanderbez
approved these changes
Feb 23, 2024
| @@ -598,11 +599,43 @@ func (k msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) | |||
| return nil, err | |||
| } | |||
|
|
|||
| // get previous staking params | |||
| previousParams, err := k.Params.Get(ctx) | |||
There was a problem hiding this comment.
nit:
Suggested change
| previousParams, err := k.Params.Get(ctx) | |
| currParams, err := k.Params.Get(ctx) |
|
Your awesome thank you so much |
12 tasks
This was referenced May 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes: #10540
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...
!in 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...
Summary by CodeRabbit