Re-enable tests in ics02_client and foreign_client (informalsyste…

…ms#535)

* ClientId constructor; re-enabled ClientCreate tests

* Improved ics18; Fixed cfg cargo annotation.

* Prep for fixing ForeignClient tests.

* Enable tests in the foreign_client module

* Reuse Height parsing

* More idiomatic methods in ibc::mock::context::MockContext

* Improve assert in build_create_client_and_send

* Remove ibc::handler::Event

* Fix extract_connection_id

* Check IBCEvent produced in create_client

* Check IBCEvent produced in test_tm_create_client_ok

* update CHANGELOG

* relayer: improve tests in foreign_client

Co-authored-by: Vitor Enes <[email protected]>

.gitignore

@@ -18,4 +18,7 @@ Cargo.lock
 .idea
 
 # Ignore VisualStudio
-.vscode
+.vscode
+
+# Ignore chain's data
+data

CHANGELOG.md

@@ -19,7 +19,7 @@
 - [relayer-cli]
   - Replace `ChannelConfig` in `Channel::new` ([#511])
   - Add `packet-send` CLI ([#470])
-  - UX improvements for relayer txs ([#536, 540])
+  - UX improvements for relayer txs ([#536, #540])
 
 - [relayer]
   - Performance improvements ([#514], [#537])
@@ -34,6 +34,11 @@
 - [modules]
   - Fix for storing `ClientType` upon 'create-client' ([#513])
 
+### BREAKING CHANGES:
+
+- [modules]
+  - The `ibc::handler::Event` is removed and handlers now produce `ibc::events::IBCEvent`s ([#535])
+
 [#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
@@ -50,6 +55,7 @@
 [#536]: https://github.com/informalsystems/ibc-rs/issues/536
 [#537]: https://github.com/informalsystems/ibc-rs/issues/537
 [#540]: https://github.com/informalsystems/ibc-rs/issues/540
+[#535]: https://github.com/informalsystems/ibc-rs/issues/535
 
 
 ## v0.0.6

relayer/src/chain/cosmos.rs

@@ -764,19 +764,19 @@ fn packet_from_tx_search_response(
             continue;
         }
 
-        for e in r.clone().tx_result.events.iter() {
+        for e in r.tx_result.events.iter() {
             if e.type_str != request.event_id.as_str() {
                 continue;
             }
 
-            let res = from_tx_response_event(e.clone());
+            let res = from_tx_response_event(e);
             if res.is_none() {
                 continue;
             }
             let event = res.unwrap();
-            let packet = match event.clone() {
-                IBCEvent::SendPacketChannel(send_ev) => Some(send_ev.packet),
-                IBCEvent::WriteAcknowledgementChannel(ack_ev) => Some(ack_ev.packet),
+            let packet = match &event {
+                IBCEvent::SendPacketChannel(send_ev) => Some(&send_ev.packet),
+                IBCEvent::WriteAcknowledgementChannel(ack_ev) => Some(&ack_ev.packet),
                 _ => None,
             };
 
@@ -900,8 +900,8 @@ pub fn tx_result_to_event(response: Response) -> Result<Vec<IBCEvent>, anomaly::
     }
 
     for event in response.deliver_tx.events {
-        if let Some(ibc_ev) = from_tx_response_event(event) {
-            result.append(&mut vec![ibc_ev])
+        if let Some(ibc_ev) = from_tx_response_event(&event) {
+            result.push(ibc_ev);
         }
     }
     Ok(result)

relayer/src/chain/mock.rs

@@ -103,12 +103,12 @@ impl Chain for MockChain {
 
     fn send_msgs(&mut self, proto_msgs: Vec<Any>) -> Result<Vec<IBCEvent>, Error> {
         // Use the ICS18Context interface to submit the set of messages.
-        self.context
+        let events = self
+            .context
             .send(proto_msgs)
             .map_err(|e| Kind::Rpc(self.config.rpc_addr.clone()).context(e))?;
 
-        // TODO FIX tests with this
-        Ok(vec![])
+        Ok(events)
     }
 
     fn get_signer(&mut self) -> Result<Id, Error> {

relayer/src/channel.rs

@@ -281,14 +281,13 @@ impl Channel {
 
 fn extract_channel_id(event: &IBCEvent) -> Result<&ChannelId, ChannelError> {
     match event {
-        IBCEvent::OpenInitChannel(ev) => Ok(ev.channel_id()),
-        IBCEvent::OpenTryChannel(ev) => Ok(ev.channel_id()),
-        IBCEvent::OpenAckChannel(ev) => Ok(ev.channel_id()),
-        IBCEvent::OpenConfirmChannel(ev) => Ok(ev.channel_id()),
-        _ => Err(ChannelError::Failed(
-            "cannot extract channel_id from result".to_string(),
-        )),
+        IBCEvent::OpenInitChannel(ev) => ev.channel_id().as_ref(),
+        IBCEvent::OpenTryChannel(ev) => ev.channel_id().as_ref(),
+        IBCEvent::OpenAckChannel(ev) => ev.channel_id().as_ref(),
+        IBCEvent::OpenConfirmChannel(ev) => ev.channel_id().as_ref(),
+        _ => None,
     }
+    .ok_or_else(|| ChannelError::Failed("cannot extract channel_id from result".to_string()))
 }
 
 /// Enumeration of proof carrying ICS4 message, helper for relayer.

relayer/src/connection.rs

@@ -281,14 +281,13 @@ impl Connection {
 
 fn extract_connection_id(event: &IBCEvent) -> Result<&ConnectionId, ConnectionError> {
     match event {
-        IBCEvent::OpenInitConnection(ev) => Ok(ev.connection_id()),
-        IBCEvent::OpenTryConnection(ev) => Ok(ev.connection_id()),
-        IBCEvent::OpenAckConnection(ev) => Ok(ev.connection_id()),
-        IBCEvent::OpenConfirmConnection(ev) => Ok(ev.connection_id()),
-        _ => Err(ConnectionError::Failed(
-            "cannot extract connection_id from result".to_string(),
-        )),
+        IBCEvent::OpenInitConnection(ev) => ev.connection_id().as_ref(),
+        IBCEvent::OpenTryConnection(ev) => ev.connection_id().as_ref(),
+        IBCEvent::OpenAckConnection(ev) => ev.connection_id().as_ref(),
+        IBCEvent::OpenConfirmConnection(ev) => ev.connection_id().as_ref(),
+        _ => None,
     }
+    .ok_or_else(|| ConnectionError::Failed("cannot extract connection_id from result".to_string()))
 }
 
 /// Enumeration of proof carrying ICS3 message, helper for relayer.

relayer/src/foreign_client.rs

@@ -174,9 +174,9 @@ pub fn build_create_client_and_send(
 ) -> Result<IBCEvent, Error> {
     let new_msg = build_create_client(dst_chain.clone(), src_chain)?;
 
-    let res = dst_chain.send_msgs(vec![new_msg.to_any::<RawMsgCreateClient>()])?;
-    assert!(!res.is_empty());
-    Ok(res[0].clone())
+    let mut res = dst_chain.send_msgs(vec![new_msg.to_any::<RawMsgCreateClient>()])?;
+    assert_eq!(res.len(), 1);
+    Ok(res.pop().unwrap())
 }
 
 /// Lower-level interface to create the message for updating a client to height `target_height`.
@@ -230,24 +230,18 @@ pub fn build_update_client_and_send(
     )?;
 
     let mut events = dst_chain.send_msgs(new_msgs)?;
-    assert!(!events.is_empty());
+    assert_eq!(events.len(), 1);
     Ok(events.pop().unwrap())
 }
 
 /// Tests the integration of crates `relayer` plus `relayer-cli` against crate `ibc`. These tests
 /// exercise various client methods (create, update, ForeignClient::new) using locally-running
 /// instances of chains built using `MockChain`.
-///
-/// ## Why are all these tests ignored?
-/// We ignore these tests as of #451, because the mock chain is not yet capable of producing
-/// transaction responses of correct types (the types should be similar to `tx_commit::Response`).
-/// Another problem is that `build_create_client_and_send` contains a Cosmos-specific method
-/// for parsing transaction response. Once this parsing stage is general enough for the Mock chain,
-/// these tests should require minimal changes to pass.
 #[cfg(test)]
 mod test {
     use std::str::FromStr;
 
+    use ibc::events::IBCEvent;
     use ibc::ics24_host::identifier::ClientId;
     use ibc::Height;
 
@@ -261,7 +255,6 @@ mod test {
 
     /// Basic test for the `build_create_client_and_send` method.
     #[test]
-    #[ignore = "cannot parse client creation against mock chain (#451), temp. disabled"]
     fn create_client_and_send_method() {
         let a_cfg = get_basic_chain_config("chain_a");
         let b_cfg = get_basic_chain_config("chain_b");
@@ -276,6 +269,7 @@ mod test {
             "build_create_client_and_send failed (chain a) with error {:?}",
             res
         );
+        assert!(matches!(res.unwrap(), IBCEvent::CreateClient(_)));
 
         // Create the client on chain b
         let res = build_create_client_and_send(b_chain.clone(), a_chain.clone());
@@ -284,11 +278,11 @@ mod test {
             "build_create_client_and_send failed (chain b) with error {:?}",
             res
         );
+        assert!(matches!(res.unwrap(), IBCEvent::CreateClient(_)));
     }
 
     /// Basic test for the `build_update_client_and_send` & `build_create_client_and_send` methods.
     #[test]
-    #[ignore = "cannot parse client creation against mock chain (#451), temp. disabled"]
     fn update_client_and_send_method() {
         let a_cfg = get_basic_chain_config("chain_a");
         let b_cfg = get_basic_chain_config("chain_b");
@@ -317,6 +311,7 @@ mod test {
             "build_create_client_and_send failed (chain a) with error {:?}",
             res
         );
+        assert!(matches!(res.as_ref().unwrap(), IBCEvent::CreateClient(_)));
         let a_client_id = extract_client_id(&res.unwrap()).unwrap().clone();
 
         // This should fail because the client on chain a already has the latest headers. Chain b,
@@ -339,6 +334,7 @@ mod test {
             "build_create_client_and_send failed (chain b) with error {:?}",
             res
         );
+        assert!(matches!(res.as_ref().unwrap(), IBCEvent::CreateClient(_)));
 
         // Remember the id of the client we created on chain b
         let b_client_id = extract_client_id(&res.unwrap()).unwrap().clone();
@@ -358,6 +354,8 @@ mod test {
                 "build_update_client_and_send failed (chain a) with error: {:?}",
                 res
             );
+            assert!(matches!(res.as_ref().unwrap(), IBCEvent::UpdateClient(_)));
+
             let a_height_current = a_chain.query_latest_height().unwrap();
             a_height_last = a_height_last.increment();
             assert_eq!(
@@ -372,6 +370,8 @@ mod test {
                 "build_update_client_and_send failed (chain b) with error: {:?}",
                 res
             );
+            assert!(matches!(res.as_ref().unwrap(), IBCEvent::UpdateClient(_)));
+
             let b_height_current = b_chain.query_latest_height().unwrap();
             b_height_last = b_height_last.increment();
             assert_eq!(
@@ -383,7 +383,6 @@ mod test {
 
     /// Tests for `ForeignClient::new()`.
     #[test]
-    #[ignore = "cannot parse client creation against mock chain (#451), temp. disabled"]
     fn foreign_client_create() {
         let a_cfg = get_basic_chain_config("chain_a");
         let b_cfg = get_basic_chain_config("chain_b");
@@ -429,7 +428,6 @@ mod test {
 
     /// Tests for `ForeignClient::update()`.
     #[test]
-    #[ignore = "cannot parse client creation against mock chain (#451), temp. disabled"]
     fn foreign_client_update() {
         let a_cfg = get_basic_chain_config("chain_a");
         let b_cfg = get_basic_chain_config("chain_b");