refactor(x/distribution)!: Use KVStoreService, context.Context and return errors instead of panic (#15948)

Author: facundomedica

CHANGELOG.md

@@ -117,6 +117,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking Changes
 
+* (x/distribution) [#15948](https://github.com/cosmos/cosmos-sdk/issues/15948) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context`. Keeper methods also now return an `error`.
 * (x/bank) [#15891](https://github.com/cosmos/cosmos-sdk/issues/15891) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context`. Also `FundAccount` and `FundModuleAccount` from the `testutil` package accept a `context.Context` instead of a `sdk.Context`, and it's position was moved to the first place.
 * (x/bank) [#15818](https://github.com/cosmos/cosmos-sdk/issues/15818) `BaseViewKeeper`'s `Logger` method now doesn't require a context. `NewBaseKeeper`, `NewBaseSendKeeper` and `NewBaseViewKeeper` now also require a `log.Logger` to be passed in.
 * (client) [#15597](https://github.com/cosmos/cosmos-sdk/pull/15597) `RegisterNodeService` now requires a config parameter.

UPGRADING.md

@@ -73,6 +73,7 @@ The following modules `NewKeeper` function now take a `KVStoreService` instead o
 * `x/auth`
 * `x/bank`
 * `x/consensus`
+* `x/distribution`
 * `x/feegrant`
 * `x/nft`
 
@@ -91,6 +92,11 @@ The following modules `NewKeeper` function now also take a `log.Logger`:
 
 * `x/bank`
 
+The following modules' `Keeper` methods now take in a `context.Context` instead of `sdk.Context`. Any module that has an interfaces for them (like "expected keepers") will need to update and re-generate mocks if needed:
+
+* `x/bank`
+* `x/distribution`
+
 
 ### depinject
 
@@ -136,8 +142,6 @@ It is now recommended to validate message directly in the message server. When t
 
 #### `x/auth`
 
-Methods in the `AccountKeeper` now use `context.Context` instead of `sdk.Context`. Any module that has an interface for it will need to update and re-generate mocks if needed.
-
 For ante handler construction via `ante.NewAnteHandler`, the field `ante.HandlerOptions.SignModeHandler` has been updated to `x/tx/signing/HandlerMap` from `x/auth/signing/SignModeHandler`.  Callers typically fetch this value from `client.TxConfig.SignModeHandler()` (which is also changed) so this change should be transparent to most users.
 
 #### `x/capability`

server/grpc/gogoreflection/fix_registration.go

@@ -42,12 +42,12 @@ func getExtension(extID int32, m proto.Message) *gogoproto.ExtensionDesc {
 	for id, desc := range proto.RegisteredExtensions(m) { //nolint:staticcheck // keep for backward compatibility
 		if id == extID {
 			return &gogoproto.ExtensionDesc{
-				ExtendedType:  desc.ExtendedType,
-				ExtensionType: desc.ExtensionType,
-				Field:         desc.Field,
-				Name:          desc.Name,
-				Tag:           desc.Tag,
-				Filename:      desc.Filename,
+				ExtendedType:  desc.ExtendedType,  //nolint:staticcheck // keep for backward compatibility
+				ExtensionType: desc.ExtensionType, //nolint:staticcheck // keep for backward compatibility
+				Field:         desc.Field,         //nolint:staticcheck // keep for backward compatibility
+				Name:          desc.Name,          //nolint:staticcheck // keep for backward compatibility
+				Tag:           desc.Tag,           //nolint:staticcheck // keep for backward compatibility
+				Filename:      desc.Filename,      //nolint:staticcheck // keep for backward compatibility
 			}
 		}
 	}

simapp/app.go

@@ -40,6 +40,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/client/grpc/cmtservice"
 	nodeservice "github.com/cosmos/cosmos-sdk/client/grpc/node"
 	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/codec/address"
 	"github.com/cosmos/cosmos-sdk/codec/types"
 	"github.com/cosmos/cosmos-sdk/runtime"
 	runtimeservices "github.com/cosmos/cosmos-sdk/runtime/services"
@@ -298,7 +299,7 @@ func NewSimApp(
 	)
 	app.MintKeeper = mintkeeper.NewKeeper(appCodec, keys[minttypes.StoreKey], app.StakingKeeper, app.AccountKeeper, app.BankKeeper, authtypes.FeeCollectorName, authtypes.NewModuleAddress(govtypes.ModuleName).String())
 
-	app.DistrKeeper = distrkeeper.NewKeeper(appCodec, keys[distrtypes.StoreKey], app.AccountKeeper, app.BankKeeper, app.StakingKeeper, authtypes.FeeCollectorName, authtypes.NewModuleAddress(govtypes.ModuleName).String())
+	app.DistrKeeper = distrkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[distrtypes.StoreKey]), app.AccountKeeper, app.BankKeeper, app.StakingKeeper, authtypes.FeeCollectorName, authtypes.NewModuleAddress(govtypes.ModuleName).String())
 
 	app.SlashingKeeper = slashingkeeper.NewKeeper(
 		appCodec, legacyAmino, keys[slashingtypes.StoreKey], app.StakingKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(),
@@ -624,7 +625,7 @@ func (app *SimApp) AutoCliOpts() autocli.AppOptions {
 		}
 	}
 
-	return autocli.AppOptions{Modules: modules}
+	return autocli.AppOptions{Modules: modules, AddressCodec: address.NewBech32Codec(sdk.Bech32MainPrefix)}
 }
 
 // DefaultGenesis returns a default genesis from the registered AppModuleBasic's.

simapp/export.go

@@ -107,10 +107,18 @@ func (app *SimApp) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []
 	// reinitialize all validators
 	app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) {
 		// donate any unwithdrawn outstanding reward fraction tokens to the community pool
-		scraps := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())
-		feePool := app.DistrKeeper.GetFeePool(ctx)
+		scraps, err := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())
+		if err != nil {
+			panic(err)
+		}
+		feePool, err := app.DistrKeeper.GetFeePool(ctx)
+		if err != nil {
+			panic(err)
+		}
 		feePool.CommunityPool = feePool.CommunityPool.Add(scraps...)
-		app.DistrKeeper.SetFeePool(ctx, feePool)
+		if err := app.DistrKeeper.SetFeePool(ctx, feePool); err != nil {
+			panic(err)
+		}
 
 		if err := app.DistrKeeper.Hooks().AfterValidatorCreated(ctx, val.GetOperator()); err != nil {
 			panic(err)

tests/integration/distribution/keeper/grpc_query_test.go

@@ -100,8 +100,11 @@ func TestGRPCValidatorOutstandingRewards(t *testing.T) {
 	tstaking.CreateValidator(f.valAddr, valConsPk0, sdk.NewInt(initialStake), true)
 
 	// set outstanding rewards
-	f.distrKeeper.SetValidatorOutstandingRewards(f.sdkCtx, f.valAddr, types.ValidatorOutstandingRewards{Rewards: valCommission})
-	rewards := f.distrKeeper.GetValidatorOutstandingRewards(f.sdkCtx, f.valAddr)
+	err := f.distrKeeper.SetValidatorOutstandingRewards(f.sdkCtx, f.valAddr, types.ValidatorOutstandingRewards{Rewards: valCommission})
+	assert.NilError(t, err)
+
+	rewards, err := f.distrKeeper.GetValidatorOutstandingRewards(f.sdkCtx, f.valAddr)
+	assert.NilError(t, err)
 
 	testCases := []struct {
 		name      string

tests/integration/distribution/keeper/msg_server_test.go

@@ -10,7 +10,6 @@ import (
 
 	cmtabcitypes "github.com/cometbft/cometbft/abci/types"
 	"github.com/cometbft/cometbft/proto/tendermint/types"
-	"github.com/stretchr/testify/require"
 	"gotest.tools/v3/assert"
 
 	"github.com/cosmos/cosmos-sdk/codec"
@@ -98,7 +97,7 @@ func initFixture(t testing.TB) *fixture {
 	stakingKeeper := stakingkeeper.NewKeeper(cdc, keys[stakingtypes.StoreKey], accountKeeper, bankKeeper, authority.String())
 
 	distrKeeper := distrkeeper.NewKeeper(
-		cdc, keys[distrtypes.StoreKey], accountKeeper, bankKeeper, stakingKeeper, distrtypes.ModuleName, authority.String(),
+		cdc, runtime.NewKVStoreService(keys[distrtypes.StoreKey]), accountKeeper, bankKeeper, stakingKeeper, distrtypes.ModuleName, authority.String(),
 	)
 
 	authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil)
@@ -151,7 +150,8 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
 		CommunityPool: sdk.NewDecCoins(sdk.DecCoin{Denom: "stake", Amount: math.LegacyNewDec(10000)}),
 	})
 	f.distrKeeper.SetParams(f.sdkCtx, distrtypes.DefaultParams())
-	initFeePool := f.distrKeeper.GetFeePool(f.sdkCtx)
+	initFeePool, err := f.distrKeeper.GetFeePool(f.sdkCtx)
+	assert.NilError(t, err)
 
 	delAddr := sdk.AccAddress(PKS[1].Address())
 	valConsAddr := sdk.ConsAddress(valConsPk0.Address())
@@ -195,7 +195,8 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
 	currentRewards := distrtypes.NewValidatorCurrentRewards(decCoins, 3)
 	f.distrKeeper.SetValidatorCurrentRewards(f.sdkCtx, f.valAddr, currentRewards)
 	f.distrKeeper.SetValidatorOutstandingRewards(f.sdkCtx, f.valAddr, distrtypes.ValidatorOutstandingRewards{Rewards: valCommission})
-	initOutstandingRewards := f.distrKeeper.GetValidatorOutstandingRewardsCoins(f.sdkCtx, f.valAddr)
+	initOutstandingRewards, err := f.distrKeeper.GetValidatorOutstandingRewardsCoins(f.sdkCtx, f.valAddr)
+	assert.NilError(t, err)
 
 	testCases := []struct {
 		name      string
@@ -258,9 +259,10 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
 		},
 	}
 	height := f.app.LastBlockHeight()
-	require.Panics(t, func() {
-		f.distrKeeper.GetPreviousProposerConsAddr(f.sdkCtx)
-	})
+
+	_, err = f.distrKeeper.GetPreviousProposerConsAddr(f.sdkCtx)
+	assert.Error(t, err, "previous proposer not set")
+
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
@@ -275,15 +277,6 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
 				panic(fmt.Errorf("expected block height to be %d, got %d", height, f.app.LastBlockHeight()))
 			}
 
-			prevProposerConsAddr := f.distrKeeper.GetPreviousProposerConsAddr(f.sdkCtx)
-			assert.Assert(t, prevProposerConsAddr.Empty() == false)
-			assert.DeepEqual(t, prevProposerConsAddr, valConsAddr)
-			var previousTotalPower int64
-			for _, voteInfo := range f.sdkCtx.VoteInfos() {
-				previousTotalPower += voteInfo.Validator.Power
-			}
-			assert.Equal(t, previousTotalPower, int64(100))
-
 			if tc.expErr {
 				assert.ErrorContains(t, err, tc.expErrMsg)
 			} else {
@@ -300,11 +293,21 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
 				assert.Assert(t, initBalance.IsAllLTE(curBalance))
 
 				// check rewards
-				curFeePool := f.distrKeeper.GetFeePool(f.sdkCtx)
+				curFeePool, _ := f.distrKeeper.GetFeePool(f.sdkCtx)
 				rewards := curFeePool.GetCommunityPool().Sub(initFeePool.CommunityPool)
-				curOutstandingRewards := f.distrKeeper.GetValidatorOutstandingRewards(f.sdkCtx, f.valAddr)
+				curOutstandingRewards, _ := f.distrKeeper.GetValidatorOutstandingRewards(f.sdkCtx, f.valAddr)
 				assert.DeepEqual(t, rewards, initOutstandingRewards.Sub(curOutstandingRewards.Rewards))
 			}
+
+			prevProposerConsAddr, err := f.distrKeeper.GetPreviousProposerConsAddr(f.sdkCtx)
+			assert.NilError(t, err)
+			assert.Assert(t, prevProposerConsAddr.Empty() == false)
+			assert.DeepEqual(t, prevProposerConsAddr, valConsAddr)
+			var previousTotalPower int64
+			for _, voteInfo := range f.sdkCtx.VoteInfos() {
+				previousTotalPower += voteInfo.Validator.Power
+			}
+			assert.Equal(t, previousTotalPower, int64(100))
 		})
 	}
 }
