Skip to content

Commit

Permalink
add empty keepers checking in ibc NewKeeper (backport #1284) (#1375)
Browse files Browse the repository at this point in the history
* 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 <25233464+colin-axner@users.noreply.github.com>

* 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 <25233464+colin-axner@users.noreply.github.com>

* Update modules/core/keeper/keeper.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
(cherry picked from commit f2577f9)

# Conflicts:
#	CHANGELOG.md
#	modules/core/keeper/msg_server_test.go

* fix conflict

* fix conflict

* fix merge

* fix tests

* imports formatting

* Update CHANGELOG.md

Co-authored-by: khanh <50263489+catShaark@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
  • Loading branch information
3 people authored May 18, 2022
1 parent 2504e42 commit 8d5fcff
Show file tree
Hide file tree
Showing 4 changed files with 157 additions and 33 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
16 changes: 16 additions & 0 deletions modules/core/keeper/keeper.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -45,6 +48,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)
Expand Down
139 changes: 139 additions & 0 deletions modules/core/keeper/keeper_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package keeper_test

import (
"testing"
"time"

"github.com/stretchr/testify/suite"

sdk "github.com/cosmos/cosmos-sdk/types"
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
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/modules/core/02-client/types"
ibchost "github.com/cosmos/ibc-go/modules/core/24-host"
ibckeeper "github.com/cosmos/ibc-go/modules/core/keeper"
ibctesting "github.com/cosmos/ibc-go/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(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))

// 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,
)
}

})
}
}
34 changes: 1 addition & 33 deletions modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
package keeper_test

import (
"testing"

"github.com/stretchr/testify/suite"

sdk "github.com/cosmos/cosmos-sdk/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
commitmenttypes "github.com/cosmos/ibc-go/modules/core/23-commitment/types"
Expand All @@ -18,40 +15,11 @@ import (
ibcmock "github.com/cosmos/ibc-go/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(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))

// 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
Expand Down

0 comments on commit 8d5fcff

Please sign in to comment.