diff --git a/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md new file mode 100644 index 000000000..cf89e9a52 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md @@ -0,0 +1,5 @@ +- [ibc-app-transfer] Bring `PrefixedDenom` parsing up to parity with `ibc-go`. + ([\#1177](https://github.com/cosmos/ibc-rs/issues/1177)) +- [ibc-app-nft-transfer] Reuse `TracePrefix` and `TracePath` from + `ibc-app-transfer-types` when parsing `PrefixedClassId`. + ([\#1178](https://github.com/cosmos/ibc-rs/pull/1178)) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 65b38fe67..8ac632fea 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -83,6 +83,33 @@ impl TracePrefix { channel_id, } } + + /// Returns a string slice with [`TracePrefix`] removed. + /// + /// If the string starts with a [`TracePrefix`], i.e. `{port-id}/channel-{id}`, + /// it returns a tuple of the removed [`TracePrefix`] and the substring after the prefix. + /// + /// If the substring is empty, it returns `None`. + /// Otherwise the substring starts with `/`. In that case, + /// the leading `/` is stripped and returned. + /// + /// If the string does not start with a [`TracePrefix`], this method returns `None`. + /// + /// This method is analogous to `strip_prefix` from the standard library. + pub fn strip(s: &str) -> Option<(Self, Option<&str>)> { + // The below two chained `split_once` calls emulate a virtual `split_twice` call, + // which is not available in the standard library. + let (port_id_s, remaining) = s.split_once('/')?; + let (channel_id_s, remaining) = remaining + .split_once('/') + .map(|(a, b)| (a, Some(b))) + .unwrap_or_else(|| (remaining, None)); + + let port_id = port_id_s.parse().ok()?; + let channel_id = channel_id_s.parse().ok()?; + + Some((Self::new(port_id, channel_id), remaining)) + } } impl Display for TracePrefix { @@ -92,9 +119,10 @@ impl Display for TracePrefix { } /// A full trace path modelled as a collection of `TracePrefix`s. -// Internally, the `TracePath` is modelled as a `Vec` but with the order reversed, i.e. -// "transfer/channel-0/transfer/channel-1/uatom" => `["transfer/channel-1", "transfer/channel-0"]` -// This is done for ease of addition/removal of prefixes. +/// +/// Internally, the `TracePath` is modelled as a `Vec` but with the order reversed, i.e. +/// "transfer/channel-0/transfer/channel-1/uatom" => `["transfer/channel-1", "transfer/channel-0"]` +/// This is done for ease of addition/removal of prefixes. #[cfg_attr( feature = "parity-scale-codec", derive( @@ -139,39 +167,43 @@ impl TracePath { pub fn empty() -> Self { Self(vec![]) } -} -impl<'a> TryFrom> for TracePath { - type Error = TokenTransferError; - - fn try_from(v: Vec<&'a str>) -> Result { - if v.len() % 2 != 0 { - return Err(TokenTransferError::InvalidTraceLength { - len: v.len() as u64, - }); - } - - let mut trace = vec![]; - let id_pairs = v.chunks_exact(2).map(|paths| (paths[0], paths[1])); - for (pos, (port_id, channel_id)) in id_pairs.rev().enumerate() { - let port_id = - PortId::from_str(port_id).map_err(|e| TokenTransferError::InvalidTracePortId { - pos: pos as u64, - validation_error: e, - })?; - let channel_id = ChannelId::from_str(channel_id).map_err(|e| { - TokenTransferError::InvalidTraceChannelId { - pos: pos as u64, - validation_error: e, - } - })?; - trace.push(TracePrefix { - port_id, - channel_id, - }); + /// Returns a string slice with [`TracePath`] or all [`TracePrefix`]es repeatedly removed. + /// + /// If the string starts with a [`TracePath`], it returns a tuple of the removed + /// [`TracePath`] and the substring after the [`TracePath`]. + /// + /// If the substring is empty, it returns `None`. + /// Otherwise the substring starts with `/`. In that case, + /// the leading `/` is stripped and returned. + /// + /// If the string does not contain any [`TracePrefix`], it returns the original string. + /// + /// This method is analogous to `trim_start_matches` from the standard library. + pub fn trim(s: &str) -> (Self, Option<&str>) { + // We can't use `TracePrefix::empty()` with `TracePrefix::add_prefix()`. + // Because we are stripping prefixes in reverse order. + let mut trace_prefixes = vec![]; + let mut current_remaining_opt = Some(s); + + loop { + let Some(current_remaining_s) = current_remaining_opt else { + break; + }; + + let Some((trace_prefix, next_remaining_opt)) = TracePrefix::strip(current_remaining_s) + else { + break; + }; + + trace_prefixes.push(trace_prefix); + current_remaining_opt = next_remaining_opt; } - Ok(trace.into()) + // Reversing is needed, as [`TracePath`] requires quick addition/removal + // of prefixes which is more performant from the end of a [`Vec`]. + trace_prefixes.reverse(); + (Self(trace_prefixes), current_remaining_opt) } } @@ -179,15 +211,15 @@ impl FromStr for TracePath { type Err = TokenTransferError; fn from_str(s: &str) -> Result { - let parts = { - let parts: Vec<&str> = s.split('/').collect(); - if parts.len() == 1 && parts[0].trim().is_empty() { - vec![] - } else { - parts - } - }; - parts.try_into() + if s.is_empty() { + return Ok(TracePath::empty()); + } + + let (trace_path, remaining_parts) = TracePath::trim(s); + remaining_parts + .is_none() + .then_some(trace_path) + .ok_or_else(|| TokenTransferError::MalformedTrace(s.to_string())) } } @@ -293,24 +325,38 @@ pub fn is_receiver_chain_source( impl FromStr for PrefixedDenom { type Err = TokenTransferError; + /// Initializes a [`PrefixedDenom`] from a string that adheres to the format + /// `{nth-port-id/channel-}/{(n-1)th-port-id/channel-}/.../{1st-port-id/channel-}/`. + /// A [`PrefixedDenom`] exhibits a sequence of `{ith-port-id/channel-}` pairs. + /// This sequence makes up the [`TracePath`] of the [`PrefixedDenom`]. + /// + /// This [`PrefixedDenom::from_str`] implementation _left-split-twice_ the argument string + /// using `/` delimiter. Then it peeks into the first two segments and attempts to convert + /// the first segment into a [`PortId`] and the second into a [`ChannelId`]. + /// This continues on the third remaining segment in a loop until a + /// `{port-id/channel-id}` pair cannot be created from the top two segments. + /// The remaining parts of the string are then considered the [`BaseDenom`]. + /// + /// For example, given the following denom trace: + /// "transfer/channel-75/factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust", + /// the first two `/`-delimited segments are `"transfer"` and `"channel-75"`. The + /// first is a valid [`PortId`], and the second is a valid [`ChannelId`], so that becomes + /// the first `{port-id/channel-id}` pair that gets added as part of the [`TracePath`] + /// of the [`PrefixedDenom`]. The next two segments are `"factory"`, a + /// valid [`PortId`], and `"stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4"`, an invalid [`ChannelId`]. + /// The loop breaks at this point, resulting in a [`TracePath`] of `"transfer/channel-75"` + /// and a [`BaseDenom`] of `"factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust"`. fn from_str(s: &str) -> Result { - let mut parts: Vec<&str> = s.split('/').collect(); - let last_part = parts.pop().expect("split() returned an empty iterator"); - - let (base_denom, trace_path) = { - if last_part == s { - (BaseDenom::from_str(s)?, TracePath::empty()) - } else { - let base_denom = BaseDenom::from_str(last_part)?; - let trace_path = TracePath::try_from(parts)?; - (base_denom, trace_path) - } - }; - - Ok(Self { - trace_path, - base_denom, - }) + match TracePath::trim(s) { + (trace_path, Some(remaining_parts)) => Ok(Self { + trace_path, + base_denom: BaseDenom::from_str(remaining_parts)?, + }), + (_, None) => Ok(Self { + trace_path: TracePath::empty(), + base_denom: BaseDenom::from_str(s)?, + }), + } } } @@ -357,80 +403,217 @@ impl Display for PrefixedDenom { #[cfg(test)] mod tests { + use rstest::rstest; + use super::*; - #[test] - fn test_denom_validation() -> Result<(), TokenTransferError> { - assert!(BaseDenom::from_str("").is_err(), "empty base denom"); - assert!(BaseDenom::from_str("uatom").is_ok(), "valid base denom"); - assert!(PrefixedDenom::from_str("").is_err(), "empty denom trace"); - assert!( - PrefixedDenom::from_str("transfer/channel-0/").is_err(), - "empty base denom with trace" - ); - assert!(PrefixedDenom::from_str("/uatom").is_err(), "empty prefix"); - assert!(PrefixedDenom::from_str("//uatom").is_err(), "empty ids"); - assert!( - PrefixedDenom::from_str("transfer/").is_err(), - "single trace" - ); - assert!( - PrefixedDenom::from_str("transfer/atom").is_err(), - "single trace with base denom" + #[rstest] + #[case("transfer")] + #[case("transfer/channel-1/ica")] + fn test_invalid_raw_demon_trace_parsing(#[case] trace_path: &str) { + let raw_denom_trace = RawDenomTrace { + path: trace_path.to_string(), + base_denom: "uatom".to_string(), + }; + + PrefixedDenom::try_from(raw_denom_trace).expect_err("failure"); + } + + #[rstest] + #[case("uatom")] + #[case("atom")] + fn test_accepted_denom(#[case] denom_str: &str) { + BaseDenom::from_str(denom_str).expect("success"); + } + + #[rstest] + #[case("")] + #[case(" ")] + fn test_rejected_denom(#[case] denom_str: &str) { + BaseDenom::from_str(denom_str).expect_err("failure"); + } + + #[rstest] + #[case( + "transfer/channel-75", + "factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust" + )] + #[case( + "transfer/channel-75/transfer/channel-123/transfer/channel-1023/transfer/channel-0", + "factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust" + )] + #[case( + "transfer/channel-75/transfer/channel-123/transfer/channel-1023/transfer/channel-0", + "//////////////////////dust" + )] + #[case("transfer/channel-0", "uatom")] + #[case("transfer/channel-0/transfer/channel-1", "uatom")] + #[case("", "/")] + #[case("", "transfer/uatom")] + #[case("", "transfer/atom")] + #[case("", "transfer//uatom")] + #[case("", "/uatom")] + #[case("", "//uatom")] + #[case("", "transfer/")] + #[case("", "(transfer)/channel-0/uatom")] + #[case("", "transfer/(channel-0)/uatom")] + // https://github.com/cosmos/ibc-go/blob/e2ad31975f2ede592912b86346b5ebf055c9e05f/modules/apps/transfer/types/trace_test.go#L17-L38 + #[case("", "uatom")] + #[case("", "uatom/")] + #[case("", "gamm/pool/1")] + #[case("", "gamm//pool//1")] + #[case("transfer/channel-1", "uatom")] + #[case("customtransfer/channel-1", "uatom")] + #[case("transfer/channel-1", "uatom/")] + #[case( + "transfer/channel-1", + "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA" + )] + #[case("transfer/channel-1", "gamm/pool/1")] + #[case("transfer/channel-1", "gamm//pool//1")] + #[case("transfer/channel-1/transfer/channel-2", "uatom")] + #[case("customtransfer/channel-1/alternativetransfer/channel-2", "uatom")] + #[case("", "transfer/uatom")] + #[case("", "transfer//uatom")] + #[case("", "channel-1/transfer/uatom")] + #[case("", "uatom/transfer")] + #[case("", "transfer/channel-1")] + #[case("transfer/channel-1", "transfer")] + #[case("", "transfer/channelToA/uatom")] + fn test_strange_but_accepted_prefixed_denom( + #[case] prefix: &str, + #[case] denom: &str, + ) -> Result<(), TokenTransferError> { + let pd_s = if prefix.is_empty() { + denom.to_owned() + } else { + format!("{prefix}/{denom}") + }; + let pd = PrefixedDenom::from_str(&pd_s)?; + + assert_eq!(pd.to_string(), pd_s); + assert_eq!(pd.trace_path.to_string(), prefix); + assert_eq!(pd.base_denom.to_string(), denom); + + Ok(()) + } + + #[rstest] + #[case("")] + #[case(" ")] + #[case("transfer/channel-1/")] + #[case("transfer/channel-1/transfer/channel-2/")] + #[case("transfer/channel-21/transfer/channel-23/ ")] + #[case("transfer/channel-0/")] + #[should_panic(expected = "EmptyBaseDenom")] + fn test_prefixed_empty_base_denom(#[case] pd_s: &str) { + PrefixedDenom::from_str(pd_s).expect("error"); + } + + #[rstest] + fn test_trace_path_order() { + let mut prefixed_denom = + PrefixedDenom::from_str("customtransfer/channel-1/alternativetransfer/channel-2/uatom") + .expect("no error"); + + assert_eq!( + prefixed_denom.trace_path.to_string(), + "customtransfer/channel-1/alternativetransfer/channel-2" ); - assert!( - PrefixedDenom::from_str("transfer/channel-0/uatom").is_ok(), - "valid single trace info" + assert_eq!(prefixed_denom.base_denom.to_string(), "uatom"); + + let trace_prefix_1 = TracePrefix::new( + "alternativetransfer".parse().unwrap(), + "channel-2".parse().unwrap(), ); - assert!( - PrefixedDenom::from_str("transfer/channel-0/transfer/channel-1/uatom").is_ok(), - "valid multiple trace info" + + let trace_prefix_2 = TracePrefix::new( + "customtransfer".parse().unwrap(), + "channel-1".parse().unwrap(), ); - assert!( - PrefixedDenom::from_str("(transfer)/channel-0/uatom").is_err(), - "invalid port" + + let trace_prefix_3 = + TracePrefix::new("transferv2".parse().unwrap(), "channel-10".parse().unwrap()); + let trace_prefix_4 = TracePrefix::new( + "transferv3".parse().unwrap(), + "channel-101".parse().unwrap(), ); - assert!( - PrefixedDenom::from_str("transfer/(channel-0)/uatom").is_err(), - "invalid channel" + + prefixed_denom.trace_path.add_prefix(trace_prefix_3.clone()); + + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_1)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_2)); + assert!(prefixed_denom.trace_path.starts_with(&trace_prefix_3)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_4)); + + assert_eq!( + prefixed_denom.to_string(), + "transferv2/channel-10/customtransfer/channel-1/alternativetransfer/channel-2/uatom" ); - Ok(()) - } + prefixed_denom.trace_path.remove_prefix(&trace_prefix_4); - #[test] - fn test_denom_trace() -> Result<(), TokenTransferError> { + assert!(!prefixed_denom.trace_path.is_empty()); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_1)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_2)); + assert!(prefixed_denom.trace_path.starts_with(&trace_prefix_3)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_4)); assert_eq!( - PrefixedDenom::from_str("transfer/channel-0/uatom")?, - PrefixedDenom { - trace_path: "transfer/channel-0".parse()?, - base_denom: "uatom".parse()? - }, - "valid single trace info" + prefixed_denom.to_string(), + "transferv2/channel-10/customtransfer/channel-1/alternativetransfer/channel-2/uatom" ); + + prefixed_denom.trace_path.remove_prefix(&trace_prefix_3); + + assert!(!prefixed_denom.trace_path.is_empty()); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_1)); + assert!(prefixed_denom.trace_path.starts_with(&trace_prefix_2)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_3)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_4)); assert_eq!( - PrefixedDenom::from_str("transfer/channel-0/transfer/channel-1/uatom")?, - PrefixedDenom { - trace_path: "transfer/channel-0/transfer/channel-1".parse()?, - base_denom: "uatom".parse()? - }, - "valid multiple trace info" + prefixed_denom.to_string(), + "customtransfer/channel-1/alternativetransfer/channel-2/uatom" ); - Ok(()) - } + prefixed_denom.trace_path.remove_prefix(&trace_prefix_2); - #[test] - fn test_denom_serde() -> Result<(), TokenTransferError> { - let dt_str = "transfer/channel-0/uatom"; - let dt = PrefixedDenom::from_str(dt_str)?; - assert_eq!(dt.to_string(), dt_str, "valid single trace info"); + assert!(!prefixed_denom.trace_path.is_empty()); + assert!(prefixed_denom.trace_path.starts_with(&trace_prefix_1)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_2)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_3)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_4)); + assert_eq!( + prefixed_denom.to_string(), + "alternativetransfer/channel-2/uatom" + ); - let dt_str = "transfer/channel-0/transfer/channel-1/uatom"; - let dt = PrefixedDenom::from_str(dt_str)?; - assert_eq!(dt.to_string(), dt_str, "valid multiple trace info"); + prefixed_denom.trace_path.remove_prefix(&trace_prefix_1); - Ok(()) + assert!(prefixed_denom.trace_path.is_empty()); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_1)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_2)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_3)); + assert!(!prefixed_denom.trace_path.starts_with(&trace_prefix_4)); + assert_eq!(prefixed_denom.to_string(), "uatom"); + } + + #[rstest] + #[case("", TracePath::empty(), Some(""))] + #[case("transfer", TracePath::empty(), Some("transfer"))] + #[case("transfer/", TracePath::empty(), Some("transfer/"))] + #[case("transfer/channel-1", TracePath::from(vec![TracePrefix::new("transfer".parse().unwrap(), ChannelId::new(1))]), None)] + #[case("transfer/channel-1/", TracePath::from(vec![TracePrefix::new("transfer".parse().unwrap(), ChannelId::new(1))]), Some(""))] + #[case("transfer/channel-1/uatom", TracePath::from(vec![TracePrefix::new("transfer".parse().unwrap(), ChannelId::new(1))]), Some("uatom"))] + #[case("transfer/channel-1/uatom/", TracePath::from(vec![TracePrefix::new("transfer".parse().unwrap(), ChannelId::new(1))]), Some("uatom/"))] + fn test_trace_path_cases( + #[case] trace_path_s: &str, + #[case] trace_path: TracePath, + #[case] remaining: Option<&str>, + ) { + let (parsed_trace_path, parsed_remaining) = TracePath::trim(trace_path_s); + + assert_eq!(parsed_trace_path, trace_path); + assert_eq!(parsed_remaining, remaining); } #[test] diff --git a/ibc-apps/ics20-transfer/types/src/error.rs b/ibc-apps/ics20-transfer/types/src/error.rs index 257fbc29d..7bd938408 100644 --- a/ibc-apps/ics20-transfer/types/src/error.rs +++ b/ibc-apps/ics20-transfer/types/src/error.rs @@ -39,6 +39,8 @@ pub enum TokenTransferError { pos: u64, validation_error: IdentifierError, }, + /// malformed trace: `{0}` + MalformedTrace(String), /// trace length must be even but got: `{len}` InvalidTraceLength { len: u64 }, /// invalid amount error: `{0}` diff --git a/ibc-apps/ics721-nft-transfer/types/Cargo.toml b/ibc-apps/ics721-nft-transfer/types/Cargo.toml index 3b5189b92..8b7d27b91 100644 --- a/ibc-apps/ics721-nft-transfer/types/Cargo.toml +++ b/ibc-apps/ics721-nft-transfer/types/Cargo.toml @@ -30,8 +30,9 @@ serde = { workspace = true, optional = true } serde_json = { workspace = true } # ibc dependencies -ibc-core = { workspace = true } -ibc-proto = { workspace = true } +ibc-core = { workspace = true } +ibc-proto = { workspace = true } +ibc-app-transfer-types = { workspace = true } ## parity dependencies parity-scale-codec = { workspace = true , optional = true } @@ -50,27 +51,32 @@ std = [ "http/std", "ibc-core/std", "ibc-proto/std", + "ibc-app-transfer-types/std", ] serde = [ "dep:serde", "ibc-core/serde", "ibc-proto/serde", + "ibc-app-transfer-types/serde", ] schema = [ "dep:schemars", "ibc-core/schema", "ibc-proto/json-schema", "serde", - "std" + "std", + "ibc-app-transfer-types/schema", ] borsh = [ "dep:borsh", "ibc-core/borsh", - "ibc-proto/borsh" + "ibc-proto/borsh", + "ibc-app-transfer-types/borsh", ] parity-scale-codec = [ "dep:parity-scale-codec", "dep:scale-info", "ibc-core/parity-scale-codec", - "ibc-proto/parity-scale-codec" + "ibc-proto/parity-scale-codec", + "ibc-app-transfer-types/parity-scale-codec", ] diff --git a/ibc-apps/ics721-nft-transfer/types/src/class.rs b/ibc-apps/ics721-nft-transfer/types/src/class.rs index c2ff9afd4..ff1293f89 100644 --- a/ibc-apps/ics721-nft-transfer/types/src/class.rs +++ b/ibc-apps/ics721-nft-transfer/types/src/class.rs @@ -2,8 +2,8 @@ use core::fmt::{self, Display, Error as FmtError, Formatter}; use core::str::FromStr; -use derive_more::From; use http::Uri; +pub use ibc_app_transfer_types::{TracePath, TracePrefix}; use ibc_core::host::types::identifiers::{ChannelId, PortId}; use ibc_core::primitives::prelude::*; #[cfg(feature = "serde")] @@ -55,152 +55,6 @@ impl FromStr for ClassId { } } -/// Class prefix, the same as ICS-20 TracePrefix -#[cfg_attr( - feature = "parity-scale-codec", - derive( - parity_scale_codec::Encode, - parity_scale_codec::Decode, - scale_info::TypeInfo - ) -)] -#[cfg_attr( - feature = "borsh", - derive(borsh::BorshSerialize, borsh::BorshDeserialize) -)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] -#[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] -pub struct TracePrefix { - port_id: PortId, - channel_id: ChannelId, -} - -impl TracePrefix { - pub fn new(port_id: PortId, channel_id: ChannelId) -> Self { - Self { - port_id, - channel_id, - } - } -} - -impl Display for TracePrefix { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { - write!(f, "{}/{}", self.port_id, self.channel_id) - } -} - -/// Class trace path, the same as ICS-20 TracePath -#[cfg_attr( - feature = "parity-scale-codec", - derive( - parity_scale_codec::Encode, - parity_scale_codec::Decode, - scale_info::TypeInfo - ) -)] -#[cfg_attr( - feature = "borsh", - derive(borsh::BorshSerialize, borsh::BorshDeserialize) -)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] -#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, From)] -pub struct TracePath(Vec); - -impl TracePath { - /// Returns true iff this path starts with the specified prefix - pub fn starts_with(&self, prefix: &TracePrefix) -> bool { - self.0.last().map(|p| p == prefix).unwrap_or(false) - } - - /// Removes the specified prefix from the path if there is a match, otherwise does nothing. - pub fn remove_prefix(&mut self, prefix: &TracePrefix) { - if self.starts_with(prefix) { - self.0.pop(); - } - } - - /// Adds the specified prefix to the path. - pub fn add_prefix(&mut self, prefix: TracePrefix) { - self.0.push(prefix) - } - - /// Returns true if the path is empty and false otherwise. - pub fn is_empty(&self) -> bool { - self.0.is_empty() - } - - /// Return empty trace path - pub fn empty() -> Self { - Self(vec![]) - } -} - -impl<'a> TryFrom> for TracePath { - type Error = NftTransferError; - - fn try_from(v: Vec<&'a str>) -> Result { - if v.len() % 2 != 0 { - return Err(NftTransferError::InvalidTraceLength { - len: v.len() as u64, - }); - } - - let mut trace = vec![]; - let id_pairs = v.chunks_exact(2).map(|paths| (paths[0], paths[1])); - for (pos, (port_id, channel_id)) in id_pairs.rev().enumerate() { - let port_id = - PortId::from_str(port_id).map_err(|e| NftTransferError::InvalidTracePortId { - pos: pos as u64, - validation_error: e, - })?; - let channel_id = ChannelId::from_str(channel_id).map_err(|e| { - NftTransferError::InvalidTraceChannelId { - pos: pos as u64, - validation_error: e, - } - })?; - trace.push(TracePrefix { - port_id, - channel_id, - }); - } - - Ok(trace.into()) - } -} - -impl FromStr for TracePath { - type Err = NftTransferError; - - fn from_str(s: &str) -> Result { - let parts = { - let parts: Vec<&str> = s.split('/').collect(); - if parts.len() == 1 && parts[0].trim().is_empty() { - vec![] - } else { - parts - } - }; - parts.try_into() - } -} - -impl Display for TracePath { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { - let path = self - .0 - .iter() - .rev() - .map(|prefix| prefix.to_string()) - .collect::>() - .join("/"); - write!(f, "{path}") - } -} - /// Prefixed class to trace sources like ICS-20 PrefixedDenom #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] @@ -267,24 +121,19 @@ pub fn is_receiver_chain_source( impl FromStr for PrefixedClassId { type Err = NftTransferError; + /// The parsing logic is same as [`FromStr`] impl of + /// [`PrefixedDenom`](ibc_app_transfer_types::PrefixedDenom) from ICS-20. fn from_str(s: &str) -> Result { - let mut parts: Vec<&str> = s.split('/').collect(); - let last_part = parts.pop().expect("split() returned an empty iterator"); - - let (base_class_id, trace_path) = { - if last_part == s { - (ClassId::from_str(s)?, TracePath::empty()) - } else { - let base_class_id = ClassId::from_str(last_part)?; - let trace_path = TracePath::try_from(parts)?; - (base_class_id, trace_path) - } - }; - - Ok(Self { - trace_path, - base_class_id, - }) + match TracePath::trim(s) { + (trace_path, Some(remaining_parts)) => Ok(Self { + trace_path, + base_class_id: ClassId::from_str(remaining_parts)?, + }), + (_, None) => Ok(Self { + trace_path: TracePath::empty(), + base_class_id: ClassId::from_str(s)?, + }), + } } } @@ -293,7 +142,9 @@ impl TryFrom for PrefixedClassId { fn try_from(value: RawClassTrace) -> Result { let base_class_id = ClassId::from_str(&value.base_class_id)?; - let trace_path = TracePath::from_str(&value.path)?; + // FIXME: separate `TracePath` error. + let trace_path = TracePath::from_str(&value.path) + .map_err(|err| NftTransferError::Other(err.to_string()))?; Ok(Self { trace_path, base_class_id, @@ -321,7 +172,7 @@ impl From for PrefixedClassId { impl Display for PrefixedClassId { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { - if self.trace_path.0.is_empty() { + if self.trace_path.is_empty() { write!(f, "{}", self.base_class_id) } else { write!(f, "{}/{}", self.trace_path, self.base_class_id) @@ -446,48 +297,50 @@ impl FromStr for ClassData { #[cfg(test)] mod tests { - use super::*; + use rstest::rstest; - #[test] - fn test_class_id_validation() -> Result<(), NftTransferError> { - assert!(ClassId::from_str("").is_err(), "empty base class ID"); - assert!(ClassId::from_str("myclass").is_ok(), "valid base class ID"); - assert!(PrefixedClassId::from_str("").is_err(), "empty class trace"); - assert!( - PrefixedClassId::from_str("transfer/channel-0/").is_err(), - "empty base class ID with trace" - ); - assert!( - PrefixedClassId::from_str("/myclass").is_err(), - "empty prefix" - ); - assert!(PrefixedClassId::from_str("//myclass").is_err(), "empty ids"); - assert!( - PrefixedClassId::from_str("transfer/").is_err(), - "single trace" - ); - assert!( - PrefixedClassId::from_str("transfer/myclass").is_err(), - "single trace with base class ID" - ); - assert!( - PrefixedClassId::from_str("transfer/channel-0/myclass").is_ok(), - "valid single trace info" - ); - assert!( - PrefixedClassId::from_str("transfer/channel-0/transfer/channel-1/myclass").is_ok(), - "valid multiple trace info" - ); - assert!( - PrefixedClassId::from_str("(transfer)/channel-0/myclass").is_err(), - "invalid port" - ); - assert!( - PrefixedClassId::from_str("transfer/(channel-0)/myclass").is_err(), - "invalid channel" - ); + use super::*; - Ok(()) + #[rstest] + #[case("myclass")] + #[case("transfer/channel-0/myclass")] + #[case("transfer/channel-0/transfer/channel-1/myclass")] + #[case("(transfer)/channel-0/myclass")] + #[case("transfer/(channel-0)/myclass")] + fn test_valid_class_id(#[case] class_id: &str) { + ClassId::from_str(class_id).expect("success"); + } + + #[rstest] + #[case("")] + #[case("")] + #[case(" ")] + fn test_invalid_class_id(#[case] class_id: &str) { + ClassId::from_str(class_id).expect_err("failure"); + } + + #[rstest] + #[case("transfer/channel-0/myclass")] + #[case("/myclass")] + #[case("//myclass")] + #[case("transfer/")] + #[case("transfer/myclass")] + #[case("transfer/channel-0/myclass")] + #[case("transfer/channel-0/transfer/channel-1/myclass")] + #[case("(transfer)/channel-0/myclass")] + #[case("transfer/(channel-0)/myclass")] + #[case("transfer/channel-0///")] + fn test_valid_prefixed_class_id(#[case] class_id: &str) { + PrefixedClassId::from_str(class_id).expect("success"); + } + + #[rstest] + #[case("")] + #[case(" ")] + #[case("transfer/channel-0/")] + #[case("transfer/channel-0/ ")] + fn test_invalid_prefixed_class_id(#[case] class_id: &str) { + PrefixedClassId::from_str(class_id).expect_err("failure"); } #[test] @@ -495,7 +348,7 @@ mod tests { assert_eq!( PrefixedClassId::from_str("transfer/channel-0/myclass")?, PrefixedClassId { - trace_path: "transfer/channel-0".parse()?, + trace_path: "transfer/channel-0".parse().expect("success"), base_class_id: "myclass".parse()? }, "valid single trace info" @@ -503,7 +356,9 @@ mod tests { assert_eq!( PrefixedClassId::from_str("transfer/channel-0/transfer/channel-1/myclass")?, PrefixedClassId { - trace_path: "transfer/channel-0/transfer/channel-1".parse()?, + trace_path: "transfer/channel-0/transfer/channel-1" + .parse() + .expect("success"), base_class_id: "myclass".parse()? }, "valid multiple trace info" @@ -543,21 +398,24 @@ mod tests { let prefix_1 = TracePrefix::new("transfer".parse().unwrap(), "channel-1".parse().unwrap()); let prefix_2 = TracePrefix::new("transfer".parse().unwrap(), "channel-0".parse().unwrap()); - let mut trace_path = TracePath(vec![prefix_1.clone()]); + let mut trace_path = TracePath::from(vec![prefix_1.clone()]); trace_path.add_prefix(prefix_2.clone()); assert_eq!( - TracePath::from_str("transfer/channel-0/transfer/channel-1")?, + TracePath::from_str("transfer/channel-0/transfer/channel-1").expect("success"), trace_path ); assert_eq!( - TracePath(vec![prefix_1.clone(), prefix_2.clone()]), + TracePath::from(vec![prefix_1.clone(), prefix_2.clone()]), trace_path ); trace_path.remove_prefix(&prefix_2); - assert_eq!(TracePath::from_str("transfer/channel-1")?, trace_path); - assert_eq!(TracePath(vec![prefix_1.clone()]), trace_path); + assert_eq!( + TracePath::from_str("transfer/channel-1").expect("success"), + trace_path + ); + assert_eq!(TracePath::from(vec![prefix_1.clone()]), trace_path); trace_path.remove_prefix(&prefix_1); assert!(trace_path.is_empty()); diff --git a/ibc-core/ics24-host/types/src/identifiers/channel_id.rs b/ibc-core/ics24-host/types/src/identifiers/channel_id.rs index 5d4583dc6..c8f9e30d7 100644 --- a/ibc-core/ics24-host/types/src/identifiers/channel_id.rs +++ b/ibc-core/ics24-host/types/src/identifiers/channel_id.rs @@ -88,9 +88,9 @@ impl AsRef for ChannelId { /// ``` /// use core::str::FromStr; /// use ibc_core_host_types::identifiers::ChannelId; -/// let channel_id = ChannelId::from_str("channelId-0"); +/// let channel_id = ChannelId::from_str("channel-0"); /// assert!(channel_id.is_ok()); -/// channel_id.map(|id| {assert_eq!(&id, "channelId-0")}); +/// channel_id.map(|id| {assert_eq!(&id, "channel-0")}); /// ``` impl PartialEq for ChannelId { fn eq(&self, other: &str) -> bool { diff --git a/ibc-core/ics24-host/types/src/identifiers/connection_id.rs b/ibc-core/ics24-host/types/src/identifiers/connection_id.rs index 883c41138..9fe9657cc 100644 --- a/ibc-core/ics24-host/types/src/identifiers/connection_id.rs +++ b/ibc-core/ics24-host/types/src/identifiers/connection_id.rs @@ -82,9 +82,9 @@ impl FromStr for ConnectionId { /// ``` /// use core::str::FromStr; /// use ibc_core_host_types::identifiers::ConnectionId; -/// let conn_id = ConnectionId::from_str("connectionId-0"); +/// let conn_id = ConnectionId::from_str("connection-0"); /// assert!(conn_id.is_ok()); -/// conn_id.map(|id| {assert_eq!(&id, "connectionId-0")}); +/// conn_id.map(|id| {assert_eq!(&id, "connection-0")}); /// ``` impl PartialEq for ConnectionId { fn eq(&self, other: &str) -> bool { diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index 47a92ac72..8346ff912 100644 --- a/ibc-core/ics24-host/types/src/validate.rs +++ b/ibc-core/ics24-host/types/src/validate.rs @@ -1,6 +1,7 @@ use ibc_primitives::prelude::*; use crate::error::IdentifierError as Error; +use crate::identifiers::{ChannelId, ConnectionId}; const VALID_SPECIAL_CHARS: &str = "._+-#[]<>"; @@ -61,6 +62,26 @@ pub fn validate_prefix_length( validate_identifier_length(prefix, min, max) } +/// Checks if the identifier is a valid named u64 index: {name}-{u64}. +/// Example: "connection-0", "connection-100", "channel-0", "channel-100". +pub fn validate_named_u64_index(id: &str, name: &str) -> Result<(), Error> { + let number_s = id + .strip_prefix(name) + .ok_or_else(|| Error::InvalidPrefix { prefix: id.into() })? + .strip_prefix('-') + .ok_or_else(|| Error::InvalidPrefix { prefix: id.into() })?; + + if number_s.starts_with('0') && number_s.len() > 1 { + return Err(Error::InvalidPrefix { prefix: id.into() }); + } + + _ = number_s + .parse::() + .map_err(|_| Error::InvalidPrefix { prefix: id.into() })?; + + Ok(()) +} + /// Default validator function for the Client types. pub fn validate_client_type(id: &str) -> Result<(), Error> { validate_identifier_chars(id)?; @@ -82,7 +103,9 @@ pub fn validate_client_identifier(id: &str) -> Result<(), Error> { /// in the ICS-24 spec. pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { validate_identifier_chars(id)?; - validate_identifier_length(id, 10, 64) + validate_identifier_length(id, 10, 64)?; + validate_named_u64_index(id, ConnectionId::prefix())?; + Ok(()) } /// Default validator function for Port identifiers. @@ -100,7 +123,9 @@ pub fn validate_port_identifier(id: &str) -> Result<(), Error> { /// the ICS-24 spec. pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { validate_identifier_chars(id)?; - validate_identifier_length(id, 8, 64) + validate_identifier_length(id, 8, 64)?; + validate_named_u64_index(id, ChannelId::prefix())?; + Ok(()) } #[cfg(test)] @@ -141,6 +166,24 @@ mod tests { assert!(id.is_err()) } + #[test] + fn parse_invalid_connection_id_indexed() { + // valid connection id with index + validate_connection_identifier("connection-0").expect("success"); + validate_connection_identifier("connection-123").expect("success"); + validate_connection_identifier("connection-18446744073709551615").expect("success"); + } + + #[test] + fn parse_invalid_connection_id_non_indexed() { + // invalid indexing for connection id + validate_connection_identifier("connection-0123").expect_err("failure"); + validate_connection_identifier("connection0123").expect_err("failure"); + validate_connection_identifier("connection000").expect_err("failure"); + // 1 << 64 = 18446744073709551616 + validate_connection_identifier("connection-18446744073709551616").expect_err("failure"); + } + #[test] fn parse_invalid_channel_id_min() { // invalid channel id, must be at least 8 characters @@ -157,6 +200,24 @@ mod tests { assert!(id.is_err()) } + #[test] + fn parse_invalid_channel_id_indexed() { + // valid channel id with index + validate_channel_identifier("channel-0").expect("success"); + validate_channel_identifier("channel-123").expect("success"); + validate_channel_identifier("channel-18446744073709551615").expect("success"); + } + + #[test] + fn parse_invalid_channel_id_non_indexed() { + // invalid indexing for channel id + validate_channel_identifier("channel-0123").expect_err("failure"); + validate_channel_identifier("channel0123").expect_err("failure"); + validate_channel_identifier("channel000").expect_err("failure"); + // 1 << 64 = 18446744073709551616 + validate_channel_identifier("channel-18446744073709551616").expect_err("failure"); + } + #[test] fn parse_invalid_client_id_min() { // invalid min client id @@ -242,4 +303,17 @@ mod tests { let result = validate_prefix_length(prefix, min, max); assert_eq!(result.is_ok(), success); } + + #[rstest] + #[case::zero_padded("channel", "001", false)] + #[case::only_zero("connection", "000", false)] + #[case::zero("channel", "0", true)] + #[case::one("connection", "1", true)] + #[case::n1234("channel", "1234", true)] + #[case::u64_max("chan", "18446744073709551615", true)] + #[case::u64_max_plus_1("chan", "18446744073709551616", false)] + fn test_named_index_validation(#[case] name: &str, #[case] id: &str, #[case] success: bool) { + let result = validate_named_u64_index(format!("{name}-{id}").as_str(), name); + assert_eq!(result.is_ok(), success, "{result:?}"); + } } diff --git a/ibc-testkit/src/fixtures/core/connection/conn_open_confirm.rs b/ibc-testkit/src/fixtures/core/connection/conn_open_confirm.rs index 151ab7e37..9a5014b87 100644 --- a/ibc-testkit/src/fixtures/core/connection/conn_open_confirm.rs +++ b/ibc-testkit/src/fixtures/core/connection/conn_open_confirm.rs @@ -14,7 +14,7 @@ pub fn dummy_conn_open_confirm() -> MsgConnectionOpenConfirm { /// Returns a dummy `RawMsgConnectionOpenConfirm` for testing purposes only! pub fn dummy_raw_msg_conn_open_confirm() -> RawMsgConnectionOpenConfirm { RawMsgConnectionOpenConfirm { - connection_id: "srcconnection".to_string(), + connection_id: "connection-118".to_string(), proof_ack: dummy_proof(), proof_height: Some(Height { revision_number: 0,