Modify OnChanOpenTry to return application version (#650)

* modify OnChanOpenTry to return negotiated version

modify IBCModule interface function OnChanOpenTry to return the negotiated app version. Tests have not been updated

* fix ibc_module_test.go tests

* fix tests

* Apply suggestions from code review

* add handshake test case

* add CHANGELOG and migration docs

* update documentation

* fix broken link

CHANGELOG.md

@@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking
 
+* (core) [\#650](https://github.com/cosmos/ibc-go/pull/650) Modify `OnChanOpenTry` IBC application module callback to return the negotiated app version. The version passed into the `MsgChanOpenTry` has been deprecated and will be ignored by core IBC.
 * (core) [\#629](https://github.com/cosmos/ibc-go/pull/629) Removes the `GetProofSpecs` from the ClientState interface. This function was previously unused by core IBC.
 * (transfer) [\#517](https://github.com/cosmos/ibc-go/pull/517) Separates the ICS 26 callback functions from `AppModule` into a new type `IBCModule` for ICS 20 transfer.
 * (modules/core/02-client) [\#536](https://github.com/cosmos/ibc-go/pull/536) `GetSelfConsensusState` return type changed from bool to error.

docs/ibc/apps.md

@@ -71,9 +71,8 @@ OnChanOpenTry(
     channelID string,
     channelCap *capabilitytypes.Capability,
     counterparty channeltypes.Counterparty,
-    version,
     counterpartyVersion string,
-) error {
+) (string, error) {
     // Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos
     // (ie chainA and chainB both call ChanOpenInit before one of them calls ChanOpenTry)
     // If the module can already authenticate the capability then the module already owns it so we don't need to claim
@@ -88,8 +87,18 @@ OnChanOpenTry(
     // ... do custom initialization logic
 
     // Use above arguments to determine if we want to abort handshake
-    err := checkArguments(args)
-    return err
+    if err := checkArguments(args); err != nil {
+        return err
+    }
+
+    // Construct application version 
+    // IBC applications must return the appropriate application version
+    // This can be a simple string or it can be a complex version constructed
+    // from the counterpartyVersion and other arguments. 
+    // The version returned will be the channel version used for both channel ends. 
+    appVersion := negotiateAppVersion(counterpartyVersion, args)
+
+    return appVersion, nil
 }
 
 // Called by IBC Handler on MsgOpenAck
@@ -157,38 +166,11 @@ OnChanCloseConfirm(
 Application modules are expected to verify versioning used during the channel handshake procedure.
 
 * `ChanOpenInit` callback should verify that the `MsgChanOpenInit.Version` is valid
-* `ChanOpenTry` callback should verify that the `MsgChanOpenTry.Version` is valid and that `MsgChanOpenTry.CounterpartyVersion` is valid.
+* `ChanOpenTry` callback should construct the application version used for both channel ends. If no application version can be constructed, it must return an error. 
 * `ChanOpenAck` callback should verify that the `MsgChanOpenAck.CounterpartyVersion` is valid and supported.
 
-IBC expects application modules to implement the `NegotiateAppVersion` method from the `IBCModule`
-interface. This method performs application version negotiation and returns the negotiated version.
-If the version cannot be negotiated, an error should be returned.
-
-```go
-// NegotiateAppVersion performs application version negotiation given the provided channel ordering, connectionID, portID, counterparty and proposed version.
-// An error is returned if version negotiation cannot be performed. For example, an application module implementing this interface
-// may decide to return an error in the event of the proposed version being incompatible with it's own
-NegotiateAppVersion(
-    ctx sdk.Context,
-    order channeltypes.Order,
-    connectionID string,
-    portID string,
-    counterparty channeltypes.Counterparty,
-    proposedVersion string,
-) (version string, err error) {
-    // do custom application version negotiation logic
-}
-```
-
-This function `NegotiateAppVersion` returns the version to be used in the `ChanOpenTry` step
-(`MsgChanOpenTry.Version`). The relayer chooses the initial version in the `ChanOpenInit` step
-(this will likely be chosen by the user controlling the relayer or by the application that
-triggers the `ChanOpenInit` step).
-
-The version submitted in the `ChanOpenInit` step (`MsgChanOpenInit.Version`) is passed as an
-argument (`proposedVersion`) to the function `NegotiateAppVersion`. This function looks at
-the `proposedVersion` and returns the matching version to be used in the `ChanOpenTry` step.
-Applications can choose to implement this in however fashion they choose.
+IBC expects application modules to perform application version negotiation in `OnChanOpenTry`. The negotiated version
+must be returned to core IBC. If the version cannot be negotiated, an error should be returned.
 
 Versions must be strings but can implement any versioning structure. If your application plans to
 have linear releases then semantic versioning is recommended. If your application plans to release

docs/ibc/middleware/develop.md

@@ -103,29 +103,32 @@ func OnChanOpenTry(
     channelID string,
     channelCap *capabilitytypes.Capability,
     counterparty channeltypes.Counterparty,
-    version,
     counterpartyVersion string,
-) error {
-      // core/04-channel/types contains a helper function to split middleware and underlying app version
-      cpMiddlewareVersion, cpAppVersion = channeltypes.SplitChannelVersion(counterpartyVersion)
-      middlewareVersion, appVersion = channeltypes.SplitChannelVersion(version)
-      if !isCompatible(cpMiddlewareVersion, middlewareVersion) {
-          return error
-      }
-      doCustomLogic()
-
-      // call the underlying applications OnChanOpenTry callback
-      app.OnChanOpenTry(
-          ctx,
-          order,
-          connectionHops,
-          portID,
-          channelID,
-          channelCap,
-          counterparty,
-          cpAppVersion, // note we only pass counterparty app version here
-          appVersion, // only pass app version
-      )
+) (string, error) {
+    doCustomLogic()
+
+    // core/04-channel/types contains a helper function to split middleware and underlying app version
+    cpMiddlewareVersion, cpAppVersion = channeltypes.SplitChannelVersion(counterpartyVersion)
+
+    // call the underlying applications OnChanOpenTry callback
+    appVersion, err := app.OnChanOpenTry(
+        ctx,
+        order,
+        connectionHops,
+        portID,
+        channelID,
+        channelCap,
+        counterparty,
+        cpAppVersion, // note we only pass counterparty app version here
+    )
+    if err != nil {
+        return err
+    }
+
+    middlewareVersion := negotiateMiddlewareVersion(cpMiddlewareVersion)
+    version := constructVersion(middlewareVersion, appVersion)
+
+    return version
 }
 
 func OnChanOpenAck(
@@ -134,15 +137,15 @@ func OnChanOpenAck(
     channelID string,
     counterpartyVersion string,
 ) error {
-      // core/04-channel/types contains a helper function to split middleware and underlying app version
-      middlewareVersion, appVersion = channeltypes.SplitChannelVersion(version)
-      if !isCompatible(middlewareVersion) {
-          return error
-      }
-      doCustomLogic()
+    // core/04-channel/types contains a helper function to split middleware and underlying app version
+    middlewareVersion, appVersion = channeltypes.SplitChannelVersion(version)
+    if !isCompatible(middlewareVersion) {
+        return error
+    }
+    doCustomLogic()
 
-      // call the underlying applications OnChanOpenTry callback
-      app.OnChanOpenAck(ctx, portID, channelID, appVersion)
+    // call the underlying applications OnChanOpenTry callback
+    app.OnChanOpenAck(ctx, portID, channelID, appVersion)
 }
 
 func OnChanOpenConfirm(
@@ -236,4 +239,4 @@ func SendPacket(appPacket channeltypes.Packet) {
 
     return ics4Keeper.SendPacket(packet)
 }
-```
+```

docs/ibc/proto-docs.md

@@ -1819,14 +1819,15 @@ MsgChannelOpenInitResponse defines the Msg/ChannelOpenInit response type.
 
 ### MsgChannelOpenTry
 MsgChannelOpenInit defines a msg sent by a Relayer to try to open a channel
-on Chain B.
+on Chain B. The version field within the Channel field has been deprecated. Its
+value will be ignored by core IBC.
 
 
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
 | `port_id` | [string](#string) |  |  |
 | `previous_channel_id` | [string](#string) |  | in the case of crossing hello's, when both chains call OpenInit, we need the channel identifier of the previous channel in state INIT |
-| `channel` | [Channel](#ibc.core.channel.v1.Channel) |  |  |
+| `channel` | [Channel](#ibc.core.channel.v1.Channel) |  | NOTE: the version field within the channel has been deprecated. Its value will be ignored by core IBC. |
 | `counterparty_version` | [string](#string) |  |  |
 | `proof_init` | [bytes](#bytes) |  |  |
 | `proof_height` | [ibc.core.client.v1.Height](#ibc.core.client.v1.Height) |  |  |

docs/migrations/v2-to-v3.md

@@ -19,9 +19,26 @@ No genesis or in-place migrations are required when upgrading from v1 or v2 of i
 ## Chains
 
 ICS27 Interchain Accounts has been added as a supported IBC application of ibc-go.
+Please see the [ICS27 documentation](../app_modules/interchain-accounts/overview.md) for more information.
 
 ## IBC Apps
 
+
+### `OnChanOpenTry` must return negotiated application version
+
+The `OnChanOpenTry` application callback has been modified.
+The return signature now includes the application version. 
+IBC applications must perform application version negoitation in `OnChanOpenTry` using the counterparty version. 
+The negotiated application version then must be returned in `OnChanOpenTry` to core IBC.
+Core IBC will set this version in the TRYOPEN channel.
+
+### `NegotiateAppVersion` removed from `IBCModule` interface
+
+Previously this logic was handled by the `NegotiateAppVersion` function.
+Relayers would query this function before calling `ChanOpenTry`.
+Applications would then need to verify that the passed in version was correct.
+Now applications will perform this version negotiation during the channel handshake, thus removing the need for `NegotiateAppVersion`.
+
 ### Channel state will not be set before application callback
 
 The channel handshake logic has been reorganized within core IBC. 
@@ -42,7 +59,10 @@ Please review the [mock](../../testing/mock/ibc_module.go) and [transfer](../../
 
 ## Relayers
 
-- No relevant changes were made in this release.
+`AppVersion` gRPC has been removed.
+The `version` string in `MsgChanOpenTry` has been deprecated and will be ignored by core IBC. 
+Relayers no longer need to determine the version to use on the `ChanOpenTry` step.
+IBC applications will determine the correct version using the counterparty version. 
 
 ## IBC Light Clients
 

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

@@ -65,10 +65,9 @@ func (im IBCModule) OnChanOpenTry(
 	channelID string,
 	chanCap *capabilitytypes.Capability,
 	counterparty channeltypes.Counterparty,
-	version,
 	counterpartyVersion 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")
 }
 
 // OnChanOpenAck implements the IBCModule interface

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

@@ -236,12 +236,13 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenTry() {
 	chanCap, found := suite.chainA.App.GetScopedIBCKeeper().GetCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
 	suite.Require().True(found)
 
-	err = cbs.OnChanOpenTry(
+	version, err := cbs.OnChanOpenTry(
 		suite.chainA.GetContext(), path.EndpointA.ChannelConfig.Order, []string{path.EndpointA.ConnectionID},
 		path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap,
-		counterparty, path.EndpointA.ChannelConfig.Version, path.EndpointB.ChannelConfig.Version,
+		counterparty, path.EndpointB.ChannelConfig.Version,
 	)
 	suite.Require().Error(err)
+	suite.Require().Equal("", version)
 }
 
 func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {

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

@@ -47,14 +47,13 @@ func (im IBCModule) OnChanOpenTry(
 	channelID string,
 	chanCap *capabilitytypes.Capability,
 	counterparty channeltypes.Counterparty,
-	version,
 	counterpartyVersion string,
-) error {
+) (string, error) {
 	if !im.keeper.IsHostEnabled(ctx) {
-		return types.ErrHostSubModuleDisabled
+		return "", types.ErrHostSubModuleDisabled
 	}
 
-	return im.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion)
+	return im.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, counterpartyVersion)
 }
 
 // OnChanOpenAck implements the IBCModule interface

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

@@ -147,16 +147,16 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
 				// mock module callback should not be called on host side
 				suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenTry = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
 					portID, channelID string, chanCap *capabilitytypes.Capability,
-					counterparty channeltypes.Counterparty, version, counterpartyVersion string,
-				) error {
-					return fmt.Errorf("mock ica auth fails")
+					counterparty channeltypes.Counterparty, counterpartyVersion string,
+				) (string, error) {
+					return "", fmt.Errorf("mock ica auth fails")
 				}
 
 			}, true,
 		},
 		{
-			"ICA callback fails - invalid version", func() {
-				channel.Version = icatypes.VersionPrefix
+			"ICA callback fails - invalid channel order", func() {
+				channel.Ordering = channeltypes.UNORDERED
 			}, false,
 		},
 	}
@@ -181,7 +181,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
 				Ordering:       channeltypes.ORDERED,
 				Counterparty:   counterparty,
 				ConnectionHops: []string{path.EndpointB.ConnectionID},
-				Version:        TestVersion,
+				Version:        "",
 			}
 
 			tc.malleate()
@@ -198,14 +198,16 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
 			cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module)
 			suite.Require().True(ok)
 
-			err = cbs.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.GetConnectionHops(),
-				path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), path.EndpointA.ChannelConfig.Version,
+			version, err := cbs.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.GetConnectionHops(),
+				path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, chanCap, channel.Counterparty, path.EndpointA.ChannelConfig.Version,
 			)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
+				suite.Require().Equal(TestVersion, version)
 			} else {
 				suite.Require().Error(err)
+				suite.Require().Equal("", version)
 			}
 
 		})