Improve error handling and string conversion in IBC crates. (#726)

* Improve error handling and string conversion in IBC crates. * Refactor crate to disallow usage of unwrap(), add more informative error messages and improve UTF-8 compliance. Update try_from convenience methods to return error messages for incorrect data ranges. * Refactor client_state.rs: Fix tests to use height.min() in expect. * Refactor host block retrieval in MockContext struct. --------- Signed-off-by: Davirain <[email protected]>
cosmos · Jul 5, 2023 · 3ac235b · 3ac235b
1 parent ab69f3e
commit 3ac235b
Show file tree

Hide file tree

Showing 19 changed files with 135 additions and 102 deletions.
diff --git a/.changelog/unreleased/improvement/655-remove-unwrap.md b/.changelog/unreleased/improvement/655-remove-unwrap.md
@@ -0,0 +1,2 @@
+- Deny use of unwrap() throughout the crate
+  ([#655](https://github.com/cosmos/ibc-rs/issues/655))
diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs
@@ -396,10 +396,7 @@ impl Ics2ClientState for ClientState {
         _client_message: Any,
         _update_kind: &UpdateKind,
     ) -> Result<(), ClientError> {
-        let frozen_client_state = self
-            .clone()
-            .with_frozen_height(Height::new(0, 1).unwrap())
-            .into_box();
+        let frozen_client_state = self.clone().with_frozen_height(Height::min(0)).into_box();
 
         ctx.store_client_state(ClientStatePath::new(client_id), frozen_client_state)?;
 
@@ -813,7 +810,7 @@ mod tests {
             trusting_period: Duration::new(64000, 0),
             unbonding_period: Duration::new(128000, 0),
             max_clock_drift: Duration::new(3, 0),
-            latest_height: Height::new(0, 10).unwrap(),
+            latest_height: Height::new(0, 10).expect("Never fails"),
             proof_specs: ProofSpecs::default(),
             upgrade_path: Default::default(),
             allow_update: AllowUpdate {
@@ -919,15 +916,15 @@ mod tests {
             Test {
                 name: "Invalid (too small) trusting trust threshold".to_string(),
                 params: ClientStateParams {
-                    trust_level: TrustThreshold::new(1, 4).unwrap(),
+                    trust_level: TrustThreshold::new(1, 4).expect("Never fails"),
                     ..default_params.clone()
                 },
                 want_pass: false,
             },
             Test {
                 name: "Invalid latest height revision number (doesn't match chain)".to_string(),
                 params: ClientStateParams {
-                    latest_height: Height::new(1, 1).unwrap(),
+                    latest_height: Height::new(1, 1).expect("Never fails"),
                     ..default_params.clone()
                 },
                 want_pass: false,
@@ -979,7 +976,7 @@ mod tests {
             trusting_period: Duration::new(64000, 0),
             unbonding_period: Duration::new(128000, 0),
             max_clock_drift: Duration::new(3, 0),
-            latest_height: Height::new(1, 10).unwrap(),
+            latest_height: Height::new(1, 10).expect("Never fails"),
             proof_specs: ProofSpecs::default(),
             upgrade_path: Default::default(),
             allow_update: AllowUpdate {
@@ -998,13 +995,13 @@ mod tests {
         let tests = vec![
             Test {
                 name: "Successful height verification".to_string(),
-                height: Height::new(1, 8).unwrap(),
+                height: Height::new(1, 8).expect("Never fails"),
                 setup: None,
                 want_pass: true,
             },
             Test {
                 name: "Invalid (too large)  client height".to_string(),
-                height: Height::new(1, 12).unwrap(),
+                height: Height::new(1, 12).expect("Never fails"),
                 setup: None,
                 want_pass: false,
             },
@@ -1023,7 +1020,7 @@ mod tests {
                 p.upgrade_path,
                 p.allow_update,
             )
-            .unwrap();
+            .expect("Never fails");
             let client_state = match test.setup {
                 Some(setup) => (setup)(client_state),
                 _ => client_state,
@@ -1050,13 +1047,17 @@ mod tests {
         });
         assert!(tm_client_state_from_raw.is_ok());
 
-        let any_from_tm_client_state =
-            Any::from(tm_client_state_from_raw.as_ref().unwrap().clone());
+        let any_from_tm_client_state = Any::from(
+            tm_client_state_from_raw
+                .as_ref()
+                .expect("Never fails")
+                .clone(),
+        );
         let tm_client_state_from_any = TmClientState::try_from(any_from_tm_client_state);
         assert!(tm_client_state_from_any.is_ok());
         assert_eq!(
-            tm_client_state_from_raw.unwrap(),
-            tm_client_state_from_any.unwrap()
+            tm_client_state_from_raw.expect("Never fails"),
+            tm_client_state_from_any.expect("Never fails")
         );
 
         // check client state creation path from a tendermint header
@@ -1067,7 +1068,7 @@ mod tests {
         assert!(tm_client_state_from_any.is_ok());
         assert_eq!(
             tm_client_state_from_header,
-            tm_client_state_from_any.unwrap()
+            tm_client_state_from_any.expect("Never fails")
         );
     }
 
@@ -1136,15 +1137,15 @@ pub mod test_util {
                     ChainId::chain_version(tm_header.chain_id.as_str()),
                     u64::from(tm_header.height),
                 )
-                .unwrap(),
+                .expect("Never fails"),
                 Default::default(),
                 Default::default(),
                 AllowUpdate {
                     after_expiry: false,
                     after_misbehaviour: false,
                 },
             )
-            .unwrap()
+            .expect("Never fails")
         }
     }
 
@@ -1159,7 +1160,7 @@ pub mod test_util {
             trusting_period: Some(Duration::from_secs(64000).into()),
             unbonding_period: Some(Duration::from_secs(128000).into()),
             max_clock_drift: Some(Duration::from_millis(3000).into()),
-            latest_height: Some(Height::new(0, 10).unwrap().into()),
+            latest_height: Some(Height::new(0, 10).expect("Never fails").into()),
             proof_specs: ProofSpecs::default().into(),
             upgrade_path: Default::default(),
             frozen_height: Some(frozen_height),

diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs b/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs
@@ -71,7 +71,11 @@ impl ClientState {
             };
 
             let options = self.as_light_client_options()?;
-            let now = ctx.host_timestamp()?.into_tm_time().unwrap();
+            let now = ctx.host_timestamp()?.into_tm_time().ok_or_else(|| {
+                ClientError::ClientSpecific {
+                    description: "host timestamp is not a valid TM timestamp".to_string(),
+                }
+            })?;
 
             // main header verification, delegated to the tendermint-light-client crate.
             self.verifier

diff --git a/crates/ibc/src/clients/ics07_tendermint/header.rs b/crates/ibc/src/clients/ics07_tendermint/header.rs
@@ -272,7 +272,7 @@ pub mod test_util {
         serde_json::from_str::<SignedHeader>(include_str!(
             "../../../tests/support/signed_header.json"
         ))
-        .unwrap()
+        .expect("Never fails")
         .header
     }
 
@@ -295,7 +295,7 @@ pub mod test_util {
         let shdr = serde_json::from_str::<SignedHeader>(include_str!(
             "../../../tests/support/signed_header.json"
         ))
-        .unwrap();
+        .expect("Never fails");
 
         // Build a set of validators.
         // Below are test values inspired form `test_validator_set()` in tendermint-rs.
@@ -304,18 +304,18 @@ pub mod test_util {
                 &hex::decode_upper(
                     "F349539C7E5EF7C49549B09C4BFC2335318AB0FE51FBFAA2433B4F13E816F4A7",
                 )
-                .unwrap(),
+                .expect("Never fails"),
             )
-            .unwrap(),
-            281_815_u64.try_into().unwrap(),
+            .expect("Never fails"),
+            281_815_u64.try_into().expect("Never fails"),
         );
 
         let vs = ValidatorSet::new(vec![v1.clone()], Some(v1));
 
         Header {
             signed_header: shdr,
             validator_set: vs.clone(),
-            trusted_height: Height::new(0, 1).unwrap(),
+            trusted_height: Height::min(0),
             trusted_next_validator_set: vs,
         }
     }

diff --git a/crates/ibc/src/core/ics02_client/events.rs b/crates/ibc/src/core/ics02_client/events.rs
@@ -154,7 +154,8 @@ impl From<HeaderAttribute> for abci::EventAttribute {
     fn from(attr: HeaderAttribute) -> Self {
         (
             HEADER_ATTRIBUTE_KEY,
-            String::from_utf8(hex::encode(attr.header.value)).unwrap(),
+            String::from_utf8(hex::encode(attr.header.value))
+                .expect("Never fails because hexadecimal is valid UTF-8"),
         )
             .into()
     }

diff --git a/crates/ibc/src/core/ics02_client/height.rs b/crates/ibc/src/core/ics02_client/height.rs
@@ -50,6 +50,13 @@ impl Height {
         })
     }
 
+    pub fn min(revision_number: u64) -> Self {
+        Self {
+            revision_number,
+            revision_height: 1,
+        }
+    }
+
     pub fn revision_number(&self) -> u64 {
         self.revision_number
     }

diff --git a/crates/ibc/src/core/ics04_channel/events/packet_attributes.rs b/crates/ibc/src/core/ics04_channel/events/packet_attributes.rs
@@ -63,7 +63,8 @@ impl TryFrom<PacketDataAttribute> for Vec<abci::EventAttribute> {
                 .into(),
             (
                 PKT_DATA_HEX_ATTRIBUTE_KEY,
-                String::from_utf8(hex::encode(attr.packet_data)).unwrap(),
+                String::from_utf8(hex::encode(attr.packet_data))
+                    .expect("Never fails because hexadecimal is valid UTF8"),
             )
                 .into(),
         ];
@@ -332,7 +333,8 @@ impl TryFrom<AcknowledgementAttribute> for Vec<abci::EventAttribute> {
                 .into(),
             (
                 PKT_ACK_HEX_ATTRIBUTE_KEY,
-                String::from_utf8(hex::encode(attr.acknowledgement)).unwrap(),
+                String::from_utf8(hex::encode(attr.acknowledgement))
+                    .expect("Never fails because hexadecimal is always valid UTF-8"),
             )
                 .into(),
         ];

diff --git a/crates/ibc/src/core/ics23_commitment/commitment.rs b/crates/ibc/src/core/ics23_commitment/commitment.rs
@@ -23,7 +23,9 @@ pub struct CommitmentRoot {
 
 impl fmt::Debug for CommitmentRoot {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let hex = Hex::upper_case().encode_to_string(&self.bytes).unwrap();
+        let hex = Hex::upper_case()
+            .encode_to_string(&self.bytes)
+            .map_err(|_| fmt::Error)?;
         f.debug_tuple("CommitmentRoot").field(&hex).finish()
     }
 }
@@ -72,7 +74,9 @@ pub struct CommitmentProofBytes {
 
 impl fmt::Debug for CommitmentProofBytes {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let hex = Hex::upper_case().encode_to_string(&self.bytes).unwrap();
+        let hex = Hex::upper_case()
+            .encode_to_string(&self.bytes)
+            .map_err(|_| fmt::Error)?;
         f.debug_tuple("CommitmentProof").field(&hex).finish()
     }
 }
@@ -100,7 +104,8 @@ impl TryFrom<RawMerkleProof> for CommitmentProofBytes {
 
     fn try_from(proof: RawMerkleProof) -> Result<Self, Self::Error> {
         let mut buf = Vec::new();
-        prost::Message::encode(&proof, &mut buf).unwrap();
+        prost::Message::encode(&proof, &mut buf)
+            .map_err(|e| Self::Error::EncodingFailure(e.to_string()))?;
         buf.try_into()
     }
 }

diff --git a/crates/ibc/src/core/ics23_commitment/error.rs b/crates/ibc/src/core/ics23_commitment/error.rs
@@ -1,5 +1,6 @@
 //! Defines the commitment error type
 
+use alloc::string::String;
 use displaydoc::Display;
 use prost::DecodeError;
 
@@ -25,6 +26,8 @@ pub enum CommitmentError {
     InvalidMerkleProof,
     /// proof verification failed
     VerificationFailure,
+    /// encoded commitment prefix is not a valid hex string: `{0}`
+    EncodingFailure(String),
 }
 
 #[cfg(feature = "std")]

diff --git a/crates/ibc/src/core/ics23_commitment/merkle.rs b/crates/ibc/src/core/ics23_commitment/merkle.rs
@@ -39,31 +39,16 @@ pub struct MerkleProof {
 /// Ref. <https://github.com/informalsystems/ibc-rs/issues/853>
 impl From<RawMerkleProof> for MerkleProof {
     fn from(proof: RawMerkleProof) -> Self {
-        let proofs: Vec<CommitmentProof> = proof
-            .proofs
-            .into_iter()
-            .map(|p| {
-                let mut encoded = Vec::new();
-                prost::Message::encode(&p, &mut encoded).unwrap();
-                prost::Message::decode(&*encoded).unwrap()
-            })
-            .collect();
-        Self { proofs }
+        Self {
+            proofs: proof.proofs,
+        }
     }
 }
 
 impl From<MerkleProof> for RawMerkleProof {
     fn from(proof: MerkleProof) -> Self {
         Self {
-            proofs: proof
-                .proofs
-                .into_iter()
-                .map(|p| {
-                    let mut encoded = Vec::new();
-                    prost::Message::encode(&p, &mut encoded).unwrap();
-                    prost::Message::decode(&*encoded).unwrap()
-                })
-                .collect(),
+            proofs: proof.proofs,
         }
     }
 }

diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs
@@ -237,7 +237,7 @@ impl FromStr for ClientId {
 
 impl Default for ClientId {
     fn default() -> Self {
-        Self::new(tm_client_type(), 0).unwrap()
+        Self::new(tm_client_type(), 0).expect("Never fails because we use a valid client type")
     }
 }
 

diff --git a/crates/ibc/src/core/timestamp.rs b/crates/ibc/src/core/timestamp.rs
@@ -121,9 +121,13 @@ impl Timestamp {
             // about year 2554, there is no risk of overflowing `Time`
             // or `OffsetDateTime`.
             let ts = OffsetDateTime::from_unix_timestamp_nanos(nanoseconds as i128)
-                .unwrap()
+                .map_err(|e: time::error::ComponentRange| {
+                    ParseTimestampError::DataOutOfRange(e.to_string())
+                })?
                 .try_into()
-                .unwrap();
+                .map_err(|e: tendermint::error::Error| {
+                    ParseTimestampError::DataOutOfRange(e.to_string())
+                })?;
             Ok(Timestamp { time: Some(ts) })
         }
     }
@@ -180,7 +184,9 @@ impl Timestamp {
             let t: OffsetDateTime = time.into();
             let s = t.unix_timestamp_nanos();
             assert!(s >= 0, "time {time:?} has negative `.timestamp()`");
-            s.try_into().unwrap()
+            s.try_into().expect(
+                "Fails UNIX timestamp is negative, but we don't allow that to be constructed",
+            )
         })
     }
 
@@ -269,13 +275,16 @@ impl Sub<Duration> for Timestamp {
 pub enum ParseTimestampError {
     /// parsing u64 integer from string error: `{0}`
     ParseInt(ParseIntError),
+    /// Out of Range: `{0}`
+    DataOutOfRange(String),
 }
 
 #[cfg(feature = "std")]
 impl std::error::Error for ParseTimestampError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match &self {
             ParseTimestampError::ParseInt(e) => Some(e),
+            ParseTimestampError::DataOutOfRange(_) => None,
         }
     }
 }

diff --git a/crates/ibc/src/lib.rs b/crates/ibc/src/lib.rs
@@ -1,6 +1,4 @@
-// TODO: disable unwraps:
-// https://github.com/informalsystems/ibc-rs/issues/987
-// #![cfg_attr(not(test), deny(clippy::unwrap_used))]
+#![cfg_attr(not(test), deny(clippy::unwrap_used))]
 #![no_std]
 #![deny(
     warnings,

diff --git a/crates/ibc/src/mock/client_state.rs b/crates/ibc/src/mock/client_state.rs
@@ -87,7 +87,7 @@ impl TryFrom<RawMockClientState> for MockClientState {
     type Error = ClientError;
 
     fn try_from(raw: RawMockClientState) -> Result<Self, Self::Error> {
-        Ok(Self::new(raw.header.unwrap().try_into()?))
+        Ok(Self::new(raw.header.expect("Never fails").try_into()?))
     }
 }
 
@@ -316,9 +316,7 @@ impl ClientState for MockClientState {
         _client_message: Any,
         _update_kind: &UpdateKind,
     ) -> Result<(), ClientError> {
-        let frozen_client_state = self
-            .with_frozen_height(Height::new(0, 1).unwrap())
-            .into_box();
+        let frozen_client_state = self.with_frozen_height(Height::min(0)).into_box();
 
         ctx.store_client_state(ClientStatePath::new(client_id), frozen_client_state)?;
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		- Deny use of unwrap() throughout the crate
		([#655](https://github.com/cosmos/ibc-rs/issues/655))