Some cleanup (followup to protobuf dedup)

CHANGELOG.md

@@ -21,16 +21,24 @@
   - Fix for mismatching `bitcoin` dep ([#525])
 
 - [modules]
-  -  Clean the validate_basic method ([#94])
+  - Clean the validate_basic method ([#94])
+  - MsgConnectionOpenAck testing improvements ([#306])
 
 - Update to `tendermint-rs` v0.17.1 ([#517])
 
+### BUG FIXES:
+
+- [modules]
+  - Fix for storing ClientType upon 'create-client' ([#513])
+
 [#94]: https://github.com/informalsystems/ibc-rs/issues/94
+[#306]: https://github.com/informalsystems/ibc-rs/issues/306
 [#470]: https://github.com/informalsystems/ibc-rs/issues/470
 [#500]: https://github.com/informalsystems/ibc-rs/issues/500
 [#505]: https://github.com/informalsystems/ibc-rs/issues/505
 [#507]: https://github.com/informalsystems/ibc-rs/issues/507
 [#511]: https://github.com/informalsystems/ibc-rs/pull/511
+[#513]: https://github.com/informalsystems/ibc-rs/issues/513
 [#514]: https://github.com/informalsystems/ibc-rs/issues/514
 [#517]: https://github.com/informalsystems/ibc-rs/issues/517
 [#525]: https://github.com/informalsystems/ibc-rs/issues/525

modules/src/ics02_client/context.rs

@@ -19,19 +19,14 @@ pub trait ClientReader {
 
 /// Defines the write-only part of ICS2 (client functions) context.
 pub trait ClientKeeper {
-    fn store_client_type(
-        &mut self,
-        client_id: ClientId,
-        client_type: ClientType,
-    ) -> Result<(), Error>;
-
     fn store_client_result(
         &mut self,
         handler_res: ClientResult,
     ) -> Result<Vec<ClientEvent>, Error> {
         match handler_res {
             Create(res) => {
                 let client_id = self.next_client_id();
+                self.store_client_type(client_id.clone(), res.client_type)?;
                 self.store_client_state(client_id.clone(), res.client_state.clone())?;
                 self.store_consensus_state(
                     client_id.clone(),
@@ -54,12 +49,21 @@ pub trait ClientKeeper {
 
     fn next_client_id(&mut self) -> ClientId;
 
+    /// Called upon successful client creation
+    fn store_client_type(
+        &mut self,
+        client_id: ClientId,
+        client_type: ClientType,
+    ) -> Result<(), Error>;
+
+    /// Called upon successful client creation and update
     fn store_client_state(
         &mut self,
         client_id: ClientId,
         client_state: AnyClientState,
     ) -> Result<(), Error>;
 
+    /// Called upon successful client creation and update
     fn store_consensus_state(
         &mut self,
         client_id: ClientId,

modules/src/ics03_connection/connection.rs

@@ -109,6 +109,11 @@ impl ConnectionEnd {
         self.state = new_state;
     }
 
+    /// Setter for the `counterparty` field.
+    pub fn set_counterparty(&mut self, new_cparty: Counterparty) {
+        self.counterparty = new_cparty;
+    }
+
     /// Setter for the `version` field.
     /// TODO: A ConnectionEnd should only store one version.
     pub fn set_version(&mut self, new_version: Version) {

modules/src/ics03_connection/error.rs

@@ -6,7 +6,7 @@ pub type Error = anomaly::Error<Kind>;
 use crate::ics24_host::identifier::{ClientId, ConnectionId};
 use crate::Height;
 
-#[derive(Clone, Debug, Error)]
+#[derive(Clone, Debug, Error, Eq, PartialEq)]
 pub enum Kind {
     #[error("connection state unknown")]
     InvalidState(i32),
@@ -74,23 +74,23 @@ pub enum Kind {
     #[error("client proof must be present")]
     NullClientProof,
 
-    #[error("the client is frozen")]
-    FrozenClient,
+    #[error("the client running locally is frozen")]
+    FrozenClient(ClientId),
 
     #[error("the connection proof verification failed")]
     ConnectionVerificationFailure,
 
-    #[error("the expected consensus state could not be retrieved")]
-    MissingClientConsensusState,
+    #[error("the consensus state at height {0} for client id {1} could not be retrieved")]
+    MissingClientConsensusState(Height, ClientId),
 
     #[error("the local consensus state could not be retrieved")]
     MissingLocalConsensusState,
 
     #[error("the consensus proof verification failed (height: {0})")]
     ConsensusStateVerificationFailure(Height),
 
-    #[error("the client state proof verification failed")]
-    ClientStateVerificationFailure,
+    #[error("the client state proof verification failed for client id: {0}")]
+    ClientStateVerificationFailure(ClientId),
 }
 
 impl Kind {

modules/src/ics03_connection/handler/conn_open_ack.rs

@@ -96,7 +96,7 @@ mod tests {
 
     use crate::handler::EventType;
     use crate::ics03_connection::connection::{ConnectionEnd, Counterparty, State};
-    use crate::ics03_connection::context::ConnectionReader;
+    use crate::ics03_connection::error::Kind;
     use crate::ics03_connection::handler::{dispatch, ConnectionResult};
     use crate::ics03_connection::msgs::conn_open_ack::test_util::get_dummy_msg_conn_open_ack;
     use crate::ics03_connection::msgs::conn_open_ack::MsgConnectionOpenAck;
@@ -114,115 +114,119 @@ mod tests {
             ctx: MockContext,
             msg: ConnectionMsg,
             want_pass: bool,
+            error_kind: Option<Kind>,
         }
 
-        let client_id = ClientId::from_str("mock_clientid").unwrap();
         let msg_ack = MsgConnectionOpenAck::try_from(get_dummy_msg_conn_open_ack()).unwrap();
-        let counterparty = Counterparty::new(
-            client_id.clone(),
-            msg_ack.counterparty_connection_id().cloned(),
-            CommitmentPrefix::from(vec![]),
-        );
+        let conn_id = msg_ack.connection_id.clone();
 
-        let incorrect_context = MockContext::default();
+        // Client parameters -- identifier and correct height (matching the proof height)
+        let client_id = ClientId::from_str("mock_clientid").unwrap();
+        let proof_height = msg_ack.proofs.height();
 
-        // A connection end (with incorrect state `Open`) that will be part of the context.
-        let incorrect_conn_end_state = ConnectionEnd::new(
-            State::Open,
-            client_id.clone(),
-            counterparty,
-            incorrect_context.get_compatible_versions(),
-            0_u64,
+        // Parametrize the host chain to have a height at least as recent as the
+        // the height of the proofs in the Ack msg.
+        let default_context = MockContext::new(
+            ChainId::new("mockgaia".to_string(), 1),
+            HostType::Mock,
+            5,
+            Height::new(1, msg_ack.proofs().height().increment().revision_height),
         );
 
-        // A connection end (with correct state but incorrect versions); exercises unsuccessful
-        // processing path.
-        let mut incorrect_conn_end_vers = incorrect_conn_end_state.clone();
-        incorrect_conn_end_vers.set_state(State::Init);
-
-        // A connection end (with correct versions and correct state, but incorrect prefix for the
-        // counterparty) that will be part of the context to exercise unsuccessful path.
-        let mut incorrect_conn_end_prefix = incorrect_conn_end_state.clone();
-        incorrect_conn_end_prefix.set_state(State::Init);
-        incorrect_conn_end_prefix.set_version(msg_ack.version().clone());
-
-        // Build a connection end that will exercise the successful path.
-        let correct_counterparty = Counterparty::new(
-            client_id.clone(),
-            msg_ack.counterparty_connection_id().cloned(),
-            CommitmentPrefix::from(b"ibc".to_vec()),
-        );
-        let correct_conn_end = ConnectionEnd::new(
+        // A connection end that will exercise the successful path.
+        let default_conn_end = ConnectionEnd::new(
             State::Init,
             client_id.clone(),
-            correct_counterparty,
+            Counterparty::new(
+                client_id.clone(),
+                msg_ack.counterparty_connection_id().cloned(),
+                CommitmentPrefix::from(b"ibc".to_vec()),
+            ),
             vec![msg_ack.version().clone()],
             0_u64,
         );
 
-        // Parametrize the (correct) host chain to have a height at least as recent as the
-        // the height of the proofs in the Ack msg.
-        let correct_context = MockContext::new(
-            ChainId::new("mockgaia".to_string(), 1),
-            HostType::Mock,
-            5,
-            Height::new(1, msg_ack.proofs().height().increment().revision_height),
-        );
+        // A connection end with incorrect state `Open`; will be part of the context.
+        let mut conn_end_open = default_conn_end.clone();
+        conn_end_open.set_state(State::Open); // incorrect field
+
+        // A connection end with correct state, but incorrect prefix for the
+        // counterparty; will be part of the context to exercise unsuccessful path.
+        let mut conn_end_prefix = conn_end_open.clone();
+        conn_end_prefix.set_state(State::Init);
+        conn_end_prefix.set_counterparty(Counterparty::new(
+            client_id.clone(),
+            msg_ack.counterparty_connection_id().cloned(),
+            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<Test> = vec![
             Test {
-                name: "Processing fails due to missing connection in context".to_string(),
-                ctx: correct_context.clone(),
+                name: "Successful processing of an Ack message".to_string(),
+                ctx: default_context
+                    .clone()
+                    .with_client(&client_id, proof_height)
+                    .with_connection(conn_id.clone(), default_conn_end.clone()),
                 msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
-                want_pass: false,
+                want_pass: true,
+                error_kind: None,
             },
             Test {
-                name: "Processing fails due to connections mismatch (incorrect state)".to_string(),
-                ctx: correct_context
-                    .clone()
-                    .with_client(&client_id, Height::new(0, 10))
-                    .with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_state),
+                name: "Processing fails because the connection does not exist in the context".to_string(),
+                ctx: MockContext::default(),   // Empty context
                 msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
                 want_pass: false,
+                error_kind: Some(Kind::UninitializedConnection(conn_id.clone())),
             },
             Test {
-                name: "Processing fails due to connections mismatch (incorrect versions)"
-                    .to_string(),
-                ctx: correct_context
+                name: "Processing fails due to connections mismatch (incorrect 'open' state)".to_string(),
+                ctx: default_context
                     .clone()
-                    .with_client(&client_id, Height::new(0, 10))
-                    .with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_vers),
+                    .with_client(&client_id, proof_height)
+                    .with_connection(conn_id.clone(), conn_end_open),
                 msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
                 want_pass: false,
+                error_kind: Some(Kind::ConnectionMismatch(conn_id.clone()))
             },
             Test {
                 name: "Processing fails: ConsensusStateVerificationFailure due to empty counterparty prefix".to_string(),
-                ctx: correct_context
+                ctx: default_context
                     .clone()
-                    .with_client(&client_id, Height::new(0, 10))
-                    .with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_prefix),
+                    .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 MissingLocalConsensusState".to_string(),
-                ctx: incorrect_context
-                    .with_client(&client_id, Height::new(0, 10))
-                    .with_connection(msg_ack.connection_id().clone(), correct_conn_end.clone()),
+                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),
                 msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
                 want_pass: false,
+                error_kind: Some(Kind::ConnectionMismatch(conn_id.clone()))
             },
             Test {
-                name: "Successful processing of Ack message".to_string(),
-                ctx: correct_context
-                    .with_client(&client_id, Height::new(0, 10))
-                    .with_connection(msg_ack.connection_id().clone(), correct_conn_end),
-                msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
-                want_pass: true,
+                name: "Processing fails due to MissingLocalConsensusState".to_string(),
+                ctx: MockContext::default()
+                    .with_client(&client_id, proof_height)
+                    .with_connection(conn_id, default_conn_end),
+                msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack)),
+                want_pass: false,
+                error_kind: Some(Kind::MissingLocalConsensusState)
             },
-        ]
-        .into_iter()
-        .collect();
+        ];
 
         for test in tests {
             let res = dispatch(&test.ctx, test.msg.clone());
@@ -248,6 +252,7 @@ mod tests {
                     }
                 }
                 Err(e) => {
+                    println!("Error for {:?} was {:#?}", test.name, e.kind());
                     assert_eq!(
                         test.want_pass,
                         false,
@@ -257,6 +262,14 @@ mod tests {
                         test.ctx.clone(),
                         e,
                     );
+                    // Verify that the error kind matches
+                    if let Some(expected_kind) = test.error_kind {
+                        assert_eq!(
+                            &expected_kind,
+                            e.kind(),
+                            "conn_open_ack: expected error kind mismatches thrown error kind"
+                        )
+                    }
                 }
             }
         }