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.

Sign up for GitHub

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/staking): update validators min commission rate after MsgUpdateParams #19537

Merged
merged 5 commits into from
Feb 23, 2024
Merged

feat(x/staking): update validators min commission rate after MsgUpdateParams #19537

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55f99e4
feat(x/staking): update validators min commission rater after `MsgUpd…
julienrbrt Feb 23, 2024
ea1c457
updates
julienrbrt Feb 23, 2024
3ee996f
docs
julienrbrt Feb 23, 2024
9a27340
typos
julienrbrt Feb 23, 2024
cfe73c7
`make lint-fix`
julienrbrt Feb 23, 2024
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
2 changes: 2 additions & 0 deletions x/staking/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* [#19537](https://github.com/cosmos/cosmos-sdk/pull/19537) Changing `MinCommissionRate` in `MsgUpdateParams` now updates the minimum commission rate for all validators.

### Improvements

* [#19277](https://github.com/cosmos/cosmos-sdk/pull/19277) Hooks calls on `SetUnbondingDelegationEntry`, `SetRedelegationEntry`, `Slash` and `RemoveValidator` returns errors instead of logging just like other hooks calls.
Expand Down
33 changes: 33 additions & 0 deletions x/staking/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper
import (
"context"
"errors"
"fmt"
"slices"
"strconv"
"time"
Expand Down Expand Up @@ -598,11 +599,43 @@ func (k msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams)
return nil, err
}

// get previous params params
previousParams, err := k.Params.Get(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
previousParams, err := k.Params.Get(ctx)
currParams, 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)
}
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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 MaxRate to the MinRate if 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).Time for setting the UpdateTime is appropriate, ensuring that the update timestamp reflects the transaction time.

Overall, the implementation meets the objectives, but consider the above points for potential refinement.

return &types.MsgUpdateParamsResponse{}, nil
}

Expand Down
50 changes: 40 additions & 10 deletions x/staking/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,10 +1027,22 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
ctx, keeper, msgServer := s.ctx, s.stakingKeeper, s.msgServer
require := s.Require()

// create validator to test commission rate
pk := ed25519.GenPrivKey().PubKey()
require.NotNil(pk)
comm := types.NewCommissionRates(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0))
s.bankKeeper.EXPECT().DelegateCoinsFromAccountToModule(gomock.Any(), Addr, types.NotBondedPoolName, gomock.Any()).AnyTimes()
msg, err := types.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(10)), types.Description{Moniker: "NewVal"}, comm, math.OneInt())
require.NoError(err)
_, err = msgServer.CreateValidator(ctx, msg)
require.NoError(err)
paramsWithUpdatedMinCommissionRate := types.DefaultParams()
paramsWithUpdatedMinCommissionRate.MinCommissionRate = math.LegacyNewDecWithPrec(5, 2)

testCases := []struct {
name string
input *types.MsgUpdateParams
expErr bool
postCheck func()
expErrMsg string
}{
{
Expand All @@ -1039,15 +1051,35 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
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",
},
{
Comment on lines 1051 to 1085
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 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.
}

Expand All @@ -1063,7 +1095,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
BondDenom: types.BondStatusBonded,
},
},
expErr: true,
expErrMsg: "minimum commission rate cannot be negative",
},
{
Expand All @@ -1079,7 +1110,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
BondDenom: types.BondStatusBonded,
},
},
expErr: true,
expErrMsg: "minimum commission rate cannot be greater than 100%",
},
{
Expand All @@ -1095,7 +1125,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
BondDenom: "",
},
},
expErr: true,
expErrMsg: "bond denom cannot be blank",
},
{
Expand All @@ -1111,7 +1140,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
BondDenom: types.BondStatusBonded,
},
},
expErr: true,
expErrMsg: "max validators must be positive",
},
{
Expand All @@ -1127,7 +1155,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
BondDenom: types.BondStatusBonded,
},
},
expErr: true,
expErrMsg: "max entries must be positive",
},
{
Expand All @@ -1143,7 +1170,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
BondDenom: "denom",
},
},
expErr: true,
expErrMsg: "unbonding time must be positive",
},
}
Expand All @@ -1152,11 +1178,15 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() {
tc := tc
s.T().Run(tc.name, func(t *testing.T) {
_, err := msgServer.UpdateParams(ctx, tc.input)
if tc.expErr {
if tc.expErrMsg != "" {
require.Error(err)
require.Contains(err.Error(), tc.expErrMsg)
} else {
require.NoError(err)

if tc.postCheck != nil {
tc.postCheck()
}
}
})
}
Expand Down
Loading