-
Notifications
You must be signed in to change notification settings - Fork 383
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
Add comments to handler for async handshake #1019
Conversation
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.
LGTM!
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.
A few minor copy-pasta typos.
@AdityaSripal, for async version negotiation in // Perform application logic callback
version, err := cbs.OnChanOpenTry(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.CounterpartyVersion)
if err != nil {
return nil, sdkerrors.Wrap(err, "channel open try callback failed")
}
// FIGME: If version is the empty string, expect the app to call WriteOpenTryChannel explicitly, possibly later...
if version != "" {
// Write channel into state
k.ChannelKeeper.WriteOpenTryChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, version)
}
return &channeltypes.MsgChannelOpenTryResponse{
Version: version,
}, nil This may also justify adding something like a |
Co-authored-by: Michael FIG <[email protected]>
// in this case, the channel identifier will be stored with a sentinel value so it is not taken | ||
// by a new channel handshake and the capability is reserved for the application module. |
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.
Is this actually needed?
@@ -447,6 +447,12 @@ function handleChanOpenInit(datagram: ChanOpenInit) { | |||
datagram.portIdentifier, | |||
datagram.counterpartyPortIdentifier | |||
) | |||
// SYNCHRONOUS: the following calls happen synchronously with the call above | |||
// ASYNCHRONOUS: the module callback will be called at a time later than the channel handler | |||
// in this case, the channel identifier will be stored with a sentinel value so it is not taken |
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.
With this you mean that a sentinel value is stored in the path channelEnds/ports/{datagram.portIdentifier}/channels/{channelIdentifier}
, right? Maybe it's more explicit to mention it like this?
@michaelfig yes. We are trying to improve the interface for middleware to avoid the complicated wiring as it exists. I think this will be much cleaner in future designs, and the ICS4 wrapper interface itself should be unnecessary |
Writeup in the comments of how to compose the utility functions provided by IBC to implement async handshakes
closes: #732
cc: @michaelfig