Skip to content

Commit ab9f45d

Browse files
catShaarkmergify[bot]
catShaark
authored and
mergify[bot]
committed
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
1 parent acceaed commit ab9f45d

File tree

4 files changed

+172
-2
lines changed

4 files changed

+172
-2
lines changed

‎CHANGELOG.md

+10
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ Ref: https://keepachangelog.com/en/1.0.0/
4646

4747
### Improvements
4848

49+
<<<<<<< HEAD
50+
=======
51+
* (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.
52+
* (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.
53+
* (modules/core/04-channel) [\#1160](https://github.com/cosmos/ibc-go/pull/1160) Improve `uint64 -> string` performance in `Logger`.
54+
* (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file.
55+
* (modules/core/04-channel) [\#1279](https://github.com/cosmos/ibc-go/pull/1279) Add selected channel version to MsgChanOpenInitResponse and MsgChanOpenTryResponse. Emit channel version during OpenInit/OpenTry
56+
* (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1`
57+
58+
>>>>>>> f2577f9 (add empty keepers checking in ibc NewKeeper (#1284))
4959
### Features
5060

5161
### Bug Fixes

‎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

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

‎modules/core/keeper/msg_server_test.go

+8-2
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
package keeper_test
22

33
import (
4+
<<<<<<< HEAD
45
"testing"
56

67
"github.com/stretchr/testify/suite"
8+
=======
9+
sdk "github.com/cosmos/cosmos-sdk/types"
10+
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
11+
>>>>>>> f2577f9 (add empty keepers checking in ibc NewKeeper (#1284))
712

813
sdk "github.com/cosmos/cosmos-sdk/types"
914
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
@@ -18,13 +23,12 @@ import (
1823
ibcmock "github.com/cosmos/ibc-go/testing/mock"
1924
)
2025

21-
const height = 10
22-
2326
var (
2427
timeoutHeight = clienttypes.NewHeight(0, 10000)
2528
maxSequence = uint64(10)
2629
)
2730

31+
<<<<<<< HEAD
2832
type KeeperTestSuite struct {
2933
suite.Suite
3034

@@ -52,6 +56,8 @@ func TestIBCTestSuite(t *testing.T) {
5256
suite.Run(t, new(KeeperTestSuite))
5357
}
5458

59+
=======
60+
>>>>>>> f2577f9 (add empty keepers checking in ibc NewKeeper (#1284))
5561
// tests the IBC handler receiving a packet on ordered and unordered channels.
5662
// It verifies that the storing of an acknowledgement on success occurs. It
5763
// tests high level properties like ordering and basic sanity checks. More

0 commit comments

Comments
 (0)