chore: remove GetHeight from ClientMessage interface (cosmos#1285)

* adding new consensus heights return value from UpdateState interface, updating 02-client events emission

* removing GetHeight() from ClientMessage interface, updating tests

* updating godocs to include returned consensus height data

* removing unused event emission func, adding deprecation notice, removing GetHeight from solomachine Header, updating tests

* removing error return from UpdateState, returning height on duplicate header update

* updating event emissions as per suggestions

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

@@ -70,14 +70,6 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte
 		return err
 	}
 
-	// Marshal the ClientMessage as an Any and encode the resulting bytes to hex.
-	// This prevents the event value from containing invalid UTF-8 characters
-	// which may cause data to be lost when JSON encoding/decoding.
-	clientMsgStr := hex.EncodeToString(types.MustMarshalClientMessage(k.cdc, clientMsg))
-
-	// set default consensus height with header height
-	consensusHeight := clientMsg.GetHeight()
-
 	foundMisbehaviour := clientState.CheckForMisbehaviour(ctx, k.cdc, clientStore, clientMsg)
 	if foundMisbehaviour {
 		clientState.UpdateStateOnMisbehaviour(ctx, k.cdc, clientStore, clientMsg)
@@ -96,14 +88,14 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte
 			)
 		}()
 
-		EmitSubmitMisbehaviourEventOnUpdate(ctx, clientID, clientState.ClientType(), consensusHeight, clientMsgStr)
+		EmitSubmitMisbehaviourEvent(ctx, clientID, clientState)
 
 		return nil
 	}
 
-	clientState.UpdateState(ctx, k.cdc, clientStore, clientMsg)
+	consensusHeights := clientState.UpdateState(ctx, k.cdc, clientStore, clientMsg)
 
-	k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
+	k.Logger(ctx).Info("client state updated", "client-id", clientID, "heights", consensusHeights)
 
 	defer func() {
 		telemetry.IncrCounterWithLabels(
@@ -117,8 +109,13 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte
 		)
 	}()
 
+	// Marshal the ClientMessage as an Any and encode the resulting bytes to hex.
+	// This prevents the event value from containing invalid UTF-8 characters
+	// which may cause data to be lost when JSON encoding/decoding.
+	clientMsgStr := hex.EncodeToString(types.MustMarshalClientMessage(k.cdc, clientMsg))
+
 	// emitting events in the keeper emits for both begin block and handler client updates
-	EmitUpdateClientEvent(ctx, clientID, clientState.ClientType(), consensusHeight, clientMsgStr)
+	EmitUpdateClientEvent(ctx, clientID, clientState.ClientType(), consensusHeights, clientMsgStr)
 
 	return nil
 }

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

@@ -1,6 +1,8 @@
 package keeper
 
 import (
+	"strings"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
@@ -24,13 +26,26 @@ func EmitCreateClientEvent(ctx sdk.Context, clientID string, clientState exporte
 }
 
 // EmitUpdateClientEvent emits an update client event
-func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeight exported.Height, clientMsgStr string) {
+func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, clientMsgStr string) {
+	var consensusHeightAttr string
+	if len(consensusHeights) != 0 {
+		consensusHeightAttr = consensusHeights[0].String()
+	}
+
+	var consensusHeightsAttr []string
+	for _, height := range consensusHeights {
+		consensusHeightsAttr = append(consensusHeightsAttr, height.String())
+	}
+
 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
 			types.EventTypeUpdateClient,
 			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
 			sdk.NewAttribute(types.AttributeKeyClientType, clientType),
-			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
+			// Deprecated: AttributeKeyConsensusHeight is deprecated and will be removed in a future release.
+			// Please use AttributeKeyConsensusHeights instead.
+			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeightAttr),
+			sdk.NewAttribute(types.AttributeKeyConsensusHeights, strings.Join(consensusHeightsAttr, ",")),
 			sdk.NewAttribute(types.AttributeKeyHeader, clientMsgStr),
 		),
 		sdk.NewEvent(
@@ -78,16 +93,3 @@ func EmitSubmitMisbehaviourEvent(ctx sdk.Context, clientID string, clientState e
 		),
 	)
 }
