Flexible Channel Handshake Selection #7439
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -96,7 +96,8 @@ func (k Keeper) ChanOpenTry( | |||||
| order types.Order, | ||||||
| connectionHops []string, | ||||||
| portID, | ||||||
| channelID, | ||||||
| provedChannelID string, | ||||||
| portCap *capabilitytypes.Capability, | ||||||
| counterparty types.Counterparty, | ||||||
| version, | ||||||
|
|
@@ -147,6 +148,15 @@ func (k Keeper) ChanOpenTry( | |||||
| ) | ||||||
| } | ||||||
|
|
||||||
| // empty-string is a sentinel value for "allow any identifier" to be selected by | ||||||
| // the counterparty channel | ||||||
| if provedChannelID != channelID && provedChannelID != "" { | ||||||
|
There was a problem hiding this comment. nit, I know the result is the same but if we change the order the second clause won't need to be evaluated as the entire condition will return false. Usually, in JS you would use
Suggested change
There was a problem hiding this comment. yea I agree which is why I did this in There was a problem hiding this comment. cc/ @cwgoes should we apply this to the spec? I think it reads slightly better There was a problem hiding this comment. Applied to the spec in cosmos/ibc@890e3d0 |
||||||
| return nil, sdkerrors.Wrapf( | ||||||
| types.ErrInvalidChannelIdentifier, | ||||||
| "proved channel identifier (%s) must equal channel identifier (%s) or be empty", provedChannelID, channelID, | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| // NOTE: this step has been switched with the one below to reverse the connection | ||||||
| // hops | ||||||
| channel := types.NewChannel(types.TRYOPEN, order, counterparty, connectionHops, version) | ||||||
|
|
@@ -159,7 +169,7 @@ func (k Keeper) ChanOpenTry( | |||||
|
|
||||||
| // expectedCounterpaty is the counterparty of the counterparty's channel end | ||||||
| // (i.e self) | ||||||
| expectedCounterparty := types.NewCounterparty(portID, provedChannelID) | ||||||
| expectedChannel := types.NewChannel( | ||||||
| types.INIT, channel.Ordering, expectedCounterparty, | ||||||
| counterpartyHops, counterpartyVersion, | ||||||
|
|
@@ -194,7 +204,8 @@ func (k Keeper) ChanOpenAck( | |||||
| portID, | ||||||
| channelID string, | ||||||
| chanCap *capabilitytypes.Capability, | ||||||
| counterpartyVersion, | ||||||
| counterpartyChannelID string, | ||||||
| proofTry []byte, | ||||||
| proofHeight exported.Height, | ||||||
| ) error { | ||||||
|
|
@@ -214,6 +225,15 @@ func (k Keeper) ChanOpenAck( | |||||
| return sdkerrors.Wrapf(types.ErrChannelCapabilityNotFound, "caller does not own capability for channel, port ID (%s) channel ID (%s)", portID, channelID) | ||||||
| } | ||||||
|
|
||||||
| // empty-string is a sentinel value for "allow any identifier" to be selected by | ||||||
| // the counterparty channel | ||||||
| if counterpartyChannelID != channel.Counterparty.ChannelId && channel.Counterparty.ChannelId != "" { | ||||||
|
There was a problem hiding this comment. ditto
Suggested change
There was a problem hiding this comment. Also applied to the spec in cosmos/ibc@890e3d0 |
||||||
| return sdkerrors.Wrapf( | ||||||
| types.ErrInvalidChannelIdentifier, | ||||||
| "counterparty channel identifier (%s) must be empty or equal to stored channel ID for counterparty (%s)", counterpartyChannelID, channel.Counterparty.ChannelId, | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| connectionEnd, found := k.connectionKeeper.GetConnection(ctx, channel.ConnectionHops[0]) | ||||||
| if !found { | ||||||
| return sdkerrors.Wrap(connectiontypes.ErrConnectionNotFound, channel.ConnectionHops[0]) | ||||||
|
|
@@ -241,7 +261,7 @@ func (k Keeper) ChanOpenAck( | |||||
|
|
||||||
| if err := k.connectionKeeper.VerifyChannelState( | ||||||
| ctx, connectionEnd, proofHeight, proofTry, | ||||||
| channel.Counterparty.PortId, counterpartyChannelID, | ||||||
| expectedChannel, | ||||||
| ); err != nil { | ||||||
| return err | ||||||
|
|
@@ -251,6 +271,7 @@ func (k Keeper) ChanOpenAck( | |||||
|
|
||||||
| channel.State = types.OPEN | ||||||
| channel.Version = counterpartyVersion | ||||||
| channel.Counterparty.ChannelId = counterpartyChannelID | ||||||
| k.SetChannel(ctx, portID, channelID, channel) | ||||||
|
|
||||||
| return nil | ||||||
|
|
||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally this is very confusing for me to read. It's not immediately clear what these mean without poring through the spec+comments.
I would prefer
channelID => desiredChannelIDandprovedChannelID => counterpartyChosenChannelIDcc: @cwgoes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree on the confusion. Why not just
desiredChannelIDandchosenChannelID? Using counterparty can be vague to whether the counterparty chose the channel ID for this chain or if the counterparty chose their own.Then the logic would be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this renaming, I can update the spec as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upon further reflection I think
counterpartyChosenChannelIDworks fine since in most cases theCounterpartywill be a different field containing thecounterparty.ChannelIdThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK!