Remove the concept of a "zero Height" (informalsystems#2346)

Prior to this PR, the `Height` type used a special `Height::zero()` value to denote the absence of height value. This caused ad hoc checks of whether a height is zero in various places.

From now on, we instead use `Option<Height>` on places where the height value may be optional, and require non-zero value in the `Height` constructor.

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

---

* From<Height> for String fix

* Use Option<Height> instead of Height::zero for consensus_height

* recv_packet still had Height::zero() in events

* TryFrom<RawHeight> for Height

* pre height parse changes

* Packet.timeout_height is now an Option<Height>

* rustdoc

* Remove Height::with_revision_height

* commit pre-modifying cli

* Finish Height::new() returns Result

* remove Height::is_zero()

* Remove `Height::zero()`

* Remove Height::default()

* Height fields accessors

* use revision_height accessor

* use revision_number accessor

* make Height fields private (WIP)

* compile error fixes

* FIXME in transfer.rs addressed

* Use existing constants instead of hardcoded strings

* changelog

* TimeoutHeight newtype

* Use TimeoutHeight everywhere

* Fixes after merging with master

* has_expired() fix

* doc url

* fix send_packet test

* Remove timeout_height == 0 && timestamp == 0 check

* Remove `has_timeout()`

* Change TimeoutHeight impl

* docstrings

* tests fix

* fix history manipulation test

* transfer test: no timeout

* msgtransfer test

* packet no timeout or timestamp test

* send_packet timeout height equal destination chain height

* send_packet test with timeout = height - 1

* test invalid timeout height

* fix docstring

* clippy

* Fix clippy warnings introduced in Rust 1.62

* Fix serialization of `TimeoutHeight` to match the format used by `Height`

Co-authored-by: Romain Ruetschi <romain@informal.systems>

.changelog/unreleased/improvements/ibc/1009-height-zero-complete.md

@@ -0,0 +1,2 @@
+- Remove the concept of a zero Height
+  ([#1009](https://github.com/informalsystems/ibc-rs/issues/1009))

e2e/e2e/common.py

@@ -10,6 +10,12 @@ class Height:
     revision_number: int
 
 
+@dataclass
+class TimeoutHeight:
+    revision_height: int
+    revision_number: int
+
+
 @dataclass
 class Duration:
     nanos: int
@@ -39,6 +45,7 @@ class Ordering(Enum):
 ClientType = NewType('ClientType', str)
 BlockHeight = NewType('BlockHeight', str)
 
+
 def split():
     sleep(0.5)
     print()

e2e/e2e/packet.py

@@ -12,7 +12,7 @@ class Packet:
     destination_port: PortId
     destination_channel: ChannelId
     data: Hex
-    timeout_height: Height
+    timeout_height: TimeoutHeight
     timeout_timestamp: Timestamp
 
 

relayer-cli/src/commands/keys/list.rs

@@ -1,4 +1,5 @@
 use alloc::collections::btree_map::BTreeMap as HashMap;
+use core::fmt::Write;
 
 use abscissa_core::clap::Parser;
 use abscissa_core::{Command, Runnable};
@@ -52,7 +53,7 @@ impl Runnable for KeysListCmd {
             Ok(keys) => {
                 let mut msg = String::new();
                 for (name, key) in keys {
-                    msg.push_str(&format!("\n- {} ({})", name, key.account));
+                    let _ = write!(msg, "\n- {} ({})", name, key.account);
                 }
                 Output::success_msg(msg).exit()
             }

relayer-cli/src/commands/query/channel.rs

@@ -60,7 +60,10 @@ impl Runnable for QueryChannelEndCmd {
                 port_id: self.port_id.clone(),
                 channel_id: self.channel_id,
                 height: self.height.map_or(QueryHeight::Latest, |revision_height| {
-                    QueryHeight::Specific(ibc::Height::new(chain.id().version(), revision_height))
+                    QueryHeight::Specific(
+                        ibc::Height::new(chain.id().version(), revision_height)
+                            .unwrap_or_else(exit_with_unrecoverable_error),
+                    )
                 }),
             },
             IncludeProof::No,

relayer-cli/src/commands/query/channel_ends.rs

@@ -14,7 +14,7 @@ use ibc_relayer::chain::requests::{
 };
 use ibc_relayer::registry::Registry;
 
-use crate::conclude::Output;
+use crate::conclude::{exit_with_unrecoverable_error, Output};
 use crate::prelude::*;
 
 #[derive(Clone, Command, Debug, Parser)]
@@ -96,7 +96,9 @@ fn do_run<Chain: ChainHandle>(cmd: &QueryChannelEndsCmd) -> Result<(), Box<dyn s
     let chain = registry.get_or_spawn(&chain_id)?;
 
     let chain_height = match cmd.height {
-        Some(height) => Height::new(chain.id().version(), height),
+        Some(height) => {
+            Height::new(chain.id().version(), height).unwrap_or_else(exit_with_unrecoverable_error)
+        }
         None => chain.query_latest_height()?,
     };
 

relayer-cli/src/commands/query/client.rs

@@ -59,7 +59,10 @@ impl Runnable for QueryClientStateCmd {
             QueryClientStateRequest {
                 client_id: self.client_id.clone(),
                 height: self.height.map_or(QueryHeight::Latest, |revision_height| {
-                    QueryHeight::Specific(ibc::Height::new(chain.id().version(), revision_height))
+                    QueryHeight::Specific(
+                        ibc::Height::new(chain.id().version(), revision_height)
+                            .unwrap_or_else(exit_with_unrecoverable_error),
+                    )
                 }),
             },
             IncludeProof::No,
@@ -135,7 +138,8 @@ impl Runnable for QueryClientConsensusCmd {
 
         match self.consensus_height {
             Some(cs_height) => {
-                let consensus_height = ibc::Height::new(counterparty_chain.version(), cs_height);
+                let consensus_height = ibc::Height::new(counterparty_chain.version(), cs_height)
+                    .unwrap_or_else(exit_with_unrecoverable_error);
 
                 let res = chain
                     .query_consensus_state(
@@ -145,10 +149,10 @@ impl Runnable for QueryClientConsensusCmd {
                             query_height: self.height.map_or(
                                 QueryHeight::Latest,
                                 |revision_height| {
-                                    QueryHeight::Specific(ibc::Height::new(
-                                        chain.id().version(),
-                                        revision_height,
-                                    ))
+                                    QueryHeight::Specific(
+                                        ibc::Height::new(chain.id().version(), revision_height)
+                                            .unwrap_or_else(exit_with_unrecoverable_error),
+                                    )
                                 },
                             ),
                         },
@@ -212,7 +216,7 @@ pub struct QueryClientHeaderCmd {
     #[clap(
         long = "height",
         value_name = "HEIGHT",
-        help = "The chain height context for the query"
+        help = "The chain height context for the query. Leave unspecified for latest height."
     )]
     height: Option<u64>,
 }
@@ -244,12 +248,14 @@ impl Runnable for QueryClientHeaderCmd {
         };
 
         let consensus_height =
-            ibc::Height::new(counterparty_chain.version(), self.consensus_height);
+            ibc::Height::new(counterparty_chain.version(), self.consensus_height)
+                .unwrap_or_else(exit_with_unrecoverable_error);
 
         let query_height = match self.height {
-            Some(revision_height) => {
-                QueryHeight::Specific(Height::new(chain.id().version(), revision_height))
-            }
+            Some(revision_height) => QueryHeight::Specific(
+                Height::new(chain.id().version(), revision_height)
+                    .unwrap_or_else(exit_with_unrecoverable_error),
+            ),
             None => QueryHeight::Latest,
         };
 

relayer-cli/src/commands/query/connection.rs

@@ -38,7 +38,7 @@ pub struct QueryConnectionEndCmd {
     #[clap(
         long = "height",
         value_name = "HEIGHT",
-        help = "Height of the state to query"
+        help = "Height of the state to query. Leave unspecified for latest height."
     )]
     height: Option<u64>,
 }
@@ -57,7 +57,10 @@ impl Runnable for QueryConnectionEndCmd {
             QueryConnectionRequest {
                 connection_id: self.connection_id.clone(),
                 height: self.height.map_or(QueryHeight::Latest, |revision_height| {
-                    QueryHeight::Specific(ibc::Height::new(chain.id().version(), revision_height))
+                    QueryHeight::Specific(
+                        ibc::Height::new(chain.id().version(), revision_height)
+                            .unwrap_or_else(exit_with_unrecoverable_error),
+                    )
                 }),
             },
             IncludeProof::No,

relayer-cli/src/commands/query/packet/ack.rs

@@ -8,7 +8,7 @@ use ibc::core::ics24_host::identifier::{ChainId, ChannelId, PortId};
 use ibc_relayer::chain::handle::ChainHandle;
 
 use crate::cli_utils::spawn_chain_runtime;
-use crate::conclude::Output;
+use crate::conclude::{exit_with_unrecoverable_error, Output};
 use crate::error::Error;
 use crate::prelude::*;
 
@@ -51,7 +51,7 @@ pub struct QueryPacketAcknowledgmentCmd {
     #[clap(
         long = "height",
         value_name = "HEIGHT",
-        help = "Height of the state to query"
+        help = "Height of the state to query. Leave unspecified for latest height."
     )]
     height: Option<u64>,
 }
@@ -71,10 +71,10 @@ impl QueryPacketAcknowledgmentCmd {
                     channel_id: self.channel_id,
                     sequence: self.sequence,
                     height: self.height.map_or(QueryHeight::Latest, |revision_height| {
-                        QueryHeight::Specific(ibc::Height::new(
-                            chain.id().version(),
-                            revision_height,
-                        ))
+                        QueryHeight::Specific(
+                            ibc::Height::new(chain.id().version(), revision_height)
+                                .unwrap_or_else(exit_with_unrecoverable_error),
+                        )
                     }),
                 },
                 IncludeProof::No,

relayer-cli/src/commands/query/packet/commitment.rs

@@ -10,7 +10,7 @@ use ibc::Height;
 use ibc_relayer::chain::handle::ChainHandle;
 
 use crate::cli_utils::spawn_chain_runtime;
-use crate::conclude::Output;
+use crate::conclude::{exit_with_unrecoverable_error, Output};
 use crate::error::Error;
 use crate::prelude::*;
 
@@ -59,7 +59,7 @@ pub struct QueryPacketCommitmentCmd {
     #[clap(
         long = "height",
         value_name = "HEIGHT",
-        help = "Height of the state to query"
+        help = "Height of the state to query. Leave unspecified for latest height."
     )]
     height: Option<u64>,
 }
@@ -79,10 +79,10 @@ impl QueryPacketCommitmentCmd {
                     channel_id: self.channel_id,
                     sequence: self.sequence,
                     height: self.height.map_or(QueryHeight::Latest, |revision_height| {
-                        QueryHeight::Specific(ibc::Height::new(
-                            chain.id().version(),
-                            revision_height,
-                        ))
+                        QueryHeight::Specific(
+                            ibc::Height::new(chain.id().version(), revision_height)
+                                .unwrap_or_else(exit_with_unrecoverable_error),
+                        )
                     }),
                 },
                 IncludeProof::No,

relayer-cli/src/commands/tx/client.rs

@@ -122,14 +122,14 @@ pub struct TxUpdateClientCmd {
     #[clap(
         long = "height",
         value_name = "REFERENCE_HEIGHT",
-        help = "The target height of the client update"
+        help = "The target height of the client update. Leave unspecified for latest height."
     )]
     target_height: Option<u64>,
 
     #[clap(
         long = "trusted-height",
         value_name = "REFERENCE_TRUSTED_HEIGHT",
-        help = "The trusted height of the client update"
+        help = "The trusted height of the client update. Leave unspecified for latest height."
     )]
     trusted_height: Option<u64>,
 }
