Merge PR #4854: Crisis Module Manager Fixes

simapp/app.go

@@ -163,14 +163,17 @@ func NewSimApp(
 	// register the staking hooks
 	// NOTE: stakingKeeper above is passed by reference, so that it will contain these hooks
 	app.stakingKeeper = *stakingKeeper.SetHooks(
-		staking.NewMultiStakingHooks(app.distrKeeper.Hooks(), app.slashingKeeper.Hooks()))
+		staking.NewMultiStakingHooks(app.distrKeeper.Hooks(), app.slashingKeeper.Hooks()),
+	)
 
+	// NOTE: Any module instantiated in the module manager that is later modified
+	// must be passed by reference here.
 	app.mm = module.NewManager(
 		genaccounts.NewAppModule(app.accountKeeper),
 		genutil.NewAppModule(app.accountKeeper, app.stakingKeeper, app.BaseApp.DeliverTx),
 		auth.NewAppModule(app.accountKeeper),
 		bank.NewAppModule(app.bankKeeper, app.accountKeeper),
-		crisis.NewAppModule(app.crisisKeeper),
+		crisis.NewAppModule(&app.crisisKeeper),
 		supply.NewAppModule(app.supplyKeeper, app.accountKeeper),
 		distr.NewAppModule(app.distrKeeper, app.supplyKeeper),
 		gov.NewAppModule(app.govKeeper, app.supplyKeeper),
@@ -184,13 +187,15 @@ func NewSimApp(
 	// CanWithdrawInvariant invariant.
 	app.mm.SetOrderBeginBlockers(mint.ModuleName, distr.ModuleName, slashing.ModuleName)
 
-	app.mm.SetOrderEndBlockers(gov.ModuleName, staking.ModuleName)
+	app.mm.SetOrderEndBlockers(crisis.ModuleName, gov.ModuleName, staking.ModuleName)
 
-	// genutils must occur after staking so that pools are properly
-	// initialized with tokens from genesis accounts.
-	app.mm.SetOrderInitGenesis(genaccounts.ModuleName, distr.ModuleName,
-		staking.ModuleName, auth.ModuleName, bank.ModuleName, slashing.ModuleName,
-		gov.ModuleName, mint.ModuleName, supply.ModuleName, crisis.ModuleName, genutil.ModuleName)
+	// NOTE: The genutils moodule must occur after staking so that pools are
+	// properly initialized with tokens from genesis accounts.
+	app.mm.SetOrderInitGenesis(
+		genaccounts.ModuleName, distr.ModuleName, staking.ModuleName,
+		auth.ModuleName, bank.ModuleName, slashing.ModuleName, gov.ModuleName,
+		mint.ModuleName, supply.ModuleName, crisis.ModuleName, genutil.ModuleName,
+	)
 
 	app.mm.RegisterInvariants(&app.crisisKeeper)
 	app.mm.RegisterRoutes(app.Router(), app.QueryRouter())

simapp/sim_test.go

@@ -554,7 +554,7 @@ func TestAppImportExport(t *testing.T) {
 
 	defer func() {
 		newDB.Close()
-		os.RemoveAll(newDir)
+		_ = os.RemoveAll(newDir)
 	}()
 
 	newApp := NewSimApp(log.NewNopLogger(), newDB, nil, true, 0, fauxMerkleModeOpt)
@@ -566,11 +566,11 @@ func TestAppImportExport(t *testing.T) {
 		panic(err)
 	}
 
-	ctxB := newApp.NewContext(true, abci.Header{})
+	ctxB := newApp.NewContext(true, abci.Header{Height: app.LastBlockHeight()})
 	newApp.mm.InitGenesis(ctxB, genesisState)
 
 	fmt.Printf("Comparing stores...\n")
-	ctxA := app.NewContext(true, abci.Header{})
+	ctxA := app.NewContext(true, abci.Header{Height: app.LastBlockHeight()})
 
 	type StoreKeysPrefixes struct {
 		A        sdk.StoreKey

x/crisis/internal/keeper/keeper.go

@@ -23,11 +23,13 @@ type Keeper struct {
 }
 
 // NewKeeper creates a new Keeper object
-func NewKeeper(paramSpace params.Subspace, invCheckPeriod uint,
-	supplyKeeper types.SupplyKeeper, feeCollectorName string) Keeper {
+func NewKeeper(
+	paramSpace params.Subspace, invCheckPeriod uint, supplyKeeper types.SupplyKeeper,
+	feeCollectorName string,
+) Keeper {
 
 	return Keeper{
-		routes:           []types.InvarRoute{},
+		routes:           make([]types.InvarRoute, 0),
 		paramSpace:       paramSpace.WithKeyTable(types.ParamKeyTable()),
 		invCheckPeriod:   invCheckPeriod,
 		supplyKeeper:     supplyKeeper,
@@ -53,22 +55,23 @@ func (k Keeper) Routes() []types.InvarRoute {
 
 // Invariants returns all the registered Crisis keeper invariants.
 func (k Keeper) Invariants() []sdk.Invariant {
-	var invars []sdk.Invariant
-	for _, route := range k.routes {
-		invars = append(invars, route.Invar)
+	invars := make([]sdk.Invariant, len(k.routes))
+	for i, route := range k.routes {
+		invars[i] = route.Invar
 	}
 	return invars
 }
 
-// assert all invariants
+// AssertInvariants asserts all registered invariants. If any invariant fails,
+// the method panics.
 func (k Keeper) AssertInvariants(ctx sdk.Context) {
 	logger := k.Logger(ctx)
 
 	start := time.Now()
 	invarRoutes := k.Routes()
+
 	for _, ir := range invarRoutes {
 		if res, stop := ir.Invar(ctx); stop {
-
 			// TODO: Include app name as part of context to allow for this to be
 			// variable.
 			panic(fmt.Errorf("invariant broken: %s\n"+
@@ -90,5 +93,3 @@ func (k Keeper) InvCheckPeriod() uint { return k.invCheckPeriod }
 func (k Keeper) SendCoinsFromAccountToFeeCollector(ctx sdk.Context, senderAddr sdk.AccAddress, amt sdk.Coins) sdk.Error {
 	return k.supplyKeeper.SendCoinsFromAccountToModule(ctx, senderAddr, k.feeCollectorName, amt)
 }
-
-// DONTCOVER

x/crisis/internal/keeper/keeper_test.go

@@ -0,0 +1,56 @@
+package keeper
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/tendermint/tendermint/libs/log"
+
+	"github.com/cosmos/cosmos-sdk/codec"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/x/crisis/internal/types"
+	"github.com/cosmos/cosmos-sdk/x/params"
+)
+
+func testPassingInvariant(_ sdk.Context) (string, bool) {
+	return "", false
+}
+
+func testFailingInvariant(_ sdk.Context) (string, bool) {
+	return "", true
+}
+
+func testKeeper(checkPeriod uint) Keeper {
+	cdc := codec.New()
+	paramsKeeper := params.NewKeeper(
+		cdc, sdk.NewKVStoreKey(params.StoreKey), sdk.NewTransientStoreKey(params.TStoreKey), params.DefaultCodespace,
+	)
+
+	return NewKeeper(paramsKeeper.Subspace(types.DefaultParamspace), checkPeriod, nil, "test")
+}
+
+func TestLogger(t *testing.T) {
+	k := testKeeper(5)
+
+	ctx := sdk.Context{}.WithLogger(log.NewNopLogger())
+	require.Equal(t, ctx.Logger(), k.Logger(ctx))
+}
+
+func TestInvariants(t *testing.T) {
+	k := testKeeper(5)
+	require.Equal(t, k.InvCheckPeriod(), uint(5))
+
+	k.RegisterRoute("testModule", "testRoute", testPassingInvariant)
+	require.Len(t, k.Routes(), 1)
+}
+
+func TestAssertInvariants(t *testing.T) {
+	k := testKeeper(5)
+	ctx := sdk.Context{}.WithLogger(log.NewNopLogger())
+
+	k.RegisterRoute("testModule", "testRoute1", testPassingInvariant)
+	require.NotPanics(t, func() { k.AssertInvariants(ctx) })
+
+	k.RegisterRoute("testModule", "testRoute2", testFailingInvariant)
+	require.Panics(t, func() { k.AssertInvariants(ctx) })
+}

x/crisis/module.go

@@ -22,27 +22,26 @@ var (
 	_ module.AppModuleBasic = AppModuleBasic{}
 )
 
-// app module basics object
+// AppModuleBasic defines the basic application module used by the crisis module.
 type AppModuleBasic struct{}
 
-var _ module.AppModuleBasic = AppModuleBasic{}
-
-// module name
+// Name returns the crisis module's name.
 func (AppModuleBasic) Name() string {
 	return ModuleName
 }
 
-// register module codec
+// RegisterCodec registers the crisis module's types for the given codec.
 func (AppModuleBasic) RegisterCodec(cdc *codec.Codec) {
 	RegisterCodec(cdc)
 }
 
-// default genesis state
+// DefaultGenesis returns default genesis state as raw bytes for the crisis
+// module.
 func (AppModuleBasic) DefaultGenesis() json.RawMessage {
 	return types.ModuleCdc.MustMarshalJSON(DefaultGenesisState())
 }
 
-// module validate genesis
+// ValidateGenesis performs genesis state validation for the crisis module.
 func (AppModuleBasic) ValidateGenesis(bz json.RawMessage) error {
 	var data types.GenesisState
 	if err := types.ModuleCdc.UnmarshalJSON(bz, &data); err != nil {
@@ -51,77 +50,83 @@ func (AppModuleBasic) ValidateGenesis(bz json.RawMessage) error {
 	return types.ValidateGenesis(data)
 }
 
-// register rest routes
+// RegisterRESTRoutes registers no REST routes for the crisis module.
 func (AppModuleBasic) RegisterRESTRoutes(_ context.CLIContext, _ *mux.Router) {}
 
-// get the root tx command of this module
+// GetTxCmd returns the root tx command for the crisis module.
 func (AppModuleBasic) GetTxCmd(cdc *codec.Codec) *cobra.Command {
 	return cli.GetTxCmd(cdc)
 }
 
-// get the root query command of this module
+// GetQueryCmd returns no root query command for the crisis module.
 func (AppModuleBasic) GetQueryCmd(_ *codec.Codec) *cobra.Command { return nil }
 
-//___________________________
-// app module for bank
+// AppModule implements an application module for the crisis module.
 type AppModule struct {
 	AppModuleBasic
-	keeper keeper.Keeper
+
+	// NOTE: We store a reference to the keeper here so that after a module
+	// manager is created, the invariants can be properly registered and
+	// executed.
+	keeper *keeper.Keeper
 }
 
 // NewAppModule creates a new AppModule object
-func NewAppModule(keeper keeper.Keeper) AppModule {
+func NewAppModule(keeper *keeper.Keeper) AppModule {
 	return AppModule{
 		AppModuleBasic: AppModuleBasic{},
 		keeper:         keeper,
 	}
 }
 
-// module name
+// Name returns the crisis module's name.
 func (AppModule) Name() string {
 	return ModuleName
 }
 
-// register invariants
+// RegisterInvariants performs a no-op.
 func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
-// module querier route name
+// Route returns the message routing key for the crisis module.
 func (AppModule) Route() string {
 	return RouterKey
 }
 
-// module handler
+// NewHandler returns an sdk.Handler for the crisis module.
 func (am AppModule) NewHandler() sdk.Handler {
-	return NewHandler(am.keeper)
+	return NewHandler(*am.keeper)
 }
 
-// module querier route name
+// QuerierRoute returns no querier route.
 func (AppModule) QuerierRoute() string { return "" }
 
-// module querier
+// NewQuerierHandler returns no sdk.Querier.
 func (AppModule) NewQuerierHandler() sdk.Querier { return nil }
 
-// module init-genesis
+// InitGenesis performs genesis initialization for the crisis module. It returns
+// no validator updates.
 func (am AppModule) InitGenesis(ctx sdk.Context, data json.RawMessage) []abci.ValidatorUpdate {
 	var genesisState GenesisState
 	types.ModuleCdc.MustUnmarshalJSON(data, &genesisState)
-	InitGenesis(ctx, am.keeper, genesisState)
+	InitGenesis(ctx, *am.keeper, genesisState)
 
 	am.keeper.AssertInvariants(ctx)
 	return []abci.ValidatorUpdate{}
 }
 
-// module export genesis
+// ExportGenesis returns the exported genesis state as raw bytes for the crisis
+// module.
 func (am AppModule) ExportGenesis(ctx sdk.Context) json.RawMessage {
-	gs := ExportGenesis(ctx, am.keeper)
+	gs := ExportGenesis(ctx, *am.keeper)
 	return types.ModuleCdc.MustMarshalJSON(gs)
 }
 
-// module begin-block
+// BeginBlock returns the begin blocker for the crisis module.
 func (AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {}
 
-// module end-block
+// EndBlock returns the end blocker for the crisis module. It returns no validator
+// updates.
 func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate {
-	EndBlocker(ctx, am.keeper)
+	EndBlocker(ctx, *am.keeper)
 	return []abci.ValidatorUpdate{}
 }