diff --git a/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md b/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md new file mode 100644 index 000000000..73561e662 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md @@ -0,0 +1,2 @@ +- Refactor and fix version validation in connection and channel handshakes +([#625](https://github.com/cosmos/ibc-rs/issues/625)) \ No newline at end of file diff --git a/crates/ibc/src/applications/transfer/context.rs b/crates/ibc/src/applications/transfer/context.rs index 192ac5353..d5f1e2534 100644 --- a/crates/ibc/src/applications/transfer/context.rs +++ b/crates/ibc/src/applications/transfer/context.rs @@ -1,3 +1,4 @@ +use crate::core::ContextError; use crate::prelude::*; use sha2::{Digest, Sha256}; @@ -134,11 +135,10 @@ pub fn on_chan_open_init_validate( }); } - if !version.is_empty() && version != &Version::new(VERSION.to_string()) { - return Err(TokenTransferError::InvalidVersion { - expect_version: Version::new(VERSION.to_string()), - got_version: version.clone(), - }); + if !version.is_empty() { + version + .verify_is_expected(Version::new(VERSION.to_string())) + .map_err(ContextError::from)?; } Ok(()) @@ -173,12 +173,10 @@ pub fn on_chan_open_try_validate( got_order: order, }); } - if counterparty_version != &Version::new(VERSION.to_string()) { - return Err(TokenTransferError::InvalidCounterpartyVersion { - expect_version: Version::new(VERSION.to_string()), - got_version: counterparty_version.clone(), - }); - } + + counterparty_version + .verify_is_expected(Version::new(VERSION.to_string())) + .map_err(ContextError::from)?; Ok(()) } @@ -202,12 +200,9 @@ pub fn on_chan_open_ack_validate( _channel_id: &ChannelId, counterparty_version: &Version, ) -> Result<(), TokenTransferError> { - if counterparty_version != &Version::new(VERSION.to_string()) { - return Err(TokenTransferError::InvalidCounterpartyVersion { - expect_version: Version::new(VERSION.to_string()), - got_version: counterparty_version.clone(), - }); - } + counterparty_version + .verify_is_expected(Version::new(VERSION.to_string())) + .map_err(ContextError::from)?; Ok(()) } diff --git a/crates/ibc/src/applications/transfer/error.rs b/crates/ibc/src/applications/transfer/error.rs index dcbb6b8be..8c00a69a5 100644 --- a/crates/ibc/src/applications/transfer/error.rs +++ b/crates/ibc/src/applications/transfer/error.rs @@ -5,7 +5,6 @@ use ibc_proto::protobuf::Error as TendermintProtoError; use uint::FromDecStrErr; use crate::core::ics04_channel::channel::Order; -use crate::core::ics04_channel::Version; use crate::core::ics24_host::error::ValidationError; use crate::core::ics24_host::identifier::{ChannelId, PortId}; use crate::core::ContextError; @@ -60,16 +59,6 @@ pub enum TokenTransferError { expect_order: Order, got_order: Order, }, - /// expected version `{expect_version}` , got `{got_version}` - InvalidVersion { - expect_version: Version, - got_version: Version, - }, - /// expected counterparty version `{expect_version}`, got `{got_version}` - InvalidCounterpartyVersion { - expect_version: Version, - got_version: Version, - }, /// channel cannot be closed CantCloseChannel, /// failed to deserialize packet data @@ -132,3 +121,9 @@ impl From for TokenTransferError { match e {} } } + +impl From for TokenTransferError { + fn from(err: ContextError) -> TokenTransferError { + Self::ContextError(err) + } +} diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs index daffc2e9e..e22a89e25 100644 --- a/crates/ibc/src/core/context.rs +++ b/crates/ibc/src/core/context.rs @@ -319,11 +319,13 @@ pub trait ValidationContext: Router { /// connection handshake protocol prefers. fn pick_version( &self, - supported_versions: &[ConnectionVersion], counterparty_candidate_versions: &[ConnectionVersion], ) -> Result { - pick_version(supported_versions, counterparty_candidate_versions) - .map_err(ContextError::ConnectionError) + let version = pick_version( + &self.get_compatible_versions(), + counterparty_candidate_versions, + )?; + Ok(version) } /// Returns the ChannelEnd for the given `port_id` and `chan_id`. diff --git a/crates/ibc/src/core/context/acknowledgement.rs b/crates/ibc/src/core/context/acknowledgement.rs index b656aa2bd..3eadd1a0f 100644 --- a/crates/ibc/src/core/context/acknowledgement.rs +++ b/crates/ibc/src/core/context/acknowledgement.rs @@ -206,7 +206,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); Fixture { ctx, diff --git a/crates/ibc/src/core/context/chan_close_confirm.rs b/crates/ibc/src/core/context/chan_close_confirm.rs index ad49f6a71..7a8e3e6df 100644 --- a/crates/ibc/src/core/context/chan_close_confirm.rs +++ b/crates/ibc/src/core/context/chan_close_confirm.rs @@ -135,7 +135,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg_chan_close_confirm = MsgChannelCloseConfirm::try_from( get_dummy_raw_msg_chan_close_confirm(client_consensus_state_height.revision_height()), diff --git a/crates/ibc/src/core/context/chan_close_init.rs b/crates/ibc/src/core/context/chan_close_init.rs index 2810f373e..338b5251c 100644 --- a/crates/ibc/src/core/context/chan_close_init.rs +++ b/crates/ibc/src/core/context/chan_close_init.rs @@ -133,7 +133,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg_chan_close_init = MsgChannelCloseInit::try_from(get_dummy_raw_msg_chan_close_init()).unwrap(); diff --git a/crates/ibc/src/core/context/chan_open_ack.rs b/crates/ibc/src/core/context/chan_open_ack.rs index 214547f60..4d12124ac 100644 --- a/crates/ibc/src/core/context/chan_open_ack.rs +++ b/crates/ibc/src/core/context/chan_open_ack.rs @@ -149,7 +149,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg = MsgChannelOpenAck::try_from(get_dummy_raw_msg_chan_open_ack(proof_height)).unwrap(); diff --git a/crates/ibc/src/core/context/chan_open_confirm.rs b/crates/ibc/src/core/context/chan_open_confirm.rs index a604b12db..b2b657008 100644 --- a/crates/ibc/src/core/context/chan_open_confirm.rs +++ b/crates/ibc/src/core/context/chan_open_confirm.rs @@ -156,7 +156,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg = MsgChannelOpenConfirm::try_from(get_dummy_raw_msg_chan_open_confirm(proof_height)) diff --git a/crates/ibc/src/core/context/chan_open_init.rs b/crates/ibc/src/core/context/chan_open_init.rs index d8c7760b1..bd14acc89 100644 --- a/crates/ibc/src/core/context/chan_open_init.rs +++ b/crates/ibc/src/core/context/chan_open_init.rs @@ -162,7 +162,8 @@ mod tests { msg_conn_init.counterparty.clone(), get_compatible_versions(), msg_conn_init.delay_period, - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/context/chan_open_try.rs b/crates/ibc/src/core/context/chan_open_try.rs index 0f59124ee..dc8379662 100644 --- a/crates/ibc/src/core/context/chan_open_try.rs +++ b/crates/ibc/src/core/context/chan_open_try.rs @@ -170,7 +170,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); // We're going to test message processing against this message. // Note: we make the counterparty's channel_id `None`. diff --git a/crates/ibc/src/core/context/recv_packet.rs b/crates/ibc/src/core/context/recv_packet.rs index dc672e9e0..be2955449 100644 --- a/crates/ibc/src/core/context/recv_packet.rs +++ b/crates/ibc/src/core/context/recv_packet.rs @@ -224,7 +224,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/context/timeout.rs b/crates/ibc/src/core/context/timeout.rs index 104c185d4..84740ef96 100644 --- a/crates/ibc/src/core/context/timeout.rs +++ b/crates/ibc/src/core/context/timeout.rs @@ -238,7 +238,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); Fixture { ctx, diff --git a/crates/ibc/src/core/handler.rs b/crates/ibc/src/core/handler.rs index d8b9facae..e51488bec 100644 --- a/crates/ibc/src/core/handler.rs +++ b/crates/ibc/src/core/handler.rs @@ -524,7 +524,8 @@ mod tests { ), vec![ConnVersion::default()], Duration::MAX, - ), + ) + .unwrap(), ); let module = DummyTransferModule::new(); diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index 660dbe198..3b389d42c 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -227,7 +227,7 @@ impl TryFrom for ConnectionEnd { return Err(ConnectionError::EmptyProtoConnectionEnd); } - Ok(Self::new( + Self::new( state, value .client_id @@ -243,7 +243,7 @@ impl TryFrom for ConnectionEnd { .map(Version::try_from) .collect::, _>>()?, Duration::from_nanos(value.delay_period), - )) + ) } } @@ -270,14 +270,21 @@ impl ConnectionEnd { counterparty: Counterparty, versions: Vec, delay_period: Duration, - ) -> Self { - Self { + ) -> Result { + // Note: `versions`'s semantics vary based on the `State` of the connection: + // + Init: contains the set of compatible versions, + // + TryOpen/Open: contains the single version chosen by the handshake protocol. + if state != State::Init && versions.len() != 1 { + return Err(ConnectionError::InvalidVersionLength); + } + + Ok(Self { state, client_id, counterparty, versions, delay_period, - } + }) } /// Getter for the state of this connection end. diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs index e70ae8de2..32e88443f 100644 --- a/crates/ibc/src/core/ics03_connection/error.rs +++ b/crates/ibc/src/core/ics03_connection/error.rs @@ -31,12 +31,18 @@ pub enum ConnectionError { EmptyProtoConnectionEnd, /// empty supported versions EmptyVersions, - /// empty supported features - EmptyFeatures, - /// no common version - NoCommonVersion, + /// single version must be negotiated on connection before opening channel + InvalidVersionLength, /// version \"`{version}`\" not supported VersionNotSupported { version: Version }, + /// no common version + NoCommonVersion, + /// empty supported features + EmptyFeatures, + /// feature \"`{feature}`\" not supported + FeatureNotSupported { feature: String }, + /// no common features + NoCommonFeatures, /// missing proof height MissingProofHeight, /// missing consensus height diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs index cc9264085..207f892d6 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs @@ -45,9 +45,10 @@ where ctx_a.validate_self_client(msg.client_state_of_a_on_b.clone())?; - if !(vars.conn_end_on_a.state_matches(&State::Init) - && vars.conn_end_on_a.versions().contains(&msg.version)) - { + msg.version + .verify_is_supported(vars.conn_end_on_a.versions())?; + + if !vars.conn_end_on_a.state_matches(&State::Init) { return Err(ConnectionError::ConnectionMismatch { connection_id: msg.conn_id_on_a.clone(), } @@ -88,7 +89,7 @@ where ), vec![msg.version.clone()], vars.conn_end_on_a.delay_period(), - ); + )?; client_state_of_b_on_a .verify_membership( @@ -261,7 +262,8 @@ mod tests { ), vec![msg.version.clone()], ZERO_DURATION, - ); + ) + .unwrap(); // A connection end with incorrect state `Open`; will be part of the context. let mut conn_end_open = default_conn_end.clone(); diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs index a2684cc02..091202b42 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs @@ -74,7 +74,7 @@ where ), conn_end_on_b.versions().to_vec(), conn_end_on_b.delay_period(), - ); + )?; client_state_of_a_on_b .verify_membership( @@ -213,7 +213,8 @@ mod tests { counterparty, ValidationContext::get_compatible_versions(&ctx_default), ZERO_DURATION, - ); + ) + .unwrap(); let mut correct_conn_end = incorrect_conn_end_state.clone(); correct_conn_end.set_state(State::TryOpen); diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs index f4901e4fa..61661a1f0 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs @@ -3,7 +3,6 @@ use crate::prelude::*; use crate::core::context::ContextError; use crate::core::ics03_connection::connection::{ConnectionEnd, Counterparty, State}; -use crate::core::ics03_connection::error::ConnectionError; use crate::core::ics03_connection::events::OpenInit; use crate::core::ics03_connection::msgs::conn_open_init::MsgConnectionOpenInit; use crate::core::ics24_host::identifier::ConnectionId; @@ -20,9 +19,7 @@ where client_state_of_b_on_a.confirm_not_frozen()?; if let Some(version) = msg.version { - if !ctx_a.get_compatible_versions().contains(&version) { - return Err(ConnectionError::VersionNotSupported { version }.into()); - } + version.verify_is_supported(&ctx_a.get_compatible_versions())?; } Ok(()) @@ -32,16 +29,12 @@ pub(crate) fn execute(ctx_a: &mut Ctx, msg: MsgConnectionOpenInit) -> Resul where Ctx: ExecutionContext, { - let versions = match msg.version { - Some(version) => { - if ctx_a.get_compatible_versions().contains(&version) { - Ok(vec![version]) - } else { - Err(ConnectionError::VersionNotSupported { version }) - } - } - None => Ok(ctx_a.get_compatible_versions()), - }?; + let versions = if let Some(version) = msg.version { + version.verify_is_supported(&ctx_a.get_compatible_versions())?; + vec![version] + } else { + ctx_a.get_compatible_versions() + }; let conn_end_on_a = ConnectionEnd::new( State::Init, @@ -53,7 +46,8 @@ where ), versions, msg.delay_period, - ); + ) + .unwrap(); // Construct the identifier for the new connection. let conn_id_on_a = ConnectionId::new(ctx_a.connection_counter()?); diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs index e35e1d194..ad4e4a74f 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs @@ -81,7 +81,7 @@ where Counterparty::new(msg.client_id_on_b.clone(), None, prefix_on_b), msg.versions_on_a.clone(), msg.delay_period, - ); + )?; client_state_of_a_on_b .verify_membership( @@ -188,8 +188,7 @@ impl LocalVars { where Ctx: ValidationContext, { - let version_on_b = - ctx_b.pick_version(&ctx_b.get_compatible_versions(), &msg.versions_on_a)?; + let version_on_b = ctx_b.pick_version(&msg.versions_on_a)?; Ok(Self { conn_id_on_b: ConnectionId::new(ctx_b.connection_counter()?), @@ -199,7 +198,8 @@ impl LocalVars { msg.counterparty.clone(), vec![version_on_b], msg.delay_period, - ), + ) + .unwrap(), client_id_on_a: msg.counterparty.client_id().clone(), conn_id_on_a: msg .counterparty diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 2602e8257..c0794d5cd 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -32,9 +32,30 @@ pub struct Version { } impl Version { - /// Checks whether or not the given feature is supported in this version - pub fn is_supported_feature(&self, feature: String) -> bool { - self.features.contains(&feature) + /// Checks whether the version has a matching version identifier and its + /// feature set is a subset of the supported features + pub fn verify_is_supported( + &self, + supported_versions: &[Version], + ) -> Result<(), ConnectionError> { + let maybe_supported_version = find_supported_version(self, supported_versions)?; + + if self.features.is_empty() { + return Err(ConnectionError::EmptyFeatures); + } + + for feature in self.features.iter() { + maybe_supported_version.verify_feature_supported(feature.to_string())?; + } + Ok(()) + } + + /// Checks whether the given feature is supported in this version + pub fn verify_feature_supported(&self, feature: String) -> Result<(), ConnectionError> { + if !self.features.contains(&feature) { + return Err(ConnectionError::FeatureNotSupported { feature }); + } + Ok(()) } } @@ -95,32 +116,75 @@ pub fn get_compatible_versions() -> Vec { vec![Version::default()] } -/// Selects a version from the intersection of locally supported and counterparty versions. +/// Iterates over the descending ordered set of compatible IBC versions and +/// selects the first version with a version identifier that is supported by the +/// counterparty. The returned version contains a feature set with the +/// intersection of the features supported by the source and counterparty +/// chains. If the feature set intersection is nil then the search for a +/// compatible version continues. This function is called in the `conn_open_try` +/// handshake procedure. +/// +/// NOTE: Empty feature set is not currently allowed for a chosen version. pub fn pick_version( supported_versions: &[Version], counterparty_versions: &[Version], ) -> Result { let mut intersection: Vec = Vec::new(); - for s in supported_versions.iter() { - for c in counterparty_versions.iter() { - if c.identifier != s.identifier { - continue; + for sv in supported_versions.iter() { + if let Ok(cv) = find_supported_version(sv, counterparty_versions) { + if let Ok(feature_set) = get_feature_set_intersection(&sv.features, &cv.features) { + intersection.push(Version { + identifier: cv.identifier, + features: feature_set, + }) } - for feature in c.features.iter() { - if feature.trim().is_empty() { - return Err(ConnectionError::EmptyFeatures); - } - } - intersection.append(&mut vec![s.clone()]); } } - intersection.sort_by(|a, b| a.identifier.cmp(&b.identifier)); + if intersection.is_empty() { return Err(ConnectionError::NoCommonVersion); } + + intersection.sort_by(|a, b| a.identifier.cmp(&b.identifier)); Ok(intersection[0].clone()) } +/// Returns the version from the list of supported versions that matches the +/// given reference version. +fn find_supported_version( + version: &Version, + supported_versions: &[Version], +) -> Result { + supported_versions + .iter() + .find(|sv| sv.identifier == version.identifier) + .ok_or(ConnectionError::VersionNotSupported { + version: version.clone(), + }) + .cloned() +} + +/// Returns the intersections of supported features by a host and the +/// counterparty features. This is done by iterating over all the features in +/// the host supported version and seeing if they exist in the feature set for +/// the counterparty version. +fn get_feature_set_intersection( + supported_features: &[String], + counterparty_features: &[String], +) -> Result, ConnectionError> { + let feature_set_intersection: Vec = supported_features + .iter() + .filter(|f| counterparty_features.contains(f)) + .cloned() + .collect(); + + if feature_set_intersection.is_empty() { + return Err(ConnectionError::NoCommonFeatures); + } + + Ok(feature_set_intersection) +} + #[cfg(test)] mod tests { use crate::prelude::*; @@ -132,12 +196,16 @@ mod tests { use crate::core::ics03_connection::error::ConnectionError; use crate::core::ics03_connection::version::{get_compatible_versions, pick_version, Version}; + fn get_dummy_features() -> Vec { + vec!["ORDER_RANDOM".to_string(), "ORDER_UNORDERED".to_string()] + } + fn good_versions() -> Vec { vec![ Version::default().into(), RawVersion { identifier: "2".to_string(), - features: vec!["ORDER_RANDOM".to_string(), "ORDER_UNORDERED".to_string()], + features: get_dummy_features(), }, ] .into_iter() @@ -147,7 +215,7 @@ mod tests { fn bad_versions_identifier() -> Vec { vec![RawVersion { identifier: "".to_string(), - features: vec!["ORDER_RANDOM".to_string(), "ORDER_UNORDERED".to_string()], + features: get_dummy_features(), }] .into_iter() .collect() @@ -168,11 +236,11 @@ mod tests { Version::default(), Version { identifier: "3".to_string(), - features: Vec::new(), + features: get_dummy_features(), }, Version { identifier: "4".to_string(), - features: Vec::new(), + features: get_dummy_features(), }, ] .into_iter() @@ -180,15 +248,15 @@ mod tests { vec![ Version { identifier: "2".to_string(), - features: Vec::new(), + features: get_dummy_features(), }, Version { identifier: "4".to_string(), - features: Vec::new(), + features: get_dummy_features(), }, Version { identifier: "3".to_string(), - features: Vec::new(), + features: get_dummy_features(), }, ] .into_iter() @@ -196,7 +264,7 @@ mod tests { // Should pick version 3 as it's the lowest of the intersection {3, 4} Version { identifier: "3".to_string(), - features: Vec::new(), + features: get_dummy_features(), }, ) } diff --git a/crates/ibc/src/core/ics04_channel/error.rs b/crates/ibc/src/core/ics04_channel/error.rs index cf60eee44..6813c1976 100644 --- a/crates/ibc/src/core/ics04_channel/error.rs +++ b/crates/ibc/src/core/ics04_channel/error.rs @@ -3,6 +3,7 @@ use super::timeout::TimeoutHeight; use crate::core::ics02_client::error as client_error; use crate::core::ics03_connection::error as connection_error; use crate::core::ics04_channel::channel::State; +use crate::core::ics04_channel::Version; use crate::core::ics05_port::error as port_error; use crate::core::ics24_host::error::ValidationError; use crate::core::ics24_host::identifier::{ChannelId, ClientId, ConnectionId, PortId}; @@ -15,8 +16,6 @@ use displaydoc::Display; #[derive(Debug, Display)] pub enum ChannelError { - /// connection error: `{0}` - Connection(connection_error::ConnectionError), /// port error: `{0}` Port(port_error::PortError), /// channel state unknown: `{state}` @@ -35,14 +34,13 @@ pub enum ChannelError { NonUtf8PacketData, /// missing counterparty MissingCounterparty, - /// no common version - NoCommonVersion, + /// expected version `{expected_version}` , got `{got_version}` + VersionNotSupported { + expected_version: Version, + got_version: Version, + }, /// missing channel end MissingChannel, - /// single version must be negotiated on connection before opening channel - InvalidVersionLengthConnection, - /// the channel ordering is not supported by connection - ChannelFeatureNotSupportedByConnection, /// the channel end (`{port_id}`, `{channel_id}`) does not exist ChannelNotFound { port_id: PortId, @@ -201,7 +199,6 @@ impl std::error::Error for PacketError { impl std::error::Error for ChannelError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match &self { - Self::Connection(e) => Some(e), Self::Port(e) => Some(e), Self::Identifier(e) => Some(e), Self::Signer(e) => Some(e), diff --git a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs b/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs index 538cff302..f632e418f 100644 --- a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs +++ b/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs @@ -196,7 +196,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_close_confirm.rs b/crates/ibc/src/core/ics04_channel/handler/chan_close_confirm.rs index cd2701133..6c1db93a2 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_close_confirm.rs +++ b/crates/ibc/src/core/ics04_channel/handler/chan_close_confirm.rs @@ -128,7 +128,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg_chan_close_confirm = MsgChannelCloseConfirm::try_from( get_dummy_raw_msg_chan_close_confirm(client_consensus_state_height.revision_height()), diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_close_init.rs b/crates/ibc/src/core/ics04_channel/handler/chan_close_init.rs index a4ec1cd27..2302cb26a 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_close_init.rs +++ b/crates/ibc/src/core/ics04_channel/handler/chan_close_init.rs @@ -83,7 +83,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg_chan_close_init = MsgChannelCloseInit::try_from(get_dummy_raw_msg_chan_close_init()).unwrap(); 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 2a286537c..820ff208e 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 @@ -136,7 +136,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg = MsgChannelOpenAck::try_from(get_dummy_raw_msg_chan_open_ack(proof_height)).unwrap(); diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_open_confirm.rs b/crates/ibc/src/core/ics04_channel/handler/chan_open_confirm.rs index ac0b037e1..93548ae1b 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_open_confirm.rs +++ b/crates/ibc/src/core/ics04_channel/handler/chan_open_confirm.rs @@ -138,7 +138,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let msg = MsgChannelOpenConfirm::try_from(get_dummy_raw_msg_chan_open_confirm(proof_height)) 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 6c0be1394..b3e3a81df 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 @@ -25,15 +25,9 @@ where let client_state_of_b_on_a = ctx_a.client_state(client_id_on_a)?; client_state_of_b_on_a.confirm_not_frozen()?; - let conn_version = match conn_end_on_a.versions() { - [version] => version, - _ => return Err(ChannelError::InvalidVersionLengthConnection.into()), - }; - - let channel_feature = msg.ordering.to_string(); - if !conn_version.is_supported_feature(channel_feature) { - return Err(ChannelError::ChannelFeatureNotSupportedByConnection.into()); - } + let conn_version = conn_end_on_a.versions(); + + conn_version[0].verify_feature_supported(msg.ordering.to_string())?; Ok(()) } @@ -76,7 +70,8 @@ mod tests { msg_conn_init.counterparty.clone(), get_compatible_versions(), msg_conn_init.delay_period, - ); + ) + .unwrap(); let client_id_on_a = ClientId::new(tm_client_type(), 0).unwrap(); let client_height = Height::new(0, 10).unwrap(); 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 35aa921e6..5a00c5142 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 @@ -30,19 +30,9 @@ where .map_err(ContextError::ChannelError); } - let conn_version = match conn_end_on_b.versions() { - [version] => version, - _ => { - return Err(ChannelError::InvalidVersionLengthConnection) - .map_err(ContextError::ChannelError) - } - }; + let conn_version = conn_end_on_b.versions(); - let channel_feature = msg.ordering.to_string(); - if !conn_version.is_supported_feature(channel_feature) { - return Err(ChannelError::ChannelFeatureNotSupportedByConnection) - .map_err(ContextError::ChannelError); - } + conn_version[0].verify_feature_supported(msg.ordering.to_string())?; // Verify proofs { @@ -132,7 +122,8 @@ mod tests { ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); // We're going to test message processing against this message. // Note: we make the counterparty's channel_id `None`. diff --git a/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs b/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs index d5c56ad99..e0dc59e70 100644 --- a/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs +++ b/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs @@ -229,7 +229,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/ics04_channel/handler/send_packet.rs b/crates/ibc/src/core/ics04_channel/handler/send_packet.rs index f01e42e70..6545512fc 100644 --- a/crates/ibc/src/core/ics04_channel/handler/send_packet.rs +++ b/crates/ibc/src/core/ics04_channel/handler/send_packet.rs @@ -187,7 +187,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); let timestamp_future = Timestamp::now().add(Duration::from_secs(10)).unwrap(); let timestamp_ns_past = 1; diff --git a/crates/ibc/src/core/ics04_channel/handler/timeout.rs b/crates/ibc/src/core/ics04_channel/handler/timeout.rs index 7b23d75d8..6b5bf2a5a 100644 --- a/crates/ibc/src/core/ics04_channel/handler/timeout.rs +++ b/crates/ibc/src/core/ics04_channel/handler/timeout.rs @@ -226,7 +226,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs b/crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs index b85d86660..fe8f66525 100644 --- a/crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs +++ b/crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs @@ -241,7 +241,8 @@ mod tests { ), get_compatible_versions(), ZERO_DURATION, - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/ics04_channel/version.rs b/crates/ibc/src/core/ics04_channel/version.rs index 7c228a5ff..026175359 100644 --- a/crates/ibc/src/core/ics04_channel/version.rs +++ b/crates/ibc/src/core/ics04_channel/version.rs @@ -8,6 +8,8 @@ use core::str::FromStr; use crate::prelude::*; +use super::error::ChannelError; + /// The version field for a `ChannelEnd`. /// /// This field is opaque to the core IBC protocol. @@ -45,6 +47,16 @@ impl Version { pub fn as_str(&self) -> &str { &self.0 } + + pub fn verify_is_expected(&self, expected_version: Version) -> Result<(), ChannelError> { + if self != &expected_version { + return Err(ChannelError::VersionNotSupported { + expected_version, + got_version: self.clone(), + }); + } + Ok(()) + } } impl From for Version {