From adfa1151b4e0e107db39d65218cb6d2815dd21d6 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 3 Apr 2024 15:37:36 +0100 Subject: [PATCH 1/3] chore: change signature of NewKeeper to directly accept a ConsensusHost --- CHANGELOG.md | 1 + modules/apps/callbacks/testing/simapp/app.go | 2 +- modules/core/02-client/keeper/keeper.go | 4 +-- modules/core/keeper/keeper.go | 9 ++--- modules/core/keeper/keeper_test.go | 33 +++++++------------ .../08-wasm/testing/simapp/app.go | 2 +- testing/simapp/app.go | 2 +- 7 files changed, 23 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7037ac65c1..33610861d46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (core/02-client, light-clients) [\#5806](https://github.com/cosmos/ibc-go/pull/5806) Decouple light client routing from their encoding structure. * (core/04-channel) [\#5991](https://github.com/cosmos/ibc-go/pull/5991) The client CLI `QueryLatestConsensusState` has been removed. * (light-clients/06-solomachine) [\#6037](https://github.com/cosmos/ibc-go/pull/6037) Remove `Initialize` function from `ClientState` and move logic to `Initialize` function of `LightClientModule`. +* (core/02-client) [\#XXXX](https://github.com/cosmos/ibc-go/pull/XXXX) Removed `stakingKeeper` as an argument to `NewKeeper` and replaced with a `ConsensusHost` implementation. ### State Machine Breaking diff --git a/modules/apps/callbacks/testing/simapp/app.go b/modules/apps/callbacks/testing/simapp/app.go index d58ca8ef669..26de8d3e158 100644 --- a/modules/apps/callbacks/testing/simapp/app.go +++ b/modules/apps/callbacks/testing/simapp/app.go @@ -407,7 +407,7 @@ func NewSimApp( app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, runtime.NewKVStoreService(keys[upgradetypes.StoreKey]), appCodec, homePath, app.BaseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String()) app.IBCKeeper = ibckeeper.NewKeeper( - appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), + appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), ibctm.NewConsensusHost(app.StakingKeeper), app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) // NOTE: The mock ContractKeeper is only created for testing. diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 1e5c6f0ca50..ff8a54a7f29 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -33,7 +33,7 @@ type Keeper struct { } // NewKeeper creates a new NewKeeper instance -func NewKeeper(cdc codec.BinaryCodec, key storetypes.StoreKey, legacySubspace types.ParamSubspace, sk types.StakingKeeper, uk types.UpgradeKeeper) Keeper { +func NewKeeper(cdc codec.BinaryCodec, key storetypes.StoreKey, legacySubspace types.ParamSubspace, consensusHost types.ConsensusHost, uk types.UpgradeKeeper) Keeper { router := types.NewRouter(key) localhostModule := localhost.NewLightClientModule(cdc, key) router.AddRoute(exported.Localhost, localhostModule) @@ -42,7 +42,7 @@ func NewKeeper(cdc codec.BinaryCodec, key storetypes.StoreKey, legacySubspace ty storeKey: key, cdc: cdc, router: router, - consensusHost: ibctm.NewConsensusHost(sk), + consensusHost: consensusHost, legacySubspace: legacySubspace, upgradeKeeper: uk, } diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index d5982a6faa6..093c8313e9b 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -40,13 +40,14 @@ type Keeper struct { // NewKeeper creates a new ibc Keeper func NewKeeper( cdc codec.BinaryCodec, key storetypes.StoreKey, paramSpace types.ParamSubspace, - stakingKeeper clienttypes.StakingKeeper, upgradeKeeper clienttypes.UpgradeKeeper, + consensusHost clienttypes.ConsensusHost, upgradeKeeper clienttypes.UpgradeKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, authority string, ) *Keeper { // panic if any of the keepers passed in is empty - if isEmpty(stakingKeeper) { - panic(errors.New("cannot initialize IBC keeper: empty staking keeper")) + if isEmpty(consensusHost) { + panic(errors.New("cannot initialize IBC keeper: empty consensus host")) } + if isEmpty(upgradeKeeper) { panic(errors.New("cannot initialize IBC keeper: empty upgrade keeper")) } @@ -59,7 +60,7 @@ func NewKeeper( panic(errors.New("authority must be non-empty")) } - clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper) + clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, consensusHost, upgradeKeeper) connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, &clientKeeper) portKeeper := portkeeper.NewKeeper(scopedKeeper) channelKeeper := channelkeeper.NewKeeper(cdc, key, &clientKeeper, &connectionKeeper, &portKeeper, scopedKeeper) diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go index 0cd6b2a5857..4b9f83103bb 100644 --- a/modules/core/keeper/keeper_test.go +++ b/modules/core/keeper/keeper_test.go @@ -5,17 +5,19 @@ import ( "testing" "time" + ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" + testifysuite "github.com/stretchr/testify/suite" upgradekeeper "cosmossdk.io/x/upgrade/keeper" - stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported" ibckeeper "github.com/cosmos/ibc-go/v8/modules/core/keeper" + ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" ibctesting "github.com/cosmos/ibc-go/v8/testing" ) @@ -61,7 +63,7 @@ func (MockStakingKeeper) UnbondingTime(_ context.Context) (time.Duration, error) // It verifies if ibckeeper.NewKeeper panic when any of the keepers passed in is empty. func (suite *KeeperTestSuite) TestNewKeeper() { var ( - stakingKeeper clienttypes.StakingKeeper + consensusHost clienttypes.ConsensusHost upgradeKeeper clienttypes.UpgradeKeeper scopedKeeper capabilitykeeper.ScopedKeeper newIBCKeeperFn func() @@ -72,21 +74,11 @@ func (suite *KeeperTestSuite) TestNewKeeper() { malleate func() expPass bool }{ - {"failure: empty staking keeper value", func() { - emptyStakingKeeperValue := stakingkeeper.Keeper{} - - stakingKeeper = emptyStakingKeeperValue + {"failure: empty consensus host value", func() { + consensusHost = &ibctm.ConsensusHost{} }, false}, - {"failure: empty staking keeper pointer", func() { - emptyStakingKeeperPointer := &stakingkeeper.Keeper{} - - stakingKeeper = emptyStakingKeeperPointer - }, false}, - {"failure: empty mock staking keeper", func() { - // use a different implementation of clienttypes.StakingKeeper - emptyMockStakingKeeper := MockStakingKeeper{} - - stakingKeeper = emptyMockStakingKeeper + {"failure: nil consensus host value", func() { + consensusHost = nil }, false}, {"failure: empty upgrade keeper value", func() { emptyUpgradeKeeperValue := upgradekeeper.Keeper{} @@ -109,7 +101,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() { suite.chainA.GetSimApp().AppCodec(), suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey), suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName), - stakingKeeper, + consensusHost, upgradeKeeper, scopedKeeper, "", // authority @@ -119,8 +111,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() { {"success: replace stakingKeeper with non-empty MockStakingKeeper", func() { // use a different implementation of clienttypes.StakingKeeper mockStakingKeeper := MockStakingKeeper{"not empty"} - - stakingKeeper = mockStakingKeeper + consensusHost = ibctm.NewConsensusHost(mockStakingKeeper) }, true}, } @@ -135,14 +126,14 @@ func (suite *KeeperTestSuite) TestNewKeeper() { suite.chainA.GetSimApp().AppCodec(), suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey), suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName), - stakingKeeper, + consensusHost, upgradeKeeper, scopedKeeper, suite.chainA.App.GetIBCKeeper().GetAuthority(), ) } - stakingKeeper = suite.chainA.GetSimApp().StakingKeeper + consensusHost = ibctm.NewConsensusHost(suite.chainA.GetSimApp().StakingKeeper) upgradeKeeper = suite.chainA.GetSimApp().UpgradeKeeper scopedKeeper = suite.chainA.GetSimApp().ScopedIBCKeeper diff --git a/modules/light-clients/08-wasm/testing/simapp/app.go b/modules/light-clients/08-wasm/testing/simapp/app.go index ba891455139..9b95dc649bd 100644 --- a/modules/light-clients/08-wasm/testing/simapp/app.go +++ b/modules/light-clients/08-wasm/testing/simapp/app.go @@ -416,7 +416,7 @@ func NewSimApp( app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, runtime.NewKVStoreService(keys[upgradetypes.StoreKey]), appCodec, homePath, app.BaseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String()) app.IBCKeeper = ibckeeper.NewKeeper( - appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), + appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), ibctm.NewConsensusHost(app.StakingKeeper), app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) // Register the proposal types diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 18b93b9f02b..4a8acb976d0 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -408,7 +408,7 @@ func NewSimApp( app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, runtime.NewKVStoreService(keys[upgradetypes.StoreKey]), appCodec, homePath, app.BaseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String()) app.IBCKeeper = ibckeeper.NewKeeper( - appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), + appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), ibctm.NewConsensusHost(app.StakingKeeper), app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), ) // Register the proposal types // Deprecated: Avoid adding new handlers, instead use the new proposal flow From f1d2a0a7ec26731bbdc226ad6fd34e533d7cbc41 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 3 Apr 2024 15:38:54 +0100 Subject: [PATCH 2/3] chore: updated PR number in CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33610861d46..2e406c1e555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (core/02-client, light-clients) [\#5806](https://github.com/cosmos/ibc-go/pull/5806) Decouple light client routing from their encoding structure. * (core/04-channel) [\#5991](https://github.com/cosmos/ibc-go/pull/5991) The client CLI `QueryLatestConsensusState` has been removed. * (light-clients/06-solomachine) [\#6037](https://github.com/cosmos/ibc-go/pull/6037) Remove `Initialize` function from `ClientState` and move logic to `Initialize` function of `LightClientModule`. -* (core/02-client) [\#XXXX](https://github.com/cosmos/ibc-go/pull/XXXX) Removed `stakingKeeper` as an argument to `NewKeeper` and replaced with a `ConsensusHost` implementation. +* (core/02-client) [\#6084](https://github.com/cosmos/ibc-go/pull/6084) Removed `stakingKeeper` as an argument to `NewKeeper` and replaced with a `ConsensusHost` implementation. ### State Machine Breaking From be613480568d70bdf40cec22f415f7ba0f934298 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 3 Apr 2024 15:55:03 +0100 Subject: [PATCH 3/3] chore: add panic on invalid staking keeper --- modules/core/keeper/keeper_test.go | 2 -- modules/light-clients/07-tendermint/consensus_host.go | 4 ++++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go index 4b9f83103bb..95f62bd110a 100644 --- a/modules/core/keeper/keeper_test.go +++ b/modules/core/keeper/keeper_test.go @@ -5,8 +5,6 @@ import ( "testing" "time" - ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" - testifysuite "github.com/stretchr/testify/suite" upgradekeeper "cosmossdk.io/x/upgrade/keeper" diff --git a/modules/light-clients/07-tendermint/consensus_host.go b/modules/light-clients/07-tendermint/consensus_host.go index 0ce7bb4e802..9abfe46ecb9 100644 --- a/modules/light-clients/07-tendermint/consensus_host.go +++ b/modules/light-clients/07-tendermint/consensus_host.go @@ -34,6 +34,10 @@ type StakingKeeper interface { // NewConsensusHost creates and returns a new ConsensusHost for tendermint consensus. func NewConsensusHost(stakingKeeper clienttypes.StakingKeeper) clienttypes.ConsensusHost { + if stakingKeeper == nil { + panic("staking keeper cannot be nil") + } + return &ConsensusHost{ stakingKeeper: stakingKeeper, }