connection handshake updates for versioning #7328
Conversation
colin-axner
requested review from
aaronc,
alessio and
alexanderbez
as code owners
Codecov Report
@@ Coverage Diff @@
## master #7328 +/- ##
==========================================
- Coverage 55.86% 52.79% -3.08%
==========================================
Files 585 436 -149
Lines 35898 26431 -9467
==========================================
- Hits 20054 13953 -6101
+ Misses 13878 11124 -2754
+ Partials 1966 1354 -612 |
Add a test for supplied versions that do not function with previous connection states in INIT and TRYOPEN
…cosmos/cosmos-sdk into colin/7025-connection-handshake-updates
There was a problem hiding this comment.
ACK, this logic looks correct to me per https://github.com/cosmos/ics/pull/479/files
colin-axner
added
the
A:automerge
There was a problem hiding this comment.
Some small nits.
Also it looks like this part of the spec change wasn't implemeneted:
https://github.com/cosmos/ics/pull/479/files#diff-4bb4959de91d075b70b4378c9000257cR323-R324
Is that going to be done in a separate PR?
| @@ -120,7 +121,7 @@ func (k Keeper) ConnOpenTry( | |||
| bytes.Equal(previousConnection.Counterparty.Prefix.Bytes(), counterparty.Prefix.Bytes()) && | |||
| previousConnection.ClientId == clientID && | |||
| previousConnection.Counterparty.ClientId == counterparty.ClientId && | |||
| previousConnection.Versions[0] == version) { | |||
| reflect.DeepEqual(previousConnection.Versions, types.GetCompatibleEncodedVersions())) { | |||
There was a problem hiding this comment.
docstring above needs to be changed.
+ // Check that existing connection versions for initialized connection is equal to compatible versions for this chain
| @@ -120,7 +121,7 @@ func (k Keeper) ConnOpenTry( | |||
| bytes.Equal(previousConnection.Counterparty.Prefix.Bytes(), counterparty.Prefix.Bytes()) && | |||
There was a problem hiding this comment.
nit: I think the previousConnection check should happen way earlier, before the version gets picked and current connection is created.
There was a problem hiding this comment.
I agree but this follows the ICS spec ordering so I think it makes sense to leave it where it is so its easier to verify correctness of the implementation against the spec. I'd suggest reordering this on the spec before reordering this in the code. Code readability for handshake calls includes readability against the spec
Per discussion with @colin-axner, my understanding is that the SDK already takes the intersection of compatible versions - is that accurate? If so, I don't think anything else needs to be done (the spec previously just didn't specify an intersection). |
| if !((connection.State == types.INIT && types.IsSupportedVersion(encodedVersion)) || | ||
| (connection.State == types.TRYOPEN && len(connection.Versions) == 1 && connection.Versions[0] == encodedVersion)) { |
There was a problem hiding this comment.
nit, this is hard to read. Can we assign a variable name for each case (init and tryopen)?
There was a problem hiding this comment.
I could alternatively do a swtich
switch {
case connection.State != INIT || connection.State != TRYOPEN:
case connection.State == INIT && !types.IsSupportedVersion(encodedVersion):
case connection.State == TRYOPEN && (len(connection.Versions) != 1 || connection.Version[0] != encodedVersion):
} There was a problem hiding this comment.
@fedekunze I update this to use a switch. I think it is more clear, let me know if any adjustments should be made
correct. I can add comments in the code to make this clear. |
Update the version checks in ConnOpenAck to use a switch to increase readability of the code as well as provide custom error messages for each possible case that may occur. Slighly modified version to review suggestion by @fedekunze
Description
Reflects changes in spec for handshakes. Is there any specific handshake tests I should add?
part 1 of: #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.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes