refactor: channel handshake version improvements (#1283)

* refactor: returning version from OnChanOpenInit

* refactor: update tests and add version to proto resp

* refactor: adding version to msg server resp

* refactor: remove unncessary if & update version on Endpoint.Ack

* fix: ics29 OnChanOpenInit remake versionMetaData before returning

* chore: update godoc

* test: adding check for expected version string

* test: adding test case for passing empty version string to ics20 onChanOpenInit

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

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

* chore: comment

* chore: changelog

* fix: ica now discards auth module version

* chore: update changelog

* adding default version for ics29

* fix: using transfer module directly rather than calling full middleware stack

* fix testing bug

* refactor: test now uses bool for isFeeEnabled rather than direct check

* docs: updating comment and migration docs

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

Author: seantking

CHANGELOG.md

@@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### API Breaking
 
 * (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used.
+* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) The `OnChanOpenInit` application callback now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).  
 * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`.
 * (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`.
 

docs/migrations/v3-to-v4.md

@@ -1,4 +1,4 @@
-# Migrating from ibc-go v2 to v3
+# Migrating from ibc-go v3 to v4
 
 This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG.
 Any changes that must be done by a user of ibc-go should be documented here.
@@ -23,4 +23,8 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g
 The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. 
 This is an API breaking change and as such IBC application developers will have to update any calls to `WriteAcknowledgement`. 
 
+The `OnChanOpenInit` application callback has been modified.
+The return signature now includes the application version as detailed in the latest IBC [spec changes](https://github.com/cosmos/ibc/pull/629).
+
+
 

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -45,18 +45,23 @@ func (im IBCMiddleware) OnChanOpenInit(
 	chanCap *capabilitytypes.Capability,
 	counterparty channeltypes.Counterparty,
 	version string,
-) error {
+) (string, error) {
 	if !im.keeper.IsControllerEnabled(ctx) {
-		return types.ErrControllerSubModuleDisabled
+		return "", types.ErrControllerSubModuleDisabled
 	}
 
 	if err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
-		return err
+		return "", err
+	}
+
+	// call underlying app's OnChanOpenInit callback with the passed in version
+	// the version returned is discarded as the ica-auth module does not have permission to edit the version string.
+	// ics27 will always return the version string containing the Metadata struct which is created during the `RegisterInterchainAccount` call.
+	if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
+		return "", err
 	}
 
-	// call underlying app's OnChanOpenInit callback with the appVersion
-	return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
-		chanCap, counterparty, version)
+	return version, nil
 }
 
 // OnChanOpenTry implements the IBCMiddleware interface

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go

@@ -147,8 +147,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
 					portID, channelID string, chanCap *capabilitytypes.Capability,
 					counterparty channeltypes.Counterparty, version string,
-				) error {
-					return fmt.Errorf("mock ica auth fails")
+				) (string, error) {
+					return "", fmt.Errorf("mock ica auth fails")
 				}
 			}, false,
 		},
@@ -197,11 +197,24 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 			cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
 			suite.Require().True(ok)
 
-			err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
+			version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
 				path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(),
 			)
 
 			if tc.expPass {
+				expMetadata := icatypes.NewMetadata(
+					icatypes.Version,
+					path.EndpointA.ConnectionID,
+					path.EndpointB.ConnectionID,
+					"",
+					icatypes.EncodingProtobuf,
+					icatypes.TxTypeSDKMultiMsg,
+				)
+
+				expBytes, err := icatypes.ModuleCdc.MarshalJSON(&expMetadata)
+				suite.Require().NoError(err)
+
+				suite.Require().Equal(version, string(expBytes))
 				suite.Require().NoError(err)
 			} else {
 				suite.Require().Error(err)

modules/apps/27-interchain-accounts/host/ibc_module.go

@@ -34,8 +34,8 @@ func (im IBCModule) OnChanOpenInit(
 	chanCap *capabilitytypes.Capability,
 	counterparty channeltypes.Counterparty,
 	version string,
-) error {
-	return sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "channel handshake must be initiated by controller chain")
+) (string, error) {
+	return "", sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "channel handshake must be initiated by controller chain")
 }
 
 // OnChanOpenTry implements the IBCModule interface

modules/apps/29-fee/ibc_middleware.go

@@ -1,6 +1,8 @@
 package fee
 
 import (
+	"strings"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
@@ -39,25 +41,44 @@ func (im IBCMiddleware) OnChanOpenInit(
 	chanCap *capabilitytypes.Capability,
 	counterparty channeltypes.Counterparty,
 	version string,
-) error {
+) (string, error) {
 	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 strings.TrimSpace(version) == "" {
+		// default version
+		versionMetadata = types.Metadata{
+			FeeVersion: types.Version,
+			AppVersion: "",
+		}
+	} else {
+		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)
+		return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
+	}
+
+	appVersion, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, versionMetadata.AppVersion)
+	if err != nil {
+		return "", err
+	}
+
+	versionMetadata.AppVersion = appVersion
+	versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
+	if err != nil {
+		return "", err
 	}
 
 	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, versionMetadata.AppVersion)
+	return string(versionBytes), nil
 }
 
 // OnChanOpenTry implements the IBCMiddleware interface
@@ -94,7 +115,6 @@ func (im IBCMiddleware) OnChanOpenTry(
 	}
 
 	versionMetadata.AppVersion = appVersion
