refactor: ics29 json encoded version metadata (#883)

* adding metadata type to ics29 protos

* updating ics29 handshake handlers to support json encoded metadata

* updating tests to support json encoded metadata

* Update modules/apps/29-fee/ibc_module.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* Update modules/apps/29-fee/ibc_module.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* renaming metadata version to fee_version

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

docs/ibc/proto-docs.md

@@ -38,6 +38,9 @@
     - [GenesisState](#ibc.applications.fee.v1.GenesisState)
     - [RegisteredRelayerAddress](#ibc.applications.fee.v1.RegisteredRelayerAddress)
 
+- [ibc/applications/fee/v1/metadata.proto](#ibc/applications/fee/v1/metadata.proto)
+    - [Metadata](#ibc.applications.fee.v1.Metadata)
+
 - [ibc/applications/fee/v1/query.proto](#ibc/applications/fee/v1/query.proto)
     - [QueryIncentivizedPacketRequest](#ibc.applications.fee.v1.QueryIncentivizedPacketRequest)
     - [QueryIncentivizedPacketResponse](#ibc.applications.fee.v1.QueryIncentivizedPacketResponse)
@@ -812,6 +815,39 @@ RegisteredRelayerAddress contains the address and counterparty address for a spe
 
 
 
+ <!-- end messages -->
+
+ <!-- end enums -->
+
+ <!-- end HasExtensions -->
+
+ <!-- end services -->
+
+
+
+<a name="ibc/applications/fee/v1/metadata.proto"></a>
+<p align="right"><a href="#top">Top</a></p>
+
+## ibc/applications/fee/v1/metadata.proto
+
+
+
+<a name="ibc.applications.fee.v1.Metadata"></a>
+
+### Metadata
+Metadata defines the ICS29 channel specific metadata encoded into the channel version bytestring
+See ICS004: https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#Versioning
+
+
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `fee_version` | [string](#string) |  | fee_version defines the ICS29 fee version |
+| `app_version` | [string](#string) |  | app_version defines the underlying application version, which may or may not be a JSON encoded bytestring |
+
+
+
+
+
  <!-- end messages -->
 
  <!-- end enums -->

modules/apps/29-fee/fee_test.go

@@ -30,7 +30,7 @@ func (suite *FeeTestSuite) SetupTest() {
 	suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
 
 	path := ibctesting.NewPath(suite.chainA, suite.chainB)
-	feeTransferVersion := channeltypes.MergeChannelVersions(types.Version, transfertypes.Version)
+	feeTransferVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version}))
 	path.EndpointA.ChannelConfig.Version = feeTransferVersion
 	path.EndpointB.ChannelConfig.Version = feeTransferVersion
 	path.EndpointA.ChannelConfig.PortID = transfertypes.PortID

modules/apps/29-fee/ibc_module.go

@@ -37,22 +37,24 @@ func (im IBCModule) OnChanOpenInit(
 	counterparty channeltypes.Counterparty,
 	version string,
 ) error {
-	mwVersion, appVersion := channeltypes.SplitChannelVersion(version)
-	// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
-	// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
-	// application.
-	// If an invalid fee version was passed, we expect the underlying application to fail on its version negotiation.
-	if mwVersion == types.Version {
-		im.keeper.SetFeeEnabled(ctx, portID, channelID)
-	} else {
-		// middleware version is not the expected version for this midddleware. Pass the full version string along,
-		// if it not valid version for any other lower middleware, an error will be returned by base application.
-		appVersion = version
+	var versionMetadata types.Metadata
+	if err := types.ModuleCdc.UnmarshalJSON([]byte(version), &versionMetadata); err != nil {
+		// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
+		// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
+		// application.
+		return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
+			chanCap, counterparty, version)
 	}
 
+	if versionMetadata.FeeVersion != types.Version {
+		return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
+	}
+
+	im.keeper.SetFeeEnabled(ctx, portID, channelID)
+
 	// call underlying app's OnChanOpenInit callback with the appVersion
 	return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
-		chanCap, counterparty, appVersion)
+		chanCap, counterparty, versionMetadata.AppVersion)
 }
 
 // OnChanOpenTry implements the IBCModule interface
@@ -68,31 +70,34 @@ func (im IBCModule) OnChanOpenTry(
 	counterparty channeltypes.Counterparty,
 	counterpartyVersion string,
 ) (string, error) {
-	cpMwVersion, cpAppVersion := channeltypes.SplitChannelVersion(counterpartyVersion)
+	var versionMetadata types.Metadata
+	if err := types.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &versionMetadata); err != nil {
+		// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
+		// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
+		// application.
+		return im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, counterpartyVersion)
+	}
 
-	// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
-	// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
-	// application.
-	// If an invalid fee version was passed, we expect the underlying application to fail on its version negotiation.
-	if cpMwVersion == types.Version {
-		im.keeper.SetFeeEnabled(ctx, portID, channelID)
-	} else {
-		// middleware versions are not the expected version for this midddleware. Pass the full version strings along,
-		// if it not valid version for any other lower middleware, an error will be returned by base application.
-		cpAppVersion = counterpartyVersion
+	if versionMetadata.FeeVersion != types.Version {
+		return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
 	}
 
+	im.keeper.SetFeeEnabled(ctx, portID, channelID)
+
 	// call underlying app's OnChanOpenTry callback with the app versions
-	appVersion, err := im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, cpAppVersion)
+	appVersion, err := im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, versionMetadata.AppVersion)
 	if err != nil {
 		return "", err
 	}
 
-	if !im.keeper.IsFeeEnabled(ctx, portID, channelID) {
-		return appVersion, nil
+	versionMetadata.AppVersion = appVersion
+
+	versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
+	if err != nil {
+		return "", err
 	}
 
-	return channeltypes.MergeChannelVersions(types.Version, appVersion), nil
+	return string(versionBytes), nil
 }
 
 // OnChanOpenAck implements the IBCModule interface
@@ -104,17 +109,22 @@ func (im IBCModule) OnChanOpenAck(
 ) error {
 	// If handshake was initialized with fee enabled it must complete with fee enabled.
 	// If handshake was initialized with fee disabled it must complete with fee disabled.
-	cpAppVersion := counterpartyVersion
 	if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
-		var cpFeeVersion string
-		cpFeeVersion, cpAppVersion = channeltypes.SplitChannelVersion(counterpartyVersion)
+		var versionMetadata types.Metadata
+		if err := types.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &versionMetadata); err != nil {
+			return sdkerrors.Wrap(types.ErrInvalidVersion, "failed to unmarshal ICS29 counterparty version metadata")
+		}
 
-		if cpFeeVersion != types.Version {
-			return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected counterparty version: %s, got: %s", types.Version, cpFeeVersion)
+		if versionMetadata.FeeVersion != types.Version {
+			return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected counterparty version: %s, got: %s", types.Version, versionMetadata.FeeVersion)
 		}
+
+		// call underlying app's OnChanOpenAck callback with the counterparty app version.
+		return im.app.OnChanOpenAck(ctx, portID, channelID, versionMetadata.AppVersion)
 	}
+
 	// call underlying app's OnChanOpenAck callback with the counterparty app version.
-	return im.app.OnChanOpenAck(ctx, portID, channelID, cpAppVersion)
+	return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion)
 }
 
 // OnChanOpenConfirm implements the IBCModule interface

modules/apps/29-fee/ibc_module_test.go

@@ -1,8 +1,6 @@
 package fee_test
 
 import (
-	"fmt"
-
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
@@ -28,40 +26,30 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
 		expPass bool
 	}{
 		{
-			"valid fee middleware and transfer version",
-			channeltypes.MergeChannelVersions(types.Version, transfertypes.Version),
+			"success - valid fee middleware and transfer version",
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version})),
 			true,
 		},
 		{
-			"fee version not included, only perform transfer logic",
+			"success - fee version not included, only perform transfer logic",
 			transfertypes.Version,
 			true,
 		},
 		{
 			"invalid fee middleware version",
-			channeltypes.MergeChannelVersions("otherfee28-1", transfertypes.Version),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: transfertypes.Version})),
 			false,
 		},
 		{
 			"invalid transfer version",
-			channeltypes.MergeChannelVersions(types.Version, "wrongics20-1"),
-			false,
-		},
-		{
-			"incorrect wrapping delimiter",
-			fmt.Sprintf("%s//%s", types.Version, transfertypes.Version),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-ics20-1"})),
 			false,
 		},
 		{
 			"transfer version not wrapped",
 			types.Version,
 			false,
 		},
