Skip to content
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

[ICS02] Allow zero Height creation, but keep the H>0 condition for CometBFT clients #603

Open
Farhad-Shabani opened this issue Apr 4, 2023 · 1 comment
Assignees
Labels
S: errors Scope: related to error handlings S: specs Scope: related to IBC protocol specifications

Comments

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Apr 4, 2023

Problem Statement

  • Client-specific condition
    As for the new Height creation (here) it isn’t allowed to create a zero height and this height >0 is a client-specific condition. Looking into the links provided here It is required for CometBFT-based chains)

  • Not in conflict with ICS02
    This condition is not clearly outlined in ICS07, though should be added there. But ICS02 explicitly requires to have a zero element for the Height type which is not in conflict with having H>0 for the CometBFT clients:

    There must also be a zero-element for a height type, referred to as 0, which is less than all non-zero heights.

  • Error handling side effect
    Upon converting a proto zero height, we have to deal with the zero-height error variant (ClientError::InvalidHeight) which is only needed for ICS07-related codes. And, in the places we have to observe H>0, this error is mapped to a different type and even left unwrap() like e.g. here & here & here & here

  • Benefit from Option<Height>
    Living in Rust, using Option and converting to None would be more efficient, where a proto zero height means the absence of height, while still allowing zero height creation. (As also suggested in Packet.timeoutHeight should have type Option<Height> (or equivalent) ibc#776)

Related Context

Acceptance Criteria

Allow zero Height creation, but keep the H>0 condition for CometBFT clients (be efficient to handle that only where we really care about not having a zero height.)

@Farhad-Shabani Farhad-Shabani added S: errors Scope: related to error handlings S: specs Scope: related to IBC protocol specifications labels Apr 4, 2023
@Farhad-Shabani Farhad-Shabani moved this to 📥 To Do in ibc-rs Apr 4, 2023
mina86 added a commit to mina86/ibc-rs that referenced this issue Nov 16, 2023
Replace ClientConsensusStatePath’s epoch and height fields with
a single height field whose type is Height.  This stores the exact
same information but allows infallible way to get Height out of the
path.

Arguably it also makes more sense conceptually since the epoch and
height are really just a single Height field.  Even in formatted paths
those two are a single component so it makes sense to have them as
a single field.

Issue: cosmos#603
@mina86
Copy link
Contributor

mina86 commented Nov 16, 2023

Related problem is that there’s no infallible way to create Height
from ClientConsensusStatePath since the latter stores height as two
u64s. I’ve looked into changing ClientConsensusStatePath definition
to be:

pub struct ClientConsensusStatePath {
    pub client_id: ClientId,
    pub height: Height,
}

This can be done without affecting serde serialisation (so that
wouldn’t be an issue) though of course it would be API-breaking
change.

PR with that change at #968

mina86 added a commit to mina86/ibc-rs that referenced this issue Nov 16, 2023
Replace ClientConsensusStatePath’s epoch and height fields with
a single height field whose type is Height.  This stores the exact
same information but allows infallible way to get Height out of the
path.

Arguably it also makes more sense conceptually since the epoch and
height are really just a single Height field.  Even in formatted paths
those two are a single component so it makes sense to have them as
a single field.

Issue: cosmos#603
mina86 added a commit to mina86/ibc-rs that referenced this issue Nov 16, 2023
Replace ClientConsensusStatePath’s epoch and height fields with
a single height field whose type is Height.  This stores the exact
same information but allows infallible way to get Height out of the
path.

Arguably it also makes more sense conceptually since the epoch and
height are really just a single Height field.  Even in formatted paths
those two are a single component so it makes sense to have them as
a single field.

Issue: cosmos#603
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: errors Scope: related to error handlings S: specs Scope: related to IBC protocol specifications
Projects
Status: 📥 To Do
Development

No branches or pull requests

3 participants