From 7cb85c5cdd97f497365959edcde9bf4273a05446 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Sat, 20 Aug 2022 23:11:44 +0200 Subject: [PATCH] refactor: remove weird casting for staking.Keeper in slashing and distribution (#12973) ## Description Both slashing and distribution simulator is using ugly casting from the nice interface type to: ```go stakeKeeper := sk.(*stakingkeeper.Keeper) ``` This creates additional dependency for `staking/keeper` package in other modules, and requires type of a specific type (here reference type, in 0.46 it's even worse - because it uses object type). EXTRA: * removed `stakingkeepr.RandomValidator` function (which btw, was also weird, because it was in the keeper module and required Keeper as an argument) and created generic `testutil.RandSliceElem` function. --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- CHANGELOG.md | 2 ++ go.work.sum | 2 +- testutil/list.go | 12 ++++++++ x/distribution/simulation/operations.go | 18 +++++------- x/distribution/types/expected_keepers.go | 2 ++ x/slashing/simulation/operations.go | 8 ++--- x/slashing/testutil/expected_keepers_mocks.go | 14 +++++++++ x/slashing/types/expected_keepers.go | 1 + x/staking/keeper/test_common.go | 13 --------- x/staking/simulation/operations.go | 29 +++++++++---------- 10 files changed, 57 insertions(+), 44 deletions(-) create mode 100644 testutil/list.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ca2c4e7e39..4e25eca3dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/authz) [#12648](https://github.com/cosmos/cosmos-sdk/pull/12648) Add an allow list, an optional list of addresses allowed to receive bank assests via authz MsgSend grant. * (cli) [#12028](https://github.com/cosmos/cosmos-sdk/pull/12028) Add the `tendermint key-migrate` to perform Tendermint v0.35 DB key migration. * (sdk.Coins) [#12627](https://github.com/cosmos/cosmos-sdk/pull/12627) Make a Denoms method on sdk.Coins. +* (testutil) [#12973](https://github.com/cosmos/cosmos-sdk/pull/12973) Add generic `testutil.RandSliceElem` function which selects a random element from the list. ### Improvements @@ -111,6 +112,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/modules) Remove all LegacyQueries and related code from modules * (store) [#11825](https://github.com/cosmos/cosmos-sdk/pull/11825) Make extension snapshotter interface safer to use, renamed the util function `WriteExtensionItem` to `WriteExtensionPayload`. * (codec) [#12964](https://github.com/cosmos/cosmos-sdk/pull/12964) `ProtoCodec.MarshalInterface` now returns an error when serializing unregistered types and a subsequent `ProtoCodec.UnmarshalInterface` would fail. +* (x/staking) [#12973](https://github.com/cosmos/cosmos-sdk/pull/12973) Removed `stakingkeeper.RandomValidator`. Use `testutil.RandSliceElem(r, sk.GetAllValidators(ctx))` instead. ### CLI Breaking Changes diff --git a/go.work.sum b/go.work.sum index 03868554df..0dc16b745d 100644 --- a/go.work.sum +++ b/go.work.sum @@ -1 +1 @@ -github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 h1:Ovs26xHkKqVztRpIrF/92BcuyuQ/YW4NSIpoGtfXNho= +github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 h1:Ovs26xHkKqVztRpIrF/92BcuyuQ/YW4NSIpoGtfXNho= \ No newline at end of file diff --git a/testutil/list.go b/testutil/list.go new file mode 100644 index 0000000000..ab6e04e2e7 --- /dev/null +++ b/testutil/list.go @@ -0,0 +1,12 @@ +package testutil + +import "math/rand" + +func RandSliceElem[E any](r *rand.Rand, elems []E) (E, bool) { + if len(elems) == 0 { + var e E + return e, false + } + + return elems[r.Intn(len(elems))], true +} diff --git a/x/distribution/simulation/operations.go b/x/distribution/simulation/operations.go index 4a08a1f53a..ce768c6a20 100644 --- a/x/distribution/simulation/operations.go +++ b/x/distribution/simulation/operations.go @@ -8,6 +8,7 @@ import ( "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/testutil" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" sdk "github.com/cosmos/cosmos-sdk/types" simtypes "github.com/cosmos/cosmos-sdk/types/simulation" @@ -15,7 +16,6 @@ import ( "github.com/cosmos/cosmos-sdk/x/distribution/keeper" "github.com/cosmos/cosmos-sdk/x/distribution/types" "github.com/cosmos/cosmos-sdk/x/simulation" - stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" ) // Simulation operation weights constants @@ -58,8 +58,6 @@ func WeightedOperations(appParams simtypes.AppParams, cdc codec.JSONCodec, ak ty }, ) - stakeKeeper := sk.(*stakingkeeper.Keeper) - interfaceRegistry := codectypes.NewInterfaceRegistry() txConfig := tx.NewTxConfig(codec.NewProtoCodec(interfaceRegistry), tx.DefaultSignModes) @@ -70,15 +68,15 @@ func WeightedOperations(appParams simtypes.AppParams, cdc codec.JSONCodec, ak ty ), simulation.NewWeightedOperation( weightMsgWithdrawDelegationReward, - SimulateMsgWithdrawDelegatorReward(txConfig, ak, bk, k, stakeKeeper), + SimulateMsgWithdrawDelegatorReward(txConfig, ak, bk, k, sk), ), simulation.NewWeightedOperation( weightMsgWithdrawValidatorCommission, - SimulateMsgWithdrawValidatorCommission(txConfig, ak, bk, k, stakeKeeper), + SimulateMsgWithdrawValidatorCommission(txConfig, ak, bk, k, sk), ), simulation.NewWeightedOperation( weightMsgFundCommunityPool, - SimulateMsgFundCommunityPool(txConfig, ak, bk, k, stakeKeeper), + SimulateMsgFundCommunityPool(txConfig, ak, bk, k, sk), ), } } @@ -120,7 +118,7 @@ func SimulateMsgSetWithdrawAddress(txConfig client.TxConfig, ak types.AccountKee } // SimulateMsgWithdrawDelegatorReward generates a MsgWithdrawDelegatorReward with random values. -func SimulateMsgWithdrawDelegatorReward(txConfig client.TxConfig, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk *stakingkeeper.Keeper) simtypes.Operation { +func SimulateMsgWithdrawDelegatorReward(txConfig client.TxConfig, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk types.StakingKeeper) simtypes.Operation { return func( r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, ) (simtypes.OperationMsg, []simtypes.FutureOperation, error) { @@ -162,11 +160,11 @@ func SimulateMsgWithdrawDelegatorReward(txConfig client.TxConfig, ak types.Accou } // SimulateMsgWithdrawValidatorCommission generates a MsgWithdrawValidatorCommission with random values. -func SimulateMsgWithdrawValidatorCommission(txConfig client.TxConfig, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk *stakingkeeper.Keeper) simtypes.Operation { +func SimulateMsgWithdrawValidatorCommission(txConfig client.TxConfig, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk types.StakingKeeper) simtypes.Operation { return func( r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, ) (simtypes.OperationMsg, []simtypes.FutureOperation, error) { - validator, ok := stakingkeeper.RandomValidator(r, sk, ctx) + validator, ok := testutil.RandSliceElem(r, sk.GetAllValidators(ctx)) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWithdrawValidatorCommission, "random validator is not ok"), nil, nil } @@ -207,7 +205,7 @@ func SimulateMsgWithdrawValidatorCommission(txConfig client.TxConfig, ak types.A // SimulateMsgFundCommunityPool simulates MsgFundCommunityPool execution where // a random account sends a random amount of its funds to the community pool. -func SimulateMsgFundCommunityPool(txConfig client.TxConfig, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk *stakingkeeper.Keeper) simtypes.Operation { +func SimulateMsgFundCommunityPool(txConfig client.TxConfig, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk types.StakingKeeper) simtypes.Operation { return func( r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, ) (simtypes.OperationMsg, []simtypes.FutureOperation, error) { diff --git a/x/distribution/types/expected_keepers.go b/x/distribution/types/expected_keepers.go index 82eee3eabe..94cea333ab 100644 --- a/x/distribution/types/expected_keepers.go +++ b/x/distribution/types/expected_keepers.go @@ -47,6 +47,8 @@ type StakingKeeper interface { fn func(index int64, delegation stakingtypes.DelegationI) (stop bool)) GetAllSDKDelegations(ctx sdk.Context) []stakingtypes.Delegation + GetAllValidators(ctx sdk.Context) (validators []stakingtypes.Validator) + GetAllDelegatorDelegations(ctx sdk.Context, delegator sdk.AccAddress) []stakingtypes.Delegation } // StakingHooks event hooks for staking validator object (noalias) diff --git a/x/slashing/simulation/operations.go b/x/slashing/simulation/operations.go index 26228fa882..b65844a900 100644 --- a/x/slashing/simulation/operations.go +++ b/x/slashing/simulation/operations.go @@ -7,6 +7,7 @@ import ( "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/testutil" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" sdk "github.com/cosmos/cosmos-sdk/types" simtypes "github.com/cosmos/cosmos-sdk/types/simulation" @@ -14,7 +15,6 @@ import ( "github.com/cosmos/cosmos-sdk/x/simulation" "github.com/cosmos/cosmos-sdk/x/slashing/keeper" "github.com/cosmos/cosmos-sdk/x/slashing/types" - stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" ) // Simulation operation weights constants @@ -40,18 +40,18 @@ func WeightedOperations( return simulation.WeightedOperations{ simulation.NewWeightedOperation( weightMsgUnjail, - SimulateMsgUnjail(codec.NewProtoCodec(interfaceRegistry), ak, bk, k, sk.(*stakingkeeper.Keeper)), + SimulateMsgUnjail(codec.NewProtoCodec(interfaceRegistry), ak, bk, k, sk), ), } } // SimulateMsgUnjail generates a MsgUnjail with random values -func SimulateMsgUnjail(cdc *codec.ProtoCodec, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk *stakingkeeper.Keeper) simtypes.Operation { +func SimulateMsgUnjail(cdc *codec.ProtoCodec, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, sk types.StakingKeeper) simtypes.Operation { return func( r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, ) (simtypes.OperationMsg, []simtypes.FutureOperation, error) { - validator, ok := stakingkeeper.RandomValidator(r, sk, ctx) + validator, ok := testutil.RandSliceElem(r, sk.GetAllValidators(ctx)) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUnjail, "validator is not ok"), nil, nil // skip } diff --git a/x/slashing/testutil/expected_keepers_mocks.go b/x/slashing/testutil/expected_keepers_mocks.go index 05eec69741..d8d729920c 100644 --- a/x/slashing/testutil/expected_keepers_mocks.go +++ b/x/slashing/testutil/expected_keepers_mocks.go @@ -267,6 +267,20 @@ func (mr *MockStakingKeeperMockRecorder) Delegation(arg0, arg1, arg2 interface{} return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delegation", reflect.TypeOf((*MockStakingKeeper)(nil).Delegation), arg0, arg1, arg2) } +// GetAllValidators mocks base method. +func (m *MockStakingKeeper) GetAllValidators(ctx types.Context) []types2.Validator { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetAllValidators", ctx) + ret0, _ := ret[0].([]types2.Validator) + return ret0 +} + +// GetAllValidators indicates an expected call of GetAllValidators. +func (mr *MockStakingKeeperMockRecorder) GetAllValidators(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllValidators", reflect.TypeOf((*MockStakingKeeper)(nil).GetAllValidators), ctx) +} + // IterateValidators mocks base method. func (m *MockStakingKeeper) IterateValidators(arg0 types.Context, arg1 func(int64, types2.ValidatorI) bool) { m.ctrl.T.Helper() diff --git a/x/slashing/types/expected_keepers.go b/x/slashing/types/expected_keepers.go index 9e15820dc4..c4cdfec1dc 100644 --- a/x/slashing/types/expected_keepers.go +++ b/x/slashing/types/expected_keepers.go @@ -50,6 +50,7 @@ type StakingKeeper interface { // Delegation allows for getting a particular delegation for a given validator // and delegator outside the scope of the staking module. Delegation(sdk.Context, sdk.AccAddress, sdk.ValAddress) stakingtypes.DelegationI + GetAllValidators(ctx sdk.Context) (validators []stakingtypes.Validator) // MaxValidators returns the maximum amount of bonded validators MaxValidators(sdk.Context) uint32 diff --git a/x/staking/keeper/test_common.go b/x/staking/keeper/test_common.go index edc9744976..d31ae06ba7 100644 --- a/x/staking/keeper/test_common.go +++ b/x/staking/keeper/test_common.go @@ -2,7 +2,6 @@ package keeper // noalias import ( "bytes" - "math/rand" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/staking/types" @@ -55,15 +54,3 @@ func TestingUpdateValidator(keeper *Keeper, ctx sdk.Context, validator types.Val return validator } - -// RandomValidator returns a random validator given access to the keeper and ctx -func RandomValidator(r *rand.Rand, keeper *Keeper, ctx sdk.Context) (val types.Validator, ok bool) { - vals := keeper.GetAllValidators(ctx) - if len(vals) == 0 { - return types.Validator{}, false - } - - i := r.Intn(len(vals)) - - return vals[i], true -} diff --git a/x/staking/simulation/operations.go b/x/staking/simulation/operations.go index c4dbd7ecd6..612fe34bf1 100644 --- a/x/staking/simulation/operations.go +++ b/x/staking/simulation/operations.go @@ -8,6 +8,7 @@ import ( "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec" simappparams "github.com/cosmos/cosmos-sdk/simapp/params" + "github.com/cosmos/cosmos-sdk/testutil" sdk "github.com/cosmos/cosmos-sdk/types" simtypes "github.com/cosmos/cosmos-sdk/types/simulation" "github.com/cosmos/cosmos-sdk/x/simulation" @@ -192,13 +193,12 @@ func SimulateMsgEditValidator(ak types.AccountKeeper, bk types.BankKeeper, k *ke return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgEditValidator, "number of validators equal zero"), nil, nil } - val, ok := keeper.RandomValidator(r, k, ctx) + val, ok := testutil.RandSliceElem(r, k.GetAllValidators(ctx)) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgEditValidator, "unable to pick a validator"), nil, nil } address := val.GetOperator() - newCommissionRate := simtypes.RandomDecAmount(r, val.Commission.MaxRate) if err := val.Commission.ValidateNewRate(newCommissionRate, ctx.BlockHeader().Time); err != nil { @@ -255,7 +255,7 @@ func SimulateMsgDelegate(ak types.AccountKeeper, bk types.BankKeeper, k *keeper. } simAccount, _ := simtypes.RandomAcc(r, accs) - val, ok := keeper.RandomValidator(r, k, ctx) + val, ok := testutil.RandSliceElem(r, k.GetAllValidators(ctx)) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgDelegate, "unable to pick a validator"), nil, nil } @@ -313,14 +313,13 @@ func SimulateMsgUndelegate(ak types.AccountKeeper, bk types.BankKeeper, k *keepe return func( r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, ) (simtypes.OperationMsg, []simtypes.FutureOperation, error) { - // get random validator - validator, ok := keeper.RandomValidator(r, k, ctx) + val, ok := testutil.RandSliceElem(r, k.GetAllValidators(ctx)) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUndelegate, "validator is not ok"), nil, nil } - valAddr := validator.GetOperator() - delegations := k.GetValidatorDelegations(ctx, validator.GetOperator()) + valAddr := val.GetOperator() + delegations := k.GetValidatorDelegations(ctx, val.GetOperator()) if delegations == nil { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUndelegate, "keeper does have any delegation entries"), nil, nil } @@ -333,7 +332,7 @@ func SimulateMsgUndelegate(ak types.AccountKeeper, bk types.BankKeeper, k *keepe return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUndelegate, "keeper does have a max unbonding delegation entries"), nil, nil } - totalBond := validator.TokensFromShares(delegation.GetShares()).TruncateInt() + totalBond := val.TokensFromShares(delegation.GetShares()).TruncateInt() if !totalBond.IsPositive() { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUndelegate, "total bond is negative"), nil, nil } @@ -395,19 +394,17 @@ func SimulateMsgCancelUnbondingDelegate(ak types.AccountKeeper, bk types.BankKee if len(k.GetAllValidators(ctx)) == 0 { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgDelegate, "number of validators equal zero"), nil, nil } - // get random account simAccount, _ := simtypes.RandomAcc(r, accs) - // get random validator - validator, ok := keeper.RandomValidator(r, k, ctx) + val, ok := testutil.RandSliceElem(r, k.GetAllValidators(ctx)) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgCancelUnbondingDelegation, "validator is not ok"), nil, nil } - if validator.IsJailed() || validator.InvalidExRate() { + if val.IsJailed() || val.InvalidExRate() { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgCancelUnbondingDelegation, "validator is jailed"), nil, nil } - valAddr := validator.GetOperator() + valAddr := val.GetOperator() unbondingDelegation, found := k.GetUnbondingDelegation(ctx, simAccount.Address, valAddr) if !found { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgCancelUnbondingDelegation, "account does have any unbonding delegation"), nil, nil @@ -474,8 +471,8 @@ func SimulateMsgBeginRedelegate(ak types.AccountKeeper, bk types.BankKeeper, k * return func( r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, accs []simtypes.Account, chainID string, ) (simtypes.OperationMsg, []simtypes.FutureOperation, error) { - // get random source validator - srcVal, ok := keeper.RandomValidator(r, k, ctx) + allVals := k.GetAllValidators(ctx) + srcVal, ok := testutil.RandSliceElem(r, allVals) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgBeginRedelegate, "unable to pick validator"), nil, nil } @@ -495,7 +492,7 @@ func SimulateMsgBeginRedelegate(ak types.AccountKeeper, bk types.BankKeeper, k * } // get random destination validator - destVal, ok := keeper.RandomValidator(r, k, ctx) + destVal, ok := testutil.RandSliceElem(r, allVals) if !ok { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgBeginRedelegate, "unable to pick validator"), nil, nil }