Continuation of removing crossing hellos in channel handshake #68

colin-axner · 2022-07-07T11:18:04Z

Description

This is a continuation of the work started in #1317. Some changes in the diffs come from updating to the latest state of the main branch on ibc-go

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

…e-channel-crossing-hellos

Deprecate previous channel id with proto tag Remove unnecessary test cases from 04-channel Remove crossing hello check in transfer application and the associated test case

AdityaSripal

Thanks @vuong177 and @colin-axner !!

crodriguezvega · 2022-07-07T11:37:34Z

docs/migrations/v3-to-v4.md

@@ -91,3 +99,5 @@ if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId,
 ## Relayers

 When using the `DenomTrace` gRPC, the full IBC denomination with the `ibc/` prefix may now be passed in.
+
+Crossing hello's are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).


Suggested change

Crossing hello's are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).

Crossing hellos are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).

chatton · 2022-07-07T11:36:06Z

CHANGELOG.md

@@ -62,7 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file.
 * (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1`
 * (app/29-fee) [\#1341](https://github.com/cosmos/ibc-go/pull/1341) Check if the fee module is locked and if the fee module is enabled before refunding all fees
-* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317)  Remove crossing hellos from ChanOpenTry
+* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove corssing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated.  


Suggested change

* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove corssing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` 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.

crodriguezvega · 2022-07-07T11:44:35Z

CHANGELOG.md

@@ -62,7 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file.
 * (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1`
 * (app/29-fee) [\#1341](https://github.com/cosmos/ibc-go/pull/1341) Check if the fee module is locked and if the fee module is enabled before refunding all fees
-* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317)  Remove crossing hellos from ChanOpenTry
+* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove corssing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated.  


Since this is an API breaking change, would it make more sense to add this to the API Breaking section? Just to give it more visibility... I know this is a bit of a problem at the moment with the structure of our changelog...

Sounds good!

damiannolan

Awesome! ❤️

damiannolan · 2022-07-07T11:56:06Z

modules/apps/transfer/ibc_module.go

-		if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
-			return "", err
-		}
+	// OpenTry must claim the channelCapability that IBC passes into the callback


mega nit: feel free to ignore

Suggested change

// OpenTry must claim the channelCapability that IBC passes into the callback

// OnChanOpenTry must claim the channel capability that IBC passes into the callback

damiannolan · 2022-07-07T11:57:41Z

modules/core/04-channel/keeper/handshake.go


 	channel := types.NewChannel(types.TRYOPEN, order, counterparty, connectionHops, version)

 	k.SetChannel(ctx, portID, channelID, channel)

-	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousChannel.State.String(), "new-state", "TRYOPEN")
+	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", "NONE", "new-state", "TRYOPEN")


Should we use UNINITIALIZED here as it's the default zero value enum for channel State?

Suggested change

k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", "NONE", "new-state", "TRYOPEN")

k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", "UNINITIALIZED", "new-state", "TRYOPEN")

damiannolan · 2022-07-07T11:58:30Z

proto/ibc/core/channel/v1/tx.proto

-  // the channel identifier of the previous channel in state INIT
-  // Deprecated: this field has been deprecated and will be ignored by core IBC because now we don't need to protect against crossing hellos anymore.
-  string previous_channel_id = 2 [(gogoproto.moretags) = "yaml:\"previous_channel_id\""];
+  // Deprecated: this field is unused. Crossing hello's are no longer supported in core IBC.


same nit as @crodriguezvega

Suggested change

// Deprecated: this field is unused. Crossing hello's are no longer supported in core IBC.

// Deprecated: this field is unused. Crossing hellos are no longer supported in core IBC.

chatton and others added 7 commits July 6, 2022 11:04

Script to dynamically generate list of e2e tests (E2E #1) (#1644)

527a11a

Merge branch 'main' of github.com:cosmos/ibc-go into colin/1311-remov…

af6bcec

…e-channel-crossing-hellos

apply remaining changes for crossing hello removal

40771a6

Deprecate previous channel id with proto tag Remove unnecessary test cases from 04-channel Remove crossing hello check in transfer application and the associated test case

remove previous channel check in WriteChannelOpenTry

c2eecab

regenerate proto files

4f7e88a

update documentation

9856d24

add migration documentation

a75219d

AdityaSripal approved these changes Jul 7, 2022

View reviewed changes

remove unnecessary doc

dbb9b0e

crodriguezvega reviewed Jul 7, 2022

View reviewed changes

chatton reviewed Jul 7, 2022

View reviewed changes

crodriguezvega reviewed Jul 7, 2022

View reviewed changes

colin-axner added 2 commits July 7, 2022 13:48

remove crossing hello notion from ChanOpenAck

c890576

apply review suggestions

86d61ed

vuong177 merged commit 4aa75e5 into notional-labs:remove-crossings-hello Jul 7, 2022

damiannolan approved these changes Jul 7, 2022

View reviewed changes

crodriguezvega mentioned this pull request Jul 11, 2022

Log handshakes as previous state UNINITIALIZED cosmos/ibc-go#1688

Closed

3 tasks

colin-axner deleted the colin/1311-remove-channel-crossing-hellos branch August 3, 2022 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continuation of removing crossing hellos in channel handshake #68

Continuation of removing crossing hellos in channel handshake #68

colin-axner commented Jul 7, 2022

AdityaSripal left a comment

crodriguezvega Jul 7, 2022

chatton Jul 7, 2022

crodriguezvega Jul 7, 2022

colin-axner Jul 7, 2022

damiannolan left a comment

damiannolan Jul 7, 2022

damiannolan Jul 7, 2022

damiannolan Jul 7, 2022

	Crossing hello's are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).
	Crossing hellos are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).

	* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove corssing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` 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.

	// OpenTry must claim the channelCapability that IBC passes into the callback
	// OnChanOpenTry must claim the channel capability that IBC passes into the callback

	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", "NONE", "new-state", "TRYOPEN")
	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", "UNINITIALIZED", "new-state", "TRYOPEN")

	// Deprecated: this field is unused. Crossing hello's are no longer supported in core IBC.
	// Deprecated: this field is unused. Crossing hellos are no longer supported in core IBC.

Continuation of removing crossing hellos in channel handshake #68

Continuation of removing crossing hellos in channel handshake #68

Conversation

colin-axner commented Jul 7, 2022

Description

AdityaSripal left a comment

Choose a reason for hiding this comment

crodriguezvega Jul 7, 2022

Choose a reason for hiding this comment

chatton Jul 7, 2022

Choose a reason for hiding this comment

crodriguezvega Jul 7, 2022

Choose a reason for hiding this comment

colin-axner Jul 7, 2022

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Jul 7, 2022

Choose a reason for hiding this comment

damiannolan Jul 7, 2022

Choose a reason for hiding this comment

damiannolan Jul 7, 2022

Choose a reason for hiding this comment