@@ -328,7 +331,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 		{
 			name: "empty delegator address",
 			preRun: func() {
-				params := f.distrKeeper.GetParams(f.sdkCtx)
+				params, _ := f.distrKeeper.GetParams(f.sdkCtx)
 				params.WithdrawAddrEnabled = true
 				assert.NilError(t, f.distrKeeper.SetParams(f.sdkCtx, params))
 			},
@@ -342,7 +345,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 		{
 			name: "empty withdraw address",
 			preRun: func() {
-				params := f.distrKeeper.GetParams(f.sdkCtx)
+				params, _ := f.distrKeeper.GetParams(f.sdkCtx)
 				params.WithdrawAddrEnabled = true
 				assert.NilError(t, f.distrKeeper.SetParams(f.sdkCtx, params))
 			},
@@ -356,7 +359,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 		{
 			name: "both empty addresses",
 			preRun: func() {
-				params := f.distrKeeper.GetParams(f.sdkCtx)
+				params, _ := f.distrKeeper.GetParams(f.sdkCtx)
 				params.WithdrawAddrEnabled = true
 				assert.NilError(t, f.distrKeeper.SetParams(f.sdkCtx, params))
 			},
@@ -370,7 +373,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 		{
 			name: "withdraw address disabled",
 			preRun: func() {
-				params := f.distrKeeper.GetParams(f.sdkCtx)
+				params, _ := f.distrKeeper.GetParams(f.sdkCtx)
 				params.WithdrawAddrEnabled = false
 				assert.NilError(t, f.distrKeeper.SetParams(f.sdkCtx, params))
 			},
@@ -384,7 +387,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 		{
 			name: "valid msg with same delegator and withdraw address",
 			preRun: func() {
-				params := f.distrKeeper.GetParams(f.sdkCtx)
+				params, _ := f.distrKeeper.GetParams(f.sdkCtx)
 				params.WithdrawAddrEnabled = true
 				assert.NilError(t, f.distrKeeper.SetParams(f.sdkCtx, params))
 			},
@@ -397,7 +400,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 		{
 			name: "valid msg",
 			preRun: func() {
-				params := f.distrKeeper.GetParams(f.sdkCtx)
+				params, _ := f.distrKeeper.GetParams(f.sdkCtx)
 				params.WithdrawAddrEnabled = true
 				assert.NilError(t, f.distrKeeper.SetParams(f.sdkCtx, params))
 			},
@@ -421,7 +424,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 				assert.ErrorContains(t, err, tc.expErrMsg)
 
 				// query the delegator withdraw address
-				addr := f.distrKeeper.GetDelegatorWithdrawAddr(f.sdkCtx, delAddr)
+				addr, _ := f.distrKeeper.GetDelegatorWithdrawAddr(f.sdkCtx, delAddr)
 				assert.DeepEqual(t, addr, delAddr)
 			} else {
 				assert.NilError(t, err)
@@ -433,7 +436,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 				assert.NilError(t, err)
 
 				// query the delegator withdraw address
-				addr := f.distrKeeper.GetDelegatorWithdrawAddr(f.sdkCtx, delAddr)
+				addr, _ := f.distrKeeper.GetDelegatorWithdrawAddr(f.sdkCtx, delAddr)
 				assert.DeepEqual(t, addr.String(), tc.msg.WithdrawAddress)
 			}
 		})
@@ -529,11 +532,11 @@ func TestMsgWithdrawValidatorCommission(t *testing.T) {
 				), balance)
 
 				// check remainder
-				remainder := f.distrKeeper.GetValidatorAccumulatedCommission(f.sdkCtx, f.valAddr).Commission
+				remainder, _ := f.distrKeeper.GetValidatorAccumulatedCommission(f.sdkCtx, f.valAddr)
 				assert.DeepEqual(t, sdk.DecCoins{
 					sdk.NewDecCoinFromDec("mytoken", math.LegacyNewDec(1).Quo(math.LegacyNewDec(4))),
 					sdk.NewDecCoinFromDec("stake", math.LegacyNewDec(1).Quo(math.LegacyNewDec(2))),
-				}, remainder)
+				}, remainder.Commission)
 			}
 		})
 
