Improve handshake callbacks #629
Conversation
|
should this pr update the ICS apps as well (ICS20 and ICS27)? Or will that be done separately? |
|
middleware spec probably needs to be updated as well |
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <crodveg@yahoo.es>
| // generate a new identifier if the provided identifier was the sentinel empty-string | ||
| channelIdentifier = generateIdentifier() | ||
| } | ||
| channelIdentifier = generateIdentifier() |
There was a problem hiding this comment.
Including the removal of crossing hellos in this pr is fine, but I'd prefer it to be done in a separate pr. I think mashing together changes which aren't necessarily associated is bad practice. It is easier to understand what changes are made and the justification for those changes if they are logically isolated in the commits pushed to the master branch. Each IBC implementor would (hopefully) update their implementations with these changes and we should make it as easy as possible for them to do so. If there isn't already, there should be some sort of CHANGELOG which can inform implementors of changes they should make
Ideally we would have 3 pr's:
- removing crossing hellos from connection
- removing crossing hellos from channel
- improvement of version negotiation in channel handshake
As an implementor, this is how I would breakup the issues/pr's from the proposed changes
As a side note, I'd love to see the justification for removing crossing hellos logged somewhere, otherwise there will be discussion of why crossing hellos was removed in the future
| } else { | ||
| // generate a new identifier if the provided identifier was the sentinel empty-string | ||
| channelIdentifier = generateIdentifier() | ||
| } |
There was a problem hiding this comment.
You need to remove previous == null checks
… aditya/improve-handshake
|
I understand what you're saying but we also have to balance the fact that the spec repo moves much much slower than implementation (we are actively working to fix this!). But in the interim, I think it makes sense to respond to things that are semantically linked in the same PR to avoid long response times. I'm planning to merge this PR as it currently is, and then write the following PRs:
That being said. I agree strongly with your suggestion for a CHANGELOG. Since we don't have one at the moment, I'll open a separate PR to write the changelog for changes since 1.0. And then future PRs will include the changelog entry before merge |
Closes: #628