From 985aae5575762eb1380611c4c0e9ed206f2b6492 Mon Sep 17 00:00:00 2001 From: Marin Basic Date: Thu, 4 Apr 2019 09:10:56 +0200 Subject: [PATCH] Return errors instead of panic (#3782) This is related to issue #3741 where fetching data from invalid store, package panic. Modify subspace.go to return errors instead of panic. Also update other packages that import subspace and handle errors. --- .pending/breaking/sdk/3782-Return-errors-i | 1 + baseapp/baseapp.go | 12 +++- cmd/gaia/app/app.go | 33 +++++++---- cmd/gaia/app/export.go | 6 +- cmd/gaia/cmd/gaiadebug/hack.go | 18 +++--- types/abci.go | 4 +- x/bank/keeper.go | 7 ++- x/crisis/params.go | 12 +++- x/distribution/abci_app.go | 4 +- x/gov/endblocker.go | 4 +- x/gov/keeper.go | 42 ++++++++++++-- x/gov/test_common.go | 6 +- x/mint/abci_app.go | 13 ++++- x/mint/genesis.go | 24 +++++--- x/mint/keeper.go | 17 +++--- x/mint/querier.go | 15 ++++- x/mint/querier_test.go | 12 +++- x/mint/test_common.go | 3 +- x/params/keeper_test.go | 28 ++++----- x/params/subspace/subspace.go | 67 ++++++++++++---------- x/slashing/app_test.go | 6 +- x/slashing/keeper_test.go | 6 +- x/slashing/params.go | 42 ++++++++++++-- x/slashing/tick.go | 6 +- x/slashing/tick_test.go | 9 ++- x/staking/app_test.go | 6 +- x/staking/handler.go | 4 +- x/staking/keeper/params.go | 24 ++++++-- 28 files changed, 296 insertions(+), 135 deletions(-) create mode 100644 .pending/breaking/sdk/3782-Return-errors-i diff --git a/.pending/breaking/sdk/3782-Return-errors-i b/.pending/breaking/sdk/3782-Return-errors-i new file mode 100644 index 000000000000..6c717963cea8 --- /dev/null +++ b/.pending/breaking/sdk/3782-Return-errors-i @@ -0,0 +1 @@ +#3782 Return errors instead of panic diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index b244a98b77c1..2b8dfd3e944c 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -538,6 +538,7 @@ func (app *BaseApp) validateHeight(req abci.RequestBeginBlock) error { // BeginBlock implements the ABCI application interface. func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeginBlock) { + var err error if app.cms.TracingEnabled() { app.cms.SetTracingContext(sdk.TraceContext( map[string]interface{}{"blockHeight": req.Header.Height}, @@ -572,7 +573,10 @@ func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeg app.deliverState.ctx = app.deliverState.ctx.WithBlockGasMeter(gasMeter) if app.beginBlocker != nil { - res = app.beginBlocker(app.deliverState.ctx, req) + res, err = app.beginBlocker(app.deliverState.ctx, req) + if err != nil { + panic(err) + } } // set the signed validators for addition to context in deliverTx @@ -874,12 +878,16 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk // EndBlock implements the ABCI interface. func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBlock) { + var err error if app.deliverState.ms.TracingEnabled() { app.deliverState.ms = app.deliverState.ms.SetTracingContext(nil).(sdk.CacheMultiStore) } if app.endBlocker != nil { - res = app.endBlocker(app.deliverState.ctx, req) + res, err = app.endBlocker(app.deliverState.ctx, req) + if err != nil { + panic(err) + } } return diff --git a/cmd/gaia/app/app.go b/cmd/gaia/app/app.go index 262115649f10..fa64c48d5d5e 100644 --- a/cmd/gaia/app/app.go +++ b/cmd/gaia/app/app.go @@ -218,30 +218,39 @@ func MakeCodec() *codec.Codec { } // application updates every end block -func (app *GaiaApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock { +func (app *GaiaApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) (resp abci.ResponseBeginBlock, err error) { // mint new tokens for the previous block - mint.BeginBlocker(ctx, app.mintKeeper) + err = mint.BeginBlocker(ctx, app.mintKeeper) + if err != nil { + return resp, err + } // distribute rewards for the previous block - distr.BeginBlocker(ctx, req, app.distrKeeper) + err = distr.BeginBlocker(ctx, req, app.distrKeeper) + if err != nil { + return resp, err + } // slash anyone who double signed. // NOTE: This should happen after distr.BeginBlocker so that // there is nothing left over in the validator fee pool, // so as to keep the CanWithdrawInvariant invariant. // TODO: This should really happen at EndBlocker. - tags := slashing.BeginBlocker(ctx, req, app.slashingKeeper) - - return abci.ResponseBeginBlock{ - Tags: tags.ToKVPairs(), - } + resp.Tags, err = slashing.BeginBlocker(ctx, req, app.slashingKeeper) + return resp, err } // application updates every end block // nolint: unparam -func (app *GaiaApp) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock { - tags := gov.EndBlocker(ctx, app.govKeeper) - validatorUpdates, endBlockerTags := staking.EndBlocker(ctx, app.stakingKeeper) +func (app *GaiaApp) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) (abci.ResponseEndBlock, error) { + tags, err := gov.EndBlocker(ctx, app.govKeeper) + if err != nil { + return abci.ResponseEndBlock{}, err + } + validatorUpdates, endBlockerTags, err := staking.EndBlocker(ctx, app.stakingKeeper) + if err != nil { + return abci.ResponseEndBlock{}, err + } tags = append(tags, endBlockerTags...) if app.assertInvariantsBlockly { @@ -251,7 +260,7 @@ func (app *GaiaApp) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.R return abci.ResponseEndBlock{ ValidatorUpdates: validatorUpdates, Tags: tags, - } + }, nil } // initialize store from a genesis state diff --git a/cmd/gaia/app/export.go b/cmd/gaia/app/export.go index 5174c1498a92..217d9a1b3d39 100644 --- a/cmd/gaia/app/export.go +++ b/cmd/gaia/app/export.go @@ -39,12 +39,16 @@ func (app *GaiaApp) ExportAppStateAndValidators(forZeroHeight bool, jailWhiteLis } app.accountKeeper.IterateAccounts(ctx, appendAccount) + mintGenesisState, err := mint.ExportGenesis(ctx, app.mintKeeper) + if err != nil { + return nil, nil, err + } genState := NewGenesisState( accounts, auth.ExportGenesis(ctx, app.accountKeeper, app.feeCollectionKeeper), bank.ExportGenesis(ctx, app.bankKeeper), staking.ExportGenesis(ctx, app.stakingKeeper), - mint.ExportGenesis(ctx, app.mintKeeper), + mintGenesisState, distr.ExportGenesis(ctx, app.distrKeeper), gov.ExportGenesis(ctx, app.govKeeper), crisis.ExportGenesis(ctx, app.crisisKeeper), diff --git a/cmd/gaia/cmd/gaiadebug/hack.go b/cmd/gaia/cmd/gaiadebug/hack.go index 51927592a414..74346a2d9246 100644 --- a/cmd/gaia/cmd/gaiadebug/hack.go +++ b/cmd/gaia/cmd/gaiadebug/hack.go @@ -218,23 +218,23 @@ func MakeCodec() *codec.Codec { } // application updates every end block -func (app *GaiaApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock { - tags := slashing.BeginBlocker(ctx, req, app.slashingKeeper) - - return abci.ResponseBeginBlock{ - Tags: tags.ToKVPairs(), - } +func (app *GaiaApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) (resp abci.ResponseBeginBlock, err error) { + resp.Tags, err = slashing.BeginBlocker(ctx, req, app.slashingKeeper) + return resp, err } // application updates every end block // nolint: unparam -func (app *GaiaApp) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock { - validatorUpdates, tags := staking.EndBlocker(ctx, app.stakingKeeper) +func (app *GaiaApp) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) (abci.ResponseEndBlock, error) { + validatorUpdates, tags, err := staking.EndBlocker(ctx, app.stakingKeeper) + if err != nil { + return abci.ResponseEndBlock{}, err + } return abci.ResponseEndBlock{ ValidatorUpdates: validatorUpdates, Tags: tags, - } + }, nil } // custom logic for gaia initialization diff --git a/types/abci.go b/types/abci.go index 0646d21e3223..bbf2e7225d9f 100644 --- a/types/abci.go +++ b/types/abci.go @@ -6,10 +6,10 @@ import abci "github.com/tendermint/tendermint/abci/types" type InitChainer func(ctx Context, req abci.RequestInitChain) abci.ResponseInitChain // run code before the transactions in a block -type BeginBlocker func(ctx Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock +type BeginBlocker func(ctx Context, req abci.RequestBeginBlock) (abci.ResponseBeginBlock, error) // run code after the transactions in a block and return updates to the validator set -type EndBlocker func(ctx Context, req abci.RequestEndBlock) abci.ResponseEndBlock +type EndBlocker func(ctx Context, req abci.RequestEndBlock) (abci.ResponseEndBlock, error) // respond to p2p filtering queries from Tendermint type PeerFilter func(info string) abci.ResponseQuery diff --git a/x/bank/keeper.go b/x/bank/keeper.go index c5322ec5c73e..cd8bc2c35853 100644 --- a/x/bank/keeper.go +++ b/x/bank/keeper.go @@ -168,7 +168,12 @@ func (keeper BaseSendKeeper) GetSendEnabled(ctx sdk.Context) bool { // SetSendEnabled sets the send enabled func (keeper BaseSendKeeper) SetSendEnabled(ctx sdk.Context, enabled bool) { - keeper.paramSpace.Set(ctx, ParamStoreKeySendEnabled, &enabled) + err := keeper.paramSpace.Set(ctx, ParamStoreKeySendEnabled, &enabled) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } } var _ ViewKeeper = (*BaseViewKeeper)(nil) diff --git a/x/crisis/params.go b/x/crisis/params.go index 14c8a2567315..91d68d1bf360 100644 --- a/x/crisis/params.go +++ b/x/crisis/params.go @@ -24,11 +24,19 @@ func ParamKeyTable() params.KeyTable { // GetConstantFee get's the constant fee from the paramSpace func (k Keeper) GetConstantFee(ctx sdk.Context) (constantFee sdk.Coin) { - k.paramSpace.Get(ctx, ParamStoreKeyConstantFee, &constantFee) + if err := k.paramSpace.Get(ctx, ParamStoreKeyConstantFee, &constantFee); err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // GetConstantFee set's the constant fee in the paramSpace func (k Keeper) SetConstantFee(ctx sdk.Context, constantFee sdk.Coin) { - k.paramSpace.Set(ctx, ParamStoreKeyConstantFee, constantFee) + if err := k.paramSpace.Set(ctx, ParamStoreKeyConstantFee, constantFee); err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } } diff --git a/x/distribution/abci_app.go b/x/distribution/abci_app.go index 0a2ef5627a90..46e2652749be 100644 --- a/x/distribution/abci_app.go +++ b/x/distribution/abci_app.go @@ -8,7 +8,7 @@ import ( ) // set the proposer for determining distribution during endblock -func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) { +func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) error { // determine the total power signing the block var previousTotalPower, sumPreviousPrecommitPower int64 @@ -29,5 +29,5 @@ func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) // record the proposer for when we payout on the next block consAddr := sdk.ConsAddress(req.Header.ProposerAddress) k.SetPreviousProposerConsAddr(ctx, consAddr) - + return nil } diff --git a/x/gov/endblocker.go b/x/gov/endblocker.go index 3da7f3a1dac1..ed08ae6c3232 100644 --- a/x/gov/endblocker.go +++ b/x/gov/endblocker.go @@ -8,7 +8,7 @@ import ( ) // Called every block, process inflation, update validator set -func EndBlocker(ctx sdk.Context, keeper Keeper) sdk.Tags { +func EndBlocker(ctx sdk.Context, keeper Keeper) (sdk.Tags, error) { logger := ctx.Logger().With("module", "x/gov") resTags := sdk.NewTags() @@ -78,5 +78,5 @@ func EndBlocker(ctx sdk.Context, keeper Keeper) sdk.Tags { resTags = resTags.AppendTag(tags.ProposalResult, tagValue) } - return resTags + return resTags, nil } diff --git a/x/gov/keeper.go b/x/gov/keeper.go index 46085cf2d986..297915303a6c 100644 --- a/x/gov/keeper.go +++ b/x/gov/keeper.go @@ -261,34 +261,64 @@ func (keeper Keeper) activateVotingPeriod(ctx sdk.Context, proposal Proposal) { // Returns the current DepositParams from the global param store func (keeper Keeper) GetDepositParams(ctx sdk.Context) DepositParams { var depositParams DepositParams - keeper.paramSpace.Get(ctx, ParamStoreKeyDepositParams, &depositParams) + err := keeper.paramSpace.Get(ctx, ParamStoreKeyDepositParams, &depositParams) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return depositParams } // Returns the current VotingParams from the global param store func (keeper Keeper) GetVotingParams(ctx sdk.Context) VotingParams { var votingParams VotingParams - keeper.paramSpace.Get(ctx, ParamStoreKeyVotingParams, &votingParams) + err := keeper.paramSpace.Get(ctx, ParamStoreKeyVotingParams, &votingParams) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return votingParams } // Returns the current TallyParam from the global param store func (keeper Keeper) GetTallyParams(ctx sdk.Context) TallyParams { var tallyParams TallyParams - keeper.paramSpace.Get(ctx, ParamStoreKeyTallyParams, &tallyParams) + err := keeper.paramSpace.Get(ctx, ParamStoreKeyTallyParams, &tallyParams) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return tallyParams } func (keeper Keeper) setDepositParams(ctx sdk.Context, depositParams DepositParams) { - keeper.paramSpace.Set(ctx, ParamStoreKeyDepositParams, &depositParams) + err := keeper.paramSpace.Set(ctx, ParamStoreKeyDepositParams, &depositParams) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } } func (keeper Keeper) setVotingParams(ctx sdk.Context, votingParams VotingParams) { - keeper.paramSpace.Set(ctx, ParamStoreKeyVotingParams, &votingParams) + err := keeper.paramSpace.Set(ctx, ParamStoreKeyVotingParams, &votingParams) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } } func (keeper Keeper) setTallyParams(ctx sdk.Context, tallyParams TallyParams) { - keeper.paramSpace.Set(ctx, ParamStoreKeyTallyParams, &tallyParams) + err := keeper.paramSpace.Set(ctx, ParamStoreKeyTallyParams, &tallyParams) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } } // Votes diff --git a/x/gov/test_common.go b/x/gov/test_common.go index a14251841d56..1703dfcf528a 100644 --- a/x/gov/test_common.go +++ b/x/gov/test_common.go @@ -58,11 +58,11 @@ func getMockApp(t *testing.T, numGenAccs int, genState GenesisState, genAccs []a // gov and staking endblocker func getEndBlocker(keeper Keeper) sdk.EndBlocker { - return func(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock { - tags := EndBlocker(ctx, keeper) + return func(ctx sdk.Context, req abci.RequestEndBlock) (abci.ResponseEndBlock, error) { + tags, err := EndBlocker(ctx, keeper) return abci.ResponseEndBlock{ Tags: tags, - } + }, err } } diff --git a/x/mint/abci_app.go b/x/mint/abci_app.go index 5016a464d18a..ea998185a4b0 100644 --- a/x/mint/abci_app.go +++ b/x/mint/abci_app.go @@ -5,11 +5,17 @@ import ( ) // Inflate every block, update inflation parameters once per hour -func BeginBlocker(ctx sdk.Context, k Keeper) { +func BeginBlocker(ctx sdk.Context, k Keeper) error { // fetch stored minter & params - minter := k.GetMinter(ctx) - params := k.GetParams(ctx) + minter, err := k.GetMinter(ctx) + if err != nil { + return err + } + params, err := k.GetParams(ctx) + if err != nil { + return err + } // recalculate inflation rate totalSupply := k.sk.TotalTokens(ctx) @@ -22,5 +28,6 @@ func BeginBlocker(ctx sdk.Context, k Keeper) { mintedCoin := minter.BlockProvision(params) k.fck.AddCollectedFees(ctx, sdk.Coins{mintedCoin}) k.sk.InflateSupply(ctx, mintedCoin.Amount) + return nil } diff --git a/x/mint/genesis.go b/x/mint/genesis.go index 11f96c6f90bb..0408eb2dfb4b 100644 --- a/x/mint/genesis.go +++ b/x/mint/genesis.go @@ -29,15 +29,25 @@ func DefaultGenesisState() GenesisState { // new mint genesis func InitGenesis(ctx sdk.Context, keeper Keeper, data GenesisState) { keeper.SetMinter(ctx, data.Minter) - keeper.SetParams(ctx, data.Params) + if err := keeper.SetParams(ctx, data.Params); err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } } -// ExportGenesis returns a GenesisState for a given context and keeper. -func ExportGenesis(ctx sdk.Context, keeper Keeper) GenesisState { - - minter := keeper.GetMinter(ctx) - params := keeper.GetParams(ctx) - return NewGenesisState(minter, params) +// ExportGenesis returns a GenesisState for a given context and keeper. The +// GenesisState will contain the pool, and validator/delegator distribution info's +func ExportGenesis(ctx sdk.Context, keeper Keeper) (GenesisState, error) { + minter, err := keeper.GetMinter(ctx) + if err != nil { + return GenesisState{}, err + } + params, err := keeper.GetParams(ctx) + if err != nil { + return GenesisState{}, err + } + return NewGenesisState(minter, params), nil } // ValidateGenesis validates the provided genesis state to ensure the diff --git a/x/mint/keeper.go b/x/mint/keeper.go index 42a9f19fbaa5..4b48b02edfad 100644 --- a/x/mint/keeper.go +++ b/x/mint/keeper.go @@ -1,6 +1,8 @@ package mint import ( + "errors" + "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/params" @@ -62,11 +64,12 @@ func ParamKeyTable() params.KeyTable { //______________________________________________________________________ // get the minter -func (k Keeper) GetMinter(ctx sdk.Context) (minter Minter) { +func (k Keeper) GetMinter(ctx sdk.Context) (minter Minter, err error) { store := ctx.KVStore(k.storeKey) b := store.Get(minterKey) if b == nil { - panic("Stored fee pool should not have been nil") + err = errors.New("Stored fee pool should not have been nil") + return } k.cdc.MustUnmarshalBinaryLengthPrefixed(b, &minter) return @@ -82,13 +85,13 @@ func (k Keeper) SetMinter(ctx sdk.Context, minter Minter) { //______________________________________________________________________ // get inflation params from the global param store -func (k Keeper) GetParams(ctx sdk.Context) Params { +func (k Keeper) GetParams(ctx sdk.Context) (Params, error) { var params Params - k.paramSpace.Get(ctx, ParamStoreKeyParams, ¶ms) - return params + err := k.paramSpace.Get(ctx, ParamStoreKeyParams, ¶ms) + return params, err } // set inflation params from the global param store -func (k Keeper) SetParams(ctx sdk.Context, params Params) { - k.paramSpace.Set(ctx, ParamStoreKeyParams, ¶ms) +func (k Keeper) SetParams(ctx sdk.Context, params Params) error { + return k.paramSpace.Set(ctx, ParamStoreKeyParams, ¶ms) } diff --git a/x/mint/querier.go b/x/mint/querier.go index 21d35ce15c8c..c6ab9ef4d50a 100644 --- a/x/mint/querier.go +++ b/x/mint/querier.go @@ -35,7 +35,10 @@ func NewQuerier(k Keeper) sdk.Querier { } func queryParams(ctx sdk.Context, k Keeper) ([]byte, sdk.Error) { - params := k.GetParams(ctx) + params, err := k.GetParams(ctx) + if err != nil { + return nil, sdk.ErrInternal(sdk.AppendMsgToErr("failed to get params", err.Error())) + } res, err := codec.MarshalJSONIndent(k.cdc, params) if err != nil { @@ -46,7 +49,10 @@ func queryParams(ctx sdk.Context, k Keeper) ([]byte, sdk.Error) { } func queryInflation(ctx sdk.Context, k Keeper) ([]byte, sdk.Error) { - minter := k.GetMinter(ctx) + minter, err := k.GetMinter(ctx) + if err != nil { + return nil, sdk.ErrInternal(sdk.AppendMsgToErr("failed to get params", err.Error())) + } res, err := codec.MarshalJSONIndent(k.cdc, minter.Inflation) if err != nil { @@ -57,7 +63,10 @@ func queryInflation(ctx sdk.Context, k Keeper) ([]byte, sdk.Error) { } func queryAnnualProvisions(ctx sdk.Context, k Keeper) ([]byte, sdk.Error) { - minter := k.GetMinter(ctx) + minter, err := k.GetMinter(ctx) + if err != nil { + return nil, sdk.ErrInternal(sdk.AppendMsgToErr("failed to get params", err.Error())) + } res, err := codec.MarshalJSONIndent(k.cdc, minter.AnnualProvisions) if err != nil { diff --git a/x/mint/querier_test.go b/x/mint/querier_test.go index e9f3c8d6c640..9bc963e5f471 100644 --- a/x/mint/querier_test.go +++ b/x/mint/querier_test.go @@ -42,7 +42,9 @@ func TestQueryParams(t *testing.T) { err := input.cdc.UnmarshalJSON(res, ¶ms) require.NoError(t, err) - require.Equal(t, input.mintKeeper.GetParams(input.ctx), params) + parm, err := input.mintKeeper.GetParams(input.ctx) + require.NoError(t, err) + require.Equal(t, parm, params) } func TestQueryInflation(t *testing.T) { @@ -56,7 +58,9 @@ func TestQueryInflation(t *testing.T) { err := input.cdc.UnmarshalJSON(res, &inflation) require.NoError(t, err) - require.Equal(t, input.mintKeeper.GetMinter(input.ctx).Inflation, inflation) + parm, err := input.mintKeeper.GetMinter(input.ctx) + require.NoError(t, err) + require.Equal(t, parm.Inflation, inflation) } func TestQueryAnnualProvisions(t *testing.T) { @@ -70,5 +74,7 @@ func TestQueryAnnualProvisions(t *testing.T) { err := input.cdc.UnmarshalJSON(res, &annualProvisions) require.NoError(t, err) - require.Equal(t, input.mintKeeper.GetMinter(input.ctx).AnnualProvisions, annualProvisions) + parm, err := input.mintKeeper.GetMinter(input.ctx) + require.NoError(t, err) + require.Equal(t, parm.AnnualProvisions, annualProvisions) } diff --git a/x/mint/test_common.go b/x/mint/test_common.go index f699acda2346..7f83a0862ef9 100644 --- a/x/mint/test_common.go +++ b/x/mint/test_common.go @@ -68,7 +68,8 @@ func newTestInput(t *testing.T) testInput { ctx := sdk.NewContext(ms, abci.Header{Time: time.Unix(0, 0)}, false, log.NewTMLogger(os.Stdout)) - mintKeeper.SetParams(ctx, DefaultParams()) + err = mintKeeper.SetParams(ctx, DefaultParams()) + require.Nil(t, err) mintKeeper.SetMinter(ctx, DefaultInitialMinter()) return testInput{ctx, cdc, mintKeeper} diff --git a/x/params/keeper_test.go b/x/params/keeper_test.go index 4d20e56ad90e..f96081e5fcec 100644 --- a/x/params/keeper_test.go +++ b/x/params/keeper_test.go @@ -108,12 +108,12 @@ func TestKeeper(t *testing.T) { // Test invalid space.Get for i, kv := range kvs { var param bool - require.Panics(t, func() { space.Get(ctx, []byte(kv.key), ¶m) }, "invalid space.Get not panics, tc #%d", i) + require.Error(t, space.Get(ctx, []byte(kv.key), ¶m), "invalid space.Get not error, tc #%d", i) } // Test invalid space.Set for i, kv := range kvs { - require.Panics(t, func() { space.Set(ctx, []byte(kv.key), true) }, "invalid space.Set not panics, tc #%d", i) + require.Error(t, space.Set(ctx, []byte(kv.key), true), "invalid space.Set not error, tc #%d", i) } // Test GetSubspace @@ -122,12 +122,12 @@ func TestKeeper(t *testing.T) { gspace, ok := keeper.GetSubspace("test") require.True(t, ok, "cannot retrieve subspace, tc #%d", i) - require.NotPanics(t, func() { gspace.Get(ctx, []byte(kv.key), &gparam) }) - require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), ¶m) }) + require.NoError(t, gspace.Get(ctx, []byte(kv.key), &gparam)) + require.NoError(t, space.Get(ctx, []byte(kv.key), ¶m)) require.Equal(t, gparam, param, "GetSubspace().Get not equal with space.Get, tc #%d", i) - require.NotPanics(t, func() { gspace.Set(ctx, []byte(kv.key), int64(i)) }) - require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), ¶m) }) + require.NoError(t, gspace.Set(ctx, []byte(kv.key), int64(i))) + require.NoError(t, space.Get(ctx, []byte(kv.key), ¶m)) require.Equal(t, int64(i), param, "GetSubspace().Set not equal with space.Get, tc #%d", i) } } @@ -184,27 +184,27 @@ func TestSubspace(t *testing.T) { // Test space.Set, space.Modified for i, kv := range kvs { require.False(t, space.Modified(ctx, []byte(kv.key)), "space.Modified returns true before setting, tc #%d", i) - require.NotPanics(t, func() { space.Set(ctx, []byte(kv.key), kv.param) }, "space.Set panics, tc #%d", i) + require.NoError(t, space.Set(ctx, []byte(kv.key), kv.param), "space.Set error, tc #%d", i) require.True(t, space.Modified(ctx, []byte(kv.key)), "space.Modified returns false after setting, tc #%d", i) } // Test space.Get, space.GetIfExists for i, kv := range kvs { - require.NotPanics(t, func() { space.GetIfExists(ctx, []byte("invalid"), kv.ptr) }, "space.GetIfExists panics when no value exists, tc #%d", i) + require.Error(t, space.GetIfExists(ctx, []byte("invalid"), kv.ptr), "space.GetIfExists error when no value exists, tc #%d", i) require.Equal(t, kv.zero, indirect(kv.ptr), "space.GetIfExists unmarshalls when no value exists, tc #%d", i) - require.Panics(t, func() { space.Get(ctx, []byte("invalid"), kv.ptr) }, "invalid space.Get not panics when no value exists, tc #%d", i) + require.Error(t, space.Get(ctx, []byte("invalid"), kv.ptr), "invalid space.Get not error when no value exists, tc #%d", i) require.Equal(t, kv.zero, indirect(kv.ptr), "invalid space.Get unmarshalls when no value exists, tc #%d", i) - require.NotPanics(t, func() { space.GetIfExists(ctx, []byte(kv.key), kv.ptr) }, "space.GetIfExists panics, tc #%d", i) + require.NoError(t, space.GetIfExists(ctx, []byte(kv.key), kv.ptr), "space.GetIfExists error, tc #%d", i) require.Equal(t, kv.param, indirect(kv.ptr), "stored param not equal, tc #%d", i) - require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), kv.ptr) }, "space.Get panics, tc #%d", i) + require.NoError(t, space.Get(ctx, []byte(kv.key), kv.ptr), "space.Get error, tc #%d", i) require.Equal(t, kv.param, indirect(kv.ptr), "stored param not equal, tc #%d", i) - require.Panics(t, func() { space.Get(ctx, []byte("invalid"), kv.ptr) }, "invalid space.Get not panics when no value exists, tc #%d", i) + require.Error(t, space.Get(ctx, []byte("invalid"), kv.ptr), "invalid space.Get not error when no value exists, tc #%d", i) require.Equal(t, kv.param, indirect(kv.ptr), "invalid space.Get unmarshalls when no value existt, tc #%d", i) - require.Panics(t, func() { space.Get(ctx, []byte(kv.key), nil) }, "invalid space.Get not panics when the pointer is nil, tc #%d", i) - require.Panics(t, func() { space.Get(ctx, []byte(kv.key), new(invalid)) }, "invalid space.Get not panics when the pointer is different type, tc #%d", i) + require.Error(t, space.Get(ctx, []byte(kv.key), nil), "invalid space.Get not error when the pointer is nil, tc #%d", i) + require.Error(t, space.Get(ctx, []byte(kv.key), new(invalid)), "invalid space.Get not error when the pointer is different type, tc #%d", i) } // Test store.Get equals space.Get diff --git a/x/params/subspace/subspace.go b/x/params/subspace/subspace.go index 72cb5f209b0f..5ffb415709ed 100644 --- a/x/params/subspace/subspace.go +++ b/x/params/subspace/subspace.go @@ -1,6 +1,7 @@ package subspace import ( + "errors" "reflect" "github.com/cosmos/cosmos-sdk/codec" @@ -90,37 +91,31 @@ func concatKeys(key, subkey []byte) (res []byte) { } // Get parameter from store -func (s Subspace) Get(ctx sdk.Context, key []byte, ptr interface{}) { +func (s Subspace) Get(ctx sdk.Context, key []byte, ptr interface{}) error { store := s.kvStore(ctx) bz := store.Get(key) - err := s.cdc.UnmarshalJSON(bz, ptr) - if err != nil { - panic(err) - } + return s.cdc.UnmarshalJSON(bz, ptr) } // GetIfExists do not modify ptr if the stored parameter is nil -func (s Subspace) GetIfExists(ctx sdk.Context, key []byte, ptr interface{}) { +func (s Subspace) GetIfExists(ctx sdk.Context, key []byte, ptr interface{}) error { store := s.kvStore(ctx) bz := store.Get(key) if bz == nil { - return - } - err := s.cdc.UnmarshalJSON(bz, ptr) - if err != nil { - panic(err) + return errors.New("store key does not exist") } + return s.cdc.UnmarshalJSON(bz, ptr) } // GetWithSubkey returns a parameter with a given key and a subkey. -func (s Subspace) GetWithSubkey(ctx sdk.Context, key, subkey []byte, ptr interface{}) { - s.Get(ctx, concatKeys(key, subkey), ptr) +func (s Subspace) GetWithSubkey(ctx sdk.Context, key, subkey []byte, ptr interface{}) error { + return s.Get(ctx, concatKeys(key, subkey), ptr) } // GetWithSubkeyIfExists returns a parameter with a given key and a subkey but does not // modify ptr if the stored parameter is nil. -func (s Subspace) GetWithSubkeyIfExists(ctx sdk.Context, key, subkey []byte, ptr interface{}) { - s.GetIfExists(ctx, concatKeys(key, subkey), ptr) +func (s Subspace) GetWithSubkeyIfExists(ctx sdk.Context, key, subkey []byte, ptr interface{}) error { + return s.GetIfExists(ctx, concatKeys(key, subkey), ptr) } // Get raw bytes of parameter from store @@ -141,10 +136,10 @@ func (s Subspace) Modified(ctx sdk.Context, key []byte) bool { return tstore.Has(key) } -func (s Subspace) checkType(store sdk.KVStore, key []byte, param interface{}) { +func (s Subspace) checkType(store sdk.KVStore, key []byte, param interface{}) error { attr, ok := s.table.m[string(key)] if !ok { - panic("Parameter not registered") + return errors.New("Parameter not registered") } ty := attr.ty @@ -154,51 +149,61 @@ func (s Subspace) checkType(store sdk.KVStore, key []byte, param interface{}) { } if pty != ty { - panic("Type mismatch with registered table") + return errors.New("Type mismatch with registered table") } + return nil } // Set stores the parameter. It returns error if stored parameter has different type from input. // It also set to the transient store to record change. -func (s Subspace) Set(ctx sdk.Context, key []byte, param interface{}) { +func (s Subspace) Set(ctx sdk.Context, key []byte, param interface{}) error { store := s.kvStore(ctx) - s.checkType(store, key, param) + if err := s.checkType(store, key, param); err != nil { + return err + } bz, err := s.cdc.MarshalJSON(param) if err != nil { - panic(err) + return err } store.Set(key, bz) tstore := s.transientStore(ctx) tstore.Set(key, []byte{}) - + return nil } // SetWithSubkey set a parameter with a key and subkey // Checks parameter type only over the key -func (s Subspace) SetWithSubkey(ctx sdk.Context, key []byte, subkey []byte, param interface{}) { +func (s Subspace) SetWithSubkey(ctx sdk.Context, key []byte, subkey []byte, param interface{}) error { store := s.kvStore(ctx) - s.checkType(store, key, param) + if err := s.checkType(store, key, param); err != nil { + return err + } newkey := concatKeys(key, subkey) bz, err := s.cdc.MarshalJSON(param) if err != nil { - panic(err) + return err } store.Set(newkey, bz) tstore := s.transientStore(ctx) tstore.Set(newkey, []byte{}) + return nil } // Get to ParamSet func (s Subspace) GetParamSet(ctx sdk.Context, ps ParamSet) { for _, pair := range ps.ParamSetPairs() { - s.Get(ctx, pair.Key, pair.Value) + err := s.Get(ctx, pair.Key, pair.Value) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + } } } @@ -210,7 +215,11 @@ func (s Subspace) SetParamSet(ctx sdk.Context, ps ParamSet) { // since SetStruct is meant to be used in InitGenesis // so this method will not be called frequently v := reflect.Indirect(reflect.ValueOf(pair.Value)).Interface() - s.Set(ctx, pair.Key, v) + err := s.Set(ctx, pair.Key, v) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + } } } @@ -225,8 +234,8 @@ type ReadOnlySubspace struct { } // Exposes Get -func (ros ReadOnlySubspace) Get(ctx sdk.Context, key []byte, ptr interface{}) { - ros.s.Get(ctx, key, ptr) +func (ros ReadOnlySubspace) Get(ctx sdk.Context, key []byte, ptr interface{}) error { + return ros.s.Get(ctx, key, ptr) } // Exposes GetRaw diff --git a/x/slashing/app_test.go b/x/slashing/app_test.go index 94d53f5889c4..31093fd98ab4 100644 --- a/x/slashing/app_test.go +++ b/x/slashing/app_test.go @@ -47,12 +47,12 @@ func getMockApp(t *testing.T) (*mock.App, staking.Keeper, Keeper) { // staking endblocker func getEndBlocker(keeper staking.Keeper) sdk.EndBlocker { - return func(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock { - validatorUpdates, tags := staking.EndBlocker(ctx, keeper) + return func(ctx sdk.Context, req abci.RequestEndBlock) (abci.ResponseEndBlock, error) { + validatorUpdates, tags, err := staking.EndBlocker(ctx, keeper) return abci.ResponseEndBlock{ ValidatorUpdates: validatorUpdates, Tags: tags, - } + }, err } } diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 7b0bd11745d6..240c7cb73bd3 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -394,7 +394,8 @@ func TestValidatorDippingInAndOut(t *testing.T) { newAmt := sdk.TokensFromTendermintPower(101) got = sh(ctx, NewTestMsgCreateValidator(addrs[1], pks[1], newAmt)) require.True(t, got.IsOK()) - validatorUpdates, _ := staking.EndBlocker(ctx, sk) + validatorUpdates, _, err := staking.EndBlocker(ctx, sk) + require.NoError(t, err) require.Equal(t, 2, len(validatorUpdates)) validator, _ := sk.GetValidator(ctx, addr) require.Equal(t, sdk.Unbonding, validator.Status) @@ -407,7 +408,8 @@ func TestValidatorDippingInAndOut(t *testing.T) { delTokens := sdk.TokensFromTendermintPower(3) got = sh(ctx, newTestMsgDelegate(sdk.AccAddress(addrs[2]), addrs[0], delTokens)) require.True(t, got.IsOK()) - validatorUpdates, _ = staking.EndBlocker(ctx, sk) + validatorUpdates, _, err = staking.EndBlocker(ctx, sk) + require.NoError(t, err) require.Equal(t, 2, len(validatorUpdates)) validator, _ = sk.GetValidator(ctx, addr) require.Equal(t, sdk.Bonded, validator.Status) diff --git a/x/slashing/params.go b/x/slashing/params.go index 5a4f18f1676f..5dd4ce8346c0 100644 --- a/x/slashing/params.go +++ b/x/slashing/params.go @@ -88,20 +88,35 @@ func DefaultParams() Params { // MaxEvidenceAge - max age for evidence func (k Keeper) MaxEvidenceAge(ctx sdk.Context) (res time.Duration) { - k.paramspace.Get(ctx, KeyMaxEvidenceAge, &res) + err := k.paramspace.Get(ctx, KeyMaxEvidenceAge, &res) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // SignedBlocksWindow - sliding window for downtime slashing func (k Keeper) SignedBlocksWindow(ctx sdk.Context) (res int64) { - k.paramspace.Get(ctx, KeySignedBlocksWindow, &res) + err := k.paramspace.Get(ctx, KeySignedBlocksWindow, &res) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // Downtime slashing threshold func (k Keeper) MinSignedPerWindow(ctx sdk.Context) int64 { var minSignedPerWindow sdk.Dec - k.paramspace.Get(ctx, KeyMinSignedPerWindow, &minSignedPerWindow) + err := k.paramspace.Get(ctx, KeyMinSignedPerWindow, &minSignedPerWindow) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } signedBlocksWindow := k.SignedBlocksWindow(ctx) // NOTE: RoundInt64 will never panic as minSignedPerWindow is @@ -111,19 +126,34 @@ func (k Keeper) MinSignedPerWindow(ctx sdk.Context) int64 { // Downtime unbond duration func (k Keeper) DowntimeJailDuration(ctx sdk.Context) (res time.Duration) { - k.paramspace.Get(ctx, KeyDowntimeJailDuration, &res) + err := k.paramspace.Get(ctx, KeyDowntimeJailDuration, &res) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // SlashFractionDoubleSign func (k Keeper) SlashFractionDoubleSign(ctx sdk.Context) (res sdk.Dec) { - k.paramspace.Get(ctx, KeySlashFractionDoubleSign, &res) + err := k.paramspace.Get(ctx, KeySlashFractionDoubleSign, &res) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // SlashFractionDowntime func (k Keeper) SlashFractionDowntime(ctx sdk.Context) (res sdk.Dec) { - k.paramspace.Get(ctx, KeySlashFractionDowntime, &res) + err := k.paramspace.Get(ctx, KeySlashFractionDowntime, &res) + if err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } diff --git a/x/slashing/tick.go b/x/slashing/tick.go index bd779973f1ca..ab1606e1a5d6 100644 --- a/x/slashing/tick.go +++ b/x/slashing/tick.go @@ -10,7 +10,7 @@ import ( ) // slashing begin block functionality -func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, sk Keeper) sdk.Tags { +func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, sk Keeper) (sdk.Tags, error) { // Iterate over all the validators which *should* have signed this block // store whether or not they have actually signed it and slash/unbond any @@ -27,9 +27,9 @@ func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, sk Keeper) sdk.Ta case tmtypes.ABCIEvidenceTypeDuplicateVote: sk.handleDoubleSign(ctx, evidence.Validator.Address, evidence.Height, evidence.Time, evidence.Validator.Power) default: - ctx.Logger().With("module", "x/slashing").Error(fmt.Sprintf("ignored unknown evidence type: %s", evidence.Type)) + return sdk.EmptyTags(), fmt.Errorf("ignored unknown evidence type: %s", evidence.Type) } } - return sdk.EmptyTags() + return sdk.EmptyTags(), nil } diff --git a/x/slashing/tick_test.go b/x/slashing/tick_test.go index cc0e5f13b4cb..33bd0d109192 100644 --- a/x/slashing/tick_test.go +++ b/x/slashing/tick_test.go @@ -42,7 +42,8 @@ func TestBeginBlocker(t *testing.T) { }}, }, } - BeginBlocker(ctx, req, keeper) + _, err := BeginBlocker(ctx, req, keeper) + require.NoError(t, err) info, found := keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(pk.Address())) require.True(t, found) @@ -64,7 +65,8 @@ func TestBeginBlocker(t *testing.T) { }}, }, } - BeginBlocker(ctx, req, keeper) + _, err := BeginBlocker(ctx, req, keeper) + require.NoError(t, err) } // for 500 blocks, mark the validator as having not signed @@ -78,7 +80,8 @@ func TestBeginBlocker(t *testing.T) { }}, }, } - BeginBlocker(ctx, req, keeper) + _, err := BeginBlocker(ctx, req, keeper) + require.NoError(t, err) } // end block diff --git a/x/staking/app_test.go b/x/staking/app_test.go index da8c52ea5b51..0d5de7c5e70f 100644 --- a/x/staking/app_test.go +++ b/x/staking/app_test.go @@ -34,13 +34,13 @@ func getMockApp(t *testing.T) (*mock.App, Keeper) { // getEndBlocker returns a staking endblocker. func getEndBlocker(keeper Keeper) sdk.EndBlocker { - return func(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock { - validatorUpdates, tags := EndBlocker(ctx, keeper) + return func(ctx sdk.Context, req abci.RequestEndBlock) (abci.ResponseEndBlock, error) { + validatorUpdates, tags, err := EndBlocker(ctx, keeper) return abci.ResponseEndBlock{ ValidatorUpdates: validatorUpdates, Tags: tags, - } + }, err } } diff --git a/x/staking/handler.go b/x/staking/handler.go index 7da83cec2e5b..dc60b1bb842e 100644 --- a/x/staking/handler.go +++ b/x/staking/handler.go @@ -34,7 +34,7 @@ func NewHandler(k keeper.Keeper) sdk.Handler { } // Called every block, update validator set -func EndBlocker(ctx sdk.Context, k keeper.Keeper) ([]abci.ValidatorUpdate, sdk.Tags) { +func EndBlocker(ctx sdk.Context, k keeper.Keeper) ([]abci.ValidatorUpdate, sdk.Tags, error) { resTags := sdk.NewTags() // Calculate validator set changes. @@ -83,7 +83,7 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) ([]abci.ValidatorUpdate, sdk.T )) } - return validatorUpdates, resTags + return validatorUpdates, resTags, nil } // These functions assume everything has been authenticated, diff --git a/x/staking/keeper/params.go b/x/staking/keeper/params.go index dede56b2c19d..27f7774193d2 100644 --- a/x/staking/keeper/params.go +++ b/x/staking/keeper/params.go @@ -20,26 +20,42 @@ func ParamKeyTable() params.KeyTable { // UnbondingTime func (k Keeper) UnbondingTime(ctx sdk.Context) (res time.Duration) { - k.paramstore.Get(ctx, types.KeyUnbondingTime, &res) + if err := k.paramstore.Get(ctx, types.KeyUnbondingTime, &res); err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // MaxValidators - Maximum number of validators func (k Keeper) MaxValidators(ctx sdk.Context) (res uint16) { - k.paramstore.Get(ctx, types.KeyMaxValidators, &res) + if err := k.paramstore.Get(ctx, types.KeyMaxValidators, &res); err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // MaxEntries - Maximum number of simultaneous unbonding // delegations or redelegations (per pair/trio) func (k Keeper) MaxEntries(ctx sdk.Context) (res uint16) { - k.paramstore.Get(ctx, types.KeyMaxEntries, &res) + if err := k.paramstore.Get(ctx, types.KeyMaxEntries, &res); err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return } // BondDenom - Bondable coin denomination func (k Keeper) BondDenom(ctx sdk.Context) (res string) { - k.paramstore.Get(ctx, types.KeyBondDenom, &res) + if err := k.paramstore.Get(ctx, types.KeyBondDenom, &res); err != nil { + // TODO: return error - needs rewrite interfaces + // and handle error on the caller side + // check PR #3782 + } return }