-		{
-			"hanging delimiter",
-			fmt.Sprintf("%s:%s:", types.Version, transfertypes.Version),
-			false,
-		},
 	}
 
 	for _, tc := range testCases {
@@ -112,32 +100,32 @@ func (suite *FeeTestSuite) TestOnChanOpenTry() {
 		expPass   bool
 	}{
 		{
-			"valid fee middleware version",
-			channeltypes.MergeChannelVersions(types.Version, transfertypes.Version),
+			"success - valid fee middleware version",
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version})),
 			false,
 			true,
 		},
 		{
-			"valid transfer version",
+			"success - valid transfer version",
 			transfertypes.Version,
 			false,
 			true,
 		},
 		{
-			"crossing hellos: valid fee middleware",
-			channeltypes.MergeChannelVersions(types.Version, transfertypes.Version),
+			"success - crossing hellos: valid fee middleware",
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version})),
 			true,
 			true,
 		},
 		{
 			"invalid fee middleware version",
-			channeltypes.MergeChannelVersions("wrongfee29-1", transfertypes.Version),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: transfertypes.Version})),
 			false,
 			false,
 		},
 		{
 			"invalid transfer version",
-			channeltypes.MergeChannelVersions(types.Version, "wrongics20-1"),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-ics20-1"})),
 			false,
 			false,
 		},
