Add Height domain type #304
Conversation
Codecov Report
@@ Coverage Diff @@
## master informalsystems/hermes#304 +/- ##
=========================================
+ Coverage 13.6% 37.6% +23.9%
=========================================
Files 69 122 +53
Lines 3752 7978 +4226
Branches 1374 2861 +1487
=========================================
+ Hits 513 3000 +2487
- Misses 2618 4667 +2049
+ Partials 621 311 -310
Continue to review full report at Codecov.
|
|
|
||
| /// is_epoch_format() checks if a chain_id is in the format required for parsing epochs | ||
| /// The chainID must be in the form: `{chainID}-{version} | ||
| fn is_epoch_format(chain_id: String) -> bool { |
There was a problem hiding this comment.
Is this specified somewhere (in an ICS)? I did not come across it so far.
There was a problem hiding this comment.
Then is this requirement we invented ourselves? In any case, if this is a requirement on the name of a chain, it probably should be captured and standardized in cosmos/ics.
Perhaps @cwgoes knows more?
There was a problem hiding this comment.
We do the same handling as in cosmos-SDK but I agree it should be standardized.
There was a problem hiding this comment.
This is a requirement we initially came up with when implementing epochs for the tendermint light client on the SDK's IBC implementation. The issue is we need the epoch number to be included in the signed header. The solution is to put it in the chainID, but this requires some standardized parsing format so we can obtain the epoch number from the chainID.
We also renamed epoch to version as per a suggestion
There was a problem hiding this comment.
Aye. Admittedly this is a bit of a hack, but it's a compromise to make future (safe) upgrades easier.
We can add these details to the ICS standards as well.
- Remove unused error kind - Remove outdated comment - Use crate::Height everywhere - Use RawHeight alias
|
Let's merge this after #292. |
modules/src/ics04_channel/msgs.rs
Outdated
| @@ -468,7 +468,7 @@ impl MsgTimeout { | |||
| packet: Packet, | |||
| next_sequence_recv: Option<u64>, | |||
| proof: CommitmentProof, | |||
| proof_height: Height, | |||
| proof_height: RawHeight, | |||
There was a problem hiding this comment.
Minor comment: it's a bit odd to see raw type side by side with domain type. I guess we could have just as well used crate::Height instead of ibc_proto::ibc::core::client::v1::Height as RawHeight; unless I'm missing something.
We can fix this in a later iteration, and perhaps also split the modules/src/ics04_channel/msgs.rs file like we did in ICS3.
There was a problem hiding this comment.
Agreed, let’s use crate::Height instead.
* WIP - add Height domain type and make the code compile * Change crate::Height to point to the domain type * Cleanup unused variables * Implement PartialOrd and Ord for Height * Address Adi's comments - Remove unused error kind - Remove outdated comment - Use crate::Height everywhere - Use RawHeight alias * Cleaned up new() for ICS4 messages and Proof to eliminate raw type Co-authored-by: Romain Ruetschi <[email protected]> Co-authored-by: Adi Seredinschi <[email protected]>
Closes: cosmos/ibc-rs#110
Closes: https://github.com/informalsystems/ibc-rs/issues/191
For contributor use:
docs/) and code commentsFiles changedin the Github PR explorer