From 55f99e4d580ef06b3d73e17296b49de9c0f58056 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 23 Feb 2024 14:42:58 +0100 Subject: [PATCH 1/5] feat(x/staking): update validators min commission rater after `MsgUpdateParams` --- x/staking/CHANGELOG.md | 2 ++ x/staking/keeper/msg_server.go | 33 +++++++++++++++++++++++++++++ x/staking/keeper/msg_server_test.go | 28 +++++++++++++++--------- 3 files changed, 53 insertions(+), 10 deletions(-) diff --git a/x/staking/CHANGELOG.md b/x/staking/CHANGELOG.md index 236e06783a54..5067befc4137 100644 --- a/x/staking/CHANGELOG.md +++ b/x/staking/CHANGELOG.md @@ -27,6 +27,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features +* []() Governance change of `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/keeper/msg_server.go b/x/staking/keeper/msg_server.go index 49ad4dcf6278..c69ac161b642 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 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 minRate + 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 3b142ada4d03..7a6c57f55e8c 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -1027,10 +1027,13 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { ctx, keeper, msgServer := s.ctx, s.stakingKeeper, s.msgServer require := s.Require() + // create validators to test commission rate + // TODO + testCases := []struct { name string input *types.MsgUpdateParams - expErr bool + postCheck func() expErrMsg string }{ { @@ -1039,7 +1042,15 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { Authority: keeper.GetAuthority(), Params: types.DefaultParams(), }, - expErr: false, + postCheck: func() { + vals, err := keeper.GetAllValidators(ctx) + require.NoError(err) + require.Len(vals, 6) + for _, val := range vals { + require.True(val.Commission.Rate.GTE(types.DefaultParams().MinCommissionRate)) + require.True(val.Commission.MaxRate.GTE(types.DefaultParams().MinCommissionRate)) + } + }, }, { name: "invalid authority", @@ -1047,7 +1058,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { Authority: "invalid", Params: types.DefaultParams(), }, - expErr: true, expErrMsg: "invalid authority", }, { @@ -1063,7 +1073,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "minimum commission rate cannot be negative", }, { @@ -1079,7 +1088,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "minimum commission rate cannot be greater than 100%", }, { @@ -1095,7 +1103,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: "", }, }, - expErr: true, expErrMsg: "bond denom cannot be blank", }, { @@ -1111,7 +1118,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "max validators must be positive", }, { @@ -1127,7 +1133,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: types.BondStatusBonded, }, }, - expErr: true, expErrMsg: "max entries must be positive", }, { @@ -1143,7 +1148,6 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { BondDenom: "denom", }, }, - expErr: true, expErrMsg: "unbonding time must be positive", }, } @@ -1152,11 +1156,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() + } } }) } From ea1c4572da9d271cb2c051feda2def88cf1c63d7 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 23 Feb 2024 14:50:51 +0100 Subject: [PATCH 2/5] updates --- x/staking/CHANGELOG.md | 2 +- x/staking/keeper/msg_server.go | 2 +- x/staking/keeper/msg_server_test.go | 36 +++++++++++++++++++++++------ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/x/staking/CHANGELOG.md b/x/staking/CHANGELOG.md index 5067befc4137..6712884d8d94 100644 --- a/x/staking/CHANGELOG.md +++ b/x/staking/CHANGELOG.md @@ -27,7 +27,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features -* []() Governance change of `MinCommissionRate` in `MsgUpdateParams`, now updates the minimum commission rate for all validators. +* [#19537](https://github.com/cosmos/cosmos-sdk/pull/19537) Changing `MinCommissionRate` in `MsgUpdateParams` now updates the minimum commission rate for all validators. ### Improvements diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index c69ac161b642..903b38d082ed 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -623,7 +623,7 @@ func (k msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) // 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 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 } diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index 7a6c57f55e8c..c0d9785557e5 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -1027,8 +1027,17 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { ctx, keeper, msgServer := s.ctx, s.stakingKeeper, s.msgServer require := s.Require() - // create validators to test commission rate - // TODO + // 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 @@ -1043,13 +1052,26 @@ func (s *KeeperTestSuite) TestMsgUpdateParams() { Params: types.DefaultParams(), }, postCheck: func() { + // verify that the commission isn't changed vals, err := keeper.GetAllValidators(ctx) require.NoError(err) - require.Len(vals, 6) - for _, val := range vals { - require.True(val.Commission.Rate.GTE(types.DefaultParams().MinCommissionRate)) - require.True(val.Commission.MaxRate.GTE(types.DefaultParams().MinCommissionRate)) - } + 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)) }, }, { From 3ee996f24a692281c5b79e5dbcd2701bbc53a595 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 23 Feb 2024 14:55:12 +0100 Subject: [PATCH 3/5] docs --- x/staking/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x/staking/README.md b/x/staking/README.md index a93a9de13b8d..6cd39d34a3d8 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 wel. +::: + ## Client ### CLI From 9a27340932427744d7b40e21d8078a74a71511c0 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 23 Feb 2024 14:57:20 +0100 Subject: [PATCH 4/5] typos --- x/staking/README.md | 2 +- x/staking/keeper/msg_server.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x/staking/README.md b/x/staking/README.md index 6cd39d34a3d8..713accb96c9f 100644 --- a/x/staking/README.md +++ b/x/staking/README.md @@ -1039,7 +1039,7 @@ The staking module contains the following parameters: | 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 wel. +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 diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index 903b38d082ed..56c3aae906a3 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -599,7 +599,7 @@ func (k msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) return nil, err } - // get previous params params + // get previous staking params previousParams, err := k.Params.Get(ctx) if err != nil { return nil, err From cfe73c762825e32f94d5cecc9d94986c7be12c79 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 23 Feb 2024 17:43:35 +0100 Subject: [PATCH 5/5] `make lint-fix` --- x/staking/keeper/msg_server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index 56c3aae906a3..dbbfb7a6499b 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -610,7 +610,7 @@ func (k msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) return nil, err } - // when min comission rate is updated, we need to update the commission rate of all validators + // 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