From 4b73e31b0094cafead50d605febcebe7657bab7d Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 23 Feb 2024 18:17:01 +0100 Subject: [PATCH] feat(x/staking): update validators min commission rate after `MsgUpdateParams` (#19537) --- x/staking/CHANGELOG.md | 2 ++ x/staking/README.md | 5 +++ x/staking/keeper/msg_server.go | 33 +++++++++++++++++++ x/staking/keeper/msg_server_test.go | 50 +++++++++++++++++++++++------ 4 files changed, 80 insertions(+), 10 deletions(-) diff --git a/x/staking/CHANGELOG.md b/x/staking/CHANGELOG.md index 236e06783a..6712884d8d 100644 --- a/x/staking/CHANGELOG.md +++ b/x/staking/CHANGELOG.md @@ -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. diff --git a/x/staking/README.md b/x/staking/README.md index a93a9de13b..713accb96c 100644 --- a/x/staking/README.md +++ b/x/staking/README.md @@ -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 @@ -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 diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index 49ad4dcf62..dbbfb7a649 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -3,6 +3,7 @@ package keeper import ( "context" "errors" + "fmt" "slices" "strconv" "time" @@ -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) + 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 } diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index 3b142ada4d..c0d9785557 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -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 }{ { @@ -1039,7 +1051,28 @@ 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", @@ -1047,7 +1080,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { Authority: "invalid", Params: types.DefaultParams(), }, - expErr: true, expErrMsg: "invalid authority", }, { @@ -1063,7 +1095,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "minimum commission rate cannot be negative", }, { @@ -1079,7 +1110,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "minimum commission rate cannot be greater than 100%", }, { @@ -1095,7 +1125,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: "", }, }, - expErr: true, expErrMsg: "bond denom cannot be blank", }, { @@ -1111,7 +1140,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "max validators must be positive", }, { @@ -1127,7 +1155,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "max entries must be positive", }, { @@ -1143,7 +1170,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: "denom", }, }, - expErr: true, expErrMsg: "unbonding time must be positive", }, } @@ -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() + } } }) }