-
 	versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
 	if err != nil {
 		return "", err
@@ -116,7 +136,7 @@ func (im IBCMiddleware) OnChanOpenAck(
 	if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
 		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")
+			return sdkerrors.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion)
 		}
 
 		if versionMetadata.FeeVersion != types.Version {

modules/apps/29-fee/ibc_middleware_test.go

@@ -25,34 +25,46 @@ var (
 // Tests OnChanOpenInit on ChainA
 func (suite *FeeTestSuite) TestOnChanOpenInit() {
 	testCases := []struct {
-		name    string
-		version string
-		expPass bool
+		name         string
+		version      string
+		expPass      bool
+		isFeeEnabled bool
 	}{
 		{
 			"success - valid fee middleware and mock version",
 			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version})),
 			true,
+			true,
 		},
 		{
 			"success - fee version not included, only perform mock logic",
 			ibcmock.Version,
 			true,
+			false,
 		},
 		{
 			"invalid fee middleware version",
 			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: ibcmock.Version})),
 			false,
+			false,
 		},
 		{
 			"invalid mock version",
 			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-mock-version"})),
 			false,
+			false,
 		},
 		{
 			"mock version not wrapped",
 			types.Version,
 			false,
+			false,
+		},
+		{
+			"passing an empty string returns default version",
+			"",
+			true,
+			true,
 		},
 	}
 
@@ -68,11 +80,11 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
 			suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
 				portID, channelID string, chanCap *capabilitytypes.Capability,
 				counterparty channeltypes.Counterparty, version string,
-			) error {
+			) (string, error) {
 				if version != ibcmock.Version {
-					return fmt.Errorf("incorrect mock version")
+					return "", fmt.Errorf("incorrect mock version")
 				}
-				return nil
+				return ibcmock.Version, nil
 			}
 
 			suite.path.EndpointA.ChannelID = ibctesting.FirstChannelID
@@ -95,13 +107,29 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
 			cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
 			suite.Require().True(ok)
 
-			err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
+			version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
 				suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, chanCap, counterparty, channel.Version)
 
 			if tc.expPass {
+				// check if the channel is fee enabled. If so version string should include metaData
+				if tc.isFeeEnabled {
+					versionMetadata := types.Metadata{
+						FeeVersion: types.Version,
+						AppVersion: ibcmock.Version,
+					}
+
+					versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
+					suite.Require().NoError(err)
+
+					suite.Require().Equal(version, string(versionBytes))
+				} else {
+					suite.Require().Equal(ibcmock.Version, version)
+				}
+
 				suite.Require().NoError(err, "unexpected error from version: %s", tc.version)
 			} else {
 				suite.Require().Error(err, "error not returned for version: %s", tc.version)
+				suite.Require().Equal("", version)
 			}
 		})
 	}

modules/apps/29-fee/transfer_test.go

@@ -68,5 +68,4 @@ func (suite *FeeTestSuite) TestFeeTransfer() {
 		fee.AckFee.Add(fee.TimeoutFee...), // ack fee paid, timeout fee refunded
 		sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)).Sub(originalChainASenderAccountBalance),
 	)
-
 }

modules/apps/transfer/ibc_module.go

@@ -3,6 +3,7 @@ package transfer
 import (
 	"fmt"
 	"math"
+	"strings"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -70,21 +71,25 @@ func (im IBCModule) OnChanOpenInit(
 	chanCap *capabilitytypes.Capability,
 	counterparty channeltypes.Counterparty,
 	version string,
-) error {
+) (string, error) {
 	if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID); err != nil {
-		return err
+		return "", err
+	}
+
+	if strings.TrimSpace(version) == "" {
+		version = types.Version
 	}
 
 	if version != types.Version {
-		return sdkerrors.Wrapf(types.ErrInvalidVersion, "got %s, expected %s", version, types.Version)
+		return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "got %s, expected %s", version, types.Version)
 	}
 
 	// Claim channel capability passed back by IBC module
 	if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
-		return err
+		return "", err
 	}
 
-	return nil
+	return version, nil
 }
 
 // OnChanOpenTry implements the IBCModule interface.

modules/apps/transfer/ibc_module_test.go

@@ -5,6 +5,7 @@ import (
 
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
+	"github.com/cosmos/ibc-go/v3/modules/apps/transfer"
 	"github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
@@ -28,6 +29,11 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
 		{
 			"success", func() {}, true,
 		},
+		{
+			"empty version string", func() {
+				channel.Version = ""
+			}, true,
+		},
 		{
 			"max channels reached", func() {
 				path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
@@ -74,25 +80,23 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
 				Version:        types.Version,
 			}
 
-			module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
-			suite.Require().NoError(err)
-
+			var err error
 			chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(ibctesting.TransferPort, path.EndpointA.ChannelID))
 			suite.Require().NoError(err)
 
-			cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
-			suite.Require().True(ok)
-
 			tc.malleate() // explicitly change fields in channel and testChannel
 
-			err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
+			transferModule := transfer.NewIBCModule(suite.chainA.GetSimApp().TransferKeeper)
+			version, err := transferModule.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
 				path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, counterparty, channel.GetVersion(),
 			)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
+				suite.Require().Equal(types.Version, version)
 			} else {
 				suite.Require().Error(err)
+				suite.Require().Equal(version, "")
 			}
 
 		})