add empty keepers checking in ibc NewKeeper (backport #1284) (#1382)

* add empty keepers checking in ibc NewKeeper (#1284) * add empty keepers checking in ibc NewKeeper * check for empty exported keepers instead of empty sdk-defined keeper structs * Update modules/core/keeper/keeper.go Co-authored-by: colin axnér <[email protected]> * remove func checkEmptyKeepers(), check empty keepers directly within func NewKeeper() * modules/core/keeper KeeperTestSuite -> MsgServerTestSuite; creat new modules/core/keeper KeeperTestSuite for testing ibckeeper.NewKeeper() * update CHANGELOG.md * DummyStakingKeeper -> MockStakingKeeper * refactor modules/core/keeper test * Update modules/core/keeper/keeper_test.go Co-authored-by: colin axnér <[email protected]> * Update modules/core/keeper/keeper.go Co-authored-by: colin axnér <[email protected]> Co-authored-by: colin axnér <[email protected]> (cherry picked from commit f2577f9) # Conflicts: # CHANGELOG.md * fix conflict * fix conflict Co-authored-by: khanh <[email protected]> Co-authored-by: crodriguezvega <[email protected]>
cosmos · May 18, 2022 · 4438d4e · 4438d4e
1 parent 996fa1c
commit 4438d4e
Show file tree

Hide file tree

Showing 4 changed files with 155 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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