Additional tests for counterparty conn & chan ids at initialization

.changelog/unreleased/breaking-changes/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

@@ -255,7 +255,7 @@ mod val_exec_ctx {
 
         /// 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, ContextError>;
+        fn generate_client_identifier(&self) -> Result<u64, ContextError>;
 
         /// Returns the ConnectionEnd for the given identifier `conn_id`.
         fn connection_end(&self, conn_id: &ConnectionId) -> Result<ConnectionEnd, ContextError>;
@@ -270,7 +270,7 @@ mod val_exec_ctx {
         fn commitment_prefix(&self) -> CommitmentPrefix;
 
         /// Returns a counter on how many connections have been created thus far.
-        fn connection_counter(&self) -> Result<u64, ContextError>;
+        fn generate_connection_identifier(&self) -> Result<u64, ContextError>;
 
         /// Function required by ICS 03. Returns the list of all possible versions that the connection
         /// handshake protocol supports.
@@ -379,7 +379,7 @@ mod val_exec_ctx {
         /// 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`.
-        fn channel_counter(&self) -> Result<u64, ContextError>;
+        fn generate_channel_identifier(&self) -> Result<u64, ContextError>;
 
         /// Returns the maximum expected time per block
         fn max_expected_time_per_block(&self) -> Duration;
@@ -580,7 +580,7 @@ mod val_exec_ctx {
         ValCtx: ValidationContext,
     {
         chan_open_init::validate(ctx_a, &msg)?;
-        let chan_id_on_a = ChannelId::new(ctx_a.channel_counter()?);
+        let chan_id_on_a = ChannelId::new(ctx_a.generate_channel_identifier()?);
 
         let module = ctx_a
             .get_route(&module_id)
@@ -605,7 +605,7 @@ mod val_exec_ctx {
     where
         ExecCtx: ExecutionContext,
     {
-        let chan_id_on_a = ChannelId::new(ctx_a.channel_counter()?);
+        let chan_id_on_a = ChannelId::new(ctx_a.generate_channel_identifier()?);
         let module = ctx_a
             .get_route_mut(&module_id)
             .ok_or(ChannelError::RouteNotFound)?;
@@ -679,7 +679,7 @@ mod val_exec_ctx {
         ValCtx: ValidationContext,
     {
         chan_open_try::validate(ctx_b, &msg)?;
-        let chan_id_on_b = ChannelId::new(ctx_b.channel_counter()?);
+        let chan_id_on_b = ChannelId::new(ctx_b.generate_channel_identifier()?);
 
         let module = ctx_b
             .get_route(&module_id)
@@ -704,7 +704,10 @@ mod val_exec_ctx {
     where
         ExecCtx: ExecutionContext,
     {
-        let chan_id_on_b = ChannelId::new(ctx_b.channel_counter()?);
+        let chan_id_on_b = ChannelId::new(ctx_b.generate_channel_identifier()?);
+        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,9 +68,9 @@ 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 client identifier which also shows 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>;
+    fn generate_client_identifier(&self) -> Result<u64, ClientError>;
 }
 
 /// Defines the write-only part of ICS2 (client functions) context.

crates/ibc/src/core/ics02_client/handler/create_client.rs

@@ -53,7 +53,7 @@ where
     } = msg;
 
     // Construct this client's identifier
-    let id_counter = ctx.client_counter()?;
+    let id_counter = ctx.generate_client_identifier()?;
 
     let client_state = ctx.decode_client_state(client_state)?;
 
@@ -82,7 +82,7 @@ where
     } = msg;
 
     // Construct this client's identifier
-    let id_counter = ctx.client_counter()?;
+    let id_counter = ctx.generate_client_identifier()?;
 
     let client_state = ctx.decode_client_state(client_state)?;
 
@@ -141,7 +141,7 @@ pub(crate) fn process(
     } = msg;
 
     // Construct this client's identifier
-    let id_counter = ctx.client_counter()?;
+    let id_counter = ctx.generate_client_identifier()?;
 
     let client_state = ctx.decode_client_state(client_state)?;
 

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

@@ -69,10 +69,9 @@ pub trait ConnectionReader {
         pick_version(supported_versions, counterparty_candidate_versions)
     }
 
-    /// 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`.
-    fn connection_counter(&self) -> Result<u64, ConnectionError>;
+    /// Returns a connection identifier which also shows how many connections have been created thus far.
+    /// The value of this counter increases only via method `ConnectionKeeper::increase_connection_counter`.
+    fn generate_connection_identifier(&self) -> Result<u64, ConnectionError>;
 
     /// Validates the `ClientState` of the client on the counterparty chain.
     fn validate_self_client(&self, counterparty_client_state: Any) -> Result<(), ConnectionError>;

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(),
         ),
@@ -66,7 +68,7 @@ where
     );
 
     // Construct the identifier for the new connection.
-    let conn_id_on_a = ConnectionId::new(ctx_a.connection_counter()?);
+    let conn_id_on_a = ConnectionId::new(ctx_a.generate_connection_identifier()?);
 
     ctx_a.log_message(format!(
         "success: conn_open_init: generated new connection identifier: {conn_id_on_a}"
@@ -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(),
         ),
@@ -126,7 +130,7 @@ pub(crate) fn process(
     );
 
     // Construct the identifier for the new connection.
-    let conn_id_on_a = ConnectionId::new(ctx_a.connection_counter()?);
+    let conn_id_on_a = ConnectionId::new(ctx_a.generate_connection_identifier()?);
 
     let result = ConnectionResult {
         connection_id: conn_id_on_a.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/handler/conn_open_try.rs

@@ -191,7 +191,7 @@ impl LocalVars {
             ctx_b.pick_version(&ctx_b.get_compatible_versions(), &msg.versions_on_a)?;
 
         Ok(Self {
-            conn_id_on_b: ConnectionId::new(ctx_b.connection_counter()?),
+            conn_id_on_b: ConnectionId::new(ctx_b.generate_connection_identifier()?),
             conn_end_on_b: ConnectionEnd::new(
                 State::TryOpen,
                 msg.client_id_on_b.clone(),
@@ -216,7 +216,7 @@ pub(crate) fn process(
 ) -> HandlerResult<ConnectionResult, ConnectionError> {
     let mut output = HandlerOutput::builder();
 
-    let conn_id_on_b = ConnectionId::new(ctx_b.connection_counter()?);
+    let conn_id_on_b = ConnectionId::new(ctx_b.generate_connection_identifier()?);
 
     ctx_b.validate_self_client(msg.client_state_of_b_on_a.clone())?;
 

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

@@ -505,7 +505,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 +520,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 +537,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 +549,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

@@ -156,10 +156,9 @@ pub trait ChannelReader {
         height: &Height,
     ) -> 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`.
-    fn channel_counter(&self) -> Result<u64, ChannelError>;
+    /// Returns a channel identifier which also shows the number of channels have been created thus far.
+    /// The value of this counter increases only via method `ChannelKeeper::increase_channel_counter`.
+    fn generate_channel_identifier(&self) -> Result<u64, ChannelError>;
 
     /// Returns the maximum expected time per block
     fn max_expected_time_per_block(&self) -> Duration;

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,
         );