diff --git a/CHANGELOG.md b/CHANGELOG.md index d5c63e31dd..4d6aac175d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ - [ibc] - Fix overflow bug in ICS03 client consensus height verification method ([#685]) + - Allow a conn open ack to succeed in the happy case ([#699]) - [ibc-relayer] - [nothing yet] @@ -44,7 +45,7 @@ ### BREAKING CHANGES - [ibc] - - [nothing yet] + - `MsgConnectionOpenAck.counterparty_connection_id` is now a `ConnectionId` instead of an `Option`([#700]) - [ibc-relayer] - [nothing yet] @@ -55,6 +56,8 @@ [#599]: https://github.com/informalsystems/ibc-rs/issues/599 [#685]: https://github.com/informalsystems/ibc-rs/issues/685 [#689]: https://github.com/informalsystems/ibc-rs/issues/689 +[#699]: https://github.com/informalsystems/ibc-rs/issues/699 +[#700]: https://github.com/informalsystems/ibc-rs/pull/700 ## v0.1.1 *February 17, 2021* diff --git a/modules/src/ics02_client/client_def.rs b/modules/src/ics02_client/client_def.rs index d2a64f944c..d4b613daa1 100644 --- a/modules/src/ics02_client/client_def.rs +++ b/modules/src/ics02_client/client_def.rs @@ -73,7 +73,7 @@ pub trait ClientDef: Clone { height: Height, prefix: &CommitmentPrefix, proof: &CommitmentProofBytes, - connection_id: &ConnectionId, + connection_id: Option<&ConnectionId>, expected_connection_end: &ConnectionEnd, ) -> Result<(), Box>; @@ -472,7 +472,7 @@ impl ClientDef for AnyClient { height: Height, prefix: &CommitmentPrefix, proof: &CommitmentProofBytes, - connection_id: &ConnectionId, + connection_id: Option<&ConnectionId>, expected_connection_end: &ConnectionEnd, ) -> Result<(), Box> { match self { diff --git a/modules/src/ics03_connection/handler/conn_open_ack.rs b/modules/src/ics03_connection/handler/conn_open_ack.rs index 73a50e0500..e4552a7962 100644 --- a/modules/src/ics03_connection/handler/conn_open_ack.rs +++ b/modules/src/ics03_connection/handler/conn_open_ack.rs @@ -30,10 +30,15 @@ pub(crate) fn process( || old_conn_end.state_matches(&State::TryOpen) && old_conn_end.versions().get(0).eq(&Some(msg.version())); - // Check that if the msg's counterparty connection id is not empty then it matches - // the old connection's counterparty. - let counterparty_matches = msg.counterparty_connection_id().is_none() - || old_conn_end.counterparty().connection_id() == msg.counterparty_connection_id(); + // Check that, if we have a counterparty connection id, then it + // matches the one in the message. + let counterparty_matches = if let Some(counterparty_connection_id) = + old_conn_end.counterparty().connection_id() + { + &msg.counterparty_connection_id == counterparty_connection_id + } else { + true + }; if state_is_consistent && counterparty_matches { Ok(old_conn_end) @@ -59,8 +64,8 @@ pub(crate) fn process( Counterparty::new( // The counterparty is the local chain. new_conn_end.client_id().clone(), // The local client identifier. - msg.counterparty_connection_id().cloned(), // This chain's connection id as known on counterparty. - ctx.commitment_prefix(), // Local commitment prefix. + Some(msg.counterparty_connection_id().clone()), // This chain's connection id as known on counterparty. + ctx.commitment_prefix(), // Local commitment prefix. ), vec![msg.version().clone()], new_conn_end.delay_period, @@ -147,7 +152,7 @@ mod tests { client_id.clone(), Counterparty::new( client_id.clone(), - msg_ack.counterparty_connection_id().cloned(), + Some(msg_ack.counterparty_connection_id().clone()), CommitmentPrefix::from(b"ibc".to_vec()), ), vec![msg_ack.version().clone()], @@ -164,20 +169,10 @@ mod tests { conn_end_prefix.set_state(State::Init); conn_end_prefix.set_counterparty(Counterparty::new( client_id.clone(), - msg_ack.counterparty_connection_id().cloned(), + Some(msg_ack.counterparty_connection_id().clone()), CommitmentPrefix::from(vec![]), // incorrect field )); - // A connection end with correct state & prefix, but incorrect counterparty; exercises - // unsuccessful processing path. - let mut conn_end_cparty = conn_end_open.clone(); - conn_end_cparty.set_state(State::Init); - conn_end_cparty.set_counterparty(Counterparty::new( - client_id.clone(), - None, // incorrect field - CommitmentPrefix::from(b"ibc".to_vec()), - )); - let tests: Vec = vec![ Test { name: "Successful processing of an Ack message".to_string(), @@ -209,21 +204,11 @@ mod tests { Test { name: "Processing fails: ConsensusStateVerificationFailure due to empty counterparty prefix".to_string(), ctx: default_context - .clone() .with_client(&client_id, proof_height) - .with_connection(conn_id.clone(), conn_end_prefix), - msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())), - want_pass: false, - error_kind: Some(Kind::ConsensusStateVerificationFailure(proof_height)) - }, - Test { - name: "Processing fails due to mismatching counterparty conn id".to_string(), - ctx: default_context - .with_client(&client_id, proof_height) - .with_connection(conn_id.clone(), conn_end_cparty), + .with_connection(conn_id, conn_end_prefix), msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack)), want_pass: false, - error_kind: Some(Kind::ConnectionMismatch(conn_id)) + error_kind: Some(Kind::ConsensusStateVerificationFailure(proof_height)) }, /* Test { diff --git a/modules/src/ics03_connection/handler/verify.rs b/modules/src/ics03_connection/handler/verify.rs index 6f6e3c89c6..7b9c76a7fd 100644 --- a/modules/src/ics03_connection/handler/verify.rs +++ b/modules/src/ics03_connection/handler/verify.rs @@ -96,7 +96,7 @@ pub fn verify_connection_proof( proof_height, connection_end.counterparty().prefix(), proof, - &connection_end.counterparty().connection_id().unwrap(), + connection_end.counterparty().connection_id(), expected_conn, ) .map_err(|_| Kind::InvalidProof)?) diff --git a/modules/src/ics03_connection/msgs/conn_open_ack.rs b/modules/src/ics03_connection/msgs/conn_open_ack.rs index 1f0d57ac6d..670ed8dbe5 100644 --- a/modules/src/ics03_connection/msgs/conn_open_ack.rs +++ b/modules/src/ics03_connection/msgs/conn_open_ack.rs @@ -1,5 +1,4 @@ use std::convert::{TryFrom, TryInto}; -use std::str::FromStr; use tendermint::account::Id as AccountId; use tendermint_proto::Protobuf; @@ -22,7 +21,7 @@ pub const TYPE_URL: &str = "/ibc.core.connection.v1.MsgConnectionOpenAck"; #[derive(Clone, Debug, PartialEq, Eq)] pub struct MsgConnectionOpenAck { pub connection_id: ConnectionId, - pub counterparty_connection_id: Option, + pub counterparty_connection_id: ConnectionId, pub client_state: Option, pub proofs: Proofs, pub version: Version, @@ -36,8 +35,8 @@ impl MsgConnectionOpenAck { } /// Getter for accessing the counterparty's connection identifier from this message. - pub fn counterparty_connection_id(&self) -> Option<&ConnectionId> { - self.counterparty_connection_id.as_ref() + pub fn counterparty_connection_id(&self) -> &ConnectionId { + &self.counterparty_connection_id } /// Getter for accessing the client state. @@ -107,18 +106,15 @@ impl TryFrom for MsgConnectionOpenAck { .filter(|x| !x.is_empty()) .map(CommitmentProofBytes::from); - let counterparty_connection_id = Some(msg.counterparty_connection_id) - .filter(|x| !x.is_empty()) - .map(|v| FromStr::from_str(v.as_str())) - .transpose() - .map_err(|e| Kind::IdentifierError.context(e))?; - Ok(Self { connection_id: msg .connection_id .parse() .map_err(|e| Kind::IdentifierError.context(e))?, - counterparty_connection_id, + counterparty_connection_id: msg + .counterparty_connection_id + .parse() + .map_err(|e| Kind::IdentifierError.context(e))?, client_state: msg .client_state .map(AnyClientState::try_from) @@ -146,9 +142,7 @@ impl From for RawMsgConnectionOpenAck { fn from(ics_msg: MsgConnectionOpenAck) -> Self { RawMsgConnectionOpenAck { connection_id: ics_msg.connection_id.as_str().to_string(), - counterparty_connection_id: ics_msg - .counterparty_connection_id - .map_or_else(|| "".to_string(), |v| v.as_str().to_string()), + counterparty_connection_id: ics_msg.counterparty_connection_id.as_str().to_string(), client_state: ics_msg .client_state .map_or_else(|| None, |v| Some(v.into())), diff --git a/modules/src/ics07_tendermint/client_def.rs b/modules/src/ics07_tendermint/client_def.rs index b11eba1405..fedccc7fe0 100644 --- a/modules/src/ics07_tendermint/client_def.rs +++ b/modules/src/ics07_tendermint/client_def.rs @@ -57,7 +57,7 @@ impl ClientDef for TendermintClient { _height: Height, _prefix: &CommitmentPrefix, _proof: &CommitmentProofBytes, - _connection_id: &ConnectionId, + _connection_id: Option<&ConnectionId>, _expected_connection_end: &ConnectionEnd, ) -> Result<(), Box> { todo!() diff --git a/modules/src/mock/client_def.rs b/modules/src/mock/client_def.rs index e7e13b6b33..ec31ff9caa 100644 --- a/modules/src/mock/client_def.rs +++ b/modules/src/mock/client_def.rs @@ -64,7 +64,7 @@ impl ClientDef for MockClient { _height: Height, _prefix: &CommitmentPrefix, _proof: &CommitmentProofBytes, - _connection_id: &ConnectionId, + _connection_id: Option<&ConnectionId>, _expected_connection_end: &ConnectionEnd, ) -> Result<(), Box> { Ok(()) diff --git a/relayer/src/connection.rs b/relayer/src/connection.rs index 2b23a3e9a6..7e56bc8abc 100644 --- a/relayer/src/connection.rs +++ b/relayer/src/connection.rs @@ -566,7 +566,7 @@ impl Connection { let new_msg = MsgConnectionOpenAck { connection_id: self.dst_connection_id().clone(), - counterparty_connection_id: Option::from(self.src_connection_id().clone()), + counterparty_connection_id: self.src_connection_id().clone(), client_state, proofs, version: src_connection.versions()[0].clone(),