From ed0e3f7891b796f652415c87afeabcf12c8c3202 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Francisco=20L=C3=B3pez?= Date: Sat, 17 Aug 2024 21:49:32 +0200 Subject: [PATCH] refactor(x/distribution): audit QA (#21316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Randy Grok <98407738+randygrok@users.noreply.github.com> Co-authored-by: marbar3778 Co-authored-by: Cosmos SDK <113218068+github-prbot@users.noreply.github.com> Co-authored-by: github-merge-queue <118344674+github-merge-queue@users.noreply.github.com> Co-authored-by: Julián Toledano Co-authored-by: Julien Robert Co-authored-by: Aaron Craelius (cherry picked from commit ee04cee0cafbf806dab9fcc21f00270f21fb6aea) --- x/distribution/README.md | 8 +- x/distribution/keeper/allocation_test.go | 128 +++++++++++++++++++++++ x/distribution/keeper/keeper.go | 2 +- x/distribution/keeper/validator.go | 12 ++- x/distribution/migrations/v4/migrate.go | 16 +-- 5 files changed, 149 insertions(+), 17 deletions(-) diff --git a/x/distribution/README.md b/x/distribution/README.md index 03e1e5baff21..d49b309f50fe 100644 --- a/x/distribution/README.md +++ b/x/distribution/README.md @@ -186,7 +186,7 @@ it can be updated with governance or the address with authority. * Params: `0x09 | ProtocolBuffer(Params)` ```protobuf reference -https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/distribution.proto#L12-L42 +https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/distribution.proto#L12-L44 ``` ## Begin Block @@ -272,7 +272,7 @@ The withdraw address cannot be any of the module accounts. These accounts are bl Response: ```protobuf reference -https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/tx.proto#L49-L60 +https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/tx.proto#L62-L73 ``` ```go @@ -324,7 +324,7 @@ The final calculated stake is equivalent to the actual staked coins in the deleg Response: ```protobuf reference -https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/tx.proto#L66-L77 +https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/tx.proto#L79-L90 ``` ### WithdrawValidatorCommission @@ -368,7 +368,7 @@ func (k Keeper) initializeDelegation(ctx context.Context, val sdk.ValAddress, de Distribution module params can be updated through `MsgUpdateParams`, which can be done using governance proposal and the signer will always be gov module account address. ```protobuf reference -https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/tx.proto#L133-L147 +https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/tx.proto#L159:L172 ``` The message handling can fail if: diff --git a/x/distribution/keeper/allocation_test.go b/x/distribution/keeper/allocation_test.go index f319ad456cbe..21d788c97843 100644 --- a/x/distribution/keeper/allocation_test.go +++ b/x/distribution/keeper/allocation_test.go @@ -375,3 +375,131 @@ func TestAllocateTokensTruncation(t *testing.T) { require.NoError(t, err) require.True(t, val2OutstandingRewards.Rewards.IsValid()) } + +func TestAllocateTokensToValidatorWithoutCommission(t *testing.T) { + ctrl := gomock.NewController(t) + key := storetypes.NewKVStoreKey(disttypes.StoreKey) + testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) + cdcOpts := codectestutil.CodecOptions{} + encCfg := moduletestutil.MakeTestEncodingConfig(cdcOpts, distribution.AppModule{}) + ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()}) + + bankKeeper := distrtestutil.NewMockBankKeeper(ctrl) + stakingKeeper := distrtestutil.NewMockStakingKeeper(ctrl) + accountKeeper := distrtestutil.NewMockAccountKeeper(ctrl) + + env := runtime.NewEnvironment(runtime.NewKVStoreService(key), coretesting.NewNopLogger()) + + valCodec := address.NewBech32Codec("cosmosvaloper") + + accountKeeper.EXPECT().GetModuleAddress("distribution").Return(distrAcc.GetAddress()) + stakingKeeper.EXPECT().ValidatorAddressCodec().Return(valCodec).AnyTimes() + + authorityAddr, err := cdcOpts.GetAddressCodec().BytesToString(authtypes.NewModuleAddress("gov")) + require.NoError(t, err) + + distrKeeper := keeper.NewKeeper( + encCfg.Codec, + env, + accountKeeper, + bankKeeper, + stakingKeeper, + testCometService, + "fee_collector", + authorityAddr, + ) + + // create validator with 0% commission + operatorAddr, err := stakingKeeper.ValidatorAddressCodec().BytesToString(valConsPk0.Address()) + require.NoError(t, err) + val, err := distrtestutil.CreateValidator(valConsPk0, operatorAddr, math.NewInt(100)) + require.NoError(t, err) + val.Commission = stakingtypes.NewCommission(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0)) + stakingKeeper.EXPECT().ValidatorByConsAddr(gomock.Any(), sdk.GetConsAddress(valConsPk0)).Return(val, nil).AnyTimes() + + // allocate tokens + tokens := sdk.DecCoins{ + {Denom: sdk.DefaultBondDenom, Amount: math.LegacyNewDec(10)}, + } + require.NoError(t, distrKeeper.AllocateTokensToValidator(ctx, val, tokens)) + + // check commission + var expectedCommission sdk.DecCoins = nil + + valBz, err := valCodec.StringToBytes(val.GetOperator()) + require.NoError(t, err) + + valCommission, err := distrKeeper.ValidatorsAccumulatedCommission.Get(ctx, valBz) + require.NoError(t, err) + require.Equal(t, expectedCommission, valCommission.Commission) + + // check current rewards + expectedRewards := tokens + + currentRewards, err := distrKeeper.ValidatorCurrentRewards.Get(ctx, valBz) + require.NoError(t, err) + require.Equal(t, expectedRewards, currentRewards.Rewards) +} + +func TestAllocateTokensWithZeroTokens(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + key := storetypes.NewKVStoreKey(disttypes.StoreKey) + testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) + cdcOpts := codectestutil.CodecOptions{} + encCfg := moduletestutil.MakeTestEncodingConfig(cdcOpts, distribution.AppModule{}) + ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()}) + + bankKeeper := distrtestutil.NewMockBankKeeper(ctrl) + stakingKeeper := distrtestutil.NewMockStakingKeeper(ctrl) + accountKeeper := distrtestutil.NewMockAccountKeeper(ctrl) + + env := runtime.NewEnvironment(runtime.NewKVStoreService(key), coretesting.NewNopLogger()) + + valCodec := address.NewBech32Codec("cosmosvaloper") + + accountKeeper.EXPECT().GetModuleAddress("distribution").Return(distrAcc.GetAddress()) + stakingKeeper.EXPECT().ValidatorAddressCodec().Return(valCodec).AnyTimes() + + authorityAddr, err := cdcOpts.GetAddressCodec().BytesToString(authtypes.NewModuleAddress("gov")) + require.NoError(t, err) + + distrKeeper := keeper.NewKeeper( + encCfg.Codec, + env, + accountKeeper, + bankKeeper, + stakingKeeper, + testCometService, + "fee_collector", + authorityAddr, + ) + + // create validator with 50% commission + operatorAddr, err := stakingKeeper.ValidatorAddressCodec().BytesToString(valConsPk0.Address()) + require.NoError(t, err) + val, err := distrtestutil.CreateValidator(valConsPk0, operatorAddr, math.NewInt(100)) + require.NoError(t, err) + val.Commission = stakingtypes.NewCommission(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)) + stakingKeeper.EXPECT().ValidatorByConsAddr(gomock.Any(), sdk.GetConsAddress(valConsPk0)).Return(val, nil).AnyTimes() + + // allocate zero tokens + tokens := sdk.DecCoins{} + require.NoError(t, distrKeeper.AllocateTokensToValidator(ctx, val, tokens)) + + // check commission + var expected sdk.DecCoins = nil + + valBz, err := valCodec.StringToBytes(val.GetOperator()) + require.NoError(t, err) + + valCommission, err := distrKeeper.ValidatorsAccumulatedCommission.Get(ctx, valBz) + require.NoError(t, err) + require.Equal(t, expected, valCommission.Commission) + + // check current rewards + currentRewards, err := distrKeeper.ValidatorCurrentRewards.Get(ctx, valBz) + require.NoError(t, err) + require.Equal(t, expected, currentRewards.Rewards) +} diff --git a/x/distribution/keeper/keeper.go b/x/distribution/keeper/keeper.go index 0c3238353eba..0ded047e93d6 100644 --- a/x/distribution/keeper/keeper.go +++ b/x/distribution/keeper/keeper.go @@ -46,7 +46,7 @@ type Keeper struct { DelegatorStartingInfo collections.Map[collections.Pair[sdk.ValAddress, sdk.AccAddress], types.DelegatorStartingInfo] // ValidatorsAccumulatedCommission key: valAddr | value: ValidatorAccumulatedCommission ValidatorsAccumulatedCommission collections.Map[sdk.ValAddress, types.ValidatorAccumulatedCommission] - // ValidatorOutstandingRewards key: valAddr | value: ValidatorOustandingRewards + // ValidatorOutstandingRewards key: valAddr | value: ValidatorOutstandingRewards ValidatorOutstandingRewards collections.Map[sdk.ValAddress, types.ValidatorOutstandingRewards] // ValidatorHistoricalRewards key: valAddr+period | value: ValidatorHistoricalRewards ValidatorHistoricalRewards collections.Map[collections.Pair[sdk.ValAddress, uint64], types.ValidatorHistoricalRewards] diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index 812fbbe5086b..0c9246e0136a 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -13,6 +13,10 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" ) +const ( + maxReferenceCount = 2 +) + // initialize rewards for a new validator func (k Keeper) initializeValidator(ctx context.Context, val sdk.ValidatorI) error { valBz, err := k.stakingKeeper.ValidatorAddressCodec().StringToBytes(val.GetOperator()) @@ -126,8 +130,8 @@ func (k Keeper) incrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr } historical.ReferenceCount++ - if historical.ReferenceCount > 2 { - panic("reference count should never exceed 2") + if historical.ReferenceCount > maxReferenceCount { + return fmt.Errorf("reference count should never exceed %d", maxReferenceCount) } return k.ValidatorHistoricalRewards.Set(ctx, collections.Join(valAddr, period), historical) } @@ -140,7 +144,7 @@ func (k Keeper) decrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr } if historical.ReferenceCount == 0 { - panic("cannot set negative reference count") + return fmt.Errorf("cannot set negative reference count") } historical.ReferenceCount-- if historical.ReferenceCount == 0 { @@ -152,7 +156,7 @@ func (k Keeper) decrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr func (k Keeper) updateValidatorSlashFraction(ctx context.Context, valAddr sdk.ValAddress, fraction math.LegacyDec) error { if fraction.GT(math.LegacyOneDec()) || fraction.IsNegative() { - panic(fmt.Sprintf("fraction must be >=0 and <=1, current fraction: %v", fraction)) + return fmt.Errorf("fraction must be >=0 and <=1, current fraction: %v", fraction) } headerinfo := k.HeaderService.HeaderInfo(ctx) diff --git a/x/distribution/migrations/v4/migrate.go b/x/distribution/migrations/v4/migrate.go index 783dc91218c7..1ecfd4a2d8f6 100644 --- a/x/distribution/migrations/v4/migrate.go +++ b/x/distribution/migrations/v4/migrate.go @@ -16,16 +16,16 @@ import ( var OldProposerKey = []byte{0x01} // MigrateStore removes the last proposer from store. -func MigrateStore(ctx context.Context, env appmodule.Environment, cdc codec.BinaryCodec) error { - store := env.KVStoreService.OpenKVStore(ctx) - return store.Delete(OldProposerKey) +func MigrateStore(ctx context.Context, env appmodule.Environment, _ codec.BinaryCodec) error { + kvStore := env.KVStoreService.OpenKVStore(ctx) + return kvStore.Delete(OldProposerKey) } // GetPreviousProposerConsAddr returns the proposer consensus address for the // current block. func GetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStoreService, cdc codec.BinaryCodec) (sdk.ConsAddress, error) { - store := storeService.OpenKVStore(ctx) - bz, err := store.Get(OldProposerKey) + kvStore := storeService.OpenKVStore(ctx) + bz, err := kvStore.Get(OldProposerKey) if err != nil { return nil, err } @@ -43,9 +43,9 @@ func GetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStore return addrValue.GetValue(), nil } -// set the proposer public key for this block +// SetPreviousProposerConsAddr set the proposer public key for this block. func SetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStoreService, cdc codec.BinaryCodec, consAddr sdk.ConsAddress) error { - store := storeService.OpenKVStore(ctx) + kvStore := storeService.OpenKVStore(ctx) bz := cdc.MustMarshal(&gogotypes.BytesValue{Value: consAddr}) - return store.Set(OldProposerKey, bz) + return kvStore.Set(OldProposerKey, bz) }