diff --git a/.changelog/unreleased/breaking-changes/20-refactor-channel-msgs.md b/.changelog/unreleased/breaking-changes/20-refactor-channel-msgs.md new file mode 100644 index 0000000000..d72f933a79 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/ibc/src/core/ics04_channel/handler.rs b/crates/ibc/src/core/ics04_channel/handler.rs index 96469cb174..56ca3fd797 100644 --- a/crates/ibc/src/core/ics04_channel/handler.rs +++ b/crates/ibc/src/core/ics04_channel/handler.rs @@ -116,12 +116,12 @@ where match msg { ChannelMsg::ChannelOpenInit(msg) => { let (extras, version) = cb.on_chan_open_init( - msg.chan_end_on_a.ordering, - &msg.chan_end_on_a.connection_hops, + msg.ordering_on_a, + &msg.connection_hops, &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(), msg.chan_id_on_b.clone()), + &msg.version_on_a, )?; result.channel_end.version = version; @@ -129,11 +129,11 @@ where } ChannelMsg::ChannelOpenTry(msg) => { let (extras, version) = cb.on_chan_open_try( - msg.chan_end_on_b.ordering, - &msg.chan_end_on_b.connection_hops, + msg.ordering_on_b, + &msg.connection_hops, &msg.port_id_on_b, &result.channel_id, - msg.chan_end_on_b.counterparty(), + &Counterparty::new(msg.port_id_on_a.clone(), msg.chan_id_on_a.clone()), &msg.version_on_a, )?; result.channel_end.version = version; diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_open_ack.rs b/crates/ibc/src/core/ics04_channel/handler/chan_open_ack.rs index 0c500a7a73..119cc323dd 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_open_ack.rs +++ b/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_on_b, 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_on_a, ); let failed_chan_end = ChannelEnd::new( State::Open, - *msg_chan_try.chan_end_on_b.ordering(), + msg_chan_try.ordering_on_b, 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_on_b.clone(), ); let tests: Vec = vec![ diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs index b4a929d001..4f0f4691f9 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs +++ b/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( ) -> HandlerResult { let mut output = HandlerOutput::builder(); - if msg.chan_end_on_a.connection_hops().len() != 1 { + if msg.connection_hops.len() != 1 { return Err(ChannelError::InvalidConnectionHopsLength { expected: 1, - actual: msg.chan_end_on_a.connection_hops().len(), + actual: msg.connection_hops.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[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_on_a.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_on_a, + Counterparty::new(msg.port_id_on_b.clone(), msg.chan_id_on_b.clone()), + msg.connection_hops.clone(), + msg.version_on_a.clone(), ); let chan_id_on_a = ChannelId::new(ctx_a.channel_counter()?); diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs b/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs index 847be25afe..710dc302ce 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs +++ b/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs @@ -19,17 +19,17 @@ pub(crate) fn process( 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.len() != 1 { return Err(ChannelError::InvalidConnectionHopsLength { expected: 1, - actual: msg.chan_end_on_b.connection_hops().len(), + actual: msg.connection_hops.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[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[0].clone(), }); } @@ -38,7 +38,7 @@ pub(crate) fn process( _ => return Err(ChannelError::InvalidVersionLengthConnection), }; - let channel_feature = msg.chan_end_on_b.ordering().to_string(); + let channel_feature = msg.ordering_on_b.to_string(); if !conn_version.is_supported_feature(channel_feature) { return Err(ChannelError::ChannelFeatureNotSuportedByConnection); } @@ -50,15 +50,14 @@ pub(crate) fn process( 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 port_id_on_a = &&msg.port_id_on_a; let chan_id_on_a = msg - .chan_end_on_b - .counterparty() - .channel_id() + .chan_id_on_a + .clone() .ok_or(ChannelError::InvalidCounterpartyChannelId)?; 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[0].clone(), }, )?; @@ -71,7 +70,7 @@ pub(crate) fn process( let expected_chan_end_on_a = ChannelEnd::new( State::Init, - *msg.chan_end_on_b.ordering(), + msg.ordering_on_b, Counterparty::new(msg.port_id_on_b.clone(), None), vec![conn_id_on_a.clone()], msg.version_on_a.clone(), @@ -86,7 +85,7 @@ pub(crate) fn process( &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)?; @@ -94,9 +93,9 @@ pub(crate) fn process( 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_on_b, + Counterparty::new(msg.port_id_on_a.clone(), msg.chan_id_on_a.clone()), + msg.connection_hops.clone(), // Note: This will be rewritten by the module callback Version::empty(), ); @@ -132,7 +131,7 @@ 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; @@ -176,16 +175,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 = 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_on_b, + Counterparty::new(msg.port_id_on_a.clone(), msg.chan_id_on_a.clone()), + msg.connection_hops.clone(), + msg.version_on_b.clone(), ); let tests: Vec = vec![ @@ -195,7 +194,7 @@ mod tests { msg: ChannelMsg::ChannelOpenTry(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[0].clone(); Box::new(move |e| match e { error::ChannelError::Connection(e) => { assert_eq!( @@ -320,18 +319,18 @@ 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; + msg.chan_id_on_a = None; let chan_id = ChannelId::new(24); let hops = vec![conn_id.clone()]; - msg.chan_end_on_b.connection_hops = hops; + msg.connection_hops = 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_on_b, + Counterparty::new(msg.port_id_on_b.clone(), msg.chan_id_on_a.clone()), + msg.connection_hops.clone(), + msg.version_on_b.clone(), ); let context = MockContext::default() .with_client(&client_id, Height::new(0, proof_height).unwrap()) diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs index 570bbda0e9..069dc51348 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs @@ -1,6 +1,9 @@ use crate::core::ics04_channel::channel::ChannelEnd; +use crate::core::ics04_channel::channel::Counterparty; +use crate::core::ics04_channel::channel::{Order, State}; use crate::core::ics04_channel::error::ChannelError; -use crate::core::ics24_host::identifier::PortId; +use crate::core::ics04_channel::Version; +use crate::core::ics24_host::identifier::{ChannelId, ConnectionId, PortId}; use crate::prelude::*; use crate::signer::Signer; use crate::tx_msg::Msg; @@ -17,15 +20,31 @@ pub const TYPE_URL: &str = "/ibc.core.channel.v1.MsgChannelOpenInit"; #[derive(Clone, Debug, PartialEq, Eq)] pub struct MsgChannelOpenInit { pub port_id_on_a: PortId, - pub chan_end_on_a: ChannelEnd, + pub ordering_on_a: Order, + pub version_on_a: Version, + pub port_id_on_b: PortId, + pub chan_id_on_b: Option, + pub connection_hops: Vec, pub signer: Signer, } impl MsgChannelOpenInit { - pub fn new(port_id_on_a: PortId, chan_end_on_a: ChannelEnd, signer: Signer) -> Self { + pub fn new( + port_id_on_a: PortId, + ordering_on_a: Order, + port_id_on_b: PortId, + version_on_a: Version, + chan_id_on_b: Option, + connection_hops: Vec, + signer: Signer, + ) -> Self { Self { port_id_on_a, - chan_end_on_a, + ordering_on_a, + version_on_a, + port_id_on_b, + chan_id_on_b, + connection_hops, signer, } } @@ -45,22 +64,33 @@ impl TryFrom for MsgChannelOpenInit { type Error = ChannelError; fn try_from(raw_msg: RawMsgChannelOpenInit) -> Result { + let chan_end_on_a: ChannelEnd = raw_msg + .channel + .ok_or(ChannelError::MissingChannel)? + .try_into()?; Ok(MsgChannelOpenInit { port_id_on_a: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, - chan_end_on_a: raw_msg - .channel - .ok_or(ChannelError::MissingChannel)? - .try_into()?, + ordering_on_a: chan_end_on_a.ordering, + port_id_on_b: chan_end_on_a.remote.port_id, + chan_id_on_b: chan_end_on_a.remote.channel_id, + connection_hops: chan_end_on_a.connection_hops, + version_on_a: chan_end_on_a.version, signer: raw_msg.signer.parse().map_err(ChannelError::Signer)?, }) } } - impl From for RawMsgChannelOpenInit { fn from(domain_msg: MsgChannelOpenInit) -> Self { + let chan_end_on_a = ChannelEnd::new( + State::Init, + domain_msg.ordering_on_a, + Counterparty::new(domain_msg.port_id_on_b, domain_msg.chan_id_on_b), + domain_msg.connection_hops, + domain_msg.version_on_a, + ); RawMsgChannelOpenInit { port_id: domain_msg.port_id_on_a.to_string(), - channel: Some(domain_msg.chan_end_on_a.into()), + channel: Some(chan_end_on_a.into()), signer: domain_msg.signer.to_string(), } } diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs index cadbeed550..3e76b4e01a 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs @@ -1,9 +1,11 @@ use crate::core::ics04_channel::channel::ChannelEnd; +use crate::core::ics04_channel::channel::Counterparty; +use crate::core::ics04_channel::channel::{Order, State}; use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::Version; use crate::core::ics23_commitment::commitment::CommitmentProofBytes; use crate::core::ics24_host::error::ValidationError; -use crate::core::ics24_host::identifier::PortId; +use crate::core::ics24_host::identifier::{ChannelId, ConnectionId, PortId}; use crate::signer::Signer; use crate::tx_msg::Msg; use crate::{prelude::*, Height}; @@ -20,8 +22,12 @@ pub const TYPE_URL: &str = "/ibc.core.channel.v1.MsgChannelOpenTry"; #[derive(Clone, Debug, PartialEq, Eq)] pub struct MsgChannelOpenTry { pub port_id_on_b: PortId, - pub chan_end_on_b: ChannelEnd, + pub ordering_on_b: Order, + pub version_on_b: Version, + pub port_id_on_a: PortId, + pub chan_id_on_a: Option, pub version_on_a: Version, + pub connection_hops: Vec, pub proof_chan_end_on_a: CommitmentProofBytes, pub proof_height_on_a: Height, pub signer: Signer, @@ -32,10 +38,15 @@ pub struct MsgChannelOpenTry { } impl MsgChannelOpenTry { + #[allow(clippy::too_many_arguments)] pub fn new( port_id_on_b: PortId, - chan_end_on_b: ChannelEnd, + ordering_on_b: Order, + version_on_b: Version, + port_id_on_a: PortId, + chan_id_on_a: Option, version_on_a: Version, + connection_hops: Vec, proof_chan_end_on_a: CommitmentProofBytes, proof_height_on_a: Height, signer: Signer, @@ -43,8 +54,12 @@ impl MsgChannelOpenTry { #[allow(deprecated)] Self { port_id_on_b, - chan_end_on_b, + ordering_on_b, + version_on_b, + port_id_on_a, + chan_id_on_a, version_on_a, + connection_hops, proof_chan_end_on_a, proof_height_on_a, signer, @@ -67,15 +82,20 @@ impl TryFrom for MsgChannelOpenTry { type Error = ChannelError; fn try_from(raw_msg: RawMsgChannelOpenTry) -> Result { + let chan_end_on_b: ChannelEnd = raw_msg + .channel + .ok_or(ChannelError::MissingChannel)? + .try_into()?; #[allow(deprecated)] let msg = MsgChannelOpenTry { port_id_on_b: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, + ordering_on_b: chan_end_on_b.ordering, previous_channel_id: raw_msg.previous_channel_id, - chan_end_on_b: raw_msg - .channel - .ok_or(ChannelError::MissingChannel)? - .try_into()?, + version_on_b: chan_end_on_b.version.clone(), + port_id_on_a: chan_end_on_b.remote.port_id, + chan_id_on_a: chan_end_on_b.remote.channel_id, version_on_a: raw_msg.counterparty_version.into(), + connection_hops: chan_end_on_b.connection_hops, proof_chan_end_on_a: raw_msg .proof_init .try_into() @@ -87,7 +107,7 @@ impl TryFrom for MsgChannelOpenTry { signer: raw_msg.signer.parse().map_err(ChannelError::Signer)?, }; - match msg.chan_end_on_b.counterparty().channel_id() { + match msg.chan_id_on_a.clone() { None => Err(ValidationError::InvalidCounterpartyChannelId), Some(_c) => Ok(()), } @@ -99,11 +119,18 @@ impl TryFrom for MsgChannelOpenTry { impl From for RawMsgChannelOpenTry { fn from(domain_msg: MsgChannelOpenTry) -> Self { + let chan_end_on_b = ChannelEnd::new( + State::Init, + domain_msg.ordering_on_b, + Counterparty::new(domain_msg.port_id_on_a, domain_msg.chan_id_on_a), + domain_msg.connection_hops, + domain_msg.version_on_b.clone(), + ); #[allow(deprecated)] RawMsgChannelOpenTry { port_id: domain_msg.port_id_on_b.to_string(), previous_channel_id: domain_msg.previous_channel_id, - channel: Some(domain_msg.chan_end_on_b.into()), + channel: Some(chan_end_on_b.into()), counterparty_version: domain_msg.version_on_a.to_string(), proof_init: domain_msg.proof_chan_end_on_a.clone().into(), proof_height: Some(domain_msg.proof_height_on_a.into()), diff --git a/crates/ibc/src/core/ics26_routing/handler.rs b/crates/ibc/src/core/ics26_routing/handler.rs index 4f4b33f431..dafb460c5b 100644 --- a/crates/ibc/src/core/ics26_routing/handler.rs +++ b/crates/ibc/src/core/ics26_routing/handler.rs @@ -317,7 +317,7 @@ mod tests { // The handler will fail to process this b/c the associated connection does not exist let mut incorrect_msg_chan_init = msg_chan_init.clone(); - incorrect_msg_chan_init.chan_end_on_a.connection_hops = vec![ConnectionId::new(590)]; + incorrect_msg_chan_init.connection_hops = vec![ConnectionId::new(590)]; let msg_chan_try = MsgChannelOpenTry::try_from(get_dummy_raw_msg_chan_open_try(client_height)).unwrap();