From ca9613488e82cef3e651979f05498faa56dff557 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 19 Sep 2023 11:20:58 +0200 Subject: [PATCH] feat: add empty address check on authority field --- .../controller/keeper/keeper.go | 4 ++ .../controller/keeper/keeper_test.go | 53 ++++++++++++++ .../host/keeper/keeper.go | 4 ++ .../host/keeper/keeper_test.go | 71 +++++++++++++++++++ modules/apps/transfer/keeper/keeper.go | 4 ++ modules/apps/transfer/keeper/keeper_test.go | 70 ++++++++++++++++++ modules/core/keeper/keeper.go | 5 ++ modules/core/keeper/keeper_test.go | 38 +++++++--- 8 files changed, 238 insertions(+), 11 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/keeper.go b/modules/apps/27-interchain-accounts/controller/keeper/keeper.go index 3dd1d3342a6..78980421d65 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/keeper.go @@ -52,6 +52,10 @@ func NewKeeper( legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable()) } + if strings.TrimSpace(authority) == "" { + panic(fmt.Errorf("authority must be non-empty")) + } + return Keeper{ storeKey: key, cdc: cdc, diff --git a/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go index 22e01d1a697..68abb999c2e 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go @@ -8,6 +8,7 @@ import ( authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/keeper" "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types" genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types" icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types" @@ -107,6 +108,58 @@ func TestKeeperTestSuite(t *testing.T) { testifysuite.Run(t, new(KeeperTestSuite)) } +func (suite *KeeperTestSuite) TestNewKeeper() { + testCases := []struct { + name string + instantiateFn func() + expPass bool + }{ + {"success", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.SubModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().ScopedICAControllerKeeper, + suite.chainA.GetSimApp().MsgServiceRouter(), + suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(), + ) + }, true}, + {"failure: empty authority", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.SubModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().ScopedICAControllerKeeper, + suite.chainA.GetSimApp().MsgServiceRouter(), + "", // authority + ) + }, false}, + } + + for _, tc := range testCases { + tc := tc + suite.SetupTest() + + suite.Run(tc.name, func() { + if tc.expPass { + suite.Require().NotPanics( + tc.instantiateFn, + ) + } else { + suite.Require().Panics( + tc.instantiateFn, + ) + } + }) + } +} + func (suite *KeeperTestSuite) TestGetAllPorts() { suite.SetupTest() diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper.go b/modules/apps/27-interchain-accounts/host/keeper/keeper.go index c9e442854e6..b60ac4d652c 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/host/keeper/keeper.go @@ -59,6 +59,10 @@ func NewKeeper( legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable()) } + if strings.TrimSpace(authority) == "" { + panic(fmt.Errorf("authority must be non-empty")) + } + return Keeper{ storeKey: key, cdc: cdc, diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go index 0c44b0f10d4..a0acc4b21d6 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go @@ -6,7 +6,10 @@ import ( testifysuite "github.com/stretchr/testify/suite" + authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper" + genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types" + "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/keeper" "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types" icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types" ibcfeekeeper "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/keeper" @@ -127,6 +130,74 @@ func TestKeeperTestSuite(t *testing.T) { testifysuite.Run(t, new(KeeperTestSuite)) } +func (suite *KeeperTestSuite) TestNewKeeper() { + testCases := []struct { + name string + instantiateFn func() + expPass bool + }{ + {"success", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.SubModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().AccountKeeper, + suite.chainA.GetSimApp().ScopedICAHostKeeper, + suite.chainA.GetSimApp().MsgServiceRouter(), + suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(), + ) + }, true}, + {"failure: interchain accounts module account does not exist", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.SubModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + authkeeper.AccountKeeper{}, // empty account keeper + suite.chainA.GetSimApp().ScopedICAHostKeeper, + suite.chainA.GetSimApp().MsgServiceRouter(), + suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(), + ) + }, false}, + {"failure: empty mock staking keeper", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.SubModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().AccountKeeper, + suite.chainA.GetSimApp().ScopedICAHostKeeper, + suite.chainA.GetSimApp().MsgServiceRouter(), + "", // authority + ) + }, false}, + } + + for _, tc := range testCases { + tc := tc + suite.SetupTest() + + suite.Run(tc.name, func() { + if tc.expPass { + suite.Require().NotPanics( + tc.instantiateFn, + ) + } else { + suite.Require().Panics( + tc.instantiateFn, + ) + } + }) + } +} + func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() { suite.SetupTest() diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 5a033980d87..e3e0da974fc 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -63,6 +63,10 @@ func NewKeeper( legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable()) } + if strings.TrimSpace(authority) == "" { + panic(fmt.Errorf("authority must be non-empty")) + } + return Keeper{ cdc: cdc, storeKey: key, diff --git a/modules/apps/transfer/keeper/keeper_test.go b/modules/apps/transfer/keeper/keeper_test.go index 56afd3fcbd5..281bebd9aff 100644 --- a/modules/apps/transfer/keeper/keeper_test.go +++ b/modules/apps/transfer/keeper/keeper_test.go @@ -12,7 +12,9 @@ import ( "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" + authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper" + "github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper" "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" channelkeeper "github.com/cosmos/ibc-go/v8/modules/core/04-channel/keeper" ibctesting "github.com/cosmos/ibc-go/v8/testing" @@ -43,6 +45,74 @@ func TestKeeperTestSuite(t *testing.T) { testifysuite.Run(t, new(KeeperTestSuite)) } +func (suite *KeeperTestSuite) TestNewKeeper() { + testCases := []struct { + name string + instantiateFn func() + expPass bool + }{ + {"success", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.ModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().AccountKeeper, + suite.chainA.GetSimApp().BankKeeper, + suite.chainA.GetSimApp().ScopedTransferKeeper, + suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(), + ) + }, true}, + {"failure: transfer module account does not exist", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.ModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + authkeeper.AccountKeeper{}, // empty account keeper + suite.chainA.GetSimApp().BankKeeper, + suite.chainA.GetSimApp().ScopedTransferKeeper, + suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(), + ) + }, false}, + {"failure: empty authority", func() { + keeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(types.StoreKey), + suite.chainA.GetSimApp().GetSubspace(types.ModuleName), + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper, + &suite.chainA.GetSimApp().IBCKeeper.PortKeeper, + suite.chainA.GetSimApp().AccountKeeper, + suite.chainA.GetSimApp().BankKeeper, + suite.chainA.GetSimApp().ScopedTransferKeeper, + "", // authority + ) + }, false}, + } + + for _, tc := range testCases { + tc := tc + suite.SetupTest() + + suite.Run(tc.name, func() { + if tc.expPass { + suite.Require().NotPanics( + tc.instantiateFn, + ) + } else { + suite.Require().Panics( + tc.instantiateFn, + ) + } + }) + } +} + func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { const denom = "atom" var expAmount sdkmath.Int diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 040d0a3ffed..5c42acbf6fb 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -3,6 +3,7 @@ package keeper import ( "fmt" "reflect" + "strings" storetypes "cosmossdk.io/store/types" @@ -64,6 +65,10 @@ func NewKeeper( panic(fmt.Errorf("cannot initialize IBC keeper: empty scoped keeper")) } + if strings.TrimSpace(authority) == "" { + panic(fmt.Errorf("authority must be non-empty")) + } + clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper) connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, clientKeeper) portKeeper := portkeeper.NewKeeper(scopedKeeper) diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go index 58e8167fb1c..0cd6b2a5857 100644 --- a/modules/core/keeper/keeper_test.go +++ b/modules/core/keeper/keeper_test.go @@ -64,17 +64,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() { stakingKeeper clienttypes.StakingKeeper upgradeKeeper clienttypes.UpgradeKeeper scopedKeeper capabilitykeeper.ScopedKeeper - newIBCKeeperFn = func() { - ibckeeper.NewKeeper( - suite.chainA.GetSimApp().AppCodec(), - suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey), - suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName), - stakingKeeper, - upgradeKeeper, - scopedKeeper, - suite.chainA.App.GetIBCKeeper().GetAuthority(), - ) - } + newIBCKeeperFn func() ) testCases := []struct { @@ -113,6 +103,19 @@ func (suite *KeeperTestSuite) TestNewKeeper() { scopedKeeper = emptyScopedKeeper }, false}, + {"failure: empty authority", func() { + newIBCKeeperFn = func() { + ibckeeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey), + suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName), + stakingKeeper, + upgradeKeeper, + scopedKeeper, + "", // authority + ) + } + }, false}, {"success: replace stakingKeeper with non-empty MockStakingKeeper", func() { // use a different implementation of clienttypes.StakingKeeper mockStakingKeeper := MockStakingKeeper{"not empty"} @@ -126,6 +129,19 @@ func (suite *KeeperTestSuite) TestNewKeeper() { suite.SetupTest() suite.Run(tc.name, func() { + // set default behaviour + newIBCKeeperFn = func() { + ibckeeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey), + suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName), + stakingKeeper, + upgradeKeeper, + scopedKeeper, + suite.chainA.App.GetIBCKeeper().GetAuthority(), + ) + } + stakingKeeper = suite.chainA.GetSimApp().StakingKeeper upgradeKeeper = suite.chainA.GetSimApp().UpgradeKeeper scopedKeeper = suite.chainA.GetSimApp().ScopedIBCKeeper