@@ -205,25 +193,31 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() {
 	}{
 		{
 			"success",
-			channeltypes.MergeChannelVersions(types.Version, transfertypes.Version),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version})),
 			func(suite *FeeTestSuite) {},
 			true,
 		},
 		{
 			"invalid fee version",
-			channeltypes.MergeChannelVersions("fee29-A", transfertypes.Version),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: transfertypes.Version})),
 			func(suite *FeeTestSuite) {},
 			false,
 		},
 		{
 			"invalid transfer version",
-			channeltypes.MergeChannelVersions(types.Version, "ics20-4"),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-ics20-1"})),
+			func(suite *FeeTestSuite) {},
+			false,
+		},
+		{
+			"invalid version fails to unmarshal metadata",
+			"invalid-version",
 			func(suite *FeeTestSuite) {},
 			false,
 		},
 		{
 			"previous INIT set without fee, however counterparty set fee version", // note this can only happen with incompetent or malicious counterparty chain
-			channeltypes.MergeChannelVersions(types.Version, transfertypes.Version),
+			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version})),
 			func(suite *FeeTestSuite) {
 				// do the first steps without fee version, then pass the fee version as counterparty version in ChanOpenACK
 				suite.path.EndpointA.ChannelConfig.Version = transfertypes.Version

modules/apps/29-fee/keeper/keeper_test.go

@@ -40,7 +40,7 @@ func (suite *KeeperTestSuite) SetupTest() {
 	suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
 
 	path := ibctesting.NewPath(suite.chainA, suite.chainB)
-	feeTransferVersion := channeltypes.MergeChannelVersions(types.Version, transfertypes.Version)
+	feeTransferVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version}))
 	path.EndpointA.ChannelConfig.Version = feeTransferVersion
 	path.EndpointB.ChannelConfig.Version = feeTransferVersion
 	path.EndpointA.ChannelConfig.PortID = transfertypes.PortID