Exclude ChannelEnd from MsgChannelOpenInit and MsgChannelOpenTry (

#327)

* Exclude ChannelEnd from MsgChannelOpenInit and MsgChannelOpenTry

* Apply naming convention on connection_hops

* Revise naming of some fields

* changelog fix

Co-authored-by: Philippe Laferriere <[email protected]>

.changelog/unreleased/breaking-changes/20-refactor-channel-msgs.md

@@ -0,0 +1,2 @@
+- Exclude `ChannelEnd` from `MsgChannelOpenInit` and `MsgChannelOpenTry` and refactor their fields to match the spec
+  ([#20](https://github.com/cosmos/ibc-rs/issues/20))

.changelog/unreleased/breaking-changes/338-make-process-pub-crate.md

@@ -0,0 +1,2 @@
+- Make internal `process()` `pub(crate)` 
+  ([#338](https://github.com/cosmos/ibc-rs/issues/338))

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

@@ -501,34 +501,35 @@ impl Display for State {
 
 #[cfg(test)]
 pub mod test_util {
-    use crate::core::ics24_host::identifier::{ChannelId, ConnectionId, PortId};
+    use crate::core::ics24_host::identifier::{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;
 
     /// Returns a dummy `RawCounterparty`, for testing only!
     /// Can be optionally parametrized with a specific channel identifier.
-    pub fn get_dummy_raw_counterparty() -> RawCounterparty {
+    pub fn get_dummy_raw_counterparty(channel_id: String) -> RawCounterparty {
         RawCounterparty {
             port_id: PortId::default().to_string(),
-            channel_id: ChannelId::default().to_string(),
+            channel_id,
         }
     }
 
     /// Returns a dummy `RawChannel`, for testing only!
-    pub fn get_dummy_raw_channel_end() -> RawChannel {
+    pub fn get_dummy_raw_channel_end(channel_id: String) -> RawChannel {
         RawChannel {
             state: 1,
             ordering: 2,
-            counterparty: Some(get_dummy_raw_counterparty()),
+            counterparty: Some(get_dummy_raw_counterparty(channel_id)),
             connection_hops: vec![ConnectionId::default().to_string()],
-            version: "ics20".to_string(), // The version is not validated.
+            version: "".to_string(), // The version is not validated.
         }
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use crate::core::ics24_host::identifier::ChannelId;
     use crate::prelude::*;
 
     use core::str::FromStr;
@@ -541,7 +542,7 @@ mod tests {
 
     #[test]
     fn channel_end_try_from_raw() {
-        let raw_channel_end = get_dummy_raw_channel_end();
+        let raw_channel_end = get_dummy_raw_channel_end(ChannelId::default().to_string());
 
         let empty_raw_channel_end = RawChannel {
             counterparty: None,

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

@@ -115,25 +115,25 @@ where
     match msg {
         ChannelMsg::OpenInit(msg) => {
             let (extras, version) = cb.on_chan_open_init(
-                msg.chan_end_on_a.ordering,
-                &msg.chan_end_on_a.connection_hops,
+                msg.ordering,
+                &msg.connection_hops_on_a,
                 &msg.port_id_on_a,
                 &result.channel_id,
-                msg.chan_end_on_a.counterparty(),
-                &msg.chan_end_on_a.version,
+                &Counterparty::new(msg.port_id_on_b.clone(), None),
+                &msg.version_proposal,
             )?;
             result.channel_end.version = version;
 
             Ok(extras)
         }
         ChannelMsg::OpenTry(msg) => {
             let (extras, version) = cb.on_chan_open_try(
-                msg.chan_end_on_b.ordering,
-                &msg.chan_end_on_b.connection_hops,
+                msg.ordering,
+                &msg.connection_hops_on_b,
                 &msg.port_id_on_b,
                 &result.channel_id,
-                msg.chan_end_on_b.counterparty(),
-                &msg.version_on_a,
+                &Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_a.clone())),
+                &msg.version_supported_on_a,
             )?;
             result.channel_end.version = version;
 

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

@@ -198,24 +198,24 @@ mod tests {
 
         let chan_end = ChannelEnd::new(
             State::Init,
-            *msg_chan_try.chan_end_on_b.ordering(),
+            msg_chan_try.ordering,
             Counterparty::new(
                 msg_chan_ack.port_id_on_a.clone(),
                 Some(msg_chan_ack.chan_id_on_a.clone()),
             ),
             connection_vec0.clone(),
-            msg_chan_try.chan_end_on_b.version().clone(),
+            msg_chan_try.version_supported_on_a.clone(),
         );
 
         let failed_chan_end = ChannelEnd::new(
             State::Open,
-            *msg_chan_try.chan_end_on_b.ordering(),
+            msg_chan_try.ordering,
             Counterparty::new(
                 msg_chan_ack.port_id_on_a.clone(),
                 Some(msg_chan_ack.chan_id_on_a.clone()),
             ),
             connection_vec0,
-            msg_chan_try.chan_end_on_b.version().clone(),
+            msg_chan_try.version_supported_on_a,
         );
 
         let tests: Vec<Test> = vec![

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

@@ -1,6 +1,6 @@
 //! Protocol logic specific to ICS4 messages of type `MsgChannelOpenInit`.
 
-use crate::core::ics04_channel::channel::{ChannelEnd, State};
+use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State};
 use crate::core::ics04_channel::context::ChannelReader;
 use crate::core::ics04_channel::error::ChannelError;
 use crate::core::ics04_channel::handler::{ChannelIdState, ChannelResult};
@@ -16,32 +16,32 @@ pub(crate) fn process<Ctx: ChannelReader>(
 ) -> HandlerResult<ChannelResult, ChannelError> {
     let mut output = HandlerOutput::builder();
 
-    if msg.chan_end_on_a.connection_hops().len() != 1 {
+    if msg.connection_hops_on_a.len() != 1 {
         return Err(ChannelError::InvalidConnectionHopsLength {
             expected: 1,
-            actual: msg.chan_end_on_a.connection_hops().len(),
+            actual: msg.connection_hops_on_a.len(),
         });
     }
 
     // An IBC connection running on the local (host) chain should exist.
-    let conn_end_on_a = ctx_a.connection_end(&msg.chan_end_on_a.connection_hops()[0])?;
+    let conn_end_on_a = ctx_a.connection_end(&msg.connection_hops_on_a[0])?;
 
     let conn_version = match conn_end_on_a.versions() {
         [version] => version,
         _ => return Err(ChannelError::InvalidVersionLengthConnection),
     };
 
-    let channel_feature = msg.chan_end_on_a.ordering().to_string();
+    let channel_feature = msg.ordering.to_string();
     if !conn_version.is_supported_feature(channel_feature) {
         return Err(ChannelError::ChannelFeatureNotSuportedByConnection);
     }
 
     let chan_end_on_a = ChannelEnd::new(
         State::Init,
-        *msg.chan_end_on_a.ordering(),
-        msg.chan_end_on_a.counterparty().clone(),
-        msg.chan_end_on_a.connection_hops().clone(),
-        msg.chan_end_on_a.version().clone(),
+        msg.ordering,
+        Counterparty::new(msg.port_id_on_b.clone(), None),
+        msg.connection_hops_on_a.clone(),
+        msg.version_proposal.clone(),
     );
 
     let chan_id_on_a = ChannelId::new(ctx_a.channel_counter()?);

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

@@ -19,17 +19,17 @@ pub(crate) fn process<Ctx: ChannelReader>(
     let mut output = HandlerOutput::builder();
 
     // An IBC connection running on the local (host) chain should exist.
-    if msg.chan_end_on_b.connection_hops().len() != 1 {
+    if msg.connection_hops_on_b.len() != 1 {
         return Err(ChannelError::InvalidConnectionHopsLength {
             expected: 1,
-            actual: msg.chan_end_on_b.connection_hops().len(),
+            actual: msg.connection_hops_on_b.len(),
         });
     }
 
-    let conn_end_on_b = ctx_b.connection_end(&msg.chan_end_on_b.connection_hops()[0])?;
+    let conn_end_on_b = ctx_b.connection_end(&msg.connection_hops_on_b[0])?;
     if !conn_end_on_b.state_matches(&ConnectionState::Open) {
         return Err(ChannelError::ConnectionNotOpen {
-            connection_id: msg.chan_end_on_b.connection_hops()[0].clone(),
+            connection_id: msg.connection_hops_on_b[0].clone(),
         });
     }
 
@@ -38,7 +38,7 @@ pub(crate) fn process<Ctx: ChannelReader>(
         _ => return Err(ChannelError::InvalidVersionLengthConnection),
     };
 
-    let channel_feature = msg.chan_end_on_b.ordering().to_string();
+    let channel_feature = msg.ordering.to_string();
     if !conn_version.is_supported_feature(channel_feature) {
         return Err(ChannelError::ChannelFeatureNotSuportedByConnection);
     }
@@ -50,15 +50,11 @@ pub(crate) fn process<Ctx: ChannelReader>(
         let consensus_state_of_a_on_b =
             ctx_b.client_consensus_state(client_id_on_b, &msg.proof_height_on_a)?;
         let prefix_on_a = conn_end_on_b.counterparty().prefix();
-        let port_id_on_a = &msg.chan_end_on_b.counterparty().port_id;
-        let chan_id_on_a = msg
-            .chan_end_on_b
-            .counterparty()
-            .channel_id()
-            .ok_or(ChannelError::InvalidCounterpartyChannelId)?;
+        let port_id_on_a = &&msg.port_id_on_a;
+        let chan_id_on_a = msg.chan_id_on_a.clone();
         let conn_id_on_a = conn_end_on_b.counterparty().connection_id().ok_or(
             ChannelError::UndefinedConnectionCounterparty {
-                connection_id: msg.chan_end_on_b.connection_hops()[0].clone(),
+                connection_id: msg.connection_hops_on_b[0].clone(),
             },
         )?;
 
@@ -71,10 +67,10 @@ pub(crate) fn process<Ctx: ChannelReader>(
 
         let expected_chan_end_on_a = ChannelEnd::new(
             State::Init,
-            *msg.chan_end_on_b.ordering(),
+            msg.ordering,
             Counterparty::new(msg.port_id_on_b.clone(), None),
             vec![conn_id_on_a.clone()],
-            msg.version_on_a.clone(),
+            msg.version_supported_on_a.clone(),
         );
 
         // Verify the proof for the channel state against the expected channel end.
@@ -86,17 +82,17 @@ pub(crate) fn process<Ctx: ChannelReader>(
                 &msg.proof_chan_end_on_a,
                 consensus_state_of_a_on_b.root(),
                 port_id_on_a,
-                chan_id_on_a,
+                &chan_id_on_a,
                 &expected_chan_end_on_a,
             )
             .map_err(ChannelError::VerifyChannelFailed)?;
     }
 
     let chan_end_on_b = ChannelEnd::new(
         State::TryOpen,
-        *msg.chan_end_on_b.ordering(),
-        msg.chan_end_on_b.counterparty().clone(),
-        msg.chan_end_on_b.connection_hops().clone(),
+        msg.ordering,
+        Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_a.clone())),
+        msg.connection_hops_on_b.clone(),
         // Note: This will be rewritten by the module callback
         Version::empty(),
     );
@@ -132,11 +128,12 @@ mod tests {
     use crate::core::ics03_connection::error as ics03_error;
     use crate::core::ics03_connection::msgs::test_util::get_dummy_raw_counterparty;
     use crate::core::ics03_connection::version::get_compatible_versions;
-    use crate::core::ics04_channel::channel::{ChannelEnd, State};
+    use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State};
     use crate::core::ics04_channel::error;
     use crate::core::ics04_channel::msgs::chan_open_try::test_util::get_dummy_raw_msg_chan_open_try;
     use crate::core::ics04_channel::msgs::chan_open_try::MsgChannelOpenTry;
     use crate::core::ics04_channel::msgs::ChannelMsg;
+    use crate::core::ics04_channel::Version;
     use crate::core::ics24_host::identifier::{ChannelId, ClientId, ConnectionId};
     use crate::mock::client_state::client_type as mock_client_type;
     use crate::mock::context::MockContext;
@@ -176,16 +173,16 @@ mod tests {
 
         let chan_id = ChannelId::new(24);
         let hops = vec![conn_id.clone()];
-        msg.chan_end_on_b.connection_hops = hops;
+        msg.connection_hops_on_b = hops;
 
         // A preloaded channel end that resides in the context. This is constructed so as to be
         // consistent with the incoming ChanOpenTry message `msg`.
         let correct_chan_end = ChannelEnd::new(
             State::Init,
-            *msg.chan_end_on_b.ordering(),
-            msg.chan_end_on_b.counterparty().clone(),
-            msg.chan_end_on_b.connection_hops().clone(),
-            msg.chan_end_on_b.version().clone(),
+            msg.ordering,
+            Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_a.clone())),
+            msg.connection_hops_on_b.clone(),
+            Version::empty(),
         );
 
         let tests: Vec<Test> = vec![
@@ -195,7 +192,7 @@ mod tests {
                 msg: ChannelMsg::OpenTry(msg.clone()),
                 want_pass: false,
                 match_error: {
-                    let connection_id = msg.chan_end_on_b.connection_hops()[0].clone();
+                    let connection_id = msg.connection_hops_on_b[0].clone();
                     Box::new(move |e| match e {
                         error::ChannelError::Connection(e) => {
                             assert_eq!(
@@ -320,18 +317,17 @@ mod tests {
         // Note: we make the counterparty's channel_id `None`.
         let mut msg =
             MsgChannelOpenTry::try_from(get_dummy_raw_msg_chan_open_try(proof_height)).unwrap();
-        msg.chan_end_on_b.remote.channel_id = None;
 
         let chan_id = ChannelId::new(24);
         let hops = vec![conn_id.clone()];
-        msg.chan_end_on_b.connection_hops = hops;
+        msg.connection_hops_on_b = hops;
 
         let chan_end = ChannelEnd::new(
             State::Init,
-            *msg.chan_end_on_b.ordering(),
-            msg.chan_end_on_b.counterparty().clone(),
-            msg.chan_end_on_b.connection_hops().clone(),
-            msg.chan_end_on_b.version().clone(),
+            msg.ordering,
+            Counterparty::new(msg.port_id_on_a.clone(), None),
+            msg.connection_hops_on_b.clone(),
+            Version::empty(),
         );
         let context = MockContext::default()
             .with_client(&client_id, Height::new(0, proof_height).unwrap())