Option type should be used with non-zero Height

[soareschen]

Crate

ibc-rs

Summary

Currently the Height type uses a special Height::zero() value to denote the absence of height value. This causes ad hoc checks of whether a height is zero in various places. We should instead use Option<Height> on places where the height value may be optional, and require non-zero value in the Height constructor.

Acceptance Criteria

• Optional height parameters should use Option<Height> instead of Height.
• Checking of height == Height::zero() is replaced with Option matching.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[plafer]

My only potential concern with this solution is that our Height type would no longer be a partially ordered set due to the exclusion of 0, which is required in ICS 2. Here, I'm assuming that None serves as 0, and Option<Height> would correspond to Height in the spec, assuming a proper implementation for cmp().

However, looking at the code, the zero height is often used as a special value, sometimes even explicitly mentioned in comments. This is confusing to me as I don't see any mention in the spec that the zero height should be treated differently.

What am I missing here? I definitely agree that Option<Height> + pattern matching should be used for treating the special case. However, I'm concerned that we might be introducing a bug w.r.t a "real" height == 0 state, since I don't understand how the zero height is used in the implementation, and how that relates back to the spec.

[hu55a1n1]

Note that Height::zero() can also mean latest height, especially in the context of Tendermint RPC -> https://docs.tendermint.com/master/rpc/#/Info/block

[romac]

We should perhaps encode this across the whole codebase as

enum Height {
  Latest,
  Specific(u64)
}

and then only convert Height::Latest to 0 or None on a case-by-case basis, depending on what the underlying API expects to mean "latest height".

[plafer]

It seems like a bunch of events use the zero height as well:

1. IbcEvent::OpenInitChannel, IbcEvent::OpenTryChannel, etc. use the zero height implicitly by being constructed with default values for all fields but the channel id.
2. IbcEvent::ReceivePacket is constructed with Height::zero() explicitly, which @adizere suggested should be replaced with current chain height.

It seems to me that these should all use chain height from ChannelReader::host_height(), right?

[romac]

It seems to me that these should all use chain height from ChannelReader::host_height(), right?

Yep, at least that's my understanding. @hu55a1n1 what do you think?

Moreover, we should stop building events via the Attributes struct and its Default instance, but rather

a) define explicitly what data each event carry in the event struct directly
b) use a constructor to build the event

We are already doing (a) for the channel events but eg. not for the connection events.

[soareschen]

For context, I tried to solve this by implementing a NonZeroHeight type here in #1011.

The plan was to split the refactoring in multiple phases:

1. All places that originally used Height do not need to be edited to become Option<Height>.
2. We can incrementally refactor functions that accept Height to instead use NonZeroHeight.
3. After all use of Height are eliminated, we can remove Height and then rename NonZeroHeight to Height.

[hu55a1n1]

Yep, at least that's my understanding. @hu55a1n1 what do you think?

It is not clear to me what the event height means from the modules' perspective, because it seems like ibc-go doesn't include a height attribute in the events that it emits. From what I can see, the height is only used by the relayer code to determine the height of the block which included the tx that emitted this event. So I am not sure if we want to keep the height as part of the IbcEvent itself. Furthermore, the event height is only set by the relayer code that deals with events. We also have Tendermint RPC event variants in the IbcEvent enum and maybe we should separate them from core IBC events in the future. (Related #1288 & #1909)

Moreover, we should stop building events via the Attributes struct and its Default instance, but rather
a) define explicitly what data each event carry in the event struct directly
b) use a constructor to build the event

💯

[plafer]

Yep, at least that's my understanding. @hu55a1n1 what do you think?

It is not clear to me what the event height means from the modules' perspective, because it seems like ibc-go doesn't include a height attribute in the events that it emits. From what I can see, the height is only used by the relayer code to determine the height of the block which included the tx that emitted this event. So I am not sure if we want to keep the height as part of the IbcEvent itself. Furthermore, the event height is only set by the relayer code that deals with events. We also have Tendermint RPC event variants in the IbcEvent enum and maybe we should separate them from core IBC events in the future. (Related #1288 & #1909)

Moreover, we should stop building events via the Attributes struct and its Default instance, but rather
a) define explicitly what data each event carry in the event struct directly
b) use a constructor to build the event

💯

Discussion relevant to #2043

[plafer]

FYI I'll wait on #2044 to be merged before I start working on this one to avoid merge conflicts.