From d0f7cc41c413bdb1e801acaa566f3e7cf06c2c93 Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Mon, 22 Apr 2024 06:22:16 -0400 Subject: [PATCH] fix(cw-context): encoding, decoding and iteration of `ConsensusState` heights (#1176) * only consider ITERATE_CONSENSUS_STATE_PREFIX keys * use proto codec * refactor with updated parse_height type sig * encode and decode height funcs * fix clippy * rm redundant type annotation * add changelog entry * correct link on changelog * rm changelog entry * handle missing case * use cw_storage_plus::Map * store or delete height keys at cw_storage_plus::Map * use cw_storage_plus::Map range or keys * rm redundant encode_height and decode_height * add comment * use imports * cargo format * deserialized result must be handled * update doc string --- Cargo.toml | 1 + ibc-clients/cw-context/Cargo.toml | 1 + .../cw-context/src/context/client_ctx.rs | 26 ++-- ibc-clients/cw-context/src/context/mod.rs | 137 +++++++++++------- ibc-clients/cw-context/src/lib.rs | 2 - ibc-clients/cw-context/src/utils/mod.rs | 22 --- 6 files changed, 105 insertions(+), 84 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b85c7e0c1..62981c026 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,6 +119,7 @@ tendermint-testgen = { version = "0.34.0", default-features = fals cosmwasm-schema = { version = "1.5.2" } cosmwasm-std = { version = "1.5.2" } cosmwasm-vm = { version = "1.5.2" } +cw-storage-plus = { version = "1.2.0" } # parity dependencies parity-scale-codec = { version = "3.6.5", default-features = false, features = ["full"] } diff --git a/ibc-clients/cw-context/Cargo.toml b/ibc-clients/cw-context/Cargo.toml index c3fd8ecb5..95132d252 100644 --- a/ibc-clients/cw-context/Cargo.toml +++ b/ibc-clients/cw-context/Cargo.toml @@ -28,6 +28,7 @@ ibc-client-wasm-types = { workspace = true, features = ["cosmwasm"] } # cosmwasm dependencies cosmwasm-schema = { workspace = true } cosmwasm-std = { workspace = true } +cw-storage-plus = { workspace = true } [features] default = ["std"] diff --git a/ibc-clients/cw-context/src/context/client_ctx.rs b/ibc-clients/cw-context/src/context/client_ctx.rs index ca1b2f6f9..acb45c311 100644 --- a/ibc-clients/cw-context/src/context/client_ctx.rs +++ b/ibc-clients/cw-context/src/context/client_ctx.rs @@ -7,12 +7,13 @@ use ibc_core::client::types::error::ClientError; use ibc_core::client::types::Height; use ibc_core::handler::types::error::ContextError; use ibc_core::host::types::identifiers::ClientId; -use ibc_core::host::types::path::{iteration_key, ClientConsensusStatePath, ClientStatePath}; +use ibc_core::host::types::path::{ClientConsensusStatePath, ClientStatePath}; use ibc_core::primitives::proto::{Any, Protobuf}; use ibc_core::primitives::Timestamp; -use super::Context; +use super::{Context, StorageMut}; use crate::api::ClientType; +use crate::context::CONSENSUS_STATE_HEIGHT_MAP; use crate::utils::AnyCodec; impl<'a, C: ClientType<'a>> ClientValidationContext for Context<'a, C> { @@ -154,11 +155,15 @@ impl<'a, C: ClientType<'a>> ClientExecutionContext for Context<'a, C> { self.insert(prefixed_height_key, revision_height_vec); - let iteration_key = iteration_key(height.revision_number(), height.revision_height()); - - let height_vec = height.to_string().into_bytes(); - - self.insert(iteration_key, height_vec); + CONSENSUS_STATE_HEIGHT_MAP + .save( + self.storage_mut(), + (height.revision_number(), height.revision_height()), + &Default::default(), + ) + .map_err(|e| ClientError::Other { + description: e.to_string(), + })?; Ok(()) } @@ -180,9 +185,10 @@ impl<'a, C: ClientType<'a>> ClientExecutionContext for Context<'a, C> { self.remove(prefixed_height_key); - let iteration_key = iteration_key(height.revision_number(), height.revision_height()); - - self.remove(iteration_key); + CONSENSUS_STATE_HEIGHT_MAP.remove( + self.storage_mut(), + (height.revision_number(), height.revision_height()), + ); Ok(()) } diff --git a/ibc-clients/cw-context/src/context/mod.rs b/ibc-clients/cw-context/src/context/mod.rs index 19b6d0006..1000f837c 100644 --- a/ibc-clients/cw-context/src/context/mod.rs +++ b/ibc-clients/cw-context/src/context/mod.rs @@ -3,7 +3,8 @@ pub mod custom_ctx; use std::str::FromStr; -use cosmwasm_std::{Deps, DepsMut, Env, Order, Storage}; +use cosmwasm_std::{Deps, DepsMut, Empty, Env, Order, Storage}; +use cw_storage_plus::{Bound, Map}; use ibc_client_wasm_types::client_state::ClientState as WasmClientState; use ibc_core::client::context::client_state::ClientStateCommon; use ibc_core::client::types::error::ClientError; @@ -18,10 +19,18 @@ use prost::Message; use crate::api::ClientType; use crate::types::{ContractError, GenesisMetadata, HeightTravel, MigrationPrefix}; -use crate::utils::{parse_height, AnyCodec}; +use crate::utils::AnyCodec; type Checksum = Vec; +/// - [`Height`] can not be used directly as keys in the map, +/// as it doesn't implement some cw_storage specific traits. +/// - Only a sorted set is needed. So the value type is set to +/// [`Empty`] following +/// ([cosmwasm-book](https://book.cosmwasm.com/cross-contract/map-storage.html#maps-as-sets)). +pub const CONSENSUS_STATE_HEIGHT_MAP: Map<'_, (u64, u64), Empty> = + Map::new(ITERATE_CONSENSUS_STATE_PREFIX); + /// Context is a wrapper around the deps and env that gives access to the /// methods under the ibc-rs Validation and Execution traits. pub struct Context<'a, C: ClientType<'a>> { @@ -130,13 +139,16 @@ impl<'a, C: ClientType<'a>> Context<'a, C> { /// Returns the storage of the context. pub fn get_heights(&self) -> Result, ClientError> { - let iterator = self.storage_ref().range(None, None, Order::Ascending); - - let heights: Vec<_> = iterator - .filter_map(|(_, value)| parse_height(value).transpose()) - .collect::>()?; - - Ok(heights) + CONSENSUS_STATE_HEIGHT_MAP + .keys(self.storage_ref(), None, None, Order::Ascending) + .map(|deserialized_result| { + let (rev_number, rev_height) = + deserialized_result.map_err(|e| ClientError::Other { + description: e.to_string(), + })?; + Height::new(rev_number, rev_height) + }) + .collect() } /// Searches for either the earliest next or latest previous height based on @@ -146,22 +158,37 @@ impl<'a, C: ClientType<'a>> Context<'a, C> { height: &Height, travel: HeightTravel, ) -> Result, ClientError> { - let iteration_key = iteration_key(height.revision_number(), height.revision_height()); - - let mut iterator = match travel { - HeightTravel::Prev => { - self.storage_ref() - .range(None, Some(&iteration_key), Order::Descending) - } - HeightTravel::Next => { - self.storage_ref() - .range(Some(&iteration_key), None, Order::Ascending) - } + let iterator = match travel { + HeightTravel::Prev => CONSENSUS_STATE_HEIGHT_MAP.range( + self.storage_ref(), + None, + Some(Bound::exclusive(( + height.revision_number(), + height.revision_height(), + ))), + Order::Descending, + ), + HeightTravel::Next => CONSENSUS_STATE_HEIGHT_MAP.range( + self.storage_ref(), + Some(Bound::exclusive(( + height.revision_number(), + height.revision_height(), + ))), + None, + Order::Ascending, + ), }; iterator + .map(|deserialized_result| { + let ((rev_number, rev_height), _) = + deserialized_result.map_err(|e| ClientError::Other { + description: e.to_string(), + })?; + Height::new(rev_number, rev_height) + }) .next() - .map_or(Ok(None), |(_, height)| parse_height(height)) + .transpose() } /// Returns the key for the client update time. @@ -191,38 +218,48 @@ impl<'a, C: ClientType<'a>> Context<'a, C> { pub fn get_metadata(&self) -> Result>, ContractError> { let mut metadata = Vec::::new(); - let start_key = ITERATE_CONSENSUS_STATE_PREFIX.to_string().into_bytes(); - - let iterator = self - .storage_ref() - .range(Some(&start_key), None, Order::Ascending); - - for (_, encoded_height) in iterator { - let height = parse_height(encoded_height)?; - - match height { - Some(height) => { - let processed_height_key = self.client_update_height_key(&height); - metadata.push(GenesisMetadata { - key: processed_height_key.clone(), - value: self.retrieve(&processed_height_key)?, - }); - let processed_time_key = self.client_update_time_key(&height); - metadata.push(GenesisMetadata { - key: processed_time_key.clone(), - value: self.retrieve(&processed_time_key)?, - }); - } - None => continue, - } + let iterator = CONSENSUS_STATE_HEIGHT_MAP + .keys(self.storage_ref(), None, None, Order::Ascending) + .map(|deserialized_result| { + let (rev_number, rev_height) = + deserialized_result.map_err(|e| ClientError::Other { + description: e.to_string(), + })?; + Height::new(rev_number, rev_height) + }); + + for height_result in iterator { + let height = height_result?; + + let processed_height_key = self.client_update_height_key(&height); + metadata.push(GenesisMetadata { + key: processed_height_key.clone(), + value: self.retrieve(&processed_height_key)?, + }); + let processed_time_key = self.client_update_time_key(&height); + metadata.push(GenesisMetadata { + key: processed_time_key.clone(), + value: self.retrieve(&processed_time_key)?, + }); } - let iterator = self - .storage_ref() - .range(Some(&start_key), None, Order::Ascending); + let iterator = CONSENSUS_STATE_HEIGHT_MAP + .keys(self.storage_ref(), None, None, Order::Ascending) + .map(|deserialized_result| { + let (rev_number, rev_height) = + deserialized_result.map_err(|e| ClientError::Other { + description: e.to_string(), + })?; + Height::new(rev_number, rev_height) + }); + + for height_result in iterator { + let height = height_result?; - for (key, height) in iterator { - metadata.push(GenesisMetadata { key, value: height }); + metadata.push(GenesisMetadata { + key: iteration_key(height.revision_number(), height.revision_height()), + value: height.encode_vec(), + }); } Ok(Some(metadata)) diff --git a/ibc-clients/cw-context/src/lib.rs b/ibc-clients/cw-context/src/lib.rs index 792811815..b1f72df01 100644 --- a/ibc-clients/cw-context/src/lib.rs +++ b/ibc-clients/cw-context/src/lib.rs @@ -18,8 +18,6 @@ )] #![forbid(unsafe_code)] -extern crate alloc; - pub mod api; pub mod context; pub mod handlers; diff --git a/ibc-clients/cw-context/src/utils/mod.rs b/ibc-clients/cw-context/src/utils/mod.rs index 612a46d5c..38e96fd59 100644 --- a/ibc-clients/cw-context/src/utils/mod.rs +++ b/ibc-clients/cw-context/src/utils/mod.rs @@ -1,25 +1,3 @@ mod codec; pub use codec::*; -use ibc_core::client::types::error::ClientError; -use ibc_core::client::types::{Height, HeightError}; - -/// Decodes a `Height` from a UTF-8 encoded byte array. -pub fn parse_height(encoded_height: Vec) -> Result, ClientError> { - let height_str = match alloc::str::from_utf8(encoded_height.as_slice()) { - Ok(s) => s, - // In cases where the height is unavailable, the encoded representation - // might not be valid UTF-8, resulting in an invalid string. In such - // instances, we return None. - Err(_) => return Ok(None), - }; - match Height::try_from(height_str) { - Ok(height) => Ok(Some(height)), - // This is a valid case, as the key may contain other data. We just skip - // it. - Err(HeightError::InvalidFormat { .. }) => Ok(None), - Err(e) => Err(ClientError::Other { - description: e.to_string(), - }), - } -}