fix(ics20): PrefixedDenom parsing

.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))

ibc-apps/ics20-transfer/types/src/denom.rs

@@ -113,6 +113,13 @@ impl Display for TracePrefix {
 pub struct TracePath(Vec<TracePrefix>);
 
 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<TracePrefix>) -> 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)
@@ -294,18 +301,34 @@ impl FromStr for PrefixedDenom {
     type Err = TokenTransferError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        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,
@@ -357,6 +380,8 @@ impl Display for PrefixedDenom {
 
 #[cfg(test)]
 mod tests {
+    use rstest::rstest;
+
     use super::*;
 
     #[test]
@@ -368,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"
@@ -386,18 +401,86 @@ 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
+        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");
+
+        Ok(())
+    }
+
+    #[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"
+    )]
+    // https://github.com/cosmos/ibc-go/blob/e2ad31975f2ede592912b86346b5ebf055c9e05f/modules/apps/transfer/types/trace_test.go#L17-L38
+    #[case("", "uatom")]
+    #[case("", "uatom/")]
+    #[case("", "gamm/pool/1")]
+    #[case("", "gamm//pool//1")]
+    #[case("transfer/channel-1", "uatom")]
+    #[case("customtransfer/channel-1", "uatom")]
+    #[case("transfer/channel-1", "uatom/")]
+    #[case(
+        "transfer/channel-1",
+        "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA"
+    )]
+    #[case("transfer/channel-1", "gamm/pool/1")]
+    #[case("transfer/channel-1", "gamm//pool//1")]
+    #[case("transfer/channel-1/transfer/channel-2", "uatom")]
+    #[case("customtransfer/channel-1/alternativetransfer/channel-2", "uatom")]
+    #[case("", "transfer/uatom")]
+    #[case("", "transfer//uatom")]
+    #[case("", "channel-1/transfer/uatom")]
+    #[case("", "uatom/transfer")]
+    #[case("", "transfer/channel-1")]
+    #[case("transfer/channel-1", "transfer")]
+    #[case("", "transfer/channelToA/uatom")]
+    fn test_strange_but_accepted_prefixed_denom(
+        #[case] prefix: &str,
+        #[case] denom: &str,
+    ) -> Result<(), TokenTransferError> {
+        let pd_s = if prefix.is_empty() {
+            denom.to_owned()
+        } else {
+            format!("{prefix}/{denom}")
+        };
+        let pd = PrefixedDenom::from_str(&pd_s)?;
+
+        assert_eq!(pd.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]
     fn test_denom_trace() -> Result<(), TokenTransferError> {
         assert_eq!(

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")]

ibc-core/ics24-host/types/src/identifiers/channel_id.rs

@@ -88,9 +88,9 @@ impl AsRef<str> 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<str> for ChannelId {
     fn eq(&self, other: &str) -> bool {

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<str> for ConnectionId {
     fn eq(&self, other: &str) -> bool {

ibc-core/ics24-host/types/src/validate.rs

@@ -1,6 +1,7 @@
 use ibc_primitives::prelude::*;
 
 use crate::error::IdentifierError as Error;
+use crate::identifiers::{ChannelId, ConnectionId};
 
 const VALID_SPECIAL_CHARS: &str = "._+-#[]<>";
 
@@ -61,6 +62,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::<u64>()
+        .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)?;
@@ -82,7 +115,9 @@ pub fn validate_client_identifier(id: &str) -> Result<(), Error> {
 /// in the ICS-24 spec.
 pub fn validate_connection_identifier(id: &str) -> Result<(), Error> {
     validate_identifier_chars(id)?;
-    validate_identifier_length(id, 10, 64)
+    validate_identifier_length(id, 10, 64)?;
+    validate_named_u64_index(id, ConnectionId::prefix())?;
+    Ok(())
 }
 
 /// Default validator function for Port identifiers.
@@ -100,7 +135,9 @@ pub fn validate_port_identifier(id: &str) -> Result<(), Error> {
 /// the ICS-24 spec.
 pub fn validate_channel_identifier(id: &str) -> Result<(), Error> {
     validate_identifier_chars(id)?;
-    validate_identifier_length(id, 8, 64)
+    validate_identifier_length(id, 8, 64)?;
+    validate_named_u64_index(id, ChannelId::prefix())?;
+    Ok(())
 }
 
 #[cfg(test)]
@@ -141,6 +178,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
@@ -157,6 +213,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
@@ -242,4 +316,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:?}");
+    }
 }

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,