refactor!: remove ExportMetadata inteface function (#5782)

* refactor: remove  interface function from ClientState

* imp: add note about performance

* docs: fix ordering

* docs: update godoc

* Apply suggestions from code review

Co-authored-by: Damian Nolan <[email protected]>

* test: add additional metadata keys

* imp: make IterateMetadata private

* docs: remove link reference

* lint: remove unused arg in private func

* docs: add migration doc entry

* markdown lint: remove empty line

* refactor: remove ExportMetadata from 08-wasm

* fix: correctly skip client and consensus keys + hardening tests

* fix: broken link

* rm: unused IterateConsensusMetadata func

* test: add code cov on panic case

* lint: take a break

* lint: fix lint directive

---------

Co-authored-by: Damian Nolan <[email protected]>

docs/docs/03-light-clients/01-developer-guide/09-setup.md → docs/docs/03-light-clients/01-developer-guide/08-setup.md

@@ -1,7 +1,7 @@
 ---
 title: Setup
 sidebar_label: Setup
-sidebar_position: 9
+sidebar_position: 8
 slug: /ibc/light-clients/setup
 ---
 

docs/docs/03-light-clients/04-wasm/03-integration.md

@@ -335,7 +335,7 @@ func CreateWasmUpgradeHandler(
 }
 ```
 
-Or alternatively the parameter can be updated via a governance proposal (see at the bottom of section [`Creating clients`](../01-developer-guide/09-setup.md#creating-clients) for an example of how to do this).
+Or alternatively the parameter can be updated via a governance proposal (see at the bottom of section [`Creating clients`](../01-developer-guide/08-setup.md#creating-clients) for an example of how to do this).
 
 ## Adding the module to the store
 

docs/docs/03-light-clients/04-wasm/07-contracts.md

@@ -34,7 +34,6 @@ The Wasm light client contract is expected to store the client and consensus sta
 ```go
 type QueryMsg struct {
   Status               *StatusMsg               `json:"status,omitempty"`
-  ExportMetadata       *ExportMetadataMsg       `json:"export_metadata,omitempty"`
   TimestampAtHeight    *TimestampAtHeightMsg    `json:"timestamp_at_height,omitempty"`
   VerifyClientMessage  *VerifyClientMessageMsg  `json:"verify_client_message,omitempty"`
   CheckForMisbehaviour *CheckForMisbehaviourMsg `json:"check_for_misbehaviour,omitempty"`
@@ -45,7 +44,6 @@ type QueryMsg struct {
 #[cw_serde]
 pub enum QueryMsg {
   Status(StatusMsg),
-  ExportMetadata(ExportMetadataMsg),
   TimestampAtHeight(TimestampAtHeightMsg),
   VerifyClientMessage(VerifyClientMessageRaw),
   CheckForMisbehaviour(CheckForMisbehaviourMsgRaw),
@@ -55,7 +53,6 @@ pub enum QueryMsg {
 To learn what it is expected from the Wasm light client contract when processing each message, please read the corresponding section of the [Light client developer guide](../01-developer-guide/01-overview.md):
 
 - For `StatusMsg`, see the section [`Status` method](../01-developer-guide/02-client-state.md#status-method).
-- For `ExportMetadataMsg`, see the section [Genesis metadata](../01-developer-guide/08-genesis.md#genesis-metadata).
 - For `TimestampAtHeightMsg`, see the section [`GetTimestampAtHeight` method](../01-developer-guide/02-client-state.md#gettimestampatheight-method).
 - For `VerifyClientMessageMsg`, see the section [`VerifyClientMessage`](../01-developer-guide/04-updates-and-misbehaviour.md#verifyclientmessage).
 - For `CheckForMisbehaviourMsg`, see the section [`CheckForMisbehaviour` method](../01-developer-guide/02-client-state.md#checkformisbehaviour-method).

docs/docs/05-migrations/13-v8-to-v9.md

@@ -53,4 +53,9 @@ Please use the new functions `path.Setup`, `path.SetupClients`, `path.SetupConne
 
 ### API removals
 
+The `ExportMetadata` interface function has been removed from the `ClientState` interface. Core IBC will export all key/value's within the 02-client store.  
+
+### 08-wasm
+
+The `ExportMetadataMsg` has been removed and is no longer required for contracts to implement. Core IBC will handle exporting all key/value's written to the store by a light client contract.
 The `ZeroCustomFields` interface function has been removed from the `ClientState` interface. Core IBC only used this function to set tendermint client states when scheduling an IBC software upgrade. The interface function has been replaced by a type assertion.  

modules/core/02-client/genesis.go

@@ -59,6 +59,8 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, gs types.GenesisState) {
 }
 
 // ExportGenesis returns the ibc client submodule's exported genesis.
+// NOTE: the export process is not optimized, it will iterate three
+// times over the 02-client sub-store.
 func ExportGenesis(ctx sdk.Context, k keeper.Keeper) types.GenesisState {
 	genClients := k.GetAllGenesisClients(ctx)
 	clientsMetadata, err := k.GetAllClientMetadata(ctx, genClients)

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

@@ -154,6 +154,42 @@ func (k Keeper) IterateConsensusStates(ctx sdk.Context, cb func(clientID string,
 	}
 }
 
+// iterateMetadata provides an iterator over all stored metadata keys in the client store.
+// For each metadata object, it will perform a callback.
+func (k Keeper) iterateMetadata(ctx sdk.Context, cb func(clientID string, key, value []byte) bool) {
+	store := ctx.KVStore(k.storeKey)
+	iterator := storetypes.KVStorePrefixIterator(store, host.KeyClientStorePrefix)
+
+	defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() })
+	for ; iterator.Valid(); iterator.Next() {
+		split := strings.Split(string(iterator.Key()), "/")
+		if len(split) == 3 && split[2] == string(host.KeyClientState) {
+			// skip client state keys
+			continue
+		}
+
+		if len(split) == 4 && split[2] == string(host.KeyConsensusStatePrefix) {
+			// skip consensus state keys
+			continue
+		}
+
+		if split[0] != string(host.KeyClientStorePrefix) {
+			panic(errorsmod.Wrapf(host.ErrInvalidPath, "path does not begin with client store prefix: expected %s, got %s", host.KeyClientStorePrefix, split[0]))
+		}
+		if strings.TrimSpace(split[1]) == "" {
+			panic(errorsmod.Wrap(host.ErrInvalidPath, "clientID is empty"))
+		}
+
+		clientID := split[1]
+
+		key := []byte(strings.Join(split[2:], "/"))
+
+		if cb(clientID, key, iterator.Value()) {
+			break
+		}
+	}
+}
+
 // GetAllGenesisClients returns all the clients in state with their client ids returned as IdentifiedClientState
 func (k Keeper) GetAllGenesisClients(ctx sdk.Context) types.IdentifiedClientStates {
 	var genClients types.IdentifiedClientStates
@@ -169,30 +205,23 @@ func (k Keeper) GetAllGenesisClients(ctx sdk.Context) types.IdentifiedClientStat
 // of IdentifiedGenesisMetadata necessary for exporting and importing client metadata
 // into the client store.
 func (k Keeper) GetAllClientMetadata(ctx sdk.Context, genClients []types.IdentifiedClientState) ([]types.IdentifiedGenesisMetadata, error) {
+	metadataMap := make(map[string][]types.GenesisMetadata)
+	k.iterateMetadata(ctx, func(clientID string, key, value []byte) bool {
+		metadataMap[clientID] = append(metadataMap[clientID], types.NewGenesisMetadata(key, value))
+		return false
+	})
+
 	genMetadata := make([]types.IdentifiedGenesisMetadata, 0)
 	for _, ic := range genClients {
-		cs, err := types.UnpackClientState(ic.ClientState)
-		if err != nil {
-			return nil, err
+		metadata := metadataMap[ic.ClientId]
+		if len(metadata) != 0 {
+			genMetadata = append(genMetadata, types.NewIdentifiedGenesisMetadata(
+				ic.ClientId,
+				metadata,
+			))
 		}
-		gms := cs.ExportMetadata(k.ClientStore(ctx, ic.ClientId))
-		if len(gms) == 0 {
-			continue
-		}
-		clientMetadata := make([]types.GenesisMetadata, len(gms))
-		for i, metadata := range gms {
-			cmd, ok := metadata.(types.GenesisMetadata)
-			if !ok {
-				return nil, errorsmod.Wrapf(types.ErrInvalidClientMetadata, "expected metadata type: %T, got: %T",
-					types.GenesisMetadata{}, cmd)
-			}
-			clientMetadata[i] = cmd
-		}
-		genMetadata = append(genMetadata, types.NewIdentifiedGenesisMetadata(
-			ic.ClientId,
-			clientMetadata,
-		))
 	}
+
 	return genMetadata, nil
 }
 

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

@@ -23,6 +23,7 @@ import (
 	"github.com/cosmos/ibc-go/v8/modules/core/02-client/keeper"
 	"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"
 	"github.com/cosmos/ibc-go/v8/modules/core/exported"
 	solomachine "github.com/cosmos/ibc-go/v8/modules/light-clients/06-solomachine"
 	ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
@@ -252,33 +253,59 @@ func (suite KeeperTestSuite) TestGetAllGenesisClients() { //nolint:govet // this
 }
 
 func (suite KeeperTestSuite) TestGetAllGenesisMetadata() { //nolint:govet // this is a test, we are okay with copying locks
+	clientA, clientB := "07-tendermint-1", "clientB"
+
+	// create some starting state
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), clientA, &ibctm.ClientState{})
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientA, types.NewHeight(0, 1), &ibctm.ConsensusState{})
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientA, types.NewHeight(0, 2), &ibctm.ConsensusState{})
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientA, types.NewHeight(0, 3), &ibctm.ConsensusState{})
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientA, types.NewHeight(2, 300), &ibctm.ConsensusState{})
+
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), clientB, &ibctm.ClientState{})
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientB, types.NewHeight(1, 100), &ibctm.ConsensusState{})
+	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientB, types.NewHeight(2, 300), &ibctm.ConsensusState{})
+
+	// NOTE: correct ordering of expected value is required
+	// Ordering is typically determined by the lexographic ordering of the height passed into each key.
 	expectedGenMetadata := []types.IdentifiedGenesisMetadata{
 		types.NewIdentifiedGenesisMetadata(
-			"07-tendermint-1",
+			clientA,
 			[]types.GenesisMetadata{
+				types.NewGenesisMetadata([]byte(fmt.Sprintf("%s/%s", host.KeyClientState, "clientMetadata")), []byte("value")),
 				types.NewGenesisMetadata(ibctm.ProcessedTimeKey(types.NewHeight(0, 1)), []byte("foo")),
 				types.NewGenesisMetadata(ibctm.ProcessedTimeKey(types.NewHeight(0, 2)), []byte("bar")),
 				types.NewGenesisMetadata(ibctm.ProcessedTimeKey(types.NewHeight(0, 3)), []byte("baz")),
+				types.NewGenesisMetadata(ibctm.ProcessedHeightKey(types.NewHeight(2, 300)), []byte(types.NewHeight(1, 100).String())),
 			},
 		),
 		types.NewIdentifiedGenesisMetadata(
-			"clientB",
+			clientB,
 			[]types.GenesisMetadata{
 				types.NewGenesisMetadata(ibctm.ProcessedTimeKey(types.NewHeight(1, 100)), []byte("val1")),
+				types.NewGenesisMetadata(ibctm.ProcessedHeightKey(types.NewHeight(2, 300)), []byte(types.NewHeight(1, 100).String())),
 				types.NewGenesisMetadata(ibctm.ProcessedTimeKey(types.NewHeight(2, 300)), []byte("val2")),
+				types.NewGenesisMetadata([]byte("key"), []byte("value")),
 			},
 		),
 	}
 
 	genClients := []types.IdentifiedClientState{
-		types.NewIdentifiedClientState("07-tendermint-1", &ibctm.ClientState{}), types.NewIdentifiedClientState("clientB", &ibctm.ClientState{}),
+		types.NewIdentifiedClientState(clientA, &ibctm.ClientState{}), types.NewIdentifiedClientState(clientB, &ibctm.ClientState{}),
 	}
 
 	suite.chainA.App.GetIBCKeeper().ClientKeeper.SetAllClientMetadata(suite.chainA.GetContext(), expectedGenMetadata)
 
 	actualGenMetadata, err := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetAllClientMetadata(suite.chainA.GetContext(), genClients)
 	suite.Require().NoError(err, "get client metadata returned error unexpectedly")
 	suite.Require().Equal(expectedGenMetadata, actualGenMetadata, "retrieved metadata is unexpected")
+
+	// set invalid key in client store which will cause panic during iteration
+	clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), "")
+	clientStore.Set([]byte("key"), []byte("val"))
+	suite.Require().Panics(func() {
+		suite.chainA.App.GetIBCKeeper().ClientKeeper.GetAllClientMetadata(suite.chainA.GetContext(), genClients) //nolint:errcheck // we expect a panic
+	})
 }
 
 func (suite KeeperTestSuite) TestGetConsensusState() { //nolint:govet // this is a test, we are okay with copying locks

modules/core/02-client/migrations/v7/solomachine.go

@@ -77,11 +77,6 @@ func (ClientState) Initialize(_ sdk.Context, _ codec.BinaryCodec, _ storetypes.K
 	panic(errors.New("legacy solo machine is deprecated"))
 }
 
-// ExportMetadata panics!
-func (ClientState) ExportMetadata(_ storetypes.KVStore) []exported.GenesisMetadata {
-	panic(errors.New("legacy solo machine is deprecated"))
-}
-
 // CheckForMisbehaviour panics!
 func (ClientState) CheckForMisbehaviour(_ sdk.Context, _ codec.BinaryCodec, _ storetypes.KVStore, _ exported.ClientMessage) bool {
 	panic(errors.New("legacy solo machine is deprecated"))

modules/core/exported/client.go

@@ -55,9 +55,6 @@ type ClientState interface {
 	// Status must return the status of the client. Only Active clients are allowed to process packets.
 	Status(ctx sdk.Context, clientStore storetypes.KVStore, cdc codec.BinaryCodec) Status
 
-	// ExportMetadata must export metadata stored within the clientStore for genesis export
-	ExportMetadata(clientStore storetypes.KVStore) []GenesisMetadata
-
 	// GetTimestampAtHeight must return the timestamp for the consensus state associated with the provided height.
 	GetTimestampAtHeight(
 		ctx sdk.Context,

modules/light-clients/06-solomachine/client_state.go

@@ -87,11 +87,6 @@ func (cs ClientState) Initialize(_ sdk.Context, cdc codec.BinaryCodec, clientSto
 	return nil
 }
 
-// ExportMetadata is a no-op since solomachine does not store any metadata in client store
-func (ClientState) ExportMetadata(_ storetypes.KVStore) []exported.GenesisMetadata {
-	return nil
-}
-
 // VerifyUpgradeAndUpdateState returns an error since solomachine client does not support upgrades
 func (ClientState) VerifyUpgradeAndUpdateState(
 	_ sdk.Context, _ codec.BinaryCodec, _ storetypes.KVStore,