@@ -546,7 +549,7 @@ func TestMsgFundCommunityPool(t *testing.T) {
 
 	// reset fee pool
 	f.distrKeeper.SetFeePool(f.sdkCtx, distrtypes.InitialFeePool())
-	initPool := f.distrKeeper.GetFeePool(f.sdkCtx)
+	initPool, _ := f.distrKeeper.GetFeePool(f.sdkCtx)
 	assert.Assert(t, initPool.CommunityPool.Empty())
 
 	initTokens := f.stakingKeeper.TokensFromConsensusPower(f.sdkCtx, int64(100))
@@ -624,7 +627,8 @@ func TestMsgFundCommunityPool(t *testing.T) {
 				assert.NilError(t, err)
 
 				// query the community pool funds
-				assert.DeepEqual(t, initPool.CommunityPool.Add(sdk.NewDecCoinsFromCoins(amount...)...), f.distrKeeper.GetFeePool(f.sdkCtx).CommunityPool)
+				feePool, _ := f.distrKeeper.GetFeePool(f.sdkCtx)
+				assert.DeepEqual(t, initPool.CommunityPool.Add(sdk.NewDecCoinsFromCoins(amount...)...), feePool.CommunityPool)
 				assert.Assert(t, f.bankKeeper.GetAllBalances(f.sdkCtx, addr).Empty())
 			}
 		})
