diff --git a/CHANGELOG.md b/CHANGELOG.md index 7af77f82ebd..a6880b9f6e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements * (modules/core/04-channel) [\#1160](https://github.com/cosmos/ibc-go/pull/1160) Improve `uint64 -> string` performance in `Logger`. +* (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. ### Features diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 69044e9e4ea..3a6cc5cb23b 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -1,6 +1,9 @@ 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" @@ -46,6 +49,19 @@ func NewKeeper( paramSpace = paramSpace.WithKeyTable(keyTable) } + // 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")) + } + + 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) 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 new file mode 100644 index 00000000000..d40d1ab34c5 --- /dev/null +++ b/modules/core/keeper/keeper_test.go @@ -0,0 +1,138 @@ +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" +) + +type KeeperTestSuite struct { + suite.Suite + + coordinator *ibctesting.Coordinator + + chainA *ibctesting.TestChain + chainB *ibctesting.TestChain +} + +func (suite *KeeperTestSuite) 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 TestKeeperTestSuite(t *testing.T) { + suite.Run(t, new(KeeperTestSuite)) +} + +// MockStakingKeeper implements clienttypes.StakingKeeper used in ibckeeper.NewKeeper +type MockStakingKeeper struct { + mockField string +} + +func (d MockStakingKeeper) GetHistoricalInfo(ctx sdk.Context, height int64) (stakingtypes.HistoricalInfo, bool) { + return stakingtypes.HistoricalInfo{}, true +} + +func (d MockStakingKeeper) 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() { + 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{} + + stakingKeeper = emptyStakingKeeper + + }, false}, + {"failure: empty mock staking keeper", func() { + // use a different implementation of clienttypes.StakingKeeper + emptyMockStakingKeeper := MockStakingKeeper{} + + stakingKeeper = emptyMockStakingKeeper + + }, false}, + {"failure: empty upgrade keeper", func() { + emptyUpgradeKeeper := upgradekeeper.Keeper{} + + upgradeKeeper = emptyUpgradeKeeper + + }, false}, + {"failure: empty scoped keeper", func() { + emptyScopedKeeper := capabilitykeeper.ScopedKeeper{} + + scopedKeeper = emptyScopedKeeper + }, false}, + {"success: replace stakingKeeper with non-empty MockStakingKeeper", func() { + // use a different implementation of clienttypes.StakingKeeper + mockStakingKeeper := MockStakingKeeper{"not empty"} + + stakingKeeper = mockStakingKeeper + }, true}, + } + + for _, tc := range testCases { + tc := tc + 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 1e1933ee724..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" @@ -18,40 +15,11 @@ 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 { - suite.Suite - - coordinator *ibctesting.Coordinator - - chainA *ibctesting.TestChain - chainB *ibctesting.TestChain -} - -// SetupTest creates a coordinator with 2 test chains. -func (suite *KeeperTestSuite) 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(KeeperTestSuite)) -} - // 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