From f2756c3feae1bcd84f2d995ce038ec5a30aa9791 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Tue, 30 Apr 2024 12:01:33 +0200 Subject: [PATCH 1/3] Validate consumer commission rate --- testutil/keeper/mocks.go | 14 ++++++++++++++ x/ccv/provider/keeper/distribution.go | 25 +++++++++++++++++++++++++ x/ccv/types/expected_keepers.go | 1 + 3 files changed, 40 insertions(+) diff --git a/testutil/keeper/mocks.go b/testutil/keeper/mocks.go index 7abadd63d5..24b7d3f4cb 100644 --- a/testutil/keeper/mocks.go +++ b/testutil/keeper/mocks.go @@ -329,6 +329,20 @@ func (mr *MockStakingKeeperMockRecorder) MaxValidators(ctx interface{}) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MaxValidators", reflect.TypeOf((*MockStakingKeeper)(nil).MaxValidators), ctx) } +// MinCommissionRate mocks base method. +func (m *MockStakingKeeper) MinCommissionRate(ctx types0.Context) math.LegacyDec { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "MinCommissionRate", ctx) + ret0, _ := ret[0].(math.LegacyDec) + return ret0 +} + +// MinCommissionRate indicates an expected call of MinCommissionRate. +func (mr *MockStakingKeeperMockRecorder) MinCommissionRate(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MinCommissionRate", reflect.TypeOf((*MockStakingKeeper)(nil).MinCommissionRate), ctx) +} + // PowerReduction mocks base method. func (m *MockStakingKeeper) PowerReduction(ctx types0.Context) math.Int { m.ctrl.T.Helper() diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 2bf0ba2a24..b60fd9831a 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -279,6 +279,31 @@ func (k Keeper) HandleSetConsumerCommissionRate(ctx sdk.Context, chainID string, types.ErrUnknownConsumerChainId, "unknown consumer chain, with id: %s", chainID) } + + // validate the commission rate + + // check it is not more than 100% + if commissionRate.GT(sdk.OneDec()) { + return errorsmod.Wrapf( + stakingtypes.ErrCommissionHuge, + "commission rate %s is greater than 100%%", commissionRate.String()) + } + + // check it is not less than 0% + if commissionRate.LT(sdk.ZeroDec()) { + return errorsmod.Wrapf( + stakingtypes.ErrCommissionNegative, + "commission rate %s is less than 0%%", commissionRate.String()) + } + + // validate agains tthe minRate + minRate := k.stakingKeeper.MinCommissionRate(ctx) + if commissionRate.LT(minRate) { + return errorsmod.Wrapf( + stakingtypes.ErrCommissionLTMinRate, + "commission rate cannot be less than %s", minRate, + ) + } // set per-consumer chain commission rate for the validator address k.SetConsumerCommissionRate( ctx, diff --git a/x/ccv/types/expected_keepers.go b/x/ccv/types/expected_keepers.go index 7b77ebd095..427281f292 100644 --- a/x/ccv/types/expected_keepers.go +++ b/x/ccv/types/expected_keepers.go @@ -58,6 +58,7 @@ type StakingKeeper interface { GetRedelegationsFromSrcValidator(ctx sdk.Context, valAddr sdk.ValAddress) (reds []stakingtypes.Redelegation) GetUnbondingType(ctx sdk.Context, id uint64) (unbondingType stakingtypes.UnbondingType, found bool) BlockValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate + MinCommissionRate(ctx sdk.Context) math.LegacyDec } // SlashingKeeper defines the contract expected to perform ccv slashing From f72b4f3586531468c24fb7c1e22bada272ee2729 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Tue, 30 Apr 2024 12:18:23 +0200 Subject: [PATCH 2/3] Add test for commission rates --- x/ccv/provider/keeper/distribution.go | 2 +- .../keeper/partial_set_security_test.go | 20 ++++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index b60fd9831a..e6ef6b2cbb 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -296,7 +296,7 @@ func (k Keeper) HandleSetConsumerCommissionRate(ctx sdk.Context, chainID string, "commission rate %s is less than 0%%", commissionRate.String()) } - // validate agains tthe minRate + // validate agains the minRate minRate := k.stakingKeeper.MinCommissionRate(ctx) if commissionRate.LT(minRate) { return errorsmod.Wrapf( diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index b18acb56ce..4a320bc1d1 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -151,7 +151,7 @@ func TestHandleOptOutFromTopNChain(t *testing.T) { } func TestHandleSetConsumerCommissionRate(t *testing.T) { - providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) + providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) defer ctrl.Finish() providerAddr := types.NewProviderConsAddress([]byte("providerAddr")) @@ -167,12 +167,30 @@ func TestHandleSetConsumerCommissionRate(t *testing.T) { _, found := providerKeeper.GetConsumerCommissionRate(ctx, chainID, providerAddr) require.False(t, found) + mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(sdk.ZeroDec()).Times(1) require.NoError(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, sdk.OneDec())) // check that the commission rate is now set cr, found := providerKeeper.GetConsumerCommissionRate(ctx, chainID, providerAddr) require.Equal(t, sdk.OneDec(), cr) require.True(t, found) + + // check some failure scenarios + require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, sdk.NewDec(2)), "commission rate should be rejected (above 100), but is not") + + require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, sdk.NewDec(-1)), "commission rate should be rejected (below 0), but is not") + + commissionRate := sdk.NewDec(1).Quo(sdk.NewDec(2)) + mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(commissionRate).AnyTimes() + + require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, sdk.ZeroDec()), "commission rate should be rejected (below min), but is not") + + // set a valid commission equal to the minimum + require.NoError(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, commissionRate)) + // check that the rate was set + cr, found = providerKeeper.GetConsumerCommissionRate(ctx, chainID, providerAddr) + require.Equal(t, commissionRate, cr) + require.True(t, found) } func TestOptInTopNValidators(t *testing.T) { From 2f3467edf0cd5ee1c9605fbc34d2f69793b6070f Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Tue, 30 Apr 2024 13:11:37 +0200 Subject: [PATCH 3/3] Remove static minimum commission rate validation from Set --- x/ccv/provider/keeper/distribution.go | 18 +----------------- .../keeper/partial_set_security_test.go | 15 +++++++++------ 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index e6ef6b2cbb..921e290843 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -280,23 +280,7 @@ func (k Keeper) HandleSetConsumerCommissionRate(ctx sdk.Context, chainID string, "unknown consumer chain, with id: %s", chainID) } - // validate the commission rate - - // check it is not more than 100% - if commissionRate.GT(sdk.OneDec()) { - return errorsmod.Wrapf( - stakingtypes.ErrCommissionHuge, - "commission rate %s is greater than 100%%", commissionRate.String()) - } - - // check it is not less than 0% - if commissionRate.LT(sdk.ZeroDec()) { - return errorsmod.Wrapf( - stakingtypes.ErrCommissionNegative, - "commission rate %s is less than 0%%", commissionRate.String()) - } - - // validate agains the minRate + // validate against the minimum commission rate minRate := k.stakingKeeper.MinCommissionRate(ctx) if commissionRate.LT(minRate) { return errorsmod.Wrapf( diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index 4a320bc1d1..c4da4d1233 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -175,15 +175,18 @@ func TestHandleSetConsumerCommissionRate(t *testing.T) { require.Equal(t, sdk.OneDec(), cr) require.True(t, found) - // check some failure scenarios - require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, sdk.NewDec(2)), "commission rate should be rejected (above 100), but is not") - - require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, sdk.NewDec(-1)), "commission rate should be rejected (below 0), but is not") - + // set minimum rate of 1/2 commissionRate := sdk.NewDec(1).Quo(sdk.NewDec(2)) mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(commissionRate).AnyTimes() - require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, sdk.ZeroDec()), "commission rate should be rejected (below min), but is not") + // try to set a rate slightly below the minimum + require.Error(t, providerKeeper.HandleSetConsumerCommissionRate( + ctx, + chainID, + providerAddr, + commissionRate.Sub(sdk.NewDec(1).Quo(sdk.NewDec(100)))), // 0.5 - 0.01 + "commission rate should be rejected (below min), but is not", + ) // set a valid commission equal to the minimum require.NoError(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, commissionRate))