refactor: remove hardcoding of tendermint self client validation (#5836)

* chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist

* chore: update service definition URL in 08-wasm stargate accepted queries

* chore: add doc comment to querier test, address nit to move defaultAcceptList

* feat(draft): add custom client validator func

* feat: add SelfClientValidator type alias func and refactor tests to confirm it works

* refactor: updated ibc client keeper to use interface type for self client validation of consensus parameters

* lint: make lint-fix

* chore: merge main and fix linter

* test: cleaned up GetSelfConsensusState tests

* test: added test cases for custom validator logic

* nit: rename receiver arg

* fix: put back ibctm import from merge conflicts

---------

Co-authored-by: chatton <[email protected]>

Author: damiannolan

modules/core/02-client/keeper/client_validator.go

@@ -0,0 +1,126 @@
+package keeper
+
+import (
+	"reflect"
+
+	errorsmod "cosmossdk.io/errors"
+	upgradetypes "cosmossdk.io/x/upgrade/types"
+
+	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"
+	"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
+}

modules/core/02-client/keeper/keeper.go

@@ -3,7 +3,6 @@ package keeper
 import (
 	"errors"
 	"fmt"
-	"reflect"
 	"strings"
 
 	errorsmod "cosmossdk.io/errors"
@@ -15,12 +14,8 @@ 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"
@@ -29,21 +24,23 @@ 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
+	legacySubspace      types.ParamSubspace
+	selfClientValidator types.SelfClientValidator
+	stakingKeeper       types.StakingKeeper
+	upgradeKeeper       types.UpgradeKeeper
 }
 
 // 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,
 	}
 }
 
@@ -63,6 +60,15 @@ 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(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.
 func (k Keeper) GenerateClientIdentifier(ctx sdk.Context, clientType string) string {
 	nextClientSeq := k.GetNextClientSequence(ctx)
@@ -282,96 +288,15 @@ 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
-// 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
+// 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 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 := 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.