Additional tests for counterparty conn & chan ids at initialization

.changelog/unreleased/improvements/175-investigate-counterpary-ids-at-init-phase.md

@@ -0,0 +1,2 @@
+- Add unit tests to cover edge scenarios for counterparty conn & chan ids at init phases
+  ([#175](https://github.com/cosmos/ibc-rs/issues/175)).

crates/ibc/src/core/context.rs

@@ -705,6 +705,9 @@ mod val_exec_ctx {
         ExecCtx: ExecutionContext,
     {
         let chan_id_on_b = ChannelId::new(ctx_b.channel_counter()?);
+        ctx_b.log_message(format!(
+            "success: channel open try with channel identifier: {chan_id_on_b}"
+        ));
         let module = ctx_b
             .get_route_mut(&module_id)
             .ok_or(ChannelError::RouteNotFound)?;

crates/ibc/src/core/ics02_client/context.rs

@@ -68,7 +68,7 @@ pub trait ClientReader {
     /// Returns the pending `ConsensusState` of the host (local) chain.
     fn pending_host_consensus_state(&self) -> Result<Box<dyn ConsensusState>, ClientError>;
 
-    /// Returns a natural number, counting how many clients have been created thus far.
+    /// Returns a natural number, counting how many clients have been created thus far..
     /// The value of this counter should increase only via method `ClientKeeper::increase_client_counter`.
     fn client_counter(&self) -> Result<u64, ClientError>;
 }

crates/ibc/src/core/ics03_connection/connection.rs

@@ -1,6 +1,5 @@
 use crate::prelude::*;
 
-use core::str::FromStr;
 use core::time::Duration;
 use core::{
     fmt::{Display, Error as FmtError, Formatter},
@@ -384,19 +383,24 @@ impl Protobuf<RawCounterparty> for Counterparty {}
 impl TryFrom<RawCounterparty> for Counterparty {
     type Error = ConnectionError;
 
-    fn try_from(value: RawCounterparty) -> Result<Self, Self::Error> {
-        let connection_id = Some(value.connection_id)
-            .filter(|x| !x.is_empty())
-            .map(|v| FromStr::from_str(v.as_str()))
-            .transpose()
-            .map_err(ConnectionError::InvalidIdentifier)?;
+    fn try_from(raw_counterparty: RawCounterparty) -> Result<Self, Self::Error> {
+        let connection_id: Option<ConnectionId> = if raw_counterparty.connection_id.is_empty() {
+            None
+        } else {
+            Some(
+                raw_counterparty
+                    .connection_id
+                    .parse()
+                    .map_err(ConnectionError::InvalidIdentifier)?,
+            )
+        };
         Ok(Counterparty::new(
-            value
+            raw_counterparty
                 .client_id
                 .parse()
                 .map_err(ConnectionError::InvalidIdentifier)?,
             connection_id,
-            value
+            raw_counterparty
                 .prefix
                 .ok_or(ConnectionError::MissingCounterparty)?
                 .key_prefix

crates/ibc/src/core/ics03_connection/context.rs

@@ -70,8 +70,7 @@ pub trait ConnectionReader {
     }
 
     /// Returns a counter on how many connections have been created thus far.
-    /// The value of this counter should increase only via method
-    /// `ConnectionKeeper::increase_connection_counter`.
+    /// The value of this counter increases only via method `ConnectionKeeper::increase_connection_counter`.
     fn connection_counter(&self) -> Result<u64, ConnectionError>;
 
     /// Validates the `ClientState` of the client on the counterparty chain.

crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs

@@ -58,6 +58,8 @@ where
         msg.client_id_on_a.clone(),
         Counterparty::new(
             msg.counterparty.client_id().clone(),
+            // Note: the counterparty connection Id passed by the relayer alternated to `None`,
+            // left for the `conn_open_try` process to set
             None,
             msg.counterparty.prefix().clone(),
         ),
@@ -118,6 +120,8 @@ pub(crate) fn process(
         msg.client_id_on_a.clone(),
         Counterparty::new(
             msg.counterparty.client_id().clone(),
+            // Note: the counterparty connection Id passed by the relayer alternated to `None`,
+            // left for the `conn_open_try` handler to set
             None,
             msg.counterparty.prefix().clone(),
         ),
@@ -186,7 +190,7 @@ mod tests {
         }
 
         let msg_conn_init_default =
-            MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init()).unwrap();
+            MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init(None)).unwrap();
         let msg_conn_init_no_version = MsgConnectionOpenInit {
             version: None,
             ..msg_conn_init_default.clone()
@@ -200,6 +204,8 @@ mod tests {
             .into(),
             ..msg_conn_init_default.clone()
         };
+        let msg_conn_init_with_couterparty_conn_id_some =
+            MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init(Some(0))).unwrap();
         let default_context = MockContext::default();
         let good_context = default_context.clone().with_client(
             &msg_conn_init_default.client_id_on_a,
@@ -228,6 +234,13 @@ mod tests {
                 expected_versions: ConnectionReader::get_compatible_versions(&good_context),
                 want_pass: true,
             },
+            Test {
+                name: "Counterparty connection id is some in MsgConnectionOpenInit msg".to_string(),
+                ctx: good_context.clone(),
+                msg: ConnectionMsg::OpenInit(msg_conn_init_with_couterparty_conn_id_some),
+                expected_versions: ConnectionReader::get_compatible_versions(&good_context),
+                want_pass: true,
+            },
             Test {
                 name: "Good parameters".to_string(),
                 ctx: good_context,

crates/ibc/src/core/ics03_connection/msgs.rs

@@ -39,10 +39,14 @@ pub mod test_util {
     use ibc_proto::ibc::core::commitment::v1::MerklePrefix;
     use ibc_proto::ibc::core::connection::v1::Counterparty as RawCounterparty;
 
-    pub fn get_dummy_raw_counterparty() -> RawCounterparty {
+    pub fn get_dummy_raw_counterparty(conn_id: Option<u64>) -> RawCounterparty {
+        let connection_id = match conn_id {
+            Some(id) => ConnectionId::new(id).to_string(),
+            None => "".to_string(),
+        };
         RawCounterparty {
             client_id: ClientId::default().to_string(),
-            connection_id: ConnectionId::default().to_string(),
+            connection_id,
             prefix: Some(MerklePrefix {
                 key_prefix: b"ibc".to_vec(),
             }),

crates/ibc/src/core/ics03_connection/msgs/conn_open_init.rs

@@ -92,10 +92,12 @@ pub mod test_util {
 
     /// Returns a dummy message, for testing only.
     /// Other unit tests may import this if they depend on a MsgConnectionOpenInit.
-    pub fn get_dummy_raw_msg_conn_open_init() -> RawMsgConnectionOpenInit {
+    pub fn get_dummy_raw_msg_conn_open_init(
+        counterparty_conn_id: Option<u64>,
+    ) -> RawMsgConnectionOpenInit {
         RawMsgConnectionOpenInit {
             client_id: ClientId::default().to_string(),
-            counterparty: Some(get_dummy_raw_counterparty()),
+            counterparty: Some(get_dummy_raw_counterparty(counterparty_conn_id)),
             version: Some(Version::default().into()),
             delay_period: 0,
             signer: get_dummy_bech32_account(),
@@ -125,7 +127,7 @@ mod tests {
             want_pass: bool,
         }
 
-        let default_init_msg = get_dummy_raw_msg_conn_open_init();
+        let default_init_msg = get_dummy_raw_msg_conn_open_init(None);
 
         let tests: Vec<Test> = vec![
             Test {
@@ -148,7 +150,7 @@ mod tests {
                         connection_id:
                             "abcdefghijksdffjssdkflweldflsfladfsfwjkrekcmmsdfsdfjflddmnopqrstu"
                                 .to_string(),
-                        ..get_dummy_raw_counterparty()
+                        ..get_dummy_raw_counterparty(None)
                     }),
                     ..default_init_msg
                 },
@@ -174,11 +176,33 @@ mod tests {
 
     #[test]
     fn to_and_from() {
-        let raw = get_dummy_raw_msg_conn_open_init();
+        let raw = get_dummy_raw_msg_conn_open_init(None);
         let msg = MsgConnectionOpenInit::try_from(raw.clone()).unwrap();
         let raw_back = RawMsgConnectionOpenInit::from(msg.clone());
         let msg_back = MsgConnectionOpenInit::try_from(raw_back.clone()).unwrap();
         assert_eq!(raw, raw_back);
         assert_eq!(msg, msg_back);
+
+        // Check if handler sets counterparty connection id to `None`
+        // in case relayer passes `MsgConnectionOpenInit` message with it set to `Some(_)`.
+        let raw_with_counterpary_conn_id_some = get_dummy_raw_msg_conn_open_init(None);
+        let msg_with_counterpary_conn_id_some =
+            MsgConnectionOpenInit::try_from(raw_with_counterpary_conn_id_some).unwrap();
+        let raw_with_counterpary_conn_id_some_back =
+            RawMsgConnectionOpenInit::from(msg_with_counterpary_conn_id_some.clone());
+        let msg_with_counterpary_conn_id_some_back =
+            MsgConnectionOpenInit::try_from(raw_with_counterpary_conn_id_some_back.clone())
+                .unwrap();
+        assert_eq!(
+            raw_with_counterpary_conn_id_some_back
+                .counterparty
+                .unwrap()
+                .connection_id,
+            "".to_string()
+        );
+        assert_eq!(
+            msg_with_counterpary_conn_id_some,
+            msg_with_counterpary_conn_id_some_back
+        );
     }
 }

crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs

@@ -178,7 +178,7 @@ pub mod test_util {
             client_id: ClientId::default().to_string(),
             previous_connection_id: ConnectionId::default().to_string(),
             client_state: Some(MockClientState::new(MockHeader::new(client_state_height)).into()),
-            counterparty: Some(get_dummy_raw_counterparty()),
+            counterparty: Some(get_dummy_raw_counterparty(Some(0))),
             delay_period: 0,
             counterparty_versions: get_compatible_versions()
                 .iter()
@@ -247,7 +247,7 @@ mod tests {
                             connection_id:
                             "abcdasdfasdfsdfasfdwefwfsdfsfsfasfwewvxcvdvwgadvaadsefghijklmnopqrstu"
                                 .to_string(),
-                            ..get_dummy_raw_counterparty()
+                            ..get_dummy_raw_counterparty(Some(0))
                         }),
                         ..default_try_msg.clone()
                     },
@@ -259,7 +259,7 @@ mod tests {
                     raw: RawMsgConnectionOpenTry {
                         counterparty: Some(RawCounterparty {
                             client_id: "ClientId_".to_string(),
-                            ..get_dummy_raw_counterparty()
+                            ..get_dummy_raw_counterparty(Some(0))
                         }),
                         ..default_try_msg.clone()
                     },

crates/ibc/src/core/ics04_channel/channel.rs

@@ -336,14 +336,23 @@ impl Protobuf<RawCounterparty> for Counterparty {}
 impl TryFrom<RawCounterparty> for Counterparty {
     type Error = ChannelError;
 
-    fn try_from(value: RawCounterparty) -> Result<Self, Self::Error> {
-        let channel_id = Some(value.channel_id)
-            .filter(|x| !x.is_empty())
-            .map(|v| FromStr::from_str(v.as_str()))
-            .transpose()
-            .map_err(ChannelError::Identifier)?;
+    fn try_from(raw_counterparty: RawCounterparty) -> Result<Self, Self::Error> {
+        let channel_id: Option<ChannelId> = if raw_counterparty.channel_id.is_empty() {
+            None
+        } else {
+            Some(
+                raw_counterparty
+                    .channel_id
+                    .parse()
+                    .map_err(ChannelError::Identifier)?,
+            )
+        };
+
         Ok(Counterparty::new(
-            value.port_id.parse().map_err(ChannelError::Identifier)?,
+            raw_counterparty
+                .port_id
+                .parse()
+                .map_err(ChannelError::Identifier)?,
             channel_id,
         ))
     }
@@ -505,7 +514,7 @@ impl Display for State {
 
 #[cfg(test)]
 pub mod test_util {
-    use crate::core::ics24_host::identifier::{ConnectionId, PortId};
+    use crate::core::ics24_host::identifier::{ChannelId, ConnectionId, PortId};
     use crate::prelude::*;
     use ibc_proto::ibc::core::channel::v1::Channel as RawChannel;
     use ibc_proto::ibc::core::channel::v1::Counterparty as RawCounterparty;
@@ -520,7 +529,11 @@ pub mod test_util {
     }
 
     /// Returns a dummy `RawChannel`, for testing only!
-    pub fn get_dummy_raw_channel_end(channel_id: String) -> RawChannel {
+    pub fn get_dummy_raw_channel_end(channel_id: Option<u64>) -> RawChannel {
+        let channel_id = match channel_id {
+            Some(id) => ChannelId::new(id).to_string(),
+            None => "".to_string(),
+        };
         RawChannel {
             state: 1,
             ordering: 2,
@@ -533,7 +546,6 @@ pub mod test_util {
 
 #[cfg(test)]
 mod tests {
-    use crate::core::ics24_host::identifier::ChannelId;
     use crate::prelude::*;
 
     use core::str::FromStr;
@@ -546,7 +558,7 @@ mod tests {
 
     #[test]
     fn channel_end_try_from_raw() {
-        let raw_channel_end = get_dummy_raw_channel_end(ChannelId::default().to_string());
+        let raw_channel_end = get_dummy_raw_channel_end(Some(0));
 
         let empty_raw_channel_end = RawChannel {
             counterparty: None,

crates/ibc/src/core/ics04_channel/context.rs

@@ -157,8 +157,7 @@ pub trait ChannelReader {
     ) -> Result<Height, ChannelError>;
 
     /// Returns a counter on the number of channel ids have been created thus far.
-    /// The value of this counter should increase only via method
-    /// `ChannelKeeper::increase_channel_counter`.
+    /// The value of this counter increases only via method `ChannelKeeper::increase_channel_counter`.
     fn channel_counter(&self) -> Result<u64, ChannelError>;
 
     /// Returns the maximum expected time per block

crates/ibc/src/core/ics04_channel/handler/chan_close_confirm.rs

@@ -234,7 +234,7 @@ mod tests {
         let conn_end = ConnectionEnd::new(
             ConnectionState::Open,
             client_id.clone(),
-            ConnectionCounterparty::try_from(get_dummy_raw_counterparty()).unwrap(),
+            ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
         );

crates/ibc/src/core/ics04_channel/handler/chan_close_init.rs

@@ -135,7 +135,7 @@ mod tests {
         let conn_end = ConnectionEnd::new(
             ConnectionState::Open,
             client_id.clone(),
-            ConnectionCounterparty::try_from(get_dummy_raw_counterparty()).unwrap(),
+            ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
         );

crates/ibc/src/core/ics04_channel/handler/chan_open_ack.rs

@@ -247,7 +247,7 @@ mod tests {
         let context = MockContext::default();
 
         let msg_conn_init =
-            MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init()).unwrap();
+            MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init(None)).unwrap();
 
         let conn_end = ConnectionEnd::new(
             ConnectionState::Open,

crates/ibc/src/core/ics04_channel/handler/chan_open_confirm.rs

@@ -243,7 +243,7 @@ mod tests {
         let conn_end = ConnectionEnd::new(
             ConnectionState::Open,
             client_id.clone(),
-            ConnectionCounterparty::try_from(get_dummy_raw_counterparty()).unwrap(),
+            ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
         );

crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs

@@ -74,6 +74,8 @@ pub(crate) fn process<Ctx: ChannelReader>(
     let chan_end_on_a = ChannelEnd::new(
         State::Init,
         msg.ordering,
+        // Note: the counterparty channel Id passed by the relayer alternated to `None`,
+        // left for the `chan_open_try` handler to set
         Counterparty::new(msg.port_id_on_b.clone(), None),
         msg.connection_hops_on_a.clone(),
         msg.version_proposal.clone(),
@@ -124,12 +126,15 @@ mod tests {
         }
 
         let msg_chan_init =
-            MsgChannelOpenInit::try_from(get_dummy_raw_msg_chan_open_init()).unwrap();
+            MsgChannelOpenInit::try_from(get_dummy_raw_msg_chan_open_init(None)).unwrap();
+
+        let msg_chan_init_with_counterparty_chan_id_some =
+            MsgChannelOpenInit::try_from(get_dummy_raw_msg_chan_open_init(Some(0))).unwrap();
 
         let context = MockContext::default();
 
         let msg_conn_init =
-            MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init()).unwrap();
+            MsgConnectionOpenInit::try_from(get_dummy_raw_msg_conn_open_init(None)).unwrap();
 
         let init_conn_end = ConnectionEnd::new(
             ConnectionState::Init,
@@ -150,10 +155,19 @@ mod tests {
             },
             Test {
                 name: "Good parameters".to_string(),
-                ctx: context.with_connection(cid, init_conn_end),
+                ctx: context
+                    .clone()
+                    .with_connection(cid.clone(), init_conn_end.clone()),
                 msg: ChannelMsg::OpenInit(msg_chan_init),
                 want_pass: true,
             },
+            Test {
+                name: "Good parameters even if counterparty channel id is set some by relayer"
+                    .to_string(),
+                ctx: context.with_connection(cid, init_conn_end),
+                msg: ChannelMsg::OpenInit(msg_chan_init_with_counterparty_chan_id_some),
+                want_pass: true,
+            },
         ]
         .into_iter()
         .collect();