Skip to content

Commit 40d2a74

Browse files
mergify[bot]catShaarkcrodriguezvega
authored
add empty keepers checking in ibc NewKeeper (backport #1284) (#1378)
* 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 tests * Update CHANGELOG.md Co-authored-by: khanh <50263489+catShaark@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
1 parent a56c313 commit 40d2a74

File tree

4 files changed

+157
-33
lines changed

4 files changed

+157
-33
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
4747
### Improvements
4848

4949
* (modules/core/04-channel) [\#1130](https://github.com/cosmos/ibc-go/pull/1130) Call `packet.GetSequence()` rather than passing func in `WriteAcknowledgement` log output
50+
* (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.
5051

5152
### Features
5253

‎modules/core/keeper/keeper.go

+16
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package keeper
22

33
import (
4+
"fmt"
5+
"reflect"
6+
47
"github.com/cosmos/cosmos-sdk/codec"
58
sdk "github.com/cosmos/cosmos-sdk/types"
69
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
@@ -45,6 +48,19 @@ func NewKeeper(
4548
paramSpace = paramSpace.WithKeyTable(keyTable)
4649
}
4750

51+
// panic if any of the keepers passed in is empty
52+
if reflect.ValueOf(stakingKeeper).IsZero() {
53+
panic(fmt.Errorf("cannot initialize IBC keeper: empty staking keeper"))
54+
}
55+
56+
if reflect.ValueOf(upgradeKeeper).IsZero() {
57+
panic(fmt.Errorf("cannot initialize IBC keeper: empty upgrade keeper"))
58+
}
59+
60+
if reflect.DeepEqual(capabilitykeeper.ScopedKeeper{}, scopedKeeper) {
61+
panic(fmt.Errorf("cannot initialize IBC keeper: empty scoped keeper"))
62+
}
63+
4864
clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper)
4965
connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, clientKeeper)
5066
portKeeper := portkeeper.NewKeeper(scopedKeeper)

‎modules/core/keeper/keeper_test.go

+139
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
package keeper_test
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/stretchr/testify/suite"
8+
9+
sdk "github.com/cosmos/cosmos-sdk/types"
10+
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
11+
stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper"
12+
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
13+
upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper"
14+
15+
clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types"
16+
ibchost "github.com/cosmos/ibc-go/v2/modules/core/24-host"
17+
ibckeeper "github.com/cosmos/ibc-go/v2/modules/core/keeper"
18+
ibctesting "github.com/cosmos/ibc-go/v2/testing"
19+
)
20+
21+
type KeeperTestSuite struct {
22+
suite.Suite
23+
24+
coordinator *ibctesting.Coordinator
25+
26+
chainA *ibctesting.TestChain
27+
chainB *ibctesting.TestChain
28+
}
29+
30+
func (suite *KeeperTestSuite) SetupTest() {
31+
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
32+
33+
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
34+
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
35+
36+
// TODO: remove
37+
// commit some blocks so that QueryProof returns valid proof (cannot return valid query if height <= 1)
38+
suite.coordinator.CommitNBlocks(suite.chainA, 2)
39+
suite.coordinator.CommitNBlocks(suite.chainB, 2)
40+
}
41+
42+
func TestKeeperTestSuite(t *testing.T) {
43+
suite.Run(t, new(KeeperTestSuite))
44+
}
45+
46+
// MockStakingKeeper implements clienttypes.StakingKeeper used in ibckeeper.NewKeeper
47+
type MockStakingKeeper struct {
48+
mockField string
49+
}
50+
51+
func (d MockStakingKeeper) GetHistoricalInfo(ctx sdk.Context, height int64) (stakingtypes.HistoricalInfo, bool) {
52+
return stakingtypes.HistoricalInfo{}, true
53+
}
54+
55+
func (d MockStakingKeeper) UnbondingTime(ctx sdk.Context) time.Duration {
56+
return 0
57+
}
58+
59+
// Test ibckeeper.NewKeeper used to initialize IBCKeeper when creating an app instance.
60+
// It verifies if ibckeeper.NewKeeper panic when any of the keepers passed in is empty.
61+
func (suite *KeeperTestSuite) TestNewKeeper() {
62+
var (
63+
stakingKeeper clienttypes.StakingKeeper
64+
upgradeKeeper clienttypes.UpgradeKeeper
65+
scopedKeeper capabilitykeeper.ScopedKeeper
66+
newIBCKeeper = func() {
67+
ibckeeper.NewKeeper(
68+
suite.chainA.GetSimApp().AppCodec(),
69+
suite.chainA.GetSimApp().GetKey(ibchost.StoreKey),
70+
suite.chainA.GetSimApp().GetSubspace(ibchost.ModuleName),
71+
stakingKeeper,
72+
upgradeKeeper,
73+
scopedKeeper,
74+
)
75+
}
76+
)
77+
78+
testCases := []struct {
79+
name string
80+
malleate func()
81+
expPass bool
82+
}{
83+
{"failure: empty staking keeper", func() {
84+
emptyStakingKeeper := stakingkeeper.Keeper{}
85+
86+
stakingKeeper = emptyStakingKeeper
87+
88+
}, false},
89+
{"failure: empty mock staking keeper", func() {
90+
// use a different implementation of clienttypes.StakingKeeper
91+
emptyMockStakingKeeper := MockStakingKeeper{}
92+
93+
stakingKeeper = emptyMockStakingKeeper
94+
95+
}, false},
96+
{"failure: empty upgrade keeper", func() {
97+
emptyUpgradeKeeper := upgradekeeper.Keeper{}
98+
99+
upgradeKeeper = emptyUpgradeKeeper
100+
101+
}, false},
102+
{"failure: empty scoped keeper", func() {
103+
emptyScopedKeeper := capabilitykeeper.ScopedKeeper{}
104+
105+
scopedKeeper = emptyScopedKeeper
106+
}, false},
107+
{"success: replace stakingKeeper with non-empty MockStakingKeeper", func() {
108+
// use a different implementation of clienttypes.StakingKeeper
109+
mockStakingKeeper := MockStakingKeeper{"not empty"}
110+
111+
stakingKeeper = mockStakingKeeper
112+
}, true},
113+
}
114+
115+
for _, tc := range testCases {
116+
tc := tc
117+
suite.SetupTest()
118+
119+
suite.Run(tc.name, func() {
120+
121+
stakingKeeper = suite.chainA.GetSimApp().StakingKeeper
122+
upgradeKeeper = suite.chainA.GetSimApp().UpgradeKeeper
123+
scopedKeeper = suite.chainA.GetSimApp().ScopedIBCKeeper
124+
125+
tc.malleate()
126+
127+
if tc.expPass {
128+
suite.Require().NotPanics(
129+
newIBCKeeper,
130+
)
131+
} else {
132+
suite.Require().Panics(
133+
newIBCKeeper,
134+
)
135+
}
136+
137+
})
138+
}
139+
}

‎modules/core/keeper/msg_server_test.go

+1-33
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,9 @@
11
package keeper_test
22

33
import (
4-
"testing"
5-
6-
"github.com/stretchr/testify/suite"
7-
84
sdk "github.com/cosmos/cosmos-sdk/types"
95
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
6+
107
clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types"
118
channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
129
commitmenttypes "github.com/cosmos/ibc-go/v2/modules/core/23-commitment/types"
@@ -18,40 +15,11 @@ import (
1815
ibcmock "github.com/cosmos/ibc-go/v2/testing/mock"
1916
)
2017

21-
const height = 10
22-
2318
var (
2419
timeoutHeight = clienttypes.NewHeight(0, 10000)
2520
maxSequence = uint64(10)
2621
)
2722

28-
type KeeperTestSuite struct {
29-
suite.Suite
30-
31-
coordinator *ibctesting.Coordinator
32-
33-
chainA *ibctesting.TestChain
34-
chainB *ibctesting.TestChain
35-
}
36-
37-
// SetupTest creates a coordinator with 2 test chains.
38-
func (suite *KeeperTestSuite) SetupTest() {
39-
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
40-
41-
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
42-
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
43-
44-
// TODO: remove
45-
// commit some blocks so that QueryProof returns valid proof (cannot return valid query if height <= 1)
46-
suite.coordinator.CommitNBlocks(suite.chainA, 2)
47-
suite.coordinator.CommitNBlocks(suite.chainB, 2)
48-
49-
}
50-
51-
func TestIBCTestSuite(t *testing.T) {
52-
suite.Run(t, new(KeeperTestSuite))
53-
}
54-
5523
// tests the IBC handler receiving a packet on ordered and unordered channels.
5624
// It verifies that the storing of an acknowledgement on success occurs. It
5725
// tests high level properties like ordering and basic sanity checks. More

0 commit comments

Comments
 (0)