From c81f2dabbc0df96a97556b78b0a697cb8532ca1f Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 19:38:48 +0200 Subject: [PATCH 01/50] add failing test --- ibc-apps/ics20-transfer/types/src/denom.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 65b38fe67..aa65aec74 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -398,6 +398,20 @@ mod tests { Ok(()) } + #[test] + fn test_invalid_channel_id() -> Result<(), TokenTransferError> { + let denom = "transfer/channel-75/factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust"; + let dt = PrefixedDenom::from_str(denom)?; + + assert_eq!(dt.trace_path.to_string(), "transfer/channel-75"); + assert_eq!( + dt.base_denom.to_string(), + "factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust" + ); + + Ok(()) + } + #[test] fn test_denom_trace() -> Result<(), TokenTransferError> { assert_eq!( From 6c0a324c35600ab46ff0cef191e42b4ab6e16b21 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 20:47:56 +0200 Subject: [PATCH 02/50] add TracePath::new --- ibc-apps/ics20-transfer/types/src/denom.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index aa65aec74..6444a6338 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -113,6 +113,13 @@ impl Display for TracePrefix { pub struct TracePath(Vec); impl TracePath { + /// Creates a new trace path from a vector of trace prefixes. + /// Reverse the order of the prefixes for easier addition/removal from the end. + pub fn new(mut trace: Vec) -> Self { + trace.reverse(); + Self(trace) + } + /// 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) From ad096f5ce913fae22d9a250886cbc16868e22865 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 20:48:28 +0200 Subject: [PATCH 03/50] imp PrefixedDenom parsing --- ibc-apps/ics20-transfer/types/src/denom.rs | 38 +++++++++++++++------- ibc-core/ics24-host/types/src/error.rs | 2 ++ 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 6444a6338..4f689a7b3 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -301,18 +301,34 @@ impl FromStr for PrefixedDenom { type Err = TokenTransferError; 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) + let mut trace_prefixes = vec![]; + + let mut remaining_parts = s; + + loop { + let parsed_prefix = remaining_parts + .split_once('/') + .and_then(|(port_id_s, remaining)| { + remaining + .split_once('/') + .map(|(channel_id_s, remaining)| (port_id_s, channel_id_s, remaining)) + }) + .and_then(|(port_id_s, channel_id_s, remaining)| { + let port_id = PortId::from_str(port_id_s).ok()?; + let channel_id = ChannelId::from_str(channel_id_s).ok()?; + Some((port_id, channel_id, remaining)) + }); + match parsed_prefix { + Some((port_id, channel_id, remaining)) => { + trace_prefixes.push(TracePrefix::new(port_id, channel_id)); + remaining_parts = remaining; + } + None => break, } - }; + } + + let trace_path = TracePath::new(trace_prefixes); + let base_denom = BaseDenom::from_str(remaining_parts)?; Ok(Self { trace_path, diff --git a/ibc-core/ics24-host/types/src/error.rs b/ibc-core/ics24-host/types/src/error.rs index 4df4c4dd9..b0b6151e0 100644 --- a/ibc-core/ics24-host/types/src/error.rs +++ b/ibc-core/ics24-host/types/src/error.rs @@ -16,6 +16,8 @@ pub enum IdentifierError { RevisionNumberOverflow, /// String `{value}` cannot be converted to packet sequence, error: `{reason}` InvalidStringAsSequence { value: String, reason: String }, + /// String `{id}` does not start with `{name}` + InvalidNamedIndex { id: String, name: String }, } #[cfg(feature = "std")] From 08710f600f96a990299d768eda4f4f51f20336bf Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 20:48:56 +0200 Subject: [PATCH 04/50] add validate_named_u64_index --- ibc-core/ics24-host/types/src/validate.rs | 32 +++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index 47a92ac72..8360d80b3 100644 --- a/ibc-core/ics24-host/types/src/validate.rs +++ b/ibc-core/ics24-host/types/src/validate.rs @@ -61,6 +61,38 @@ 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::InvalidNamedIndex { + id: id.into(), + name: name.into(), + })? + .strip_prefix('-') + .ok_or_else(|| Error::InvalidNamedIndex { + id: id.into(), + name: name.into(), + })?; + + if number_s.starts_with('0') && number_s.len() > 1 { + return Err(Error::InvalidNamedIndex { + id: id.into(), + name: name.into(), + }); + } + + _ = number_s + .parse::() + .map_err(|_| Error::InvalidNamedIndex { + id: id.into(), + name: name.into(), + })?; + + Ok(()) +} + /// Default validator function for the Client types. pub fn validate_client_type(id: &str) -> Result<(), Error> { validate_identifier_chars(id)?; From 9d2a9eac787e5fb02da5c7f80fdf0fbc46a575f0 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 20:49:26 +0200 Subject: [PATCH 05/50] validate_named_u64_index in channel and connection validation --- ibc-core/ics24-host/types/src/validate.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index 8360d80b3..6ce26f246 100644 --- a/ibc-core/ics24-host/types/src/validate.rs +++ b/ibc-core/ics24-host/types/src/validate.rs @@ -114,7 +114,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, "connection")?; + Ok(()) } /// Default validator function for Port identifiers. @@ -132,7 +134,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, "channel")?; + Ok(()) } #[cfg(test)] From eba0690056504f33fc6ae1583d8049d5f1889197 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 20:49:44 +0200 Subject: [PATCH 06/50] add tests for validate_named_u64_index --- ibc-core/ics24-host/types/src/validate.rs | 50 +++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index 6ce26f246..b800c1d3b 100644 --- a/ibc-core/ics24-host/types/src/validate.rs +++ b/ibc-core/ics24-host/types/src/validate.rs @@ -177,6 +177,25 @@ mod tests { assert!(id.is_err()) } + #[test] + fn parse_invalid_connection_id_indexed() { + // valid connection id with index + validate_connection_identifier("connection-0").expect("no error"); + validate_connection_identifier("connection-123").expect("no error"); + validate_connection_identifier("connection-18446744073709551615").expect("no error"); + } + + #[test] + fn parse_invalid_connection_id_non_idexed() { + // invalid indexing for connection id + validate_connection_identifier("connection-0123").expect_err("InvalidNamedIndex"); + validate_connection_identifier("connection0123").expect_err("InvalidNamedIndex"); + validate_connection_identifier("connection000").expect_err("InvalidNamedIndex"); + // 1 << 64 = 18446744073709551616 + validate_connection_identifier("connection-18446744073709551616") + .expect_err("InvalidNamedIndex"); + } + #[test] fn parse_invalid_channel_id_min() { // invalid channel id, must be at least 8 characters @@ -193,6 +212,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("no error"); + validate_channel_identifier("channel-123").expect("no error"); + validate_channel_identifier("channel-18446744073709551615").expect("no error"); + } + + #[test] + fn parse_invalid_channel_id_non_idexed() { + // invalid indexing for channel id + validate_channel_identifier("channel-0123").expect_err("InvalidNamedIndex"); + validate_channel_identifier("channel0123").expect_err("InvalidNamedIndex"); + validate_channel_identifier("channel000").expect_err("InvalidNamedIndex"); + // 1 << 64 = 18446744073709551616 + validate_channel_identifier("channel-18446744073709551616").expect_err("InvalidNamedIndex"); + } + #[test] fn parse_invalid_client_id_min() { // invalid min client id @@ -278,4 +315,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:?}"); + } } From dfac6b5f9bc2fd133ee219ef88762537ba44d347 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 20:51:03 +0200 Subject: [PATCH 07/50] update PrefixDenom parsing tests --- ibc-apps/ics20-transfer/types/src/denom.rs | 34 +++++++++++++++------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 4f689a7b3..193d1a5f9 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -380,6 +380,8 @@ impl Display for PrefixedDenom { #[cfg(test)] mod tests { + use rstest::rstest; + use super::*; #[test] @@ -421,16 +423,28 @@ mod tests { Ok(()) } - #[test] - fn test_invalid_channel_id() -> Result<(), TokenTransferError> { - let denom = "transfer/channel-75/factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust"; - let dt = PrefixedDenom::from_str(denom)?; - - assert_eq!(dt.trace_path.to_string(), "transfer/channel-75"); - assert_eq!( - dt.base_denom.to_string(), - "factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust" - ); + #[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" + )] + fn test_strange_prefixed_denom( + #[case] prefix: &str, + #[case] denom: &str, + ) -> Result<(), TokenTransferError> { + let pd_s = format!("{}/{}", prefix, denom); + let pd = PrefixedDenom::from_str(&pd_s)?; + + assert_eq!(pd.trace_path.to_string(), prefix); + assert_eq!(pd.base_denom.to_string(), denom); Ok(()) } From be171a19388bc3cf266c7f2cf042a94c42a2e597 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 21:10:22 +0200 Subject: [PATCH 08/50] update accepted cases in ibc-go --- ibc-apps/ics20-transfer/types/src/denom.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 193d1a5f9..3dbde7655 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -393,16 +393,6 @@ mod tests { 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" - ); assert!( PrefixedDenom::from_str("transfer/channel-0/uatom").is_ok(), "valid single trace info" @@ -420,6 +410,13 @@ mod tests { "invalid channel" ); + // the followings are valid denom according to `ibc-go` + // https://github.com/cosmos/ibc-go/blob/e2ad31975f2ede592912b86346b5ebf055c9e05f/modules/apps/transfer/types/trace_test.go#L17-L38 + PrefixedDenom::from_str("/uatom").expect("no error"); + PrefixedDenom::from_str("//uatom").expect("no error"); + PrefixedDenom::from_str("transfer/").expect("no error"); + PrefixedDenom::from_str("transfer/atom").expect("no error"); + Ok(()) } From e59060ed3b6c0ab86b6121e9a90db442c1844509 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 21:30:53 +0200 Subject: [PATCH 09/50] update accepted cases --- ibc-apps/ics20-transfer/types/src/denom.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 3dbde7655..0f0bc11f9 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -401,14 +401,6 @@ mod tests { PrefixedDenom::from_str("transfer/channel-0/transfer/channel-1/uatom").is_ok(), "valid multiple trace info" ); - assert!( - PrefixedDenom::from_str("(transfer)/channel-0/uatom").is_err(), - "invalid port" - ); - assert!( - PrefixedDenom::from_str("transfer/(channel-0)/uatom").is_err(), - "invalid channel" - ); // the followings are valid denom according to `ibc-go` // https://github.com/cosmos/ibc-go/blob/e2ad31975f2ede592912b86346b5ebf055c9e05f/modules/apps/transfer/types/trace_test.go#L17-L38 @@ -416,6 +408,8 @@ mod tests { PrefixedDenom::from_str("//uatom").expect("no error"); PrefixedDenom::from_str("transfer/").expect("no error"); PrefixedDenom::from_str("transfer/atom").expect("no error"); + PrefixedDenom::from_str("(transfer)/channel-0/uatom").expect("no error"); + PrefixedDenom::from_str("transfer/(channel-0)/uatom").expect("no error"); Ok(()) } From 04b40dc619d41873b90f412fa9eac78a5dfe6f29 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 21:31:18 +0200 Subject: [PATCH 10/50] add test cases from ibc-go --- ibc-apps/ics20-transfer/types/src/denom.rs | 44 ++++++++++++++++++---- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 0f0bc11f9..42223a7f1 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -427,17 +427,45 @@ mod tests { "transfer/channel-75/transfer/channel-123/transfer/channel-1023/transfer/channel-0", "//////////////////////dust" )] - fn test_strange_prefixed_denom( - #[case] prefix: &str, - #[case] denom: &str, - ) -> Result<(), TokenTransferError> { - let pd_s = format!("{}/{}", prefix, denom); - let pd = PrefixedDenom::from_str(&pd_s)?; + // 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")] + #[should_panic] + #[case("", "")] + #[should_panic] + #[case("transfer/channel-1", "")] + #[should_panic] + #[case("transfer/channel-1/transfer/channel-2", "")] + fn test_strange_prefixed_denom(#[case] prefix: &str, #[case] denom: &str) { + let pd_s = if prefix.is_empty() { + denom.to_owned() + } else { + format!("{}/{}", prefix, denom) + }; + let pd = PrefixedDenom::from_str(&pd_s).expect("no error"); assert_eq!(pd.trace_path.to_string(), prefix); assert_eq!(pd.base_denom.to_string(), denom); - - Ok(()) } #[test] From a9b8d8b7763546541238a168fb1e6e6703b9fa61 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 21:31:37 +0200 Subject: [PATCH 11/50] use valid connection id --- ibc-testkit/src/fixtures/core/connection/conn_open_confirm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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, From 6542a23281af59303540025cfe7deb0f54951f91 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 21:48:07 +0200 Subject: [PATCH 12/50] failing doc tests --- ibc-core/ics24-host/types/src/identifiers/channel_id.rs | 4 ++-- ibc-core/ics24-host/types/src/identifiers/connection_id.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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 { From 3337f6591e5321ebdfe44be480627271537368db Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 21:48:20 +0200 Subject: [PATCH 13/50] use existing constants --- ibc-core/ics24-host/types/src/validate.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index b800c1d3b..09685c091 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 = "._+-#[]<>"; @@ -115,7 +116,7 @@ pub fn validate_client_identifier(id: &str) -> Result<(), Error> { pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { validate_identifier_chars(id)?; validate_identifier_length(id, 10, 64)?; - validate_named_u64_index(id, "connection")?; + validate_named_u64_index(id, ConnectionId::prefix())?; Ok(()) } @@ -135,7 +136,7 @@ pub fn validate_port_identifier(id: &str) -> Result<(), Error> { pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { validate_identifier_chars(id)?; validate_identifier_length(id, 8, 64)?; - validate_named_u64_index(id, "channel")?; + validate_named_u64_index(id, ChannelId::prefix())?; Ok(()) } From a88e1d27ad67690ee96b69d1ee30bac6b7ef7e67 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 21:51:20 +0200 Subject: [PATCH 14/50] changelog entry --- .../unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md 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..f081bfe6d --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md @@ -0,0 +1,2 @@ +- [ibc-apps-transfer] Compatible `PrefixedDenom` parsing with `ibc-go`. + ([\#1177](https://github.com/cosmos/ibc-rs/issues/1177)) From dd021d9c7ebf05831039697665780839b0a843f3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 19 Apr 2024 22:04:51 +0200 Subject: [PATCH 15/50] refactor tests --- ibc-apps/ics20-transfer/types/src/denom.rs | 31 +++++++++++++++------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 42223a7f1..26ea9a360 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -450,22 +450,35 @@ mod tests { #[case("", "transfer/channel-1")] #[case("transfer/channel-1", "transfer")] #[case("", "transfer/channelToA/uatom")] - #[should_panic] - #[case("", "")] - #[should_panic] - #[case("transfer/channel-1", "")] - #[should_panic] - #[case("transfer/channel-1/transfer/channel-2", "")] - fn test_strange_prefixed_denom(#[case] prefix: &str, #[case] denom: &str) { + 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) + format!("{prefix}/{denom}") }; - let pd = PrefixedDenom::from_str(&pd_s).expect("no error"); + let pd = PrefixedDenom::from_str(&pd_s)?; assert_eq!(pd.trace_path.to_string(), prefix); assert_eq!(pd.base_denom.to_string(), denom); + + Ok(()) + } + + #[rstest] + #[case("")] + #[case("transfer/channel-1")] + #[case("transfer/channel-1/transfer/channel-2")] + #[should_panic(expected = "EmptyBaseDenom")] + fn test_prefixed_empty_base_denom(#[case] prefix: &str) { + let pd_s = if prefix.is_empty() { + "".to_owned() + } else { + format!("{prefix}/") + }; + PrefixedDenom::from_str(&pd_s).expect("error"); } #[test] From 4a7c0347ff8039d33e325f8c5ad882c559c99f55 Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Sat, 20 Apr 2024 04:38:33 -0400 Subject: [PATCH 16/50] apply suggestions from code review Co-authored-by: Sean Chen Co-authored-by: Farhad Shabani Signed-off-by: Rano | Ranadeep --- .../unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md | 2 +- ibc-core/ics24-host/types/src/validate.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md index f081bfe6d..c7dd829c3 100644 --- a/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md +++ b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md @@ -1,2 +1,2 @@ -- [ibc-apps-transfer] Compatible `PrefixedDenom` parsing with `ibc-go`. +- [ibc-apps-transfer] Bring `PrefixedDenom` parsing up to parity with `ibc-go`. ([\#1177](https://github.com/cosmos/ibc-rs/issues/1177)) diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index 09685c091..7286bbc05 100644 --- a/ibc-core/ics24-host/types/src/validate.rs +++ b/ibc-core/ics24-host/types/src/validate.rs @@ -187,7 +187,7 @@ mod tests { } #[test] - fn parse_invalid_connection_id_non_idexed() { + fn parse_invalid_connection_id_non_indexed() { // invalid indexing for connection id validate_connection_identifier("connection-0123").expect_err("InvalidNamedIndex"); validate_connection_identifier("connection0123").expect_err("InvalidNamedIndex"); @@ -222,7 +222,7 @@ mod tests { } #[test] - fn parse_invalid_channel_id_non_idexed() { + fn parse_invalid_channel_id_non_indexed() { // invalid indexing for channel id validate_channel_identifier("channel-0123").expect_err("InvalidNamedIndex"); validate_channel_identifier("channel0123").expect_err("InvalidNamedIndex"); From 9d246d43d3306990010efc63253d513cdee43b0c Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 10:54:17 +0200 Subject: [PATCH 17/50] add comment for PrefixedDenom::from_str --- ibc-apps/ics20-transfer/types/src/denom.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 26ea9a360..b3cd44ebf 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -300,6 +300,27 @@ pub fn is_receiver_chain_source( impl FromStr for PrefixedDenom { type Err = TokenTransferError; + /// Initializes a [`PrefixedDenom`] from a string that adheres to the format + /// `{port-id-1/channel-id-1}/{port-id-2/channel-id-2}/.../{port-id-n/channel-id-n}/base-denom`. + /// A [`PrefixedDenom`] exhibits a sequence of `{port-id/channel-id}` 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"`, and 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 trace_prefixes = vec![]; From be0f6935e6bf48d1a16faacc5f01aec8199a83d1 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:00:29 +0200 Subject: [PATCH 18/50] rm TracePath::new --- ibc-apps/ics20-transfer/types/src/denom.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index b3cd44ebf..f10506887 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -92,9 +92,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( @@ -113,13 +114,6 @@ impl Display for TracePrefix { pub struct TracePath(Vec); impl TracePath { - /// Creates a new trace path from a vector of trace prefixes. - /// Reverse the order of the prefixes for easier addition/removal from the end. - pub fn new(mut trace: Vec) -> Self { - trace.reverse(); - Self(trace) - } - /// 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) @@ -348,7 +342,11 @@ impl FromStr for PrefixedDenom { } } - let trace_path = TracePath::new(trace_prefixes); + // reversing is needed, as [`TracePath`] requires quick addition/removal + // of prefixes which is performant from the end of a [`Vec`]. + trace_prefixes.reverse(); + let trace_path = TracePath::from(trace_prefixes); + let base_denom = BaseDenom::from_str(remaining_parts)?; Ok(Self { From 02d57c81abab55182fa999abd0879130626767f3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:05:48 +0200 Subject: [PATCH 19/50] collect the same tests --- ibc-apps/ics20-transfer/types/src/denom.rs | 24 ++-------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index f10506887..3b1c4f320 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -446,6 +446,8 @@ mod tests { "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")] // https://github.com/cosmos/ibc-go/blob/e2ad31975f2ede592912b86346b5ebf055c9e05f/modules/apps/transfer/types/trace_test.go#L17-L38 #[case("", "uatom")] #[case("", "uatom/")] @@ -500,28 +502,6 @@ mod tests { PrefixedDenom::from_str(&pd_s).expect("error"); } - #[test] - fn test_denom_trace() -> Result<(), TokenTransferError> { - assert_eq!( - PrefixedDenom::from_str("transfer/channel-0/uatom")?, - PrefixedDenom { - trace_path: "transfer/channel-0".parse()?, - base_denom: "uatom".parse()? - }, - "valid single trace info" - ); - 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" - ); - - Ok(()) - } - #[test] fn test_denom_serde() -> Result<(), TokenTransferError> { let dt_str = "transfer/channel-0/uatom"; From b1804b036d5a8915ea7ef734e5e022a48dfb66f1 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:06:38 +0200 Subject: [PATCH 20/50] test parsed PrefixedDenom string repr --- ibc-apps/ics20-transfer/types/src/denom.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 3b1c4f320..870658396 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -482,6 +482,7 @@ mod tests { }; 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); @@ -502,19 +503,6 @@ mod tests { PrefixedDenom::from_str(&pd_s).expect("error"); } - #[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"); - - 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"); - - Ok(()) - } - #[test] fn test_trace_path() -> Result<(), TokenTransferError> { assert!(TracePath::from_str("").is_ok(), "empty trace path"); From baedfb70edada2f910e4b5e2e93451219ad8c1d9 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:24:45 +0200 Subject: [PATCH 21/50] test trace order --- ibc-apps/ics20-transfer/types/src/denom.rs | 87 ++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 870658396..216c7ee45 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -503,6 +503,93 @@ mod tests { 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_eq!(prefixed_denom.base_denom.to_string(), "uatom"); + + let trace_prefix_1 = TracePrefix::new( + "alternativetransfer".parse().unwrap(), + "channel-2".parse().unwrap(), + ); + + let trace_prefix_2 = TracePrefix::new( + "customtransfer".parse().unwrap(), + "channel-1".parse().unwrap(), + ); + + 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(), + ); + + 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" + ); + + prefixed_denom.trace_path.remove_prefix(&trace_prefix_4); + + 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(), + "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!( + prefixed_denom.to_string(), + "customtransfer/channel-1/alternativetransfer/channel-2/uatom" + ); + + prefixed_denom.trace_path.remove_prefix(&trace_prefix_2); + + 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" + ); + + prefixed_denom.trace_path.remove_prefix(&trace_prefix_1); + + 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"); + } + #[test] fn test_trace_path() -> Result<(), TokenTransferError> { assert!(TracePath::from_str("").is_ok(), "empty trace path"); From 32f54074427fdb26dbe6e6e3dcaa21e13d8c5d41 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:30:21 +0200 Subject: [PATCH 22/50] more tests --- ibc-apps/ics20-transfer/types/src/denom.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 216c7ee45..29b357aec 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -448,6 +448,9 @@ mod tests { )] #[case("transfer/channel-0", "uatom")] #[case("transfer/channel-0/transfer/channel-1", "uatom")] + #[case("", "/")] + #[case("", "transfer/uatom")] + #[case("", "transfer//uatom")] // https://github.com/cosmos/ibc-go/blob/e2ad31975f2ede592912b86346b5ebf055c9e05f/modules/apps/transfer/types/trace_test.go#L17-L38 #[case("", "uatom")] #[case("", "uatom/")] From e3c109b46d2dbd3b36da348bf0cb10cf02899846 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:30:28 +0200 Subject: [PATCH 23/50] update tests --- ibc-apps/ics20-transfer/types/src/denom.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 29b357aec..ae7d57b88 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -493,17 +493,13 @@ mod tests { } #[rstest] + #[case("transfer/channel-1/")] + #[case("transfer/channel-1/transfer/channel-2/")] #[case("")] - #[case("transfer/channel-1")] - #[case("transfer/channel-1/transfer/channel-2")] + #[case("transfer/channel-0/")] #[should_panic(expected = "EmptyBaseDenom")] - fn test_prefixed_empty_base_denom(#[case] prefix: &str) { - let pd_s = if prefix.is_empty() { - "".to_owned() - } else { - format!("{prefix}/") - }; - PrefixedDenom::from_str(&pd_s).expect("error"); + fn test_prefixed_empty_base_denom(#[case] pd_s: &str) { + PrefixedDenom::from_str(pd_s).expect("error"); } #[rstest] From dfe5a7c863e32c5bc9dc02a0300a7d792a39f231 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:33:39 +0200 Subject: [PATCH 24/50] rm redundant error variant --- ibc-core/ics24-host/types/src/error.rs | 2 -- ibc-core/ics24-host/types/src/validate.rs | 20 ++++---------------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/ibc-core/ics24-host/types/src/error.rs b/ibc-core/ics24-host/types/src/error.rs index b0b6151e0..4df4c4dd9 100644 --- a/ibc-core/ics24-host/types/src/error.rs +++ b/ibc-core/ics24-host/types/src/error.rs @@ -16,8 +16,6 @@ pub enum IdentifierError { RevisionNumberOverflow, /// String `{value}` cannot be converted to packet sequence, error: `{reason}` InvalidStringAsSequence { value: String, reason: String }, - /// String `{id}` does not start with `{name}` - InvalidNamedIndex { id: String, name: String }, } #[cfg(feature = "std")] diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index 7286bbc05..3d607db0d 100644 --- a/ibc-core/ics24-host/types/src/validate.rs +++ b/ibc-core/ics24-host/types/src/validate.rs @@ -67,29 +67,17 @@ pub fn validate_prefix_length( pub fn validate_named_u64_index(id: &str, name: &str) -> Result<(), Error> { let number_s = id .strip_prefix(name) - .ok_or_else(|| Error::InvalidNamedIndex { - id: id.into(), - name: name.into(), - })? + .ok_or_else(|| Error::InvalidPrefix { prefix: id.into() })? .strip_prefix('-') - .ok_or_else(|| Error::InvalidNamedIndex { - id: id.into(), - name: name.into(), - })?; + .ok_or_else(|| Error::InvalidPrefix { prefix: id.into() })?; if number_s.starts_with('0') && number_s.len() > 1 { - return Err(Error::InvalidNamedIndex { - id: id.into(), - name: name.into(), - }); + return Err(Error::InvalidPrefix { prefix: id.into() }); } _ = number_s .parse::() - .map_err(|_| Error::InvalidNamedIndex { - id: id.into(), - name: name.into(), - })?; + .map_err(|_| Error::InvalidPrefix { prefix: id.into() })?; Ok(()) } From 5dd55593af8cd93008b0873121a3fa81499b1054 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 11:37:55 +0200 Subject: [PATCH 25/50] refactor tests --- ibc-core/ics24-host/types/src/validate.rs | 29 +++++++++++------------ 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/ibc-core/ics24-host/types/src/validate.rs b/ibc-core/ics24-host/types/src/validate.rs index 3d607db0d..8346ff912 100644 --- a/ibc-core/ics24-host/types/src/validate.rs +++ b/ibc-core/ics24-host/types/src/validate.rs @@ -169,20 +169,19 @@ mod tests { #[test] fn parse_invalid_connection_id_indexed() { // valid connection id with index - validate_connection_identifier("connection-0").expect("no error"); - validate_connection_identifier("connection-123").expect("no error"); - validate_connection_identifier("connection-18446744073709551615").expect("no error"); + 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("InvalidNamedIndex"); - validate_connection_identifier("connection0123").expect_err("InvalidNamedIndex"); - validate_connection_identifier("connection000").expect_err("InvalidNamedIndex"); + 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("InvalidNamedIndex"); + validate_connection_identifier("connection-18446744073709551616").expect_err("failure"); } #[test] @@ -204,19 +203,19 @@ mod tests { #[test] fn parse_invalid_channel_id_indexed() { // valid channel id with index - validate_channel_identifier("channel-0").expect("no error"); - validate_channel_identifier("channel-123").expect("no error"); - validate_channel_identifier("channel-18446744073709551615").expect("no error"); + 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("InvalidNamedIndex"); - validate_channel_identifier("channel0123").expect_err("InvalidNamedIndex"); - validate_channel_identifier("channel000").expect_err("InvalidNamedIndex"); + 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("InvalidNamedIndex"); + validate_channel_identifier("channel-18446744073709551616").expect_err("failure"); } #[test] From 5d04ebf0ceb7fd2820af85f0578ac61f001bd5ea Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 16:40:14 +0200 Subject: [PATCH 26/50] update doc string --- ibc-apps/ics20-transfer/types/src/denom.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index ae7d57b88..ad5786880 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -304,7 +304,7 @@ impl FromStr for PrefixedDenom { /// 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`. + /// The remaining parts of the string are then considered the [`BaseDenom`]. /// /// For example, given the following denom trace: /// "transfer/channel-75/factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust", @@ -312,7 +312,7 @@ impl FromStr for PrefixedDenom { /// 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"`, and invalid `ChannelId`. + /// 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 { From 50ebb2f2a6e0aa4ee673b1be1a69204be7820b84 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 17:06:49 +0200 Subject: [PATCH 27/50] add comment on virtual split_twice --- ibc-apps/ics20-transfer/types/src/denom.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index ad5786880..2318adef7 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -321,6 +321,8 @@ impl FromStr for PrefixedDenom { let mut remaining_parts = s; loop { + // The below two chained `split_once` calls emulate a virtual `split_twice` call, + // which is not available in the standard library. let parsed_prefix = remaining_parts .split_once('/') .and_then(|(port_id_s, remaining)| { From 063c1df6da3646915407d8e992c94f85c278a972 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 20 Apr 2024 17:25:50 +0200 Subject: [PATCH 28/50] update doc str --- ibc-apps/ics20-transfer/types/src/denom.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 2318adef7..5cfad00a8 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -295,8 +295,8 @@ impl FromStr for PrefixedDenom { type Err = TokenTransferError; /// Initializes a [`PrefixedDenom`] from a string that adheres to the format - /// `{port-id-1/channel-id-1}/{port-id-2/channel-id-2}/.../{port-id-n/channel-id-n}/base-denom`. - /// A [`PrefixedDenom`] exhibits a sequence of `{port-id/channel-id}` pairs. + /// `{1st-port-id/channel-}/{2nd-port-id/channel-}/.../{nth-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 From f72517ff1e078dd413fa103a73e8f14fe0d986c3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 22 Apr 2024 14:55:43 +0200 Subject: [PATCH 29/50] add TracePrefix::strip and TracePath::trim --- ibc-apps/ics20-transfer/types/src/denom.rs | 77 +++++++++++++--------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 5cfad00a8..ec3241e64 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -83,6 +83,26 @@ impl TracePrefix { channel_id, } } + + /// Returns a string slice with [`TracePrefix`] removed. + /// + /// If the string starts a [`TracePrefix`] format, i.e. `{port-id}/channel-{id}/`, + /// it returns a tuple of the removed [`TracePrefix`] and the substring after the prefix. + /// + /// If the string does not start with a [`TracePrefix`], it returns `None`. + /// + /// It is analogous to `strip_prefix` from standard libray. + pub fn strip(s: &str) -> Option<(Self, &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('/')?; + + let port_id = PortId::from_str(port_id_s).ok()?; + let channel_id = ChannelId::from_str(channel_id_s).ok()?; + + Some((Self::new(port_id, channel_id), remaining)) + } } impl Display for TracePrefix { @@ -140,6 +160,30 @@ impl TracePath { pub fn empty() -> Self { Self(vec![]) } + + /// 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 string does not have any [`TracePrefix`], it returns original string. + /// + /// It is analogous to `trim_start_matches` from standard library. + pub fn trim(s: &str) -> (Self, &str) { + let mut trace_prefixes = vec![]; + + let mut remaining_parts = s; + + while let Some((trace_prefix, remaining)) = TracePrefix::strip(remaining_parts) { + trace_prefixes.push(trace_prefix); + remaining_parts = remaining; + } + + // reversing is needed, as [`TracePath`] requires quick addition/removal + // of prefixes which is performant from the end of a [`Vec`]. + trace_prefixes.reverse(); + (Self(trace_prefixes), remaining_parts) + } } impl<'a> TryFrom> for TracePath { @@ -316,38 +360,7 @@ impl FromStr for PrefixedDenom { /// 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 trace_prefixes = vec![]; - - let mut remaining_parts = s; - - loop { - // The below two chained `split_once` calls emulate a virtual `split_twice` call, - // which is not available in the standard library. - let parsed_prefix = remaining_parts - .split_once('/') - .and_then(|(port_id_s, remaining)| { - remaining - .split_once('/') - .map(|(channel_id_s, remaining)| (port_id_s, channel_id_s, remaining)) - }) - .and_then(|(port_id_s, channel_id_s, remaining)| { - let port_id = PortId::from_str(port_id_s).ok()?; - let channel_id = ChannelId::from_str(channel_id_s).ok()?; - Some((port_id, channel_id, remaining)) - }); - match parsed_prefix { - Some((port_id, channel_id, remaining)) => { - trace_prefixes.push(TracePrefix::new(port_id, channel_id)); - remaining_parts = remaining; - } - None => break, - } - } - - // reversing is needed, as [`TracePath`] requires quick addition/removal - // of prefixes which is performant from the end of a [`Vec`]. - trace_prefixes.reverse(); - let trace_path = TracePath::from(trace_prefixes); + let (trace_path, remaining_parts) = TracePath::trim(s); let base_denom = BaseDenom::from_str(remaining_parts)?; From 25111df6139cde06aa6711d39da58df057ab8c6a Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 22 Apr 2024 14:57:39 +0200 Subject: [PATCH 30/50] add whitespace cases --- ibc-apps/ics20-transfer/types/src/denom.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index ec3241e64..83be597e8 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -510,7 +510,9 @@ mod tests { #[rstest] #[case("transfer/channel-1/")] #[case("transfer/channel-1/transfer/channel-2/")] + #[case("transfer/channel-21/transfer/channel-23/ ")] #[case("")] + #[case(" ")] #[case("transfer/channel-0/")] #[should_panic(expected = "EmptyBaseDenom")] fn test_prefixed_empty_base_denom(#[case] pd_s: &str) { From 76cf402c4592625ac27db27d45dd26744dc5ecbd Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 22 Apr 2024 15:03:46 +0200 Subject: [PATCH 31/50] refactor tests --- ibc-apps/ics20-transfer/types/src/denom.rs | 48 +++++++++------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 83be597e8..9cbec93fa 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -418,34 +418,18 @@ mod tests { 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("transfer/channel-0/uatom").is_ok(), - "valid single trace info" - ); - assert!( - PrefixedDenom::from_str("transfer/channel-0/transfer/channel-1/uatom").is_ok(), - "valid multiple trace info" - ); - - // the followings are valid denom according to `ibc-go` - // https://github.com/cosmos/ibc-go/blob/e2ad31975f2ede592912b86346b5ebf055c9e05f/modules/apps/transfer/types/trace_test.go#L17-L38 - PrefixedDenom::from_str("/uatom").expect("no error"); - PrefixedDenom::from_str("//uatom").expect("no error"); - PrefixedDenom::from_str("transfer/").expect("no error"); - PrefixedDenom::from_str("transfer/atom").expect("no error"); - PrefixedDenom::from_str("(transfer)/channel-0/uatom").expect("no error"); - PrefixedDenom::from_str("transfer/(channel-0)/uatom").expect("no error"); + #[rstest] + #[case("uatom")] + #[case("atom")] + fn test_accepted_denom(#[case] denom_str: &str) { + BaseDenom::from_str(denom_str).expect("success"); + } - Ok(()) + #[rstest] + #[case("")] + #[case(" ")] + fn test_rejected_denom(#[case] denom_str: &str) { + BaseDenom::from_str(denom_str).expect_err("failure"); } #[rstest] @@ -465,7 +449,13 @@ mod tests { #[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/")] @@ -508,11 +498,11 @@ mod tests { } #[rstest] + #[case("")] + #[case(" ")] #[case("transfer/channel-1/")] #[case("transfer/channel-1/transfer/channel-2/")] #[case("transfer/channel-21/transfer/channel-23/ ")] - #[case("")] - #[case(" ")] #[case("transfer/channel-0/")] #[should_panic(expected = "EmptyBaseDenom")] fn test_prefixed_empty_base_denom(#[case] pd_s: &str) { From 1524a5088c9c7ccecdde91ee5bd4622eda94f786 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 22 Apr 2024 15:05:21 +0200 Subject: [PATCH 32/50] spelling --- ibc-apps/ics20-transfer/types/src/denom.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 9cbec93fa..7a9dc44e4 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -91,7 +91,7 @@ impl TracePrefix { /// /// If the string does not start with a [`TracePrefix`], it returns `None`. /// - /// It is analogous to `strip_prefix` from standard libray. + /// It is analogous to `strip_prefix` from standard library. pub fn strip(s: &str) -> Option<(Self, &str)> { // The below two chained `split_once` calls emulate a virtual `split_twice` call, // which is not available in the standard library. From 8720be03198541d43ae98ff7eca597789dafc580 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 22 Apr 2024 15:08:11 +0200 Subject: [PATCH 33/50] parse over from_str --- ibc-apps/ics20-transfer/types/src/denom.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 7a9dc44e4..b089cf413 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -98,8 +98,8 @@ impl TracePrefix { let (port_id_s, remaining) = s.split_once('/')?; let (channel_id_s, remaining) = remaining.split_once('/')?; - let port_id = PortId::from_str(port_id_s).ok()?; - let channel_id = ChannelId::from_str(channel_id_s).ok()?; + let port_id = port_id_s.parse().ok()?; + let channel_id = channel_id_s.parse().ok()?; Some((Self::new(port_id, channel_id), remaining)) } From 93a81d5cf6b0e006bb23ae95436dc1c4a4f48f22 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 22 Apr 2024 15:11:58 +0200 Subject: [PATCH 34/50] nit --- ibc-apps/ics20-transfer/types/src/denom.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index b089cf413..943c3f71a 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -171,7 +171,6 @@ impl TracePath { /// It is analogous to `trim_start_matches` from standard library. pub fn trim(s: &str) -> (Self, &str) { let mut trace_prefixes = vec![]; - let mut remaining_parts = s; while let Some((trace_prefix, remaining)) = TracePrefix::strip(remaining_parts) { From 94ac9417379bbe5db9b9ac011ecbf066aad4d655 Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Mon, 22 Apr 2024 10:05:56 -0400 Subject: [PATCH 35/50] apply suggestions from code review Co-authored-by: Sean Chen Signed-off-by: Rano | Ranadeep --- ibc-apps/ics20-transfer/types/src/denom.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 943c3f71a..c6caebc19 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -86,12 +86,12 @@ impl TracePrefix { /// Returns a string slice with [`TracePrefix`] removed. /// - /// If the string starts a [`TracePrefix`] format, i.e. `{port-id}/channel-{id}/`, + /// 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 string does not start with a [`TracePrefix`], it returns `None`. + /// If the string does not start with a [`TracePrefix`], this method returns `None`. /// - /// It is analogous to `strip_prefix` from standard library. + /// This method is analogous to `strip_prefix` from the standard library. pub fn strip(s: &str) -> Option<(Self, &str)> { // The below two chained `split_once` calls emulate a virtual `split_twice` call, // which is not available in the standard library. @@ -166,9 +166,9 @@ impl TracePath { /// If the string starts with a [`TracePath`], it returns a tuple of the removed /// [`TracePath`] and the substring after the [`TracePath`]. /// - /// If the string does not have any [`TracePrefix`], it returns original string. + /// If the string does not contain any [`TracePrefix`], it returns the original string. /// - /// It is analogous to `trim_start_matches` from standard library. + /// This method is analogous to `trim_start_matches` from the standard library. pub fn trim(s: &str) -> (Self, &str) { let mut trace_prefixes = vec![]; let mut remaining_parts = s; @@ -179,7 +179,7 @@ impl TracePath { } // reversing is needed, as [`TracePath`] requires quick addition/removal - // of prefixes which is performant from the end of a [`Vec`]. + // of prefixes which is more performant from the end of a [`Vec`]. trace_prefixes.reverse(); (Self(trace_prefixes), remaining_parts) } From 7408994c1f08b255b08c24c63c1435f363aac3ef Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 14:24:16 +0200 Subject: [PATCH 36/50] add failing test --- ibc-apps/ics20-transfer/types/src/denom.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index c6caebc19..726387200 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -196,6 +196,7 @@ impl<'a> TryFrom> for TracePath { } let mut trace = vec![]; + // this does not take care of the remainder if the length is not even 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 = @@ -417,6 +418,19 @@ mod tests { use super::*; + #[rstest] + #[case("")] + #[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("failure"); + } + #[rstest] #[case("uatom")] #[case("atom")] From d34d9466071b9b67bdd800a0c157776762eff4a5 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 14:29:27 +0200 Subject: [PATCH 37/50] fix TracePath::from_str --- ibc-apps/ics20-transfer/types/src/denom.rs | 49 +++------------------- ibc-apps/ics20-transfer/types/src/error.rs | 2 + 2 files changed, 7 insertions(+), 44 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 726387200..742ad137b 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -185,54 +185,15 @@ impl TracePath { } } -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![]; - // this does not take care of the remainder if the length is not even - 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, - }); - } - - Ok(trace.into()) - } -} - 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() + let (trace_path, remaining_parts) = TracePath::trim(s); + remaining_parts + .is_empty() + .then_some(trace_path) + .ok_or_else(|| TokenTransferError::MalformedTrace(s.to_string())) } } 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}` From 03b08d7bfdffab5e8b3162aac2320c26f3140ce1 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 14:36:09 +0200 Subject: [PATCH 38/50] fix parsing --- ibc-apps/ics20-transfer/types/src/denom.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 742ad137b..a8baf5431 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -189,7 +189,14 @@ impl FromStr for TracePath { type Err = TokenTransferError; fn from_str(s: &str) -> Result { - let (trace_path, remaining_parts) = TracePath::trim(s); + if s.is_empty() { + return Ok(TracePath::empty()); + } + + // This is needed as TracePath trims `{port-id}/{channel-id}/`. + let slashed_s = format!("{}/", s); + + let (trace_path, remaining_parts) = TracePath::trim(slashed_s.as_str()); remaining_parts .is_empty() .then_some(trace_path) From 29be17b032f2c1968493a0e8831a8e2aa52116ed Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 14:57:51 +0200 Subject: [PATCH 39/50] fix test --- ibc-apps/ics20-transfer/types/src/denom.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index a8baf5431..b6a85dd2d 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -387,7 +387,6 @@ mod tests { use super::*; #[rstest] - #[case("")] #[case("transfer")] #[case("transfer/channel-1/ica")] fn test_invalid_raw_demon_trace_parsing(#[case] trace_path: &str) { @@ -396,7 +395,7 @@ mod tests { base_denom: "uatom".to_string(), }; - PrefixedDenom::try_from(raw_denom_trace).expect("failure"); + PrefixedDenom::try_from(raw_denom_trace).expect_err("failure"); } #[rstest] From 2b117335ebfe31421c47a25521884771bc69bf2d Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 14:59:38 +0200 Subject: [PATCH 40/50] add ibc-app-transfer-types dep --- ibc-apps/ics721-nft-transfer/types/Cargo.toml | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/ibc-apps/ics721-nft-transfer/types/Cargo.toml b/ibc-apps/ics721-nft-transfer/types/Cargo.toml index 9deb1a361..8ec4134b4 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", ] From 1eecc2b5be4a2f7a1e740cab4e23843a94ef5eea Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 15:00:07 +0200 Subject: [PATCH 41/50] rm reusable code --- .../ics721-nft-transfer/types/src/class.rs | 148 +----------------- 1 file changed, 1 insertion(+), 147 deletions(-) diff --git a/ibc-apps/ics721-nft-transfer/types/src/class.rs b/ibc-apps/ics721-nft-transfer/types/src/class.rs index c2ff9afd4..8f5ccd1da 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))] From 6ff2abf56123a29cbc76f0b2508abd21fc38948d Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 15:01:25 +0200 Subject: [PATCH 42/50] update impls --- .../ics721-nft-transfer/types/src/class.rs | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/ibc-apps/ics721-nft-transfer/types/src/class.rs b/ibc-apps/ics721-nft-transfer/types/src/class.rs index 8f5ccd1da..845e2d36a 100644 --- a/ibc-apps/ics721-nft-transfer/types/src/class.rs +++ b/ibc-apps/ics721-nft-transfer/types/src/class.rs @@ -121,19 +121,12 @@ 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) - } - }; + let (trace_path, remaining_parts) = TracePath::trim(s); + + let base_class_id = ClassId::from_str(remaining_parts)?; Ok(Self { trace_path, @@ -147,7 +140,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, @@ -175,7 +170,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) From bd7922ed63bb2d24bc8d9b8e9fca2ca57db3d5e7 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 15:01:44 +0200 Subject: [PATCH 43/50] update tests --- .../ics721-nft-transfer/types/src/class.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/ibc-apps/ics721-nft-transfer/types/src/class.rs b/ibc-apps/ics721-nft-transfer/types/src/class.rs index 845e2d36a..ff2ab8cb3 100644 --- a/ibc-apps/ics721-nft-transfer/types/src/class.rs +++ b/ibc-apps/ics721-nft-transfer/types/src/class.rs @@ -344,7 +344,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" @@ -352,7 +352,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" @@ -392,21 +394,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()); From 736d4a1deddd8f5277af4cc731d3fa730e7afd54 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 15:02:09 +0200 Subject: [PATCH 44/50] accepted class ids --- ibc-apps/ics721-nft-transfer/types/src/class.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ibc-apps/ics721-nft-transfer/types/src/class.rs b/ibc-apps/ics721-nft-transfer/types/src/class.rs index ff2ab8cb3..ffacf32e1 100644 --- a/ibc-apps/ics721-nft-transfer/types/src/class.rs +++ b/ibc-apps/ics721-nft-transfer/types/src/class.rs @@ -307,16 +307,16 @@ mod tests { "empty base class ID with trace" ); assert!( - PrefixedClassId::from_str("/myclass").is_err(), + PrefixedClassId::from_str("/myclass").is_ok(), "empty prefix" ); - assert!(PrefixedClassId::from_str("//myclass").is_err(), "empty ids"); + assert!(PrefixedClassId::from_str("//myclass").is_ok(), "empty ids"); assert!( - PrefixedClassId::from_str("transfer/").is_err(), + PrefixedClassId::from_str("transfer/").is_ok(), "single trace" ); assert!( - PrefixedClassId::from_str("transfer/myclass").is_err(), + PrefixedClassId::from_str("transfer/myclass").is_ok(), "single trace with base class ID" ); assert!( @@ -328,11 +328,11 @@ mod tests { "valid multiple trace info" ); assert!( - PrefixedClassId::from_str("(transfer)/channel-0/myclass").is_err(), + PrefixedClassId::from_str("(transfer)/channel-0/myclass").is_ok(), "invalid port" ); assert!( - PrefixedClassId::from_str("transfer/(channel-0)/myclass").is_err(), + PrefixedClassId::from_str("transfer/(channel-0)/myclass").is_ok(), "invalid channel" ); From badb01898dd1da1f1346dc88b445dbeee284597d Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 15:19:25 +0200 Subject: [PATCH 45/50] refactor tests --- .../ics721-nft-transfer/types/src/class.rs | 80 ++++++++++--------- 1 file changed, 41 insertions(+), 39 deletions(-) diff --git a/ibc-apps/ics721-nft-transfer/types/src/class.rs b/ibc-apps/ics721-nft-transfer/types/src/class.rs index ffacf32e1..7b820afdd 100644 --- a/ibc-apps/ics721-nft-transfer/types/src/class.rs +++ b/ibc-apps/ics721-nft-transfer/types/src/class.rs @@ -295,48 +295,50 @@ impl FromStr for ClassData { #[cfg(test)] mod tests { + use rstest::rstest; + use super::*; - #[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_ok(), - "empty prefix" - ); - assert!(PrefixedClassId::from_str("//myclass").is_ok(), "empty ids"); - assert!( - PrefixedClassId::from_str("transfer/").is_ok(), - "single trace" - ); - assert!( - PrefixedClassId::from_str("transfer/myclass").is_ok(), - "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_ok(), - "invalid port" - ); - assert!( - PrefixedClassId::from_str("transfer/(channel-0)/myclass").is_ok(), - "invalid channel" - ); + #[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"); + } - Ok(()) + #[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] From eb9456fabe0582fd0320a607f820d8f128c56768 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 24 Apr 2024 15:22:27 +0200 Subject: [PATCH 46/50] update changelog entry --- .../unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md index c7dd829c3..4bb064600 100644 --- a/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md +++ b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md @@ -1,2 +1,5 @@ -- [ibc-apps-transfer] Bring `PrefixedDenom` parsing up to parity with `ibc-go`. +- [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`. + ([\#1178](https://github.com/cosmos/ibc-rs/pull/1178)) From 43e46ad7c1e74cde789d62641bdf3a678b3fe1d3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 25 Apr 2024 12:15:36 +0200 Subject: [PATCH 47/50] opt trace prefix and path parsing --- ibc-apps/ics20-transfer/types/src/denom.rs | 65 ++++++++++++------- .../ics721-nft-transfer/types/src/class.rs | 18 ++--- 2 files changed, 53 insertions(+), 30 deletions(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index b6a85dd2d..624ad0275 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -86,17 +86,24 @@ impl TracePrefix { /// Returns a string slice with [`TracePrefix`] removed. /// - /// If the string starts with a [`TracePrefix`], i.e. `{port-id}/channel-{id}/`, + /// 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, &str)> { + 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('/')?; + 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()?; @@ -166,22 +173,37 @@ impl TracePath { /// 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, &str) { + 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 remaining_parts = s; + 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; + }; - while let Some((trace_prefix, remaining)) = TracePrefix::strip(remaining_parts) { trace_prefixes.push(trace_prefix); - remaining_parts = remaining; + current_remaining_opt = next_remaining_opt; } - // reversing is needed, as [`TracePath`] requires quick addition/removal + // 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), remaining_parts) + (Self(trace_prefixes), current_remaining_opt) } } @@ -193,12 +215,9 @@ impl FromStr for TracePath { return Ok(TracePath::empty()); } - // This is needed as TracePath trims `{port-id}/{channel-id}/`. - let slashed_s = format!("{}/", s); - - let (trace_path, remaining_parts) = TracePath::trim(slashed_s.as_str()); + let (trace_path, remaining_parts) = TracePath::trim(s); remaining_parts - .is_empty() + .is_none() .then_some(trace_path) .ok_or_else(|| TokenTransferError::MalformedTrace(s.to_string())) } @@ -328,14 +347,16 @@ impl FromStr for PrefixedDenom { /// 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 (trace_path, remaining_parts) = TracePath::trim(s); - - let base_denom = BaseDenom::from_str(remaining_parts)?; - - 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)?, + }), + } } } diff --git a/ibc-apps/ics721-nft-transfer/types/src/class.rs b/ibc-apps/ics721-nft-transfer/types/src/class.rs index 7b820afdd..ff1293f89 100644 --- a/ibc-apps/ics721-nft-transfer/types/src/class.rs +++ b/ibc-apps/ics721-nft-transfer/types/src/class.rs @@ -124,14 +124,16 @@ impl FromStr for PrefixedClassId { /// 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 (trace_path, remaining_parts) = TracePath::trim(s); - - let base_class_id = ClassId::from_str(remaining_parts)?; - - 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)?, + }), + } } } From a7846403dd1677cafd72da8e2d8cc74c7e1304e8 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 25 Apr 2024 12:15:44 +0200 Subject: [PATCH 48/50] add new tests --- ibc-apps/ics20-transfer/types/src/denom.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 624ad0275..5cae4701d 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -597,6 +597,25 @@ mod tests { 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] fn test_trace_path() -> Result<(), TokenTransferError> { assert!(TracePath::from_str("").is_ok(), "empty trace path"); From 7f465b0af5749d1711205fd86b8c6a219976f4c8 Mon Sep 17 00:00:00 2001 From: Sean Chen Date: Fri, 26 Apr 2024 10:03:51 -0500 Subject: [PATCH 49/50] Update changelog entry --- .../unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md index 4bb064600..cf89e9a52 100644 --- a/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md +++ b/.changelog/unreleased/bug-fixes/1177-fix-prefixed-denom-parsing.md @@ -1,5 +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`. + `ibc-app-transfer-types` when parsing `PrefixedClassId`. ([\#1178](https://github.com/cosmos/ibc-rs/pull/1178)) From 53fad12fe60e4248e74a476578cf1999e3d3ba34 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 26 Apr 2024 17:43:33 +0200 Subject: [PATCH 50/50] fix trace-prefix index order --- ibc-apps/ics20-transfer/types/src/denom.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs index 5cae4701d..8ac632fea 100644 --- a/ibc-apps/ics20-transfer/types/src/denom.rs +++ b/ibc-apps/ics20-transfer/types/src/denom.rs @@ -326,7 +326,7 @@ impl FromStr for PrefixedDenom { type Err = TokenTransferError; /// Initializes a [`PrefixedDenom`] from a string that adheres to the format - /// `{1st-port-id/channel-}/{2nd-port-id/channel-}/.../{nth-port-id/channel-}/`. + /// `{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`]. ///