-
-// EmitSubmitMisbehaviourEventOnUpdate emits a client misbehaviour event on a client update event
-func EmitSubmitMisbehaviourEventOnUpdate(ctx sdk.Context, clientID string, clientType string, consensusHeight exported.Height, headerStr string) {
-	ctx.EventManager().EmitEvent(
-		sdk.NewEvent(
-			types.EventTypeSubmitMisbehaviour,
-			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
-			sdk.NewAttribute(types.AttributeKeyClientType, clientType),
-			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
-			sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
-		),
-	)
-}

modules/core/02-client/legacy/v100/solomachine.go

@@ -108,7 +108,7 @@ func (cs *ClientState) VerifyClientMessage(
 }
 
 // UpdateState panis!
-func (cs *ClientState) UpdateState(_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientMessage) error {
+func (cs *ClientState) UpdateState(_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientMessage) []exported.Height {
 	panic("legacy solo machine is deprecated!")
 }
 

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

@@ -8,11 +8,12 @@ import (
 
 // IBC client events
 const (
-	AttributeKeyClientID        = "client_id"
-	AttributeKeySubjectClientID = "subject_client_id"
-	AttributeKeyClientType      = "client_type"
-	AttributeKeyConsensusHeight = "consensus_height"
-	AttributeKeyHeader          = "header"
+	AttributeKeyClientID         = "client_id"
+	AttributeKeySubjectClientID  = "subject_client_id"
+	AttributeKeyClientType       = "client_type"
+	AttributeKeyConsensusHeight  = "consensus_height"
+	AttributeKeyConsensusHeights = "consensus_heights"
+	AttributeKeyHeader           = "header"
 )
 
 // IBC client events vars

modules/core/exported/client.go

@@ -65,8 +65,8 @@ type ClientState interface {
 	VerifyClientMessage(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg ClientMessage) error
 
 	// UpdateState updates and stores as necessary any associated information for an IBC client, such as the ClientState and corresponding ConsensusState.
-	// An error is returned if ClientMessage is of type Misbehaviour
-	UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) error
+	// Upon successful update, a list of consensus heights is returned. It assumes the ClientMessage has already been verified.
+	UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) []Height
 
 	// Update and Misbehaviour functions
 	CheckSubstituteAndUpdateState(ctx sdk.Context, cdc codec.BinaryCodec, subjectClientStore, substituteClientStore sdk.KVStore, substituteClient ClientState) (ClientState, error)
@@ -206,7 +206,6 @@ type ConsensusState interface {
 type ClientMessage interface {
 	proto.Message
 
-	GetHeight() Height
 	ClientType() string
 	ValidateBasic() error
 }

modules/light-clients/06-solomachine/types/header.go

@@ -17,13 +17,6 @@ func (Header) ClientType() string {
 	return exported.Solomachine
 }
 
-// GetHeight returns the current sequence number as the height.
-// Return clientexported.Height to satisfy interface
-// Revision number is always 0 for a solo-machine
-func (h Header) GetHeight() exported.Height {
-	return clienttypes.NewHeight(0, h.Sequence)
-}
-
 // GetPubKey unmarshals the new public key into a cryptotypes.PubKey type.
 // An error is returned if the new public key is nil or the cached value
 // is not a PubKey.

modules/light-clients/06-solomachine/types/update.go

@@ -1,6 +1,8 @@
 package types
 
 import (
+	"fmt"
+
 	"github.com/cosmos/cosmos-sdk/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -81,10 +83,11 @@ func (cs ClientState) verifyMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec,
 }
 
 // UpdateState updates the consensus state to the new public key and an incremented sequence.
-func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) error {
+// A list containing the updated consensus height is returned.
+func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) []exported.Height {
 	smHeader, ok := clientMsg.(*Header)
 	if !ok {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected %T got %T", Header{}, clientMsg)
+		panic(fmt.Errorf("unsupported ClientMessage: %T", clientMsg))
 	}
 
 	// create new solomachine ConsensusState
@@ -99,7 +102,7 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client
 
 	clientStore.Set(host.ClientStateKey(), clienttypes.MustMarshalClientState(cdc, &cs))
 
-	return nil
+	return []exported.Height{clienttypes.NewHeight(0, cs.Sequence)}
 }
 
 // CheckForMisbehaviour returns true for type Misbehaviour (passed VerifyClientMessage check), otherwise returns false

modules/light-clients/06-solomachine/types/update_test.go

@@ -441,23 +441,27 @@ func (suite *SoloMachineTestSuite) TestUpdateState() {
 			suite.Run(tc.name, func() {
 				tc.setup() // setup test
 
-				err := clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg)
-
 				if tc.expPass {
-					suite.Require().NoError(err)
+					consensusHeights := clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg)
 
 					clientStateBz := suite.store.Get(host.ClientStateKey())
 					suite.Require().NotEmpty(clientStateBz)
 
 					newClientState := clienttypes.MustUnmarshalClientState(suite.chainA.Codec, clientStateBz)
 
+					suite.Require().Len(consensusHeights, 1)
+					suite.Require().Equal(uint64(0), consensusHeights[0].GetRevisionNumber())
+					suite.Require().Equal(newClientState.(*types.ClientState).Sequence, consensusHeights[0].GetRevisionHeight())
+
 					suite.Require().False(newClientState.(*types.ClientState).IsFrozen)
 					suite.Require().Equal(clientMsg.(*types.Header).Sequence+1, newClientState.(*types.ClientState).Sequence)
 					suite.Require().Equal(clientMsg.(*types.Header).NewPublicKey, newClientState.(*types.ClientState).ConsensusState.PublicKey)
 					suite.Require().Equal(clientMsg.(*types.Header).NewDiversifier, newClientState.(*types.ClientState).ConsensusState.Diversifier)
 					suite.Require().Equal(clientMsg.(*types.Header).Timestamp, newClientState.(*types.ClientState).ConsensusState.Timestamp)
 				} else {
-					suite.Require().Error(err)
+					suite.Require().Panics(func() {
+						clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg)
+					})
 				}
 
 			})

