From 411c440d4d7b7802ad9dc066c52f1f2500a38349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 22 Dec 2021 17:03:41 +0100 Subject: [PATCH] Modify `OnChanOpenTry` application callback to perform app version negotitation (#646) * remove NegotiateAppVersion and AppVersion gRPC (#643) The NegotiateAppVersion callback has been removed from the IBC Application interface. The gRPC AppVersion has been removed. The app version negoitation will be handled by applications by returning the version in OnChanOpenTry. * 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 | 1 + docs/ibc/apps.md | 50 +++------ docs/ibc/middleware/develop.md | 65 +++++------ docs/ibc/proto-docs.md | 72 +----------- docs/migrations/v2-to-v3.md | 22 +++- .../controller/ibc_module.go | 17 +-- .../controller/ibc_module_test.go | 61 +---------- .../27-interchain-accounts/host/ibc_module.go | 19 +--- .../host/ibc_module_test.go | 76 ++----------- .../host/keeper/handshake.go | 35 ++---- .../host/keeper/handshake_test.go | 26 +---- .../host/keeper/keeper.go | 21 ---- .../host/keeper/keeper_test.go | 2 +- .../host/keeper/relay_test.go | 2 + modules/apps/transfer/ibc_module.go | 41 ++----- modules/apps/transfer/ibc_module_test.go | 11 +- modules/apps/transfer/keeper/keeper_test.go | 2 + modules/apps/transfer/transfer_test.go | 2 + modules/core/04-channel/keeper/handshake.go | 3 +- .../core/04-channel/keeper/handshake_test.go | 15 ++- modules/core/04-channel/types/msgs.go | 2 + modules/core/04-channel/types/tx.pb.go | 6 +- modules/core/05-port/keeper/grpc_query.go | 52 --------- .../core/05-port/keeper/grpc_query_test.go | 103 ------------------ modules/core/05-port/types/module.go | 32 +++--- modules/core/keeper/grpc_query.go | 6 - modules/core/keeper/msg_server.go | 7 +- modules/core/types/query.go | 4 - proto/ibc/core/channel/v1/tx.proto | 6 +- proto/ibc/core/port/v1/query.proto | 35 ------ testing/endpoint.go | 4 + testing/mock/ibc_app.go | 16 +-- testing/mock/ibc_module.go | 31 +----- testing/values.go | 2 +- 34 files changed, 190 insertions(+), 659 deletions(-) delete mode 100644 modules/core/05-port/keeper/grpc_query.go delete mode 100644 modules/core/05-port/keeper/grpc_query_test.go delete mode 100644 proto/ibc/core/port/v1/query.proto diff --git a/CHANGELOG.md b/CHANGELOG.md index 435e0e7330f..f977789ec8e 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/ibc/apps.md b/docs/ibc/apps.md index 6a6b39ba8d7..bb2716fa0b1 100644 --- a/docs/ibc/apps.md +++ b/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 diff --git a/docs/ibc/middleware/develop.md b/docs/ibc/middleware/develop.md index b4cd037e5e6..1d75e3965a8 100644 --- a/docs/ibc/middleware/develop.md +++ b/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) } -``` \ No newline at end of file +``` diff --git a/docs/ibc/proto-docs.md b/docs/ibc/proto-docs.md index c3b10abc545..8baaa8eda1a 100644 --- a/docs/ibc/proto-docs.md +++ b/docs/ibc/proto-docs.md @@ -207,12 +207,6 @@ - [Msg](#ibc.core.connection.v1.Msg) -- [ibc/core/port/v1/query.proto](#ibc/core/port/v1/query.proto) - - [QueryAppVersionRequest](#ibc.core.port.v1.QueryAppVersionRequest) - - [QueryAppVersionResponse](#ibc.core.port.v1.QueryAppVersionResponse) - - - [Query](#ibc.core.port.v1.Query) - - [ibc/core/types/v1/genesis.proto](#ibc/core/types/v1/genesis.proto) - [GenesisState](#ibc.core.types.v1.GenesisState) @@ -1825,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) | | | @@ -3107,67 +3102,6 @@ Msg defines the ibc/connection Msg service. - -

Top

- -## ibc/core/port/v1/query.proto - - - - - -### QueryAppVersionRequest -QueryAppVersionRequest is the request type for the Query/AppVersion RPC method - - -| Field | Type | Label | Description | -| ----- | ---- | ----- | ----------- | -| `port_id` | [string](#string) | | port unique identifier | -| `connection_id` | [string](#string) | | connection unique identifier | -| `ordering` | [ibc.core.channel.v1.Order](#ibc.core.channel.v1.Order) | | whether the channel is ordered or unordered | -| `counterparty` | [ibc.core.channel.v1.Counterparty](#ibc.core.channel.v1.Counterparty) | | counterparty channel end | -| `proposed_version` | [string](#string) | | proposed version | - - - - - - - - -### QueryAppVersionResponse -QueryAppVersionResponse is the response type for the Query/AppVersion RPC method. - - -| Field | Type | Label | Description | -| ----- | ---- | ----- | ----------- | -| `port_id` | [string](#string) | | port id associated with the request identifiers | -| `version` | [string](#string) | | supported app version | - - - - - - - - - - - - - - -### Query -Query defines the gRPC querier service - -| Method Name | Request Type | Response Type | Description | HTTP Verb | Endpoint | -| ----------- | ------------ | ------------- | ------------| ------- | -------- | -| `AppVersion` | [QueryAppVersionRequest](#ibc.core.port.v1.QueryAppVersionRequest) | [QueryAppVersionResponse](#ibc.core.port.v1.QueryAppVersionResponse) | AppVersion queries an IBC Port and determines the appropriate application version to be used | | - - - - -

Top

diff --git a/docs/migrations/v2-to-v3.md b/docs/migrations/v2-to-v3.md index 06a1cb938da..c847a9d3bc9 100644 --- a/docs/migrations/v2-to-v3.md +++ b/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 diff --git a/modules/apps/27-interchain-accounts/controller/ibc_module.go b/modules/apps/27-interchain-accounts/controller/ibc_module.go index dff1456d20c..1aa362a4247 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_module.go +++ b/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 @@ -163,15 +162,3 @@ func (im IBCModule) OnTimeoutPacket( return im.app.OnTimeoutPacket(ctx, packet, relayer) } - -// NegotiateAppVersion implements the IBCModule interface -func (im IBCModule) NegotiateAppVersion( - ctx sdk.Context, - order channeltypes.Order, - connectionID string, - portID string, - counterparty channeltypes.Counterparty, - proposedVersion string, -) (string, error) { - return "", sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "ICS-27 app version negotiation is unsupported on controller chains") -} diff --git a/modules/apps/27-interchain-accounts/controller/ibc_module_test.go b/modules/apps/27-interchain-accounts/controller/ibc_module_test.go index ea4fc108f49..5137606e764 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_module_test.go +++ b/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() { @@ -630,59 +631,3 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() { }) } } - -func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { - var ( - proposedVersion string - ) - testCases := []struct { - name string - malleate func() - expPass bool - }{ - { - "ICA OnRecvPacket fails with ErrInvalidChannelFlow", func() {}, false, - }, - } - - for _, tc := range testCases { - tc := tc - - suite.Run(tc.name, func() { - suite.SetupTest() - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) - suite.Require().NoError(err) - - module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) - suite.Require().NoError(err) - - cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) - - counterpartyPortID, err := icatypes.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) - suite.Require().NoError(err) - - counterparty := channeltypes.Counterparty{ - PortId: counterpartyPortID, - ChannelId: path.EndpointB.ChannelID, - } - - proposedVersion = icatypes.VersionPrefix - - tc.malleate() - - version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, path.EndpointA.ChannelConfig.PortID, counterparty, proposedVersion) - if tc.expPass { - suite.Require().NoError(err) - suite.Require().NoError(icatypes.ValidateVersion(version)) - suite.Require().Equal(TestVersion, version) - } else { - suite.Require().Error(err) - suite.Require().Empty(version) - } - }) - } -} diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index 91513dd434f..bf847349c69 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/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 @@ -137,15 +136,3 @@ func (im IBCModule) OnTimeoutPacket( ) error { return sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "cannot cause a packet timeout on a host channel end, a host chain does not send a packet over the channel") } - -// NegotiateAppVersion implements the IBCModule interface -func (im IBCModule) NegotiateAppVersion( - ctx sdk.Context, - order channeltypes.Order, - connectionID string, - portID string, - counterparty channeltypes.Counterparty, - proposedVersion string, -) (string, error) { - return im.keeper.NegotiateAppVersion(ctx, order, connectionID, portID, counterparty, proposedVersion) -} diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index 245f6dbac63..1f76d661a6c 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/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) } }) @@ -584,61 +586,3 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() { }) } } - -func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { - var ( - proposedVersion string - ) - testCases := []struct { - name string - malleate func() - expPass bool - }{ - { - "success", func() {}, true, - }, - { - "invalid proposed version", func() { - proposedVersion = "invalid version" - }, false, - }, - } - - for _, tc := range testCases { - tc := tc - - suite.Run(tc.name, func() { - suite.SetupTest() - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), icatypes.PortID) - suite.Require().NoError(err) - - cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) - - counterpartyPortID, err := icatypes.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) - suite.Require().NoError(err) - - counterparty := &channeltypes.Counterparty{ - PortId: counterpartyPortID, - ChannelId: path.EndpointB.ChannelID, - } - - proposedVersion = icatypes.VersionPrefix - - tc.malleate() - - version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, icatypes.PortID, *counterparty, proposedVersion) - if tc.expPass { - suite.Require().NoError(err) - suite.Require().NoError(icatypes.ValidateVersion(version)) - suite.Require().Equal(TestVersion, version) - } else { - suite.Require().Error(err) - suite.Require().Empty(version) - } - }) - } -} diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index 0b06ad0042e..bb65da4a33b 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -14,6 +14,8 @@ import ( // OnChanOpenTry performs basic validation of the ICA channel // and registers a new interchain account (if it doesn't exist). +// The version returned will include the registered interchain +// account address. func (k Keeper) OnChanOpenTry( ctx sdk.Context, order channeltypes.Order, @@ -22,60 +24,47 @@ func (k Keeper) OnChanOpenTry( channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, - version, counterpartyVersion string, -) error { +) (string, error) { if order != channeltypes.ORDERED { - return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order) + return "", sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order) } if portID != icatypes.PortID { - return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "expected %s, got %s", icatypes.PortID, portID) + return "", sdkerrors.Wrapf(porttypes.ErrInvalidPort, "expected %s, got %s", icatypes.PortID, portID) } connSequence, err := icatypes.ParseHostConnSequence(counterparty.PortId) if err != nil { - return sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, counterparty.PortId) + return "", sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, counterparty.PortId) } counterpartyConnSequence, err := icatypes.ParseControllerConnSequence(counterparty.PortId) if err != nil { - return sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, counterparty.PortId) + return "", sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, counterparty.PortId) } if err := k.validateControllerPortParams(ctx, connectionHops, connSequence, counterpartyConnSequence); err != nil { - return sdkerrors.Wrapf(err, "failed to validate controller port %s", counterparty.PortId) - } - - if err := icatypes.ValidateVersion(version); err != nil { - return sdkerrors.Wrap(err, "version validation failed") + return "", sdkerrors.Wrapf(err, "failed to validate controller port %s", counterparty.PortId) } if counterpartyVersion != icatypes.VersionPrefix { - return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.VersionPrefix, version) + return "", sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.VersionPrefix, counterpartyVersion) } // 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 { - return sdkerrors.Wrapf(err, "failed to claim capability for channel %s on port %s", channelID, portID) + return "", sdkerrors.Wrapf(err, "failed to claim capability for channel %s on port %s", channelID, portID) } - // Check to ensure that the version string contains the expected address generated from the Counterparty portID accAddr := icatypes.GenerateAddress(k.accountKeeper.GetModuleAddress(icatypes.ModuleName), counterparty.PortId) - parsedAddr, err := icatypes.ParseAddressFromVersion(version) - if err != nil { - return sdkerrors.Wrapf(err, "expected format , got %s", icatypes.Delimiter, version) - } - - if parsedAddr != accAddr.String() { - return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "version contains invalid account address: expected %s, got %s", parsedAddr, accAddr) - } // Register interchain account if it does not already exist k.RegisterInterchainAccount(ctx, accAddr, counterparty.PortId) + version := icatypes.NewAppVersion(icatypes.VersionPrefix, accAddr.String()) - return nil + return version, nil } // OnChanOpenConfirm completes the handshake process by setting the active channel in state on the host chain diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go index 03346efa1a1..48179f1a8a0 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go @@ -81,14 +81,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { }, false, }, - { - "invalid version", - func() { - channel.Version = "version" - path.EndpointB.SetChannel(*channel) - }, - false, - }, { "invalid counterparty version", func() { @@ -106,17 +98,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { }, false, }, - { - "invalid account address", - func() { - portID, err := icatypes.GeneratePortID("invalid-owner-addr", "connection-0", "connection-0") - suite.Require().NoError(err) - - channel.Counterparty.PortId = portID - path.EndpointB.SetChannel(*channel) - }, - false, - }, } for _, tc := range testCases { @@ -151,15 +132,16 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { tc.malleate() // malleate mutates test data - err = suite.chainB.GetSimApp().ICAHostKeeper.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.GetConnectionHops(), - path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), - counterpartyVersion, + version, err := suite.chainB.GetSimApp().ICAHostKeeper.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.GetConnectionHops(), + path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, chanCap, channel.Counterparty, counterpartyVersion, ) if tc.expPass { suite.Require().NoError(err) + suite.Require().Equal(TestVersion, version) } else { suite.Require().Error(err) + suite.Require().Equal("", version) } }) } diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper.go b/modules/apps/27-interchain-accounts/host/keeper/keeper.go index b9a557726ee..5d8569a6e1a 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/host/keeper/keeper.go @@ -7,7 +7,6 @@ import ( baseapp "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" paramtypes "github.com/cosmos/cosmos-sdk/x/params/types" @@ -15,7 +14,6 @@ import ( "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" - channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v3/modules/core/24-host" ) @@ -180,22 +178,3 @@ func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, portID string, addr store := ctx.KVStore(k.storeKey) store.Set(icatypes.KeyOwnerAccount(portID), []byte(address)) } - -// NegotiateAppVersion handles application version negotation for the IBC interchain accounts module -func (k Keeper) NegotiateAppVersion( - ctx sdk.Context, - order channeltypes.Order, - connectionID string, - portID string, - counterparty channeltypes.Counterparty, - proposedVersion string, -) (string, error) { - if proposedVersion != icatypes.VersionPrefix { - return "", sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", icatypes.VersionPrefix, proposedVersion) - } - - moduleAccAddr := k.accountKeeper.GetModuleAddress(icatypes.ModuleName) - accAddr := icatypes.GenerateAddress(moduleAccAddr, counterparty.PortId) - - return icatypes.NewAppVersion(icatypes.VersionPrefix, accAddr.String()), nil -} diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go index e8cffcdb640..102cb278ebe 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go @@ -50,7 +50,7 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path { path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED path.EndpointA.ChannelConfig.Version = icatypes.VersionPrefix - path.EndpointB.ChannelConfig.Version = TestVersion + path.EndpointB.ChannelConfig.Version = icatypes.VersionPrefix return path } diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index b8e4ec54ba7..ef8709b5ce1 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -250,6 +250,8 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { transferPath := ibctesting.NewPath(suite.chainB, suite.chainC) transferPath.EndpointA.ChannelConfig.PortID = ibctesting.TransferPort transferPath.EndpointB.ChannelConfig.PortID = ibctesting.TransferPort + transferPath.EndpointA.ChannelConfig.Version = transfertypes.Version + transferPath.EndpointB.ChannelConfig.Version = transfertypes.Version suite.coordinator.Setup(transferPath) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 5956fa585ad..1ad67d16a85 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -37,7 +37,6 @@ func ValidateTransferChannelParams( order channeltypes.Order, portID string, channelID string, - version string, ) error { // NOTE: for escrow address security only 2^32 channels are allowed to be created // Issue: https://github.com/cosmos/cosmos-sdk/issues/7737 @@ -58,9 +57,6 @@ func ValidateTransferChannelParams( return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid port: %s, expected %s", portID, boundPort) } - if version != types.Version { - return sdkerrors.Wrapf(types.ErrInvalidVersion, "got %s, expected %s", version, types.Version) - } return nil } @@ -75,10 +71,14 @@ func (im IBCModule) OnChanOpenInit( counterparty channeltypes.Counterparty, version string, ) error { - if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID, version); err != nil { + if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID); err != nil { return err } + if 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 @@ -87,7 +87,7 @@ func (im IBCModule) OnChanOpenInit( return nil } -// OnChanOpenTry implements the IBCModule interface +// OnChanOpenTry implements the IBCModule interface. func (im IBCModule) OnChanOpenTry( ctx sdk.Context, order channeltypes.Order, @@ -96,15 +96,14 @@ func (im IBCModule) OnChanOpenTry( channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, - version, counterpartyVersion string, -) error { - if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID, version); err != nil { - return err +) (string, error) { + if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID); err != nil { + return "", err } if counterpartyVersion != types.Version { - return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: got: %s, expected %s", counterpartyVersion, types.Version) + return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: got: %s, expected %s", counterpartyVersion, types.Version) } // Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos @@ -114,11 +113,11 @@ func (im IBCModule) OnChanOpenTry( if !im.keeper.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) { // Only claim channel capability passed back by IBC module if we do not already own it if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return err + return "", err } } - return nil + return types.Version, nil } // OnChanOpenAck implements the IBCModule interface @@ -279,19 +278,3 @@ func (im IBCModule) OnTimeoutPacket( return nil } - -// NegotiateAppVersion implements the IBCModule interface -func (im IBCModule) NegotiateAppVersion( - ctx sdk.Context, - order channeltypes.Order, - connectionID string, - portID string, - counterparty channeltypes.Counterparty, - proposedVersion string, -) (string, error) { - if proposedVersion != types.Version { - return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.Version, proposedVersion) - } - - return types.Version, nil -} diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 7ab67b0af06..9ffd7ddcf4f 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -135,11 +135,6 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() { path.EndpointA.ChannelConfig.PortID = ibctesting.MockPort }, false, }, - { - "invalid version", func() { - channel.Version = "version" - }, false, - }, { "invalid counterparty version", func() { counterpartyVersion = "version" @@ -178,14 +173,16 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() { tc.malleate() // explicitly change fields in channel and testChannel - err = cbs.OnChanOpenTry(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), - path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), counterpartyVersion, + version, err := cbs.OnChanOpenTry(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), + path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, counterpartyVersion, ) if tc.expPass { suite.Require().NoError(err) + suite.Require().Equal(types.Version, version) } else { suite.Require().Error(err) + suite.Require().Equal("", version) } }) diff --git a/modules/apps/transfer/keeper/keeper_test.go b/modules/apps/transfer/keeper/keeper_test.go index ef748757588..0fdb1121387 100644 --- a/modules/apps/transfer/keeper/keeper_test.go +++ b/modules/apps/transfer/keeper/keeper_test.go @@ -40,6 +40,8 @@ func NewTransferPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path { path := ibctesting.NewPath(chainA, chainB) path.EndpointA.ChannelConfig.PortID = ibctesting.TransferPort path.EndpointB.ChannelConfig.PortID = ibctesting.TransferPort + path.EndpointA.ChannelConfig.Version = types.Version + path.EndpointB.ChannelConfig.Version = types.Version return path } diff --git a/modules/apps/transfer/transfer_test.go b/modules/apps/transfer/transfer_test.go index d2f822c83a2..fc16aa39a28 100644 --- a/modules/apps/transfer/transfer_test.go +++ b/modules/apps/transfer/transfer_test.go @@ -34,6 +34,8 @@ func NewTransferPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path { path := ibctesting.NewPath(chainA, chainB) path.EndpointA.ChannelConfig.PortID = ibctesting.TransferPort path.EndpointB.ChannelConfig.PortID = ibctesting.TransferPort + path.EndpointA.ChannelConfig.Version = types.Version + path.EndpointB.ChannelConfig.Version = types.Version return path } diff --git a/modules/core/04-channel/keeper/handshake.go b/modules/core/04-channel/keeper/handshake.go index e35ae4bd851..a9c5046c099 100644 --- a/modules/core/04-channel/keeper/handshake.go +++ b/modules/core/04-channel/keeper/handshake.go @@ -118,7 +118,6 @@ func (k Keeper) ChanOpenTry( previousChannelID string, portCap *capabilitytypes.Capability, counterparty types.Counterparty, - version, counterpartyVersion string, proofInit []byte, proofHeight exported.Height, @@ -148,7 +147,7 @@ func (k Keeper) ChanOpenTry( previousChannel.Counterparty.PortId == counterparty.PortId && previousChannel.Counterparty.ChannelId == "" && previousChannel.ConnectionHops[0] == connectionHops[0] && // ChanOpenInit will only set a single connection hop - previousChannel.Version == version) { + previousChannel.Version == counterpartyVersion) { return "", nil, sdkerrors.Wrap(types.ErrInvalidChannel, "channel fields mismatch previous channel fields") } diff --git a/modules/core/04-channel/keeper/handshake_test.go b/modules/core/04-channel/keeper/handshake_test.go index 1ed17c6d3fc..a9c24beb0aa 100644 --- a/modules/core/04-channel/keeper/handshake_test.go +++ b/modules/core/04-channel/keeper/handshake_test.go @@ -166,6 +166,19 @@ func (suite *KeeperTestSuite) TestChanOpenTry() { previousChannelID = path.EndpointB.ChannelID portCap = suite.chainB.GetPortCapability(ibctesting.MockPort) }, true}, + {"previous channel with invalid version, crossing hello", func() { + suite.coordinator.SetupConnections(path) + path.SetChannelOrdered() + + // modify channel version + path.EndpointA.ChannelConfig.Version = "invalid version" + + err := suite.coordinator.ChanOpenInitOnBothChains(path) + suite.Require().NoError(err) + + previousChannelID = path.EndpointB.ChannelID + portCap = suite.chainB.GetPortCapability(ibctesting.MockPort) + }, false}, {"previous channel with invalid state", func() { suite.coordinator.SetupConnections(path) @@ -272,7 +285,7 @@ func (suite *KeeperTestSuite) TestChanOpenTry() { channelID, cap, err := suite.chainB.App.GetIBCKeeper().ChannelKeeper.ChanOpenTry( suite.chainB.GetContext(), types.ORDERED, []string{path.EndpointB.ConnectionID}, - path.EndpointB.ChannelConfig.PortID, previousChannelID, portCap, counterparty, path.EndpointB.ChannelConfig.Version, path.EndpointA.ChannelConfig.Version, + path.EndpointB.ChannelConfig.PortID, previousChannelID, portCap, counterparty, path.EndpointA.ChannelConfig.Version, proof, malleateHeight(proofHeight, heightDiff), ) diff --git a/modules/core/04-channel/types/msgs.go b/modules/core/04-channel/types/msgs.go index d940eb9ae8c..b6e625ff32e 100644 --- a/modules/core/04-channel/types/msgs.go +++ b/modules/core/04-channel/types/msgs.go @@ -61,6 +61,8 @@ func (msg MsgChannelOpenInit) GetSigners() []sdk.AccAddress { var _ sdk.Msg = &MsgChannelOpenTry{} // NewMsgChannelOpenTry creates a new MsgChannelOpenTry instance +// The version string is deprecated and will be ignored by core IBC. +// It is left as an argument for go API backwards compatibility. // nolint:interfacer func NewMsgChannelOpenTry( portID, previousChannelID, version string, channelOrder Order, connectionHops []string, diff --git a/modules/core/04-channel/types/tx.pb.go b/modules/core/04-channel/types/tx.pb.go index 1a007540a64..2d3c75345ce 100644 --- a/modules/core/04-channel/types/tx.pb.go +++ b/modules/core/04-channel/types/tx.pb.go @@ -108,12 +108,14 @@ func (m *MsgChannelOpenInitResponse) XXX_DiscardUnknown() { var xxx_messageInfo_MsgChannelOpenInitResponse proto.InternalMessageInfo // 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. type MsgChannelOpenTry struct { PortId string `protobuf:"bytes,1,opt,name=port_id,json=portId,proto3" json:"port_id,omitempty" yaml:"port_id"` // in the case of crossing hello's, when both chains call OpenInit, we need // the channel identifier of the previous channel in state INIT - PreviousChannelId string `protobuf:"bytes,2,opt,name=previous_channel_id,json=previousChannelId,proto3" json:"previous_channel_id,omitempty" yaml:"previous_channel_id"` + PreviousChannelId string `protobuf:"bytes,2,opt,name=previous_channel_id,json=previousChannelId,proto3" json:"previous_channel_id,omitempty" yaml:"previous_channel_id"` + // NOTE: the version field within the channel has been deprecated. Its value will be ignored by core IBC. Channel Channel `protobuf:"bytes,3,opt,name=channel,proto3" json:"channel"` CounterpartyVersion string `protobuf:"bytes,4,opt,name=counterparty_version,json=counterpartyVersion,proto3" json:"counterparty_version,omitempty" yaml:"counterparty_version"` ProofInit []byte `protobuf:"bytes,5,opt,name=proof_init,json=proofInit,proto3" json:"proof_init,omitempty" yaml:"proof_init"` diff --git a/modules/core/05-port/keeper/grpc_query.go b/modules/core/05-port/keeper/grpc_query.go deleted file mode 100644 index 508c9749ccc..00000000000 --- a/modules/core/05-port/keeper/grpc_query.go +++ /dev/null @@ -1,52 +0,0 @@ -package keeper - -import ( - "context" - - sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" - - "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" - host "github.com/cosmos/ibc-go/v3/modules/core/24-host" -) - -var _ types.QueryServer = (*Keeper)(nil) - -// AppVersion implements the Query/AppVersion gRPC method -func (q Keeper) AppVersion(c context.Context, req *types.QueryAppVersionRequest) (*types.QueryAppVersionResponse, error) { - if req == nil { - return nil, status.Error(codes.InvalidArgument, "empty request") - } - - if err := validategRPCRequest(req.PortId); err != nil { - return nil, err - } - - ctx := sdk.UnwrapSDKContext(c) - module, _, err := q.LookupModuleByPort(ctx, req.PortId) - if err != nil { - return nil, status.Errorf(codes.NotFound, sdkerrors.Wrap(err, "could not retrieve module from port-id").Error()) - } - - ibcModule, found := q.Router.GetRoute(module) - if !found { - return nil, status.Errorf(codes.NotFound, sdkerrors.Wrapf(types.ErrInvalidRoute, "route not found to module: %s", module).Error()) - } - - version, err := ibcModule.NegotiateAppVersion(ctx, req.Ordering, req.ConnectionId, req.PortId, *req.Counterparty, req.ProposedVersion) - if err != nil { - return nil, status.Errorf(codes.InvalidArgument, sdkerrors.Wrap(err, "version negotation failed").Error()) - } - - return types.NewQueryAppVersionResponse(req.PortId, version), nil -} - -func validategRPCRequest(portID string) error { - if err := host.PortIdentifierValidator(portID); err != nil { - return status.Error(codes.InvalidArgument, err.Error()) - } - - return nil -} diff --git a/modules/core/05-port/keeper/grpc_query_test.go b/modules/core/05-port/keeper/grpc_query_test.go deleted file mode 100644 index d7f144ab3bf..00000000000 --- a/modules/core/05-port/keeper/grpc_query_test.go +++ /dev/null @@ -1,103 +0,0 @@ -package keeper_test - -import ( - "fmt" - - sdk "github.com/cosmos/cosmos-sdk/types" - - channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" - "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" - "github.com/cosmos/ibc-go/v3/testing/mock" -) - -func (suite *KeeperTestSuite) TestAppVersion() { - var ( - req *types.QueryAppVersionRequest - expVersion string - ) - - testCases := []struct { - msg string - malleate func() - expPass bool - }{ - { - "empty request", - func() { - req = nil - }, - false, - }, - { - "invalid port ID", - func() { - req = &types.QueryAppVersionRequest{ - PortId: "", - } - }, - false, - }, - { - "module not found", - func() { - req = &types.QueryAppVersionRequest{ - PortId: "mock-port-id", - } - }, - false, - }, - { - "version negotiation failure", - func() { - - expVersion = mock.Version - - req = &types.QueryAppVersionRequest{ - PortId: "mock", // retrieves the mock testing module - Counterparty: &channeltypes.Counterparty{ - PortId: "mock-port-id", - ChannelId: "mock-channel-id", - }, - ProposedVersion: "invalid-proposed-version", - } - }, - false, - }, - { - "success", - func() { - - expVersion = mock.Version - - req = &types.QueryAppVersionRequest{ - PortId: "mock", // retrieves the mock testing module - Counterparty: &channeltypes.Counterparty{ - PortId: "mock-port-id", - ChannelId: "mock-channel-id", - }, - ProposedVersion: mock.Version, - } - }, - true, - }, - } - - for _, tc := range testCases { - suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { - suite.SetupTest() // reset - - tc.malleate() - - ctx := sdk.WrapSDKContext(suite.ctx) - res, err := suite.keeper.AppVersion(ctx, req) - - if tc.expPass { - suite.Require().NoError(err) - suite.Require().NotNil(res) - suite.Require().Equal(expVersion, res.Version) - } else { - suite.Require().Error(err) - } - }) - } -} diff --git a/modules/core/05-port/types/module.go b/modules/core/05-port/types/module.go index cd8c5719dc3..9c7442a9d76 100644 --- a/modules/core/05-port/types/module.go +++ b/modules/core/05-port/types/module.go @@ -2,8 +2,8 @@ package types import ( sdk "github.com/cosmos/cosmos-sdk/types" - capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" "github.com/cosmos/ibc-go/v3/modules/core/exported" ) @@ -11,6 +11,10 @@ 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( ctx sdk.Context, order channeltypes.Order, @@ -22,6 +26,14 @@ type IBCModule interface { version string, ) error + // OnChanOpenTry will verify the relayer-chosen parameters along with the + // counterparty-chosen version string and perform custom TRY logic. + // If the relayer-chosen parameters are invalid, the callback must return + // an error to abort the handshake. If the counterparty-chosen version is not + // compatible with this modules supported versions, the callback must return + // an error to abort the handshake. If the versions are compatible, the try callback + // must select the final version string and return it to core IBC. + // OnChanOpenTry may also perform custom initialization logic OnChanOpenTry( ctx sdk.Context, order channeltypes.Order, @@ -30,10 +42,11 @@ type IBCModule interface { channelID string, channelCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, - version, counterpartyVersion string, - ) error + ) (version string, err error) + // OnChanOpenAck will error if the counterparty selected version string + // is invalid to abort the handshake. It may also perform custom ACK logic. OnChanOpenAck( ctx sdk.Context, portID, @@ -41,6 +54,7 @@ type IBCModule interface { counterpartyVersion string, ) error + // OnChanOpenConfirm will perform custom CONFIRM logic and may error to abort the handshake. OnChanOpenConfirm( ctx sdk.Context, portID, @@ -82,18 +96,6 @@ type IBCModule interface { packet channeltypes.Packet, relayer sdk.AccAddress, ) error - - // 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) } // ICS4Wrapper implements the ICS4 interfaces that IBC applications use to send packets and acknolwedgements. diff --git a/modules/core/keeper/grpc_query.go b/modules/core/keeper/grpc_query.go index 5e45facf84f..2fb171a9c02 100644 --- a/modules/core/keeper/grpc_query.go +++ b/modules/core/keeper/grpc_query.go @@ -6,7 +6,6 @@ import ( clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/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" ) // ClientState implements the IBC QueryServer interface @@ -133,8 +132,3 @@ func (q Keeper) UnreceivedAcks(c context.Context, req *channeltypes.QueryUnrecei func (q Keeper) NextSequenceReceive(c context.Context, req *channeltypes.QueryNextSequenceReceiveRequest) (*channeltypes.QueryNextSequenceReceiveResponse, error) { return q.ChannelKeeper.NextSequenceReceive(c, req) } - -// AppVersion implements the IBC QueryServer interface -func (q Keeper) AppVersion(c context.Context, req *porttypes.QueryAppVersionRequest) (*porttypes.QueryAppVersionResponse, error) { - return q.PortKeeper.AppVersion(c, req) -} diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 039167da7a0..dbc4ac07812 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -312,19 +312,20 @@ func (k Keeper) ChannelOpenTry(goCtx context.Context, msg *channeltypes.MsgChann // Perform 04-channel verification channelID, cap, err := k.ChannelKeeper.ChanOpenTry(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, msg.PreviousChannelId, - portCap, msg.Channel.Counterparty, msg.Channel.Version, msg.CounterpartyVersion, msg.ProofInit, msg.ProofHeight, + portCap, msg.Channel.Counterparty, msg.CounterpartyVersion, msg.ProofInit, msg.ProofHeight, ) if err != nil { return nil, sdkerrors.Wrap(err, "channel handshake open try failed") } // Perform application logic callback - if err = cbs.OnChanOpenTry(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.Channel.Version, msg.CounterpartyVersion); err != nil { + version, err := cbs.OnChanOpenTry(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.CounterpartyVersion) + if err != nil { return nil, sdkerrors.Wrap(err, "channel open try callback failed") } // Write channel into state - k.ChannelKeeper.WriteOpenTryChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, msg.Channel.Version) + k.ChannelKeeper.WriteOpenTryChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, version) return &channeltypes.MsgChannelOpenTryResponse{}, nil } diff --git a/modules/core/types/query.go b/modules/core/types/query.go index dd14f6c57f0..ef9d0589448 100644 --- a/modules/core/types/query.go +++ b/modules/core/types/query.go @@ -9,8 +9,6 @@ import ( connectiontypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types" channel "github.com/cosmos/ibc-go/v3/modules/core/04-channel" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" - port "github.com/cosmos/ibc-go/v3/modules/core/05-port" - porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" ) // QueryServer defines the IBC interfaces that the gRPC query server must implement @@ -18,7 +16,6 @@ type QueryServer interface { clienttypes.QueryServer connectiontypes.QueryServer channeltypes.QueryServer - porttypes.QueryServer } // RegisterQueryService registers each individual IBC submodule query service @@ -26,5 +23,4 @@ func RegisterQueryService(server grpc.Server, queryService QueryServer) { client.RegisterQueryService(server, queryService) connection.RegisterQueryService(server, queryService) channel.RegisterQueryService(server, queryService) - port.RegisterQueryService(server, queryService) } diff --git a/proto/ibc/core/channel/v1/tx.proto b/proto/ibc/core/channel/v1/tx.proto index c9322c1def7..4f28418c9a2 100644 --- a/proto/ibc/core/channel/v1/tx.proto +++ b/proto/ibc/core/channel/v1/tx.proto @@ -57,7 +57,8 @@ message MsgChannelOpenInit { message MsgChannelOpenInitResponse {} // 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. message MsgChannelOpenTry { option (gogoproto.equal) = false; option (gogoproto.goproto_getters) = false; @@ -65,7 +66,8 @@ message MsgChannelOpenTry { string port_id = 1 [(gogoproto.moretags) = "yaml:\"port_id\""]; // in the case of crossing hello's, when both chains call OpenInit, we need // the channel identifier of the previous channel in state INIT - string previous_channel_id = 2 [(gogoproto.moretags) = "yaml:\"previous_channel_id\""]; + string previous_channel_id = 2 [(gogoproto.moretags) = "yaml:\"previous_channel_id\""]; + // NOTE: the version field within the channel has been deprecated. Its value will be ignored by core IBC. Channel channel = 3 [(gogoproto.nullable) = false]; string counterparty_version = 4 [(gogoproto.moretags) = "yaml:\"counterparty_version\""]; bytes proof_init = 5 [(gogoproto.moretags) = "yaml:\"proof_init\""]; diff --git a/proto/ibc/core/port/v1/query.proto b/proto/ibc/core/port/v1/query.proto deleted file mode 100644 index 4ae44a5db70..00000000000 --- a/proto/ibc/core/port/v1/query.proto +++ /dev/null @@ -1,35 +0,0 @@ -syntax = "proto3"; - -package ibc.core.port.v1; - -option go_package = "github.com/cosmos/ibc-go/v3/modules/core/05-port/types"; - -import "ibc/core/channel/v1/channel.proto"; - -// Query defines the gRPC querier service -service Query { - // AppVersion queries an IBC Port and determines the appropriate application version to be used - rpc AppVersion(QueryAppVersionRequest) returns (QueryAppVersionResponse) {} -} - -// QueryAppVersionRequest is the request type for the Query/AppVersion RPC method -message QueryAppVersionRequest { - // port unique identifier - string port_id = 1; - // connection unique identifier - string connection_id = 2; - // whether the channel is ordered or unordered - ibc.core.channel.v1.Order ordering = 3; - // counterparty channel end - ibc.core.channel.v1.Counterparty counterparty = 4; - // proposed version - string proposed_version = 5; -} - -// QueryAppVersionResponse is the response type for the Query/AppVersion RPC method. -message QueryAppVersionResponse { - // port id associated with the request identifiers - string port_id = 1; - // supported app version - string version = 2; -} diff --git a/testing/endpoint.go b/testing/endpoint.go index a2ab014fe99..4c3804879c9 100644 --- a/testing/endpoint.go +++ b/testing/endpoint.go @@ -306,6 +306,10 @@ func (endpoint *Endpoint) ChanOpenTry() error { require.NoError(endpoint.Chain.t, err) } + // update version to selected app version + // NOTE: this update must be performed after the endpoint channelID is set + endpoint.ChannelConfig.Version = endpoint.GetChannel().Version + return nil } diff --git a/testing/mock/ibc_app.go b/testing/mock/ibc_app.go index a4a7460c05f..a3f2db3bc6d 100644 --- a/testing/mock/ibc_app.go +++ b/testing/mock/ibc_app.go @@ -8,6 +8,7 @@ import ( "github.com/cosmos/ibc-go/v3/modules/core/exported" ) +// MockIBCApp contains IBC application module callbacks as defined in 05-port. type MockIBCApp struct { OnChanOpenInit func( ctx sdk.Context, @@ -28,9 +29,8 @@ type MockIBCApp struct { channelID string, channelCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, - version, counterpartyVersion string, - ) error + ) (version string, err error) OnChanOpenAck func( ctx sdk.Context, @@ -80,16 +80,4 @@ type MockIBCApp struct { packet channeltypes.Packet, relayer sdk.AccAddress, ) error - - // 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 func( - ctx sdk.Context, - order channeltypes.Order, - connectionID string, - portID string, - counterparty channeltypes.Counterparty, - proposedVersion string, - ) (version string, err error) } diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index 128f17cf799..84e57b8f25d 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -2,7 +2,6 @@ package mock import ( "bytes" - "errors" "strconv" sdk "github.com/cosmos/cosmos-sdk/types" @@ -49,18 +48,18 @@ func (im IBCModule) OnChanOpenInit( // OnChanOpenTry implements the IBCModule interface. func (im IBCModule) OnChanOpenTry( ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string, - channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version, counterpartyVersion string, -) error { + channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, counterpartyVersion string, +) (version string, err error) { if im.IBCApp.OnChanOpenTry != nil { - return im.IBCApp.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion) + return im.IBCApp.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, counterpartyVersion) } // Claim channel capability passed back by IBC module if err := im.scopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return err + return "", err } - return nil + return Version, nil } // OnChanOpenAck implements the IBCModule interface. @@ -152,23 +151,3 @@ func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, return nil } - -// NegotiateAppVersion implements the IBCModule interface. -func (im IBCModule) NegotiateAppVersion( - ctx sdk.Context, - order channeltypes.Order, - connectionID string, - portID string, - counterparty channeltypes.Counterparty, - proposedVersion string, -) (string, error) { - if im.IBCApp.NegotiateAppVersion != nil { - return im.IBCApp.NegotiateAppVersion(ctx, order, connectionID, portID, counterparty, proposedVersion) - } - - if proposedVersion != Version { // allow testing of error scenarios - return "", errors.New("failed to negotiate app version") - } - - return Version, nil -} diff --git a/testing/values.go b/testing/values.go index e341b98fd50..8dbfa66d3ab 100644 --- a/testing/values.go +++ b/testing/values.go @@ -29,7 +29,7 @@ const ( MaxClockDrift time.Duration = time.Second * 10 DefaultDelayPeriod uint64 = 0 - DefaultChannelVersion = ibctransfertypes.Version + DefaultChannelVersion = mock.Version InvalidID = "IDisInvalid" // Application Ports