-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flexible Channel Handshake Selection #7439
Changes from 1 commit
48f8f60
bb75426
31e9da1
d1dfab5
6b40439
dc313ef
c2ec1d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 string, | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea I agree which is why I did this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc/ @cwgoes should we apply this to the spec? I think it reads slightly better There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied to the spec in cosmos/ibc@890e3d0 |
||||||
return nil, sdkerrors.Wrapf( | ||||||
types.ErrInvalidChannelIdentifier, | ||||||
"proved channelidentifier (%s) must equal channel identifier (%s) or be empty", provedChannelID, channelID, | ||||||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
) | ||||||
} | ||||||
|
||||||
// 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, channelID) | ||||||
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 string, | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, channel.Counterparty.ChannelId, | ||||||
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 | ||||||
|
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 => desiredChannelID
andprovedChannelID => counterpartyChosenChannelID
cc: @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
desiredChannelID
andchosenChannelID
? 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
counterpartyChosenChannelID
works fine since in most cases theCounterparty
will be a different field containing thecounterparty.ChannelId
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.
OK!