From bd29d3696df1cf6345f3e488af0ff2a296c0b120 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 29 Jul 2022 14:32:11 +0200 Subject: [PATCH] fix: assert previous channel identifer is empty in Msg ValidateBasic (backport #1792) (#1827) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: assert previous channel identifer is empty in Msg ValidateBasic (#1792) * remove previous channel id from NewMsgChanOpenTry, assert previous channel id to be empty * add changelog entry * move changelog entry to correct location * add migration docs * Update docs/migrations/v3-to-v4.md Co-authored-by: Carlos Rodriguez * Update CHANGELOG.md Co-authored-by: Carlos Rodriguez Co-authored-by: Carlos Rodriguez (cherry picked from commit 4a781e55c14363a07961dbc3314a6aa4c84491d5) # Conflicts: # CHANGELOG.md * fix conflicts Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Carlos Rodriguez --- CHANGELOG.md | 1 + docs/migrations/v3-to-v4.md | 2 ++ .../controller/ibc_middleware_test.go | 2 +- modules/core/04-channel/types/msgs.go | 7 ++-- modules/core/04-channel/types/msgs_test.go | 36 +++++++++---------- testing/endpoint.go | 2 +- 6 files changed, 24 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 811b32497ac..164f5a76921 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking +* (core/04-channel) [\#1792](https://github.com/cosmos/ibc-go/pull/1792) Remove `PreviousChannelID` from `NewMsgChannelOpenTry` arguments. `MsgChannelOpenTry.ValidateBasic()` returns error if the deprecated `PreviousChannelID` is not empty. * (modules/core/03-connection) [\#1672](https://github.com/cosmos/ibc-go/pull/1672) Remove crossing hellos from connection handshakes. The `PreviousConnectionId` in `MsgConnectionOpenTry` has been deprecated. * (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove crossing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated. * (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used. diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index bdf8654dcb7..ec75d3be569 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -63,6 +63,8 @@ Crossing hellos have been removed from 04-channel handshake negotiation. IBC Applications no longer need to account from already claimed capabilities in the `OnChanOpenTry` callback. The capability provided by core IBC must be able to be claimed with error. `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated and is no longer used by core IBC. +`NewMsgChannelOpenTry` no longer takes in the `PreviousChannelId` as crossing hellos are no longer supported. A non-empty `PreviousChannelId` will fail basic validation for this message. + ### ICS27 - Interchain Accounts The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middleware, for relayer incentivization of ICS27 packets. 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 44c1ced71b9..cc0a5113669 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -244,7 +244,7 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenTry() { proofInit, proofHeight := path.EndpointB.Chain.QueryProof(channelKey) // use chainA (controller) for ChanOpenTry - msg := channeltypes.NewMsgChannelOpenTry(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, TestVersion, channeltypes.ORDERED, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, TestVersion, proofInit, proofHeight, icatypes.ModuleName) + msg := channeltypes.NewMsgChannelOpenTry(path.EndpointA.ChannelConfig.PortID, TestVersion, channeltypes.ORDERED, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, TestVersion, proofInit, proofHeight, icatypes.ModuleName) handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg) _, err = handler(suite.chainA.GetContext(), msg) diff --git a/modules/core/04-channel/types/msgs.go b/modules/core/04-channel/types/msgs.go index 6bc2a490f31..13475796a8e 100644 --- a/modules/core/04-channel/types/msgs.go +++ b/modules/core/04-channel/types/msgs.go @@ -66,7 +66,7 @@ var _ sdk.Msg = &MsgChannelOpenTry{} // It is left as an argument for go API backwards compatibility. // nolint:interfacer func NewMsgChannelOpenTry( - portID, previousChannelID, version string, channelOrder Order, connectionHops []string, + portID, version string, channelOrder Order, connectionHops []string, counterpartyPortID, counterpartyChannelID, counterpartyVersion string, proofInit []byte, proofHeight clienttypes.Height, signer string, ) *MsgChannelOpenTry { @@ -74,7 +74,6 @@ func NewMsgChannelOpenTry( channel := NewChannel(TRYOPEN, channelOrder, counterparty, connectionHops, version) return &MsgChannelOpenTry{ PortId: portID, - PreviousChannelId: previousChannelID, Channel: channel, CounterpartyVersion: counterpartyVersion, ProofInit: proofInit, @@ -89,9 +88,7 @@ func (msg MsgChannelOpenTry) ValidateBasic() error { return sdkerrors.Wrap(err, "invalid port ID") } if msg.PreviousChannelId != "" { - if !IsValidChannelID(msg.PreviousChannelId) { - return sdkerrors.Wrap(ErrInvalidChannelIdentifier, "invalid previous channel ID") - } + return sdkerrors.Wrap(ErrInvalidChannelIdentifier, "previous channel identifier must be empty, this field has been deprecated as crossing hellos are no longer supported") } if len(msg.ProofInit) == 0 { return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof init") diff --git a/modules/core/04-channel/types/msgs_test.go b/modules/core/04-channel/types/msgs_test.go index 8beed13d3ef..0c23bce06e5 100644 --- a/modules/core/04-channel/types/msgs_test.go +++ b/modules/core/04-channel/types/msgs_test.go @@ -153,25 +153,23 @@ func (suite *TypesTestSuite) TestMsgChannelOpenTryValidateBasic() { msg *types.MsgChannelOpenTry expPass bool }{ - {"", types.NewMsgChannelOpenTry(portid, chanid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), true}, - {"too short port id", types.NewMsgChannelOpenTry(invalidShortPort, chanid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"too long port id", types.NewMsgChannelOpenTry(invalidLongPort, chanid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"port id contains non-alpha", types.NewMsgChannelOpenTry(invalidPort, chanid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"too short channel id", types.NewMsgChannelOpenTry(portid, invalidShortChannel, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"too long channel id", types.NewMsgChannelOpenTry(portid, invalidLongChannel, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"channel id contains non-alpha", types.NewMsgChannelOpenTry(portid, invalidChannel, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"", types.NewMsgChannelOpenTry(portid, chanid, version, types.ORDERED, connHops, cpportid, cpchanid, "", suite.proof, height, addr), true}, - {"proof height is zero", types.NewMsgChannelOpenTry(portid, chanid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, clienttypes.ZeroHeight(), addr), false}, - {"invalid channel order", types.NewMsgChannelOpenTry(portid, chanid, version, types.Order(4), connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"connection hops more than 1 ", types.NewMsgChannelOpenTry(portid, chanid, version, types.UNORDERED, invalidConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"too short connection id", types.NewMsgChannelOpenTry(portid, chanid, version, types.UNORDERED, invalidShortConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"too long connection id", types.NewMsgChannelOpenTry(portid, chanid, version, types.UNORDERED, invalidLongConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"connection id contains non-alpha", types.NewMsgChannelOpenTry(portid, chanid, version, types.UNORDERED, []string{invalidConnection}, cpportid, cpchanid, version, suite.proof, height, addr), false}, - {"", types.NewMsgChannelOpenTry(portid, chanid, "", types.UNORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), true}, - {"invalid counterparty port id", types.NewMsgChannelOpenTry(portid, chanid, version, types.UNORDERED, connHops, invalidPort, cpchanid, version, suite.proof, height, addr), false}, - {"invalid counterparty channel id", types.NewMsgChannelOpenTry(portid, chanid, version, types.UNORDERED, connHops, cpportid, invalidChannel, version, suite.proof, height, addr), false}, - {"empty proof", types.NewMsgChannelOpenTry(portid, chanid, version, types.UNORDERED, connHops, cpportid, cpchanid, version, emptyProof, height, addr), false}, - {"channel not in TRYOPEN state", &types.MsgChannelOpenTry{portid, chanid, initChannel, version, suite.proof, height, addr}, false}, + {"", types.NewMsgChannelOpenTry(portid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), true}, + {"too short port id", types.NewMsgChannelOpenTry(invalidShortPort, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"too long port id", types.NewMsgChannelOpenTry(invalidLongPort, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"port id contains non-alpha", types.NewMsgChannelOpenTry(invalidPort, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"", types.NewMsgChannelOpenTry(portid, version, types.ORDERED, connHops, cpportid, cpchanid, "", suite.proof, height, addr), true}, + {"proof height is zero", types.NewMsgChannelOpenTry(portid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, clienttypes.ZeroHeight(), addr), false}, + {"invalid channel order", types.NewMsgChannelOpenTry(portid, version, types.Order(4), connHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"connection hops more than 1 ", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, invalidConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"too short connection id", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, invalidShortConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"too long connection id", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, invalidLongConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"connection id contains non-alpha", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, []string{invalidConnection}, cpportid, cpchanid, version, suite.proof, height, addr), false}, + {"", types.NewMsgChannelOpenTry(portid, "", types.UNORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), true}, + {"invalid counterparty port id", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, connHops, invalidPort, cpchanid, version, suite.proof, height, addr), false}, + {"invalid counterparty channel id", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, connHops, cpportid, invalidChannel, version, suite.proof, height, addr), false}, + {"empty proof", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, connHops, cpportid, cpchanid, version, emptyProof, height, addr), false}, + {"channel not in TRYOPEN state", &types.MsgChannelOpenTry{portid, "", initChannel, version, suite.proof, height, addr}, false}, + {"previous channel id is not empty", &types.MsgChannelOpenTry{portid, chanid, initChannel, version, suite.proof, height, addr}, false}, } for _, tc := range testCases { diff --git a/testing/endpoint.go b/testing/endpoint.go index 67e1b23b7ec..b2ad0c6d711 100644 --- a/testing/endpoint.go +++ b/testing/endpoint.go @@ -295,7 +295,7 @@ func (endpoint *Endpoint) ChanOpenTry() error { proof, height := endpoint.Counterparty.Chain.QueryProof(channelKey) msg := channeltypes.NewMsgChannelOpenTry( - endpoint.ChannelConfig.PortID, "", // does not support handshake continuation + endpoint.ChannelConfig.PortID, endpoint.ChannelConfig.Version, endpoint.ChannelConfig.Order, []string{endpoint.ConnectionID}, endpoint.Counterparty.ChannelConfig.PortID, endpoint.Counterparty.ChannelID, endpoint.Counterparty.ChannelConfig.Version, proof, height,