add empty keepers checking in ibc NewKeeper

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. 

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,15 @@ 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"))
+	} 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)
 	connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, clientKeeper)
 	portKeeper := portkeeper.NewKeeper(scopedKeeper)

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()
+		})
+	}
+}

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,15 +47,15 @@ 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.
 // 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 *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