refactor: reusable metadata validation (#729)

* define and generate new metadata proto type

* refactor types pkg, remove version string parsers, add PortPrefix

* refactor ica entrypoint and handshake to handle connection ids in metadata

* fixing broken test cases

* adding controller port and metadata validation, adding new testcases

* updating proto doc and removing commented out code

* updating testcase names, adding metadata constructor func, updating PortPrefix and removing Delimiter

* adding ErrInvalidControllerPort and ErrInvalidHostPort

* refactoring metadata validation to reusable func

* returning correct err type

* regenerating protos after merge conflicts

* adding separate validation funcs for controller and host

* correcting error msg in ValidateHostMetadata

* updating with review suggestions

* adding additional empty address check to ACK step, adding test case

* adding strings.Trimspace

* adding success with empty address testcase for ValidateHostMetadata

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

@@ -8,7 +8,6 @@ import (
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
 	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
-	connectiontypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types"
 )
@@ -47,14 +46,10 @@ func (k Keeper) OnChanOpenInit(
 		return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
 	}
 
-	if err := k.validateConnectionParams(ctx, connectionHops, metadata.ControllerConnectionId, metadata.HostConnectionId); err != nil {
+	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
 		return err
 	}
 
-	if metadata.Version != icatypes.Version {
-		return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version)
-	}
-
 	activeChannelID, found := k.GetOpenActiveChannel(ctx, portID)
 	if found {
 		return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "existing active channel %s for portID %s", activeChannelID, portID)
@@ -84,12 +79,17 @@ func (k Keeper) OnChanOpenAck(
 		return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
 	}
 
-	if err := icatypes.ValidateAccountAddress(metadata.Address); err != nil {
+	channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
+	if !found {
+		return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
+	}
+
+	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, metadata); err != nil {
 		return err
 	}
 
-	if metadata.Version != icatypes.Version {
-		return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version)
+	if strings.TrimSpace(metadata.Address) == "" {
+		return sdkerrors.Wrap(icatypes.ErrInvalidAccountAddress, "interchain account address cannot be empty")
 	}
 
 	k.SetActiveChannelID(ctx, portID, channelID)
@@ -107,22 +107,3 @@ func (k Keeper) OnChanCloseConfirm(
 
 	return nil
 }
-
-// validateConnectionParams asserts the provided controller and host connection identifiers match that of the associated connection stored in state
-func (k Keeper) validateConnectionParams(ctx sdk.Context, connectionHops []string, controllerConnectionID, hostConnectionID string) error {
-	connectionID := connectionHops[0]
-	connection, err := k.channelKeeper.GetConnection(ctx, connectionID)
-	if err != nil {
-		return err
-	}
-
-	if controllerConnectionID != connectionID {
-		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connectionID, controllerConnectionID)
-	}
-
-	if hostConnectionID != connection.GetCounterparty().GetConnectionID() {
-		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connection.GetCounterparty().GetConnectionID(), hostConnectionID)
-	}
-
-	return nil
-}

modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go

@@ -223,6 +223,18 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() {
 			},
 			false,
 		},
+		{
+			"empty account address",
+			func() {
+				metadata.Address = ""
+
+				versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+
+				path.EndpointA.Counterparty.ChannelConfig.Version = string(versionBytes)
+			},
+			false,
+		},
 		{
 			"invalid counterparty version",
 			func() {

modules/apps/27-interchain-accounts/host/keeper/handshake.go

@@ -8,7 +8,6 @@ import (
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
 	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
-	connectiontypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
 )
@@ -44,14 +43,10 @@ func (k Keeper) OnChanOpenTry(
 		return "", sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
 	}
 
-	if err := k.validateConnectionParams(ctx, connectionHops, metadata.ControllerConnectionId, metadata.HostConnectionId); err != nil {
+	if err := icatypes.ValidateHostMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
 		return "", err
 	}
 
-	if metadata.Version != icatypes.Version {
-		return "", sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version)
-	}
-
 	// On the host chain the capability may only be claimed during the OnChanOpenTry
 	// The capability being claimed in OpenInit is for a controller chain (the port is different)
 	if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
@@ -93,22 +88,3 @@ func (k Keeper) OnChanCloseConfirm(
 
 	return nil
 }
-
-// validateConnectionParams asserts the provided controller and host connection identifiers match that of the associated connection stored in state
-func (k Keeper) validateConnectionParams(ctx sdk.Context, connectionHops []string, controllerConnectionID, hostConnectionID string) error {
-	connectionID := connectionHops[0]
-	connection, err := k.channelKeeper.GetConnection(ctx, connectionID)
-	if err != nil {
-		return err
-	}
-
-	if hostConnectionID != connectionID {
-		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connectionID, controllerConnectionID)
-	}
-
-	if controllerConnectionID != connection.GetCounterparty().GetConnectionID() {
-		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connection.GetCounterparty().GetConnectionID(), hostConnectionID)
-	}
-
-	return nil
-}

modules/apps/27-interchain-accounts/types/metadata.go

@@ -1,5 +1,12 @@
 package types
 
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+
+	connectiontypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types"
+)
+
 // NewMetadata creates and returns a new ICS27 Metadata instance
 func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress string) Metadata {
 	return Metadata{
@@ -9,3 +16,64 @@ func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress s
 		Address:                accAddress,
 	}
 }
+
+// ValidateControllerMetadata performs validation of the provided ICS27 controller metadata parameters
+func ValidateControllerMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error {
+	connection, err := channelKeeper.GetConnection(ctx, connectionHops[0])
+	if err != nil {
+		return err
+	}
+
+	if err := validateConnectionParams(metadata, connectionHops[0], connection.GetCounterparty().GetConnectionID()); err != nil {
+		return err
+	}
+
+	if metadata.Address != "" {
+		if err := ValidateAccountAddress(metadata.Address); err != nil {
+			return err
+		}
+	}
+
+	if metadata.Version != Version {
+		return sdkerrors.Wrapf(ErrInvalidVersion, "expected %s, got %s", Version, metadata.Version)
+	}
+
+	return nil
+}
+
+// ValidateHostMetadata performs validation of the provided ICS27 host metadata parameters
+func ValidateHostMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error {
+	connection, err := channelKeeper.GetConnection(ctx, connectionHops[0])
+	if err != nil {
+		return err
+	}
+
+	if err := validateConnectionParams(metadata, connection.GetCounterparty().GetConnectionID(), connectionHops[0]); err != nil {
+		return err
+	}
+
+	if metadata.Address != "" {
+		if err := ValidateAccountAddress(metadata.Address); err != nil {
+			return err
+		}
+	}
+
+	if metadata.Version != Version {
+		return sdkerrors.Wrapf(ErrInvalidVersion, "expected %s, got %s", Version, metadata.Version)
+	}
+
+	return nil
+}
+
+// validateConnectionParams compares the given the controller and host connection IDs to those set in the provided ICS27 Metadata
+func validateConnectionParams(metadata Metadata, controllerConnectionID, hostConnectionID string) error {
+	if metadata.ControllerConnectionId != controllerConnectionID {
+		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", controllerConnectionID, metadata.ControllerConnectionId)
+	}
+
+	if metadata.HostConnectionId != hostConnectionID {
+		return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", hostConnectionID, metadata.HostConnectionId)
+	}
+
+	return nil
+}