From 6ef38d6de8eb597309ce82be9dac3e4fba3ab244 Mon Sep 17 00:00:00 2001 From: romelukaku Date: Thu, 21 Apr 2022 22:37:27 +0700 Subject: [PATCH 01/10] add empty keepers checking in ibc NewKeeper --- modules/core/keeper/keeper.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 69044e9e4ea..75ec09d88f6 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -1,11 +1,16 @@ package keeper import ( + "fmt" + "reflect" + "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" paramtypes "github.com/cosmos/cosmos-sdk/x/params/types" + stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" + upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper" clientkeeper "github.com/cosmos/ibc-go/v3/modules/core/02-client/keeper" clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" connectionkeeper "github.com/cosmos/ibc-go/v3/modules/core/03-connection/keeper" @@ -46,6 +51,11 @@ func NewKeeper( paramSpace = paramSpace.WithKeyTable(keyTable) } + err := checkEmptyKeepers(stakingKeeper, upgradeKeeper, scopedKeeper) + if err != nil { + panic(fmt.Errorf("cannot initialize ibc keeper: %w", err)) + } + clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper) connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, clientKeeper) portKeeper := portkeeper.NewKeeper(scopedKeeper) @@ -76,3 +86,17 @@ func (k *Keeper) SetRouter(rtr *porttypes.Router) { k.Router = rtr k.Router.Seal() } + +func checkEmptyKeepers(stakingKeeper clienttypes.StakingKeeper, upgradeKeeper clienttypes.UpgradeKeeper, + scopedKeeper capabilitykeeper.ScopedKeeper) error { + if reflect.DeepEqual(stakingkeeper.Keeper{}, stakingKeeper.(stakingkeeper.Keeper)) { + return fmt.Errorf("empty staking keeper") + } + if reflect.DeepEqual(upgradekeeper.Keeper{}, upgradeKeeper.(upgradekeeper.Keeper)) { + return fmt.Errorf("empty upgradeKeeper") + } + if reflect.DeepEqual(capabilitykeeper.ScopedKeeper{}, scopedKeeper) { + return fmt.Errorf("empty scopedKeeper") + } + return nil +} From 91bd0ab3bbd19eb009bec90940eb78840109711c Mon Sep 17 00:00:00 2001 From: romelukaku Date: Sat, 23 Apr 2022 10:02:52 +0700 Subject: [PATCH 02/10] check for empty exported keepers instead of empty sdk-defined keeper structs --- modules/core/keeper/keeper.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 75ec09d88f6..1eb10ee2828 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -9,8 +9,6 @@ import ( capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" paramtypes "github.com/cosmos/cosmos-sdk/x/params/types" - stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" - upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper" clientkeeper "github.com/cosmos/ibc-go/v3/modules/core/02-client/keeper" clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" connectionkeeper "github.com/cosmos/ibc-go/v3/modules/core/03-connection/keeper" @@ -89,10 +87,10 @@ func (k *Keeper) SetRouter(rtr *porttypes.Router) { func checkEmptyKeepers(stakingKeeper clienttypes.StakingKeeper, upgradeKeeper clienttypes.UpgradeKeeper, scopedKeeper capabilitykeeper.ScopedKeeper) error { - if reflect.DeepEqual(stakingkeeper.Keeper{}, stakingKeeper.(stakingkeeper.Keeper)) { + if reflect.ValueOf(stakingKeeper).IsZero() { return fmt.Errorf("empty staking keeper") } - if reflect.DeepEqual(upgradekeeper.Keeper{}, upgradeKeeper.(upgradekeeper.Keeper)) { + if reflect.ValueOf(upgradeKeeper).IsZero() { return fmt.Errorf("empty upgradeKeeper") } if reflect.DeepEqual(capabilitykeeper.ScopedKeeper{}, scopedKeeper) { From ca101c2eec9f15dd7831c922ebaddd3e62ce65e6 Mon Sep 17 00:00:00 2001 From: khanh <50263489+catShaark@users.noreply.github.com> Date: Mon, 25 Apr 2022 19:08:47 +0700 Subject: [PATCH 03/10] Update modules/core/keeper/keeper.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- modules/core/keeper/keeper.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 1eb10ee2828..f4acc2c7b2d 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -49,9 +49,8 @@ func NewKeeper( paramSpace = paramSpace.WithKeyTable(keyTable) } - err := checkEmptyKeepers(stakingKeeper, upgradeKeeper, scopedKeeper) - if err != nil { - panic(fmt.Errorf("cannot initialize ibc keeper: %w", err)) + if err := checkEmptyKeepers(stakingKeeper, upgradeKeeper, scopedKeeper); err != nil { + panic(fmt.Errorf("cannot initialize IBC keeper: %w", err)) } clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper) From 1911cf3fc524dfc11192df60727c1e37e82e4bcf Mon Sep 17 00:00:00 2001 From: romelukaku Date: Tue, 26 Apr 2022 10:05:56 +0700 Subject: [PATCH 04/10] remove func checkEmptyKeepers(), check empty keepers directly within func NewKeeper() --- modules/core/keeper/keeper.go | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index f4acc2c7b2d..1a61ecff384 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -49,8 +49,13 @@ func NewKeeper( paramSpace = paramSpace.WithKeyTable(keyTable) } - if err := checkEmptyKeepers(stakingKeeper, upgradeKeeper, scopedKeeper); err != nil { - panic(fmt.Errorf("cannot initialize IBC keeper: %w", err)) + // panic if any of the keepers passed in is empty + if reflect.ValueOf(stakingKeeper).IsZero() { + panic(fmt.Errorf("cannot initialize ibc keeper: empty staking keeper")) + } else if reflect.ValueOf(upgradeKeeper).IsZero() { + panic(fmt.Errorf("cannot initialize ibc keeper: empty upgrade keeper")) + } else if reflect.DeepEqual(capabilitykeeper.ScopedKeeper{}, scopedKeeper) { + panic(fmt.Errorf("cannot initialize ibc keeper: empty scoped keeper")) } clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper) @@ -83,17 +88,3 @@ func (k *Keeper) SetRouter(rtr *porttypes.Router) { k.Router = rtr k.Router.Seal() } - -func checkEmptyKeepers(stakingKeeper clienttypes.StakingKeeper, upgradeKeeper clienttypes.UpgradeKeeper, - scopedKeeper capabilitykeeper.ScopedKeeper) error { - if reflect.ValueOf(stakingKeeper).IsZero() { - return fmt.Errorf("empty staking keeper") - } - if reflect.ValueOf(upgradeKeeper).IsZero() { - return fmt.Errorf("empty upgradeKeeper") - } - if reflect.DeepEqual(capabilitykeeper.ScopedKeeper{}, scopedKeeper) { - return fmt.Errorf("empty scopedKeeper") - } - return nil -} From 2c5809153797e298df4d36c0177ac4d0260f2eb3 Mon Sep 17 00:00:00 2001 From: romelukaku Date: Tue, 26 Apr 2022 13:23:13 +0700 Subject: [PATCH 05/10] modules/core/keeper KeeperTestSuite -> MsgServerTestSuite; creat new modules/core/keeper KeeperTestSuite for testing ibckeeper.NewKeeper() --- modules/core/keeper/keeper_test.go | 124 +++++++++++++++++++++++++ modules/core/keeper/msg_server_test.go | 18 ++-- 2 files changed, 132 insertions(+), 10 deletions(-) create mode 100644 modules/core/keeper/keeper_test.go diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go new file mode 100644 index 00000000000..f2a1559d923 --- /dev/null +++ b/modules/core/keeper/keeper_test.go @@ -0,0 +1,124 @@ +package keeper_test + +import ( + "testing" + "time" + + sdk "github.com/cosmos/cosmos-sdk/types" + capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" + "github.com/stretchr/testify/suite" + + stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" + upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper" + clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" + ibchost "github.com/cosmos/ibc-go/v3/modules/core/24-host" + ibckeeper "github.com/cosmos/ibc-go/v3/modules/core/keeper" + ibctesting "github.com/cosmos/ibc-go/v3/testing" + "github.com/cosmos/ibc-go/v3/testing/simapp" +) + +type KeeperTestSuite struct { + suite.Suite + + app *simapp.SimApp + stakingKeeper clienttypes.StakingKeeper + upgradeKeeper clienttypes.UpgradeKeeper + scopedKeeper capabilitykeeper.ScopedKeeper +} + +func (suite *KeeperTestSuite) SetupTest() { + coordinator := ibctesting.NewCoordinator(suite.T(), 1) + chainA := coordinator.GetChain(ibctesting.GetChainID(1)) + + suite.app = chainA.App.(*simapp.SimApp) + + suite.stakingKeeper = suite.app.StakingKeeper + suite.upgradeKeeper = suite.app.UpgradeKeeper + suite.scopedKeeper = suite.app.ScopedIBCKeeper +} + +func (suite *KeeperTestSuite) NewIBCKeeper() { + ibckeeper.NewKeeper( + suite.app.AppCodec(), + suite.app.GetKey(ibchost.StoreKey), + suite.app.GetSubspace(ibchost.ModuleName), + suite.stakingKeeper, + suite.upgradeKeeper, + suite.scopedKeeper, + ) +} + +func TestKeeperTestSuite(t *testing.T) { + suite.Run(t, new(KeeperTestSuite)) +} + +// DummyStakingKeeper implements clienttypes.StakingKeeper used in ibckeeper.NewKeeper +type DummyStakingKeeper struct { + dummyField string +} + +func (d DummyStakingKeeper) GetHistoricalInfo(ctx sdk.Context, height int64) (stakingtypes.HistoricalInfo, bool) { + return stakingtypes.HistoricalInfo{}, true +} + +func (d DummyStakingKeeper) UnbondingTime(ctx sdk.Context) time.Duration { + return 0 +} + +// Test ibckeeper.NewKeeper used to initialize IBCKeeper when creating an app instance. +// It verifies if ibckeeper.NewKeeper panic when any of the keepers passed in is empty. +func (suite *KeeperTestSuite) TestNewKeeper() { + + testCases := []struct { + name string + malleate func() + }{ + {"failure: empty staking keeper", func() { + emptyStakingKeeper := stakingkeeper.Keeper{} + + suite.stakingKeeper = emptyStakingKeeper + + suite.Require().Panics(suite.NewIBCKeeper) + }}, + {"failure: empty dummy staking keeper", func() { + // use a different implementation of clienttypes.StakingKeeper + emptyDummyStakingKeeper := DummyStakingKeeper{} + + suite.stakingKeeper = emptyDummyStakingKeeper + + suite.Require().Panics(suite.NewIBCKeeper) + }}, + {"failure: empty upgrade keeper", func() { + emptyUpgradeKeeper := upgradekeeper.Keeper{} + + suite.upgradeKeeper = emptyUpgradeKeeper + + suite.Require().Panics(suite.NewIBCKeeper) + }}, + {"failure: empty scoped keeper", func() { + emptyScopedKeeper := capabilitykeeper.ScopedKeeper{} + + suite.scopedKeeper = emptyScopedKeeper + + suite.Require().Panics(suite.NewIBCKeeper) + }}, + {"success: replace stakingKeeper with non-empty DummyStakingKeeper", func() { + // use a different implementation of clienttypes.StakingKeeper + dummyStakingKeeper := DummyStakingKeeper{"not empty"} + + suite.stakingKeeper = dummyStakingKeeper + + suite.Require().NotPanics(suite.NewIBCKeeper) + }}, + } + + for _, tc := range testCases { + tc := tc + suite.SetupTest() + + suite.Run(tc.name, func() { + tc.malleate() + }) + } +} diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 1e1933ee724..71dcc1e2a3f 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -18,14 +18,12 @@ import ( ibcmock "github.com/cosmos/ibc-go/v3/testing/mock" ) -const height = 10 - var ( timeoutHeight = clienttypes.NewHeight(0, 10000) maxSequence = uint64(10) ) -type KeeperTestSuite struct { +type MsgServerTestSuite struct { suite.Suite coordinator *ibctesting.Coordinator @@ -35,7 +33,7 @@ type KeeperTestSuite struct { } // SetupTest creates a coordinator with 2 test chains. -func (suite *KeeperTestSuite) SetupTest() { +func (suite *MsgServerTestSuite) SetupTest() { suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) @@ -49,7 +47,7 @@ func (suite *KeeperTestSuite) SetupTest() { } func TestIBCTestSuite(t *testing.T) { - suite.Run(t, new(KeeperTestSuite)) + suite.Run(t, new(MsgServerTestSuite)) } // tests the IBC handler receiving a packet on ordered and unordered channels. @@ -57,7 +55,7 @@ func TestIBCTestSuite(t *testing.T) { // tests high level properties like ordering and basic sanity checks. More // rigorous testing of 'RecvPacket' can be found in the // 04-channel/keeper/packet_test.go. -func (suite *KeeperTestSuite) TestHandleRecvPacket() { +func (suite *MsgServerTestSuite) TestHandleRecvPacket() { var ( packet channeltypes.Packet path *ibctesting.Path @@ -229,7 +227,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() { // occurs. It test high level properties like ordering and basic sanity // checks. More rigorous testing of 'AcknowledgePacket' // can be found in the 04-channel/keeper/packet_test.go. -func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() { +func (suite *MsgServerTestSuite) TestHandleAcknowledgePacket() { var ( packet channeltypes.Packet path *ibctesting.Path @@ -375,7 +373,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() { // high level properties like ordering and basic sanity checks. More // rigorous testing of 'TimeoutPacket' and 'TimeoutExecuted' can be found in // the 04-channel/keeper/timeout_test.go. -func (suite *KeeperTestSuite) TestHandleTimeoutPacket() { +func (suite *MsgServerTestSuite) TestHandleTimeoutPacket() { var ( packet channeltypes.Packet packetKey []byte @@ -506,7 +504,7 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() { // commitment occurs. It tests high level properties like ordering and basic // sanity checks. More rigorous testing of 'TimeoutOnClose' and //'TimeoutExecuted' can be found in the 04-channel/keeper/timeout_test.go. -func (suite *KeeperTestSuite) TestHandleTimeoutOnClosePacket() { +func (suite *MsgServerTestSuite) TestHandleTimeoutOnClosePacket() { var ( packet channeltypes.Packet packetKey []byte @@ -657,7 +655,7 @@ func (suite *KeeperTestSuite) TestHandleTimeoutOnClosePacket() { } } -func (suite *KeeperTestSuite) TestUpgradeClient() { +func (suite *MsgServerTestSuite) TestUpgradeClient() { var ( path *ibctesting.Path upgradedClient exported.ClientState From 96a10c0bdf6a0f84499711cfb42fbf4dab2e6ea8 Mon Sep 17 00:00:00 2001 From: romelukaku Date: Tue, 26 Apr 2022 13:43:41 +0700 Subject: [PATCH 06/10] update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b84cecfd72..d9f9a9c8bbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (modules/core/keeper) [\#1284](https://github.com/cosmos/ibc-go/pull/1284) Add sanity check for the keepers passed into `ibckeeper.NewKeeper`. `ibckeeper.NewKeeper` now panics if any of the keepers passed in is empty. * (middleware) [\#1022](https://github.com/cosmos/ibc-go/pull/1022) Add `GetAppVersion` to the ICS4Wrapper interface. This function should be used by IBC applications to obtain their own version since the version set in the channel structure may be wrapped many times by middleware. * (modules/core/04-channel) [\#1160](https://github.com/cosmos/ibc-go/pull/1160) Improve `uint64 -> string` performance in `Logger`. * (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file. From 18c46ef7cbeff9669a963a4e6ab2168d784ffd07 Mon Sep 17 00:00:00 2001 From: romelukaku Date: Tue, 26 Apr 2022 19:24:46 +0700 Subject: [PATCH 07/10] DummyStakingKeeper -> MockStakingKeeper --- modules/core/keeper/keeper_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go index f2a1559d923..29b61c5c058 100644 --- a/modules/core/keeper/keeper_test.go +++ b/modules/core/keeper/keeper_test.go @@ -53,16 +53,16 @@ func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } -// DummyStakingKeeper implements clienttypes.StakingKeeper used in ibckeeper.NewKeeper -type DummyStakingKeeper struct { - dummyField string +// MockStakingKeeper implements clienttypes.StakingKeeper used in ibckeeper.NewKeeper +type MockStakingKeeper struct { + mockField string } -func (d DummyStakingKeeper) GetHistoricalInfo(ctx sdk.Context, height int64) (stakingtypes.HistoricalInfo, bool) { +func (d MockStakingKeeper) GetHistoricalInfo(ctx sdk.Context, height int64) (stakingtypes.HistoricalInfo, bool) { return stakingtypes.HistoricalInfo{}, true } -func (d DummyStakingKeeper) UnbondingTime(ctx sdk.Context) time.Duration { +func (d MockStakingKeeper) UnbondingTime(ctx sdk.Context) time.Duration { return 0 } @@ -83,9 +83,9 @@ func (suite *KeeperTestSuite) TestNewKeeper() { }}, {"failure: empty dummy staking keeper", func() { // use a different implementation of clienttypes.StakingKeeper - emptyDummyStakingKeeper := DummyStakingKeeper{} + emptyMockStakingKeeper := MockStakingKeeper{} - suite.stakingKeeper = emptyDummyStakingKeeper + suite.stakingKeeper = emptyMockStakingKeeper suite.Require().Panics(suite.NewIBCKeeper) }}, @@ -103,11 +103,11 @@ func (suite *KeeperTestSuite) TestNewKeeper() { suite.Require().Panics(suite.NewIBCKeeper) }}, - {"success: replace stakingKeeper with non-empty DummyStakingKeeper", func() { + {"success: replace stakingKeeper with non-empty MockStakingKeeper", func() { // use a different implementation of clienttypes.StakingKeeper - dummyStakingKeeper := DummyStakingKeeper{"not empty"} + mockStakingKeeper := MockStakingKeeper{"not empty"} - suite.stakingKeeper = dummyStakingKeeper + suite.stakingKeeper = mockStakingKeeper suite.Require().NotPanics(suite.NewIBCKeeper) }}, From e31cc1e64c569481aff08a5e0b5dd59ecf6df42f Mon Sep 17 00:00:00 2001 From: romelukaku Date: Tue, 26 Apr 2022 20:07:21 +0700 Subject: [PATCH 08/10] refactor modules/core/keeper test --- modules/core/keeper/keeper_test.go | 92 +++++++++++++++----------- modules/core/keeper/msg_server_test.go | 40 ++--------- 2 files changed, 58 insertions(+), 74 deletions(-) diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go index 29b61c5c058..1c212a200ed 100644 --- a/modules/core/keeper/keeper_test.go +++ b/modules/core/keeper/keeper_test.go @@ -15,38 +15,27 @@ import ( ibchost "github.com/cosmos/ibc-go/v3/modules/core/24-host" ibckeeper "github.com/cosmos/ibc-go/v3/modules/core/keeper" ibctesting "github.com/cosmos/ibc-go/v3/testing" - "github.com/cosmos/ibc-go/v3/testing/simapp" ) type KeeperTestSuite struct { suite.Suite - app *simapp.SimApp - stakingKeeper clienttypes.StakingKeeper - upgradeKeeper clienttypes.UpgradeKeeper - scopedKeeper capabilitykeeper.ScopedKeeper + coordinator *ibctesting.Coordinator + + chainA *ibctesting.TestChain + chainB *ibctesting.TestChain } func (suite *KeeperTestSuite) SetupTest() { - coordinator := ibctesting.NewCoordinator(suite.T(), 1) - chainA := coordinator.GetChain(ibctesting.GetChainID(1)) - - suite.app = chainA.App.(*simapp.SimApp) + suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) - suite.stakingKeeper = suite.app.StakingKeeper - suite.upgradeKeeper = suite.app.UpgradeKeeper - suite.scopedKeeper = suite.app.ScopedIBCKeeper -} + suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) + suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2)) -func (suite *KeeperTestSuite) NewIBCKeeper() { - ibckeeper.NewKeeper( - suite.app.AppCodec(), - suite.app.GetKey(ibchost.StoreKey), - suite.app.GetSubspace(ibchost.ModuleName), - suite.stakingKeeper, - suite.upgradeKeeper, - suite.scopedKeeper, - ) + // TODO: remove + // commit some blocks so that QueryProof returns valid proof (cannot return valid query if height <= 1) + suite.coordinator.CommitNBlocks(suite.chainA, 2) + suite.coordinator.CommitNBlocks(suite.chainB, 2) } func TestKeeperTestSuite(t *testing.T) { @@ -69,48 +58,57 @@ func (d MockStakingKeeper) UnbondingTime(ctx sdk.Context) time.Duration { // Test ibckeeper.NewKeeper used to initialize IBCKeeper when creating an app instance. // It verifies if ibckeeper.NewKeeper panic when any of the keepers passed in is empty. func (suite *KeeperTestSuite) TestNewKeeper() { + var ( + stakingKeeper clienttypes.StakingKeeper + upgradeKeeper clienttypes.UpgradeKeeper + scopedKeeper capabilitykeeper.ScopedKeeper + newIBCKeeper = func() { + ibckeeper.NewKeeper( + suite.chainA.GetSimApp().AppCodec(), + suite.chainA.GetSimApp().GetKey(ibchost.StoreKey), + suite.chainA.GetSimApp().GetSubspace(ibchost.ModuleName), + stakingKeeper, + upgradeKeeper, + scopedKeeper, + ) + } + ) testCases := []struct { name string malleate func() + expPass bool }{ {"failure: empty staking keeper", func() { emptyStakingKeeper := stakingkeeper.Keeper{} - suite.stakingKeeper = emptyStakingKeeper + stakingKeeper = emptyStakingKeeper - suite.Require().Panics(suite.NewIBCKeeper) - }}, + }, false}, {"failure: empty dummy staking keeper", func() { // use a different implementation of clienttypes.StakingKeeper emptyMockStakingKeeper := MockStakingKeeper{} - suite.stakingKeeper = emptyMockStakingKeeper + stakingKeeper = emptyMockStakingKeeper - suite.Require().Panics(suite.NewIBCKeeper) - }}, + }, false}, {"failure: empty upgrade keeper", func() { emptyUpgradeKeeper := upgradekeeper.Keeper{} - suite.upgradeKeeper = emptyUpgradeKeeper + upgradeKeeper = emptyUpgradeKeeper - suite.Require().Panics(suite.NewIBCKeeper) - }}, + }, false}, {"failure: empty scoped keeper", func() { emptyScopedKeeper := capabilitykeeper.ScopedKeeper{} - suite.scopedKeeper = emptyScopedKeeper - - suite.Require().Panics(suite.NewIBCKeeper) - }}, + scopedKeeper = emptyScopedKeeper + }, false}, {"success: replace stakingKeeper with non-empty MockStakingKeeper", func() { // use a different implementation of clienttypes.StakingKeeper mockStakingKeeper := MockStakingKeeper{"not empty"} - suite.stakingKeeper = mockStakingKeeper - - suite.Require().NotPanics(suite.NewIBCKeeper) - }}, + stakingKeeper = mockStakingKeeper + }, true}, } for _, tc := range testCases { @@ -118,7 +116,23 @@ func (suite *KeeperTestSuite) TestNewKeeper() { suite.SetupTest() suite.Run(tc.name, func() { + + stakingKeeper = suite.chainA.GetSimApp().StakingKeeper + upgradeKeeper = suite.chainA.GetSimApp().UpgradeKeeper + scopedKeeper = suite.chainA.GetSimApp().ScopedIBCKeeper + tc.malleate() + + if tc.expPass { + suite.Require().NotPanics( + newIBCKeeper, + ) + } else { + suite.Require().Panics( + newIBCKeeper, + ) + } + }) } } diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 71dcc1e2a3f..7e01241be81 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1,11 +1,8 @@ package keeper_test import ( - "testing" - sdk "github.com/cosmos/cosmos-sdk/types" upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" - "github.com/stretchr/testify/suite" clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" @@ -23,39 +20,12 @@ var ( maxSequence = uint64(10) ) -type MsgServerTestSuite struct { - suite.Suite - - coordinator *ibctesting.Coordinator - - chainA *ibctesting.TestChain - chainB *ibctesting.TestChain -} - -// SetupTest creates a coordinator with 2 test chains. -func (suite *MsgServerTestSuite) SetupTest() { - suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) - - suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) - suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2)) - - // TODO: remove - // commit some blocks so that QueryProof returns valid proof (cannot return valid query if height <= 1) - suite.coordinator.CommitNBlocks(suite.chainA, 2) - suite.coordinator.CommitNBlocks(suite.chainB, 2) - -} - -func TestIBCTestSuite(t *testing.T) { - suite.Run(t, new(MsgServerTestSuite)) -} - // tests the IBC handler receiving a packet on ordered and unordered channels. // It verifies that the storing of an acknowledgement on success occurs. It // tests high level properties like ordering and basic sanity checks. More // rigorous testing of 'RecvPacket' can be found in the // 04-channel/keeper/packet_test.go. -func (suite *MsgServerTestSuite) TestHandleRecvPacket() { +func (suite *KeeperTestSuite) TestHandleRecvPacket() { var ( packet channeltypes.Packet path *ibctesting.Path @@ -227,7 +197,7 @@ func (suite *MsgServerTestSuite) TestHandleRecvPacket() { // occurs. It test high level properties like ordering and basic sanity // checks. More rigorous testing of 'AcknowledgePacket' // can be found in the 04-channel/keeper/packet_test.go. -func (suite *MsgServerTestSuite) TestHandleAcknowledgePacket() { +func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() { var ( packet channeltypes.Packet path *ibctesting.Path @@ -373,7 +343,7 @@ func (suite *MsgServerTestSuite) TestHandleAcknowledgePacket() { // high level properties like ordering and basic sanity checks. More // rigorous testing of 'TimeoutPacket' and 'TimeoutExecuted' can be found in // the 04-channel/keeper/timeout_test.go. -func (suite *MsgServerTestSuite) TestHandleTimeoutPacket() { +func (suite *KeeperTestSuite) TestHandleTimeoutPacket() { var ( packet channeltypes.Packet packetKey []byte @@ -504,7 +474,7 @@ func (suite *MsgServerTestSuite) TestHandleTimeoutPacket() { // commitment occurs. It tests high level properties like ordering and basic // sanity checks. More rigorous testing of 'TimeoutOnClose' and //'TimeoutExecuted' can be found in the 04-channel/keeper/timeout_test.go. -func (suite *MsgServerTestSuite) TestHandleTimeoutOnClosePacket() { +func (suite *KeeperTestSuite) TestHandleTimeoutOnClosePacket() { var ( packet channeltypes.Packet packetKey []byte @@ -655,7 +625,7 @@ func (suite *MsgServerTestSuite) TestHandleTimeoutOnClosePacket() { } } -func (suite *MsgServerTestSuite) TestUpgradeClient() { +func (suite *KeeperTestSuite) TestUpgradeClient() { var ( path *ibctesting.Path upgradedClient exported.ClientState From 258637c1a1120a98a3af367292197d4ff1a6d8b9 Mon Sep 17 00:00:00 2001 From: khanh <50263489+catShaark@users.noreply.github.com> Date: Wed, 27 Apr 2022 23:01:20 +0700 Subject: [PATCH 09/10] Update modules/core/keeper/keeper_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- modules/core/keeper/keeper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go index 1c212a200ed..d40d1ab34c5 100644 --- a/modules/core/keeper/keeper_test.go +++ b/modules/core/keeper/keeper_test.go @@ -85,7 +85,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() { stakingKeeper = emptyStakingKeeper }, false}, - {"failure: empty dummy staking keeper", func() { + {"failure: empty mock staking keeper", func() { // use a different implementation of clienttypes.StakingKeeper emptyMockStakingKeeper := MockStakingKeeper{} From a5f9c6dbf1a4723270ed2bbcb5a5da8c0053a26a Mon Sep 17 00:00:00 2001 From: khanh <50263489+catShaark@users.noreply.github.com> Date: Wed, 27 Apr 2022 23:01:28 +0700 Subject: [PATCH 10/10] Update modules/core/keeper/keeper.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- modules/core/keeper/keeper.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 1a61ecff384..3a6cc5cb23b 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -51,11 +51,15 @@ func NewKeeper( // panic if any of the keepers passed in is empty if reflect.ValueOf(stakingKeeper).IsZero() { - panic(fmt.Errorf("cannot initialize ibc keeper: empty staking keeper")) - } else if reflect.ValueOf(upgradeKeeper).IsZero() { - panic(fmt.Errorf("cannot initialize ibc keeper: empty upgrade keeper")) - } else if reflect.DeepEqual(capabilitykeeper.ScopedKeeper{}, scopedKeeper) { - panic(fmt.Errorf("cannot initialize ibc keeper: empty scoped keeper")) + panic(fmt.Errorf("cannot initialize IBC keeper: empty staking keeper")) + } + + if reflect.ValueOf(upgradeKeeper).IsZero() { + panic(fmt.Errorf("cannot initialize IBC keeper: empty upgrade keeper")) + } + + if reflect.DeepEqual(capabilitykeeper.ScopedKeeper{}, scopedKeeper) { + panic(fmt.Errorf("cannot initialize IBC keeper: empty scoped keeper")) } clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper)