refactor: channel handshake version improvements

CHANGELOG.md

@@ -43,6 +43,8 @@ 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).  
+
 
 ### State Machine Breaking
 

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

@@ -42,18 +42,21 @@ func (im IBCModule) 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
+	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 IBCModule interface

modules/apps/27-interchain-accounts/controller/ibc_module_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_module.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"
@@ -36,25 +38,44 @@ func (im IBCModule) 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 IBCModule interface
@@ -91,7 +112,6 @@ func (im IBCModule) OnChanOpenTry(
 	}
 
 	versionMetadata.AppVersion = appVersion
-
 	versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
 	if err != nil {
 		return "", err
@@ -113,7 +133,7 @@ func (im IBCModule) 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_module_test.go

@@ -54,6 +54,11 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
 			types.Version,
 			false,
 		},
+		{
+			"passing an empty string returns default version",
+			"",
+			true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -68,11 +73,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 +100,30 @@ 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
+				isFeeEnabled := suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
+				if 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

@@ -28,6 +28,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)
@@ -85,14 +90,16 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
 
 			tc.malleate() // explicitly change fields in channel and testChannel
 
-			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, 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, "")
 			}
 
 		})

modules/core/05-port/types/module.go

@@ -11,10 +11,16 @@ import (
 // IBCModule defines an interface that implements all the callbacks
 // that modules must define as specified in ICS-26
 type IBCModule interface {
-	// OnChanOpenInit will verify that the relayer-chosen parameters are
-	// valid and perform any custom INIT logic.It may return an error if
-	// the chosen parameters are invalid in which case the handshake is aborted.
-	// OnChanOpenInit should return an error if the provided version is invalid.
+	// OnChanOpenInit will verify that the relayer-chosen parameters
+	// are valid and perform any custom INIT logic.
+	// It may return an error if the chosen parameters are invalid
+	// in which case the handshake is aborted.
+	// If the provided version string is non-empty, OnChanOpenInit should return
+	// the version string if valid or an error if the provided version is invalid.
+	// If the version string is empty, OnChanOpenInit is expected to
+	// return a default version string representing the version(s) it supports.
+	// If there is no default version string for the application,
+	// it should return an error if provided version is empty string.
 	OnChanOpenInit(
 		ctx sdk.Context,
 		order channeltypes.Order,
@@ -24,7 +30,7 @@ type IBCModule interface {
 		channelCap *capabilitytypes.Capability,
 		counterparty channeltypes.Counterparty,
 		version string,
-	) error
+	) (string, error)
 
 	// OnChanOpenTry will verify the relayer-chosen parameters along with the
 	// counterparty-chosen version string and perform custom TRY logic.

modules/core/keeper/msg_server.go

@@ -185,16 +185,17 @@ func (k Keeper) ChannelOpenInit(goCtx context.Context, msg *channeltypes.MsgChan
 	}
 
 	// Perform application logic callback
-	if err = cbs.OnChanOpenInit(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.Channel.Version); err != nil {
+	version, err := cbs.OnChanOpenInit(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.Channel.Version)
+	if err != nil {
 		return nil, sdkerrors.Wrap(err, "channel open init callback failed")
 	}
 
 	// Write channel into state
-	k.ChannelKeeper.WriteOpenInitChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, msg.Channel.Version)
+	k.ChannelKeeper.WriteOpenInitChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, version)
 
 	return &channeltypes.MsgChannelOpenInitResponse{
 		ChannelId: channelID,
-		Version:   msg.Channel.Version,
+		Version:   version,
 	}, nil
 }
 

testing/endpoint.go

@@ -279,6 +279,10 @@ func (endpoint *Endpoint) ChanOpenInit() error {
 	endpoint.ChannelID, err = ParseChannelIDFromEvents(res.GetEvents())
 	require.NoError(endpoint.Chain.T, err)
 
+	// update version to selected app version
+	// NOTE: this update must be performed after SendMsgs()
+	endpoint.ChannelConfig.Version = endpoint.GetChannel().Version
+
 	return nil
 }
 

testing/mock/ibc_app.go

@@ -23,7 +23,7 @@ type MockIBCApp struct {
 		channelCap *capabilitytypes.Capability,
 		counterparty channeltypes.Counterparty,
 		version string,
-	) error
+	) (string, error)
 
 	OnChanOpenTry func(
 		ctx sdk.Context,