Refactor and fix version validation in connection and channel handshakes #626
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
+ Coverage 72.97% 73.07% +0.09%
==========================================
Files 126 126
Lines 15684 15678 -6
==========================================
+ Hits 11446 11456 +10
+ Misses 4238 4222 -16
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| if self.versions.len() != 1 { | ||
| return Err(ConnectionError::InvalidVersionLength); | ||
| } |
There was a problem hiding this comment.
I don't think ibc-go enforces this, right? If not, neither should we
There was a problem hiding this comment.
-
The way a connection is created, neither us nor IBC-go will end up with a connection supporting multiple versions (
Vec<Version>). Sinceget_compatible_versionsreturns a vector of size 1, and thepick_versionalso matches a single version. -
Though, already the
get_compatible_versionsAPI allows hosts to introduce multiple versions. Then, storing aConnectionEndon host with different versions is possible (bc of here), which is not good looking at how a channel is initialized. TheMsgChannelOpenTry::validate()method doesn't know which connection version should be checked for ordering.
I kept this condition as we are not supposed to see multiples upon calling versions. We had this before as InvalidVersionLengthConnection error. Perhaps, we should drop it after figuring out how to manage various versions or not allowing users for the Vec<Version> in get_compatible_versions. WDYT?
There was a problem hiding this comment.
I looked into it a bit more. The problem is that versions has different semantics based on which handshake step it is in:
- after INIT:
versionsholds the compatible versions- This was given by the relayer, or a set of default values
- after TRY,ACK,CONFIRM:
versionsholds THE version (exactly one) that was chosen by the handshake
Therefore, to accomodate both cases, we currently store a Vec<Version>. My takeaways:
- There's probably a better way to model this using an
enumwhich differentates between the 2 cases. - For now though, we should update the check to:
if !matches!(self.state, State::Init) && self.versions.len() != 1(and update the comment because this is hard to read)
There was a problem hiding this comment.
But then the invariant is enforced at "read time", which is not very elegant. A better way is to enforce the invariant in new(); when we create the object, we ensure that it is valid, and then we don't have to check this everytime we read (and add a ?)
There was a problem hiding this comment.
See if this works? 63c50a3
plafer
changed the title
Version validationVersion validation in connection and channel handshakes
plafer
changed the title
Version validation in connection and channel handshakes
|
Note: made minor name changes, see if you like aa91e71 |
|
Could you also update basecoin to run the integration tests on this branch? |
| pub fn state_matches(&self, expected: &State) -> bool { | ||
| self == expected | ||
| } |
There was a problem hiding this comment.
Just noticed this; this is a reimplementation of the standard PartialEq::eq. 3fa02e6
There was a problem hiding this comment.
👌
Let's wait for the integration tests to finish and then we can merge informalsystems/basecoin-rs#95
…kes (#626) * Streamline codebase around Version validation * Fix versions() check * Add a docstring for versions() check * Simplify version check in conn_open_ack * Remove unnecessary for * Resolve a minor issue * Further pruning * favor slices to `Vec` * Mend feature checks * Fix clippy * Allow empty version for on_chan_open_init_validate * Fix pick() test * Modify pick_version * Edit changelog description * name changes * Move version length check into the new ConnectionEnd constructor * Revert Versions signature * docstring * remove redundant function --------- Co-authored-by: Philippe Laferrière <plafer@protonmail.com>
Closes: #625
(As a side note, this PR streamlined also a few errors, showing that some of our error confusion comes from how certain logic is implemented)
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.