diff --git a/CHANGELOG.md b/CHANGELOG.md index b38ee315afc..7968f31ea84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking * (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used. +* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) The `OnChanOpenInit` application callback now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629). * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`. * (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`. diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index 90e9af256d2..5a3d61deaae 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -1,4 +1,4 @@ -# Migrating from ibc-go v2 to v3 +# Migrating from ibc-go v3 to v4 This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG. Any changes that must be done by a user of ibc-go should be documented here. @@ -23,4 +23,8 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. This is an API breaking change and as such IBC application developers will have to update any calls to `WriteAcknowledgement`. +The `OnChanOpenInit` application callback has been modified. +The return signature now includes the application version as detailed in the latest IBC [spec changes](https://github.com/cosmos/ibc/pull/629). + + diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index c12f5d3e9ce..9bf874fa8eb 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -45,18 +45,23 @@ func (im IBCMiddleware) OnChanOpenInit( chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string, -) error { +) (string, error) { if !im.keeper.IsControllerEnabled(ctx) { - return types.ErrControllerSubModuleDisabled + return "", types.ErrControllerSubModuleDisabled } if err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil { - return err + return "", err + } + + // call underlying app's OnChanOpenInit callback with the passed in version + // the version returned is discarded as the ica-auth module does not have permission to edit the version string. + // ics27 will always return the version string containing the Metadata struct which is created during the `RegisterInterchainAccount` call. + if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil { + return "", err } - // call underlying app's OnChanOpenInit callback with the appVersion - return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, - chanCap, counterparty, version) + return version, nil } // OnChanOpenTry implements the IBCMiddleware interface diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go index ea1b5185af2..dd7849efca9 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -147,8 +147,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID, channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string, - ) error { - return fmt.Errorf("mock ica auth fails") + ) (string, error) { + return "", fmt.Errorf("mock ica auth fails") } }, false, }, @@ -197,11 +197,24 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) suite.Require().True(ok) - err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), + version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(), ) if tc.expPass { + expMetadata := icatypes.NewMetadata( + icatypes.Version, + path.EndpointA.ConnectionID, + path.EndpointB.ConnectionID, + "", + icatypes.EncodingProtobuf, + icatypes.TxTypeSDKMultiMsg, + ) + + expBytes, err := icatypes.ModuleCdc.MarshalJSON(&expMetadata) + suite.Require().NoError(err) + + suite.Require().Equal(version, string(expBytes)) suite.Require().NoError(err) } else { suite.Require().Error(err) diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index fb403c71937..1598801601f 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/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 diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index 96bb8be4030..3e6e7afb26d 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -1,6 +1,8 @@ package fee import ( + "strings" + sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" @@ -39,25 +41,44 @@ func (im IBCMiddleware) OnChanOpenInit( chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string, -) error { +) (string, error) { var versionMetadata types.Metadata - if err := types.ModuleCdc.UnmarshalJSON([]byte(version), &versionMetadata); err != nil { - // Since it is valid for fee version to not be specified, the above middleware version may be for a middleware - // lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying - // application. - return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, - chanCap, counterparty, version) + + if strings.TrimSpace(version) == "" { + // default version + versionMetadata = types.Metadata{ + FeeVersion: types.Version, + AppVersion: "", + } + } else { + if err := types.ModuleCdc.UnmarshalJSON([]byte(version), &versionMetadata); err != nil { + // Since it is valid for fee version to not be specified, the above middleware version may be for a middleware + // lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying + // application. + return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, + chanCap, counterparty, version) + } } if versionMetadata.FeeVersion != types.Version { - return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion) + return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion) + } + + appVersion, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, versionMetadata.AppVersion) + if err != nil { + return "", err + } + + versionMetadata.AppVersion = appVersion + versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata) + if err != nil { + return "", err } im.keeper.SetFeeEnabled(ctx, portID, channelID) // call underlying app's OnChanOpenInit callback with the appVersion - return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, - chanCap, counterparty, versionMetadata.AppVersion) + return string(versionBytes), nil } // OnChanOpenTry implements the IBCMiddleware interface @@ -94,7 +115,6 @@ func (im IBCMiddleware) OnChanOpenTry( } versionMetadata.AppVersion = appVersion - versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata) if err != nil { return "", err @@ -116,7 +136,7 @@ func (im IBCMiddleware) OnChanOpenAck( if im.keeper.IsFeeEnabled(ctx, portID, channelID) { var versionMetadata types.Metadata if err := types.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &versionMetadata); err != nil { - return sdkerrors.Wrap(types.ErrInvalidVersion, "failed to unmarshal ICS29 counterparty version metadata") + return sdkerrors.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion) } if versionMetadata.FeeVersion != types.Version { diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 8c76720ad3e..4b6d7f98959 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -25,34 +25,46 @@ var ( // Tests OnChanOpenInit on ChainA func (suite *FeeTestSuite) TestOnChanOpenInit() { testCases := []struct { - name string - version string - expPass bool + name string + version string + expPass bool + isFeeEnabled bool }{ { "success - valid fee middleware and mock version", string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version})), true, + true, }, { "success - fee version not included, only perform mock logic", ibcmock.Version, true, + false, }, { "invalid fee middleware version", string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: ibcmock.Version})), false, + false, }, { "invalid mock version", string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-mock-version"})), false, + false, }, { "mock version not wrapped", types.Version, false, + false, + }, + { + "passing an empty string returns default version", + "", + true, + true, }, } @@ -68,11 +80,11 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() { suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID, channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string, - ) error { + ) (string, error) { if version != ibcmock.Version { - return fmt.Errorf("incorrect mock version") + return "", fmt.Errorf("incorrect mock version") } - return nil + return ibcmock.Version, nil } suite.path.EndpointA.ChannelID = ibctesting.FirstChannelID @@ -95,13 +107,29 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() { cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) suite.Require().True(ok) - err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), + version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, chanCap, counterparty, channel.Version) if tc.expPass { + // check if the channel is fee enabled. If so version string should include metaData + if tc.isFeeEnabled { + versionMetadata := types.Metadata{ + FeeVersion: types.Version, + AppVersion: ibcmock.Version, + } + + versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata) + suite.Require().NoError(err) + + suite.Require().Equal(version, string(versionBytes)) + } else { + suite.Require().Equal(ibcmock.Version, version) + } + suite.Require().NoError(err, "unexpected error from version: %s", tc.version) } else { suite.Require().Error(err, "error not returned for version: %s", tc.version) + suite.Require().Equal("", version) } }) } diff --git a/modules/apps/29-fee/transfer_test.go b/modules/apps/29-fee/transfer_test.go index 9d7557fd6c4..ba1f505d9c0 100644 --- a/modules/apps/29-fee/transfer_test.go +++ b/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), ) - } diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index f5ed807d8b2..b9bfc2be189 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/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. diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 3dcb5518cbb..95779ad2e8e 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -5,6 +5,7 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + "github.com/cosmos/ibc-go/v3/modules/apps/transfer" "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v3/modules/core/24-host" @@ -28,6 +29,11 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() { { "success", func() {}, true, }, + { + "empty version string", func() { + channel.Version = "" + }, true, + }, { "max channels reached", func() { path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1) @@ -74,25 +80,23 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() { Version: types.Version, } - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort) - suite.Require().NoError(err) - + var err error chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(ibctesting.TransferPort, path.EndpointA.ChannelID)) suite.Require().NoError(err) - cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) - suite.Require().True(ok) - tc.malleate() // explicitly change fields in channel and testChannel - err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), + transferModule := transfer.NewIBCModule(suite.chainA.GetSimApp().TransferKeeper) + version, err := transferModule.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, counterparty, channel.GetVersion(), ) if tc.expPass { suite.Require().NoError(err) + suite.Require().Equal(types.Version, version) } else { suite.Require().Error(err) + suite.Require().Equal(version, "") } }) diff --git a/modules/core/05-port/types/module.go b/modules/core/05-port/types/module.go index e6ba8f3449b..7ac57479dbd 100644 --- a/modules/core/05-port/types/module.go +++ b/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. diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index e8b7c0fd254..a4aeb496f6f 100644 --- a/modules/core/keeper/msg_server.go +++ b/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 } diff --git a/testing/endpoint.go b/testing/endpoint.go index 02c4e9aac39..94fa14f8b0d 100644 --- a/testing/endpoint.go +++ b/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 } diff --git a/testing/mock/ibc_app.go b/testing/mock/ibc_app.go index 77eb17b8c6f..0504c2df0de 100644 --- a/testing/mock/ibc_app.go +++ b/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, diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index e58f6ae7156..69f3c388a8a 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "strconv" + "strings" sdk "github.com/cosmos/cosmos-sdk/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" @@ -32,18 +33,21 @@ func NewIBCModule(appModule *AppModule, app *MockIBCApp) IBCModule { func (im IBCModule) OnChanOpenInit( ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string, channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string, -) error { +) (string, error) { + if strings.TrimSpace(version) == "" { + version = Version + } + if im.IBCApp.OnChanOpenInit != nil { return im.IBCApp.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version) - } // Claim channel capability passed back by IBC module if err := im.IBCApp.ScopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { - return err + return "", err } - return nil + return version, nil } // OnChanOpenTry implements the IBCModule interface.