From 7aa772dd8adca44cbdfe1292938c9e186414d647 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 14 Apr 2023 11:57:12 -0700 Subject: [PATCH 01/19] Streamline codebase around Version validation --- .../625-refactor-version-validation.md | 2 + crates/ibc/src/core/context.rs | 11 +++-- .../src/core/ics03_connection/connection.rs | 7 ++- crates/ibc/src/core/ics03_connection/error.rs | 2 + .../ics03_connection/handler/conn_open_ack.rs | 5 +- .../handler/conn_open_confirm.rs | 2 +- .../handler/conn_open_init.rs | 23 ++++----- .../ics03_connection/handler/conn_open_try.rs | 3 +- .../ibc/src/core/ics03_connection/version.rs | 48 +++++++++++++------ crates/ibc/src/core/ics04_channel/error.rs | 4 -- .../ics04_channel/handler/chan_open_init.rs | 12 ++--- .../ics04_channel/handler/chan_open_try.rs | 14 ++---- 12 files changed, 71 insertions(+), 62 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/625-refactor-version-validation.md 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..617e06df6 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md @@ -0,0 +1,2 @@ +- Streamline codebase around `Version` validation +([#625](https://github.com/cosmos/ibc-rs/issues/625)) \ No newline at end of file diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs index a9d4f1fe9..fea1ae421 100644 --- a/crates/ibc/src/core/context.rs +++ b/crates/ibc/src/core/context.rs @@ -22,6 +22,7 @@ use self::acknowledgement::{acknowledgement_packet_execute, acknowledgement_pack use self::recv_packet::{recv_packet_execute, recv_packet_validate}; use self::timeout::{timeout_packet_execute, timeout_packet_validate, TimeoutMsgType}; +use super::ics03_connection::version::pick_version; use super::{ ics02_client::error::ClientError, ics03_connection::error::ConnectionError, @@ -36,7 +37,7 @@ use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics03_connection::connection::ConnectionEnd; use crate::core::ics03_connection::version::{ - get_compatible_versions, pick_version, Version as ConnectionVersion, + get_compatible_versions, Version as ConnectionVersion, }; use crate::core::ics04_channel::channel::ChannelEnd; use crate::core::ics04_channel::commitment::{AcknowledgementCommitment, PacketCommitment}; @@ -313,11 +314,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) + pick_version( + self.get_compatible_versions().as_slice(), + counterparty_candidate_versions, + ) + .map_err(ContextError::ConnectionError) } /// Returns the ChannelEnd for the given `port_id` and `chan_id`. diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index 660dbe198..127feb36c 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -331,8 +331,11 @@ impl ConnectionEnd { } /// Getter for the list of versions in this connection end. - pub fn versions(&self) -> &[Version] { - &self.versions + pub fn versions(&self) -> Result, ConnectionError> { + if self.versions.is_empty() { + return Err(ConnectionError::EmptyVersions); + } + Ok(self.versions.clone()) } /// Getter for the counterparty. diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs index e70ae8de2..85f95ae7c 100644 --- a/crates/ibc/src/core/ics03_connection/error.rs +++ b/crates/ibc/src/core/ics03_connection/error.rs @@ -37,6 +37,8 @@ pub enum ConnectionError { NoCommonVersion, /// version \"`{version}`\" not supported VersionNotSupported { version: Version }, + /// feature \"`{feature}`\" not supported + FeatureNotSupported { feature: String }, /// 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 a3c392e35..874801077 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 @@ -46,7 +46,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 + .ensure_version_supported(vars.conn_end_on_a.versions()?) + .is_ok()) { return Err(ConnectionError::ConnectionMismatch { connection_id: msg.conn_id_on_a.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 5b1ac3774..3ca50ca49 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 @@ -72,7 +72,7 @@ where Some(msg.conn_id_on_b.clone()), prefix_on_b, ), - conn_end_on_b.versions().to_vec(), + conn_end_on_b.versions()?, conn_end_on_b.delay_period(), ); 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 d540bda67..2302384f3 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.ensure_version_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.ensure_version_supported(ctx_a.get_compatible_versions())?; + vec![version] + } else { + ctx_a.get_compatible_versions() + }; let conn_end_on_a = ConnectionEnd::new( State::Init, @@ -177,7 +170,7 @@ mod tests { ) .unwrap(); assert_eq!(conn_end.state().clone(), State::Init); - assert_eq!(conn_end.versions(), expected_version); + assert_eq!(conn_end.versions().unwrap(), expected_version); } } } 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 722805ca5..f55550542 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 @@ -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()?), diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 2602e8257..55a85ee33 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -32,9 +32,33 @@ pub struct Version { } impl Version { + /// Check whether or not the given version is compatible with supported versions + pub fn ensure_version_supported( + &self, + supported_versions: Vec, + ) -> Result<(), ConnectionError> { + let maybe_supported_version = supported_versions + .iter() + .find(|sv| sv.identifier == self.identifier) + .ok_or(ConnectionError::VersionNotSupported { + version: self.clone(), + })?; + + for feature in self.features.iter() { + maybe_supported_version.ensure_feature_supported(feature.to_string())?; + } + Ok(()) + } + /// 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) + pub fn ensure_feature_supported(&self, feature: String) -> Result<(), ConnectionError> { + if feature.trim().is_empty() { + return Err(ConnectionError::EmptyFeatures); + } + if !self.features.contains(&feature) { + return Err(ConnectionError::FeatureNotSupported { feature }); + } + Ok(()) } } @@ -101,23 +125,19 @@ pub fn pick_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 feature in c.features.iter() { - if feature.trim().is_empty() { - return Err(ConnectionError::EmptyFeatures); - } - } - intersection.append(&mut vec![s.clone()]); + for v in counterparty_versions.iter() { + if v.ensure_version_supported(supported_versions.to_vec()) + .is_ok() + { + intersection.append(&mut vec![v.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()) } diff --git a/crates/ibc/src/core/ics04_channel/error.rs b/crates/ibc/src/core/ics04_channel/error.rs index cf60eee44..4a7f7ddbc 100644 --- a/crates/ibc/src/core/ics04_channel/error.rs +++ b/crates/ibc/src/core/ics04_channel/error.rs @@ -39,10 +39,6 @@ pub enum ChannelError { NoCommonVersion, /// 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, 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..bf7abeb56 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,14 +25,10 @@ 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()?; + + for version in conn_version { + version.ensure_feature_supported(msg.ordering.to_string())?; } Ok(()) 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..6f271dea5 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,18 +30,10 @@ 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); + for version in conn_version { + version.ensure_feature_supported(msg.ordering.to_string())?; } // Verify proofs From cf3635cc2168c9a9c1cec136560689a4dafda014 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 14 Apr 2023 12:39:19 -0700 Subject: [PATCH 02/19] Fix versions() check --- crates/ibc/src/core/ics03_connection/connection.rs | 4 ++-- crates/ibc/src/core/ics03_connection/error.rs | 10 ++++++---- crates/ibc/src/core/ics03_connection/version.rs | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index 127feb36c..e0a419051 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -332,8 +332,8 @@ impl ConnectionEnd { /// Getter for the list of versions in this connection end. pub fn versions(&self) -> Result, ConnectionError> { - if self.versions.is_empty() { - return Err(ConnectionError::EmptyVersions); + if self.versions.len() != 1 { + return Err(ConnectionError::InvalidVersionLength); } Ok(self.versions.clone()) } diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs index 85f95ae7c..595faabb4 100644 --- a/crates/ibc/src/core/ics03_connection/error.rs +++ b/crates/ibc/src/core/ics03_connection/error.rs @@ -31,12 +31,14 @@ 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 }, /// missing proof height diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 55a85ee33..31f52bca2 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -37,7 +37,7 @@ impl Version { &self, supported_versions: Vec, ) -> Result<(), ConnectionError> { - let maybe_supported_version = supported_versions + let maybe_version_supported = supported_versions .iter() .find(|sv| sv.identifier == self.identifier) .ok_or(ConnectionError::VersionNotSupported { @@ -45,7 +45,7 @@ impl Version { })?; for feature in self.features.iter() { - maybe_supported_version.ensure_feature_supported(feature.to_string())?; + maybe_version_supported.ensure_feature_supported(feature.to_string())?; } Ok(()) } From 9acefa2d7fa1e5c0bac07a960ee8ba79c15814b6 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 14 Apr 2023 12:46:41 -0700 Subject: [PATCH 03/19] Add a docstring for versions() check --- crates/ibc/src/core/ics03_connection/connection.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index e0a419051..e5ab3016a 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -332,6 +332,7 @@ impl ConnectionEnd { /// Getter for the list of versions in this connection end. pub fn versions(&self) -> Result, ConnectionError> { + // Note: With the current implementation, only one version is supported per connection. if self.versions.len() != 1 { return Err(ConnectionError::InvalidVersionLength); } From a7a2ae906511f87ccac5dd1c1667f96822aac91d Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 14 Apr 2023 13:06:19 -0700 Subject: [PATCH 04/19] Simplify version check in conn_open_ack --- .../src/core/ics03_connection/handler/conn_open_ack.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 874801077..2647a815d 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,12 +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) - && msg - .version - .ensure_version_supported(vars.conn_end_on_a.versions()?) - .is_ok()) - { + msg.version + .ensure_version_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(), } From 1a75bd0a22e33272cf875129c7f2c5c926da8a55 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 14 Apr 2023 13:15:07 -0700 Subject: [PATCH 05/19] Remove unnecessary for --- crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs | 4 +--- crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) 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 bf7abeb56..004495425 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 @@ -27,9 +27,7 @@ where let conn_version = conn_end_on_a.versions()?; - for version in conn_version { - version.ensure_feature_supported(msg.ordering.to_string())?; - } + conn_version[0].ensure_feature_supported(msg.ordering.to_string())?; Ok(()) } 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 6f271dea5..21d968901 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 @@ -32,9 +32,7 @@ where let conn_version = conn_end_on_b.versions()?; - for version in conn_version { - version.ensure_feature_supported(msg.ordering.to_string())?; - } + conn_version[0].ensure_feature_supported(msg.ordering.to_string())?; // Verify proofs { From 2701f9971efdcad4a2dc1164b35659c68ca570ed Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 14 Apr 2023 17:49:02 -0700 Subject: [PATCH 06/19] Resolve a minor issue --- crates/ibc/src/core/context.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs index fea1ae421..569470892 100644 --- a/crates/ibc/src/core/context.rs +++ b/crates/ibc/src/core/context.rs @@ -22,7 +22,6 @@ use self::acknowledgement::{acknowledgement_packet_execute, acknowledgement_pack use self::recv_packet::{recv_packet_execute, recv_packet_validate}; use self::timeout::{timeout_packet_execute, timeout_packet_validate, TimeoutMsgType}; -use super::ics03_connection::version::pick_version; use super::{ ics02_client::error::ClientError, ics03_connection::error::ConnectionError, @@ -37,7 +36,7 @@ use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics03_connection::connection::ConnectionEnd; use crate::core::ics03_connection::version::{ - get_compatible_versions, Version as ConnectionVersion, + get_compatible_versions, pick_version, Version as ConnectionVersion, }; use crate::core::ics04_channel::channel::ChannelEnd; use crate::core::ics04_channel::commitment::{AcknowledgementCommitment, PacketCommitment}; @@ -317,7 +316,7 @@ pub trait ValidationContext: Router { counterparty_candidate_versions: &[ConnectionVersion], ) -> Result { pick_version( - self.get_compatible_versions().as_slice(), + &self.get_compatible_versions(), counterparty_candidate_versions, ) .map_err(ContextError::ConnectionError) From 351efaeeda16621d2321832ecf953a00259067fe Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 14 Apr 2023 18:44:42 -0700 Subject: [PATCH 07/19] Further pruning --- .../ibc/src/applications/transfer/context.rs | 29 +++++++------------ crates/ibc/src/applications/transfer/error.rs | 17 ++++------- crates/ibc/src/core/context.rs | 6 ++-- .../ics03_connection/handler/conn_open_ack.rs | 2 +- .../handler/conn_open_init.rs | 4 +-- .../ibc/src/core/ics03_connection/version.rs | 24 +++++++-------- crates/ibc/src/core/ics04_channel/error.rs | 11 +++---- .../ics04_channel/handler/chan_open_init.rs | 2 +- .../ics04_channel/handler/chan_open_try.rs | 2 +- crates/ibc/src/core/ics04_channel/version.rs | 12 ++++++++ 10 files changed, 55 insertions(+), 54 deletions(-) diff --git a/crates/ibc/src/applications/transfer/context.rs b/crates/ibc/src/applications/transfer/context.rs index d58efff41..fa96fb859 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,12 +135,9 @@ 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(), - }); - } + version + .verify_version_supported(Version::new(VERSION.to_string())) + .map_err(ContextError::from)?; Ok(()) } @@ -173,12 +171,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_version_supported(Version::new(VERSION.to_string())) + .map_err(ContextError::from)?; Ok(()) } @@ -202,12 +198,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_version_supported(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 569470892..36dd0a27d 100644 --- a/crates/ibc/src/core/context.rs +++ b/crates/ibc/src/core/context.rs @@ -315,11 +315,11 @@ pub trait ValidationContext: Router { &self, counterparty_candidate_versions: &[ConnectionVersion], ) -> Result { - pick_version( + let version = pick_version( &self.get_compatible_versions(), counterparty_candidate_versions, - ) - .map_err(ContextError::ConnectionError) + )?; + Ok(version) } /// Returns the ChannelEnd for the given `port_id` and `chan_id`. 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 2647a815d..f3bf339bf 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 @@ -46,7 +46,7 @@ where ctx_a.validate_self_client(msg.client_state_of_a_on_b.clone())?; msg.version - .ensure_version_supported(vars.conn_end_on_a.versions()?)?; + .verify_version_supported(vars.conn_end_on_a.versions()?)?; if !vars.conn_end_on_a.state_matches(&State::Init) { return Err(ConnectionError::ConnectionMismatch { 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 2302384f3..7fbe97c47 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 @@ -19,7 +19,7 @@ where client_state_of_b_on_a.confirm_not_frozen()?; if let Some(version) = msg.version { - version.ensure_version_supported(ctx_a.get_compatible_versions())?; + version.verify_version_supported(ctx_a.get_compatible_versions())?; } Ok(()) @@ -30,7 +30,7 @@ where Ctx: ExecutionContext, { let versions = if let Some(version) = msg.version { - version.ensure_version_supported(ctx_a.get_compatible_versions())?; + version.verify_version_supported(ctx_a.get_compatible_versions())?; vec![version] } else { ctx_a.get_compatible_versions() diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 31f52bca2..5417310d3 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -33,7 +33,7 @@ pub struct Version { impl Version { /// Check whether or not the given version is compatible with supported versions - pub fn ensure_version_supported( + pub fn verify_version_supported( &self, supported_versions: Vec, ) -> Result<(), ConnectionError> { @@ -45,13 +45,13 @@ impl Version { })?; for feature in self.features.iter() { - maybe_version_supported.ensure_feature_supported(feature.to_string())?; + maybe_version_supported.verify_feature_supported(feature.to_string())?; } Ok(()) } /// Checks whether or not the given feature is supported in this version - pub fn ensure_feature_supported(&self, feature: String) -> Result<(), ConnectionError> { + pub fn verify_feature_supported(&self, feature: String) -> Result<(), ConnectionError> { if feature.trim().is_empty() { return Err(ConnectionError::EmptyFeatures); } @@ -119,19 +119,19 @@ pub fn get_compatible_versions() -> Vec { vec![Version::default()] } -/// Selects a version from the intersection of locally supported and counterparty versions. +/// Selects only one version from the intersection of locally supported and counterparty versions. pub fn pick_version( supported_versions: &[Version], counterparty_versions: &[Version], ) -> Result { - let mut intersection: Vec = Vec::new(); - for v in counterparty_versions.iter() { - if v.ensure_version_supported(supported_versions.to_vec()) - .is_ok() - { - intersection.append(&mut vec![v.clone()]); - } - } + let mut intersection: Vec = counterparty_versions + .iter() + .filter(|v| { + v.verify_version_supported(supported_versions.to_vec()) + .is_ok() + }) + .cloned() + .collect(); if intersection.is_empty() { return Err(ConnectionError::NoCommonVersion); diff --git a/crates/ibc/src/core/ics04_channel/error.rs b/crates/ibc/src/core/ics04_channel/error.rs index 4a7f7ddbc..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,8 +34,11 @@ 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, /// the channel end (`{port_id}`, `{channel_id}`) does not exist @@ -197,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/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs index 004495425..aa1cf1294 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 @@ -27,7 +27,7 @@ where let conn_version = conn_end_on_a.versions()?; - conn_version[0].ensure_feature_supported(msg.ordering.to_string())?; + conn_version[0].verify_feature_supported(msg.ordering.to_string())?; Ok(()) } 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 21d968901..d979b5706 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 @@ -32,7 +32,7 @@ where let conn_version = conn_end_on_b.versions()?; - conn_version[0].ensure_feature_supported(msg.ordering.to_string())?; + conn_version[0].verify_feature_supported(msg.ordering.to_string())?; // Verify proofs { diff --git a/crates/ibc/src/core/ics04_channel/version.rs b/crates/ibc/src/core/ics04_channel/version.rs index 7c228a5ff..797ac00ca 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_version_supported(&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 { From 789646f2b8a1908a847f5fb0f48e5f86e53d44e4 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 21 Apr 2023 15:15:43 -0400 Subject: [PATCH 08/19] favor slices to `Vec` --- crates/ibc/src/core/ics03_connection/connection.rs | 4 ++-- .../src/core/ics03_connection/handler/conn_open_confirm.rs | 2 +- .../src/core/ics03_connection/handler/conn_open_init.rs | 4 ++-- crates/ibc/src/core/ics03_connection/version.rs | 7 ++----- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index e5ab3016a..4bc9fbd07 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -331,12 +331,12 @@ impl ConnectionEnd { } /// Getter for the list of versions in this connection end. - pub fn versions(&self) -> Result, ConnectionError> { + pub fn versions(&self) -> Result<&[Version], ConnectionError> { // Note: With the current implementation, only one version is supported per connection. if self.versions.len() != 1 { return Err(ConnectionError::InvalidVersionLength); } - Ok(self.versions.clone()) + Ok(&self.versions) } /// Getter for the counterparty. 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 fd8ebf64e..aa24aea3d 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 @@ -72,7 +72,7 @@ where Some(msg.conn_id_on_b.clone()), prefix_on_b, ), - conn_end_on_b.versions()?, + conn_end_on_b.versions()?.to_vec(), conn_end_on_b.delay_period(), ); 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 e3bccf507..1749afed0 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 @@ -19,7 +19,7 @@ where client_state_of_b_on_a.confirm_not_frozen()?; if let Some(version) = msg.version { - version.verify_version_supported(ctx_a.get_compatible_versions())?; + version.verify_version_supported(&ctx_a.get_compatible_versions())?; } Ok(()) @@ -30,7 +30,7 @@ where Ctx: ExecutionContext, { let versions = if let Some(version) = msg.version { - version.verify_version_supported(ctx_a.get_compatible_versions())?; + version.verify_version_supported(&ctx_a.get_compatible_versions())?; vec![version] } else { ctx_a.get_compatible_versions() diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 5417310d3..d0bb961f2 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -35,7 +35,7 @@ impl Version { /// Check whether or not the given version is compatible with supported versions pub fn verify_version_supported( &self, - supported_versions: Vec, + supported_versions: &[Version], ) -> Result<(), ConnectionError> { let maybe_version_supported = supported_versions .iter() @@ -126,10 +126,7 @@ pub fn pick_version( ) -> Result { let mut intersection: Vec = counterparty_versions .iter() - .filter(|v| { - v.verify_version_supported(supported_versions.to_vec()) - .is_ok() - }) + .filter(|v| v.verify_version_supported(supported_versions).is_ok()) .cloned() .collect(); From fdab426831e6f160d5646ad19c1349ed26b4021b Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 21 Apr 2023 13:48:03 -0700 Subject: [PATCH 09/19] Mend feature checks --- crates/ibc/src/core/ics03_connection/version.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index d0bb961f2..5dad48298 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -44,6 +44,10 @@ impl Version { version: self.clone(), })?; + if self.features.len() == 0 { + return Err(ConnectionError::EmptyFeatures); + } + for feature in self.features.iter() { maybe_version_supported.verify_feature_supported(feature.to_string())?; } @@ -52,9 +56,6 @@ impl Version { /// Checks whether or not the given feature is supported in this version pub fn verify_feature_supported(&self, feature: String) -> Result<(), ConnectionError> { - if feature.trim().is_empty() { - return Err(ConnectionError::EmptyFeatures); - } if !self.features.contains(&feature) { return Err(ConnectionError::FeatureNotSupported { feature }); } From ebce818759f779652969b27284084e1f51a15278 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 21 Apr 2023 13:55:07 -0700 Subject: [PATCH 10/19] Fix clippy --- crates/ibc/src/core/ics03_connection/version.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 5dad48298..161fc2448 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -44,7 +44,7 @@ impl Version { version: self.clone(), })?; - if self.features.len() == 0 { + if self.features.is_empty() { return Err(ConnectionError::EmptyFeatures); } From 46b933883fa15efdec087561c29c014c7ef37b2d Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 21 Apr 2023 13:58:28 -0700 Subject: [PATCH 11/19] Allow empty version for on_chan_open_init_validate --- crates/ibc/src/applications/transfer/context.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/ibc/src/applications/transfer/context.rs b/crates/ibc/src/applications/transfer/context.rs index 11a59c2a8..df3e34dc6 100644 --- a/crates/ibc/src/applications/transfer/context.rs +++ b/crates/ibc/src/applications/transfer/context.rs @@ -135,9 +135,11 @@ pub fn on_chan_open_init_validate( }); } - version - .verify_version_supported(Version::new(VERSION.to_string())) - .map_err(ContextError::from)?; + if !version.is_empty() { + version + .verify_version_supported(Version::new(VERSION.to_string())) + .map_err(ContextError::from)?; + } Ok(()) } From 20ce42704b96c17aa4da8a9e79fdb23cd70962a0 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 21 Apr 2023 14:07:39 -0700 Subject: [PATCH 12/19] Fix pick() test --- .../ibc/src/core/ics03_connection/version.rs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 161fc2448..578e79595 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -150,12 +150,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() @@ -165,7 +169,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() @@ -186,11 +190,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() @@ -198,15 +202,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() @@ -214,7 +218,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(), }, ) } From 3d53f13a3a73dd23798ba317594f73664564991c Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 21 Apr 2023 22:00:08 -0700 Subject: [PATCH 13/19] Modify pick_version --- crates/ibc/src/core/ics03_connection/error.rs | 2 + .../ibc/src/core/ics03_connection/version.rs | 76 +++++++++++++++---- 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs index 595faabb4..32e88443f 100644 --- a/crates/ibc/src/core/ics03_connection/error.rs +++ b/crates/ibc/src/core/ics03_connection/error.rs @@ -41,6 +41,8 @@ pub enum ConnectionError { 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/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 578e79595..417f39d74 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -32,29 +32,25 @@ pub struct Version { } impl Version { - /// Check whether or not the given version is compatible with supported versions + /// Checks whether the version has a matching version identifier and its + /// feature set is a subset of the supported features pub fn verify_version_supported( &self, supported_versions: &[Version], ) -> Result<(), ConnectionError> { - let maybe_version_supported = supported_versions - .iter() - .find(|sv| sv.identifier == self.identifier) - .ok_or(ConnectionError::VersionNotSupported { - version: self.clone(), - })?; + 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_version_supported.verify_feature_supported(feature.to_string())?; + maybe_supported_version.verify_feature_supported(feature.to_string())?; } Ok(()) } - /// Checks whether or not the given feature is supported in this version + /// 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 }); @@ -120,16 +116,30 @@ pub fn get_compatible_versions() -> Vec { vec![Version::default()] } -/// Selects only one 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 = counterparty_versions - .iter() - .filter(|v| v.verify_version_supported(supported_versions).is_ok()) - .cloned() - .collect(); + let mut intersection: Vec = Vec::new(); + 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, + }) + } + } + } if intersection.is_empty() { return Err(ConnectionError::NoCommonVersion); @@ -139,6 +149,42 @@ pub fn pick_version( 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::*; From 8240cc1a8a51d4f342dafa529f4d5028355537e2 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 21 Apr 2023 22:01:34 -0700 Subject: [PATCH 14/19] Edit changelog description --- .../breaking-changes/625-refactor-version-validation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md b/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md index 617e06df6..73561e662 100644 --- a/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md +++ b/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md @@ -1,2 +1,2 @@ -- Streamline codebase around `Version` validation +- 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 From aa91e71137ef8bfc15e584f84a7e160d1016daee Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Mon, 24 Apr 2023 09:56:12 -0400 Subject: [PATCH 15/19] name changes --- crates/ibc/src/applications/transfer/context.rs | 6 +++--- .../ibc/src/core/ics03_connection/handler/conn_open_ack.rs | 2 +- .../ibc/src/core/ics03_connection/handler/conn_open_init.rs | 4 ++-- crates/ibc/src/core/ics03_connection/version.rs | 2 +- crates/ibc/src/core/ics04_channel/version.rs | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/ibc/src/applications/transfer/context.rs b/crates/ibc/src/applications/transfer/context.rs index df3e34dc6..d5f1e2534 100644 --- a/crates/ibc/src/applications/transfer/context.rs +++ b/crates/ibc/src/applications/transfer/context.rs @@ -137,7 +137,7 @@ pub fn on_chan_open_init_validate( if !version.is_empty() { version - .verify_version_supported(Version::new(VERSION.to_string())) + .verify_is_expected(Version::new(VERSION.to_string())) .map_err(ContextError::from)?; } @@ -175,7 +175,7 @@ pub fn on_chan_open_try_validate( } counterparty_version - .verify_version_supported(Version::new(VERSION.to_string())) + .verify_is_expected(Version::new(VERSION.to_string())) .map_err(ContextError::from)?; Ok(()) @@ -201,7 +201,7 @@ pub fn on_chan_open_ack_validate( counterparty_version: &Version, ) -> Result<(), TokenTransferError> { counterparty_version - .verify_version_supported(Version::new(VERSION.to_string())) + .verify_is_expected(Version::new(VERSION.to_string())) .map_err(ContextError::from)?; Ok(()) 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 413c58919..ba0569b19 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 @@ -46,7 +46,7 @@ where ctx_a.validate_self_client(msg.client_state_of_a_on_b.clone())?; msg.version - .verify_version_supported(vars.conn_end_on_a.versions()?)?; + .verify_is_supported(vars.conn_end_on_a.versions()?)?; if !vars.conn_end_on_a.state_matches(&State::Init) { return Err(ConnectionError::ConnectionMismatch { 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 1749afed0..bb789349a 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 @@ -19,7 +19,7 @@ where client_state_of_b_on_a.confirm_not_frozen()?; if let Some(version) = msg.version { - version.verify_version_supported(&ctx_a.get_compatible_versions())?; + version.verify_is_supported(&ctx_a.get_compatible_versions())?; } Ok(()) @@ -30,7 +30,7 @@ where Ctx: ExecutionContext, { let versions = if let Some(version) = msg.version { - version.verify_version_supported(&ctx_a.get_compatible_versions())?; + version.verify_is_supported(&ctx_a.get_compatible_versions())?; vec![version] } else { ctx_a.get_compatible_versions() diff --git a/crates/ibc/src/core/ics03_connection/version.rs b/crates/ibc/src/core/ics03_connection/version.rs index 417f39d74..c0794d5cd 100644 --- a/crates/ibc/src/core/ics03_connection/version.rs +++ b/crates/ibc/src/core/ics03_connection/version.rs @@ -34,7 +34,7 @@ pub struct Version { impl Version { /// Checks whether the version has a matching version identifier and its /// feature set is a subset of the supported features - pub fn verify_version_supported( + pub fn verify_is_supported( &self, supported_versions: &[Version], ) -> Result<(), ConnectionError> { diff --git a/crates/ibc/src/core/ics04_channel/version.rs b/crates/ibc/src/core/ics04_channel/version.rs index 797ac00ca..026175359 100644 --- a/crates/ibc/src/core/ics04_channel/version.rs +++ b/crates/ibc/src/core/ics04_channel/version.rs @@ -48,7 +48,7 @@ impl Version { &self.0 } - pub fn verify_version_supported(&self, expected_version: Version) -> Result<(), ChannelError> { + pub fn verify_is_expected(&self, expected_version: Version) -> Result<(), ChannelError> { if self != &expected_version { return Err(ChannelError::VersionNotSupported { expected_version, From 63c50a34179e091703194dc9f17c516a53136360 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 24 Apr 2023 09:00:30 -0700 Subject: [PATCH 16/19] Move version length check into the new ConnectionEnd constructor --- .../ibc/src/core/context/acknowledgement.rs | 3 +- .../src/core/context/chan_close_confirm.rs | 3 +- .../ibc/src/core/context/chan_close_init.rs | 3 +- crates/ibc/src/core/context/chan_open_ack.rs | 3 +- .../ibc/src/core/context/chan_open_confirm.rs | 3 +- crates/ibc/src/core/context/chan_open_init.rs | 3 +- crates/ibc/src/core/context/chan_open_try.rs | 3 +- crates/ibc/src/core/context/recv_packet.rs | 3 +- crates/ibc/src/core/context/timeout.rs | 3 +- crates/ibc/src/core/handler.rs | 3 +- .../src/core/ics03_connection/connection.rs | 28 ++++++++++++------- .../ics03_connection/handler/conn_open_ack.rs | 5 ++-- .../handler/conn_open_confirm.rs | 5 ++-- .../handler/conn_open_init.rs | 3 +- .../ics03_connection/handler/conn_open_try.rs | 5 ++-- .../ics04_channel/handler/acknowledgement.rs | 3 +- .../handler/chan_close_confirm.rs | 3 +- .../ics04_channel/handler/chan_close_init.rs | 3 +- .../ics04_channel/handler/chan_open_ack.rs | 3 +- .../handler/chan_open_confirm.rs | 3 +- .../ics04_channel/handler/chan_open_init.rs | 3 +- .../ics04_channel/handler/chan_open_try.rs | 3 +- .../core/ics04_channel/handler/recv_packet.rs | 3 +- .../core/ics04_channel/handler/send_packet.rs | 3 +- .../src/core/ics04_channel/handler/timeout.rs | 3 +- .../ics04_channel/handler/timeout_on_close.rs | 3 +- 26 files changed, 71 insertions(+), 38 deletions(-) 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 4bc9fbd07..40ddba4bd 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" may contains a set of compatible versions initially, + // provided by the relayer or default values, but during the (TRY, ACK, + // CONFIRM), it holds the single version chosen by the handshake protocol. + if !state.state_matches(&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. @@ -332,10 +339,6 @@ impl ConnectionEnd { /// Getter for the list of versions in this connection end. pub fn versions(&self) -> Result<&[Version], ConnectionError> { - // Note: With the current implementation, only one version is supported per connection. - if self.versions.len() != 1 { - return Err(ConnectionError::InvalidVersionLength); - } Ok(&self.versions) } @@ -506,9 +509,14 @@ impl State { } } + /// Returns true if this connection state matches the expected. + pub fn state_matches(&self, expected: &State) -> bool { + self == expected + } + /// Returns whether or not this connection state is `Open`. pub fn is_open(self) -> bool { - self == State::Open + self.state_matches(&State::Open) } /// Returns whether or not this connection with this state 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 ba0569b19..0162188b7 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 @@ -89,7 +89,7 @@ where ), vec![msg.version.clone()], vars.conn_end_on_a.delay_period(), - ); + )?; client_state_of_b_on_a .verify_membership( @@ -262,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 aa24aea3d..f4c33ade9 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 bb789349a..748ed875b 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 @@ -46,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 1fa69cb11..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( @@ -198,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/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 aa1cf1294..bc6b5bb52 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 @@ -70,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 d979b5706..9650dc14e 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 @@ -122,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, From 41d1d30592fcf0af1b38dc264d0c0eca1dcf0adf Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 24 Apr 2023 09:02:48 -0700 Subject: [PATCH 17/19] Revert Versions signature --- crates/ibc/src/core/ics03_connection/connection.rs | 4 ++-- crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs | 2 +- .../src/core/ics03_connection/handler/conn_open_confirm.rs | 2 +- .../ibc/src/core/ics03_connection/handler/conn_open_init.rs | 2 +- crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs | 2 +- crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index 40ddba4bd..bf09d7dea 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -338,8 +338,8 @@ impl ConnectionEnd { } /// Getter for the list of versions in this connection end. - pub fn versions(&self) -> Result<&[Version], ConnectionError> { - Ok(&self.versions) + pub fn versions(&self) -> &[Version] { + &self.versions } /// Getter for the counterparty. 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 0162188b7..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 @@ -46,7 +46,7 @@ where ctx_a.validate_self_client(msg.client_state_of_a_on_b.clone())?; msg.version - .verify_is_supported(vars.conn_end_on_a.versions()?)?; + .verify_is_supported(vars.conn_end_on_a.versions())?; if !vars.conn_end_on_a.state_matches(&State::Init) { return Err(ConnectionError::ConnectionMismatch { 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 f4c33ade9..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 @@ -72,7 +72,7 @@ where Some(msg.conn_id_on_b.clone()), prefix_on_b, ), - conn_end_on_b.versions()?.to_vec(), + conn_end_on_b.versions().to_vec(), conn_end_on_b.delay_period(), )?; 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 748ed875b..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 @@ -171,7 +171,7 @@ mod tests { ) .unwrap(); assert_eq!(conn_end.state().clone(), State::Init); - assert_eq!(conn_end.versions().unwrap(), expected_version); + assert_eq!(conn_end.versions(), expected_version); } } } 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 bc6b5bb52..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,7 +25,7 @@ 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 = conn_end_on_a.versions()?; + let conn_version = conn_end_on_a.versions(); conn_version[0].verify_feature_supported(msg.ordering.to_string())?; 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 9650dc14e..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,7 +30,7 @@ where .map_err(ContextError::ChannelError); } - let conn_version = conn_end_on_b.versions()?; + let conn_version = conn_end_on_b.versions(); conn_version[0].verify_feature_supported(msg.ordering.to_string())?; From cbab071201dff5b6eb5cd07b9da0e78d8109c648 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Mon, 24 Apr 2023 12:14:28 -0400 Subject: [PATCH 18/19] docstring --- crates/ibc/src/core/ics03_connection/connection.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index bf09d7dea..e676fccbb 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -271,9 +271,9 @@ impl ConnectionEnd { versions: Vec, delay_period: Duration, ) -> Result { - // Note: "versions" may contains a set of compatible versions initially, - // provided by the relayer or default values, but during the (TRY, ACK, - // CONFIRM), it holds the single version chosen by the handshake protocol. + // 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_matches(&State::Init) && versions.len() != 1 { return Err(ConnectionError::InvalidVersionLength); } From 3fa02e6808ff51ca162f91b6d9fca90df2a7ca0d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Mon, 24 Apr 2023 13:05:32 -0400 Subject: [PATCH 19/19] remove redundant function --- crates/ibc/src/core/ics03_connection/connection.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index e676fccbb..3b389d42c 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -274,7 +274,7 @@ impl ConnectionEnd { // 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_matches(&State::Init) && versions.len() != 1 { + if state != State::Init && versions.len() != 1 { return Err(ConnectionError::InvalidVersionLength); } @@ -509,14 +509,9 @@ impl State { } } - /// Returns true if this connection state matches the expected. - pub fn state_matches(&self, expected: &State) -> bool { - self == expected - } - /// Returns whether or not this connection state is `Open`. pub fn is_open(self) -> bool { - self.state_matches(&State::Open) + self == State::Open } /// Returns whether or not this connection with this state