From 8cc87df0a577a12702261d180d092b65ee7d4330 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 16 Nov 2023 19:35:30 +0100 Subject: [PATCH] feat: turn ClientConsensusStatePath::haight into Height MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/cosmos/ibc-rs/issues/603 --- .../src/testapp/ibc/core/client_ctx.rs | 11 +-- .../src/testapp/ibc/core/core_ctx.rs | 8 +- crates/ibc/src/core/ics24_host/path.rs | 84 ++++++++++++++++--- 3 files changed, 80 insertions(+), 23 deletions(-) diff --git a/crates/ibc-testkit/src/testapp/ibc/core/client_ctx.rs b/crates/ibc-testkit/src/testapp/ibc/core/client_ctx.rs index 17bb7ff616..6c6c6751b3 100644 --- a/crates/ibc-testkit/src/testapp/ibc/core/client_ctx.rs +++ b/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(()) } diff --git a/crates/ibc-testkit/src/testapp/ibc/core/core_ctx.rs b/crates/ibc-testkit/src/testapp/ibc/core/core_ctx.rs index 470f28a86d..dc479e88d7 100644 --- a/crates/ibc-testkit/src/testapp/ibc/core/core_ctx.rs +++ b/crates/ibc-testkit/src/testapp/ibc/core/core_ctx.rs @@ -70,18 +70,18 @@ impl ValidationContext for MockContext { client_cons_state_path: &ClientConsensusStatePath, ) -> Result { 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) diff --git a/crates/ibc/src/core/ics24_host/path.rs b/crates/ibc/src/core/ics24_host/path.rs index 6630abbf0b..11c2820f74 100644 --- a/crates/ibc/src/core/ics24_host/path.rs +++ b/crates/ibc/src/core/ics24_host/path.rs @@ -74,22 +74,85 @@ 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(&self, serializer: S) -> Result + 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; see + /// [`Self::serialize`]. + fn deserialize(deserializer: D) -> Result + 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 +543,7 @@ fn parse_client_paths(components: &[&str]) -> Option { Some( ClientConsensusStatePath { client_id, - epoch, - height, + height: Height::new(epoch, height).ok()?, } .into(), ) @@ -862,8 +924,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 +951,7 @@ mod tests { path.unwrap(), Path::ClientConsensusState(ClientConsensusStatePath { client_id: ClientId::default(), - epoch: 15, - height: 31, + height: Height::new(15, 31).unwrap(), }) ); }