refactor: x/crisis audit changes (#13990)

likhita-809 · julienrbrt · web-flow · commit a19eead6917d · 2022-11-24T14:53:22.000Z
* wip: nits

* add tests for VerifyInvariant and increase codecov for keeper

* add genesis test

* cover all keeper code in tests

Co-authored-by: Julien Robert &lt;julien@rbrt.fr&gt;
diff --git a/proto/cosmos/crisis/module/v1/module.proto b/proto/cosmos/crisis/module/v1/module.proto
@@ -10,6 +10,7 @@ message Module {
     go_import: "github.com/cosmos/cosmos-sdk/x/crisis"
   };
 
+  // fee_collector_name is the name of the FeeCollector ModuleAccount.
   string fee_collector_name = 1;
 
   // authority defines the custom module authority. If not set, defaults to the governance module.
diff --git a/proto/cosmos/crisis/v1beta1/tx.proto b/proto/cosmos/crisis/v1beta1/tx.proto
@@ -13,7 +13,7 @@ import "cosmos/base/v1beta1/coin.proto";
 service Msg {
   option (cosmos.msg.v1.service) = true;
 
-  // VerifyInvariant defines a method to verify a particular invariance.
+  // VerifyInvariant defines a method to verify a particular invariant.
   rpc VerifyInvariant(MsgVerifyInvariant) returns (MsgVerifyInvariantResponse);
 
   // UpdateParams defines a governance operation for updating the x/crisis module
@@ -31,8 +31,13 @@ message MsgVerifyInvariant {
   option (gogoproto.equal)           = false;
   option (gogoproto.goproto_getters) = false;
 
+  // sender is the account address of private key to send coins to fee collector account.
   string sender                = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+  
+  // name of the invariant module.
   string invariant_module_name = 2;
+
+  // invariant_route is the msg's invariant route.
   string invariant_route       = 3;
 }
 
diff --git a/x/crisis/exported/exported.go b/x/crisis/exported/exported.go
@@ -13,7 +13,6 @@ type (
 	//
 	// NOTE: This is used solely for migration of x/params managed parameters.
 	Subspace interface {
-		// GetParamSet(ctx sdk.Context, ps ParamSet)
 		Get(ctx sdk.Context, key []byte, ptr interface{})
 	}
 )
diff --git a/x/crisis/keeper/genesis_test.go b/x/crisis/keeper/genesis_test.go
@@ -0,0 +1,70 @@
+package keeper_test
+
+import (
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/suite"
+
+	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/testutil"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
+	"github.com/cosmos/cosmos-sdk/x/crisis"
+	"github.com/cosmos/cosmos-sdk/x/crisis/keeper"
+	crisistestutil "github.com/cosmos/cosmos-sdk/x/crisis/testutil"
+	"github.com/cosmos/cosmos-sdk/x/crisis/types"
+)
+
+type GenesisTestSuite struct {
+	suite.Suite
+
+	sdkCtx sdk.Context
+	keeper keeper.Keeper
+	cdc    codec.BinaryCodec
+}
+
+func TestGenesisTestSuite(t *testing.T) {
+	suite.Run(t, new(GenesisTestSuite))
+}
+
+func (s *GenesisTestSuite) SetupTest() {
+	key := sdk.NewKVStoreKey(types.StoreKey)
+	testCtx := testutil.DefaultContextWithDB(s.T(), key, sdk.NewTransientStoreKey("transient_test"))
+	encCfg := moduletestutil.MakeTestEncodingConfig(crisis.AppModuleBasic{})
+
+	// gomock initializations
+	ctrl := gomock.NewController(s.T())
+	s.cdc = codec.NewProtoCodec(encCfg.InterfaceRegistry)
+	s.sdkCtx = testCtx.Ctx
+
+	supplyKeeper := crisistestutil.NewMockSupplyKeeper(ctrl)
+
+	s.keeper = *keeper.NewKeeper(s.cdc, key, 5, supplyKeeper, "", "")
+}
+
+func (s *GenesisTestSuite) TestImportExportGenesis() {
+	// default params
+	constantFee := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000))
+	err := s.keeper.SetConstantFee(s.sdkCtx, constantFee)
+	s.Require().NoError(err)
+	genesis := s.keeper.ExportGenesis(s.sdkCtx)
+
+	// set constant fee to zero
+	constantFee = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0))
+	err = s.keeper.SetConstantFee(s.sdkCtx, constantFee)
+	s.Require().NoError(err)
+
+	s.keeper.InitGenesis(s.sdkCtx, genesis)
+	newGenesis := s.keeper.ExportGenesis(s.sdkCtx)
+	s.Require().Equal(genesis, newGenesis)
+}
+
+func (s *GenesisTestSuite) TestInitGenesis() {
+	genesisState := types.DefaultGenesisState()
+	genesisState.ConstantFee = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000))
+	s.keeper.InitGenesis(s.sdkCtx, genesisState)
+
+	constantFee := s.keeper.GetConstantFee(s.sdkCtx)
+	s.Require().Equal(genesisState.ConstantFee, constantFee)
+}
diff --git a/x/crisis/keeper/keeper_test.go b/x/crisis/keeper/keeper_test.go
@@ -3,10 +3,10 @@ package keeper_test
 import (
 	"testing"
 
-	"github.com/cosmos/cosmos-sdk/testutil"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
 
+	"github.com/cosmos/cosmos-sdk/testutil"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
 	"github.com/cosmos/cosmos-sdk/x/crisis"
@@ -40,7 +40,8 @@ func TestInvariants(t *testing.T) {
 
 	orgInvRoutes := keeper.Routes()
 	keeper.RegisterRoute("testModule", "testRoute", func(sdk.Context) (string, bool) { return "", false })
-	require.Equal(t, len(keeper.Routes()), len(orgInvRoutes)+1)
+	invar := keeper.Invariants()
+	require.Equal(t, len(invar), len(orgInvRoutes)+1)
 }
 
 func TestAssertInvariants(t *testing.T) {
diff --git a/x/crisis/keeper/migrator.go b/x/crisis/keeper/migrator.go
@@ -12,16 +12,17 @@ type Migrator struct {
 	legacySubspace exported.Subspace
 }
 
+// NewMigrator returns a new Migrator.
 func NewMigrator(k *Keeper, ss exported.Subspace) Migrator {
 	return Migrator{
 		keeper:         k,
 		legacySubspace: ss,
 	}
 }
 
-// Migrate1to2 migrates the x/mint module state from the consensus version 1 to
+// Migrate1to2 migrates the x/crisis module state from the consensus version 1 to
 // version 2. Specifically, it takes the parameters that are currently stored
-// and managed by the x/params modules and stores them directly into the x/mint
+// and managed by the x/params modules and stores them directly into the x/crisis
 // module state.
 func (m Migrator) Migrate1to2(ctx sdk.Context) error {
 	return v2.MigrateStore(ctx, m.keeper.storeKey, m.legacySubspace, m.keeper.cdc)
diff --git a/x/crisis/keeper/msg_server.go b/x/crisis/keeper/msg_server.go
@@ -11,6 +11,8 @@ import (
 
 var _ types.MsgServer = &Keeper{}
 
+// VerifyInvariant implements MsgServer.VerifyInvariant method.
+// It defines a method to verify a particular invariant.
 func (k *Keeper) VerifyInvariant(goCtx context.Context, msg *types.MsgVerifyInvariant) (*types.MsgVerifyInvariantResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 	constantFee := sdk.NewCoins(k.GetConstantFee(ctx))
@@ -62,6 +64,8 @@ func (k *Keeper) VerifyInvariant(goCtx context.Context, msg *types.MsgVerifyInva
 	return &types.MsgVerifyInvariantResponse{}, nil
 }
 
+// UpdateParams implements MsgServer.UpdateParams method.
+// It defines a method to update the x/crisis module parameters.
 func (k *Keeper) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
 	if k.authority != req.Authority {
 		return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, req.Authority)
diff --git a/x/crisis/keeper/msg_server_test.go b/x/crisis/keeper/msg_server_test.go
@@ -3,25 +3,28 @@ package keeper_test
 import (
 	"testing"
 
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/suite"
+
 	"github.com/cosmos/cosmos-sdk/testutil"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
 	"github.com/cosmos/cosmos-sdk/x/crisis"
 	"github.com/cosmos/cosmos-sdk/x/crisis/keeper"
 	crisistestutil "github.com/cosmos/cosmos-sdk/x/crisis/testutil"
 	"github.com/cosmos/cosmos-sdk/x/crisis/types"
-	"github.com/golang/mock/gomock"
-	"github.com/stretchr/testify/suite"
 )
 
 type KeeperTestSuite struct {
 	suite.Suite
 
-	ctx    sdk.Context
-	keeper *keeper.Keeper
+	ctx        sdk.Context
+	authKeeper *crisistestutil.MockSupplyKeeper
+	keeper     *keeper.Keeper
 }
 
 func (s *KeeperTestSuite) SetupTest() {
+	// gomock initializations
 	ctrl := gomock.NewController(s.T())
 	supplyKeeper := crisistestutil.NewMockSupplyKeeper(ctrl)
 
@@ -32,6 +35,79 @@ func (s *KeeperTestSuite) SetupTest() {
 
 	s.ctx = testCtx.Ctx
 	s.keeper = keeper
+	s.authKeeper = supplyKeeper
+}
+
+func (s *KeeperTestSuite) TestMsgVerifyInvariant() {
+	// default params
+	constantFee := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000))
+	err := s.keeper.SetConstantFee(s.ctx, constantFee)
+	s.Require().NoError(err)
+
+	sender := sdk.AccAddress([]byte("addr1_______________"))
+
+	s.authKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2)
+	s.keeper.RegisterRoute("bank", "total-supply", func(sdk.Context) (string, bool) { return "", false })
+
+	testCases := []struct {
+		name      string
+		input     *types.MsgVerifyInvariant
+		expErr    bool
+		expErrMsg string
+	}{
+		{
+			name: "empty sender not allowed",
+			input: &types.MsgVerifyInvariant{
+				Sender:              "",
+				InvariantModuleName: "bank",
+				InvariantRoute:      "total-supply",
+			},
+			expErr:    true,
+			expErrMsg: "empty address string is not allowed",
+		},
+		{
+			name: "invalid sender address",
+			input: &types.MsgVerifyInvariant{
+				Sender:              "invalid address",
+				InvariantModuleName: "bank",
+				InvariantRoute:      "total-supply",
+			},
+			expErr:    true,
+			expErrMsg: "decoding bech32 failed",
+		},
+		{
+			name: "unregistered invariant route",
+			input: &types.MsgVerifyInvariant{
+				Sender:              sender.String(),
+				InvariantModuleName: "module",
+				InvariantRoute:      "invalidroute",
+			},
+			expErr:    true,
+			expErrMsg: "unknown invariant",
+		},
+		{
+			name: "valid invariant",
+			input: &types.MsgVerifyInvariant{
+				Sender:              sender.String(),
+				InvariantModuleName: "bank",
+				InvariantRoute:      "total-supply",
+			},
+			expErr: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		s.Run(tc.name, func() {
+			_, err = s.keeper.VerifyInvariant(s.ctx, tc.input)
+			if tc.expErr {
+				s.Require().Error(err)
+				s.Require().Contains(err.Error(), tc.expErrMsg)
+			} else {
+				s.Require().NoError(err)
+			}
+		})
+	}
 }
 
 func (s *KeeperTestSuite) TestMsgUpdateParams() {
diff --git a/x/crisis/types/codec.go b/x/crisis/types/codec.go
@@ -19,6 +19,7 @@ func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
 	legacy.RegisterAminoMsg(cdc, &MsgUpdateParams{}, "cosmos-sdk/x/crisis/MsgUpdateParams")
 }
 
+// RegisterInterfaces registers the interfaces types with the Interface Registry.
 func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
 	registry.RegisterImplementations((*sdk.Msg)(nil),
 		&MsgVerifyInvariant{},
diff --git a/x/crisis/types/msgs.go b/x/crisis/types/msgs.go
@@ -24,8 +24,11 @@ func NewMsgVerifyInvariant(sender sdk.AccAddress, invModeName, invRoute string)
 	}
 }
 
+// Route returns the MsgVerifyInvariant's route.
 func (msg MsgVerifyInvariant) Route() string { return ModuleName }
-func (msg MsgVerifyInvariant) Type() string  { return TypeMsgVerifyInvariant }
+
+// Type returns the MsgVerifyInvariant's type.
+func (msg MsgVerifyInvariant) Type() string { return TypeMsgVerifyInvariant }
 
 // get the bytes for the message signer to sign on
 func (msg MsgVerifyInvariant) GetSigners() []sdk.AccAddress {
@@ -52,8 +55,11 @@ func (msg MsgVerifyInvariant) FullInvariantRoute() string {
 	return msg.InvariantModuleName + "/" + msg.InvariantRoute
 }
 
+// Route returns the MsgUpdateParams's route.
 func (msg MsgUpdateParams) Route() string { return ModuleName }
-func (msg MsgUpdateParams) Type() string  { return TypeMsgUpdateParams }
+
+// Type returns the MsgUpdateParams's type.
+func (msg MsgUpdateParams) Type() string { return TypeMsgUpdateParams }
 
 // GetSigners returns the signer addresses that are expected to sign the result
 // of GetSignBytes.

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,6 @@ type (`
`13`	`13`	`//`
`14`	`14`	`// NOTE: This is used solely for migration of x/params managed parameters.`
`15`	`15`	`Subspace interface {`
`16`		`- // GetParamSet(ctx sdk.Context, ps ParamSet)`
`17`	`16`	`Get(ctx sdk.Context, key []byte, ptr interface{})`
`18`	`17`	`}`
`19`	`18`	`)`
Original file line number	Diff line number	Diff line change
`@@ -12,16 +12,17 @@ type Migrator struct {`
`12`	`12`	`legacySubspace exported.Subspace`
`13`	`13`	`}`
`14`	`14`
	`15`	`+// NewMigrator returns a new Migrator.`
`15`	`16`	`func NewMigrator(k *Keeper, ss exported.Subspace) Migrator {`
`16`	`17`	`return Migrator{`
`17`	`18`	`keeper: k,`
`18`	`19`	`legacySubspace: ss,`
`19`	`20`	`}`
`20`	`21`	`}`
`21`	`22`
`22`		`-// Migrate1to2 migrates the x/mint module state from the consensus version 1 to`
	`23`	`+// Migrate1to2 migrates the x/crisis module state from the consensus version 1 to`
`23`	`24`	`// version 2. Specifically, it takes the parameters that are currently stored`
`24`		`-// and managed by the x/params modules and stores them directly into the x/mint`
	`25`	`+// and managed by the x/params modules and stores them directly into the x/crisis`
`25`	`26`	`// module state.`
`26`	`27`	`func (m Migrator) Migrate1to2(ctx sdk.Context) error {`
`27`	`28`	`return v2.MigrateStore(ctx, m.keeper.storeKey, m.legacySubspace, m.keeper.cdc)`
Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@ func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {`
`19`	`19`	`legacy.RegisterAminoMsg(cdc, &MsgUpdateParams{}, "cosmos-sdk/x/crisis/MsgUpdateParams")`
`20`	`20`	`}`
`21`	`21`
	`22`	`+// RegisterInterfaces registers the interfaces types with the Interface Registry.`
`22`	`23`	`func RegisterInterfaces(registry codectypes.InterfaceRegistry) {`
`23`	`24`	`registry.RegisterImplementations((*sdk.Msg)(nil),`
`24`	`25`	`&MsgVerifyInvariant{},`