modules/light-clients/07-tendermint/types/update.go

@@ -2,6 +2,7 @@ package types
 
 import (
 	"bytes"
+	"fmt"
 	"reflect"
 
 	"github.com/cosmos/cosmos-sdk/codec"
@@ -154,19 +155,20 @@ func (cs *ClientState) verifyHeader(
 // If we are updating to a past height, a consensus state is created for that height to be persisted in client store
 // If we are updating to a future height, the consensus state is created and the client state is updated to reflect
 // the new latest height
+// A list containing the updated consensus height is returned.
 // UpdateState must only be used to update within a single revision, thus header revision number and trusted height's revision
 // number must be the same. To update to a new revision, use a separate upgrade path
 // UpdateState will prune the oldest consensus state if it is expired.
-func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) error {
+func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) []exported.Height {
 	header, ok := clientMsg.(*Header)
 	if !ok {
-		return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", &Header{}, header)
+		panic(fmt.Errorf("expected type %T, got %T", &Header{}, clientMsg))
 	}
 
 	// check for duplicate update
 	if consensusState, _ := GetConsensusState(clientStore, cdc, header.GetHeight()); consensusState != nil {
 		// perform no-op
-		return nil
+		return []exported.Height{header.GetHeight()}
 	}
 
 	cs.pruneOldestConsensusState(ctx, cdc, clientStore)
@@ -175,6 +177,7 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client
 	if height.GT(cs.LatestHeight) {
 		cs.LatestHeight = height
 	}
+
 	consensusState := &ConsensusState{
 		Timestamp:          header.GetTime(),
 		Root:               commitmenttypes.NewMerkleRoot(header.Header.GetAppHash()),
@@ -186,7 +189,7 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client
 	setConsensusState(clientStore, cdc, consensusState, header.GetHeight())
 	setConsensusMetadata(ctx, clientStore, header.GetHeight())
 
-	return nil
+	return []exported.Height{height}
 }
 
 // pruneOldestConsensusState will retrieve the earliest consensus state for this clientID and check if it is expired. If it is,