From d8de56920a7a7de169a88fd1b5fc505ec9d0685a Mon Sep 17 00:00:00 2001 From: elizabeth Date: Fri, 18 Oct 2024 15:20:48 -0400 Subject: [PATCH 1/4] get height from grpc metadata for relevant ibc queries --- .../core/component/ibc/src/component/rpc.rs | 1 + .../ibc/src/component/rpc/client_query.rs | 41 ++++++++++++- .../ibc/src/component/rpc/connection_query.rs | 21 ++++++- .../ibc/src/component/rpc/consensus_query.rs | 59 ++++++++++++++++++- .../component/ibc/src/component/rpc/utils.rs | 27 +++++++++ 5 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 crates/core/component/ibc/src/component/rpc/utils.rs diff --git a/crates/core/component/ibc/src/component/rpc.rs b/crates/core/component/ibc/src/component/rpc.rs index 4d125b1350..4b54f52105 100644 --- a/crates/core/component/ibc/src/component/rpc.rs +++ b/crates/core/component/ibc/src/component/rpc.rs @@ -6,6 +6,7 @@ use super::HostInterface; mod client_query; mod connection_query; mod consensus_query; +mod utils; use std::marker::PhantomData; diff --git a/crates/core/component/ibc/src/component/rpc/client_query.rs b/crates/core/component/ibc/src/component/rpc/client_query.rs index 9b52f35097..82eaa1bdf0 100644 --- a/crates/core/component/ibc/src/component/rpc/client_query.rs +++ b/crates/core/component/ibc/src/component/rpc/client_query.rs @@ -13,6 +13,7 @@ use ibc_proto::ibc::core::client::v1::{ }; use prost::Message; +use crate::component::rpc::utils::height_from_str; use ibc_types::core::client::ClientId; use ibc_types::core::client::Height; use ibc_types::path::ClientConsensusStatePath; @@ -34,7 +35,25 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, Status> { - let snapshot = self.storage.latest_snapshot(); + let Some(height_val) = request.metadata().get("height") else { + return Err(tonic::Status::aborted("missing height")); + }; + + let height_str: &str = height_val + .to_str() + .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; + + let snapshot = if height_str == "0" { + self.storage.latest_snapshot() + } else { + let height = height_from_str(height_str) + .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; + + self.storage + .snapshot(height.revision_height as u64) + .ok_or(tonic::Status::aborted(format!("invalid height")))? + }; + let client_id = ClientId::from_str(&request.get_ref().client_id) .map_err(|e| tonic::Status::invalid_argument(format!("invalid client id: {e}")))?; let height = Height { @@ -109,7 +128,25 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let Some(height_val) = request.metadata().get("height") else { + return Err(tonic::Status::aborted("missing height")); + }; + + let height_str: &str = height_val + .to_str() + .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; + + let snapshot = if height_str == "0" { + self.storage.latest_snapshot() + } else { + let height = height_from_str(height_str) + .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; + + self.storage + .snapshot(height.revision_height as u64) + .ok_or(tonic::Status::aborted(format!("invalid height")))? + }; + let client_id = ClientId::from_str(&request.get_ref().client_id) .map_err(|e| tonic::Status::invalid_argument(format!("invalid client id: {e}")))?; let height = if request.get_ref().latest_height { diff --git a/crates/core/component/ibc/src/component/rpc/connection_query.rs b/crates/core/component/ibc/src/component/rpc/connection_query.rs index ae98d9f381..4cf9c8009f 100644 --- a/crates/core/component/ibc/src/component/rpc/connection_query.rs +++ b/crates/core/component/ibc/src/component/rpc/connection_query.rs @@ -19,6 +19,7 @@ use ibc_types::DomainType; use prost::Message; use std::str::FromStr; +use crate::component::rpc::utils::height_from_str; use crate::component::{ConnectionStateReadExt, HostInterface}; use crate::prefix::MerklePrefixExt; use crate::IBC_COMMITMENT_PREFIX; @@ -33,7 +34,25 @@ impl ConnectionQuery for IbcQuery request: tonic::Request, ) -> std::result::Result, tonic::Status> { tracing::debug!("querying connection {:?}", request); - let snapshot = self.storage.latest_snapshot(); + let Some(height_val) = request.metadata().get("height") else { + return Err(tonic::Status::aborted("missing height")); + }; + + let height_str: &str = height_val + .to_str() + .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; + + let snapshot = if height_str == "0" { + self.storage.latest_snapshot() + } else { + let height = height_from_str(height_str) + .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; + + self.storage + .snapshot(height.revision_height as u64) + .ok_or(tonic::Status::aborted(format!("invalid height")))? + }; + let connection_id = &ConnectionId::from_str(&request.get_ref().connection_id) .map_err(|e| tonic::Status::aborted(format!("invalid connection id: {e}")))?; diff --git a/crates/core/component/ibc/src/component/rpc/consensus_query.rs b/crates/core/component/ibc/src/component/rpc/consensus_query.rs index 39221e2384..74de835849 100644 --- a/crates/core/component/ibc/src/component/rpc/consensus_query.rs +++ b/crates/core/component/ibc/src/component/rpc/consensus_query.rs @@ -29,6 +29,7 @@ use prost::Message; use std::str::FromStr; +use crate::component::rpc::utils::height_from_str; use crate::component::{ChannelStateReadExt, ConnectionStateReadExt, HostInterface}; use super::IbcQuery; @@ -349,7 +350,24 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let Some(height_val) = request.metadata().get("height") else { + return Err(tonic::Status::aborted("missing height")); + }; + + let height_str: &str = height_val + .to_str() + .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; + + let snapshot = if height_str == "0" { + self.storage.latest_snapshot() + } else { + let height = height_from_str(height_str) + .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; + + self.storage + .snapshot(height.revision_height as u64) + .ok_or(tonic::Status::aborted(format!("invalid height")))? + }; let port_id = PortId::from_str(&request.get_ref().port_id) .map_err(|e| tonic::Status::aborted(format!("invalid port id: {e}")))?; @@ -453,7 +471,24 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let Some(height_val) = request.metadata().get("height") else { + return Err(tonic::Status::aborted("missing height")); + }; + + let height_str: &str = height_val + .to_str() + .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; + + let snapshot = if height_str == "0" { + self.storage.latest_snapshot() + } else { + let height = height_from_str(height_str) + .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; + + self.storage + .snapshot(height.revision_height as u64) + .ok_or(tonic::Status::aborted(format!("invalid height")))? + }; let port_id = PortId::from_str(&request.get_ref().port_id) .map_err(|e| tonic::Status::aborted(format!("invalid port id: {e}")))?; @@ -488,7 +523,25 @@ impl ConsensusQuery for IbcQuery request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = self.storage.latest_snapshot(); + let Some(height_val) = request.metadata().get("height") else { + return Err(tonic::Status::aborted("missing height")); + }; + + let height_str: &str = height_val + .to_str() + .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; + + let snapshot = if height_str == "0" { + self.storage.latest_snapshot() + } else { + let height = height_from_str(height_str) + .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; + + self.storage + .snapshot(height.revision_height as u64) + .ok_or(tonic::Status::aborted(format!("invalid height")))? + }; + let channel_id = ChannelId::from_str(request.get_ref().channel_id.as_str()) .map_err(|e| tonic::Status::aborted(format!("invalid channel id: {e}")))?; let port_id = PortId::from_str(request.get_ref().port_id.as_str()) diff --git a/crates/core/component/ibc/src/component/rpc/utils.rs b/crates/core/component/ibc/src/component/rpc/utils.rs new file mode 100644 index 0000000000..108a14248a --- /dev/null +++ b/crates/core/component/ibc/src/component/rpc/utils.rs @@ -0,0 +1,27 @@ +use anyhow::anyhow; +use ibc_proto::ibc::core::client::v1::Height; + +pub(crate) fn height_from_str(value: &str) -> anyhow::Result { + let split: Vec<&str> = value.split('-').collect(); + + if split.len() != 2 { + return Err(anyhow!("invalid height string")); + } + + let revision_number = split[0] + .parse::() + .map_err(|e| anyhow!("failed to parse revision number"))?; + + let revision_height = split[1] + .parse::() + .map_err(|e| anyhow!("failed to parse revision height"))?; + + if revision_number == 0 && revision_height == 0 { + return Err(anyhow!("height is zero")); + } + + Ok(Height { + revision_number, + revision_height, + }) +} From f3c6c9f020d1b43d1b954ab54479d9d6a8119867 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 24 Oct 2024 14:10:29 +0200 Subject: [PATCH 2/4] streamline the fix --- .../ibc/src/component/rpc/client_query.rs | 46 ++---- .../ibc/src/component/rpc/connection_query.rs | 23 +-- .../ibc/src/component/rpc/consensus_query.rs | 68 +++------ .../component/ibc/src/component/rpc/utils.rs | 135 +++++++++++++++--- 4 files changed, 151 insertions(+), 121 deletions(-) diff --git a/crates/core/component/ibc/src/component/rpc/client_query.rs b/crates/core/component/ibc/src/component/rpc/client_query.rs index 82eaa1bdf0..dc03a655d5 100644 --- a/crates/core/component/ibc/src/component/rpc/client_query.rs +++ b/crates/core/component/ibc/src/component/rpc/client_query.rs @@ -13,7 +13,6 @@ use ibc_proto::ibc::core::client::v1::{ }; use prost::Message; -use crate::component::rpc::utils::height_from_str; use ibc_types::core::client::ClientId; use ibc_types::core::client::Height; use ibc_types::path::ClientConsensusStatePath; @@ -27,6 +26,7 @@ use crate::component::{ClientStateReadExt, HostInterface}; use crate::prefix::MerklePrefixExt; use crate::IBC_COMMITMENT_PREFIX; +use super::utils::determine_snapshot_from_height_header; use super::IbcQuery; #[async_trait] @@ -35,23 +35,11 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, Status> { - let Some(height_val) = request.metadata().get("height") else { - return Err(tonic::Status::aborted("missing height")); - }; - - let height_str: &str = height_val - .to_str() - .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; - - let snapshot = if height_str == "0" { - self.storage.latest_snapshot() - } else { - let height = height_from_str(height_str) - .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; - - self.storage - .snapshot(height.revision_height as u64) - .ok_or(tonic::Status::aborted(format!("invalid height")))? + let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, }; let client_id = ClientId::from_str(&request.get_ref().client_id) @@ -128,23 +116,11 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let Some(height_val) = request.metadata().get("height") else { - return Err(tonic::Status::aborted("missing height")); - }; - - let height_str: &str = height_val - .to_str() - .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; - - let snapshot = if height_str == "0" { - self.storage.latest_snapshot() - } else { - let height = height_from_str(height_str) - .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; - - self.storage - .snapshot(height.revision_height as u64) - .ok_or(tonic::Status::aborted(format!("invalid height")))? + let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, }; let client_id = ClientId::from_str(&request.get_ref().client_id) diff --git a/crates/core/component/ibc/src/component/rpc/connection_query.rs b/crates/core/component/ibc/src/component/rpc/connection_query.rs index 4cf9c8009f..699088fd8f 100644 --- a/crates/core/component/ibc/src/component/rpc/connection_query.rs +++ b/crates/core/component/ibc/src/component/rpc/connection_query.rs @@ -19,7 +19,7 @@ use ibc_types::DomainType; use prost::Message; use std::str::FromStr; -use crate::component::rpc::utils::height_from_str; +use crate::component::rpc::utils::determine_snapshot_from_height_header; use crate::component::{ConnectionStateReadExt, HostInterface}; use crate::prefix::MerklePrefixExt; use crate::IBC_COMMITMENT_PREFIX; @@ -34,23 +34,12 @@ impl ConnectionQuery for IbcQuery request: tonic::Request, ) -> std::result::Result, tonic::Status> { tracing::debug!("querying connection {:?}", request); - let Some(height_val) = request.metadata().get("height") else { - return Err(tonic::Status::aborted("missing height")); - }; - - let height_str: &str = height_val - .to_str() - .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; - - let snapshot = if height_str == "0" { - self.storage.latest_snapshot() - } else { - let height = height_from_str(height_str) - .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; - self.storage - .snapshot(height.revision_height as u64) - .ok_or(tonic::Status::aborted(format!("invalid height")))? + let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, }; let connection_id = &ConnectionId::from_str(&request.get_ref().connection_id) diff --git a/crates/core/component/ibc/src/component/rpc/consensus_query.rs b/crates/core/component/ibc/src/component/rpc/consensus_query.rs index 74de835849..54bbd1a22f 100644 --- a/crates/core/component/ibc/src/component/rpc/consensus_query.rs +++ b/crates/core/component/ibc/src/component/rpc/consensus_query.rs @@ -29,9 +29,9 @@ use prost::Message; use std::str::FromStr; -use crate::component::rpc::utils::height_from_str; use crate::component::{ChannelStateReadExt, ConnectionStateReadExt, HostInterface}; +use super::utils::determine_snapshot_from_height_header; use super::IbcQuery; #[async_trait] @@ -350,23 +350,11 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let Some(height_val) = request.metadata().get("height") else { - return Err(tonic::Status::aborted("missing height")); - }; - - let height_str: &str = height_val - .to_str() - .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; - - let snapshot = if height_str == "0" { - self.storage.latest_snapshot() - } else { - let height = height_from_str(height_str) - .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; - - self.storage - .snapshot(height.revision_height as u64) - .ok_or(tonic::Status::aborted(format!("invalid height")))? + let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, }; let port_id = PortId::from_str(&request.get_ref().port_id) @@ -471,23 +459,11 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let Some(height_val) = request.metadata().get("height") else { - return Err(tonic::Status::aborted("missing height")); - }; - - let height_str: &str = height_val - .to_str() - .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; - - let snapshot = if height_str == "0" { - self.storage.latest_snapshot() - } else { - let height = height_from_str(height_str) - .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; - - self.storage - .snapshot(height.revision_height as u64) - .ok_or(tonic::Status::aborted(format!("invalid height")))? + let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, }; let port_id = PortId::from_str(&request.get_ref().port_id) @@ -523,23 +499,11 @@ impl ConsensusQuery for IbcQuery request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let Some(height_val) = request.metadata().get("height") else { - return Err(tonic::Status::aborted("missing height")); - }; - - let height_str: &str = height_val - .to_str() - .map_err(|e| tonic::Status::aborted(format!("invalid height: {e}")))?; - - let snapshot = if height_str == "0" { - self.storage.latest_snapshot() - } else { - let height = height_from_str(height_str) - .map_err(|e| tonic::Status::aborted(format!("couldn't get snapshot: {e}")))?; - - self.storage - .snapshot(height.revision_height as u64) - .ok_or(tonic::Status::aborted(format!("invalid height")))? + let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + Err(err) => return Err(tonic::Status::aborted( + format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") + )), + Ok(snapshot) => snapshot, }; let channel_id = ChannelId::from_str(request.get_ref().channel_id.as_str()) diff --git a/crates/core/component/ibc/src/component/rpc/utils.rs b/crates/core/component/ibc/src/component/rpc/utils.rs index 108a14248a..5bb78f0fad 100644 --- a/crates/core/component/ibc/src/component/rpc/utils.rs +++ b/crates/core/component/ibc/src/component/rpc/utils.rs @@ -1,27 +1,128 @@ -use anyhow::anyhow; +use std::str::FromStr; + +use anyhow::bail; +use anyhow::Context as _; +use cnidarium::Snapshot; +use cnidarium::Storage; use ibc_proto::ibc::core::client::v1::Height; -pub(crate) fn height_from_str(value: &str) -> anyhow::Result { - let split: Vec<&str> = value.split('-').collect(); +pub(in crate::component::rpc) fn determine_snapshot_from_height_header( + storage: Storage, + request: &tonic::Request, +) -> anyhow::Result { + let height_entry = request + .metadata() + .get("height") + .context("no `height` header")? + .to_str() + .context("value of `height` header was not ASCII")?; + + 'state: { + let height = match parse_as_ibc_height(height_entry) + .context("failed to parse value as IBC height") + { + Err(err) => break 'state Err(err), + Ok(height) => height.revision_height, + }; + if height == 0 { + Ok(storage.latest_snapshot()) + } else { + storage + .snapshot(height) + .with_context(|| format!("could not open snapshot at revision height `{height}`")) + } + } + .with_context(|| { + format!("failed determine snapshot from `\"height\": \"{height_entry}` header") + }) +} + +/// Utility to implement +#[derive(Debug)] +struct TheHeight(Height); - if split.len() != 2 { - return Err(anyhow!("invalid height string")); +impl TheHeight { + fn zero() -> Self { + Self(Height { + revision_number: 0, + revision_height: 0, + }) } + fn into_inner(self) -> Height { + self.0 + } +} + +impl FromStr for TheHeight { + type Err = anyhow::Error; - let revision_number = split[0] - .parse::() - .map_err(|e| anyhow!("failed to parse revision number"))?; + fn from_str(input: &str) -> Result { + const FORM: &str = "input was not of the form '0' or '-'"; - let revision_height = split[1] - .parse::() - .map_err(|e| anyhow!("failed to parse revision height"))?; + let mut parts = input.split('-'); - if revision_number == 0 && revision_height == 0 { - return Err(anyhow!("height is zero")); + let revision_number = parts + .next() + .context(FORM)? + .parse::() + .context("failed to parse revision number as u64")?; + let revision_height = match parts.next() { + None if revision_number == 0 => return Ok(Self::zero()), + None => bail!(FORM), + Some(rev_height) => rev_height + .parse::() + .context("failed to parse revision height as u64")?, + }; + + Ok(TheHeight(Height { + revision_number, + revision_height, + })) } +} - Ok(Height { - revision_number, - revision_height, - }) +fn parse_as_ibc_height(input: &str) -> anyhow::Result { + let height = input + .trim() + .parse::() + .context("failed to parse as IBC height")? + .into_inner(); + + Ok(height) +} + +#[cfg(test)] +mod tests { + use ibc_proto::ibc::core::client::v1::Height; + + use super::TheHeight; + + fn zero() -> Height { + Height { + revision_number: 0, + revision_height: 0, + } + } + + fn height(revision_number: u64, revision_height: u64) -> Height { + Height { + revision_number, + revision_height, + } + } + + #[track_caller] + fn assert_ibc_height_is_parsed_correctly(input: &str, expected: Height) { + let actual = input.parse::().unwrap().into_inner(); + assert_eq!(expected, actual); + } + + #[test] + fn parse_ibc_height() { + assert_ibc_height_is_parsed_correctly("0", zero()); + assert_ibc_height_is_parsed_correctly("0-0", zero()); + assert_ibc_height_is_parsed_correctly("0-1", height(0, 1)); + assert_ibc_height_is_parsed_correctly("1-0", height(1, 0)); + assert_ibc_height_is_parsed_correctly("1-1", height(1, 1)); + } } From 998434fce8c1d7ad36b1420bce195f0a62f70c1b Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 24 Oct 2024 14:59:17 +0200 Subject: [PATCH 3/4] provide trace information --- .../component/ibc/src/component/rpc/utils.rs | 42 +++++++++---------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/crates/core/component/ibc/src/component/rpc/utils.rs b/crates/core/component/ibc/src/component/rpc/utils.rs index 5bb78f0fad..e69aa39762 100644 --- a/crates/core/component/ibc/src/component/rpc/utils.rs +++ b/crates/core/component/ibc/src/component/rpc/utils.rs @@ -5,36 +5,32 @@ use anyhow::Context as _; use cnidarium::Snapshot; use cnidarium::Storage; use ibc_proto::ibc::core::client::v1::Height; +use tracing::debug; +use tracing::instrument; +#[instrument(skip_all, level = "debug")] pub(in crate::component::rpc) fn determine_snapshot_from_height_header( storage: Storage, request: &tonic::Request, ) -> anyhow::Result { - let height_entry = request - .metadata() - .get("height") - .context("no `height` header")? - .to_str() - .context("value of `height` header was not ASCII")?; - - 'state: { - let height = match parse_as_ibc_height(height_entry) - .context("failed to parse value as IBC height") - { - Err(err) => break 'state Err(err), - Ok(height) => height.revision_height, - }; - if height == 0 { - Ok(storage.latest_snapshot()) - } else { - storage - .snapshot(height) - .with_context(|| format!("could not open snapshot at revision height `{height}`")) + let height = match request.metadata().get("height") { + None => { + debug!("height header was missing; assuming a height of 0"); + TheHeight::zero().into_inner() } + Some(entry) => entry + .to_str() + .context("height header was present but its entry was not ASCII") + .and_then(parse_as_ibc_height) + .context("failed to height header as IBC height")?, + }; + if height.revision_height == 0 { + Ok(storage.latest_snapshot()) + } else { + storage + .snapshot(height.revision_height) + .context("failed to create state snapshot from IBC height in height header") } - .with_context(|| { - format!("failed determine snapshot from `\"height\": \"{height_entry}` header") - }) } /// Utility to implement From 1607f78dbe8bd14989551dec16ed30f95b3367c7 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 24 Oct 2024 15:31:27 +0200 Subject: [PATCH 4/4] more tests --- .../ibc/src/component/rpc/client_query.rs | 6 +- .../ibc/src/component/rpc/connection_query.rs | 4 +- .../ibc/src/component/rpc/consensus_query.rs | 8 +-- .../component/ibc/src/component/rpc/utils.rs | 62 +++++++++++++++---- 4 files changed, 58 insertions(+), 22 deletions(-) diff --git a/crates/core/component/ibc/src/component/rpc/client_query.rs b/crates/core/component/ibc/src/component/rpc/client_query.rs index dc03a655d5..6865b23b76 100644 --- a/crates/core/component/ibc/src/component/rpc/client_query.rs +++ b/crates/core/component/ibc/src/component/rpc/client_query.rs @@ -26,7 +26,7 @@ use crate::component::{ClientStateReadExt, HostInterface}; use crate::prefix::MerklePrefixExt; use crate::IBC_COMMITMENT_PREFIX; -use super::utils::determine_snapshot_from_height_header; +use super::utils::determine_snapshot_from_metadata; use super::IbcQuery; #[async_trait] @@ -35,7 +35,7 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, Status> { - let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { Err(err) => return Err(tonic::Status::aborted( format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") )), @@ -116,7 +116,7 @@ impl ClientQuery for IbcQuery { &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { Err(err) => return Err(tonic::Status::aborted( format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") )), diff --git a/crates/core/component/ibc/src/component/rpc/connection_query.rs b/crates/core/component/ibc/src/component/rpc/connection_query.rs index 699088fd8f..2046b8be98 100644 --- a/crates/core/component/ibc/src/component/rpc/connection_query.rs +++ b/crates/core/component/ibc/src/component/rpc/connection_query.rs @@ -19,7 +19,7 @@ use ibc_types::DomainType; use prost::Message; use std::str::FromStr; -use crate::component::rpc::utils::determine_snapshot_from_height_header; +use crate::component::rpc::utils::determine_snapshot_from_metadata; use crate::component::{ConnectionStateReadExt, HostInterface}; use crate::prefix::MerklePrefixExt; use crate::IBC_COMMITMENT_PREFIX; @@ -35,7 +35,7 @@ impl ConnectionQuery for IbcQuery ) -> std::result::Result, tonic::Status> { tracing::debug!("querying connection {:?}", request); - let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { Err(err) => return Err(tonic::Status::aborted( format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") )), diff --git a/crates/core/component/ibc/src/component/rpc/consensus_query.rs b/crates/core/component/ibc/src/component/rpc/consensus_query.rs index 54bbd1a22f..f8d58dc681 100644 --- a/crates/core/component/ibc/src/component/rpc/consensus_query.rs +++ b/crates/core/component/ibc/src/component/rpc/consensus_query.rs @@ -31,7 +31,7 @@ use std::str::FromStr; use crate::component::{ChannelStateReadExt, ConnectionStateReadExt, HostInterface}; -use super::utils::determine_snapshot_from_height_header; +use super::utils::determine_snapshot_from_metadata; use super::IbcQuery; #[async_trait] @@ -350,7 +350,7 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { Err(err) => return Err(tonic::Status::aborted( format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") )), @@ -459,7 +459,7 @@ impl ConsensusQuery for IbcQuery &self, request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { Err(err) => return Err(tonic::Status::aborted( format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") )), @@ -499,7 +499,7 @@ impl ConsensusQuery for IbcQuery request: tonic::Request, ) -> std::result::Result, tonic::Status> { - let snapshot = match determine_snapshot_from_height_header(self.storage.clone(), &request) { + let snapshot = match determine_snapshot_from_metadata(self.storage.clone(), request.metadata()) { Err(err) => return Err(tonic::Status::aborted( format!("could not determine the correct snapshot to open given the `\"height\"` header of the request: {err:#}") )), diff --git a/crates/core/component/ibc/src/component/rpc/utils.rs b/crates/core/component/ibc/src/component/rpc/utils.rs index e69aa39762..642d4bcff2 100644 --- a/crates/core/component/ibc/src/component/rpc/utils.rs +++ b/crates/core/component/ibc/src/component/rpc/utils.rs @@ -8,32 +8,45 @@ use ibc_proto::ibc::core::client::v1::Height; use tracing::debug; use tracing::instrument; +type Type = tonic::metadata::MetadataMap; + +/// Determines which state snapshot to open given the height header in a [`MetadataMap`]. +/// +/// Returns the latest snapshot if the height header is 0, 0-0, or absent. #[instrument(skip_all, level = "debug")] -pub(in crate::component::rpc) fn determine_snapshot_from_height_header( +pub(in crate::component::rpc) fn determine_snapshot_from_metadata( storage: Storage, - request: &tonic::Request, + metadata: &Type, ) -> anyhow::Result { - let height = match request.metadata().get("height") { + let height = determine_height_from_metadata(metadata) + .context("failed to determine height from metadata")?; + if height.revision_height == 0 { + Ok(storage.latest_snapshot()) + } else { + storage + .snapshot(height.revision_height) + .context("failed to create state snapshot from IBC height in height header") + } +} + +#[instrument(skip_all, level = "debug")] +fn determine_height_from_metadata( + metadata: &tonic::metadata::MetadataMap, +) -> anyhow::Result { + match metadata.get("height") { None => { debug!("height header was missing; assuming a height of 0"); - TheHeight::zero().into_inner() + Ok(TheHeight::zero().into_inner()) } Some(entry) => entry .to_str() .context("height header was present but its entry was not ASCII") .and_then(parse_as_ibc_height) - .context("failed to height header as IBC height")?, - }; - if height.revision_height == 0 { - Ok(storage.latest_snapshot()) - } else { - storage - .snapshot(height.revision_height) - .context("failed to create state snapshot from IBC height in height header") + .context("failed to parse height header as IBC height"), } } -/// Utility to implement +/// Newtype wrapper around [`Height`] to implement [`FromStr`]. #[derive(Debug)] struct TheHeight(Height); @@ -90,6 +103,9 @@ fn parse_as_ibc_height(input: &str) -> anyhow::Result { #[cfg(test)] mod tests { use ibc_proto::ibc::core::client::v1::Height; + use tonic::metadata::MetadataMap; + + use crate::component::rpc::utils::determine_height_from_metadata; use super::TheHeight; @@ -121,4 +137,24 @@ mod tests { assert_ibc_height_is_parsed_correctly("1-0", height(1, 0)); assert_ibc_height_is_parsed_correctly("1-1", height(1, 1)); } + + #[track_caller] + fn assert_ibc_height_is_determined_correctly(input: Option<&str>, expected: Height) { + let mut metadata = MetadataMap::new(); + if let Some(input) = input { + metadata.insert("height", input.parse().unwrap()); + } + let actual = determine_height_from_metadata(&metadata).unwrap(); + assert_eq!(expected, actual); + } + + #[test] + fn determine_ibc_height_from_metadata() { + assert_ibc_height_is_determined_correctly(None, zero()); + assert_ibc_height_is_determined_correctly(Some("0"), zero()); + assert_ibc_height_is_determined_correctly(Some("0-0"), zero()); + assert_ibc_height_is_determined_correctly(Some("0-1"), height(0, 1)); + assert_ibc_height_is_determined_correctly(Some("1-0"), height(1, 0)); + assert_ibc_height_is_determined_correctly(Some("1-1"), height(1, 1)); + } }