From e1bee481e85d8bffab24ce9bd9b1bf67c71ceb15 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 17 Apr 2023 19:19:04 -0700 Subject: [PATCH 01/21] Add missing ClientType validation --- .../621-add-client-type-validation.md | 2 + .../clients/ics07_tendermint/client_state.rs | 3 +- .../ibc/src/clients/ics07_tendermint/mod.rs | 4 +- crates/ibc/src/core/context.rs | 3 +- .../ibc/src/core/ics02_client/client_state.rs | 3 +- .../ibc/src/core/ics02_client/client_type.rs | 36 -------- crates/ibc/src/core/ics02_client/error.rs | 2 +- crates/ibc/src/core/ics02_client/events.rs | 4 +- .../ics02_client/handler/update_client.rs | 3 +- crates/ibc/src/core/ics02_client/mod.rs | 1 - .../ibc/src/core/ics03_connection/events.rs | 5 +- crates/ibc/src/core/ics24_host/error.rs | 4 +- crates/ibc/src/core/ics24_host/identifier.rs | 92 ++++++++++++++++++- crates/ibc/src/mock/client_state.rs | 5 +- crates/ibc/src/mock/context.rs | 5 +- 15 files changed, 109 insertions(+), 63 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/621-add-client-type-validation.md delete mode 100644 crates/ibc/src/core/ics02_client/client_type.rs diff --git a/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md b/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md new file mode 100644 index 000000000..a313d18b3 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md @@ -0,0 +1,2 @@ +- Add missing `ClientType` validation and move `ClientType` under the ICS24 section + ([#621](https://github.com/cosmos/ibc-rs/issues/621)) \ No newline at end of file diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index 22ba8fb86..15ff8aa4f 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -27,7 +27,6 @@ use crate::clients::ics07_tendermint::error::Error; use crate::clients::ics07_tendermint::header::Header as TmHeader; use crate::clients::ics07_tendermint::misbehaviour::Misbehaviour as TmMisbehaviour; use crate::core::ics02_client::client_state::{ClientState as Ics2ClientState, UpdatedState}; -use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics02_client::error::ClientError; use crate::core::ics02_client::trust_threshold::TrustThreshold; @@ -36,7 +35,7 @@ use crate::core::ics23_commitment::commitment::{ }; use crate::core::ics23_commitment::merkle::{apply_prefix, MerkleProof}; use crate::core::ics23_commitment::specs::ProofSpecs; -use crate::core::ics24_host::identifier::{ChainId, ClientId}; +use crate::core::ics24_host::identifier::{ChainId, ClientId, ClientType}; use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath, ClientUpgradePath}; use crate::core::ics24_host::Path; use crate::timestamp::ZERO_DURATION; diff --git a/crates/ibc/src/clients/ics07_tendermint/mod.rs b/crates/ibc/src/clients/ics07_tendermint/mod.rs index 7dc66f3ec..9f85406c1 100644 --- a/crates/ibc/src/clients/ics07_tendermint/mod.rs +++ b/crates/ibc/src/clients/ics07_tendermint/mod.rs @@ -3,7 +3,7 @@ use alloc::string::ToString; -use crate::core::ics02_client::client_type::ClientType; +use crate::core::ics24_host::identifier::ClientType; pub mod client_state; pub mod consensus_state; @@ -14,5 +14,5 @@ pub mod misbehaviour; pub(crate) const TENDERMINT_CLIENT_TYPE: &str = "07-tendermint"; pub fn client_type() -> ClientType { - ClientType::new(TENDERMINT_CLIENT_TYPE.to_string()) + ClientType::new_unchecked(TENDERMINT_CLIENT_TYPE.to_string()) } diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs index a9d4f1fe9..8cf1d5cb2 100644 --- a/crates/ibc/src/core/context.rs +++ b/crates/ibc/src/core/context.rs @@ -32,7 +32,6 @@ use core::time::Duration; use ibc_proto::google::protobuf::Any; use crate::core::ics02_client::client_state::ClientState; -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::{ @@ -45,7 +44,7 @@ use crate::core::ics04_channel::msgs::{ChannelMsg, PacketMsg}; use crate::core::ics04_channel::packet::{Receipt, Sequence}; use crate::core::ics05_port::error::PortError::UnknownPort; use crate::core::ics23_commitment::commitment::CommitmentPrefix; -use crate::core::ics24_host::identifier::{ConnectionId, PortId}; +use crate::core::ics24_host::identifier::{ClientType, ConnectionId, PortId}; use crate::core::ics24_host::path::{ AckPath, ChannelEndPath, ClientConnectionPath, ClientConsensusStatePath, ClientStatePath, ClientTypePath, CommitmentPath, ConnectionPath, ReceiptPath, SeqAckPath, SeqRecvPath, diff --git a/crates/ibc/src/core/ics02_client/client_state.rs b/crates/ibc/src/core/ics02_client/client_state.rs index 04f689306..32b4caa41 100644 --- a/crates/ibc/src/core/ics02_client/client_state.rs +++ b/crates/ibc/src/core/ics02_client/client_state.rs @@ -6,12 +6,11 @@ use ibc_proto::google::protobuf::Any; use ibc_proto::ibc::core::commitment::v1::MerkleProof; use ibc_proto::protobuf::Protobuf as ErasedProtobuf; -use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::error::ClientError; use crate::core::ics23_commitment::commitment::{ CommitmentPrefix, CommitmentProofBytes, CommitmentRoot, }; -use crate::core::ics24_host::identifier::{ChainId, ClientId}; +use crate::core::ics24_host::identifier::{ChainId, ClientId, ClientType}; use crate::core::ics24_host::Path; use crate::dynamic_typing::AsAny; use crate::erased::ErasedSerialize; diff --git a/crates/ibc/src/core/ics02_client/client_type.rs b/crates/ibc/src/core/ics02_client/client_type.rs deleted file mode 100644 index 6ad746033..000000000 --- a/crates/ibc/src/core/ics02_client/client_type.rs +++ /dev/null @@ -1,36 +0,0 @@ -use crate::prelude::*; -use core::fmt::{Display, Error as FmtError, Formatter}; - -#[cfg_attr( - feature = "parity-scale-codec", - derive( - parity_scale_codec::Encode, - parity_scale_codec::Decode, - scale_info::TypeInfo - ) -)] -#[cfg_attr( - feature = "borsh", - derive(borsh::BorshSerialize, borsh::BorshDeserialize) -)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -/// Type of the client, depending on the specific consensus algorithm. -#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct ClientType(String); - -impl ClientType { - pub fn new(s: String) -> Self { - Self(s) - } - - /// Yields this identifier as a borrowed `&str` - pub fn as_str(&self) -> &str { - &self.0 - } -} - -impl Display for ClientType { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { - write!(f, "ClientType({})", self.0) - } -} diff --git a/crates/ibc/src/core/ics02_client/error.rs b/crates/ibc/src/core/ics02_client/error.rs index 95d45eaa7..ff1d5c114 100644 --- a/crates/ibc/src/core/ics02_client/error.rs +++ b/crates/ibc/src/core/ics02_client/error.rs @@ -4,10 +4,10 @@ use crate::prelude::*; use displaydoc::Display; use ibc_proto::protobuf::Error as TendermintProtoError; -use crate::core::ics02_client::client_type::ClientType; use crate::core::ics23_commitment::error::CommitmentError; use crate::core::ics24_host::error::ValidationError; use crate::core::ics24_host::identifier::ClientId; +use crate::core::ics24_host::identifier::ClientType; use crate::signer::SignerError; use crate::timestamp::Timestamp; use crate::Height; diff --git a/crates/ibc/src/core/ics02_client/events.rs b/crates/ibc/src/core/ics02_client/events.rs index a3237b609..00e807980 100644 --- a/crates/ibc/src/core/ics02_client/events.rs +++ b/crates/ibc/src/core/ics02_client/events.rs @@ -5,9 +5,9 @@ use ibc_proto::google::protobuf::Any; use subtle_encoding::hex; use tendermint::abci; -use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::height::Height; use crate::core::ics24_host::identifier::ClientId; +use crate::core::ics24_host::identifier::ClientType; use crate::events::IbcEventType; use crate::prelude::*; @@ -408,7 +408,7 @@ mod tests { expected_values: Vec<&'static str>, } - let client_type = ClientType::new("07-tendermint".to_string()); + let client_type = ClientType::new_unchecked("07-tendermint".to_string()); let client_id = ClientId::new(client_type.clone(), 0).unwrap(); let consensus_height = Height::new(0, 5).unwrap(); let consensus_heights = vec![Height::new(0, 5).unwrap(), Height::new(0, 7).unwrap()]; diff --git a/crates/ibc/src/core/ics02_client/handler/update_client.rs b/crates/ibc/src/core/ics02_client/handler/update_client.rs index e2514d064..64ae14348 100644 --- a/crates/ibc/src/core/ics02_client/handler/update_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/update_client.rs @@ -95,12 +95,11 @@ mod tests { use crate::clients::ics07_tendermint::header::Header as TmHeader; use crate::clients::ics07_tendermint::misbehaviour::Misbehaviour as TmMisbehaviour; use crate::core::ics02_client::client_state::ClientState; - use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics02_client::handler::update_client::{execute, validate}; use crate::core::ics02_client::msgs::update_client::{MsgUpdateClient, UpdateKind}; use crate::core::ics23_commitment::specs::ProofSpecs; - use crate::core::ics24_host::identifier::{ChainId, ClientId}; + use crate::core::ics24_host::identifier::{ChainId, ClientId, ClientType}; use crate::core::ValidationContext; use crate::events::{IbcEvent, IbcEventType}; use crate::mock::client_state::client_type as mock_client_type; diff --git a/crates/ibc/src/core/ics02_client/mod.rs b/crates/ibc/src/core/ics02_client/mod.rs index 33ed6674d..111839494 100644 --- a/crates/ibc/src/core/ics02_client/mod.rs +++ b/crates/ibc/src/core/ics02_client/mod.rs @@ -1,7 +1,6 @@ //! ICS 02: Client implementation for verifying remote IBC-enabled chains. pub mod client_state; -pub mod client_type; pub mod consensus_state; pub mod error; pub mod events; diff --git a/crates/ibc/src/core/ics03_connection/events.rs b/crates/ibc/src/core/ics03_connection/events.rs index d25558e43..26ff81d0b 100644 --- a/crates/ibc/src/core/ics03_connection/events.rs +++ b/crates/ibc/src/core/ics03_connection/events.rs @@ -284,8 +284,9 @@ impl From for abci::Event { #[cfg(test)] mod tests { + use super::*; - use crate::core::ics02_client::client_type::ClientType; + use crate::core::ics24_host::identifier::ClientType; use tendermint::abci::Event as AbciEvent; #[test] @@ -297,7 +298,7 @@ mod tests { expected_values: Vec<&'static str>, } - let client_type = ClientType::new("07-tendermint".to_string()); + let client_type = ClientType::new_unchecked("07-tendermint".to_string()); let conn_id_on_a = ConnectionId::default(); let client_id_on_a = ClientId::new(client_type.clone(), 0).unwrap(); let conn_id_on_b = ConnectionId::new(1); diff --git a/crates/ibc/src/core/ics24_host/error.rs b/crates/ibc/src/core/ics24_host/error.rs index ef78eb941..a04bf9182 100644 --- a/crates/ibc/src/core/ics24_host/error.rs +++ b/crates/ibc/src/core/ics24_host/error.rs @@ -15,10 +15,10 @@ pub enum ValidationError { }, /// identifier `{id}` must only contain alphanumeric characters or `.`, `_`, `+`, `-`, `#`, - `[`, `]`, `<`, `>` InvalidCharacter { id: String }, + /// identifier prefix `{prefix}` is invalid + InvalidPrefix { prefix: String }, /// identifier cannot be empty Empty, - /// Invalid channel id in counterparty - InvalidCounterpartyChannelId, } #[cfg(feature = "std")] diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 7edd011ce..68504ed70 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -6,7 +6,6 @@ use derive_more::Into; use super::validate::*; use crate::clients::ics07_tendermint::client_type as tm_client_type; -use crate::core::ics02_client::client_type::ClientType; use crate::core::ics24_host::error::ValidationError; use crate::prelude::*; @@ -177,6 +176,90 @@ impl From for ChainId { } } +use crate::core::ics24_host::validate::validate_client_identifier; + +#[cfg_attr( + feature = "parity-scale-codec", + derive( + parity_scale_codec::Encode, + parity_scale_codec::Decode, + scale_info::TypeInfo + ) +)] +#[cfg_attr( + feature = "borsh", + derive(borsh::BorshSerialize, borsh::BorshDeserialize) +)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +/// Type of the client, depending on the specific consensus algorithm. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct ClientType(String); + +impl ClientType { + /// Constructs a new `ClientType` from the given `String` without performing any validation. + pub fn new_unchecked(s: String) -> Self { + Self(s) + } + + /// Constructs a new `ClientType` from the given `String` if it ends with a valid client identifier. + pub fn new(s: String) -> Result { + let client_type = Self::new_unchecked(s); + client_type.validate()?; + Ok(client_type) + } + + /// Yields this identifier as a borrowed `&str` + pub fn as_str(&self) -> &str { + &self.0 + } + + /// Validates the client type. It cannot be blank or empty. It must be a valid + /// client identifier when used with '0' or the maximum uint64 as the sequence + /// ``` + /// # use ibc::core::ics24_host::identifier::ClientType; + /// let healthy_client_type = ClientType::new("07-tendermint".to_string()); + /// assert!(healthy_client_type.is_ok()); + /// + /// let faulty_client_type = ClientType::new("07-tendermint-".to_string()); + /// assert!(faulty_client_type.is_err()); + /// + /// let short_client_type = ClientType::new("<7Char".to_string()); + /// assert!(short_client_type.is_err()); + /// + /// let lengthy_client_type = ClientType::new("InvalidClientTypeWithLengthOfClientId>65Char".to_string()); + /// assert!(lengthy_client_type.is_err()); + /// ``` + pub fn validate(&self) -> Result<(), ValidationError> { + let client_type_str = self.as_str().trim(); + + // Check that the client type is not blank + if client_type_str.is_empty() { + return Err(ValidationError::Empty)?; + } + + // Check that the client type does not end with a dash + let re = safe_regex::regex!(br".*[^-]"); + if !re.is_match(client_type_str.as_bytes()) { + return Err(ValidationError::InvalidPrefix { + prefix: client_type_str.to_string(), + })?; + } + + // Check that the client type is a valid client identifier when used with `0` + validate_client_identifier(&format!("{client_type_str}-{}", u64::MIN))?; + + // Check that the client type is a valid client identifier when used with `u64::MAX` + validate_client_identifier(&format!("{client_type_str}-{}", u64::MAX))?; + Ok(()) + } +} + +impl Display for ClientType { + fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { + write!(f, "ClientType({})", self.0) + } +} + #[cfg_attr( feature = "parity-scale-codec", derive( @@ -200,13 +283,14 @@ impl ClientId { /// /// ``` /// # use ibc::core::ics24_host::identifier::ClientId; - /// # use ibc::core::ics02_client::client_type::ClientType; - /// let tm_client_id = ClientId::new(ClientType::new("07-tendermint".to_string()), 0); + /// # use ibc::core::ics24_host::identifier::ClientType; + /// let tm_client_id = ClientId::new(ClientType::new("07-tendermint".to_string()).unwrap(), 0); /// assert!(tm_client_id.is_ok()); /// tm_client_id.map(|id| { assert_eq!(&id, "07-tendermint-0") }); /// ``` pub fn new(client_type: ClientType, counter: u64) -> Result { - let prefix = client_type.as_str(); + client_type.validate()?; + let prefix = client_type.as_str().trim(); let id = format!("{prefix}-{counter}"); Self::from_str(id.as_str()) } diff --git a/crates/ibc/src/mock/client_state.rs b/crates/ibc/src/mock/client_state.rs index 63c0f19ce..ae5814df0 100644 --- a/crates/ibc/src/mock/client_state.rs +++ b/crates/ibc/src/mock/client_state.rs @@ -11,13 +11,12 @@ use ibc_proto::ibc::mock::ClientState as RawMockClientState; use ibc_proto::protobuf::Protobuf; use crate::core::ics02_client::client_state::{ClientState, UpdatedState}; -use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics02_client::error::ClientError; use crate::core::ics23_commitment::commitment::{ CommitmentPrefix, CommitmentProofBytes, CommitmentRoot, }; -use crate::core::ics24_host::identifier::{ChainId, ClientId}; +use crate::core::ics24_host::identifier::{ChainId, ClientId, ClientType}; use crate::core::ics24_host::Path; use crate::mock::client_state::client_type as mock_client_type; use crate::mock::consensus_state::MockConsensusState; @@ -33,7 +32,7 @@ pub const MOCK_CLIENT_STATE_TYPE_URL: &str = "/ibc.mock.ClientState"; pub const MOCK_CLIENT_TYPE: &str = "9999-mock"; pub fn client_type() -> ClientType { - ClientType::new(MOCK_CLIENT_TYPE.to_string()) + ClientType::new_unchecked(MOCK_CLIENT_TYPE.to_string()) } /// A mock of an IBC client record as it is stored in a mock context. diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index 97d5a5046..d2ad4410f 100644 --- a/crates/ibc/src/mock/context.rs +++ b/crates/ibc/src/mock/context.rs @@ -29,7 +29,6 @@ use crate::clients::ics07_tendermint::client_state::ClientState as TmClientState use crate::core::context::ContextError; use crate::core::context::Router; use crate::core::ics02_client::client_state::ClientState; -use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics02_client::error::ClientError; use crate::core::ics02_client::header::Header; @@ -40,7 +39,9 @@ use crate::core::ics04_channel::commitment::{AcknowledgementCommitment, PacketCo use crate::core::ics04_channel::error::{ChannelError, PacketError}; use crate::core::ics04_channel::packet::{Receipt, Sequence}; use crate::core::ics23_commitment::commitment::CommitmentPrefix; -use crate::core::ics24_host::identifier::{ChainId, ChannelId, ClientId, ConnectionId, PortId}; +use crate::core::ics24_host::identifier::{ + ChainId, ChannelId, ClientId, ClientType, ConnectionId, PortId, +}; use crate::core::ics26_routing::context::{Module, ModuleId}; use crate::core::ics26_routing::msgs::MsgEnvelope; use crate::core::{dispatch, ExecutionContext, ValidationContext}; From eef26339665e6723205d5188de18f95494d82324 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 19 Apr 2023 10:43:47 -0700 Subject: [PATCH 02/21] Move ClientType verification under validate.rs to cover all needed checks --- .../handler/conn_open_init.rs | 5 ++ .../ics03_connection/handler/conn_open_try.rs | 8 ++ crates/ibc/src/core/ics24_host/identifier.rs | 35 +++----- crates/ibc/src/core/ics24_host/validate.rs | 81 ++++++++++++++----- 4 files changed, 85 insertions(+), 44 deletions(-) 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..b6db5e1b8 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 @@ -2,6 +2,7 @@ use crate::prelude::*; use crate::core::context::ContextError; +use crate::core::ics02_client::error::ClientError; use crate::core::ics03_connection::connection::{ConnectionEnd, Counterparty, State}; use crate::core::ics03_connection::error::ConnectionError; use crate::core::ics03_connection::events::OpenInit; @@ -15,6 +16,10 @@ pub(crate) fn validate(ctx_a: &Ctx, msg: MsgConnectionOpenInit) -> Result<( where Ctx: ValidationContext, { + msg.client_id_on_a + .validate() + .map_err(ClientError::InvalidClientIdentifier)?; + // An IBC client running on the local (host) chain should exist. let client_state_of_b_on_a = ctx_a.client_state(&msg.client_id_on_a)?; client_state_of_b_on_a.confirm_not_frozen()?; 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..b0e5c66c3 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 @@ -35,6 +35,10 @@ fn validate_impl( where Ctx: ValidationContext, { + msg.client_id_on_b + .validate() + .map_err(ClientError::InvalidClientIdentifier)?; + ctx_b.validate_self_client(msg.client_state_of_b_on_a.clone())?; let host_height = ctx_b.host_height().map_err(|_| ConnectionError::Other { @@ -51,6 +55,10 @@ where let client_id_on_a = msg.counterparty.client_id(); + client_id_on_a + .validate() + .map_err(ClientError::InvalidClientIdentifier)?; + // Verify proofs { let client_state_of_a_on_b = diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 68504ed70..c55e14ecb 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -203,9 +203,9 @@ impl ClientType { /// Constructs a new `ClientType` from the given `String` if it ends with a valid client identifier. pub fn new(s: String) -> Result { - let client_type = Self::new_unchecked(s); - client_type.validate()?; - Ok(client_type) + let s_trim = s.trim(); + validate_client_type(s_trim)?; + Ok(Self(s_trim.to_string())) } /// Yields this identifier as a borrowed `&str` @@ -230,27 +230,7 @@ impl ClientType { /// assert!(lengthy_client_type.is_err()); /// ``` pub fn validate(&self) -> Result<(), ValidationError> { - let client_type_str = self.as_str().trim(); - - // Check that the client type is not blank - if client_type_str.is_empty() { - return Err(ValidationError::Empty)?; - } - - // Check that the client type does not end with a dash - let re = safe_regex::regex!(br".*[^-]"); - if !re.is_match(client_type_str.as_bytes()) { - return Err(ValidationError::InvalidPrefix { - prefix: client_type_str.to_string(), - })?; - } - - // Check that the client type is a valid client identifier when used with `0` - validate_client_identifier(&format!("{client_type_str}-{}", u64::MIN))?; - - // Check that the client type is a valid client identifier when used with `u64::MAX` - validate_client_identifier(&format!("{client_type_str}-{}", u64::MAX))?; - Ok(()) + validate_client_type(self.as_str()) } } @@ -289,7 +269,6 @@ impl ClientId { /// tm_client_id.map(|id| { assert_eq!(&id, "07-tendermint-0") }); /// ``` pub fn new(client_type: ClientType, counter: u64) -> Result { - client_type.validate()?; let prefix = client_type.as_str().trim(); let id = format!("{prefix}-{counter}"); Self::from_str(id.as_str()) @@ -304,6 +283,12 @@ impl ClientId { pub fn as_bytes(&self) -> &[u8] { self.0.as_bytes() } + + /// Validates that the client identifier string is formatted correctly and + /// only contains valid characters. + pub fn validate(&self) -> Result<(), ValidationError> { + validate_client_identifier(self.as_str()) + } } /// This implementation provides a `to_string` method. diff --git a/crates/ibc/src/core/ics24_host/validate.rs b/crates/ibc/src/core/ics24_host/validate.rs index bfa2d04ab..c04fbd1a8 100644 --- a/crates/ibc/src/core/ics24_host/validate.rs +++ b/crates/ibc/src/core/ics24_host/validate.rs @@ -8,9 +8,11 @@ const VALID_SPECIAL_CHARS: &str = "._+-#[]<>"; /// Default validator function for identifiers. /// -/// A valid identifier only contain lowercase alphabetic characters, and be of a given min and max -/// length. -pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error> { +/// A valid identifier only contain valid characters, and be of a given min and +/// max length as specified in the +/// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)] +/// spec. +pub fn validate_identifier_default(id: &str, min: usize, max: usize) -> Result<(), Error> { assert!(max >= min); // Check identifier is not empty @@ -48,43 +50,84 @@ pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error Ok(()) } +/// Checks if the client type is of valid format and can be parsed into the +/// client identifier. +pub fn validate_client_type(client_type: &str) -> Result<(), Error> { + // Check that the client type is not blank + if client_type.is_empty() { + return Err(Error::Empty)?; + } + + // Check that the client type does not end with a dash + let re = safe_regex::regex!(br".*[^-]"); + if !re.is_match(client_type.as_bytes()) { + return Err(Error::InvalidPrefix { + prefix: client_type.to_string(), + })?; + } + + // Check that the client type is a valid client identifier when used with `0` + validate_client_identifier(&format!("{client_type}-{}", u64::MIN))?; + + // Check that the client type is a valid client identifier when used with `u64::MAX` + validate_client_identifier(&format!("{client_type}-{}", u64::MAX))?; + Ok(()) +} + +/// Checks if the client identifier is of valid format and can be parsed into +/// the `ClientId` type. +pub fn validate_client_identifier_format(id: &str) -> Result<(), Error> { + let split_id: Vec<_> = id.split("-").collect(); + let last_index = split_id.len() - 1; + let client_type_str = &id[..last_index]; + + validate_client_type(client_type_str.trim())?; + + split_id[last_index] + .parse::() + .map_err(|_| Error::InvalidCharacter { id: id.into() })?; + + Ok(()) +} + /// Default validator function for Client identifiers. /// -/// A valid identifier must be between 9-64 characters and only contain lowercase -/// alphabetic characters, +/// A valid client identifier must be between 9-64 characters as specified in +/// the ICS-24 spec. pub fn validate_client_identifier(id: &str) -> Result<(), Error> { - validate_identifier(id, 9, 64) + validate_identifier_default(id, 9, 64)?; + validate_client_identifier_format(id) } /// Default validator function for Connection identifiers. /// -/// A valid Identifier must be between 10-64 characters and only contain lowercase -/// alphabetic characters, +/// A valid connection identifier must be between 10-64 characters as specified +/// in the ICS-24 spec. pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { - validate_identifier(id, 10, 64) + validate_identifier_default(id, 10, 64) } /// Default validator function for Port identifiers. /// -/// A valid Identifier must be between 2-128 characters and only contain lowercase -/// alphabetic characters, +/// A valid port identifier must be between 2-128 characters as specified in the +/// ICS-24 spec. pub fn validate_port_identifier(id: &str) -> Result<(), Error> { - validate_identifier(id, 2, 128) + validate_identifier_default(id, 2, 128) } /// Default validator function for Channel identifiers. /// -/// A valid identifier must be between 8-64 characters and only contain -/// alphabetic characters, +/// A valid channel identifier must be between 8-64 characters as specified in +/// the ICS-24 spec. pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { - validate_identifier(id, 8, 64) + validate_identifier_default(id, 8, 64) } #[cfg(test)] mod tests { use crate::core::ics24_host::validate::{ validate_channel_identifier, validate_client_identifier, validate_connection_identifier, - validate_identifier, validate_port_identifier, + validate_identifier_default, validate_port_identifier, }; use test_log::test; @@ -155,21 +198,21 @@ mod tests { #[test] fn parse_invalid_id_chars() { // invalid id chars - let id = validate_identifier("channel@01", 1, 10); + let id = validate_identifier_default("channel@01", 1, 10); assert!(id.is_err()) } #[test] fn parse_invalid_id_empty() { // invalid id empty - let id = validate_identifier("", 1, 10); + let id = validate_identifier_default("", 1, 10); assert!(id.is_err()) } #[test] fn parse_invalid_id_path_separator() { // invalid id with path separator - let id = validate_identifier("id/1", 1, 10); + let id = validate_identifier_default("id/1", 1, 10); assert!(id.is_err()) } } From 3244dbc95bc36aaebd0472a96824a06eb7c8ea9c Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 19 Apr 2023 11:06:32 -0700 Subject: [PATCH 03/21] Move ClientType tests into the validate.rs --- crates/ibc/src/core/ics24_host/identifier.rs | 17 +-------- crates/ibc/src/core/ics24_host/validate.rs | 37 ++++++++++++++++---- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index c55e14ecb..1cbfbcf00 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -213,22 +213,7 @@ impl ClientType { &self.0 } - /// Validates the client type. It cannot be blank or empty. It must be a valid - /// client identifier when used with '0' or the maximum uint64 as the sequence - /// ``` - /// # use ibc::core::ics24_host::identifier::ClientType; - /// let healthy_client_type = ClientType::new("07-tendermint".to_string()); - /// assert!(healthy_client_type.is_ok()); - /// - /// let faulty_client_type = ClientType::new("07-tendermint-".to_string()); - /// assert!(faulty_client_type.is_err()); - /// - /// let short_client_type = ClientType::new("<7Char".to_string()); - /// assert!(short_client_type.is_err()); - /// - /// let lengthy_client_type = ClientType::new("InvalidClientTypeWithLengthOfClientId>65Char".to_string()); - /// assert!(lengthy_client_type.is_err()); - /// ``` + /// Validates the client type to ensure it ends with a valid client identifier. pub fn validate(&self) -> Result<(), ValidationError> { validate_client_type(self.as_str()) } diff --git a/crates/ibc/src/core/ics24_host/validate.rs b/crates/ibc/src/core/ics24_host/validate.rs index c04fbd1a8..47c5d4c54 100644 --- a/crates/ibc/src/core/ics24_host/validate.rs +++ b/crates/ibc/src/core/ics24_host/validate.rs @@ -67,19 +67,20 @@ pub fn validate_client_type(client_type: &str) -> Result<(), Error> { } // Check that the client type is a valid client identifier when used with `0` - validate_client_identifier(&format!("{client_type}-{}", u64::MIN))?; + validate_identifier_default(&format!("{client_type}-{}", u64::MIN), 9, 64)?; // Check that the client type is a valid client identifier when used with `u64::MAX` - validate_client_identifier(&format!("{client_type}-{}", u64::MAX))?; + validate_identifier_default(&format!("{client_type}-{}", u64::MAX), 9, 64)?; + Ok(()) } /// Checks if the client identifier is of valid format and can be parsed into /// the `ClientId` type. pub fn validate_client_identifier_format(id: &str) -> Result<(), Error> { - let split_id: Vec<_> = id.split("-").collect(); + let split_id: Vec<_> = id.split('-').collect(); let last_index = split_id.len() - 1; - let client_type_str = &id[..last_index]; + let client_type_str = split_id[..last_index].join("-"); validate_client_type(client_type_str.trim())?; @@ -126,8 +127,8 @@ pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { #[cfg(test)] mod tests { use crate::core::ics24_host::validate::{ - validate_channel_identifier, validate_client_identifier, validate_connection_identifier, - validate_identifier_default, validate_port_identifier, + validate_channel_identifier, validate_client_identifier, validate_client_type, + validate_connection_identifier, validate_identifier_default, validate_port_identifier, }; use test_log::test; @@ -215,4 +216,28 @@ mod tests { let id = validate_identifier_default("id/1", 1, 10); assert!(id.is_err()) } + + #[test] + fn parse_healthy_client_type() { + let id = validate_client_type("07-tendermint"); + assert!(id.is_ok()) + } + + #[test] + fn parse_faulty_client_type() { + let id = validate_client_type("07-tendermint-"); + assert!(id.is_err()) + } + + #[test] + fn parse_short_client_type() { + let id = validate_client_type("<7Char"); + assert!(id.is_err()) + } + + #[test] + fn parse_lengthy_client_type() { + let id = validate_client_type("InvalidClientTypeWithLengthOfClientId>65Char"); + assert!(id.is_err()) + } } From f2e599f729b1d43b7d15539014275395ecb4f8e1 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 19 Apr 2023 11:12:45 -0700 Subject: [PATCH 04/21] Some adjustments --- .../breaking-changes/621-add-client-type-validation.md | 3 ++- crates/ibc/src/core/ics24_host/identifier.rs | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md b/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md index a313d18b3..248b61b90 100644 --- a/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md +++ b/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md @@ -1,2 +1,3 @@ -- Add missing `ClientType` validation and move `ClientType` under the ICS24 section +- Add missing `ClientType` validation functions and move `ClientType` under the + ICS24 section ([#621](https://github.com/cosmos/ibc-rs/issues/621)) \ No newline at end of file diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 1cbfbcf00..4671f1c64 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -6,6 +6,8 @@ use derive_more::Into; use super::validate::*; use crate::clients::ics07_tendermint::client_type as tm_client_type; +use crate::core::ics24_host::validate::validate_client_identifier; + use crate::core::ics24_host::error::ValidationError; use crate::prelude::*; @@ -176,8 +178,6 @@ impl From for ChainId { } } -use crate::core::ics24_host::validate::validate_client_identifier; - #[cfg_attr( feature = "parity-scale-codec", derive( @@ -196,8 +196,8 @@ use crate::core::ics24_host::validate::validate_client_identifier; pub struct ClientType(String); impl ClientType { - /// Constructs a new `ClientType` from the given `String` without performing any validation. - pub fn new_unchecked(s: String) -> Self { + /// Constructs a new instance without performing any validation primarily for use in testing. + pub(crate) fn new_unchecked(s: String) -> Self { Self(s) } From 24877e438ddc42de05d5fc0abf3b3cd5b708e933 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 19 Apr 2023 11:25:14 -0700 Subject: [PATCH 05/21] Fix tests --- crates/ibc/src/core/ics02_client/handler/update_client.rs | 8 ++++---- crates/ibc/src/core/ics02_client/msgs/update_client.rs | 2 +- crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs | 2 +- .../src/core/ics03_connection/handler/conn_open_ack.rs | 2 +- .../core/ics03_connection/handler/conn_open_confirm.rs | 2 +- .../ibc/src/core/ics03_connection/msgs/conn_open_try.rs | 2 +- crates/ibc/src/core/ics24_host/identifier.rs | 4 ++-- crates/ibc/src/core/ics24_host/validate.rs | 1 + 8 files changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/ibc/src/core/ics02_client/handler/update_client.rs b/crates/ibc/src/core/ics02_client/handler/update_client.rs index 64ae14348..15c0861f8 100644 --- a/crates/ibc/src/core/ics02_client/handler/update_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/update_client.rs @@ -145,13 +145,13 @@ mod tests { #[test] fn test_update_nonexisting_client() { - let client_id = ClientId::from_str("mockclient1").unwrap(); + let client_id = ClientId::from_str("mockclient-1").unwrap(); let signer = get_dummy_account_id(); let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); let msg = MsgUpdateClient { - client_id: ClientId::from_str("nonexistingclient").unwrap(), + client_id: ClientId::from_str("nonexistingclient-0").unwrap(), client_message: MockHeader::new(Height::new(0, 46).unwrap()).into(), update_kind: UpdateKind::UpdateClient, signer, @@ -502,10 +502,10 @@ mod tests { /// Tests misbehaviour handling failure for a non-existent client #[test] fn test_misbehaviour_nonexisting_client() { - let client_id = ClientId::from_str("mockclient1").unwrap(); + let client_id = ClientId::from_str("mockclient-1").unwrap(); let height = Height::new(0, 46).unwrap(); let msg = MsgUpdateClient { - client_id: ClientId::from_str("nonexistingclient").unwrap(), + client_id: ClientId::from_str("nonexistingclient-0").unwrap(), client_message: MockMisbehaviour { client_id: client_id.clone(), header1: MockHeader::new(height), diff --git a/crates/ibc/src/core/ics02_client/msgs/update_client.rs b/crates/ibc/src/core/ics02_client/msgs/update_client.rs index 16695f6ca..ed1d7c355 100644 --- a/crates/ibc/src/core/ics02_client/msgs/update_client.rs +++ b/crates/ibc/src/core/ics02_client/msgs/update_client.rs @@ -127,7 +127,7 @@ mod tests { #[test] fn msg_update_client_serialization() { - let client_id: ClientId = "tendermint".parse().unwrap(); + let client_id: ClientId = "tendermint-0".parse().unwrap(); let signer = get_dummy_account_id(); let header = get_dummy_ics07_header(); diff --git a/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs b/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs index 22e129fa0..f2c23b2b5 100644 --- a/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs @@ -174,7 +174,7 @@ mod tests { #[test] fn msg_upgrade_client_serialization() { - let client_id: ClientId = "tendermint".parse().unwrap(); + let client_id: ClientId = "tendermint-0".parse().unwrap(); let signer = get_dummy_account_id(); let height = Height::new(1, 1).unwrap(); 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..ec86c205d 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 @@ -241,7 +241,7 @@ mod tests { let msg = MsgConnectionOpenAck::new_dummy(10, 10); // Client parameters -- identifier and correct height (matching the proof height) - let client_id = ClientId::from_str("mock_clientid").unwrap(); + let client_id = ClientId::from_str("mock_clientid-0").unwrap(); let proof_height = msg.proofs_height_on_b; let conn_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..a6bfcb7bc 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 @@ -197,7 +197,7 @@ mod tests { } fn conn_open_confirm_fixture(ctx: Ctx) -> Fixture { - let client_id = ClientId::from_str("mock_clientid").unwrap(); + let client_id = ClientId::from_str("mock_clientid-0").unwrap(); let msg = MsgConnectionOpenConfirm::new_dummy(); let counterparty = Counterparty::new( client_id.clone(), diff --git a/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs index 21f484f31..d38a69410 100644 --- a/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs +++ b/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs @@ -266,7 +266,7 @@ mod tests { .to_string(), raw: RawMsgConnectionOpenTry { counterparty: Some(RawCounterparty { - client_id: "ClientId_".to_string(), + client_id: "ClientId-0".to_string(), ..get_dummy_raw_counterparty(Some(0)) }), ..default_try_msg.clone() diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 4671f1c64..dc7b60328 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -301,9 +301,9 @@ impl Default for ClientId { /// ``` /// use core::str::FromStr; /// use ibc::core::ics24_host::identifier::ClientId; -/// let client_id = ClientId::from_str("clientidtwo"); +/// let client_id = ClientId::from_str("clientid-0"); /// assert!(client_id.is_ok()); -/// client_id.map(|id| {assert_eq!(&id, "clientidtwo")}); +/// client_id.map(|id| {assert_eq!(&id, "clientid-0")}); /// ``` impl PartialEq for ClientId { fn eq(&self, other: &str) -> bool { diff --git a/crates/ibc/src/core/ics24_host/validate.rs b/crates/ibc/src/core/ics24_host/validate.rs index 47c5d4c54..ff8af98e7 100644 --- a/crates/ibc/src/core/ics24_host/validate.rs +++ b/crates/ibc/src/core/ics24_host/validate.rs @@ -97,6 +97,7 @@ pub fn validate_client_identifier_format(id: &str) -> Result<(), Error> { /// the ICS-24 spec. pub fn validate_client_identifier(id: &str) -> Result<(), Error> { validate_identifier_default(id, 9, 64)?; + std::println!("Validating client identifier: {}", id); validate_client_identifier_format(id) } From e5f6710d83940848de68a390e76b9627050eaa95 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 19 Apr 2023 11:31:06 -0700 Subject: [PATCH 06/21] Mend ClientId for upgrade-client tests --- crates/ibc/src/core/ics02_client/handler/upgrade_client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs b/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs index 0bbf70425..64dd2a650 100644 --- a/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs @@ -190,7 +190,7 @@ mod tests { fn upgrade_client_fail_nonexisting_client(fixture: Fixture) { let Fixture { ctx, mut msg } = fixture; - msg.client_id = ClientId::from_str("nonexistingclient").unwrap(); + msg.client_id = ClientId::from_str("nonexistingclient-0").unwrap(); let res = validate(&ctx, msg); assert!( From 3010b6d94c004493e49c4b34aebe24c8d4918834 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 19 Apr 2023 11:35:03 -0700 Subject: [PATCH 07/21] Rename to default_validate_identifier --- crates/ibc/src/core/ics24_host/validate.rs | 23 +++++++++++----------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/validate.rs b/crates/ibc/src/core/ics24_host/validate.rs index ff8af98e7..dd95031cd 100644 --- a/crates/ibc/src/core/ics24_host/validate.rs +++ b/crates/ibc/src/core/ics24_host/validate.rs @@ -12,7 +12,7 @@ const VALID_SPECIAL_CHARS: &str = "._+-#[]<>"; /// max length as specified in the /// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)] /// spec. -pub fn validate_identifier_default(id: &str, min: usize, max: usize) -> Result<(), Error> { +pub fn validate_default_identifier(id: &str, min: usize, max: usize) -> Result<(), Error> { assert!(max >= min); // Check identifier is not empty @@ -67,10 +67,10 @@ pub fn validate_client_type(client_type: &str) -> Result<(), Error> { } // Check that the client type is a valid client identifier when used with `0` - validate_identifier_default(&format!("{client_type}-{}", u64::MIN), 9, 64)?; + validate_default_identifier(&format!("{client_type}-{}", u64::MIN), 9, 64)?; // Check that the client type is a valid client identifier when used with `u64::MAX` - validate_identifier_default(&format!("{client_type}-{}", u64::MAX), 9, 64)?; + validate_default_identifier(&format!("{client_type}-{}", u64::MAX), 9, 64)?; Ok(()) } @@ -96,8 +96,7 @@ pub fn validate_client_identifier_format(id: &str) -> Result<(), Error> { /// A valid client identifier must be between 9-64 characters as specified in /// the ICS-24 spec. pub fn validate_client_identifier(id: &str) -> Result<(), Error> { - validate_identifier_default(id, 9, 64)?; - std::println!("Validating client identifier: {}", id); + validate_default_identifier(id, 9, 64)?; validate_client_identifier_format(id) } @@ -106,7 +105,7 @@ pub fn validate_client_identifier(id: &str) -> Result<(), Error> { /// A valid connection identifier must be between 10-64 characters as specified /// in the ICS-24 spec. pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { - validate_identifier_default(id, 10, 64) + validate_default_identifier(id, 10, 64) } /// Default validator function for Port identifiers. @@ -114,7 +113,7 @@ pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { /// A valid port identifier must be between 2-128 characters as specified in the /// ICS-24 spec. pub fn validate_port_identifier(id: &str) -> Result<(), Error> { - validate_identifier_default(id, 2, 128) + validate_default_identifier(id, 2, 128) } /// Default validator function for Channel identifiers. @@ -122,14 +121,14 @@ pub fn validate_port_identifier(id: &str) -> Result<(), Error> { /// A valid channel identifier must be between 8-64 characters as specified in /// the ICS-24 spec. pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { - validate_identifier_default(id, 8, 64) + validate_default_identifier(id, 8, 64) } #[cfg(test)] mod tests { use crate::core::ics24_host::validate::{ validate_channel_identifier, validate_client_identifier, validate_client_type, - validate_connection_identifier, validate_identifier_default, validate_port_identifier, + validate_connection_identifier, validate_default_identifier, validate_port_identifier, }; use test_log::test; @@ -200,21 +199,21 @@ mod tests { #[test] fn parse_invalid_id_chars() { // invalid id chars - let id = validate_identifier_default("channel@01", 1, 10); + let id = validate_default_identifier("channel@01", 1, 10); assert!(id.is_err()) } #[test] fn parse_invalid_id_empty() { // invalid id empty - let id = validate_identifier_default("", 1, 10); + let id = validate_default_identifier("", 1, 10); assert!(id.is_err()) } #[test] fn parse_invalid_id_path_separator() { // invalid id with path separator - let id = validate_identifier_default("id/1", 1, 10); + let id = validate_default_identifier("id/1", 1, 10); assert!(id.is_err()) } From 262170e480b1a2897bd4185f92a3c206a151e934 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 19 Apr 2023 17:51:36 -0700 Subject: [PATCH 08/21] Refactor for more generic functions --- crates/ibc/src/core/ics24_host/validate.rs | 55 ++++++++++++++-------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/validate.rs b/crates/ibc/src/core/ics24_host/validate.rs index dd95031cd..27e11216c 100644 --- a/crates/ibc/src/core/ics24_host/validate.rs +++ b/crates/ibc/src/core/ics24_host/validate.rs @@ -50,39 +50,50 @@ pub fn validate_default_identifier(id: &str, min: usize, max: usize) -> Result<( Ok(()) } -/// Checks if the client type is of valid format and can be parsed into the -/// client identifier. -pub fn validate_client_type(client_type: &str) -> Result<(), Error> { - // Check that the client type is not blank - if client_type.is_empty() { +/// Checks if the prefix is valid and can form valid domain identifiers. +pub fn validate_prefix_identifier( + prefix: &str, + min_id_length: usize, + max_id_length: usize, +) -> Result<(), Error> { + // Checks if the prefix is not blank + if prefix.is_empty() { return Err(Error::Empty)?; } - // Check that the client type does not end with a dash + // Checks of the prefix does not end with a dash let re = safe_regex::regex!(br".*[^-]"); - if !re.is_match(client_type.as_bytes()) { + if !re.is_match(prefix.as_bytes()) { return Err(Error::InvalidPrefix { - prefix: client_type.to_string(), + prefix: prefix.to_string(), })?; } - // Check that the client type is a valid client identifier when used with `0` - validate_default_identifier(&format!("{client_type}-{}", u64::MIN), 9, 64)?; + // Checks if the prefix forms a valid default identifier when used with `0` + validate_default_identifier( + &format!("{prefix}-{}", u64::MIN), + min_id_length, + max_id_length, + )?; - // Check that the client type is a valid client identifier when used with `u64::MAX` - validate_default_identifier(&format!("{client_type}-{}", u64::MAX), 9, 64)?; + // Checks if the prefix forms a valid default identifier when used with `u64::MAX` + validate_default_identifier( + &format!("{prefix}-{}", u64::MAX), + min_id_length, + max_id_length, + )?; Ok(()) } -/// Checks if the client identifier is of valid format and can be parsed into -/// the `ClientId` type. -pub fn validate_client_identifier_format(id: &str) -> Result<(), Error> { +/// Checks if the identifier is of valid format and can be parsed into the +/// correct domain identifier type. +pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error> { let split_id: Vec<_> = id.split('-').collect(); let last_index = split_id.len() - 1; - let client_type_str = split_id[..last_index].join("-"); + let prefix = split_id[..last_index].join("-"); - validate_client_type(client_type_str.trim())?; + validate_prefix_identifier(prefix.trim(), min, max)?; split_id[last_index] .parse::() @@ -91,13 +102,17 @@ pub fn validate_client_identifier_format(id: &str) -> Result<(), Error> { Ok(()) } +/// Default validator function for the Client types. +pub fn validate_client_type(id: &str) -> Result<(), Error> { + validate_prefix_identifier(id, 9, 64) +} + /// Default validator function for Client identifiers. /// /// A valid client identifier must be between 9-64 characters as specified in -/// the ICS-24 spec. +/// the ICS-24 spec. pub fn validate_client_identifier(id: &str) -> Result<(), Error> { - validate_default_identifier(id, 9, 64)?; - validate_client_identifier_format(id) + validate_identifier(id, 9, 64) } /// Default validator function for Connection identifiers. From c6ebd3ead002891ecc09a01ef86067282a372f10 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 20 Apr 2023 07:46:51 -0700 Subject: [PATCH 09/21] Revise changelog --- .../breaking-changes/621-add-client-type-validation.md | 3 --- .../621-missing-client-identifier-validation.md | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) delete mode 100644 .changelog/unreleased/breaking-changes/621-add-client-type-validation.md create mode 100644 .changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md diff --git a/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md b/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md deleted file mode 100644 index 248b61b90..000000000 --- a/.changelog/unreleased/breaking-changes/621-add-client-type-validation.md +++ /dev/null @@ -1,3 +0,0 @@ -- Add missing `ClientType` validation functions and move `ClientType` under the - ICS24 section - ([#621](https://github.com/cosmos/ibc-rs/issues/621)) \ No newline at end of file diff --git a/.changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md b/.changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md new file mode 100644 index 000000000..5ccfd7b67 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md @@ -0,0 +1,3 @@ +- Add missing `ClientType` and `ClientId` validation checks and move `ClientType` under the + ICS24 section + ([#621](https://github.com/cosmos/ibc-rs/issues/621)) \ No newline at end of file From 72f635e299708d7824e6f656535ed5a53badb2ac Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 20 Apr 2023 10:50:57 -0700 Subject: [PATCH 10/21] Add missing counterparty client_id check --- .../ibc/src/core/ics03_connection/handler/conn_open_init.rs | 5 +++++ .../ibc/src/core/ics03_connection/handler/conn_open_try.rs | 5 +++++ 2 files changed, 10 insertions(+) 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 c3d828c88..b963503a5 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 @@ -20,6 +20,11 @@ where .validate() .map_err(ClientError::InvalidClientIdentifier)?; + msg.counterparty + .client_id() + .validate() + .map_err(ClientError::InvalidClientIdentifier)?; + // An IBC client running on the local (host) chain should exist. let client_state_of_b_on_a = ctx_a.client_state(&msg.client_id_on_a)?; client_state_of_b_on_a.confirm_not_frozen()?; 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 8d3cfd956..bd5732aa8 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 @@ -39,6 +39,11 @@ where .validate() .map_err(ClientError::InvalidClientIdentifier)?; + msg.counterparty + .client_id() + .validate() + .map_err(ClientError::InvalidClientIdentifier)?; + ctx_b.validate_self_client(msg.client_state_of_b_on_a.clone())?; let host_height = ctx_b.host_height().map_err(|_| ConnectionError::Other { From f04699e43444538565622ba93d2f73dcca47e1b8 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 20 Apr 2023 13:56:59 -0700 Subject: [PATCH 11/21] Remove duplicate checks and unnecessary methods --- .../core/ics03_connection/handler/conn_open_init.rs | 10 ---------- .../core/ics03_connection/handler/conn_open_try.rs | 13 ------------- crates/ibc/src/core/ics24_host/identifier.rs | 11 ----------- 3 files changed, 34 deletions(-) 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 b963503a5..f4901e4fa 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 @@ -2,7 +2,6 @@ use crate::prelude::*; use crate::core::context::ContextError; -use crate::core::ics02_client::error::ClientError; use crate::core::ics03_connection::connection::{ConnectionEnd, Counterparty, State}; use crate::core::ics03_connection::error::ConnectionError; use crate::core::ics03_connection::events::OpenInit; @@ -16,15 +15,6 @@ pub(crate) fn validate(ctx_a: &Ctx, msg: MsgConnectionOpenInit) -> Result<( where Ctx: ValidationContext, { - msg.client_id_on_a - .validate() - .map_err(ClientError::InvalidClientIdentifier)?; - - msg.counterparty - .client_id() - .validate() - .map_err(ClientError::InvalidClientIdentifier)?; - // An IBC client running on the local (host) chain should exist. let client_state_of_b_on_a = ctx_a.client_state(&msg.client_id_on_a)?; client_state_of_b_on_a.confirm_not_frozen()?; 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 bd5732aa8..e35e1d194 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 @@ -35,15 +35,6 @@ fn validate_impl( where Ctx: ValidationContext, { - msg.client_id_on_b - .validate() - .map_err(ClientError::InvalidClientIdentifier)?; - - msg.counterparty - .client_id() - .validate() - .map_err(ClientError::InvalidClientIdentifier)?; - ctx_b.validate_self_client(msg.client_state_of_b_on_a.clone())?; let host_height = ctx_b.host_height().map_err(|_| ConnectionError::Other { @@ -60,10 +51,6 @@ where let client_id_on_a = msg.counterparty.client_id(); - client_id_on_a - .validate() - .map_err(ClientError::InvalidClientIdentifier)?; - // Verify proofs { let client_state_of_a_on_b = diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index dc7b60328..f5ba46cd3 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -212,11 +212,6 @@ impl ClientType { pub fn as_str(&self) -> &str { &self.0 } - - /// Validates the client type to ensure it ends with a valid client identifier. - pub fn validate(&self) -> Result<(), ValidationError> { - validate_client_type(self.as_str()) - } } impl Display for ClientType { @@ -268,12 +263,6 @@ impl ClientId { pub fn as_bytes(&self) -> &[u8] { self.0.as_bytes() } - - /// Validates that the client identifier string is formatted correctly and - /// only contains valid characters. - pub fn validate(&self) -> Result<(), ValidationError> { - validate_client_identifier(self.as_str()) - } } /// This implementation provides a `to_string` method. From 5080510d6e89961b49b86d3338505eb1c754d6f3 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 24 Apr 2023 12:03:26 -0700 Subject: [PATCH 12/21] Add missing changelog --- .../breaking-changes/621-missing-client-identifier-validation.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{v0.38.0 => unreleased}/breaking-changes/621-missing-client-identifier-validation.md (100%) diff --git a/.changelog/v0.38.0/breaking-changes/621-missing-client-identifier-validation.md b/.changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md similarity index 100% rename from .changelog/v0.38.0/breaking-changes/621-missing-client-identifier-validation.md rename to .changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md From 551758392e9410785502fcc1b953c05e32888dd3 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Tue, 25 Apr 2023 13:18:47 -0700 Subject: [PATCH 13/21] Add missing message validation checks --- .../233-Missing-message-validation-checks.md | 2 + crates/ibc/src/applications/transfer/error.rs | 31 +--- .../applications/transfer/msgs/transfer.rs | 40 ++--- .../transfer/relay/send_transfer.rs | 20 +-- .../client_state/misbehaviour.rs | 30 +--- .../client_state/update_client.rs | 27 +-- .../ics07_tendermint/consensus_state.rs | 30 ++-- .../ibc/src/clients/ics07_tendermint/error.rs | 25 ++- .../src/clients/ics07_tendermint/header.rs | 53 +++++- .../clients/ics07_tendermint/misbehaviour.rs | 29 ++- crates/ibc/src/core/context.rs | 2 +- .../ibc/src/core/context/acknowledgement.rs | 6 +- .../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 | 6 +- crates/ibc/src/core/handler.rs | 12 +- .../ics02_client/handler/create_client.rs | 4 +- .../src/core/ics03_connection/connection.rs | 37 ++-- crates/ibc/src/core/ics03_connection/error.rs | 6 +- .../ics03_connection/handler/conn_open_ack.rs | 41 ++--- .../handler/conn_open_confirm.rs | 21 +-- .../ics03_connection/handler/conn_open_try.rs | 20 +-- .../msgs/conn_open_confirm.rs | 2 +- crates/ibc/src/core/ics04_channel/channel.rs | 167 +++++++++++------- crates/ibc/src/core/ics04_channel/error.rs | 66 +++---- .../ics04_channel/handler/acknowledgement.rs | 28 +-- .../handler/chan_close_confirm.rs | 34 +--- .../ics04_channel/handler/chan_close_init.rs | 28 +-- .../ics04_channel/handler/chan_open_ack.rs | 36 ++-- .../handler/chan_open_confirm.rs | 37 ++-- .../ics04_channel/handler/chan_open_init.rs | 13 +- .../ics04_channel/handler/chan_open_try.rs | 24 +-- .../core/ics04_channel/handler/recv_packet.rs | 28 +-- .../core/ics04_channel/handler/send_packet.rs | 25 +-- .../src/core/ics04_channel/handler/timeout.rs | 18 +- .../ics04_channel/handler/timeout_on_close.rs | 24 +-- .../ics04_channel/msgs/chan_close_confirm.rs | 7 +- .../ics04_channel/msgs/chan_close_init.rs | 7 +- .../core/ics04_channel/msgs/chan_open_ack.rs | 12 +- .../ics04_channel/msgs/chan_open_confirm.rs | 7 +- .../core/ics04_channel/msgs/chan_open_init.rs | 10 +- .../core/ics04_channel/msgs/chan_open_try.rs | 25 ++- .../src/core/ics04_channel/msgs/timeout.rs | 4 +- .../ics04_channel/msgs/timeout_on_close.rs | 5 +- crates/ibc/src/core/ics04_channel/packet.rs | 51 +++--- crates/ibc/src/core/ics04_channel/timeout.rs | 8 + crates/ibc/src/core/ics04_channel/version.rs | 8 +- crates/ibc/src/core/ics24_host/identifier.rs | 75 ++++---- crates/ibc/src/core/ics24_host/validate.rs | 18 +- crates/ibc/src/timestamp.rs | 5 + 55 files changed, 560 insertions(+), 675 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/233-Missing-message-validation-checks.md diff --git a/.changelog/unreleased/breaking-changes/233-Missing-message-validation-checks.md b/.changelog/unreleased/breaking-changes/233-Missing-message-validation-checks.md new file mode 100644 index 000000000..cb680b824 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/233-Missing-message-validation-checks.md @@ -0,0 +1,2 @@ +- Add missing validation checks for all the IBC message types + ([#233](https://github.com/cosmos/ibc-rs/issues/233)) diff --git a/crates/ibc/src/applications/transfer/error.rs b/crates/ibc/src/applications/transfer/error.rs index 8c00a69a5..588053f86 100644 --- a/crates/ibc/src/applications/transfer/error.rs +++ b/crates/ibc/src/applications/transfer/error.rs @@ -15,25 +15,13 @@ use crate::signer::SignerError; pub enum TokenTransferError { /// context error: `{0}` ContextError(ContextError), + /// invalid identifier: `{0}` + InvalidIdentifier(ValidationError), /// destination channel not found in the counterparty of port_id `{port_id}` and channel_id `{channel_id}` DestinationChannelNotFound { port_id: PortId, channel_id: ChannelId, }, - /// invalid port identifier `{context}`, validation error: `{validation_error}` - InvalidPortId { - context: String, - validation_error: ValidationError, - }, - /// invalid channel identifier `{context}`, validation error: `{validation_error}` - InvalidChannelId { - context: String, - validation_error: ValidationError, - }, - /// invalid packet timeout height value `{context}` - InvalidPacketTimeoutHeight { context: String }, - /// invalid packet timeout timestamp value `{timestamp}` - InvalidPacketTimeoutTimestamp { timestamp: u64 }, /// base denomination is empty EmptyBaseDenom, /// invalid prot id n trace at position: `{pos}`, validation error: `{validation_error}` @@ -91,14 +79,7 @@ impl std::error::Error for TokenTransferError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match &self { Self::ContextError(e) => Some(e), - Self::InvalidPortId { - validation_error: e, - .. - } => Some(e), - Self::InvalidChannelId { - validation_error: e, - .. - } => Some(e), + Self::InvalidIdentifier(e) => Some(e), Self::InvalidTracePortId { validation_error: e, .. @@ -127,3 +108,9 @@ impl From for TokenTransferError { Self::ContextError(err) } } + +impl From for TokenTransferError { + fn from(err: ValidationError) -> TokenTransferError { + Self::InvalidIdentifier(err) + } +} diff --git a/crates/ibc/src/applications/transfer/msgs/transfer.rs b/crates/ibc/src/applications/transfer/msgs/transfer.rs index 11ebb632e..741b33e51 100644 --- a/crates/ibc/src/applications/transfer/msgs/transfer.rs +++ b/crates/ibc/src/applications/transfer/msgs/transfer.rs @@ -1,6 +1,5 @@ //! This is the definition of a transfer messages that an application submits to a chain. -use crate::applications::transfer::packet::PacketData; use crate::prelude::*; use ibc_proto::google::protobuf::Any; @@ -8,8 +7,11 @@ use ibc_proto::ibc::applications::transfer::v1::MsgTransfer as RawMsgTransfer; use ibc_proto::protobuf::Protobuf; use crate::applications::transfer::error::TokenTransferError; +use crate::applications::transfer::packet::PacketData; +use crate::core::ics04_channel::error::PacketError; use crate::core::ics04_channel::timeout::TimeoutHeight; use crate::core::ics24_host::identifier::{ChannelId, PortId}; +use crate::core::ContextError; use crate::timestamp::Timestamp; use crate::tx_msg::Msg; @@ -51,30 +53,22 @@ impl TryFrom for MsgTransfer { fn try_from(raw_msg: RawMsgTransfer) -> Result { let timeout_timestamp_on_b = Timestamp::from_nanoseconds(raw_msg.timeout_timestamp) - .map_err(|_| TokenTransferError::InvalidPacketTimeoutTimestamp { - timestamp: raw_msg.timeout_timestamp, - })?; - - let timeout_height_on_b: TimeoutHeight = - raw_msg.timeout_height.try_into().map_err(|e| { - TokenTransferError::InvalidPacketTimeoutHeight { - context: format!("invalid timeout height {e}"), - } - })?; + .map_err(PacketError::InvalidPacketTimestamp) + .map_err(ContextError::from)?; + + let timeout_height_on_b: TimeoutHeight = raw_msg + .timeout_height + .try_into() + .map_err(ContextError::from)?; + + // Packet timeout height and packet timeout timestamp cannot both be unset. + if !timeout_height_on_b.is_set() && !timeout_timestamp_on_b.is_set() { + return Err(PacketError::MissingTimeout).map_err(ContextError::from)?; + } Ok(MsgTransfer { - port_id_on_a: raw_msg.source_port.parse().map_err(|e| { - TokenTransferError::InvalidPortId { - context: raw_msg.source_port.clone(), - validation_error: e, - } - })?, - chan_id_on_a: raw_msg.source_channel.parse().map_err(|e| { - TokenTransferError::InvalidChannelId { - context: raw_msg.source_channel.clone(), - validation_error: e, - } - })?, + port_id_on_a: raw_msg.source_port.parse()?, + chan_id_on_a: raw_msg.source_channel.parse()?, packet_data: PacketData { token: raw_msg .token diff --git a/crates/ibc/src/applications/transfer/relay/send_transfer.rs b/crates/ibc/src/applications/transfer/relay/send_transfer.rs index 569901e0f..2a286275f 100644 --- a/crates/ibc/src/applications/transfer/relay/send_transfer.rs +++ b/crates/ibc/src/applications/transfer/relay/send_transfer.rs @@ -29,9 +29,7 @@ where ctx_a.can_send_coins()?; let chan_end_path_on_a = ChannelEndPath::new(&msg.port_id_on_a, &msg.chan_id_on_a); - let chan_end_on_a = ctx_a - .channel_end(&chan_end_path_on_a) - .map_err(TokenTransferError::ContextError)?; + let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?; let port_id_on_b = chan_end_on_a.counterparty().port_id().clone(); let chan_id_on_b = chan_end_on_a @@ -44,9 +42,7 @@ where .clone(); let seq_send_path_on_a = SeqSendPath::new(&msg.port_id_on_a, &msg.chan_id_on_a); - let sequence = ctx_a - .get_next_sequence_send(&seq_send_path_on_a) - .map_err(TokenTransferError::ContextError)?; + let sequence = ctx_a.get_next_sequence_send(&seq_send_path_on_a)?; let token = &msg.packet_data.token; @@ -84,7 +80,7 @@ where } }; - send_packet_validate(ctx_a, &packet).map_err(TokenTransferError::ContextError)?; + send_packet_validate(ctx_a, &packet)?; Ok(()) } @@ -97,9 +93,7 @@ where Ctx: TokenTransferExecutionContext, { let chan_end_path_on_a = ChannelEndPath::new(&msg.port_id_on_a, &msg.chan_id_on_a); - let chan_end_on_a = ctx_a - .channel_end(&chan_end_path_on_a) - .map_err(TokenTransferError::ContextError)?; + let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?; let port_on_b = chan_end_on_a.counterparty().port_id().clone(); let chan_on_b = chan_end_on_a @@ -113,9 +107,7 @@ where // get the next sequence let seq_send_path_on_a = SeqSendPath::new(&msg.port_id_on_a, &msg.chan_id_on_a); - let sequence = ctx_a - .get_next_sequence_send(&seq_send_path_on_a) - .map_err(TokenTransferError::ContextError)?; + let sequence = ctx_a.get_next_sequence_send(&seq_send_path_on_a)?; let token = &msg.packet_data.token; @@ -155,7 +147,7 @@ where } }; - send_packet_execute(ctx_a, packet).map_err(TokenTransferError::ContextError)?; + send_packet_execute(ctx_a, packet)?; { ctx_a.log_message(format!( diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state/misbehaviour.rs b/crates/ibc/src/clients/ics07_tendermint/client_state/misbehaviour.rs index d9387750d..0fe4120fc 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state/misbehaviour.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state/misbehaviour.rs @@ -23,6 +23,8 @@ impl ClientState { client_id: &ClientId, misbehaviour: TmMisbehaviour, ) -> Result<(), ClientError> { + misbehaviour.validate_basic()?; + let header_1 = misbehaviour.header1(); let trusted_consensus_state_1 = { let consensus_state_path = @@ -41,39 +43,11 @@ impl ClientState { downcast_tm_consensus_state(consensus_state.as_ref()) }?; - self.check_misbehaviour_headers_consistency(header_1, header_2)?; - let current_timestamp = ctx.host_timestamp()?; self.verify_misbehaviour_header(header_1, &trusted_consensus_state_1, current_timestamp)?; self.verify_misbehaviour_header(header_2, &trusted_consensus_state_2, current_timestamp) } - pub fn check_misbehaviour_headers_consistency( - &self, - header_1: &TmHeader, - header_2: &TmHeader, - ) -> Result<(), ClientError> { - if header_1.signed_header.header.chain_id != header_2.signed_header.header.chain_id { - return Err(Error::InvalidRawMisbehaviour { - reason: "headers must have identical chain_ids".to_owned(), - } - .into()); - } - - if header_1.height() < header_2.height() { - return Err(Error::InvalidRawMisbehaviour { - reason: format!( - "headers1 height is less than header2 height ({} < {})", - header_1.height(), - header_2.height() - ), - } - .into()); - } - - Ok(()) - } - pub fn verify_misbehaviour_header( &self, header: &TmHeader, diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs b/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs index fcd1fb401..946406ac0 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs @@ -20,31 +20,12 @@ impl ClientState { client_id: &ClientId, header: TmHeader, ) -> Result<(), ClientError> { + // Checks that the header fields are valid. + header.validate_basic()?; + // The tendermint-light-client crate though works on heights that are assumed // to have the same revision number. We ensure this here. - if header.height().revision_number() != self.chain_id().version() { - return Err(ClientError::ClientSpecific { - description: Error::MismatchedRevisions { - current_revision: self.chain_id().version(), - update_revision: header.height().revision_number(), - } - .to_string(), - }); - } - - // We also need to ensure that the trusted height (representing the - // height of the header already on chain for which this client update is - // based on) must be smaller than height of the new header that we're - // installing. - if header.height() <= header.trusted_height { - return Err(ClientError::HeaderVerificationFailure { - reason: format!( - "header height <= header trusted height ({} <= {})", - header.height(), - header.trusted_height - ), - }); - } + header.verify_chain_id_matches(&self.chain_id())?; // Delegate to tendermint-light-client, which contains the required checks // of the new header against the trusted consensus state. diff --git a/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs b/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs index 1adbbed7d..e553f3220 100644 --- a/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs @@ -49,6 +49,18 @@ impl TryFrom for ConsensusState { type Error = Error; fn try_from(raw: RawConsensusState) -> Result { + let proto_root = raw + .root + .ok_or(Error::InvalidRawClientState { + reason: "missing commitment root".into(), + })? + .hash; + if proto_root.is_empty() { + return Err(Error::InvalidRawClientState { + reason: "empty commitment root".into(), + }); + }; + let ibc_proto::google::protobuf::Timestamp { seconds, nanos } = raw.timestamp.ok_or(Error::InvalidRawClientState { reason: "missing timestamp".into(), @@ -62,19 +74,15 @@ impl TryFrom for ConsensusState { reason: format!("invalid timestamp: {e}"), })?; + let next_validators_hash = Hash::from_bytes(Algorithm::Sha256, &raw.next_validators_hash) + .map_err(|e| Error::InvalidRawClientState { + reason: e.to_string(), + })?; + Ok(Self { - root: raw - .root - .ok_or(Error::InvalidRawClientState { - reason: "missing commitment root".into(), - })? - .hash - .into(), + root: proto_root.into(), timestamp, - next_validators_hash: Hash::from_bytes(Algorithm::Sha256, &raw.next_validators_hash) - .map_err(|e| Error::InvalidRawClientState { - reason: e.to_string(), - })?, + next_validators_hash, }) } } diff --git a/crates/ibc/src/clients/ics07_tendermint/error.rs b/crates/ibc/src/clients/ics07_tendermint/error.rs index 500d2ff32..c2864c438 100644 --- a/crates/ibc/src/clients/ics07_tendermint/error.rs +++ b/crates/ibc/src/clients/ics07_tendermint/error.rs @@ -11,7 +11,6 @@ use tendermint::account::Id; use tendermint::{Error as TendermintError, Hash}; use tendermint_light_client_verifier::errors::VerificationErrorDetail as LightClientErrorDetail; use tendermint_light_client_verifier::operations::VotingPowerTally; -use tendermint_light_client_verifier::types::Validator; use tendermint_light_client_verifier::Verdict; #[derive(Debug, Display)] @@ -69,11 +68,13 @@ pub enum Error { InvalidHeaderHeight { height: u64 }, /// Disallowed to create a new client with a frozen height FrozenHeightNotAllowed, - /// the header's current/trusted revision number (`{current_revision}`) and the update's revision number (`{update_revision}`) should be the same - MismatchedRevisions { - current_revision: u64, - update_revision: u64, + /// the header's trusted revision number (`{trusted_revision}`) and the update's revision number (`{header_revision}`) should be the same + MismatchHeightRevisions { + trusted_revision: u64, + header_revision: u64, }, + /// the given chain-id (`{given}`) does not match the chain-id of the client (`{expected}`) + MismatchHeaderChainId { given: String, expected: String }, /// not enough trust because insufficient validators overlap: `{reason}` NotEnoughTrustedValsSigned { reason: VotingPowerTally }, /// verification failed: `{detail}` @@ -82,11 +83,10 @@ pub enum Error { ProcessedTimeNotFound { client_id: ClientId, height: Height }, /// Processed height for the client `{client_id}` at height `{height}` not found ProcessedHeightNotFound { client_id: ClientId, height: Height }, - /// trusted validators `{trusted_validator_set:?}`, does not hash to latest trusted validators. Expected: `{next_validators_hash}`, got: `{trusted_val_hash}` - MisbehaviourTrustedValidatorHashMismatch { - trusted_validator_set: Vec, - next_validators_hash: Hash, - trusted_val_hash: Hash, + /// The given hash of the validators does not matches the given hash in the signed header. Expected: `{signed_header_validators_hash}`, got: `{validators_hash}` + MismatchValidatorsHashes { + validators_hash: Hash, + signed_header_validators_hash: Hash, }, /// current timestamp minus the latest consensus state timestamp is greater than or equal to the trusting period (`{duration_since_consensus_state:?}` >= `{trusting_period:?}`) ConsensusStateTimestampGteTrustingPeriod { @@ -97,11 +97,6 @@ pub enum Error { MisbehaviourHeadersBlockHashesEqual, /// headers are not at same height and are monotonically increasing MisbehaviourHeadersNotAtSameHeight, - /// header chain-id `{header_chain_id}` does not match the light client's chain-id `{chain_id}`) - MisbehaviourHeadersChainIdMismatch { - header_chain_id: String, - chain_id: String, - }, /// invalid raw client id: `{client_id}` InvalidRawClientId { client_id: String }, } diff --git a/crates/ibc/src/clients/ics07_tendermint/header.rs b/crates/ibc/src/clients/ics07_tendermint/header.rs index 0c62ec547..bf5aa39e1 100644 --- a/crates/ibc/src/clients/ics07_tendermint/header.rs +++ b/crates/ibc/src/clients/ics07_tendermint/header.rs @@ -79,6 +79,52 @@ impl Header { next_validators_hash: consensus_state.next_validators_hash, }) } + + pub fn verify_chain_id_matches(&self, chain_id: &ChainId) -> Result<(), Error> { + if self.height().revision_number() != chain_id.version() { + return Err(Error::MismatchHeaderChainId { + given: self.signed_header.header.chain_id.to_string(), + expected: chain_id.to_string(), + }); + } + Ok(()) + } + + /// Checks if the fields of a given header are consistent with the trusted fields of this header. + pub fn validate_basic(&self) -> Result<(), Error> { + if self.height().revision_number() != self.trusted_height.revision_number() { + return Err(Error::MismatchHeightRevisions { + trusted_revision: self.trusted_height.revision_number(), + header_revision: self.height().revision_number(), + }); + } + + // We need to ensure that the trusted height (representing the + // height of the header already on chain for which this client update is + // based on) must be smaller than height of the new header that we're + // installing. + if self.trusted_height >= self.height() { + return Err(Error::InvalidHeaderHeight { + height: self.height().revision_height(), + }); + } + + if self.validator_set.hash() != self.signed_header.header.validators_hash { + return Err(Error::MismatchValidatorsHashes { + signed_header_validators_hash: self.signed_header.header.validators_hash, + validators_hash: self.validator_set.hash(), + }); + } + + if self.trusted_next_validator_set.hash() != self.signed_header.header.next_validators_hash + { + return Err(Error::MismatchValidatorsHashes { + signed_header_validators_hash: self.signed_header.header.next_validators_hash, + validators_hash: self.trusted_next_validator_set.hash(), + }); + } + Ok(()) + } } impl crate::core::ics02_client::header::Header for Header { @@ -122,13 +168,6 @@ impl TryFrom for Header { .map_err(Error::InvalidRawHeader)?, }; - if header.height().revision_number() != header.trusted_height.revision_number() { - return Err(Error::MismatchedRevisions { - current_revision: header.trusted_height.revision_number(), - update_revision: header.height().revision_number(), - }); - } - Ok(header) } } diff --git a/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs b/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs index 2f23859eb..35d30847a 100644 --- a/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs +++ b/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs @@ -9,7 +9,7 @@ use prost::Message; use crate::clients::ics07_tendermint::error::Error; use crate::clients::ics07_tendermint::header::Header; use crate::core::ics02_client::error::ClientError; -use crate::core::ics24_host::identifier::{ChainId, ClientId}; +use crate::core::ics24_host::identifier::ClientId; pub const TENDERMINT_MISBEHAVIOUR_TYPE_URL: &str = "/ibc.lightclients.tendermint.v1.Misbehaviour"; @@ -42,13 +42,28 @@ impl Misbehaviour { &self.header2 } - pub fn chain_id_matches(&self, chain_id: &ChainId) -> bool { - assert_eq!( - self.header1.signed_header.header.chain_id, self.header2.signed_header.header.chain_id, - "this is enforced by the ctor" - ); + pub fn validate_basic(&self) -> Result<(), Error> { + self.header1.validate_basic()?; + self.header2.validate_basic()?; - self.header1.signed_header.header.chain_id.as_str() == chain_id.as_str() + if self.header1.signed_header.header.chain_id != self.header2.signed_header.header.chain_id + { + return Err(Error::InvalidRawMisbehaviour { + reason: "headers must have identical chain_ids".to_owned(), + }); + } + + if self.header1.height() < self.header2.height() { + return Err(Error::InvalidRawMisbehaviour { + reason: format!( + "header1 height is less than header2 height ({} < {})", + self.header1.height(), + self.header2.height() + ), + }); + } + + Ok(()) } } diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs index 17177d712..b439db65e 100644 --- a/crates/ibc/src/core/context.rs +++ b/crates/ibc/src/core/context.rs @@ -75,7 +75,7 @@ pub enum ContextError { ClientError(ClientError), /// ICS03 Connection error: {0} ConnectionError(ConnectionError), - /// Ics04 Channel error: {0} + /// ICS04 Channel error: {0} ChannelError(ChannelError), /// ICS04 Packet error: {0} PacketError(PacketError), diff --git a/crates/ibc/src/core/context/acknowledgement.rs b/crates/ibc/src/core/context/acknowledgement.rs index 3eadd1a0f..0b836bb20 100644 --- a/crates/ibc/src/core/context/acknowledgement.rs +++ b/crates/ibc/src/core/context/acknowledgement.rs @@ -186,7 +186,8 @@ mod tests { ), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let chan_end_on_a_ordered = ChannelEnd::new( State::Open, @@ -194,7 +195,8 @@ mod tests { Counterparty::new(packet.port_id_on_b.clone(), Some(packet.chan_id_on_b)), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let conn_end_on_a = ConnectionEnd::new( ConnectionState::Open, diff --git a/crates/ibc/src/core/context/chan_close_confirm.rs b/crates/ibc/src/core/context/chan_close_confirm.rs index 7a8e3e6df..53f3bae68 100644 --- a/crates/ibc/src/core/context/chan_close_confirm.rs +++ b/crates/ibc/src/core/context/chan_close_confirm.rs @@ -152,7 +152,8 @@ mod tests { ), vec![conn_id.clone()], Version::default(), - ); + ) + .unwrap(); let mut context = default_context .with_client(&client_id, client_consensus_state_height) diff --git a/crates/ibc/src/core/context/chan_close_init.rs b/crates/ibc/src/core/context/chan_close_init.rs index 338b5251c..3e6f08cb8 100644 --- a/crates/ibc/src/core/context/chan_close_init.rs +++ b/crates/ibc/src/core/context/chan_close_init.rs @@ -148,7 +148,8 @@ mod tests { ), vec![conn_id.clone()], Version::default(), - ); + ) + .unwrap(); let mut context = { let mut default_context = MockContext::default(); diff --git a/crates/ibc/src/core/context/chan_open_ack.rs b/crates/ibc/src/core/context/chan_open_ack.rs index 4d12124ac..bef706398 100644 --- a/crates/ibc/src/core/context/chan_open_ack.rs +++ b/crates/ibc/src/core/context/chan_open_ack.rs @@ -161,7 +161,8 @@ mod tests { Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_b.clone())), vec![conn_id_on_a.clone()], msg.version_on_b.clone(), - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/context/chan_open_confirm.rs b/crates/ibc/src/core/context/chan_open_confirm.rs index b2b657008..9a1a7b251 100644 --- a/crates/ibc/src/core/context/chan_open_confirm.rs +++ b/crates/ibc/src/core/context/chan_open_confirm.rs @@ -169,7 +169,8 @@ mod tests { Counterparty::new(msg.port_id_on_b.clone(), Some(ChannelId::default())), vec![conn_id_on_b.clone()], Version::default(), - ); + ) + .unwrap(); Fixture { context, diff --git a/crates/ibc/src/core/context/chan_open_init.rs b/crates/ibc/src/core/context/chan_open_init.rs index bd14acc89..cdb3d7dea 100644 --- a/crates/ibc/src/core/context/chan_open_init.rs +++ b/crates/ibc/src/core/context/chan_open_init.rs @@ -70,7 +70,8 @@ where Counterparty::new(msg.port_id_on_b.clone(), None), msg.connection_hops_on_a.clone(), msg.version_proposal.clone(), - ); + ) + .unwrap(); let chan_end_path_on_a = ChannelEndPath::new(&msg.port_id_on_a, &chan_id_on_a); ctx_a.store_channel(&chan_end_path_on_a, chan_end_on_a)?; diff --git a/crates/ibc/src/core/context/chan_open_try.rs b/crates/ibc/src/core/context/chan_open_try.rs index dc8379662..df8755356 100644 --- a/crates/ibc/src/core/context/chan_open_try.rs +++ b/crates/ibc/src/core/context/chan_open_try.rs @@ -70,7 +70,8 @@ where Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_a.clone())), msg.connection_hops_on_b.clone(), version.clone(), - ); + ) + .unwrap(); let chan_end_path_on_b = ChannelEndPath::new(&msg.port_id_on_b, &chan_id_on_b); ctx_b.store_channel(&chan_end_path_on_b, chan_end_on_b)?; diff --git a/crates/ibc/src/core/context/recv_packet.rs b/crates/ibc/src/core/context/recv_packet.rs index be2955449..05acc4311 100644 --- a/crates/ibc/src/core/context/recv_packet.rs +++ b/crates/ibc/src/core/context/recv_packet.rs @@ -212,7 +212,8 @@ mod tests { Counterparty::new(packet.port_id_on_a, Some(packet.chan_id_on_a)), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let conn_end_on_b = ConnectionEnd::new( ConnectionState::Open, diff --git a/crates/ibc/src/core/context/timeout.rs b/crates/ibc/src/core/context/timeout.rs index 84740ef96..32f1f8180 100644 --- a/crates/ibc/src/core/context/timeout.rs +++ b/crates/ibc/src/core/context/timeout.rs @@ -218,7 +218,8 @@ mod tests { ), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let chan_end_on_a_unordered = ChannelEnd::new( State::Open, @@ -226,7 +227,8 @@ mod tests { Counterparty::new(packet.port_id_on_b.clone(), Some(packet.chan_id_on_b)), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let conn_end_on_a = ConnectionEnd::new( ConnectionState::Open, diff --git a/crates/ibc/src/core/handler.rs b/crates/ibc/src/core/handler.rs index e51488bec..4257a868d 100644 --- a/crates/ibc/src/core/handler.rs +++ b/crates/ibc/src/core/handler.rs @@ -590,7 +590,8 @@ mod tests { ChannelCounterparty::new(PortId::default(), Some(ChannelId::default())), vec![ConnectionId::new(0)], ChannelVersion::default(), - ), + ) + .unwrap(), ); let msg_chan_open_ack = @@ -621,7 +622,8 @@ mod tests { ChannelCounterparty::new(PortId::default(), Some(ChannelId::default())), vec![ConnectionId::new(0)], ChannelVersion::default(), - ), + ) + .unwrap(), ); let msg_chan_open_confirm = @@ -652,7 +654,8 @@ mod tests { ChannelCounterparty::new(PortId::default(), Some(ChannelId::default())), vec![ConnectionId::new(0)], ChannelVersion::default(), - ), + ) + .unwrap(), ); let msg_chan_close_init = @@ -683,7 +686,8 @@ mod tests { ChannelCounterparty::new(PortId::default(), Some(ChannelId::default())), vec![ConnectionId::new(0)], ChannelVersion::default(), - ), + ) + .unwrap(), ); let msg_chan_close_confirm = diff --git a/crates/ibc/src/core/ics02_client/handler/create_client.rs b/crates/ibc/src/core/ics02_client/handler/create_client.rs index 40d9fee45..06b6981f7 100644 --- a/crates/ibc/src/core/ics02_client/handler/create_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/create_client.rs @@ -25,7 +25,7 @@ where { let MsgCreateClient { client_state, - consensus_state: _, + consensus_state, signer: _, } = msg; @@ -34,6 +34,8 @@ where let client_state = ctx.decode_client_state(client_state)?; + client_state.initialise(consensus_state)?; + let client_type = client_state.client_type(); let client_id = ClientId::new(client_type, id_counter).map_err(|e| { diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index 3b389d42c..c40540085 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -220,13 +220,19 @@ impl TryFrom for ConnectionEnd { type Error = ConnectionError; fn try_from(value: RawConnectionEnd) -> Result { let state = value.state.try_into()?; + if state == State::Uninitialized { return Ok(ConnectionEnd::default()); } + if value.client_id.is_empty() { return Err(ConnectionError::EmptyProtoConnectionEnd); } + if value.versions.is_empty() { + return Err(ConnectionError::EmptyVersions); + } + Self::new( state, value @@ -319,17 +325,23 @@ impl ConnectionEnd { /// Helper function to determine whether the connection is open. pub fn is_open(&self) -> bool { - self.state_matches(&State::Open) + self.state == State::Open } /// Helper function to determine whether the connection is uninitialized. pub fn is_uninitialized(&self) -> bool { - self.state_matches(&State::Uninitialized) + self.state == State::Uninitialized } - /// Helper function to compare the state of this end with another state. - pub fn state_matches(&self, other: &State) -> bool { - self.state.eq(other) + /// Checks if the state of this connection end matches with an expected state. + pub fn verify_state_matches(&self, expected: &State) -> Result<(), ConnectionError> { + if !self.state.eq(expected) { + return Err(ConnectionError::InvalidState { + expected: expected.to_string(), + actual: self.state.to_string(), + }); + } + Ok(()) } /// Getter for the client id on the local party of this connection end. @@ -353,11 +365,6 @@ impl ConnectionEnd { self.delay_period } - /// TODO: Clean this up, probably not necessary. - pub fn validate_basic(&self) -> Result<(), ValidationError> { - self.counterparty.validate_basic() - } - pub(crate) fn proto_encode_vec(&self) -> Result, ConnectionError> { let value = self .encode_vec() @@ -505,7 +512,10 @@ impl State { 1 => Ok(Self::Init), 2 => Ok(Self::TryOpen), 3 => Ok(Self::Open), - _ => Err(ConnectionError::InvalidState { state: s }), + _ => Err(ConnectionError::InvalidState { + expected: "Must be one of: 0, 1, 2, 3".to_string(), + actual: s.to_string(), + }), } } @@ -542,7 +552,10 @@ impl TryFrom for State { 1 => Ok(Self::Init), 2 => Ok(Self::TryOpen), 3 => Ok(Self::Open), - _ => Err(ConnectionError::InvalidState { state: value }), + _ => Err(ConnectionError::InvalidState { + expected: "Must be one of: 0, 1, 2, 3".to_string(), + actual: value.to_string(), + }), } } } diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs index 32e88443f..41c4cd22e 100644 --- a/crates/ibc/src/core/ics03_connection/error.rs +++ b/crates/ibc/src/core/ics03_connection/error.rs @@ -14,10 +14,8 @@ use ibc_proto::protobuf::Error as ProtoError; pub enum ConnectionError { /// client error: `{0}` Client(client_error::ClientError), - /// connection state is unknown: `{state}` - InvalidState { state: i32 }, - /// connection end for identifier `{connection_id}` was never initialized - ConnectionMismatch { connection_id: ConnectionId }, + /// invalid connection state: expected `{expected}`, actual `{actual}` + InvalidState { expected: String, actual: String }, /// invalid connection end error: `{0}` InvalidConnectionEnd(ProtoError), /// consensus height claimed by the client on the other party is too advanced: `{target_height}` (host chain current height: `{current_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 03917928c..b6913bb55 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 @@ -48,32 +48,18 @@ where 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(), - } - .into()); - } + vars.conn_end_on_a.verify_state_matches(&State::Init)?; // Proof verification. { - let client_state_of_b_on_a = - ctx_a - .client_state(vars.client_id_on_a()) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch client state".to_string(), - })?; + let client_state_of_b_on_a = ctx_a.client_state(vars.client_id_on_a())?; client_state_of_b_on_a.confirm_not_frozen()?; client_state_of_b_on_a.validate_proof_height(msg.proofs_height_on_b)?; let client_cons_state_path_on_a = ClientConsensusStatePath::new(vars.client_id_on_a(), &msg.proofs_height_on_b); - let consensus_state_of_b_on_a = ctx_a - .consensus_state(&client_cons_state_path_on_a) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch client consensus state".to_string(), - })?; + let consensus_state_of_b_on_a = ctx_a.consensus_state(&client_cons_state_path_on_a)?; let prefix_on_a = ctx_a.commitment_prefix(); let prefix_on_b = vars.conn_end_on_a.counterparty().prefix(); @@ -115,11 +101,8 @@ where client_error: e, })?; - let expected_consensus_state_of_a_on_b = ctx_a - .host_consensus_state(&msg.consensus_height_of_a_on_b) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch host consensus state".to_string(), - })?; + let expected_consensus_state_of_a_on_b = + ctx_a.host_consensus_state(&msg.consensus_height_of_a_on_b)?; let client_cons_state_path_on_b = ClientConsensusStatePath::new(vars.client_id_on_b(), &msg.consensus_height_of_a_on_b); @@ -322,11 +305,10 @@ mod tests { }) => { assert_eq!(cons_state_height, target_height); } - ContextError::ConnectionError(ConnectionError::ConnectionMismatch { - connection_id, - }) => { - assert_eq!(connection_id, right_connection_id) - } + ContextError::ConnectionError(ConnectionError::InvalidState { + expected: _, + actual: _, + }) => {} _ => unreachable!(), } } @@ -392,8 +374,9 @@ mod tests { #[test] fn conn_open_ack_connection_mismatch() { let fxt = conn_open_ack_fixture(Ctx::NewWithConnectionEndOpen); - let expected_err = ContextError::ConnectionError(ConnectionError::ConnectionMismatch { - connection_id: fxt.msg.conn_id_on_a.clone(), + let expected_err = ContextError::ConnectionError(ConnectionError::InvalidState { + expected: State::Init.to_string(), + actual: State::Open.to_string(), }); conn_open_ack_validate(&fxt, Expect::Failure(Some(expected_err))); } 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 e712064c8..72534cd1c 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 @@ -30,12 +30,8 @@ where Ctx: ValidationContext, { let conn_end_on_b = vars.conn_end_on_b(); - if !conn_end_on_b.state_matches(&State::TryOpen) { - return Err(ConnectionError::ConnectionMismatch { - connection_id: msg.conn_id_on_b.clone(), - } - .into()); - } + + conn_end_on_b.verify_state_matches(&State::TryOpen)?; let client_id_on_a = vars.client_id_on_a(); let client_id_on_b = vars.client_id_on_b(); @@ -43,23 +39,14 @@ where // Verify proofs { - let client_state_of_a_on_b = - ctx_b - .client_state(client_id_on_b) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch client state".to_string(), - })?; + let client_state_of_a_on_b = ctx_b.client_state(client_id_on_b)?; client_state_of_a_on_b.confirm_not_frozen()?; client_state_of_a_on_b.validate_proof_height(msg.proof_height_on_a)?; let client_cons_state_path_on_b = ClientConsensusStatePath::new(client_id_on_b, &msg.proof_height_on_a); - let consensus_state_of_a_on_b = ctx_b - .consensus_state(&client_cons_state_path_on_b) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch client consensus state".to_string(), - })?; + let consensus_state_of_a_on_b = ctx_b.consensus_state(&client_cons_state_path_on_b)?; let prefix_on_a = conn_end_on_b.counterparty().prefix(); let prefix_on_b = ctx_b.commitment_prefix(); 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 ad4e4a74f..d7d8b1c4e 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 @@ -53,23 +53,14 @@ where // Verify proofs { - let client_state_of_a_on_b = - ctx_b - .client_state(vars.conn_end_on_b.client_id()) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch client state".to_string(), - })?; + let client_state_of_a_on_b = ctx_b.client_state(vars.conn_end_on_b.client_id())?; client_state_of_a_on_b.confirm_not_frozen()?; client_state_of_a_on_b.validate_proof_height(msg.proofs_height_on_a)?; let client_cons_state_path_on_b = ClientConsensusStatePath::new(&msg.client_id_on_b, &msg.proofs_height_on_a); - let consensus_state_of_a_on_b = ctx_b - .consensus_state(&client_cons_state_path_on_b) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch client consensus state".to_string(), - })?; + let consensus_state_of_a_on_b = ctx_b.consensus_state(&client_cons_state_path_on_b)?; let prefix_on_a = vars.conn_end_on_b.counterparty().prefix(); let prefix_on_b = ctx_b.commitment_prefix(); @@ -107,11 +98,8 @@ where client_error: e, })?; - let expected_consensus_state_of_b_on_a = ctx_b - .host_consensus_state(&msg.consensus_height_of_b_on_a) - .map_err(|_| ConnectionError::Other { - description: "failed to fetch host consensus state".to_string(), - })?; + let expected_consensus_state_of_b_on_a = + ctx_b.host_consensus_state(&msg.consensus_height_of_b_on_a)?; let client_cons_state_path_on_a = ClientConsensusStatePath::new(client_id_on_a, &msg.consensus_height_of_b_on_a); diff --git a/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs index 5960f4e91..40f07dd65 100644 --- a/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs +++ b/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs @@ -87,7 +87,7 @@ pub mod test_util { pub fn get_dummy_raw_msg_conn_open_confirm() -> RawMsgConnectionOpenConfirm { RawMsgConnectionOpenConfirm { - connection_id: "srcconnection".to_string(), + connection_id: "src-connection-0".to_string(), proof_ack: get_dummy_proof(), proof_height: Some(Height { revision_number: 0, diff --git a/crates/ibc/src/core/ics04_channel/channel.rs b/crates/ibc/src/core/ics04_channel/channel.rs index 33ac662a9..a195a0f54 100644 --- a/crates/ibc/src/core/ics04_channel/channel.rs +++ b/crates/ibc/src/core/ics04_channel/channel.rs @@ -59,8 +59,8 @@ impl TryFrom for IdentifiedChannelEnd { }; Ok(IdentifiedChannelEnd { - port_id: value.port_id.parse().map_err(ChannelError::Identifier)?, - channel_id: value.channel_id.parse().map_err(ChannelError::Identifier)?, + port_id: value.port_id.parse()?, + channel_id: value.channel_id.parse()?, channel_end: raw_channel_end.try_into()?, }) } @@ -117,18 +117,6 @@ impl Display for ChannelEnd { } } -impl Default for ChannelEnd { - fn default() -> Self { - ChannelEnd { - state: State::Uninitialized, - ordering: Default::default(), - remote: Counterparty::default(), - connection_hops: Vec::new(), - version: Version::default(), - } - } -} - impl Protobuf for ChannelEnd {} impl TryFrom for ChannelEnd { @@ -137,10 +125,6 @@ impl TryFrom for ChannelEnd { fn try_from(value: RawChannel) -> Result { let chan_state: State = State::from_i32(value.state)?; - if chan_state == State::Uninitialized { - return Ok(ChannelEnd::default()); - } - let chan_ordering = Order::from_i32(value.ordering)?; // Assemble the 'remote' attribute of the Channel, which represents the Counterparty. @@ -154,18 +138,11 @@ impl TryFrom for ChannelEnd { .connection_hops .into_iter() .map(|conn_id| ConnectionId::from_str(conn_id.as_str())) - .collect::, _>>() - .map_err(ChannelError::Identifier)?; + .collect::, _>>()?; let version = value.version.into(); - Ok(ChannelEnd::new( - chan_state, - chan_ordering, - remote, - connection_hops, - version, - )) + ChannelEnd::new(chan_state, chan_ordering, remote, connection_hops, version) } } @@ -186,8 +163,12 @@ impl From for RawChannel { } impl ChannelEnd { - /// Creates a new ChannelEnd in state Uninitialized and other fields parametrized. - pub fn new( + /// Creates a new `ChannelEnd` without performing basic validation on its arguments. + /// + /// NOTE: This method is meant for the proto message conversion from the domain + /// `MsgChannelOpenInit` and `MsgChannelOpenTry` types to satisfy their `Protobuf` + /// trait bounds. + pub(super) fn new_unchecked( state: State, ordering: Order, remote: Counterparty, @@ -203,6 +184,19 @@ impl ChannelEnd { } } + /// Creates a new `ChannelEnd` with performing basic validation on its arguments. + pub fn new( + state: State, + ordering: Order, + remote: Counterparty, + connection_hops: Vec, + version: Version, + ) -> Result { + let channel_end = Self::new_unchecked(state, ordering, remote, connection_hops, version); + channel_end.validate_basic()?; + Ok(channel_end) + } + /// Updates the ChannelEnd to assume a new State 's'. pub fn set_state(&mut self, s: State) { self.state = s; @@ -218,7 +212,7 @@ impl ChannelEnd { /// Returns `true` if this `ChannelEnd` is in state [`State::Open`]. pub fn is_open(&self) -> bool { - self.state_matches(&State::Open) + self.state == State::Open } pub fn state(&self) -> &State { @@ -242,18 +236,57 @@ impl ChannelEnd { } pub fn validate_basic(&self) -> Result<(), ChannelError> { - if self.connection_hops.len() != 1 { + if self.state == State::Uninitialized { + return Err(ChannelError::InvalidState { + expected: "Channel state cannot be Uninitialized".to_string(), + actual: self.state.to_string(), + }); + } + + if self.ordering == Order::None { + return Err(ChannelError::InvalidOrderType { + expected: "Channel ordering cannot be None".to_string(), + actual: self.ordering.to_string(), + }); + } + + // Current IBC version only supports one connection hop. + self.verify_connection_hops_length(1)?; + + Ok(()) + } + + /// Checks if the `connection_hops` has a length of `expected`. + pub fn verify_connection_hops_length(&self, expected: usize) -> Result<(), ChannelError> { + if self.connection_hops.len() != expected { return Err(ChannelError::InvalidConnectionHopsLength { - expected: 1, + expected, actual: self.connection_hops.len(), }); } - self.counterparty().validate_basic() + Ok(()) + } + + /// Checks if the state of this channel end matches the expected state. + pub fn verify_state_matches(&self, expected: &State) -> Result<(), ChannelError> { + if !self.state.eq(expected) { + return Err(ChannelError::InvalidState { + expected: expected.to_string(), + actual: self.state.to_string(), + }); + } + Ok(()) } - /// Helper function to compare the state of this end with another state. - pub fn state_matches(&self, other: &State) -> bool { - self.state.eq(other) + /// Checks if the state of this channel end is not closed. + pub fn verify_not_closed(&self) -> Result<(), ChannelError> { + if self.state.eq(&State::Closed) { + return Err(ChannelError::InvalidState { + expected: "Channel state cannot be Closed".to_string(), + actual: self.state.to_string(), + }); + } + Ok(()) } /// Helper function to compare the order of this end with another order. @@ -265,8 +298,25 @@ impl ChannelEnd { self.connection_hops.eq(other) } - pub fn counterparty_matches(&self, other: &Counterparty) -> bool { - self.counterparty().eq(other) + /// Checks if the counterparty of this channel end matches with an expected counterparty. + pub fn verify_counterparty_matches(&self, expected: &Counterparty) -> Result<(), ChannelError> { + if !self.counterparty().eq(expected) { + return Err(ChannelError::InvalidCounterparty { + expected: expected.clone(), + actual: self.counterparty().clone(), + }); + } + Ok(()) + } + + pub(crate) fn verify_empty_counterparty_channel_id(&self) -> Result<(), ChannelError> { + if !self.counterparty().channel_id.eq(&None) { + return Err(ChannelError::InvalidChannelId { + expected: "Counterparty channel id must be empty".to_string(), + actual: format!("{:?}", self.counterparty().channel_id), + }); + } + Ok(()) } pub fn version_matches(&self, other: &Version) -> bool { @@ -317,10 +367,6 @@ impl Counterparty { pub fn channel_id(&self) -> Option<&ChannelId> { self.channel_id.as_ref() } - - pub fn validate_basic(&self) -> Result<(), ChannelError> { - Ok(()) - } } impl Display for Counterparty { @@ -349,19 +395,11 @@ impl TryFrom for Counterparty { let channel_id: Option = if raw_counterparty.channel_id.is_empty() { None } else { - Some( - raw_counterparty - .channel_id - .parse() - .map_err(ChannelError::Identifier)?, - ) + Some(raw_counterparty.channel_id.parse()?) }; Ok(Counterparty::new( - raw_counterparty - .port_id - .parse() - .map_err(ChannelError::Identifier)?, + raw_counterparty.port_id.parse()?, channel_id, )) } @@ -427,8 +465,9 @@ impl Order { 0 => Ok(Self::None), 1 => Ok(Self::Unordered), 2 => Ok(Self::Ordered), - _ => Err(ChannelError::UnknownOrderType { - type_id: nr.to_string(), + _ => Err(ChannelError::InvalidOrderType { + expected: "Must be one of 0, 1, 2".to_string(), + actual: nr.to_string(), }), } } @@ -442,8 +481,9 @@ impl FromStr for Order { "uninitialized" => Ok(Self::None), "unordered" => Ok(Self::Unordered), "ordered" => Ok(Self::Ordered), - _ => Err(ChannelError::UnknownOrderType { - type_id: s.to_string(), + _ => Err(ChannelError::InvalidOrderType { + expected: "Must be one of 'uninitialized', 'unordered', 'ordered'".to_string(), + actual: s.to_string(), }), } } @@ -491,7 +531,10 @@ impl State { 2 => Ok(Self::TryOpen), 3 => Ok(Self::Open), 4 => Ok(Self::Closed), - _ => Err(ChannelError::UnknownState { state: s }), + _ => Err(ChannelError::InvalidState { + expected: "Must be one of: 0, 1, 2, 3, 4".to_string(), + actual: s.to_string(), + }), } } @@ -538,13 +581,13 @@ pub mod test_util { } /// Returns a dummy `RawChannel`, for testing only! - pub fn get_dummy_raw_channel_end(channel_id: Option) -> RawChannel { + pub fn get_dummy_raw_channel_end(state: i32, channel_id: Option) -> RawChannel { let channel_id = match channel_id { Some(id) => ChannelId::new(id).to_string(), None => "".to_string(), }; RawChannel { - state: 1, + state, ordering: 2, counterparty: Some(get_dummy_raw_counterparty(channel_id)), connection_hops: vec![ConnectionId::default().to_string()], @@ -567,7 +610,7 @@ mod tests { #[test] fn channel_end_try_from_raw() { - let raw_channel_end = get_dummy_raw_channel_end(Some(0)); + let raw_channel_end = get_dummy_raw_channel_end(2, Some(0)); let empty_raw_channel_end = RawChannel { counterparty: None, @@ -622,12 +665,12 @@ mod tests { name: "Raw channel end with two correct connection ids in connection hops" .to_string(), params: RawChannel { - connection_hops: vec!["connection1".to_string(), "connection2".to_string()] + connection_hops: vec!["connection-1".to_string(), "connection-2".to_string()] .into_iter() .collect(), ..raw_channel_end.clone() }, - want_pass: true, + want_pass: false, }, Test { name: "Raw channel end with correct params".to_string(), diff --git a/crates/ibc/src/core/ics04_channel/error.rs b/crates/ibc/src/core/ics04_channel/error.rs index 6813c1976..6a46c2769 100644 --- a/crates/ibc/src/core/ics04_channel/error.rs +++ b/crates/ibc/src/core/ics04_channel/error.rs @@ -1,3 +1,4 @@ +use super::channel::Counterparty; use super::packet::Sequence; use super::timeout::TimeoutHeight; use crate::core::ics02_client::error as client_error; @@ -18,12 +19,14 @@ use displaydoc::Display; pub enum ChannelError { /// port error: `{0}` Port(port_error::PortError), - /// channel state unknown: `{state}` - UnknownState { state: i32 }, - /// channel order type unknown: `{type_id}` - UnknownOrderType { type_id: String }, /// invalid channel end: `{channel_end}` InvalidChannelEnd { channel_end: String }, + /// invalid channel id: expected `{expected}`, actual `{actual}` + InvalidChannelId { expected: String, actual: String }, + /// invalid channel state: expected `{expected}`, actual `{actual}` + InvalidState { expected: String, actual: String }, + /// invalid channel order type: expected `{expected}`, actual `{actual}` + InvalidOrderType { expected: String, actual: String }, /// invalid connection hops length: expected `{expected}`; actual `{actual}` InvalidConnectionHopsLength { expected: usize, actual: usize }, /// invalid signer address error: `{0}` @@ -34,11 +37,8 @@ pub enum ChannelError { NonUtf8PacketData, /// missing counterparty MissingCounterparty, - /// expected version `{expected_version}` , got `{got_version}` - VersionNotSupported { - expected_version: Version, - got_version: Version, - }, + /// version not supported: expected `{expected}`, actual `{actual}` + VersionNotSupported { expected: Version, actual: Version }, /// missing channel end MissingChannel, /// the channel end (`{port_id}`, `{channel_id}`) does not exist @@ -58,8 +58,11 @@ pub enum ChannelError { value: String, error: core::num::ParseIntError, }, - /// Invalid channel id in counterparty - InvalidCounterpartyChannelId, + /// invalid channel counterparty: expected `{expected}`, actual `{actual}` + InvalidCounterparty { + expected: Counterparty, + actual: Counterparty, + }, /// Processed time for the client `{client_id}` at height `{height}` not found ProcessedTimeNotFound { client_id: ClientId, height: Height }, /// Processed height for the client `{client_id}` at height `{height}` not found @@ -70,18 +73,12 @@ pub enum ChannelError { AppModule { description: String }, /// other error: `{description}` Other { description: String }, - /// Channel `{channel_id}` is Closed - ChannelClosed { channel_id: ChannelId }, - /// the associated connection `{connection_id}` is not OPEN - ConnectionNotOpen { connection_id: ConnectionId }, /// Undefined counterparty connection for `{connection_id}` UndefinedConnectionCounterparty { connection_id: ConnectionId }, - /// Channel `{channel_id}` should not be state `{state}` - InvalidChannelState { channel_id: ChannelId, state: State }, /// invalid proof: empty proof InvalidProof, /// identifier error: `{0}` - Identifier(ValidationError), + InvalidIdentifier(ValidationError), } #[derive(Debug, Display)] @@ -90,13 +87,6 @@ pub enum PacketError { Connection(connection_error::ConnectionError), /// channel error: `{0}` Channel(ChannelError), - /// Channel `{channel_id}` is Closed - ChannelClosed { channel_id: ChannelId }, - /// packet destination port `{port_id}` and channel `{channel_id}` doesn't match the counterparty's port/channel - InvalidPacketCounterparty { - port_id: PortId, - channel_id: ChannelId, - }, /// Receiving chain block height `{chain_height}` >= packet timeout height `{timeout_height}` LowPacketHeight { chain_height: Height, @@ -148,14 +138,16 @@ pub enum PacketError { RouteNotFound, /// packet sequence cannot be 0 ZeroPacketSequence, - /// invalid timeout height for the packet - InvalidTimeoutHeight, /// packet data bytes cannot be empty ZeroPacketData, + /// invalid timeout height for the packet + InvalidTimeoutHeight, /// Invalid packet timeout timestamp value error: `{0}` InvalidPacketTimestamp(crate::timestamp::ParseTimestampError), - /// identifier error: `{0}` - Identifier(ValidationError), + /// missing timeout + MissingTimeout, + /// invalid identifier error: `{0}` + InvalidIdentifier(ValidationError), /// Missing sequence number for sending packets on port `{port_id}` and channel `{channel_id}` MissingNextSendSeq { port_id: PortId, @@ -182,6 +174,18 @@ pub enum PacketError { CannotEncodeSequence { sequence: Sequence }, } +impl From for ChannelError { + fn from(err: ValidationError) -> Self { + Self::InvalidIdentifier(err) + } +} + +impl From for PacketError { + fn from(err: ValidationError) -> Self { + Self::InvalidIdentifier(err) + } +} + #[cfg(feature = "std")] impl std::error::Error for PacketError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { @@ -189,7 +193,7 @@ impl std::error::Error for PacketError { Self::Connection(e) => Some(e), Self::Channel(e) => Some(e), Self::Signer(e) => Some(e), - Self::Identifier(e) => Some(e), + Self::InvalidIdentifier(e) => Some(e), _ => None, } } @@ -200,7 +204,7 @@ impl std::error::Error for ChannelError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match &self { Self::Port(e) => Some(e), - Self::Identifier(e) => Some(e), + Self::InvalidIdentifier(e) => Some(e), Self::Signer(e) => Some(e), Self::PacketVerificationFailed { client_error: e, .. diff --git a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs b/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs index f632e418f..39b157038 100644 --- a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs +++ b/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs @@ -1,6 +1,5 @@ use crate::core::ics03_connection::connection::State as ConnectionState; use crate::core::ics03_connection::delay::verify_conn_delay_passed; -use crate::core::ics04_channel::channel::State; use crate::core::ics04_channel::channel::{Counterparty, Order}; use crate::core::ics04_channel::commitment::{compute_ack_commitment, compute_packet_commitment}; use crate::core::ics04_channel::error::ChannelError; @@ -22,35 +21,21 @@ where let chan_end_path_on_a = ChannelEndPath::new(&packet.port_id_on_a, &packet.chan_id_on_a); let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?; - if !chan_end_on_a.state_matches(&State::Open) { - return Err(PacketError::ChannelClosed { - channel_id: packet.chan_id_on_a.clone(), - } - .into()); - } + // Checks the channel end not be `Closed`. + // This allows for optimistic packet processing before a channel opens + chan_end_on_a.verify_not_closed()?; let counterparty = Counterparty::new( packet.port_id_on_b.clone(), Some(packet.chan_id_on_b.clone()), ); - if !chan_end_on_a.counterparty_matches(&counterparty) { - return Err(PacketError::InvalidPacketCounterparty { - port_id: packet.port_id_on_b.clone(), - channel_id: packet.chan_id_on_b.clone(), - } - .into()); - } + chan_end_on_a.verify_counterparty_matches(&counterparty)?; let conn_id_on_a = &chan_end_on_a.connection_hops()[0]; let conn_end_on_a = ctx_a.connection_end(conn_id_on_a)?; - if !conn_end_on_a.state_matches(&ConnectionState::Open) { - return Err(PacketError::ConnectionNotOpen { - connection_id: chan_end_on_a.connection_hops()[0].clone(), - } - .into()); - } + conn_end_on_a.verify_state_matches(&ConnectionState::Open)?; let commitment_path_on_a = CommitmentPath::new(&packet.port_id_on_a, &packet.chan_id_on_a, packet.seq_on_a); @@ -184,7 +169,8 @@ mod tests { Counterparty::new(packet.port_id_on_b.clone(), Some(packet.chan_id_on_b)), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let conn_end_on_a = ConnectionEnd::new( ConnectionState::Open, 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 6c1db93a2..1f501ae65 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 @@ -1,7 +1,7 @@ //! Protocol logic specific to ICS4 messages of type `MsgChannelCloseConfirm`. use crate::core::ics03_connection::connection::State as ConnectionState; -use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State}; +use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State as ChannelState}; use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::msgs::chan_close_confirm::MsgChannelCloseConfirm; use crate::core::ics24_host::path::{ChannelEndPath, ClientConsensusStatePath}; @@ -18,30 +18,11 @@ where let chan_end_on_b = ctx_b.channel_end(&chan_end_path_on_b)?; // Validate that the channel end is in a state where it can be closed. - if chan_end_on_b.state_matches(&State::Closed) { - return Err(ChannelError::ChannelClosed { - channel_id: msg.chan_id_on_b.clone(), - } - .into()); - } - - // An OPEN IBC connection running on the local (host) chain should exist. - if chan_end_on_b.connection_hops().len() != 1 { - return Err(ChannelError::InvalidConnectionHopsLength { - expected: 1, - actual: chan_end_on_b.connection_hops().len(), - } - .into()); - } + chan_end_on_b.verify_not_closed()?; let conn_end_on_b = ctx_b.connection_end(&chan_end_on_b.connection_hops()[0])?; - if !conn_end_on_b.state_matches(&ConnectionState::Open) { - return Err(ChannelError::ConnectionNotOpen { - connection_id: chan_end_on_b.connection_hops()[0].clone(), - } - .into()); - } + conn_end_on_b.verify_state_matches(&ConnectionState::Open)?; // Verify proofs { @@ -59,7 +40,7 @@ where let chan_id_on_a = chan_end_on_b .counterparty() .channel_id() - .ok_or(ChannelError::InvalidCounterpartyChannelId)?; + .ok_or(ChannelError::MissingCounterparty)?; let conn_id_on_a = conn_end_on_b.counterparty().connection_id().ok_or( ChannelError::UndefinedConnectionCounterparty { connection_id: chan_end_on_b.connection_hops()[0].clone(), @@ -67,12 +48,12 @@ where )?; let expected_chan_end_on_a = ChannelEnd::new( - State::Closed, + ChannelState::Closed, *chan_end_on_b.ordering(), Counterparty::new(msg.port_id_on_b.clone(), Some(msg.chan_id_on_b.clone())), vec![conn_id_on_a.clone()], chan_end_on_b.version().clone(), - ); + )?; let chan_end_path_on_a = ChannelEndPath::new(port_id_on_a, chan_id_on_a); // Verify the proof for the channel state against the expected channel end. @@ -145,7 +126,8 @@ mod tests { ), vec![conn_id.clone()], Version::default(), - ); + ) + .unwrap(); let context = default_context .with_client(&client_id, client_consensus_state_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 2302cb26a..c20181b70 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 @@ -1,7 +1,5 @@ //! Protocol logic specific to ICS4 messages of type `MsgChannelCloseInit`. use crate::core::ics03_connection::connection::State as ConnectionState; -use crate::core::ics04_channel::channel::State; -use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::msgs::chan_close_init::MsgChannelCloseInit; use crate::core::ics24_host::path::ChannelEndPath; @@ -15,31 +13,14 @@ where let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?; // Validate that the channel end is in a state where it can be closed. - if chan_end_on_a.state_matches(&State::Closed) { - return Err(ChannelError::InvalidChannelState { - channel_id: msg.chan_id_on_a.clone(), - state: chan_end_on_a.state, - } - .into()); - } + chan_end_on_a.verify_not_closed()?; // An OPEN IBC connection running on the local (host) chain should exist. - if chan_end_on_a.connection_hops().len() != 1 { - return Err(ChannelError::InvalidConnectionHopsLength { - expected: 1, - actual: chan_end_on_a.connection_hops().len(), - } - .into()); - } + chan_end_on_a.verify_connection_hops_length(1)?; let conn_end_on_a = ctx_a.connection_end(&chan_end_on_a.connection_hops()[0])?; - if !conn_end_on_a.state_matches(&ConnectionState::Open) { - return Err(ChannelError::ConnectionNotOpen { - connection_id: chan_end_on_a.connection_hops()[0].clone(), - } - .into()); - } + conn_end_on_a.verify_state_matches(&ConnectionState::Open)?; let client_id_on_a = conn_end_on_a.client_id(); let client_state_of_b_on_a = ctx_a.client_state(client_id_on_a)?; @@ -98,7 +79,8 @@ mod tests { ), vec![conn_id.clone()], Version::default(), - ); + ) + .unwrap(); let context = { let default_context = MockContext::default(); 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 820ff208e..be60e54f2 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 @@ -1,6 +1,6 @@ //! Protocol logic specific to ICS4 messages of type `MsgChannelOpenAck`. use crate::core::ics03_connection::connection::State as ConnectionState; -use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State}; +use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State as ChannelState}; use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::msgs::chan_open_ack::MsgChannelOpenAck; use crate::core::ics24_host::path::{ChannelEndPath, ClientConsensusStatePath}; @@ -16,32 +16,14 @@ where let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?; // Validate that the channel end is in a state where it can be ack. - if !chan_end_on_a.state_matches(&State::Init) { - return Err(ChannelError::InvalidChannelState { - channel_id: msg.chan_id_on_a.clone(), - state: chan_end_on_a.state, - } - .into()); - } + chan_end_on_a.verify_state_matches(&ChannelState::Init)?; // An OPEN IBC connection running on the local (host) chain should exist. - - if chan_end_on_a.connection_hops().len() != 1 { - return Err(ChannelError::InvalidConnectionHopsLength { - expected: 1, - actual: chan_end_on_a.connection_hops().len(), - } - .into()); - } + chan_end_on_a.verify_connection_hops_length(1)?; let conn_end_on_a = ctx_a.connection_end(&chan_end_on_a.connection_hops()[0])?; - if !conn_end_on_a.state_matches(&ConnectionState::Open) { - return Err(ChannelError::ConnectionNotOpen { - connection_id: chan_end_on_a.connection_hops()[0].clone(), - } - .into()); - } + conn_end_on_a.verify_state_matches(&ConnectionState::Open)?; // Verify proofs { @@ -63,14 +45,14 @@ where )?; let expected_chan_end_on_b = ChannelEnd::new( - State::TryOpen, + ChannelState::TryOpen, // Note: Both ends of a channel must have the same ordering, so it's // fine to use A's ordering here *chan_end_on_a.ordering(), Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_a.clone())), vec![conn_id_on_b.clone()], msg.version_on_b.clone(), - ); + )?; let chan_end_path_on_b = ChannelEndPath::new(port_id_on_b, &msg.chan_id_on_b); // Verify the proof for the channel state against the expected channel end. @@ -148,7 +130,8 @@ mod tests { Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_b.clone())), vec![conn_id_on_a.clone()], msg.version_on_b.clone(), - ); + ) + .unwrap(); Fixture { context, @@ -202,7 +185,8 @@ mod tests { Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_b.clone())), vec![conn_id_on_a.clone()], msg.version_on_b.clone(), - ); + ) + .unwrap(); let context = context .with_client(&client_id_on_a, Height::new(0, proof_height).unwrap()) .with_connection(conn_id_on_a, conn_end_on_a) 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 93548ae1b..bef94a29f 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 @@ -1,7 +1,7 @@ //! Protocol logic specific to ICS4 messages of type `MsgChannelOpenConfirm`. use crate::core::ics03_connection::connection::State as ConnectionState; -use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State}; +use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State as ChannelState}; use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::msgs::chan_open_confirm::MsgChannelOpenConfirm; use crate::core::ics24_host::path::{ChannelEndPath, ClientConsensusStatePath}; @@ -18,31 +18,14 @@ where let chan_end_on_b = ctx_b.channel_end(&chan_end_path_on_b)?; // Validate that the channel end is in a state where it can be confirmed. - if !chan_end_on_b.state_matches(&State::TryOpen) { - return Err(ChannelError::InvalidChannelState { - channel_id: msg.chan_id_on_b.clone(), - state: chan_end_on_b.state, - } - .into()); - } + chan_end_on_b.verify_state_matches(&ChannelState::TryOpen)?; // An OPEN IBC connection running on the local (host) chain should exist. - if chan_end_on_b.connection_hops().len() != 1 { - return Err(ChannelError::InvalidConnectionHopsLength { - expected: 1, - actual: chan_end_on_b.connection_hops().len(), - } - .into()); - } + chan_end_on_b.verify_connection_hops_length(1)?; let conn_end_on_b = ctx_b.connection_end(&chan_end_on_b.connection_hops()[0])?; - if !conn_end_on_b.state_matches(&ConnectionState::Open) { - return Err(ChannelError::ConnectionNotOpen { - connection_id: chan_end_on_b.connection_hops()[0].clone(), - } - .into()); - } + conn_end_on_b.verify_state_matches(&ConnectionState::Open)?; // Verify proofs { @@ -60,7 +43,7 @@ where let chan_id_on_a = chan_end_on_b .counterparty() .channel_id() - .ok_or(ChannelError::InvalidCounterpartyChannelId)?; + .ok_or(ChannelError::MissingCounterparty)?; let conn_id_on_a = conn_end_on_b.counterparty().connection_id().ok_or( ChannelError::UndefinedConnectionCounterparty { connection_id: chan_end_on_b.connection_hops()[0].clone(), @@ -68,12 +51,12 @@ where )?; let expected_chan_end_on_a = ChannelEnd::new( - State::Open, + ChannelState::Open, *chan_end_on_b.ordering(), Counterparty::new(msg.port_id_on_b.clone(), Some(msg.chan_id_on_b.clone())), vec![conn_id_on_a.clone()], chan_end_on_b.version.clone(), - ); + )?; let chan_end_path_on_a = ChannelEndPath::new(port_id_on_a, chan_id_on_a); // Verify the proof for the channel state against the expected channel end. @@ -151,7 +134,8 @@ mod tests { Counterparty::new(msg.port_id_on_b.clone(), Some(ChannelId::default())), vec![conn_id_on_b.clone()], Version::default(), - ); + ) + .unwrap(); Fixture { context, @@ -205,7 +189,8 @@ mod tests { Counterparty::new(msg.port_id_on_b.clone(), Some(ChannelId::default())), vec![conn_id_on_b.clone()], Version::default(), - ); + ) + .unwrap(); let context = context .with_client(&client_id_on_b, Height::new(0, proof_height).unwrap()) .with_connection(conn_id_on_b, conn_end_on_b) 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 b3e3a81df..57de89a50 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs +++ b/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs @@ -1,6 +1,5 @@ //! Protocol logic specific to ICS4 messages of type `MsgChannelOpenInit`. -use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::msgs::chan_open_init::MsgChannelOpenInit; use crate::prelude::*; @@ -10,17 +9,11 @@ pub fn validate(ctx_a: &Ctx, msg: &MsgChannelOpenInit) -> Result<(), Contex where Ctx: ValidationContext, { - if msg.connection_hops_on_a.len() != 1 { - return Err(ChannelError::InvalidConnectionHopsLength { - expected: 1, - actual: msg.connection_hops_on_a.len(), - } - .into()); - } - // An IBC connection running on the local (host) chain should exist. let conn_end_on_a = ctx_a.connection_end(&msg.connection_hops_on_a[0])?; + // Note: Not needed check if the connection end is OPEN. Optimistic channel handshake is allowed. + let client_id_on_a = conn_end_on_a.client_id(); let client_state_of_b_on_a = ctx_a.client_state(client_id_on_a)?; client_state_of_b_on_a.confirm_not_frozen()?; @@ -108,7 +101,7 @@ mod tests { fn chan_open_init_success_counterparty_chan_id_set(fixture: Fixture) { let Fixture { context, .. } = fixture; - let msg = MsgChannelOpenInit::try_from(get_dummy_raw_msg_chan_open_init(Some(0))).unwrap(); + let msg = MsgChannelOpenInit::try_from(get_dummy_raw_msg_chan_open_init(None)).unwrap(); let res = validate(&context, &msg); 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 5a00c5142..e11a7d7a8 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 @@ -1,7 +1,7 @@ //! Protocol logic specific to ICS4 messages of type `MsgChannelOpenTry`. use crate::core::ics03_connection::connection::State as ConnectionState; -use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State}; +use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, State as ChannelState}; use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::msgs::chan_open_try::MsgChannelOpenTry; use crate::core::ics24_host::path::{ChannelEndPath, ClientConsensusStatePath}; @@ -13,22 +13,12 @@ pub fn validate(ctx_b: &Ctx, msg: &MsgChannelOpenTry) -> Result<(), Context where Ctx: ValidationContext, { - // An IBC connection running on the local (host) chain should exist. - if msg.connection_hops_on_b.len() != 1 { - return Err(ChannelError::InvalidConnectionHopsLength { - expected: 1, - actual: msg.connection_hops_on_b.len(), - }) - .map_err(ContextError::ChannelError); - } + // Note: Verify that the provided message contains only one connection end + // has been done during the conversion from the proto to the domain type. let conn_end_on_b = ctx_b.connection_end(&msg.connection_hops_on_b[0])?; - if !conn_end_on_b.state_matches(&ConnectionState::Open) { - return Err(ChannelError::ConnectionNotOpen { - connection_id: msg.connection_hops_on_b[0].clone(), - }) - .map_err(ContextError::ChannelError); - } + + conn_end_on_b.verify_state_matches(&ConnectionState::Open)?; let conn_version = conn_end_on_b.versions(); @@ -55,12 +45,12 @@ where )?; let expected_chan_end_on_a = ChannelEnd::new( - State::Init, + ChannelState::Init, msg.ordering, Counterparty::new(msg.port_id_on_b.clone(), None), vec![conn_id_on_a.clone()], msg.version_supported_on_a.clone(), - ); + )?; let chan_end_path_on_a = ChannelEndPath::new(&port_id_on_a, &chan_id_on_a); // Verify the proof for the channel state against the expected channel end. 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 e0dc59e70..36dbccb16 100644 --- a/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs +++ b/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs @@ -1,6 +1,6 @@ use crate::core::ics03_connection::connection::State as ConnectionState; use crate::core::ics03_connection::delay::verify_conn_delay_passed; -use crate::core::ics04_channel::channel::{Counterparty, Order, State}; +use crate::core::ics04_channel::channel::{Counterparty, Order, State as ChannelState}; use crate::core::ics04_channel::commitment::compute_packet_commitment; use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::error::PacketError; @@ -21,36 +21,19 @@ where ChannelEndPath::new(&msg.packet.port_id_on_b, &msg.packet.chan_id_on_b); let chan_end_on_b = ctx_b.channel_end(&chan_end_path_on_b)?; - if !chan_end_on_b.state_matches(&State::Open) { - return Err(PacketError::InvalidChannelState { - channel_id: msg.packet.chan_id_on_a.clone(), - state: chan_end_on_b.state, - } - .into()); - } + chan_end_on_b.verify_state_matches(&ChannelState::Open)?; let counterparty = Counterparty::new( msg.packet.port_id_on_a.clone(), Some(msg.packet.chan_id_on_a.clone()), ); - if !chan_end_on_b.counterparty_matches(&counterparty) { - return Err(PacketError::InvalidPacketCounterparty { - port_id: msg.packet.port_id_on_a.clone(), - channel_id: msg.packet.chan_id_on_a.clone(), - } - .into()); - } + chan_end_on_b.verify_counterparty_matches(&counterparty)?; let conn_id_on_b = &chan_end_on_b.connection_hops()[0]; let conn_end_on_b = ctx_b.connection_end(conn_id_on_b)?; - if !conn_end_on_b.state_matches(&ConnectionState::Open) { - return Err(PacketError::ConnectionNotOpen { - connection_id: chan_end_on_b.connection_hops()[0].clone(), - } - .into()); - } + conn_end_on_b.verify_state_matches(&ConnectionState::Open)?; let latest_height = ctx_b.host_height()?; if msg.packet.timeout_height_on_b.has_expired(latest_height) { @@ -217,7 +200,8 @@ mod tests { Counterparty::new(packet.port_id_on_a, Some(packet.chan_id_on_a)), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let conn_end_on_b = ConnectionEnd::new( ConnectionState::Open, 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 6545512fc..76ba7663c 100644 --- a/crates/ibc/src/core/ics04_channel/handler/send_packet.rs +++ b/crates/ibc/src/core/ics04_channel/handler/send_packet.rs @@ -1,5 +1,4 @@ use crate::core::ics04_channel::channel::Counterparty; -use crate::core::ics04_channel::channel::State; use crate::core::ics04_channel::commitment::compute_packet_commitment; use crate::core::ics04_channel::context::SendPacketExecutionContext; use crate::core::ics04_channel::events::SendPacket; @@ -33,26 +32,19 @@ pub fn send_packet_validate( let chan_end_path_on_a = ChannelEndPath::new(&packet.port_id_on_a, &packet.chan_id_on_a); let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?; - if chan_end_on_a.state_matches(&State::Closed) { - return Err(PacketError::ChannelClosed { - channel_id: packet.chan_id_on_a.clone(), - } - .into()); - } + // Checks the channel end not be `Closed`. + // This allows for optimistic packet processing before a channel opens + chan_end_on_a.verify_not_closed()?; let counterparty = Counterparty::new( packet.port_id_on_b.clone(), Some(packet.chan_id_on_b.clone()), ); - if !chan_end_on_a.counterparty_matches(&counterparty) { - return Err(PacketError::InvalidPacketCounterparty { - port_id: packet.port_id_on_b.clone(), - channel_id: packet.chan_id_on_b.clone(), - } - .into()); - } + chan_end_on_a.verify_counterparty_matches(&counterparty)?; + let conn_id_on_a = &chan_end_on_a.connection_hops()[0]; + let conn_end_on_a = ctx_a.connection_end(conn_id_on_a)?; let client_id_on_a = conn_end_on_a.client_id(); @@ -170,12 +162,13 @@ mod tests { let context = MockContext::default(); let chan_end_on_a = ChannelEnd::new( - State::TryOpen, + State::Open, Order::default(), Counterparty::new(PortId::default(), Some(ChannelId::default())), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let conn_end_on_a = ConnectionEnd::new( ConnectionState::Open, diff --git a/crates/ibc/src/core/ics04_channel/handler/timeout.rs b/crates/ibc/src/core/ics04_channel/handler/timeout.rs index 6b5bf2a5a..5a16e4871 100644 --- a/crates/ibc/src/core/ics04_channel/handler/timeout.rs +++ b/crates/ibc/src/core/ics04_channel/handler/timeout.rs @@ -23,25 +23,14 @@ where &msg.packet.chan_id_on_a, ))?; - if !chan_end_on_a.state_matches(&State::Open) { - return Err(PacketError::ChannelClosed { - channel_id: msg.packet.chan_id_on_a.clone(), - } - .into()); - } + chan_end_on_a.verify_state_matches(&State::Open)?; let counterparty = Counterparty::new( msg.packet.port_id_on_b.clone(), Some(msg.packet.chan_id_on_b.clone()), ); - if !chan_end_on_a.counterparty_matches(&counterparty) { - return Err(PacketError::InvalidPacketCounterparty { - port_id: msg.packet.port_id_on_b.clone(), - channel_id: msg.packet.chan_id_on_b.clone(), - } - .into()); - } + chan_end_on_a.verify_counterparty_matches(&counterparty)?; let conn_id_on_a = chan_end_on_a.connection_hops()[0].clone(); let conn_end_on_a = ctx_a.connection_end(&conn_id_on_a)?; @@ -211,7 +200,8 @@ mod tests { Counterparty::new(packet.port_id_on_b.clone(), Some(packet.chan_id_on_b)), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let mut chan_end_on_a_ordered = chan_end_on_a_unordered.clone(); chan_end_on_a_ordered.ordering = Order::Ordered; 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 fe8f66525..833ae9d24 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 @@ -26,13 +26,7 @@ where Some(packet.chan_id_on_b.clone()), ); - if !chan_end_on_a.counterparty_matches(&counterparty) { - return Err(PacketError::InvalidPacketCounterparty { - port_id: packet.port_id_on_b.clone(), - channel_id: packet.chan_id_on_b.clone(), - } - .into()); - } + chan_end_on_a.verify_counterparty_matches(&counterparty)?; let commitment_path_on_a = CommitmentPath::new( &msg.packet.port_id_on_a, @@ -79,13 +73,10 @@ where let consensus_state_of_b_on_a = ctx_a.consensus_state(&client_cons_state_path_on_a)?; let prefix_on_b = conn_end_on_a.counterparty().prefix(); let port_id_on_b = chan_end_on_a.counterparty().port_id.clone(); - let chan_id_on_b = - chan_end_on_a - .counterparty() - .channel_id() - .ok_or(PacketError::Channel( - ChannelError::InvalidCounterpartyChannelId, - ))?; + let chan_id_on_b = chan_end_on_a + .counterparty() + .channel_id() + .ok_or(PacketError::Channel(ChannelError::MissingCounterparty))?; let conn_id_on_b = conn_end_on_a.counterparty().connection_id().ok_or( PacketError::UndefinedConnectionCounterparty { connection_id: chan_end_on_a.connection_hops()[0].clone(), @@ -102,7 +93,7 @@ where expected_counterparty, expected_conn_hops_on_b, chan_end_on_a.version().clone(), - ); + )?; let chan_end_path_on_b = ChannelEndPath(port_id_on_b, chan_id_on_b.clone()); @@ -229,7 +220,8 @@ mod tests { Counterparty::new(packet.port_id_on_b.clone(), Some(packet.chan_id_on_b)), vec![ConnectionId::default()], Version::new("ics20-1".to_string()), - ); + ) + .unwrap(); let conn_end_on_a = ConnectionEnd::new( ConnectionState::Open, diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_close_confirm.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_close_confirm.rs index 3ac3a36b8..4765c4864 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_close_confirm.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_close_confirm.rs @@ -41,11 +41,8 @@ impl TryFrom for MsgChannelCloseConfirm { fn try_from(raw_msg: RawMsgChannelCloseConfirm) -> Result { Ok(MsgChannelCloseConfirm { - port_id_on_b: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, - chan_id_on_b: raw_msg - .channel_id - .parse() - .map_err(ChannelError::Identifier)?, + port_id_on_b: raw_msg.port_id.parse()?, + chan_id_on_b: raw_msg.channel_id.parse()?, proof_chan_end_on_a: raw_msg .proof_init .try_into() diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs index 34e575710..6673ae7ec 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs @@ -37,11 +37,8 @@ impl TryFrom for MsgChannelCloseInit { fn try_from(raw_msg: RawMsgChannelCloseInit) -> Result { Ok(MsgChannelCloseInit { - port_id_on_a: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, - chan_id_on_a: raw_msg - .channel_id - .parse() - .map_err(ChannelError::Identifier)?, + port_id_on_a: raw_msg.port_id.parse()?, + chan_id_on_a: raw_msg.channel_id.parse()?, signer: raw_msg.signer.parse().map_err(ChannelError::Signer)?, }) } diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_ack.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_ack.rs index 97dc89bb2..34ddfdf92 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_ack.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_ack.rs @@ -41,15 +41,9 @@ impl TryFrom for MsgChannelOpenAck { fn try_from(raw_msg: RawMsgChannelOpenAck) -> Result { Ok(MsgChannelOpenAck { - port_id_on_a: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, - chan_id_on_a: raw_msg - .channel_id - .parse() - .map_err(ChannelError::Identifier)?, - chan_id_on_b: raw_msg - .counterparty_channel_id - .parse() - .map_err(ChannelError::Identifier)?, + port_id_on_a: raw_msg.port_id.parse()?, + chan_id_on_a: raw_msg.channel_id.parse()?, + chan_id_on_b: raw_msg.counterparty_channel_id.parse()?, version_on_b: raw_msg.counterparty_version.into(), proof_chan_end_on_b: raw_msg .proof_try diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_confirm.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_confirm.rs index b74309912..dcc5e445c 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_confirm.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_confirm.rs @@ -39,11 +39,8 @@ impl TryFrom for MsgChannelOpenConfirm { fn try_from(raw_msg: RawMsgChannelOpenConfirm) -> Result { Ok(MsgChannelOpenConfirm { - port_id_on_b: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, - chan_id_on_b: raw_msg - .channel_id - .parse() - .map_err(ChannelError::Identifier)?, + port_id_on_b: raw_msg.port_id.parse()?, + chan_id_on_b: raw_msg.channel_id.parse()?, proof_chan_end_on_a: raw_msg .proof_ack .try_into() diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs index e8c39b2c1..04dd76b3e 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs @@ -46,8 +46,11 @@ impl TryFrom for MsgChannelOpenInit { .channel .ok_or(ChannelError::MissingChannel)? .try_into()?; + chan_end_on_a.verify_state_matches(&State::Init)?; + chan_end_on_a.verify_empty_counterparty_channel_id()?; + Ok(MsgChannelOpenInit { - port_id_on_a: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, + port_id_on_a: raw_msg.port_id.parse()?, connection_hops_on_a: chan_end_on_a.connection_hops, port_id_on_b: chan_end_on_a.remote.port_id, ordering: chan_end_on_a.ordering, @@ -56,9 +59,10 @@ impl TryFrom for MsgChannelOpenInit { }) } } + impl From for RawMsgChannelOpenInit { fn from(domain_msg: MsgChannelOpenInit) -> Self { - let chan_end_on_a = ChannelEnd::new( + let chan_end_on_a = ChannelEnd::new_unchecked( State::Init, domain_msg.ordering, Counterparty::new(domain_msg.port_id_on_b, None), @@ -88,7 +92,7 @@ pub mod test_util { ) -> RawMsgChannelOpenInit { RawMsgChannelOpenInit { port_id: PortId::default().to_string(), - channel: Some(get_dummy_raw_channel_end(counterparty_channel_id)), + channel: Some(get_dummy_raw_channel_end(1, counterparty_channel_id)), signer: get_dummy_bech32_account(), } } diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs index c63c7cfff..05d8904ef 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs @@ -56,9 +56,20 @@ impl TryFrom for MsgChannelOpenTry { .channel .ok_or(ChannelError::MissingChannel)? .try_into()?; + + chan_end_on_b.verify_state_matches(&State::TryOpen)?; + + #[allow(deprecated)] + if !raw_msg.previous_channel_id.is_empty() { + return Err(ChannelError::InvalidChannelId { + expected: "previous channel id must be empty. It has been deprecated as crossing hellos are no longer supported".to_string(), + actual: raw_msg.previous_channel_id, + }); + } + #[allow(deprecated)] let msg = MsgChannelOpenTry { - port_id_on_b: raw_msg.port_id.parse().map_err(ChannelError::Identifier)?, + port_id_on_b: raw_msg.port_id.parse()?, ordering: chan_end_on_b.ordering, previous_channel_id: raw_msg.previous_channel_id, connection_hops_on_b: chan_end_on_b.connection_hops, @@ -66,7 +77,7 @@ impl TryFrom for MsgChannelOpenTry { chan_id_on_a: chan_end_on_b .remote .channel_id - .ok_or(ChannelError::InvalidCounterpartyChannelId)?, + .ok_or(ChannelError::MissingCounterparty)?, version_supported_on_a: raw_msg.counterparty_version.into(), proof_chan_end_on_a: raw_msg .proof_init @@ -86,8 +97,8 @@ impl TryFrom for MsgChannelOpenTry { impl From for RawMsgChannelOpenTry { fn from(domain_msg: MsgChannelOpenTry) -> Self { - let chan_end_on_b = ChannelEnd::new( - State::Init, + let chan_end_on_b = ChannelEnd::new_unchecked( + State::TryOpen, domain_msg.ordering, Counterparty::new(domain_msg.port_id_on_a, Some(domain_msg.chan_id_on_a)), domain_msg.connection_hops_on_b, @@ -112,7 +123,7 @@ pub mod test_util { use ibc_proto::ibc::core::channel::v1::MsgChannelOpenTry as RawMsgChannelOpenTry; use crate::core::ics04_channel::channel::test_util::get_dummy_raw_channel_end; - use crate::core::ics24_host::identifier::{ChannelId, PortId}; + use crate::core::ics24_host::identifier::PortId; use crate::test_utils::{get_dummy_bech32_account, get_dummy_proof}; use ibc_proto::ibc::core::client::v1::Height; @@ -121,8 +132,8 @@ pub mod test_util { #[allow(deprecated)] RawMsgChannelOpenTry { port_id: PortId::default().to_string(), - previous_channel_id: ChannelId::default().to_string(), - channel: Some(get_dummy_raw_channel_end(Some(0))), + previous_channel_id: "".to_string(), + channel: Some(get_dummy_raw_channel_end(2, Some(0))), counterparty_version: "".to_string(), proof_init: get_dummy_proof(), proof_height: Some(Height { diff --git a/crates/ibc/src/core/ics04_channel/msgs/timeout.rs b/crates/ibc/src/core/ics04_channel/msgs/timeout.rs index 368832ab9..d0326df6d 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/timeout.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/timeout.rs @@ -40,7 +40,9 @@ impl TryFrom for MsgTimeout { type Error = PacketError; fn try_from(raw_msg: RawMsgTimeout) -> Result { - // TODO: Domain type verification for the next sequence: this should probably be > 0. + if raw_msg.next_sequence_recv == 0 { + return Err(PacketError::ZeroPacketSequence); + } Ok(MsgTimeout { packet: raw_msg .packet diff --git a/crates/ibc/src/core/ics04_channel/msgs/timeout_on_close.rs b/crates/ibc/src/core/ics04_channel/msgs/timeout_on_close.rs index 8158ed397..90d22005d 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/timeout_on_close.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/timeout_on_close.rs @@ -39,7 +39,10 @@ impl TryFrom for MsgTimeoutOnClose { type Error = PacketError; fn try_from(raw_msg: RawMsgTimeoutOnClose) -> Result { - // TODO: Domain type verification for the next sequence: this should probably be > 0. + if raw_msg.next_sequence_recv == 0 { + return Err(PacketError::ZeroPacketSequence); + } + Ok(MsgTimeoutOnClose { packet: raw_msg .packet diff --git a/crates/ibc/src/core/ics04_channel/packet.rs b/crates/ibc/src/core/ics04_channel/packet.rs index 33ea4e4a2..7ed61c226 100644 --- a/crates/ibc/src/core/ics04_channel/packet.rs +++ b/crates/ibc/src/core/ics04_channel/packet.rs @@ -193,7 +193,7 @@ impl Packet { pub fn timed_out(&self, dst_chain_ts: &Timestamp, dst_chain_height: Height) -> bool { let height_timed_out = self.timeout_height_on_b.has_expired(dst_chain_height); - let timestamp_timed_out = self.timeout_timestamp_on_b != Timestamp::none() + let timestamp_timed_out = self.timeout_timestamp_on_b.is_set() && dst_chain_ts.check_expiry(&self.timeout_timestamp_on_b) == Expired; height_timed_out || timestamp_timed_out @@ -225,6 +225,10 @@ impl TryFrom for Packet { return Err(PacketError::ZeroPacketSequence); } + if raw_pkt.data.is_empty() { + return Err(PacketError::ZeroPacketData); + } + // Note: ibc-go currently (July 2022) incorrectly treats the timeout // heights `{revision_number : >0, revision_height: 0}` as valid // timeouts. However, heights with `revision_height == 0` are invalid in @@ -238,31 +242,20 @@ impl TryFrom for Packet { .try_into() .map_err(|_| PacketError::InvalidTimeoutHeight)?; - if raw_pkt.data.is_empty() { - return Err(PacketError::ZeroPacketData); - } - let timeout_timestamp_on_b = Timestamp::from_nanoseconds(raw_pkt.timeout_timestamp) .map_err(PacketError::InvalidPacketTimestamp)?; + // Packet timeout height and packet timeout timestamp cannot both be unset. + if !packet_timeout_height.is_set() && !timeout_timestamp_on_b.is_set() { + return Err(PacketError::MissingTimeout); + } + Ok(Packet { seq_on_a: Sequence::from(raw_pkt.sequence), - port_id_on_a: raw_pkt - .source_port - .parse() - .map_err(PacketError::Identifier)?, - chan_id_on_a: raw_pkt - .source_channel - .parse() - .map_err(PacketError::Identifier)?, - port_id_on_b: raw_pkt - .destination_port - .parse() - .map_err(PacketError::Identifier)?, - chan_id_on_b: raw_pkt - .destination_channel - .parse() - .map_err(PacketError::Identifier)?, + port_id_on_a: raw_pkt.source_port.parse()?, + chan_id_on_a: raw_pkt.source_channel.parse()?, + port_id_on_b: raw_pkt.destination_port.parse()?, + chan_id_on_b: raw_pkt.destination_channel.parse()?, data: raw_pkt.data, timeout_height_on_b: packet_timeout_height, timeout_timestamp_on_b, @@ -332,10 +325,10 @@ mod tests { } let proof_height = 10; - let default_raw_packet = get_dummy_raw_packet(proof_height, 0); - let raw_packet_no_timeout_or_timestamp = get_dummy_raw_packet(0, 0); + let default_raw_packet = get_dummy_raw_packet(proof_height, 1000); + let raw_packet_no_timeout_or_timestamp = get_dummy_raw_packet(10, 0); - let mut raw_packet_invalid_timeout_height = get_dummy_raw_packet(0, 0); + let mut raw_packet_invalid_timeout_height = get_dummy_raw_packet(0, 10); raw_packet_invalid_timeout_height.timeout_height = Some(RawHeight { revision_number: 1, revision_height: 0, @@ -351,7 +344,7 @@ mod tests { // Note: ibc-go currently (July 2022) incorrectly rejects this // case, even though it is allowed in ICS-4. name: "Packet with no timeout of timestamp".to_string(), - raw: raw_packet_no_timeout_or_timestamp, + raw: raw_packet_no_timeout_or_timestamp.clone(), want_pass: true, }, Test { @@ -470,6 +463,14 @@ mod tests { }, want_pass: true, }, + Test { + name: "Missing both timeout height and timestamp".to_string(), + raw: RawPacket { + timeout_height: None, + ..raw_packet_no_timeout_or_timestamp + }, + want_pass: false, + } ]; for test in tests { diff --git a/crates/ibc/src/core/ics04_channel/timeout.rs b/crates/ibc/src/core/ics04_channel/timeout.rs index 5dac73e38..aba55f99f 100644 --- a/crates/ibc/src/core/ics04_channel/timeout.rs +++ b/crates/ibc/src/core/ics04_channel/timeout.rs @@ -34,6 +34,14 @@ pub enum TimeoutHeight { } impl TimeoutHeight { + /// Returns if the timeout height is set. + pub fn is_set(&self) -> bool { + match self { + TimeoutHeight::At(_) => true, + TimeoutHeight::Never => false, + } + } + pub fn no_timeout() -> Self { Self::Never } diff --git a/crates/ibc/src/core/ics04_channel/version.rs b/crates/ibc/src/core/ics04_channel/version.rs index 026175359..dd3e39bf7 100644 --- a/crates/ibc/src/core/ics04_channel/version.rs +++ b/crates/ibc/src/core/ics04_channel/version.rs @@ -48,11 +48,11 @@ impl Version { &self.0 } - pub fn verify_is_expected(&self, expected_version: Version) -> Result<(), ChannelError> { - if self != &expected_version { + pub fn verify_is_expected(&self, expected: Version) -> Result<(), ChannelError> { + if self != &expected { return Err(ChannelError::VersionNotSupported { - expected_version, - got_version: self.clone(), + expected, + actual: self.clone(), }); } Ok(()) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index f5ba46cd3..c443e3a7b 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -11,6 +11,13 @@ use crate::core::ics24_host::validate::validate_client_identifier; use crate::core::ics24_host::error::ValidationError; use crate::prelude::*; +const CONNECTION_ID_PREFIX: &str = "connection"; +const CHANNEL_ID_PREFIX: &str = "channel"; + +const DEFAULT_CHAIN_ID: &str = "defaultChainId"; +const DEFAULT_PORT_ID: &str = "defaultPort"; +const TRANSFER_PORT_ID: &str = "transfer"; + /// This type is subject to future changes. /// /// TODO: ChainId validation is not standardized yet. @@ -131,7 +138,9 @@ impl ChainId { self.id = { let mut split: Vec<&str> = self.id.split('-').collect(); let version = version.to_string(); - *split.last_mut().unwrap() = &version; + if let Some(last_elem) = split.last_mut() { + *last_elem = &version; + } split.join("-") }; self.version = version; @@ -162,13 +171,13 @@ impl From for tendermint::chain::Id { impl From for ChainId { fn from(id: tendermint::chain::Id) -> Self { - ChainId::from_str(id.as_str()).unwrap() + ChainId::from(id.to_string()) } } impl Default for ChainId { fn default() -> Self { - "defaultChainId".to_string().parse().unwrap() + Self::from_string(DEFAULT_CHAIN_ID) } } @@ -329,12 +338,12 @@ impl ConnectionId { /// ``` pub fn new(identifier: u64) -> Self { let id = format!("{}-{}", Self::prefix(), identifier); - Self::from_str(id.as_str()).unwrap() + Self(id) } /// Returns the static prefix to be used across all connection identifiers. pub fn prefix() -> &'static str { - "connection" + CONNECTION_ID_PREFIX } /// Get this identifier as a borrowed `&str` @@ -400,9 +409,13 @@ impl PartialEq for ConnectionId { pub struct PortId(String); impl PortId { + pub fn new(id: String) -> Result { + Self::from_str(&id) + } + /// Infallible creation of the well-known transfer port pub fn transfer() -> Self { - Self("transfer".to_string()) + Self(TRANSFER_PORT_ID.to_string()) } /// Get this identifier as a borrowed `&str` @@ -414,6 +427,10 @@ impl PortId { pub fn as_bytes(&self) -> &[u8] { self.0.as_bytes() } + + pub fn validate(&self) -> Result<(), ValidationError> { + validate_port_identifier(self.as_str()) + } } /// This implementation provides a `to_string` method. @@ -439,7 +456,7 @@ impl AsRef for PortId { impl Default for PortId { fn default() -> Self { - "defaultPort".to_string().parse().unwrap() + Self(DEFAULT_PORT_ID.to_string()) } } @@ -460,8 +477,6 @@ impl Default for PortId { pub struct ChannelId(String); impl ChannelId { - const PREFIX: &'static str = "channel-"; - /// Builds a new channel identifier. Like client and connection identifiers, channel ids are /// deterministically formed from two elements: a prefix `prefix`, and a monotonically /// increasing `counter`, separated by a dash "-". @@ -474,10 +489,15 @@ impl ChannelId { /// assert_eq!(chan_id.to_string(), "channel-27"); /// ``` pub fn new(identifier: u64) -> Self { - let id = format!("{}{}", Self::PREFIX, identifier); + let id = format!("{}-{}", Self::prefix(), identifier); Self(id) } + /// Returns the static prefix to be used across all channel identifiers. + pub fn prefix() -> &'static str { + CHANNEL_ID_PREFIX + } + /// Get this identifier as a borrowed `&str` pub fn as_str(&self) -> &str { &self.0 @@ -529,38 +549,3 @@ impl PartialEq for ChannelId { self.as_str().eq(other) } } - -#[cfg_attr( - feature = "parity-scale-codec", - derive( - parity_scale_codec::Encode, - parity_scale_codec::Decode, - scale_info::TypeInfo - ) -)] -#[cfg_attr( - feature = "borsh", - derive(borsh::BorshSerialize, borsh::BorshDeserialize) -)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -/// A pair of [`PortId`] and [`ChannelId`] are used together for sending IBC packets. -#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct PortChannelId { - pub channel_id: ChannelId, - pub port_id: PortId, -} - -impl PortChannelId { - pub fn new(channel_id: ChannelId, port_id: PortId) -> Self { - Self { - channel_id, - port_id, - } - } -} - -impl Display for PortChannelId { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { - write!(f, "{}/{}", self.port_id, self.channel_id) - } -} diff --git a/crates/ibc/src/core/ics24_host/validate.rs b/crates/ibc/src/core/ics24_host/validate.rs index 27e11216c..b1be757c1 100644 --- a/crates/ibc/src/core/ics24_host/validate.rs +++ b/crates/ibc/src/core/ics24_host/validate.rs @@ -120,7 +120,15 @@ pub fn validate_client_identifier(id: &str) -> Result<(), Error> { /// A valid connection identifier must be between 10-64 characters as specified /// in the ICS-24 spec. pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { - validate_default_identifier(id, 10, 64) + validate_identifier(id, 10, 64) +} + +/// Default validator function for Channel identifiers. +/// +/// A valid channel identifier must be between 8-64 characters as specified in +/// the ICS-24 spec. +pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { + validate_identifier(id, 8, 64) } /// Default validator function for Port identifiers. @@ -131,14 +139,6 @@ pub fn validate_port_identifier(id: &str) -> Result<(), Error> { validate_default_identifier(id, 2, 128) } -/// Default validator function for Channel identifiers. -/// -/// A valid channel identifier must be between 8-64 characters as specified in -/// the ICS-24 spec. -pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { - validate_default_identifier(id, 8, 64) -} - #[cfg(test)] mod tests { use crate::core::ics24_host::validate::{ diff --git a/crates/ibc/src/timestamp.rs b/crates/ibc/src/timestamp.rs index db6bf9f3d..b30f4a46c 100644 --- a/crates/ibc/src/timestamp.rs +++ b/crates/ibc/src/timestamp.rs @@ -221,6 +221,11 @@ impl Timestamp { _ => Expiry::InvalidTimestamp, } } + + /// Checks whether the timestamp is set. + pub fn is_set(&self) -> bool { + self.time.is_some() + } } impl Display for Timestamp { From 4af0a70fe860b6b08555c54970cb69c1a209305f Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 26 Apr 2023 10:52:17 -0700 Subject: [PATCH 14/21] Adjusted counterpary init Conn/Chan id checks --- .../src/core/ics03_connection/connection.rs | 8 +++++-- .../handler/conn_open_init.rs | 3 +-- .../ics03_connection/handler/conn_open_try.rs | 3 +-- .../ics03_connection/msgs/conn_open_init.rs | 12 ++++++---- crates/ibc/src/core/ics04_channel/channel.rs | 22 ++++++++++--------- .../core/ics04_channel/msgs/chan_open_init.rs | 2 +- 6 files changed, 29 insertions(+), 21 deletions(-) diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs index c40540085..04b46c1f8 100644 --- a/crates/ibc/src/core/ics03_connection/connection.rs +++ b/crates/ibc/src/core/ics03_connection/connection.rs @@ -16,7 +16,6 @@ use crate::core::ics02_client::error::ClientError; use crate::core::ics03_connection::error::ConnectionError; use crate::core::ics03_connection::version::Version; use crate::core::ics23_commitment::commitment::CommitmentPrefix; -use crate::core::ics24_host::error::ValidationError; use crate::core::ics24_host::identifier::{ClientId, ConnectionId}; use crate::timestamp::ZERO_DURATION; @@ -468,7 +467,12 @@ impl Counterparty { &self.prefix } - pub fn validate_basic(&self) -> Result<(), ValidationError> { + /// Called upon initiating a connection handshake on the host chain to verify + /// that the counterparty connection id has not been set. + pub(crate) fn verify_empty_connection_id(&self) -> Result<(), ConnectionError> { + if self.connection_id().is_some() { + return Err(ConnectionError::InvalidCounterparty); + } Ok(()) } } 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 61661a1f0..612201610 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,8 +46,7 @@ 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 d7d8b1c4e..e4cb545c2 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 @@ -186,8 +186,7 @@ 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/msgs/conn_open_init.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_init.rs index 56270111e..3cfc0abf4 100644 --- a/crates/ibc/src/core/ics03_connection/msgs/conn_open_init.rs +++ b/crates/ibc/src/core/ics03_connection/msgs/conn_open_init.rs @@ -40,15 +40,19 @@ impl TryFrom for MsgConnectionOpenInit { type Error = ConnectionError; fn try_from(msg: RawMsgConnectionOpenInit) -> Result { + let counterparty: Counterparty = msg + .counterparty + .ok_or(ConnectionError::MissingCounterparty)? + .try_into()?; + + counterparty.verify_empty_connection_id()?; + Ok(Self { client_id_on_a: msg .client_id .parse() .map_err(ConnectionError::InvalidIdentifier)?, - counterparty: msg - .counterparty - .ok_or(ConnectionError::MissingCounterparty)? - .try_into()?, + counterparty, version: msg.version.map(|version| version.try_into()).transpose()?, delay_period: Duration::from_nanos(msg.delay_period), signer: msg.signer.parse().map_err(ConnectionError::Signer)?, diff --git a/crates/ibc/src/core/ics04_channel/channel.rs b/crates/ibc/src/core/ics04_channel/channel.rs index a195a0f54..f11ba1a08 100644 --- a/crates/ibc/src/core/ics04_channel/channel.rs +++ b/crates/ibc/src/core/ics04_channel/channel.rs @@ -309,16 +309,6 @@ impl ChannelEnd { Ok(()) } - pub(crate) fn verify_empty_counterparty_channel_id(&self) -> Result<(), ChannelError> { - if !self.counterparty().channel_id.eq(&None) { - return Err(ChannelError::InvalidChannelId { - expected: "Counterparty channel id must be empty".to_string(), - actual: format!("{:?}", self.counterparty().channel_id), - }); - } - Ok(()) - } - pub fn version_matches(&self, other: &Version) -> bool { self.version().eq(other) } @@ -367,6 +357,18 @@ impl Counterparty { pub fn channel_id(&self) -> Option<&ChannelId> { self.channel_id.as_ref() } + + /// Called upon initiating a channel handshake on the host chain to verify + /// that the counterparty channel id has not been set. + pub(crate) fn verify_empty_channel_id(&self) -> Result<(), ChannelError> { + if self.channel_id().is_some() { + return Err(ChannelError::InvalidChannelId { + expected: "Counterparty channel id must be empty".to_string(), + actual: format!("{:?}", self.channel_id), + }); + } + Ok(()) + } } impl Display for Counterparty { diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs index 04dd76b3e..d134697d0 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs @@ -47,7 +47,7 @@ impl TryFrom for MsgChannelOpenInit { .ok_or(ChannelError::MissingChannel)? .try_into()?; chan_end_on_a.verify_state_matches(&State::Init)?; - chan_end_on_a.verify_empty_counterparty_channel_id()?; + chan_end_on_a.counterparty().verify_empty_channel_id()?; Ok(MsgChannelOpenInit { port_id_on_a: raw_msg.port_id.parse()?, From 655815d4764b965c1b98e4e1a88f9d0b5bd90120 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 1 May 2023 17:47:57 -0700 Subject: [PATCH 15/21] Fix some nits --- crates/ibc/src/core/context/chan_open_init.rs | 3 +-- crates/ibc/src/core/context/chan_open_try.rs | 3 +-- .../core/ics02_client/handler/upgrade_client.rs | 2 +- .../src/core/ics02_client/msgs/update_client.rs | 2 +- .../src/core/ics02_client/msgs/upgrade_client.rs | 2 +- crates/ibc/src/core/ics03_connection/events.rs | 1 + .../ics03_connection/handler/conn_open_ack.rs | 2 +- .../handler/conn_open_confirm.rs | 2 +- .../ics03_connection/msgs/conn_open_confirm.rs | 2 +- .../core/ics03_connection/msgs/conn_open_try.rs | 2 +- .../core/ics04_channel/msgs/chan_close_init.rs | 10 ++-------- crates/ibc/src/core/ics24_host/identifier.rs | 4 ++-- crates/ibc/src/core/ics24_host/validate.rs | 16 ++++++++-------- 13 files changed, 22 insertions(+), 29 deletions(-) diff --git a/crates/ibc/src/core/context/chan_open_init.rs b/crates/ibc/src/core/context/chan_open_init.rs index cdb3d7dea..10f4f0dbd 100644 --- a/crates/ibc/src/core/context/chan_open_init.rs +++ b/crates/ibc/src/core/context/chan_open_init.rs @@ -70,8 +70,7 @@ where Counterparty::new(msg.port_id_on_b.clone(), None), msg.connection_hops_on_a.clone(), msg.version_proposal.clone(), - ) - .unwrap(); + )?; let chan_end_path_on_a = ChannelEndPath::new(&msg.port_id_on_a, &chan_id_on_a); ctx_a.store_channel(&chan_end_path_on_a, chan_end_on_a)?; diff --git a/crates/ibc/src/core/context/chan_open_try.rs b/crates/ibc/src/core/context/chan_open_try.rs index df8755356..0e3af389b 100644 --- a/crates/ibc/src/core/context/chan_open_try.rs +++ b/crates/ibc/src/core/context/chan_open_try.rs @@ -70,8 +70,7 @@ where Counterparty::new(msg.port_id_on_a.clone(), Some(msg.chan_id_on_a.clone())), msg.connection_hops_on_b.clone(), version.clone(), - ) - .unwrap(); + )?; let chan_end_path_on_b = ChannelEndPath::new(&msg.port_id_on_b, &chan_id_on_b); ctx_b.store_channel(&chan_end_path_on_b, chan_end_on_b)?; diff --git a/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs b/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs index 96c5636b3..cd437a735 100644 --- a/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs @@ -195,7 +195,7 @@ mod tests { fn upgrade_client_fail_nonexisting_client(fixture: Fixture) { let Fixture { ctx, mut msg } = fixture; - msg.client_id = ClientId::from_str("nonexistingclient-0").unwrap(); + msg.client_id = ClientId::from_str("nonexistingclient").unwrap(); let res = validate(&ctx, msg); assert!( diff --git a/crates/ibc/src/core/ics02_client/msgs/update_client.rs b/crates/ibc/src/core/ics02_client/msgs/update_client.rs index a69047aea..59cf5e7ea 100644 --- a/crates/ibc/src/core/ics02_client/msgs/update_client.rs +++ b/crates/ibc/src/core/ics02_client/msgs/update_client.rs @@ -86,7 +86,7 @@ mod tests { #[test] fn msg_update_client_serialization() { - let client_id: ClientId = "tendermint-0".parse().unwrap(); + let client_id: ClientId = "tendermint".parse().unwrap(); let signer = get_dummy_account_id(); let header = get_dummy_ics07_header(); diff --git a/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs b/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs index 1580000da..c8bffe20e 100644 --- a/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs @@ -174,7 +174,7 @@ mod tests { #[test] fn msg_upgrade_client_serialization() { - let client_id: ClientId = "tendermint-0".parse().unwrap(); + let client_id: ClientId = "tendermint".parse().unwrap(); let signer = get_dummy_account_id(); let height = Height::new(1, 1).unwrap(); diff --git a/crates/ibc/src/core/ics03_connection/events.rs b/crates/ibc/src/core/ics03_connection/events.rs index 75b28f593..01e8be2d5 100644 --- a/crates/ibc/src/core/ics03_connection/events.rs +++ b/crates/ibc/src/core/ics03_connection/events.rs @@ -305,6 +305,7 @@ impl From for abci::Event { #[cfg(test)] mod tests { + use super::*; use crate::core::ics02_client::client_type::ClientType; use tendermint::abci::Event as AbciEvent; 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 871e79212..3a3422bb2 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 @@ -227,7 +227,7 @@ mod tests { let msg = MsgConnectionOpenAck::new_dummy(10, 10); // Client parameters -- identifier and correct height (matching the proof height) - let client_id = ClientId::from_str("mock_clientid-0").unwrap(); + let client_id = ClientId::from_str("mock_clientid").unwrap(); let proof_height = msg.proofs_height_on_b; let conn_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 eb2d2b3f4..ad4cff038 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 @@ -186,7 +186,7 @@ mod tests { } fn conn_open_confirm_fixture(ctx: Ctx) -> Fixture { - let client_id = ClientId::from_str("mock_clientid-0").unwrap(); + let client_id = ClientId::from_str("mock_clientid").unwrap(); let msg = MsgConnectionOpenConfirm::new_dummy(); let counterparty = Counterparty::new( client_id.clone(), diff --git a/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs index 18ab53cbd..714884030 100644 --- a/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs +++ b/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs @@ -87,7 +87,7 @@ pub mod test_util { pub fn get_dummy_raw_msg_conn_open_confirm() -> RawMsgConnectionOpenConfirm { RawMsgConnectionOpenConfirm { - connection_id: "src-connection-0".to_string(), + connection_id: "srcconnection".to_string(), proof_ack: get_dummy_proof(), proof_height: Some(Height { revision_number: 0, diff --git a/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs index df2644e28..2be603e32 100644 --- a/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs +++ b/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs @@ -266,7 +266,7 @@ mod tests { .to_string(), raw: RawMsgConnectionOpenTry { counterparty: Some(RawCounterparty { - client_id: "ClientId-0".to_string(), + client_id: "ClientId_".to_string(), ..get_dummy_raw_counterparty(Some(0)) }), ..default_try_msg.clone() diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs index a9e1d8e85..28be8f840 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_close_init.rs @@ -37,14 +37,8 @@ impl TryFrom for MsgChannelCloseInit { fn try_from(raw_msg: RawMsgChannelCloseInit) -> Result { Ok(MsgChannelCloseInit { - port_id_on_a: raw_msg - .port_id - .parse() - .map_err(ChannelError::InvalidIdentifier)?, - chan_id_on_a: raw_msg - .channel_id - .parse() - .map_err(ChannelError::InvalidIdentifier)?, + port_id_on_a: raw_msg.port_id.parse()?, + chan_id_on_a: raw_msg.channel_id.parse()?, signer: raw_msg.signer.into(), }) } diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index f3a5f4145..ee4402377 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -259,9 +259,9 @@ impl Default for ClientId { /// ``` /// use core::str::FromStr; /// use ibc::core::ics24_host::identifier::ClientId; -/// let client_id = ClientId::from_str("clientid-0"); +/// let client_id = ClientId::from_str("clientidtwo"); /// assert!(client_id.is_ok()); -/// client_id.map(|id| {assert_eq!(&id, "clientid-0")}); +/// client_id.map(|id| {assert_eq!(&id, "clientidtwo")}); /// ``` impl PartialEq for ClientId { fn eq(&self, other: &str) -> bool { diff --git a/crates/ibc/src/core/ics24_host/validate.rs b/crates/ibc/src/core/ics24_host/validate.rs index 15ae62ed2..c5c8c1641 100644 --- a/crates/ibc/src/core/ics24_host/validate.rs +++ b/crates/ibc/src/core/ics24_host/validate.rs @@ -99,14 +99,6 @@ pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { validate_identifier(id, 10, 64) } -/// Default validator function for Channel identifiers. -/// -/// A valid channel identifier must be between 8-64 characters as specified in -/// the ICS-24 spec. -pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { - validate_identifier(id, 8, 64) -} - /// Default validator function for Port identifiers. /// /// A valid port identifier must be between 2-128 characters as specified in the @@ -115,6 +107,14 @@ pub fn validate_port_identifier(id: &str) -> Result<(), Error> { validate_identifier(id, 2, 128) } +/// Default validator function for Channel identifiers. +/// +/// A valid channel identifier must be between 8-64 characters as specified in +/// the ICS-24 spec. +pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { + validate_identifier(id, 8, 64) +} + #[cfg(test)] mod tests { use crate::core::ics24_host::validate::{ From 5609fd170c597251b36185b11e5043cf235f8fae Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 5 May 2023 10:24:15 -0400 Subject: [PATCH 16/21] name changes --- .../clients/ics07_tendermint/client_state/update_client.rs | 2 +- crates/ibc/src/clients/ics07_tendermint/header.rs | 2 +- crates/ibc/src/core/ics04_channel/channel.rs | 4 ++-- crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs | 2 +- crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs b/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs index 946406ac0..8001f2461 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs @@ -25,7 +25,7 @@ impl ClientState { // The tendermint-light-client crate though works on heights that are assumed // to have the same revision number. We ensure this here. - header.verify_chain_id_matches(&self.chain_id())?; + header.verify_chain_id_version_matches_height(&self.chain_id())?; // Delegate to tendermint-light-client, which contains the required checks // of the new header against the trusted consensus state. diff --git a/crates/ibc/src/clients/ics07_tendermint/header.rs b/crates/ibc/src/clients/ics07_tendermint/header.rs index 0eb4d4bd2..047552381 100644 --- a/crates/ibc/src/clients/ics07_tendermint/header.rs +++ b/crates/ibc/src/clients/ics07_tendermint/header.rs @@ -80,7 +80,7 @@ impl Header { }) } - pub fn verify_chain_id_matches(&self, chain_id: &ChainId) -> Result<(), Error> { + pub fn verify_chain_id_version_matches_height(&self, chain_id: &ChainId) -> Result<(), Error> { if self.height().revision_number() != chain_id.version() { return Err(Error::MismatchHeaderChainId { given: self.signed_header.header.chain_id.to_string(), diff --git a/crates/ibc/src/core/ics04_channel/channel.rs b/crates/ibc/src/core/ics04_channel/channel.rs index d4b67523a..4eb44f55c 100644 --- a/crates/ibc/src/core/ics04_channel/channel.rs +++ b/crates/ibc/src/core/ics04_channel/channel.rs @@ -168,7 +168,7 @@ impl ChannelEnd { /// NOTE: This method is meant for the proto message conversion from the domain /// `MsgChannelOpenInit` and `MsgChannelOpenTry` types to satisfy their `Protobuf` /// trait bounds. - pub(super) fn new_unchecked( + pub(super) fn new_without_validation( state: State, ordering: Order, remote: Counterparty, @@ -192,7 +192,7 @@ impl ChannelEnd { connection_hops: Vec, version: Version, ) -> Result { - let channel_end = Self::new_unchecked(state, ordering, remote, connection_hops, version); + let channel_end = Self::new_without_validation(state, ordering, remote, connection_hops, version); channel_end.validate_basic()?; Ok(channel_end) } diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs index 197661cef..60dcaae91 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs @@ -62,7 +62,7 @@ impl TryFrom for MsgChannelOpenInit { impl From for RawMsgChannelOpenInit { fn from(domain_msg: MsgChannelOpenInit) -> Self { - let chan_end_on_a = ChannelEnd::new_unchecked( + let chan_end_on_a = ChannelEnd::new_without_validation( State::Init, domain_msg.ordering, Counterparty::new(domain_msg.port_id_on_b, None), diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs index 0c7f55dba..217fdfe5f 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs @@ -97,7 +97,7 @@ impl TryFrom for MsgChannelOpenTry { impl From for RawMsgChannelOpenTry { fn from(domain_msg: MsgChannelOpenTry) -> Self { - let chan_end_on_b = ChannelEnd::new_unchecked( + let chan_end_on_b = ChannelEnd::new_without_validation( State::TryOpen, domain_msg.ordering, Counterparty::new(domain_msg.port_id_on_a, Some(domain_msg.chan_id_on_a)), From 1134f01f1ce926a22c8a899d60b70eab6b70966b Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 5 May 2023 10:26:35 -0400 Subject: [PATCH 17/21] fmt --- crates/ibc/src/core/ics04_channel/channel.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics04_channel/channel.rs b/crates/ibc/src/core/ics04_channel/channel.rs index 4eb44f55c..fe40ec99a 100644 --- a/crates/ibc/src/core/ics04_channel/channel.rs +++ b/crates/ibc/src/core/ics04_channel/channel.rs @@ -192,7 +192,8 @@ impl ChannelEnd { connection_hops: Vec, version: Version, ) -> Result { - let channel_end = Self::new_without_validation(state, ordering, remote, connection_hops, version); + let channel_end = + Self::new_without_validation(state, ordering, remote, connection_hops, version); channel_end.validate_basic()?; Ok(channel_end) } From 7bec947f9faa4dbaefcf1e11a725236326cbd300 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 May 2023 08:15:37 -0700 Subject: [PATCH 18/21] Fix channel state check for acknowledgement --- .../ibc/src/core/ics04_channel/handler/acknowledgement.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs b/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs index 1a41373b8..dbe3011b5 100644 --- a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs +++ b/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs @@ -1,6 +1,6 @@ use crate::core::ics03_connection::connection::State as ConnectionState; use crate::core::ics03_connection::delay::verify_conn_delay_passed; -use crate::core::ics04_channel::channel::{Counterparty, Order}; +use crate::core::ics04_channel::channel::{Counterparty, Order, State as ChannelState}; use crate::core::ics04_channel::commitment::{compute_ack_commitment, compute_packet_commitment}; use crate::core::ics04_channel::error::ChannelError; use crate::core::ics04_channel::error::PacketError; @@ -23,9 +23,7 @@ where let chan_end_path_on_a = ChannelEndPath::new(&packet.port_id_on_a, &packet.chan_id_on_a); let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?; - // Checks the channel end not be `Closed`. - // This allows for optimistic packet processing before a channel opens - chan_end_on_a.verify_not_closed()?; + chan_end_on_a.verify_state_matches(&ChannelState::Open)?; let counterparty = Counterparty::new( packet.port_id_on_b.clone(), From 1f01e765c1a4959ea2085bedb66f848013a3325f Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 May 2023 08:40:19 -0700 Subject: [PATCH 19/21] Remove comment of connection_hops_length check --- crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs | 3 --- 1 file changed, 3 deletions(-) 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 ebc712bee..5f8adada4 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 @@ -17,9 +17,6 @@ where { ctx_b.validate_message_signer(&msg.signer)?; - // Note: Verify that the provided message contains only one connection end - // has been done during the conversion from the proto to the domain type. - let conn_end_on_b = ctx_b.connection_end(&msg.connection_hops_on_b[0])?; conn_end_on_b.verify_state_matches(&ConnectionState::Open)?; From 7f65497c1cb727c584092b439e46026981045468 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 May 2023 09:32:23 -0700 Subject: [PATCH 20/21] Remove previous_channel_id field from MsgChannelOpenTry --- crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs index 217fdfe5f..da4c3b0a7 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs @@ -30,9 +30,6 @@ pub struct MsgChannelOpenTry { pub ordering: Order, pub signer: Signer, - #[deprecated(since = "0.22.0")] - /// Only kept here for proper conversion to/from the raw type - pub previous_channel_id: String, #[deprecated(since = "0.22.0")] /// Only kept here for proper conversion to/from the raw type pub version_proposal: Version, @@ -71,7 +68,6 @@ impl TryFrom for MsgChannelOpenTry { let msg = MsgChannelOpenTry { port_id_on_b: raw_msg.port_id.parse()?, ordering: chan_end_on_b.ordering, - previous_channel_id: raw_msg.previous_channel_id, connection_hops_on_b: chan_end_on_b.connection_hops, port_id_on_a: chan_end_on_b.remote.port_id, chan_id_on_a: chan_end_on_b @@ -107,7 +103,7 @@ impl From for RawMsgChannelOpenTry { #[allow(deprecated)] RawMsgChannelOpenTry { port_id: domain_msg.port_id_on_b.to_string(), - previous_channel_id: domain_msg.previous_channel_id, + previous_channel_id: "".to_string(), // Excessive field to satisfy the type conversion", channel: Some(chan_end_on_b.into()), counterparty_version: domain_msg.version_supported_on_a.to_string(), proof_init: domain_msg.proof_chan_end_on_a.clone().into(), From cb71f2c2c1ae0706b1440fd17c0c46fc964a3853 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 5 May 2023 10:12:23 -0700 Subject: [PATCH 21/21] Expose verify_connection_hops_length for chan_open_init/try --- crates/ibc/src/core/ics04_channel/channel.rs | 37 +++++++++++-------- .../ics04_channel/handler/chan_close_init.rs | 2 +- .../ics04_channel/handler/chan_open_ack.rs | 2 +- .../handler/chan_open_confirm.rs | 2 +- .../ics04_channel/handler/chan_open_init.rs | 1 + .../ics04_channel/handler/chan_open_try.rs | 2 + .../core/ics04_channel/msgs/chan_open_init.rs | 10 +++++ .../core/ics04_channel/msgs/chan_open_try.rs | 10 +++++ 8 files changed, 48 insertions(+), 18 deletions(-) diff --git a/crates/ibc/src/core/ics04_channel/channel.rs b/crates/ibc/src/core/ics04_channel/channel.rs index fe40ec99a..0dd88cee0 100644 --- a/crates/ibc/src/core/ics04_channel/channel.rs +++ b/crates/ibc/src/core/ics04_channel/channel.rs @@ -251,20 +251,6 @@ impl ChannelEnd { }); } - // Current IBC version only supports one connection hop. - self.verify_connection_hops_length(1)?; - - Ok(()) - } - - /// Checks if the `connection_hops` has a length of `expected`. - pub fn verify_connection_hops_length(&self, expected: usize) -> Result<(), ChannelError> { - if self.connection_hops.len() != expected { - return Err(ChannelError::InvalidConnectionHopsLength { - expected, - actual: self.connection_hops.len(), - }); - } Ok(()) } @@ -310,11 +296,32 @@ impl ChannelEnd { Ok(()) } + /// Checks if the `connection_hops` has a length of `expected`. + /// + /// Note: Current IBC version only supports one connection hop. + pub fn verify_connection_hops_length(&self) -> Result<(), ChannelError> { + verify_connection_hops_length(&self.connection_hops, 1) + } + pub fn version_matches(&self, other: &Version) -> bool { self.version().eq(other) } } +/// Checks if the `connection_hops` has a length of `expected`. +pub(crate) fn verify_connection_hops_length( + connection_hops: &Vec, + expected: usize, +) -> Result<(), ChannelError> { + if connection_hops.len() != expected { + return Err(ChannelError::InvalidConnectionHopsLength { + expected, + actual: connection_hops.len(), + }); + } + Ok(()) +} + #[cfg_attr( feature = "parity-scale-codec", derive( @@ -664,7 +671,7 @@ mod tests { .collect(), ..raw_channel_end.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Raw channel end with correct params".to_string(), 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 da8398b16..417130fea 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 @@ -18,7 +18,7 @@ where chan_end_on_a.verify_not_closed()?; // An OPEN IBC connection running on the local (host) chain should exist. - chan_end_on_a.verify_connection_hops_length(1)?; + chan_end_on_a.verify_connection_hops_length()?; let conn_end_on_a = ctx_a.connection_end(&chan_end_on_a.connection_hops()[0])?; 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 672507eee..8397802f3 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 @@ -24,7 +24,7 @@ where chan_end_on_a.verify_state_matches(&ChannelState::Init)?; // An OPEN IBC connection running on the local (host) chain should exist. - chan_end_on_a.verify_connection_hops_length(1)?; + chan_end_on_a.verify_connection_hops_length()?; let conn_end_on_a = ctx_a.connection_end(&chan_end_on_a.connection_hops()[0])?; 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 edc05a6a0..7a2e6fcab 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 @@ -25,7 +25,7 @@ where chan_end_on_b.verify_state_matches(&ChannelState::TryOpen)?; // An OPEN IBC connection running on the local (host) chain should exist. - chan_end_on_b.verify_connection_hops_length(1)?; + chan_end_on_b.verify_connection_hops_length()?; let conn_end_on_b = ctx_b.connection_end(&chan_end_on_b.connection_hops()[0])?; 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 41ef700a3..ee4c67439 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 @@ -11,6 +11,7 @@ where { ctx_a.validate_message_signer(&msg.signer)?; + msg.verify_connection_hops_length()?; // An IBC connection running on the local (host) chain should exist. let conn_end_on_a = ctx_a.connection_end(&msg.connection_hops_on_a[0])?; 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 5f8adada4..ae048da20 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 @@ -17,6 +17,8 @@ where { ctx_b.validate_message_signer(&msg.signer)?; + msg.verify_connection_hops_length()?; + let conn_end_on_b = ctx_b.connection_end(&msg.connection_hops_on_b[0])?; conn_end_on_b.verify_state_matches(&ConnectionState::Open)?; diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs index 60dcaae91..dbae00acb 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_init.rs @@ -1,3 +1,4 @@ +use crate::core::ics04_channel::channel::verify_connection_hops_length; use crate::core::ics04_channel::channel::ChannelEnd; use crate::core::ics04_channel::channel::Counterparty; use crate::core::ics04_channel::channel::{Order, State}; @@ -28,6 +29,15 @@ pub struct MsgChannelOpenInit { pub version_proposal: Version, } +impl MsgChannelOpenInit { + /// Checks if the `connection_hops` has a length of `expected`. + /// + /// Note: Current IBC version only supports one connection hop. + pub(crate) fn verify_connection_hops_length(&self) -> Result<(), ChannelError> { + verify_connection_hops_length(&self.connection_hops_on_a, 1) + } +} + impl Msg for MsgChannelOpenInit { type Raw = RawMsgChannelOpenInit; diff --git a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs index da4c3b0a7..e13b73585 100644 --- a/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs +++ b/crates/ibc/src/core/ics04_channel/msgs/chan_open_try.rs @@ -1,3 +1,4 @@ +use crate::core::ics04_channel::channel::verify_connection_hops_length; use crate::core::ics04_channel::channel::ChannelEnd; use crate::core::ics04_channel::channel::Counterparty; use crate::core::ics04_channel::channel::{Order, State}; @@ -35,6 +36,15 @@ pub struct MsgChannelOpenTry { pub version_proposal: Version, } +impl MsgChannelOpenTry { + /// Checks if the `connection_hops` has a length of `expected`. + /// + /// Note: Current IBC version only supports one connection hop. + pub(crate) fn verify_connection_hops_length(&self) -> Result<(), ChannelError> { + verify_connection_hops_length(&self.connection_hops_on_b, 1) + } +} + impl Msg for MsgChannelOpenTry { type Raw = RawMsgChannelOpenTry;