feat: turn ClientConsensusStatePath::haight into Height

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: #603

Author: mina86

crates/ibc-testkit/src/testapp/ibc/core/client_ctx.rs

@@ -235,11 +235,9 @@ impl ClientExecutionContext for MockContext {
                 client_state: Default::default(),
             });
 
-        let height = Height::new(consensus_state_path.epoch, consensus_state_path.height)
-            .expect("Never fails");
         client_record
             .consensus_states
-            .insert(height, consensus_state);
+            .insert(consensus_state_path.height, consensus_state);
 
         Ok(())
     }
@@ -258,10 +256,9 @@ impl ClientExecutionContext for MockContext {
                 client_state: Default::default(),
             });
 
-        let height = Height::new(consensus_state_path.epoch, consensus_state_path.height)
-            .expect("Never fails");
-
-        client_record.consensus_states.remove(&height);
+        client_record
+            .consensus_states
+            .remove(&consensus_state_path.height);
 
         Ok(())
     }

crates/ibc-testkit/src/testapp/ibc/core/core_ctx.rs

@@ -70,18 +70,18 @@ impl ValidationContext for MockContext {
         client_cons_state_path: &ClientConsensusStatePath,
     ) -> Result<AnyConsensusState, ContextError> {
         let client_id = &client_cons_state_path.client_id;
-        let height = Height::new(client_cons_state_path.epoch, client_cons_state_path.height)?;
+        let height = &client_cons_state_path.height;
         match self.ibc_store.lock().clients.get(client_id) {
-            Some(client_record) => match client_record.consensus_states.get(&height) {
+            Some(client_record) => match client_record.consensus_states.get(height) {
                 Some(consensus_state) => Ok(consensus_state.clone()),
                 None => Err(ClientError::ConsensusStateNotFound {
                     client_id: client_id.clone(),
-                    height,
+                    height: height.clone(),
                 }),
             },
             None => Err(ClientError::ConsensusStateNotFound {
                 client_id: client_id.clone(),
-                height,
+                height: height.clone(),
             }),
         }
         .map_err(ContextError::ClientError)

crates/ibc/src/core/ics24_host/path.rs

@@ -74,22 +74,84 @@ impl ClientStatePath {
     feature = "borsh",
     derive(borsh::BorshSerialize, borsh::BorshDeserialize)
 )]
-#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)]
-#[display(fmt = "clients/{client_id}/consensusStates/{epoch}-{height}")]
+#[display(
+    fmt = "clients/{}/consensusStates/{}-{}",
+    client_id,
+    "height.revision_number()",
+    "height.revision_height()"
+)]
 pub struct ClientConsensusStatePath {
     pub client_id: ClientId,
-    pub epoch: u64,
-    pub height: u64,
+    pub height: Height,
 }
 
 impl ClientConsensusStatePath {
     pub fn new(client_id: &ClientId, height: &Height) -> ClientConsensusStatePath {
         ClientConsensusStatePath {
             client_id: client_id.clone(),
-            epoch: height.revision_number(),
-            height: height.revision_height(),
+            height: height.clone(),
+        }
+    }
+
+    pub fn revision_number(&self) -> u64 {
+        self.height.revision_number()
+    }
+
+    pub fn revision_height(&self) -> u64 {
+        self.height.revision_height()
+    }
+}
+
+#[cfg(feature = "serde")]
+impl serde::Serialize for ClientConsensusStatePath {
+    /// Serialises the path as a struct with three elements.
+    ///
+    /// Rather than serialising the path as a nested type with `height` being an
+    /// embedded structure, serialises it as a structure with three fields:
+    ///
+    /// ```ignore
+    /// struct ClientConsensusStatePath {
+    ///     client_id: ClientId,
+    ///     epoch: u64,   // equal height.revision_number()
+    ///     height: u64,  // equal height.revision_height()
+    /// }
+    /// ```
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        use serde::ser::SerializeStruct;
+
+        let mut serializer = serializer.serialize_struct("ClientConsensusStatePath", 3)?;
+        serializer.serialize_field("client_id", &self.client_id)?;
+        serializer.serialize_field("epoch", &self.revision_number())?;
+        serializer.serialize_field("height", &self.revision_height())?;
+        serializer.end()
+    }
+}
+
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for ClientConsensusStatePath {
+    /// Deserialises the path from a struct with three elements.
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        use serde::de::Error;
+
+        #[derive(serde::Deserialize)]
+        struct ClientConsensusStatePath {
+            client_id: ClientId,
+            epoch: u64,
+            height: u64,
         }
+
+        let path = ClientConsensusStatePath::deserialize(deserializer)?;
+        let client_id = path.client_id;
+        Height::new(path.epoch, path.height)
+            .map(|height| Self { client_id, height })
+            .map_err(|_| D::Error::custom("height cannot be zero"))
     }
 }
 
@@ -480,8 +542,7 @@ fn parse_client_paths(components: &[&str]) -> Option<Path> {
         Some(
             ClientConsensusStatePath {
                 client_id,
-                epoch,
-                height,
+                height: Height::new(epoch, height).ok()?,
             }
             .into(),
         )
@@ -862,8 +923,7 @@ mod tests {
             parse_client_paths(&components),
             Some(Path::ClientConsensusState(ClientConsensusStatePath {
                 client_id: ClientId::default(),
-                epoch: 15,
-                height: 31,
+                height: Height::new(15, 31).unwrap(),
             }))
         );
     }
@@ -890,8 +950,7 @@ mod tests {
             path.unwrap(),
             Path::ClientConsensusState(ClientConsensusStatePath {
                 client_id: ClientId::default(),
-                epoch: 15,
-                height: 31,
+                height: Height::new(15, 31).unwrap(),
             })
         );
     }