Packet.timeoutHeight should have type Option<Height> (or equivalent)

[plafer]

Packet.timeoutHeight treats the minimum height 0 specially: a height of 0 means "no timeout". This is taken from this check in timeoutPacket():

abortTransactionUnless(
      (packet.timeoutHeight > 0 && proofHeight >= packet.timeoutHeight) ||
      (packet.timeoutTimestamp > 0 && connection.getTimestampAtHeight(proofHeight) > packet.timeoutTimestamp))

This is inconsistent with the ICS-2 definition of Height, which specifies that 0 is a valid height. It would be cleaner to represent this special case directly in the type: anything equivalent to Rust's Option<Height> (or Option<NonZeroHeight> depending on if we want to allow a timeout at height 0 or not).

[AdityaSripal]

Hmm, but even if height 0 is valid it would still be sensible to error if it was passed as the timeoutHeight correct? Since -1 is not a valid height, there could be no sensible timeout height that was the lowest valid height possible.

It seems like we could change the implementation to reflect this. Though it is a very minor improvement

[plafer]

Hmm, but even if height 0 is valid it would still be sensible to error if it was passed as the timeoutHeight correct?

I would personally wouldn't error on height 0, and instead just follow the expected semantics of "this packet is always timed out", even though not a very useful packet. This isn't too different from a timeoutHeight of 1 or 2 (which similarly pretty much is always timed out).

Since -1 is not a valid height, there could be no sensible timeout height that was the lowest valid height possible.

I'm not sure I understand what you mean here.

To rephrase my suggestion, timeoutHeight == None would encode "no timeout", and timeoutHeight == Some(height) would encode a timeout, with all valid Height values (including 0) possible for height.

It seems like we could change the implementation to reflect this. Though it is a very minor improvement

I consider it relatively important, as currently timeout heights don't work like all other heights. In the Rust implementation, we had to create a new type TimeoutHeight with special encoding/decoding code to capture the difference.

[AdityaSripal]

How would this work with the current ibc-go implementation where the type is a uint64 at the moment?

[plafer]

Indeed it would be a breaking change to fix this. I opted to flag it regardless, in case there ever is a backward incompatible v2 of the protocol.