@@ -750,7 +754,7 @@ func TestMsgUpdateParams(t *testing.T) {
 				assert.NilError(t, err)
 
 				// query the params and verify it has been updated
-				params := f.distrKeeper.GetParams(f.sdkCtx)
+				params, _ := f.distrKeeper.GetParams(f.sdkCtx)
 				assert.DeepEqual(t, distrtypes.DefaultParams(), params)
 			}
 		})
@@ -765,7 +769,7 @@ func TestMsgCommunityPoolSpend(t *testing.T) {
 	f.distrKeeper.SetFeePool(f.sdkCtx, distrtypes.FeePool{
 		CommunityPool: sdk.NewDecCoins(sdk.DecCoin{Denom: "stake", Amount: math.LegacyNewDec(10000)}),
 	})
-	initialFeePool := f.distrKeeper.GetFeePool(f.sdkCtx)
+	initialFeePool, _ := f.distrKeeper.GetFeePool(f.sdkCtx)
 
 	initTokens := f.stakingKeeper.TokensFromConsensusPower(f.sdkCtx, int64(100))
 	f.bankKeeper.MintCoins(f.sdkCtx, distrtypes.ModuleName, sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, initTokens)))
@@ -828,7 +832,7 @@ func TestMsgCommunityPoolSpend(t *testing.T) {
 				assert.NilError(t, err)
 
 				// query the community pool to verify it has been updated
-				communityPool := f.distrKeeper.GetFeePoolCommunityCoins(f.sdkCtx)
+				communityPool, _ := f.distrKeeper.GetFeePoolCommunityCoins(f.sdkCtx)
 				newPool, negative := initialFeePool.CommunityPool.SafeSub(sdk.NewDecCoinsFromCoins(tc.msg.Amount...))
 				assert.Assert(t, negative == false)
 				assert.DeepEqual(t, communityPool, newPool)
@@ -928,7 +932,7 @@ func TestMsgDepositValidatorRewardsPool(t *testing.T) {
 				assert.NilError(t, err)
 
 				// check validator outstanding rewards
-				outstandingRewards := f.distrKeeper.GetValidatorOutstandingRewards(f.sdkCtx, val)
+				outstandingRewards, _ := f.distrKeeper.GetValidatorOutstandingRewards(f.sdkCtx, val)
 				for _, c := range tc.msg.Amount {
 					x := outstandingRewards.Rewards.AmountOf(c.Denom)
 					assert.DeepEqual(t, x, sdk.NewDecFromInt(c.Amount))

x/distribution/README.md

@@ -286,7 +286,7 @@ https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/
 ```
 
 ```go
-func (k Keeper) SetWithdrawAddr(ctx sdk.Context, delegatorAddr sdk.AccAddress, withdrawAddr sdk.AccAddress) error
+func (k Keeper) SetWithdrawAddr(ctx context.Context, delegatorAddr sdk.AccAddress, withdrawAddr sdk.AccAddress) error
 	if k.blockedAddrs[withdrawAddr.String()] {
 		fail with "`{withdrawAddr}` is not allowed to receive external funds"
 	}
@@ -351,7 +351,7 @@ This message sends coins directly from the sender to the community pool.
 The transaction fails if the amount cannot be transferred from the sender to the distribution module account.
 
 ```go
-func (k Keeper) FundCommunityPool(ctx sdk.Context, amount sdk.Coins, sender sdk.AccAddress) error {
+func (k Keeper) FundCommunityPool(ctx context.Context, amount sdk.Coins, sender sdk.AccAddress) error {
     if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, amount); err != nil {
         return err
     }
@@ -375,7 +375,7 @@ Initializing a delegation increments the validator period and keeps track of the
 
 ```go
 // initialize starting info for a new delegation
-func (k Keeper) initializeDelegation(ctx sdk.Context, val sdk.ValAddress, del sdk.AccAddress) {
+func (k Keeper) initializeDelegation(ctx context.Context, val sdk.ValAddress, del sdk.AccAddress) {
     // period has already been incremented - we want to store the period ended by this delegation action
     previousPeriod := k.GetValidatorCurrentRewards(ctx, val).Period - 1