address my self-review comments and some of rabbit's comments

CHANGELOG.md

@@ -40,6 +40,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking
 
+* (core/02-client, light-clients) [\#5806](https://github.com/cosmos/ibc-go/pull/5806) Decouple light client routing from their encoding structure.
+
 ### State Machine Breaking
 
 ### Improvements

docs/docs/03-light-clients/01-developer-guide/01-overview.md

@@ -47,7 +47,7 @@ For example a light client module may need to:
 - recover and upgrade clients
 - verify membership and non-membership
 
-The methods which make up this interface are detailed at a more granular level in the [LightClientModule section of this guide](02-light-client-module.md).
+The methods which make up this interface are detailed at a more granular level in the [`LightClientModule` section of this guide](02-light-client-module.md).
 
 Please refer to the `07-tendermint`'s [`LightClientModule` definition](https://github.com/cosmos/ibc-go/blob/501a8462345da099144efe91d495bfcfa18d760d/modules/light-clients/07-tendermint/light_client_module.go#L17) for more information. 
 
@@ -63,7 +63,7 @@ For example:
 - Constraints used for client upgrades.
 
 The `ClientState` type maintained within the light client module *must* implement the [`ClientState`](https://github.com/cosmos/ibc-go/tree/02-client-refactor-beta1/modules/core/exported/client.go#L36) interface defined in `core/modules/exported/client.go`.
-The methods which make up this interface are detailed at a more granular level in the [ClientState section of this guide](03-client-state.md).
+The methods which make up this interface are detailed at a more granular level in the [`ClientState` section of this guide](03-client-state.md).
 
 Please refer to the `07-tendermint` light client module's [`ClientState` definition](https://github.com/cosmos/ibc-go/tree/02-client-refactor-beta1/proto/ibc/lightclients/tendermint/v1/tendermint.proto#L18) containing information such as chain ID, status, latest height, unbonding period and proof specifications.
 

docs/docs/03-light-clients/01-developer-guide/03-client-state.md

@@ -8,12 +8,12 @@ slug: /ibc/light-clients/client-state
 
 # Implementing the `ClientState` interface
 
-Learn how to implement the [`ClientState`](https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/exported/client.go#L36) interface. This list of methods described here does not include all methods of the interface. Some methods are explained in detail in the relevant sections of the guide.
+Learn how to implement the [`ClientState`](https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/exported/client.go#L36) interface.
 
 ## `ClientType` method
 
 `ClientType` should return a unique string identifier of the light client. This will be used when generating a client identifier.
-The format is created as follows: `ClientType-{N}` where `{N}` is the unique global nonce associated with a specific client.
+The format is created as follows: `{client-type}-{N}` where `{N}` is the unique global nonce associated with a specific client (e.g `07-tendermint-0`).
 
 ## `Validate` method
 

docs/docs/03-light-clients/01-developer-guide/04-consensus-state.md

@@ -20,9 +20,7 @@ This is the type of client consensus. It should be the same as the `ClientType`
 
 ## `GetTimestamp` method
 
-*Deprecated*: soon to be removed from interface
-
-`GetTimestamp` should return the timestamp (in nanoseconds) of the consensus state snapshot.
+`GetTimestamp` should return the timestamp (in nanoseconds) of the consensus state snapshot. This function has been deprecated and will be removed in a future release.
 
 ## `ValidateBasic` method
 

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

@@ -76,7 +76,7 @@ The following functions have also been removed from the `ClientState` interface:
 |`ExportMetadata`               |                             |
 |`ZeroCustomFields`             |                             |
 
-Please check also the [light client developer guide](../03-light-clients/01-developer-guide/01-overview.md) for more information. The light client module implementation for `07-tendermint` may also be useful as reference.
+Please check also the [Light client developer guide](../03-light-clients/01-developer-guide/01-overview.md) for more information. The light client module implementation for `07-tendermint` may also be useful as reference.
 
 ### 07-tendermint
 

e2e/tests/transfer/localhost_test.go

@@ -55,13 +55,15 @@ func (s *LocalhostTransferTestSuite) TestMsgTransfer_Localhost() {
 	t.Run("verify begin blocker was executed", func(t *testing.T) {
 		cs, err := s.QueryClientState(ctx, chainA, exported.LocalhostClientID)
 		s.Require().NoError(err)
-		originalHeight := cs.(*localhost.ClientState).LatestHeight
+
+		localhostClientState := cs.(*localhost.ClientState)
+		originalHeight := localhostClientState.LatestHeight
 
 		s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA), "failed to wait for blocks")
 
 		cs, err = s.QueryClientState(ctx, chainA, exported.LocalhostClientID)
 		s.Require().NoError(err)
-		s.Require().True(cs.(*localhost.ClientState).LatestHeight.GT(originalHeight), "client state height was not incremented")
+		s.Require().True(localhostClientState.LatestHeight.GT(originalHeight), "client state height was not incremented")
 	})
 
 	t.Run("channel open init localhost", func(t *testing.T) {

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

@@ -344,22 +344,22 @@ func (suite *KeeperTestSuite) TestQueryConsensusStates() {
 				path := ibctesting.NewPath(suite.chainA, suite.chainB)
 				path.SetupClients()
 
-				height1 := path.EndpointA.GetClientLatestHeight()
+				height1 := path.EndpointA.GetClientLatestHeight().(types.Height)
 				expConsensusStates = append(
 					expConsensusStates,
 					types.NewConsensusStateWithHeight(
-						height1.(types.Height),
+						height1,
 						path.EndpointA.GetConsensusState(height1),
 					))
 
 				err := path.EndpointA.UpdateClient()
 				suite.Require().NoError(err)
 
-				height2 := path.EndpointA.GetClientLatestHeight()
+				height2 := path.EndpointA.GetClientLatestHeight().(types.Height)
 				expConsensusStates = append(
 					expConsensusStates,
 					types.NewConsensusStateWithHeight(
-						height2.(types.Height),
+						height2,
 						path.EndpointA.GetConsensusState(height2),
 					))
 

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

@@ -337,20 +337,20 @@ func (suite *KeeperTestSuite) TestGetConsensusState() {
 // 2 clients in total are created on chainA. The first client is updated so it contains an initial consensus state
 // and a consensus state at the update height.
 func (suite *KeeperTestSuite) TestGetAllConsensusStates() {
-	path := ibctesting.NewPath(suite.chainA, suite.chainB)
-	path.SetupClients()
+	path1 := ibctesting.NewPath(suite.chainA, suite.chainB)
+	path1.SetupClients()
 
-	expConsensusHeight0 := path.EndpointA.GetClientLatestHeight()
-	consensusState0, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, expConsensusHeight0)
+	expConsensusHeight0 := path1.EndpointA.GetClientLatestHeight()
+	consensusState0, ok := suite.chainA.GetConsensusState(path1.EndpointA.ClientID, expConsensusHeight0)
 	suite.Require().True(ok)
 
 	// update client to create a second consensus state
-	err := path.EndpointA.UpdateClient()
+	err := path1.EndpointA.UpdateClient()
 	suite.Require().NoError(err)
 
-	expConsensusHeight1 := path.EndpointA.GetClientLatestHeight()
+	expConsensusHeight1 := path1.EndpointA.GetClientLatestHeight()
 	suite.Require().True(expConsensusHeight1.GT(expConsensusHeight0))
-	consensusState1, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, expConsensusHeight1)
+	consensusState1, ok := suite.chainA.GetConsensusState(path1.EndpointA.ClientID, expConsensusHeight1)
 	suite.Require().True(ok)
 
 	expConsensus := []exported.ConsensusState{
@@ -369,7 +369,7 @@ func (suite *KeeperTestSuite) TestGetAllConsensusStates() {
 	expConsensus2 := []exported.ConsensusState{consensusState2}
 
 	expConsensusStates := types.ClientsConsensusStates{
-		types.NewClientConsensusStates(path.EndpointA.ClientID, []types.ConsensusStateWithHeight{
+		types.NewClientConsensusStates(path1.EndpointA.ClientID, []types.ConsensusStateWithHeight{
 			types.NewConsensusStateWithHeight(expConsensusHeight0.(types.Height), expConsensus[0]),
 			types.NewConsensusStateWithHeight(expConsensusHeight1.(types.Height), expConsensus[1]),
 		}),

modules/core/02-client/types/router.go

@@ -25,9 +25,16 @@ func NewRouter(key storetypes.StoreKey) *Router {
 }
 
 // AddRoute adds LightClientModule for a given module name. It returns the Router
-// so AddRoute calls can be linked. It will panic if the Router is sealed.
-// The store provider will be registered on the light client module.
+// so AddRoute calls can be linked. The store provider will be registered on the
+// light client module. This function will panic if:
+// - the Router is sealed,
+// - or a module is already registered for the provided client type,
+// - or the client type is invalid.
 func (rtr *Router) AddRoute(clientType string, module exported.LightClientModule) *Router {
+	if err := ValidateClientType(clientType); err != nil {
+		panic(err)
+	}
+
 	if rtr.HasRoute(clientType) {
 		panic(fmt.Errorf("route %s has already been registered", module))
 	}

modules/core/keeper/msg_server.go

@@ -27,7 +27,7 @@ var (
 )
 
 // CreateClient defines a rpc handler method for MsgCreateClient.
-// NOTE: The raw bytes of the conrete types encoded into protobuf.Any is passed to the client keeper.
+// NOTE: The raw bytes of the concrete types encoded into protobuf.Any is passed to the client keeper.
 // The 02-client handler will route to the appropriate light client module based on client type and it is the responsibility
 // of the light client module to unmarshal and interpret the proto encoded bytes.
 // Backwards compatibility with older versions of ibc-go is maintained through the light client module reconstructing and encoding
@@ -64,7 +64,7 @@ func (k Keeper) UpdateClient(goCtx context.Context, msg *clienttypes.MsgUpdateCl
 }
 
 // UpgradeClient defines a rpc handler method for MsgUpgradeClient.
-// NOTE: The raw bytes of the conrete types encoded into protobuf.Any is passed to the client keeper.
+// NOTE: The raw bytes of the concrete types encoded into protobuf.Any is passed to the client keeper.
 // The 02-client handler will route to the appropriate light client module based on client identifier and it is the responsibility
 // of the light client module to unmarshal and interpret the proto encoded bytes.
 // Backwards compatibility with older versions of ibc-go is maintained through the light client module reconstructing and encoding

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

@@ -1,5 +1,5 @@
 /*
-Package solomachine implements a concrete ClientState, ConsensusState,
+Package solomachine implements a concrete LightClientModule, ClientState, ConsensusState,
 Header and Misbehaviour types for the Solo Machine light client.
 This implementation is based off the ICS 06 specification
 (https://github.com/cosmos/ibc/tree/master/spec/client/ics-006-solo-machine-client)

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

@@ -9,6 +9,8 @@ import (
 	host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
 )
 
+// getClientState retrieves the client state from the store using the provided KVStore and codec.
+// It returns the unmarshaled ClientState and a boolean indicating if the state was found.
 func getClientState(store storetypes.KVStore, cdc codec.BinaryCodec) (*ClientState, bool) {
 	bz := store.Get(host.ClientStateKey())
 	if len(bz) == 0 {

modules/light-clients/07-tendermint/doc.go

@@ -1,5 +1,5 @@
 /*
-Package tendermint implements a concrete ClientState, ConsensusState,
+Package tendermint implements a concrete LightClientModule, ClientState, ConsensusState,
 Header, Misbehaviour and types for the Tendermint consensus light client.
 This implementation is based off the ICS 07 specification
 (https://github.com/cosmos/ibc/tree/main/spec/client/ics-007-tendermint-client)

modules/light-clients/07-tendermint/internal/keeper/keeper.go

@@ -16,7 +16,7 @@ type Keeper struct {
 	authority string
 }
 
-// NewKeeper returns a new instance of the Keeper
+// NewKeeper returns a new instance of the Keeper. It panics if the authority is empty.
 func NewKeeper(cdc codec.BinaryCodec, authority string) Keeper {
 	if strings.TrimSpace(authority) == "" {
 		panic(errors.New("authority must be non-empty"))

modules/light-clients/07-tendermint/store.go

@@ -50,6 +50,8 @@ func setClientState(clientStore storetypes.KVStore, cdc codec.BinaryCodec, clien
 	clientStore.Set(key, val)
 }
 
+// getClientState retrieves the client state from the store using the provided KVStore and codec.
+// It returns the unmarshaled ClientState and a boolean indicating if the state was found.
 func getClientState(store storetypes.KVStore, cdc codec.BinaryCodec) (*ClientState, bool) {
 	bz := store.Get(host.ClientStateKey())
 	if len(bz) == 0 {

modules/light-clients/08-wasm/doc.go

@@ -1,5 +1,5 @@
 /*
-Package wasm implements a concrete ClientState, ConsensusState,
+Package wasm implements a concrete LightClientModule, ClientState, ConsensusState,
 ClientMessage and types for the proxy light client module communicating
 with underlying Wasm light clients.
 This implementation is based off the ICS 08 specification

modules/light-clients/08-wasm/keeper/msg_server_test.go

@@ -253,6 +253,7 @@ func (suite *KeeperTestSuite) TestMsgMigrateContract() {
 
 	for _, tc := range testCases {
 		suite.Run(tc.name, func() {
+			var ok bool
 			suite.SetupWasmWithMockVM()
 
 			storeWasmCode(suite, wasmtesting.Code)
@@ -263,7 +264,8 @@ func (suite *KeeperTestSuite) TestMsgMigrateContract() {
 			suite.Require().NoError(err)
 
 			// this is the old client state
-			expClientState = endpoint.GetClientState().(*types.ClientState)
+			expClientState, ok = endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 
 			tc.malleate()
 

modules/light-clients/08-wasm/types/client_state_test.go

@@ -86,7 +86,8 @@ func (suite *TypesTestSuite) TestStatus() {
 			tc.malleate()
 
 			clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), endpoint.ClientID)
-			clientState := endpoint.GetClientState().(*types.ClientState)
+			clientState, ok := endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 
 			status := clientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec())
 			suite.Require().Equal(tc.expStatus, status)
@@ -153,7 +154,9 @@ func (suite *TypesTestSuite) TestGetTimestampAtHeight() {
 			suite.Require().NoError(err)
 
 			clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), endpoint.ClientID)
-			clientState := endpoint.GetClientState().(*types.ClientState)
+			clientState, ok := endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
+
 			height = clientState.LatestHeight
 
 			tc.malleate()
@@ -453,6 +456,7 @@ func (suite *TypesTestSuite) TestVerifyMembership() {
 
 	for _, tc := range testCases {
 		suite.Run(tc.name, func() {
+			var ok bool
 			suite.SetupWasmWithMockVM()
 
 			endpoint := wasmtesting.NewWasmEndpoint(suite.chainA)
@@ -465,7 +469,8 @@ func (suite *TypesTestSuite) TestVerifyMembership() {
 			value = []byte("value")
 
 			clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), endpoint.ClientID)
-			clientState = endpoint.GetClientState().(*types.ClientState)
+			clientState, ok = endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 
 			tc.malleate()
 
@@ -594,6 +599,7 @@ func (suite *TypesTestSuite) TestVerifyNonMembership() {
 
 	for _, tc := range testCases {
 		suite.Run(tc.name, func() {
+			var ok bool
 			suite.SetupWasmWithMockVM()
 
 			endpoint := wasmtesting.NewWasmEndpoint(suite.chainA)
@@ -605,7 +611,8 @@ func (suite *TypesTestSuite) TestVerifyNonMembership() {
 			proofHeight = clienttypes.NewHeight(0, 1)
 
 			clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), endpoint.ClientID)
-			clientState = endpoint.GetClientState().(*types.ClientState)
+			clientState, ok = endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 
 			tc.malleate()
 

modules/light-clients/08-wasm/types/misbehaviour_handle_test.go

@@ -77,7 +77,8 @@ func (suite *TypesTestSuite) TestCheckForMisbehaviour() {
 			err := endpoint.CreateClient()
 			suite.Require().NoError(err)
 
-			clientState := endpoint.GetClientState().(*types.ClientState)
+			clientState, ok := endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 			clientMessage = &types.ClientMessage{
 				Data: clienttypes.MustMarshalClientMessage(suite.chainA.App.AppCodec(), wasmtesting.MockTendermintClientMisbehaviour),
 			}

modules/light-clients/08-wasm/types/proposal_handle_test.go

@@ -95,7 +95,8 @@ func (suite *TypesTestSuite) TestCheckSubstituteAndUpdateState() {
 			suite.Require().NoError(err)
 
 			subjectClientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), endpointA.ClientID)
-			subjectClientState := endpointA.GetClientState().(*types.ClientState)
+			subjectClientState, ok := endpointA.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 
 			substituteEndpoint := wasmtesting.NewWasmEndpoint(suite.chainA)
 			err = substituteEndpoint.CreateClient()

modules/light-clients/08-wasm/types/querier_test.go

@@ -108,7 +108,9 @@ func (suite *TypesTestSuite) TestCustomQuery() {
 			tc.malleate()
 
 			clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), endpoint.ClientID)
-			clientState := endpoint.GetClientState().(*types.ClientState)
+			clientState, ok := endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
+
 			clientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec())
 
 			// reset query plugins after each test

modules/light-clients/08-wasm/types/store.go

@@ -32,6 +32,8 @@ var (
 	substitutePrefix = []byte("substitute/")
 )
 
+// GetClientState retrieves the client state from the store using the provided KVStore and codec.
+// It returns the unmarshaled ClientState and a boolean indicating if the state was found.
 func GetClientState(store storetypes.KVStore, cdc codec.BinaryCodec) (*ClientState, bool) {
 	bz := store.Get(host.ClientStateKey())
 	if len(bz) == 0 {

modules/light-clients/08-wasm/types/update_test.go

@@ -79,7 +79,8 @@ func (suite *TypesTestSuite) TestUpdateState() {
 
 					bz := store.Get(host.ClientStateKey())
 					suite.Require().NotEmpty(bz)
-					clientState := clienttypes.MustUnmarshalClientState(suite.chainA.Codec, bz).(*types.ClientState)
+					clientState, ok := clienttypes.MustUnmarshalClientState(suite.chainA.Codec, bz).(*types.ClientState)
+					suite.Require().True(ok)
 					clientState.LatestHeight = mockHeight
 					expectedClientStateBz = clienttypes.MustMarshalClientState(suite.chainA.App.AppCodec(), clientState)
 					store.Set(host.ClientStateKey(), expectedClientStateBz)
@@ -146,7 +147,8 @@ func (suite *TypesTestSuite) TestUpdateState() {
 
 			tc.malleate()
 
-			clientState := endpoint.GetClientState().(*types.ClientState)
+			clientState, ok := endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 
 			var heights []exported.Height
 			updateState := func() {
@@ -222,7 +224,8 @@ func (suite *TypesTestSuite) TestUpdateStateOnMisbehaviour() {
 					// set new client state in store
 					bz := store.Get(host.ClientStateKey())
 					suite.Require().NotEmpty(bz)
-					clientState := clienttypes.MustUnmarshalClientState(suite.chainA.App.AppCodec(), bz).(*types.ClientState)
+					clientState, ok := clienttypes.MustUnmarshalClientState(suite.chainA.App.AppCodec(), bz).(*types.ClientState)
+					suite.Require().True(ok)
 					clientState.LatestHeight = mockHeight
 					expectedClientStateBz = clienttypes.MustMarshalClientState(suite.chainA.App.AppCodec(), clientState)
 					store.Set(host.ClientStateKey(), expectedClientStateBz)
@@ -273,8 +276,8 @@ func (suite *TypesTestSuite) TestUpdateStateOnMisbehaviour() {
 			clientMsg = &types.ClientMessage{
 				Data: clienttypes.MustMarshalClientMessage(suite.chainA.App.AppCodec(), wasmtesting.MockTendermintClientMisbehaviour),
 			}
-			clientState := endpoint.GetClientState().(*types.ClientState)
-
+			clientState, ok := endpoint.GetClientState().(*types.ClientState)
+			suite.Require().True(ok)
 			tc.malleate()
 
 			if tc.panicErr == nil {