From a19eead6917d81c05df96b30eabbd0f24cfc41a1 Mon Sep 17 00:00:00 2001 From: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> Date: Thu, 24 Nov 2022 20:23:22 +0530 Subject: [PATCH] refactor: x/crisis audit changes (#13990) * 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 --- proto/cosmos/crisis/module/v1/module.proto | 1 + proto/cosmos/crisis/v1beta1/tx.proto | 7 +- x/crisis/exported/exported.go | 1 - x/crisis/keeper/genesis_test.go | 70 ++++++++++++++++++ x/crisis/keeper/keeper_test.go | 5 +- x/crisis/keeper/migrator.go | 5 +- x/crisis/keeper/msg_server.go | 4 ++ x/crisis/keeper/msg_server_test.go | 84 ++++++++++++++++++++-- x/crisis/types/codec.go | 1 + x/crisis/types/msgs.go | 10 ++- 10 files changed, 176 insertions(+), 12 deletions(-) create mode 100644 x/crisis/keeper/genesis_test.go diff --git a/proto/cosmos/crisis/module/v1/module.proto b/proto/cosmos/crisis/module/v1/module.proto index a0a00f713a..fe9249626a 100644 --- 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 index ba734b5db7..fc9a3fceec 100644 --- 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 index 4360786511..8b43ec74fc 100644 --- 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 new file mode 100644 index 0000000000..5a85b11afa --- /dev/null +++ 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 index 905cb99aee..ecfa81967b 100644 --- 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 index 53b68fb17f..fa962e4db0 100644 --- a/x/crisis/keeper/migrator.go +++ b/x/crisis/keeper/migrator.go @@ -12,6 +12,7 @@ type Migrator struct { legacySubspace exported.Subspace } +// NewMigrator returns a new Migrator. func NewMigrator(k *Keeper, ss exported.Subspace) Migrator { return Migrator{ keeper: k, @@ -19,9 +20,9 @@ func NewMigrator(k *Keeper, ss exported.Subspace) Migrator { } } -// 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 index 2f6ed48bc8..6b1ad58342 100644 --- 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 index 2c2ac3ebeb..07f4f5f61f 100644 --- a/x/crisis/keeper/msg_server_test.go +++ b/x/crisis/keeper/msg_server_test.go @@ -3,6 +3,9 @@ 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" @@ -10,18 +13,18 @@ import ( "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 index a971c042d7..9e2ed19261 100644 --- 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 index b7af338a08..c2e2942ba1 100644 --- 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.