@@ -166,12 +166,16 @@ impl Runnable for TxUpdateClientCmd {
         };
 
         let target_height = self.target_height.map_or(QueryHeight::Latest, |height| {
-            QueryHeight::Specific(Height::new(src_chain.id().version(), height))
+            QueryHeight::Specific(
+                Height::new(src_chain.id().version(), height)
+                    .unwrap_or_else(exit_with_unrecoverable_error),
+            )
         });
 
-        let trusted_height = self
-            .trusted_height
-            .map(|height| Height::new(src_chain.id().version(), height));
+        let trusted_height = self.trusted_height.map(|height| {
+            Height::new(src_chain.id().version(), height)
+                .unwrap_or_else(exit_with_unrecoverable_error)
+        });
 
         let client = ForeignClient::find(src_chain, dst_chain, &self.dst_client_id)
             .unwrap_or_else(exit_with_unrecoverable_error);

relayer-cli/src/commands/tx/transfer.rs

@@ -142,14 +142,6 @@ impl TxIcs20MsgTransferCmd {
             return Err("number of messages should be greater than zero".into());
         }
 
-        if self.timeout_height_offset == 0 && self.timeout_seconds == 0 {
-            return Err(
-                "packet timeout height and packet timeout timestamp cannot both be 0, \
-                please specify either --timeout-height-offset or --timeout-seconds"
-                    .into(),
-            );
-        }
-
         let opts = TransferOptions {
             packet_src_port_id: self.src_port_id.clone(),
             packet_src_channel_id: self.src_channel_id,