Flexible Channel Handshake Selection #7439

colin-axner · 2020-10-02T10:01:34Z

Description

closes: #7025

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

Add ProvedID to ChanOpenTry which allows for flexible handshake identifier selection. Add CounterpartyChannelID to OpenAck. Update and add to handshake tests. Modify msgs_test. Counterparty ValidateBasic will return nil on an empty channel identifier to allow for flexible handshake identifier selection to succeed.

codecov · 2020-10-02T10:06:36Z

Codecov Report

Merging #7439 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7439      +/-   ##
==========================================
+ Coverage   55.09%   55.11%   +0.02%     
==========================================
  Files         588      588              
  Lines       36782    36800      +18     
==========================================
+ Hits        20266    20284      +18     
  Misses      14415    14415              
  Partials     2101     2101

x/ibc/core/04-channel/keeper/handshake.go

…osmos/cosmos-sdk into colin/7025-channel-handshake-selection

fedekunze

ACK, minor comment

fedekunze · 2020-10-02T10:13:33Z

x/ibc/core/04-channel/keeper/handshake.go

@@ -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 != "" {


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 if object && object.field... to check for undefined objects.

Suggested change

if provedChannelID != channelID && provedChannelID != "" {

if provedChannelID != "" && provedChannelID != channelID {

yea I agree which is why I did this in msgs.go. I left how the spec ordered it in handshake for consistency. This is also how it is in connection handshake so I'd prefer to update this in a separate pr since it isn't api breaking

cc/ @cwgoes should we apply this to the spec? I think it reads slightly better

Applied to the spec in cosmos/ibc@890e3d0

fedekunze · 2020-10-02T10:13:59Z

x/ibc/core/04-channel/keeper/handshake.go

@@ -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 != "" {


ditto

Suggested change

if counterpartyChannelID != channel.Counterparty.ChannelId && channel.Counterparty.ChannelId != "" {

if channel.Counterparty.ChannelId != "" && counterpartyChannelID != channel.Counterparty.ChannelId {

Also applied to the spec in cosmos/ibc@890e3d0

fedekunze · 2020-10-02T10:17:31Z

x/ibc/core/04-channel/types/msgs.go

@@ -164,6 +169,9 @@ func (msg MsgChannelOpenAck) ValidateBasic() error {
 	if err := host.ChannelIdentifierValidator(msg.ChannelId); err != nil {
 		return sdkerrors.Wrap(err, "invalid channel ID")
 	}
+	if err := host.ChannelIdentifierValidator(msg.CounterpartyChannelId); err != nil {


shouldn't this be

if msg.CounterpartyChannelId != "" { if err := host.ChannelIdentifierValidator(msg.CounterpartyChannelId); err != nil {

no, CounterpartyChannelId must be set. The != "" check in the handshake is against the stored counterparty channel id not the provided one

By the time Ack rolls around, a connection ID should have been selected. The "" stored string in the handshake just indicates that the selection was flexible and the counterparty channel id should be used as the counterparty's channel id

colin-axner · 2020-10-02T10:31:16Z

@fedekunze could you run make proto after this merges and before a rc is cut. My proto tools are still broken so there might be some diffs

fedekunze · 2020-10-02T10:53:00Z

@fedekunze could you run make proto after this merges and before a rc is cut. My proto tools are still broken so there might be some diffs

Just checked and there wasn't any diff file

AdityaSripal

LGTM, but one thing I don't understand is why not just always have flexible ID selection. It would be simpler to always have chains choosing their own identifiers.

Is there ever a reason why they shouldn't? cc: @cwgoes

AdityaSripal · 2020-10-02T14:31:44Z

x/ibc/core/04-channel/keeper/handshake.go

+	channelID,
+	provedChannelID string,


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 => desiredChannelID and provedChannelID => counterpartyChosenChannelID

cc: @cwgoes

agree on the confusion. Why not just desiredChannelID and chosenChannelID? 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

// empty chosen channel id allows the desired channel id to be used for identifier selection if chosenChannelID != "" && chosenChannelID != desiredChannelID { return err }

I'm fine with this renaming, I can update the spec as well

upon further reflection I think counterpartyChosenChannelID works fine since in most cases the Counterparty will be a different field containing the counterparty.ChannelId

rename provedID to counterpartyChosenID for connection and channel. Update if statement in handshake and tests. Ref: #7439 (comment)

* rename provedID rename provedID to counterpartyChosenID for connection and channel. Update if statement in handshake and tests. Ref: #7439 (comment) * update docs Co-authored-by: Christopher Goes <[email protected]>

* rename provedID rename provedID to counterpartyChosenID for connection and channel. Update if statement in handshake and tests. Ref: cosmos/cosmos-sdk#7439 (comment) * update docs Co-authored-by: Christopher Goes <[email protected]>

colin-axner requested review from aaronc, alessio and alexanderbez as code owners October 2, 2020 10:01

colin-axner changed the title ~~update channel handshake~~ Flexible Channel Handshake Selection Oct 2, 2020

colin-axner requested review from fedekunze, cwgoes and AdityaSripal October 2, 2020 10:02

colin-axner commented Oct 2, 2020

View reviewed changes

x/ibc/core/04-channel/keeper/handshake.go Outdated Show resolved Hide resolved

colin-axner and others added 3 commits October 2, 2020 12:06

Update x/ibc/core/04-channel/keeper/handshake.go

bb75426

add conn open init docs

31e9da1

Merge branch 'colin/7025-channel-handshake-selection' of github.com:c…

d1dfab5

…osmos/cosmos-sdk into colin/7025-channel-handshake-selection

fedekunze approved these changes Oct 2, 2020

View reviewed changes

fedekunze reviewed Oct 2, 2020

View reviewed changes

update docs for conn open try and conn open ack

6b40439

update channel docs

dc313ef

AdityaSripal approved these changes Oct 2, 2020

View reviewed changes

Merge branch 'master' into colin/7025-channel-handshake-selection

c2ec1d3

fedekunze added the x/ibc label Oct 2, 2020

fedekunze merged commit 0047099 into master Oct 2, 2020

fedekunze deleted the colin/7025-channel-handshake-selection branch October 2, 2020 19:17

colin-axner added a commit that referenced this pull request Oct 5, 2020

rename provedID

2fde5f2

rename provedID to counterpartyChosenID for connection and channel. Update if statement in handshake and tests. Ref: #7439 (comment)

colin-axner mentioned this pull request Oct 5, 2020

Flexible handshake followup #7454

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flexible Channel Handshake Selection #7439

Flexible Channel Handshake Selection #7439

colin-axner commented Oct 2, 2020 •

edited

Loading

codecov bot commented Oct 2, 2020 •

edited

Loading

fedekunze left a comment

fedekunze Oct 2, 2020

colin-axner Oct 2, 2020

colin-axner Oct 2, 2020

cwgoes Oct 5, 2020

fedekunze Oct 2, 2020

cwgoes Oct 5, 2020

fedekunze Oct 2, 2020

colin-axner Oct 2, 2020

colin-axner Oct 2, 2020

colin-axner commented Oct 2, 2020 •

edited

Loading

fedekunze commented Oct 2, 2020 •

edited

Loading

AdityaSripal left a comment

AdityaSripal Oct 2, 2020

colin-axner Oct 2, 2020

cwgoes Oct 2, 2020

colin-axner Oct 5, 2020

cwgoes Oct 5, 2020

	if provedChannelID != channelID && provedChannelID != "" {
	if provedChannelID != "" && provedChannelID != channelID {

	if counterpartyChannelID != channel.Counterparty.ChannelId && channel.Counterparty.ChannelId != "" {
	if channel.Counterparty.ChannelId != "" && counterpartyChannelID != channel.Counterparty.ChannelId {

Flexible Channel Handshake Selection #7439

Flexible Channel Handshake Selection #7439

Conversation

colin-axner commented Oct 2, 2020 • edited Loading

Description

codecov bot commented Oct 2, 2020 • edited Loading

Codecov Report

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Oct 2, 2020 • edited Loading

fedekunze commented Oct 2, 2020 • edited Loading

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Oct 2, 2020 •

edited

Loading

codecov bot commented Oct 2, 2020 •

edited

Loading

colin-axner commented Oct 2, 2020 •

edited

Loading

fedekunze commented Oct 2, 2020 •

edited

Loading