From 9ef56c916a00346ad5b3448b4471c7b102538c68 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 7 Feb 2024 14:27:29 +0100 Subject: [PATCH 01/12] chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist --- modules/light-clients/08-wasm/types/querier.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/light-clients/08-wasm/types/querier.go b/modules/light-clients/08-wasm/types/querier.go index d9eb1376492..83cdaf472f6 100644 --- a/modules/light-clients/08-wasm/types/querier.go +++ b/modules/light-clients/08-wasm/types/querier.go @@ -133,6 +133,10 @@ func NewDefaultQueryPlugins() *QueryPlugins { // AcceptListStargateQuerier allows all queries that are in the provided accept list. // This function returns protobuf encoded responses in bytes. func AcceptListStargateQuerier(acceptedQueries []string) func(sdk.Context, *wasmvmtypes.StargateQuery) ([]byte, error) { + defaultAcceptList := []string{ + "/ibc.core.client.v1.Query/VerifyMembershipProof", + } + return func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { // append user defined accepted queries to default list defined above. acceptedQueries = append(defaultAcceptList, acceptedQueries...) From 94bf3718c604c35744166cf3365df9bf242fe20f Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 8 Feb 2024 19:33:50 +0100 Subject: [PATCH 02/12] chore: update service definition URL in 08-wasm stargate accepted queries --- modules/light-clients/08-wasm/types/querier.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/light-clients/08-wasm/types/querier.go b/modules/light-clients/08-wasm/types/querier.go index 83cdaf472f6..66f7d7e1e06 100644 --- a/modules/light-clients/08-wasm/types/querier.go +++ b/modules/light-clients/08-wasm/types/querier.go @@ -134,7 +134,7 @@ func NewDefaultQueryPlugins() *QueryPlugins { // This function returns protobuf encoded responses in bytes. func AcceptListStargateQuerier(acceptedQueries []string) func(sdk.Context, *wasmvmtypes.StargateQuery) ([]byte, error) { defaultAcceptList := []string{ - "/ibc.core.client.v1.Query/VerifyMembershipProof", + "/ibc.core.client.v1.Query/VerifyMembership", } return func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { From e54a1613b23b0d2365e79a638230c8cb562b8b1f Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 13 Feb 2024 11:58:54 +0100 Subject: [PATCH 03/12] chore: add doc comment to querier test, address nit to move defaultAcceptList --- modules/light-clients/08-wasm/types/querier.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/modules/light-clients/08-wasm/types/querier.go b/modules/light-clients/08-wasm/types/querier.go index 66f7d7e1e06..d9eb1376492 100644 --- a/modules/light-clients/08-wasm/types/querier.go +++ b/modules/light-clients/08-wasm/types/querier.go @@ -133,10 +133,6 @@ func NewDefaultQueryPlugins() *QueryPlugins { // AcceptListStargateQuerier allows all queries that are in the provided accept list. // This function returns protobuf encoded responses in bytes. func AcceptListStargateQuerier(acceptedQueries []string) func(sdk.Context, *wasmvmtypes.StargateQuery) ([]byte, error) { - defaultAcceptList := []string{ - "/ibc.core.client.v1.Query/VerifyMembership", - } - return func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { // append user defined accepted queries to default list defined above. acceptedQueries = append(defaultAcceptList, acceptedQueries...) From e8b7a202d068f8aff52d4e518938bd28643fd2a3 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 13 Feb 2024 13:32:08 +0100 Subject: [PATCH 04/12] feat(draft): add custom client validator func --- modules/core/02-client/keeper/keeper.go | 45 ++++++++++++++++--------- modules/core/02-client/types/client.go | 4 +++ modules/core/keeper/keeper.go | 5 +++ 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 2ef23ee5e58..fa90e7c7429 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -29,11 +29,12 @@ import ( // Keeper represents a type that grants read and write permissions to any client // state information type Keeper struct { - storeKey storetypes.StoreKey - cdc codec.BinaryCodec - legacySubspace types.ParamSubspace - stakingKeeper types.StakingKeeper - upgradeKeeper types.UpgradeKeeper + storeKey storetypes.StoreKey + cdc codec.BinaryCodec + clientValidator types.ClientValidator + legacySubspace types.ParamSubspace + stakingKeeper types.StakingKeeper + upgradeKeeper types.UpgradeKeeper } // NewKeeper creates a new NewKeeper instance @@ -63,6 +64,11 @@ func (k Keeper) UpdateLocalhostClient(ctx sdk.Context, clientState exported.Clie return clientState.UpdateState(ctx, k.cdc, k.ClientStore(ctx, exported.LocalhostClientID), nil) } +// SetSelfClientValidator sets a custom self client validation function. +func (k *Keeper) SetSelfClientValidator(validateFn types.ClientValidator) { + k.clientValidator = validateFn +} + // GenerateClientIdentifier returns the next client identifier. func (k Keeper) GenerateClientIdentifier(ctx sdk.Context, clientType string) string { nextClientSeq := k.GetNextClientSequence(ctx) @@ -305,16 +311,7 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) ( return consensusState, nil } -// ValidateSelfClient validates the client parameters for a client of the running chain -// This function is only used to validate the client state the counterparty stores for this chain -// Client must be in same revision as the executing chain -func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { - tmClient, ok := clientState.(*ibctm.ClientState) - if !ok { - return errorsmod.Wrapf(types.ErrInvalidClient, "client must be a Tendermint client, expected: %T, got: %T", - &ibctm.ClientState{}, tmClient) - } - +func (k Keeper) validateSelfTmClient(ctx sdk.Context, tmClient *ibctm.ClientState) error { if !tmClient.FrozenHeight.IsZero() { return types.ErrClientFrozen } @@ -371,9 +368,27 @@ func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientS expectedUpgradePath, tmClient.UpgradePath) } } + return nil } +// ValidateSelfClient validates the client parameters for a client of the running chain +// This function is only used to validate the client state the counterparty stores for this chain +// Client must be in same revision as the executing chain +func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + tmClient, ok := clientState.(*ibctm.ClientState) + if ok { + return k.validateSelfTmClient(ctx, tmClient) + } + + if k.clientValidator == nil { + return errorsmod.Wrapf(types.ErrInvalidClient, "client must be a Tendermint client, expected: %T, got: %T", + &ibctm.ClientState{}, tmClient) + } + + return k.clientValidator(ctx, clientState) +} + // GetUpgradePlan executes the upgrade keeper GetUpgradePlan function. func (k Keeper) GetUpgradePlan(ctx sdk.Context) (upgradetypes.Plan, error) { return k.upgradeKeeper.GetUpgradePlan(ctx) diff --git a/modules/core/02-client/types/client.go b/modules/core/02-client/types/client.go index 31da1a54e70..9b686525839 100644 --- a/modules/core/02-client/types/client.go +++ b/modules/core/02-client/types/client.go @@ -11,6 +11,7 @@ import ( errorsmod "cosmossdk.io/errors" codectypes "github.com/cosmos/cosmos-sdk/codec/types" + sdk "github.com/cosmos/cosmos-sdk/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" "github.com/cosmos/ibc-go/v8/modules/core/exported" @@ -21,6 +22,9 @@ var ( _ codectypes.UnpackInterfacesMessage = (*ConsensusStateWithHeight)(nil) ) +// ClientValidator is a type alias for a function which validates self client :D TODO: update me +type ClientValidator func(ctx sdk.Context, clientState exported.ClientState) error + // NewIdentifiedClientState creates a new IdentifiedClientState instance func NewIdentifiedClientState(clientID string, clientState exported.ClientState) IdentifiedClientState { msg, ok := clientState.(proto.Message) diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 08ee5dc027e..6aea277867f 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -91,6 +91,11 @@ func (k *Keeper) SetRouter(rtr *porttypes.Router) { k.Router.Seal() } +// SetSelfClientValidator delegates to the IBC client keeper to set the self client validation func. +func (k Keeper) SetSelfClientValidator(validateFn clienttypes.ClientValidator) { + k.ClientKeeper.SetSelfClientValidator(validateFn) +} + // GetAuthority returns the ibc module's authority. func (k Keeper) GetAuthority() string { return k.authority From 50a62fe771ad9ff737c08b93990a9e2ba4bb3e9d Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 13 Feb 2024 14:37:00 +0100 Subject: [PATCH 05/12] feat: add SelfClientValidator type alias func and refactor tests to confirm it works --- modules/core/02-client/keeper/keeper.go | 53 +++++++------ modules/core/02-client/keeper/keeper_test.go | 81 ++++++++++++++++---- modules/core/02-client/types/client.go | 11 ++- modules/core/02-client/types/errors.go | 1 + modules/core/keeper/keeper.go | 5 -- 5 files changed, 102 insertions(+), 49 deletions(-) diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index fa90e7c7429..3011dae9022 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -29,12 +29,12 @@ import ( // Keeper represents a type that grants read and write permissions to any client // state information type Keeper struct { - storeKey storetypes.StoreKey - cdc codec.BinaryCodec - clientValidator types.ClientValidator - legacySubspace types.ParamSubspace - stakingKeeper types.StakingKeeper - upgradeKeeper types.UpgradeKeeper + storeKey storetypes.StoreKey + cdc codec.BinaryCodec + legacySubspace types.ParamSubspace + selfClientValidator types.SelfClientValidator + stakingKeeper types.StakingKeeper + upgradeKeeper types.UpgradeKeeper } // NewKeeper creates a new NewKeeper instance @@ -65,8 +65,8 @@ func (k Keeper) UpdateLocalhostClient(ctx sdk.Context, clientState exported.Clie } // SetSelfClientValidator sets a custom self client validation function. -func (k *Keeper) SetSelfClientValidator(validateFn types.ClientValidator) { - k.clientValidator = validateFn +func (k *Keeper) SetSelfClientValidator(validateFn types.SelfClientValidator) { + k.selfClientValidator = validateFn } // GenerateClientIdentifier returns the next client identifier. @@ -311,6 +311,26 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) ( return consensusState, nil } +// ValidateSelfClient validates the client parameters for a client of the running chain. +// This function is only used to validate the client state the counterparty stores for this chain. +// NOTE: If the client type is not of type Tendermint then delegate to a custom client validator function. +// This allows support for non-Tendermint clients, for example 08-wasm clients. +func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + tmClient, ok := clientState.(*ibctm.ClientState) + if ok { + return k.validateSelfTmClient(ctx, tmClient) + } + + if k.selfClientValidator == nil { + return errorsmod.Wrapf(types.ErrClientTypeNotSupported, "cannot validate self client of type: %T", clientState) + } + + return k.selfClientValidator(ctx, clientState) +} + +// validateSelfTmClient validates the tendermint client parameters for a client of the running chain. +// This function is only used to validate the client state the counterparty stores for this chain. +// Client must be in same revision as the executing chain. func (k Keeper) validateSelfTmClient(ctx sdk.Context, tmClient *ibctm.ClientState) error { if !tmClient.FrozenHeight.IsZero() { return types.ErrClientFrozen @@ -372,23 +392,6 @@ func (k Keeper) validateSelfTmClient(ctx sdk.Context, tmClient *ibctm.ClientStat return nil } -// ValidateSelfClient validates the client parameters for a client of the running chain -// This function is only used to validate the client state the counterparty stores for this chain -// Client must be in same revision as the executing chain -func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { - tmClient, ok := clientState.(*ibctm.ClientState) - if ok { - return k.validateSelfTmClient(ctx, tmClient) - } - - if k.clientValidator == nil { - return errorsmod.Wrapf(types.ErrInvalidClient, "client must be a Tendermint client, expected: %T, got: %T", - &ibctm.ClientState{}, tmClient) - } - - return k.clientValidator(ctx, clientState) -} - // GetUpgradePlan executes the upgrade keeper GetUpgradePlan function. func (k Keeper) GetUpgradePlan(ctx sdk.Context) (upgradetypes.Plan, error) { return k.upgradeKeeper.GetUpgradePlan(ctx) diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go index c879cdd0920..681028c3226 100644 --- a/modules/core/02-client/keeper/keeper_test.go +++ b/modules/core/02-client/keeper/keeper_test.go @@ -149,77 +149,124 @@ func (suite *KeeperTestSuite) TestValidateSelfClient() { testClientHeight := types.GetSelfHeight(suite.chainA.GetContext()) testClientHeight.RevisionHeight-- + var clientState exported.ClientState + testCases := []struct { - name string - clientState exported.ClientState - expPass bool + name string + malleate func() + expPass bool // TODO: expError }{ { "success", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, true, }, { "success with nil UpgradePath", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), nil), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), nil) + }, + true, + }, + { + "success with custom self validator: solomachine", + func() { + clientState = solomachine.NewClientState(1, &solomachine.ConsensusState{}) + + // add some mock validation logic + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetSelfClientValidator(func(ctx sdk.Context, clientState exported.ClientState) error { + smClientState, ok := clientState.(*solomachine.ClientState) + suite.Require().True(ok) + suite.Require().Equal(uint64(1), smClientState.Sequence) + + return nil + }) + }, true, }, { "frozen client", - &ibctm.ClientState{ChainId: suite.chainA.ChainID, TrustLevel: ibctm.DefaultTrustLevel, TrustingPeriod: trustingPeriod, UnbondingPeriod: ubdPeriod, MaxClockDrift: maxClockDrift, FrozenHeight: testClientHeight, LatestHeight: testClientHeight, ProofSpecs: commitmenttypes.GetSDKSpecs(), UpgradePath: ibctesting.UpgradePath}, + func() { + clientState = &ibctm.ClientState{ChainId: suite.chainA.ChainID, TrustLevel: ibctm.DefaultTrustLevel, TrustingPeriod: trustingPeriod, UnbondingPeriod: ubdPeriod, MaxClockDrift: maxClockDrift, FrozenHeight: testClientHeight, LatestHeight: testClientHeight, ProofSpecs: commitmenttypes.GetSDKSpecs(), UpgradePath: ibctesting.UpgradePath} + }, false, }, { "incorrect chainID", - ibctm.NewClientState("gaiatestnet", ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), + func() { + clientState = ibctm.NewClientState("gaiatestnet", ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, false, }, { "invalid client height", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, types.GetSelfHeight(suite.chainA.GetContext()).Increment().(types.Height), commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, types.GetSelfHeight(suite.chainA.GetContext()).Increment().(types.Height), commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, false, }, { "invalid client type", - solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}), + func() { + clientState = solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}) + }, false, }, { "invalid client revision", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeightRevision1, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeightRevision1, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, false, }, { "invalid proof specs", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, nil, ibctesting.UpgradePath), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, nil, ibctesting.UpgradePath) + }, false, }, { "invalid trust level", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), false, + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, + false, }, { "invalid unbonding period", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod+10, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod+10, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, false, }, { "invalid trusting period", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, ubdPeriod+10, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, ubdPeriod+10, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + }, false, }, { "invalid upgrade path", - ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), []string{"bad", "upgrade", "path"}), + func() { + clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), []string{"bad", "upgrade", "path"}) + }, false, }, } for _, tc := range testCases { tc := tc - suite.Run(tc.name, func() { - err := suite.chainA.App.GetIBCKeeper().ClientKeeper.ValidateSelfClient(suite.chainA.GetContext(), tc.clientState) + suite.SetupTest() + + tc.malleate() + + err := suite.chainA.App.GetIBCKeeper().ClientKeeper.ValidateSelfClient(suite.chainA.GetContext(), clientState) + if tc.expPass { suite.Require().NoError(err, "expected valid client for case: %s", tc.name) } else { diff --git a/modules/core/02-client/types/client.go b/modules/core/02-client/types/client.go index 9b686525839..22150cc6b9a 100644 --- a/modules/core/02-client/types/client.go +++ b/modules/core/02-client/types/client.go @@ -22,8 +22,15 @@ var ( _ codectypes.UnpackInterfacesMessage = (*ConsensusStateWithHeight)(nil) ) -// ClientValidator is a type alias for a function which validates self client :D TODO: update me -type ClientValidator func(ctx sdk.Context, clientState exported.ClientState) error +// SelfClientValidator is a type alias for a function which validates self client :D TODO: update me +type SelfClientValidator func(ctx sdk.Context, clientState exported.ClientState) error + +// TODO: introduce an interface type to handle host validation funcs. The above SelfClientValidator type alias func will not be +// enough as we need to handle GetSelfConsensusState +type SelfClientValidatorI interface { + ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error + GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) +} // NewIdentifiedClientState creates a new IdentifiedClientState instance func NewIdentifiedClientState(clientID string, clientState exported.ClientState) IdentifiedClientState { diff --git a/modules/core/02-client/types/errors.go b/modules/core/02-client/types/errors.go index 3a726378cbc..3dc556901f8 100644 --- a/modules/core/02-client/types/errors.go +++ b/modules/core/02-client/types/errors.go @@ -36,4 +36,5 @@ var ( ErrClientNotActive = errorsmod.Register(SubModuleName, 29, "client state is not active") ErrFailedMembershipVerification = errorsmod.Register(SubModuleName, 30, "membership verification failed") ErrFailedNonMembershipVerification = errorsmod.Register(SubModuleName, 31, "non-membership verification failed") + ErrClientTypeNotSupported = errorsmod.Register(SubModuleName, 32, "client type not supported") ) diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go index 6aea277867f..08ee5dc027e 100644 --- a/modules/core/keeper/keeper.go +++ b/modules/core/keeper/keeper.go @@ -91,11 +91,6 @@ func (k *Keeper) SetRouter(rtr *porttypes.Router) { k.Router.Seal() } -// SetSelfClientValidator delegates to the IBC client keeper to set the self client validation func. -func (k Keeper) SetSelfClientValidator(validateFn clienttypes.ClientValidator) { - k.ClientKeeper.SetSelfClientValidator(validateFn) -} - // GetAuthority returns the ibc module's authority. func (k Keeper) GetAuthority() string { return k.authority From b8df59f75291651cb06e2299c8a309a2c6334987 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 13 Feb 2024 17:19:29 +0100 Subject: [PATCH 06/12] refactor: updated ibc client keeper to use interface type for self client validation of consensus parameters --- .../core/02-client/keeper/client_validator.go | 124 ++++++++++++++++++ modules/core/02-client/keeper/keeper.go | 122 ++--------------- modules/core/02-client/keeper/keeper_test.go | 53 ++++---- modules/core/02-client/types/client.go | 10 +- testing/mock/self_client_validator.go | 31 +++++ 5 files changed, 202 insertions(+), 138 deletions(-) create mode 100644 modules/core/02-client/keeper/client_validator.go create mode 100644 testing/mock/self_client_validator.go diff --git a/modules/core/02-client/keeper/client_validator.go b/modules/core/02-client/keeper/client_validator.go new file mode 100644 index 00000000000..d762ef24482 --- /dev/null +++ b/modules/core/02-client/keeper/client_validator.go @@ -0,0 +1,124 @@ +package keeper + +import ( + "reflect" + + errorsmod "cosmossdk.io/errors" + upgradetypes "cosmossdk.io/x/upgrade/types" + "github.com/cometbft/cometbft/light" + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" + ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" + "github.com/cosmos/ibc-go/v8/modules/core/exported" + ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" +) + +var _ types.SelfClientValidator = (*TendermintClientValidator)(nil) + +// TendermintClientValidator implements the SelfClientValidator interface. +type TendermintClientValidator struct { + stakingKeeper types.StakingKeeper +} + +// NewTendermintClientValidator creates and returns a new SelfClientValidator for tendermint consensus. +func NewTendermintClientValidator(stakingKeeper types.StakingKeeper) *TendermintClientValidator { + return &TendermintClientValidator{ + stakingKeeper: stakingKeeper, + } +} + +// GetSelfConsensusState implements types.SelfClientValidatorI. +func (tcv *TendermintClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + selfHeight, ok := height.(types.Height) + if !ok { + return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected %T, got %T", types.Height{}, height) + } + + // check that height revision matches chainID revision + revision := types.ParseChainID(ctx.ChainID()) + if revision != height.GetRevisionNumber() { + return nil, errorsmod.Wrapf(types.ErrInvalidHeight, "chainID revision number does not match height revision number: expected %d, got %d", revision, height.GetRevisionNumber()) + } + + histInfo, err := tcv.stakingKeeper.GetHistoricalInfo(ctx, int64(selfHeight.RevisionHeight)) + if err != nil { + return nil, errorsmod.Wrapf(err, "height %d", selfHeight.RevisionHeight) + } + + consensusState := &ibctm.ConsensusState{ + Timestamp: histInfo.Header.Time, + Root: commitmenttypes.NewMerkleRoot(histInfo.Header.GetAppHash()), + NextValidatorsHash: histInfo.Header.NextValidatorsHash, + } + + return consensusState, nil +} + +// ValidateSelfClient implements types.SelfClientValidatorI. +func (tcv *TendermintClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + tmClient, ok := clientState.(*ibctm.ClientState) + if !ok { + return errorsmod.Wrapf(types.ErrInvalidClient, "client must be a Tendermint client, expected: %T, got: %T", &ibctm.ClientState{}, tmClient) + } + + if !tmClient.FrozenHeight.IsZero() { + return types.ErrClientFrozen + } + + if ctx.ChainID() != tmClient.ChainId { + return errorsmod.Wrapf(types.ErrInvalidClient, "invalid chain-id. expected: %s, got: %s", + ctx.ChainID(), tmClient.ChainId) + } + + revision := types.ParseChainID(ctx.ChainID()) + + // client must be in the same revision as executing chain + if tmClient.LatestHeight.RevisionNumber != revision { + return errorsmod.Wrapf(types.ErrInvalidClient, "client is not in the same revision as the chain. expected revision: %d, got: %d", + tmClient.LatestHeight.RevisionNumber, revision) + } + + selfHeight := types.NewHeight(revision, uint64(ctx.BlockHeight())) + if tmClient.LatestHeight.GTE(selfHeight) { + return errorsmod.Wrapf(types.ErrInvalidClient, "client has LatestHeight %d greater than or equal to chain height %d", + tmClient.LatestHeight, selfHeight) + } + + expectedProofSpecs := commitmenttypes.GetSDKSpecs() + if !reflect.DeepEqual(expectedProofSpecs, tmClient.ProofSpecs) { + return errorsmod.Wrapf(types.ErrInvalidClient, "client has invalid proof specs. expected: %v got: %v", + expectedProofSpecs, tmClient.ProofSpecs) + } + + if err := light.ValidateTrustLevel(tmClient.TrustLevel.ToTendermint()); err != nil { + return errorsmod.Wrapf(types.ErrInvalidClient, "trust-level invalid: %v", err) + } + + expectedUbdPeriod, err := tcv.stakingKeeper.UnbondingTime(ctx) + if err != nil { + return errorsmod.Wrapf(err, "failed to retrieve unbonding period") + } + + if expectedUbdPeriod != tmClient.UnbondingPeriod { + return errorsmod.Wrapf(types.ErrInvalidClient, "invalid unbonding period. expected: %s, got: %s", + expectedUbdPeriod, tmClient.UnbondingPeriod) + } + + if tmClient.UnbondingPeriod < tmClient.TrustingPeriod { + return errorsmod.Wrapf(types.ErrInvalidClient, "unbonding period must be greater than trusting period. unbonding period (%d) < trusting period (%d)", + tmClient.UnbondingPeriod, tmClient.TrustingPeriod) + } + + if len(tmClient.UpgradePath) != 0 { + // For now, SDK IBC implementation assumes that upgrade path (if defined) is defined by SDK upgrade module + expectedUpgradePath := []string{upgradetypes.StoreKey, upgradetypes.KeyUpgradedIBCState} + if !reflect.DeepEqual(expectedUpgradePath, tmClient.UpgradePath) { + return errorsmod.Wrapf(types.ErrInvalidClient, "upgrade path must be the upgrade path defined by upgrade module. expected %v, got %v", + expectedUpgradePath, tmClient.UpgradePath) + } + } + + return nil +} diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 3011dae9022..f50ccd97d96 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -3,7 +3,6 @@ package keeper import ( "errors" "fmt" - "reflect" "strings" errorsmod "cosmossdk.io/errors" @@ -15,14 +14,9 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cometbft/cometbft/light" - "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" - commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" - ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" "github.com/cosmos/ibc-go/v8/modules/core/exported" - ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" localhost "github.com/cosmos/ibc-go/v8/modules/light-clients/09-localhost" ) @@ -40,11 +34,12 @@ 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 { return Keeper{ - storeKey: key, - cdc: cdc, - legacySubspace: legacySubspace, - stakingKeeper: sk, - upgradeKeeper: uk, + storeKey: key, + cdc: cdc, + legacySubspace: legacySubspace, + selfClientValidator: NewTendermintClientValidator(sk), + stakingKeeper: sk, + upgradeKeeper: uk, } } @@ -65,8 +60,12 @@ func (k Keeper) UpdateLocalhostClient(ctx sdk.Context, clientState exported.Clie } // SetSelfClientValidator sets a custom self client validation function. -func (k *Keeper) SetSelfClientValidator(validateFn types.SelfClientValidator) { - k.selfClientValidator = validateFn +func (k *Keeper) SetSelfClientValidator(selfClientValidator types.SelfClientValidator) { + if selfClientValidator == nil { + panic(fmt.Errorf("cannot set a nil self client validator")) + } + + k.selfClientValidator = selfClientValidator } // GenerateClientIdentifier returns the next client identifier. @@ -288,27 +287,7 @@ func (k Keeper) GetLatestClientConsensusState(ctx sdk.Context, clientID string) // and returns the expected consensus state at that height. // For now, can only retrieve self consensus states for the current revision func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { - selfHeight, ok := height.(types.Height) - if !ok { - return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected %T, got %T", types.Height{}, height) - } - // check that height revision matches chainID revision - revision := types.ParseChainID(ctx.ChainID()) - if revision != height.GetRevisionNumber() { - return nil, errorsmod.Wrapf(types.ErrInvalidHeight, "chainID revision number does not match height revision number: expected %d, got %d", revision, height.GetRevisionNumber()) - } - histInfo, err := k.stakingKeeper.GetHistoricalInfo(ctx, int64(selfHeight.RevisionHeight)) - if err != nil { - return nil, errorsmod.Wrapf(err, "height %d", selfHeight.RevisionHeight) - } - - consensusState := &ibctm.ConsensusState{ - Timestamp: histInfo.Header.Time, - Root: commitmenttypes.NewMerkleRoot(histInfo.Header.GetAppHash()), - NextValidatorsHash: histInfo.Header.NextValidatorsHash, - } - - return consensusState, nil + return k.selfClientValidator.GetSelfConsensusState(ctx, height) } // ValidateSelfClient validates the client parameters for a client of the running chain. @@ -316,80 +295,7 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) ( // NOTE: If the client type is not of type Tendermint then delegate to a custom client validator function. // This allows support for non-Tendermint clients, for example 08-wasm clients. func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { - tmClient, ok := clientState.(*ibctm.ClientState) - if ok { - return k.validateSelfTmClient(ctx, tmClient) - } - - if k.selfClientValidator == nil { - return errorsmod.Wrapf(types.ErrClientTypeNotSupported, "cannot validate self client of type: %T", clientState) - } - - return k.selfClientValidator(ctx, clientState) -} - -// validateSelfTmClient validates the tendermint client parameters for a client of the running chain. -// This function is only used to validate the client state the counterparty stores for this chain. -// Client must be in same revision as the executing chain. -func (k Keeper) validateSelfTmClient(ctx sdk.Context, tmClient *ibctm.ClientState) error { - if !tmClient.FrozenHeight.IsZero() { - return types.ErrClientFrozen - } - - if ctx.ChainID() != tmClient.ChainId { - return errorsmod.Wrapf(types.ErrInvalidClient, "invalid chain-id. expected: %s, got: %s", - ctx.ChainID(), tmClient.ChainId) - } - - revision := types.ParseChainID(ctx.ChainID()) - - // client must be in the same revision as executing chain - if tmClient.LatestHeight.RevisionNumber != revision { - return errorsmod.Wrapf(types.ErrInvalidClient, "client is not in the same revision as the chain. expected revision: %d, got: %d", - tmClient.LatestHeight.RevisionNumber, revision) - } - - selfHeight := types.NewHeight(revision, uint64(ctx.BlockHeight())) - if tmClient.LatestHeight.GTE(selfHeight) { - return errorsmod.Wrapf(types.ErrInvalidClient, "client has LatestHeight %d greater than or equal to chain height %d", - tmClient.LatestHeight, selfHeight) - } - - expectedProofSpecs := commitmenttypes.GetSDKSpecs() - if !reflect.DeepEqual(expectedProofSpecs, tmClient.ProofSpecs) { - return errorsmod.Wrapf(types.ErrInvalidClient, "client has invalid proof specs. expected: %v got: %v", - expectedProofSpecs, tmClient.ProofSpecs) - } - - if err := light.ValidateTrustLevel(tmClient.TrustLevel.ToTendermint()); err != nil { - return errorsmod.Wrapf(types.ErrInvalidClient, "trust-level invalid: %v", err) - } - - expectedUbdPeriod, err := k.stakingKeeper.UnbondingTime(ctx) - if err != nil { - return errorsmod.Wrapf(err, "failed to retrieve unbonding period") - } - - if expectedUbdPeriod != tmClient.UnbondingPeriod { - return errorsmod.Wrapf(types.ErrInvalidClient, "invalid unbonding period. expected: %s, got: %s", - expectedUbdPeriod, tmClient.UnbondingPeriod) - } - - if tmClient.UnbondingPeriod < tmClient.TrustingPeriod { - return errorsmod.Wrapf(types.ErrInvalidClient, "unbonding period must be greater than trusting period. unbonding period (%d) < trusting period (%d)", - tmClient.UnbondingPeriod, tmClient.TrustingPeriod) - } - - if len(tmClient.UpgradePath) != 0 { - // For now, SDK IBC implementation assumes that upgrade path (if defined) is defined by SDK upgrade module - expectedUpgradePath := []string{upgradetypes.StoreKey, upgradetypes.KeyUpgradedIBCState} - if !reflect.DeepEqual(expectedUpgradePath, tmClient.UpgradePath) { - return errorsmod.Wrapf(types.ErrInvalidClient, "upgrade path must be the upgrade path defined by upgrade module. expected %v, got %v", - expectedUpgradePath, tmClient.UpgradePath) - } - } - - return nil + return k.selfClientValidator.ValidateSelfClient(ctx, clientState) } // GetUpgradePlan executes the upgrade keeper GetUpgradePlan function. diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go index 681028c3226..46bf8245332 100644 --- a/modules/core/02-client/keeper/keeper_test.go +++ b/modules/core/02-client/keeper/keeper_test.go @@ -29,6 +29,7 @@ import ( ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" localhost "github.com/cosmos/ibc-go/v8/modules/light-clients/09-localhost" ibctesting "github.com/cosmos/ibc-go/v8/testing" + "github.com/cosmos/ibc-go/v8/testing/mock" "github.com/cosmos/ibc-go/v8/testing/simapp" ) @@ -154,107 +155,111 @@ func (suite *KeeperTestSuite) TestValidateSelfClient() { testCases := []struct { name string malleate func() - expPass bool // TODO: expError + expError error }{ { "success", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) }, - true, + nil, }, { "success with nil UpgradePath", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), nil) }, - true, + nil, }, { "success with custom self validator: solomachine", func() { clientState = solomachine.NewClientState(1, &solomachine.ConsensusState{}) - // add some mock validation logic - suite.chainA.App.GetIBCKeeper().ClientKeeper.SetSelfClientValidator(func(ctx sdk.Context, clientState exported.ClientState) error { - smClientState, ok := clientState.(*solomachine.ClientState) - suite.Require().True(ok) - suite.Require().Equal(uint64(1), smClientState.Sequence) + smSelfClientValidator := &mock.MockClientValidator{ + ValidateSelfClientFn: func(ctx sdk.Context, clientState exported.ClientState) error { + smClientState, ok := clientState.(*solomachine.ClientState) + suite.Require().True(ok) + suite.Require().Equal(uint64(1), smClientState.Sequence) - return nil - }) + return nil + }, + } + + // add some mock validation logic + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetSelfClientValidator(smSelfClientValidator) }, - true, + nil, }, { "frozen client", func() { clientState = &ibctm.ClientState{ChainId: suite.chainA.ChainID, TrustLevel: ibctm.DefaultTrustLevel, TrustingPeriod: trustingPeriod, UnbondingPeriod: ubdPeriod, MaxClockDrift: maxClockDrift, FrozenHeight: testClientHeight, LatestHeight: testClientHeight, ProofSpecs: commitmenttypes.GetSDKSpecs(), UpgradePath: ibctesting.UpgradePath} }, - false, + types.ErrClientFrozen, }, { "incorrect chainID", func() { clientState = ibctm.NewClientState("gaiatestnet", ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) }, - false, + types.ErrInvalidClient, }, { "invalid client height", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, types.GetSelfHeight(suite.chainA.GetContext()).Increment().(types.Height), commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) }, - false, + types.ErrInvalidClient, }, { "invalid client type", func() { clientState = solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}) }, - false, + types.ErrInvalidClient, }, { "invalid client revision", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeightRevision1, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) }, - false, + types.ErrInvalidClient, }, { "invalid proof specs", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, nil, ibctesting.UpgradePath) }, - false, + types.ErrInvalidClient, }, { "invalid trust level", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) }, - false, + types.ErrInvalidClient, }, { "invalid unbonding period", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod+10, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) }, - false, + types.ErrInvalidClient, }, { "invalid trusting period", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, ubdPeriod+10, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) }, - false, + types.ErrInvalidClient, }, { "invalid upgrade path", func() { clientState = ibctm.NewClientState(suite.chainA.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), []string{"bad", "upgrade", "path"}) }, - false, + types.ErrInvalidClient, }, } @@ -267,7 +272,8 @@ func (suite *KeeperTestSuite) TestValidateSelfClient() { err := suite.chainA.App.GetIBCKeeper().ClientKeeper.ValidateSelfClient(suite.chainA.GetContext(), clientState) - if tc.expPass { + expPass := tc.expError == nil + if expPass { suite.Require().NoError(err, "expected valid client for case: %s", tc.name) } else { suite.Require().Error(err, "expected invalid client for case: %s", tc.name) @@ -355,7 +361,8 @@ func (suite KeeperTestSuite) TestGetAllGenesisMetadata() { //nolint:govet // thi }) } -func (suite KeeperTestSuite) TestGetConsensusState() { //nolint:govet // this is a test, we are okay with copying locks +// TODO(chatton): refactor me and make me nice :) +func (suite *KeeperTestSuite) TestGetConsensusState() { suite.ctx = suite.ctx.WithBlockHeight(10) cases := []struct { name string diff --git a/modules/core/02-client/types/client.go b/modules/core/02-client/types/client.go index 22150cc6b9a..2b2447aced8 100644 --- a/modules/core/02-client/types/client.go +++ b/modules/core/02-client/types/client.go @@ -22,14 +22,10 @@ var ( _ codectypes.UnpackInterfacesMessage = (*ConsensusStateWithHeight)(nil) ) -// SelfClientValidator is a type alias for a function which validates self client :D TODO: update me -type SelfClientValidator func(ctx sdk.Context, clientState exported.ClientState) error - -// TODO: introduce an interface type to handle host validation funcs. The above SelfClientValidator type alias func will not be -// enough as we need to handle GetSelfConsensusState -type SelfClientValidatorI interface { - ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error +// SelfClientValidator defines an interface used to validate an IBC ClientState against a host chain's underlying consensus parameters. +type SelfClientValidator interface { GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) + ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error } // NewIdentifiedClientState creates a new IdentifiedClientState instance diff --git a/testing/mock/self_client_validator.go b/testing/mock/self_client_validator.go new file mode 100644 index 00000000000..9a1297725c1 --- /dev/null +++ b/testing/mock/self_client_validator.go @@ -0,0 +1,31 @@ +package mock + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + "github.com/cosmos/ibc-go/v8/modules/core/exported" +) + +var _ clienttypes.SelfClientValidator = (*MockClientValidator)(nil) + +type MockClientValidator struct { + GetSelfConsensusStateFn func(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) + ValidateSelfClientFn func(ctx sdk.Context, clientState exported.ClientState) error +} + +func (mcv *MockClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + if mcv.GetSelfConsensusStateFn == nil { + return nil, nil + } + + return mcv.GetSelfConsensusStateFn(ctx, height) +} + +func (mcv *MockClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + if mcv.ValidateSelfClientFn == nil { + return nil + } + + return mcv.ValidateSelfClientFn(ctx, clientState) +} From 375f06b3fadec054a03eb8361d5d7f81cb536317 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 13 Feb 2024 17:31:59 +0100 Subject: [PATCH 07/12] lint: make lint-fix --- modules/core/02-client/keeper/client_validator.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/core/02-client/keeper/client_validator.go b/modules/core/02-client/keeper/client_validator.go index d762ef24482..7010c5958e4 100644 --- a/modules/core/02-client/keeper/client_validator.go +++ b/modules/core/02-client/keeper/client_validator.go @@ -5,9 +5,11 @@ import ( errorsmod "cosmossdk.io/errors" upgradetypes "cosmossdk.io/x/upgrade/types" - "github.com/cometbft/cometbft/light" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cometbft/cometbft/light" + "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" From 7afa5d46cbbd7c71bd0ad65915483fabf4fc4305 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Feb 2024 09:47:35 +0000 Subject: [PATCH 08/12] chore: merge main and fix linter --- modules/core/02-client/keeper/keeper_test.go | 2 +- testing/mock/self_client_validator.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go index 46bf8245332..11b899848f3 100644 --- a/modules/core/02-client/keeper/keeper_test.go +++ b/modules/core/02-client/keeper/keeper_test.go @@ -176,7 +176,7 @@ func (suite *KeeperTestSuite) TestValidateSelfClient() { func() { clientState = solomachine.NewClientState(1, &solomachine.ConsensusState{}) - smSelfClientValidator := &mock.MockClientValidator{ + smSelfClientValidator := &mock.ClientValidator{ ValidateSelfClientFn: func(ctx sdk.Context, clientState exported.ClientState) error { smClientState, ok := clientState.(*solomachine.ClientState) suite.Require().True(ok) diff --git a/testing/mock/self_client_validator.go b/testing/mock/self_client_validator.go index 9a1297725c1..b96367bdb94 100644 --- a/testing/mock/self_client_validator.go +++ b/testing/mock/self_client_validator.go @@ -7,14 +7,14 @@ import ( "github.com/cosmos/ibc-go/v8/modules/core/exported" ) -var _ clienttypes.SelfClientValidator = (*MockClientValidator)(nil) +var _ clienttypes.SelfClientValidator = (*ClientValidator)(nil) -type MockClientValidator struct { +type ClientValidator struct { GetSelfConsensusStateFn func(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) ValidateSelfClientFn func(ctx sdk.Context, clientState exported.ClientState) error } -func (mcv *MockClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { +func (mcv *ClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { if mcv.GetSelfConsensusStateFn == nil { return nil, nil } @@ -22,7 +22,7 @@ func (mcv *MockClientValidator) GetSelfConsensusState(ctx sdk.Context, height ex return mcv.GetSelfConsensusStateFn(ctx, height) } -func (mcv *MockClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { +func (mcv *ClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { if mcv.ValidateSelfClientFn == nil { return nil } From 97a1b5a461964821e3001556eaf63413a5c42acc Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 15 Feb 2024 09:49:31 +0000 Subject: [PATCH 09/12] test: cleaned up GetSelfConsensusState tests --- modules/core/02-client/keeper/keeper_test.go | 38 ++++++++++++++------ 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go index 11b899848f3..a4d4d411364 100644 --- a/modules/core/02-client/keeper/keeper_test.go +++ b/modules/core/02-client/keeper/keeper_test.go @@ -361,28 +361,44 @@ func (suite KeeperTestSuite) TestGetAllGenesisMetadata() { //nolint:govet // thi }) } -// TODO(chatton): refactor me and make me nice :) func (suite *KeeperTestSuite) TestGetConsensusState() { + var height types.Height + suite.ctx = suite.ctx.WithBlockHeight(10) + cases := []struct { - name string - height types.Height - expPass bool + name string + malleate func() + expError error }{ - {"zero height", types.ZeroHeight(), false}, - {"height > latest height", types.NewHeight(0, uint64(suite.ctx.BlockHeight())+1), false}, - {"latest height - 1", types.NewHeight(0, uint64(suite.ctx.BlockHeight())-1), true}, - {"latest height", types.GetSelfHeight(suite.ctx), true}, + {"zero height", func() { + height = types.ZeroHeight() + }, stakingtypes.ErrNoHistoricalInfo}, + {"height > latest height", func() { + height = types.NewHeight(0, uint64(suite.ctx.BlockHeight())+1) + }, stakingtypes.ErrNoHistoricalInfo}, + {"latest height - 1", func() { + height = types.NewHeight(0, uint64(suite.ctx.BlockHeight())-1) + }, nil}, + {"latest height", func() { + height = types.GetSelfHeight(suite.ctx) + }, nil}, } for i, tc := range cases { tc := tc - cs, err := suite.keeper.GetSelfConsensusState(suite.ctx, tc.height) - if tc.expPass { + height = types.ZeroHeight() + + tc.malleate() + + cs, err := suite.keeper.GetSelfConsensusState(suite.ctx, height) + + expPass := tc.expError == nil + if expPass { suite.Require().NoError(err, "Case %d should have passed: %s", i, tc.name) suite.Require().NotNil(cs, "Case %d should have passed: %s", i, tc.name) } else { - suite.Require().Error(err, "Case %d should have failed: %s", i, tc.name) + suite.Require().ErrorIs(err, tc.expError, "Case %d should have failed: %s", i, tc.name) suite.Require().Nil(cs, "Case %d should have failed: %s", i, tc.name) } } From 97509fe04247c0904c716ffee6f94850d70d39dc Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 15 Feb 2024 09:57:59 +0000 Subject: [PATCH 10/12] test: added test cases for custom validator logic --- modules/core/02-client/keeper/keeper_test.go | 29 ++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go index a4d4d411364..6302ffb362b 100644 --- a/modules/core/02-client/keeper/keeper_test.go +++ b/modules/core/02-client/keeper/keeper_test.go @@ -364,8 +364,6 @@ func (suite KeeperTestSuite) TestGetAllGenesisMetadata() { //nolint:govet // thi func (suite *KeeperTestSuite) TestGetConsensusState() { var height types.Height - suite.ctx = suite.ctx.WithBlockHeight(10) - cases := []struct { name string malleate func() @@ -377,6 +375,30 @@ func (suite *KeeperTestSuite) TestGetConsensusState() { {"height > latest height", func() { height = types.NewHeight(0, uint64(suite.ctx.BlockHeight())+1) }, stakingtypes.ErrNoHistoricalInfo}, + { + name: "custom client validator: failure", + malleate: func() { + clientValidator := &mock.ClientValidator{ + GetSelfConsensusStateFn: func(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + return nil, mock.MockApplicationCallbackError + }, + } + suite.keeper.SetSelfClientValidator(clientValidator) + }, + expError: mock.MockApplicationCallbackError, + }, + { + name: "custom client validator: success", + malleate: func() { + clientValidator := &mock.ClientValidator{ + GetSelfConsensusStateFn: func(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + return &solomachine.ConsensusState{}, nil + }, + } + suite.keeper.SetSelfClientValidator(clientValidator) + }, + expError: nil, + }, {"latest height - 1", func() { height = types.NewHeight(0, uint64(suite.ctx.BlockHeight())-1) }, nil}, @@ -386,7 +408,10 @@ func (suite *KeeperTestSuite) TestGetConsensusState() { } for i, tc := range cases { + suite.SetupTest() + suite.ctx = suite.ctx.WithBlockHeight(10) tc := tc + height = types.ZeroHeight() tc.malleate() From 24ae8f59ad2c8368675e412f6386b7b333197910 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 15 Feb 2024 16:27:59 +0100 Subject: [PATCH 11/12] nit: rename receiver arg --- ...{self_client_validator.go => client_validator.go} | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) rename testing/mock/{self_client_validator.go => client_validator.go} (56%) diff --git a/testing/mock/self_client_validator.go b/testing/mock/client_validator.go similarity index 56% rename from testing/mock/self_client_validator.go rename to testing/mock/client_validator.go index b96367bdb94..ad7be616b20 100644 --- a/testing/mock/self_client_validator.go +++ b/testing/mock/client_validator.go @@ -14,18 +14,18 @@ type ClientValidator struct { ValidateSelfClientFn func(ctx sdk.Context, clientState exported.ClientState) error } -func (mcv *ClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { - if mcv.GetSelfConsensusStateFn == nil { +func (cv *ClientValidator) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { + if cv.GetSelfConsensusStateFn == nil { return nil, nil } - return mcv.GetSelfConsensusStateFn(ctx, height) + return cv.GetSelfConsensusStateFn(ctx, height) } -func (mcv *ClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { - if mcv.ValidateSelfClientFn == nil { +func (cv *ClientValidator) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { + if cv.ValidateSelfClientFn == nil { return nil } - return mcv.ValidateSelfClientFn(ctx, clientState) + return cv.ValidateSelfClientFn(ctx, clientState) } From ae49990b87953a679f21a3ddbd29e635e2a9ad9b Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 15 Feb 2024 16:48:05 +0100 Subject: [PATCH 12/12] fix: put back ibctm import from merge conflicts --- modules/core/02-client/keeper/keeper.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index f50ccd97d96..21ab11ffad9 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -17,6 +17,7 @@ import ( "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" "github.com/cosmos/ibc-go/v8/modules/core/exported" + ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" localhost "github.com/cosmos/ibc-go/v8/modules/light-clients/09-localhost" )