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

Jump to bottom

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 all 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
5 changes: 5 additions & 0 deletions x/staking/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,7 @@ When this message is processed the following actions occur:

The `MsgUpdateParams` update the staking module parameters.
The params are updated through a governance proposal where the signer is the gov module account address.
When the `MinCommissionRate` is updated, all validators with a lower (max) commission rate than `MinCommissionRate` will be updated to `MinCommissionRate`.

```protobuf reference
https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/staking/v1beta1/tx.proto#L182-L195
Expand Down Expand Up @@ -1037,6 +1038,10 @@ The staking module contains the following parameters:
| KeyRotationFee | sdk.Coin | "1000000stake" |
| MaxConsPubkeyRotations | int | 1 |

:::warning
Manually updating the `MinCommissionRate` parameter will not affect the commission rate of the existing validators. It will only affect the commission rate of the new validators. Update the parameter with `MsgUpdateParams` to affect the commission rate of the existing validators as well.
:::

## Client

### CLI
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 staking 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 commission 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)
}
}
}
}

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