Brief relayer refactor to improve testing and add semantic dependenci…

…es (informalsystems#448)

* Starting refactor for Connection & Channel relayer objects

* Make chain handle object-safe via dyn_clone.

* Went back to clone() method [h/t Romain for suggesting clone_trait_object!]

* Refactoring Connection and ForeignClient

* Added tests for client update

* Added height check in ForeignClient update tests

* Removed useless test/code

* Added changelog entry

* Consolidating names (cf. review of informalsystems#364).

* Added more semantic deps

CHANGELOG.md

@@ -13,11 +13,13 @@
 - [relayer]
   - Mock chain (implementing IBC handlers) and integration against CLI ([#158])
   - Relayer tests for client update (ping pong) against MockChain ([#381])
+  - Relayer refactor to improve testing and add semantic dependencies ([#447]) 
 
 [#158]: https://github.com/informalsystems/ibc-rs/issues/158
 [#379]: https://github.com/informalsystems/ibc-rs/issues/379
 [#381]: https://github.com/informalsystems/ibc-rs/issues/381
 [#443]: https://github.com/informalsystems/ibc-rs/issues/443
+[#447]: https://github.com/informalsystems/ibc-rs/issues/447
 
 ## v0.0.5
 *December 2, 2020*

relayer-cli/src/commands/tx/client.rs

@@ -8,9 +8,7 @@ use crate::prelude::*;
 use relayer::chain::runtime::ChainRuntime;
 use relayer::chain::CosmosSDKChain;
 use relayer::config::ChainConfig;
-use relayer::foreign_client::{
-    build_create_client_and_send, build_update_client_and_send, ForeignClientConfig,
-};
+use relayer::foreign_client::{build_update_client_and_send, ForeignClient, ForeignClientConfig};
 
 #[derive(Clone, Command, Debug, Options)]
 pub struct TxCreateClientCmd {
@@ -52,13 +50,11 @@ impl Runnable for TxCreateClientCmd {
         let (src_chain, _) = ChainRuntime::<CosmosSDKChain>::spawn(src_chain_config).unwrap();
         let (dst_chain, _) = ChainRuntime::<CosmosSDKChain>::spawn(dst_chain_config).unwrap();
 
-        let res: Result<Vec<String>, Error> =
-            build_create_client_and_send(&dst_chain, &src_chain, &opts)
-                .map_err(|e| Kind::Tx.context(e).into());
+        let res = ForeignClient::new(dst_chain, src_chain, opts).map_err(|e| Kind::Tx.context(e));
 
         match res {
             Ok(receipt) => status_ok!("Success", "client created: {:?}", receipt),
-            Err(e) => status_err!("client create failed: {}", e),
+            Err(e) => status_err!("client create failed: {:?}", e),
         }
     }
 }
@@ -103,7 +99,7 @@ impl Runnable for TxUpdateClientCmd {
         let (dst_chain, _) = ChainRuntime::<CosmosSDKChain>::spawn(dst_chain_config).unwrap();
 
         let res: Result<Vec<String>, Error> =
-            build_update_client_and_send(&dst_chain, &src_chain, &opts)
+            build_update_client_and_send(dst_chain, src_chain, &opts)
                 .map_err(|e| Kind::Tx.context(e).into());
 
         match res {

relayer-cli/src/commands/tx/packet.rs

@@ -72,7 +72,7 @@ impl Runnable for TxRawPacketRecvCmd {
             ChainRuntime::<CosmosSDKChain>::spawn(opts.dst_chain_config.clone()).unwrap();
 
         let res: Result<Vec<String>, Error> =
-            build_and_send_recv_packet_messages(&dst_chain, &src_chain, &opts)
+            build_and_send_recv_packet_messages(dst_chain, src_chain, &opts)
                 .map_err(|e| Kind::Tx.context(e).into());
 
         match res {

relayer/Cargo.toml

@@ -39,6 +39,7 @@ itertools = "0.9.0"
 dyn-clonable = "0.9.0"
 tonic = "0.3.1"
 dirs-next = "2.0.0"
+dyn-clone = "1.0.3"
 
 [dependencies.tendermint]
 version = "=0.17.0-rc3"

relayer/src/chain/handle.rs

@@ -2,6 +2,8 @@ use std::sync::Arc;
 
 use crossbeam_channel as channel;
 
+use dyn_clone::DynClone;
+
 use ibc_proto::ibc::core::channel::v1::{
     PacketAckCommitment, QueryPacketCommitmentsRequest, QueryUnreceivedPacketsRequest,
 };
@@ -32,6 +34,7 @@ use crate::{error::Error, event::monitor::EventBatch};
 mod prod;
 
 pub use prod::ProdChainHandle;
+use std::fmt::Debug;
 
 pub type Subscription = channel::Receiver<Arc<EventBatch>>;
 
@@ -42,9 +45,9 @@ pub fn reply_channel<T>() -> (ReplyTo<T>, Reply<T>) {
     channel::bounded(1)
 }
 
-/// Inputs that a Handle may send to a Runtime.
+/// Requests that a `ChainHandle` may send to a `ChainRuntime`.
 #[derive(Clone, Debug)]
-pub enum HandleInput {
+pub enum ChainRequest {
     Terminate {
         reply_to: ReplyTo<()>,
     },
@@ -201,7 +204,10 @@ pub enum HandleInput {
     },
 }
 
-pub trait ChainHandle: Clone + Send + Sync {
+// Make `clone` accessible to a ChainHandle object
+dyn_clone::clone_trait_object!(ChainHandle);
+
+pub trait ChainHandle: DynClone + Send + Sync + Debug {
     fn id(&self) -> ChainId;
 
     fn query(&self, path: Path, height: Height, prove: bool) -> Result<QueryResponse, Error>;

relayer/src/chain/handle/prod.rs

@@ -23,7 +23,7 @@ use ibc::{
 // FIXME: the handle should not depend on tendermint-specific types
 use tendermint::account::Id as AccountId;
 
-use super::{reply_channel, ChainHandle, HandleInput, ReplyTo, Subscription};
+use super::{reply_channel, ChainHandle, ChainRequest, ReplyTo, Subscription};
 
 use crate::{
     chain::QueryResponse,
@@ -34,24 +34,30 @@ use crate::{
 
 #[derive(Debug, Clone)]
 pub struct ProdChainHandle {
+    /// Chain identifier
     chain_id: ChainId,
-    sender: channel::Sender<HandleInput>,
+
+    /// The handle's channel for sending requests to the runtime
+    runtime_sender: channel::Sender<ChainRequest>,
 }
 
 impl ProdChainHandle {
-    pub fn new(chain_id: ChainId, sender: channel::Sender<HandleInput>) -> Self {
-        Self { chain_id, sender }
+    pub fn new(chain_id: ChainId, sender: channel::Sender<ChainRequest>) -> Self {
+        Self {
+            chain_id,
+            runtime_sender: sender,
+        }
     }
 
     fn send<F, O>(&self, f: F) -> Result<O, Error>
     where
-        F: FnOnce(ReplyTo<O>) -> HandleInput,
+        F: FnOnce(ReplyTo<O>) -> ChainRequest,
         O: Debug,
     {
         let (sender, receiver) = reply_channel();
         let input = f(sender);
 
-        self.sender
+        self.runtime_sender
             .send(input)
             .map_err(|e| Kind::Channel.context(e))?;
 
@@ -64,26 +70,26 @@ impl ChainHandle for ProdChainHandle {
         self.chain_id.clone()
     }
 
-    fn subscribe(&self, _chain_id: ChainId) -> Result<Subscription, Error> {
-        self.send(|reply_to| HandleInput::Subscribe { reply_to })
-    }
-
     fn query(
         &self,
         path: ibc::ics24_host::Path,
         height: Height,
         prove: bool,
     ) -> Result<QueryResponse, Error> {
-        self.send(|reply_to| HandleInput::Query {
+        self.send(|reply_to| ChainRequest::Query {
             path,
             height,
             prove,
             reply_to,
         })
     }
 
+    fn subscribe(&self, _chain_id: ChainId) -> Result<Subscription, Error> {
+        self.send(|reply_to| ChainRequest::Subscribe { reply_to })
+    }
+
     fn send_msgs(&self, proto_msgs: Vec<prost_types::Any>) -> Result<Vec<String>, Error> {
-        self.send(|reply_to| HandleInput::SendMsgs {
+        self.send(|reply_to| ChainRequest::SendMsgs {
             proto_msgs,
             reply_to,
         })
@@ -94,19 +100,19 @@ impl ChainHandle for ProdChainHandle {
     // }
 
     fn get_minimal_set(&self, from: Height, to: Height) -> Result<Vec<AnyHeader>, Error> {
-        self.send(|reply_to| HandleInput::GetMinimalSet { from, to, reply_to })
+        self.send(|reply_to| ChainRequest::GetMinimalSet { from, to, reply_to })
     }
 
     fn get_signer(&self) -> Result<AccountId, Error> {
-        self.send(|reply_to| HandleInput::Signer { reply_to })
+        self.send(|reply_to| ChainRequest::Signer { reply_to })
     }
 
     fn get_key(&self) -> Result<KeyEntry, Error> {
-        self.send(|reply_to| HandleInput::Key { reply_to })
+        self.send(|reply_to| ChainRequest::Key { reply_to })
     }
 
     fn module_version(&self, port_id: &PortId) -> Result<String, Error> {
-        self.send(|reply_to| HandleInput::ModuleVersion {
+        self.send(|reply_to| ChainRequest::ModuleVersion {
             port_id: port_id.clone(),
             reply_to,
         })
@@ -124,15 +130,15 @@ impl ChainHandle for ProdChainHandle {
     // }
 
     fn query_latest_height(&self) -> Result<Height, Error> {
-        self.send(|reply_to| HandleInput::QueryLatestHeight { reply_to })
+        self.send(|reply_to| ChainRequest::QueryLatestHeight { reply_to })
     }
 
     fn query_client_state(
         &self,
         client_id: &ClientId,
         height: Height,
     ) -> Result<AnyClientState, Error> {
-        self.send(|reply_to| HandleInput::QueryClientState {
+        self.send(|reply_to| ChainRequest::QueryClientState {
             client_id: client_id.clone(),
             height,
             reply_to,
@@ -147,19 +153,19 @@ impl ChainHandle for ProdChainHandle {
     // ) -> Result<ChannelEnd, Error>;
 
     fn query_commitment_prefix(&self) -> Result<CommitmentPrefix, Error> {
-        self.send(|reply_to| HandleInput::QueryCommitmentPrefix { reply_to })
+        self.send(|reply_to| ChainRequest::QueryCommitmentPrefix { reply_to })
     }
 
     fn query_compatible_versions(&self) -> Result<Vec<String>, Error> {
-        self.send(|reply_to| HandleInput::QueryCompatibleVersions { reply_to })
+        self.send(|reply_to| ChainRequest::QueryCompatibleVersions { reply_to })
     }
 
     fn query_connection(
         &self,
         connection_id: &ConnectionId,
         height: Height,
     ) -> Result<ConnectionEnd, Error> {
-        self.send(|reply_to| HandleInput::QueryConnection {
+        self.send(|reply_to| ChainRequest::QueryConnection {
             connection_id: connection_id.clone(),
             height,
             reply_to,
@@ -172,7 +178,7 @@ impl ChainHandle for ProdChainHandle {
         channel_id: &ChannelId,
         height: Height,
     ) -> Result<ChannelEnd, Error> {
-        self.send(|reply_to| HandleInput::QueryChannel {
+        self.send(|reply_to| ChainRequest::QueryChannel {
             port_id: port_id.clone(),
             channel_id: channel_id.clone(),
             height,
@@ -185,7 +191,7 @@ impl ChainHandle for ProdChainHandle {
         client_id: &ClientId,
         height: Height,
     ) -> Result<(AnyClientState, MerkleProof), Error> {
-        self.send(|reply_to| HandleInput::ProvenClientState {
+        self.send(|reply_to| ChainRequest::ProvenClientState {
             client_id: client_id.clone(),
             height,
             reply_to,
@@ -197,7 +203,7 @@ impl ChainHandle for ProdChainHandle {
         connection_id: &ConnectionId,
         height: Height,
     ) -> Result<(ConnectionEnd, MerkleProof), Error> {
-        self.send(|reply_to| HandleInput::ProvenConnection {
+        self.send(|reply_to| ChainRequest::ProvenConnection {
             connection_id: connection_id.clone(),
             height,
             reply_to,
@@ -210,7 +216,7 @@ impl ChainHandle for ProdChainHandle {
         consensus_height: Height,
         height: Height,
     ) -> Result<(AnyConsensusState, MerkleProof), Error> {
-        self.send(|reply_to| HandleInput::ProvenClientConsensus {
+        self.send(|reply_to| ChainRequest::ProvenClientConsensus {
             client_id: client_id.clone(),
             consensus_height,
             height,
@@ -223,19 +229,19 @@ impl ChainHandle for ProdChainHandle {
         trusted_height: Height,
         target_height: Height,
     ) -> Result<AnyHeader, Error> {
-        self.send(|reply_to| HandleInput::BuildHeader {
+        self.send(|reply_to| ChainRequest::BuildHeader {
             trusted_height,
             target_height,
             reply_to,
         })
     }
 
     fn build_client_state(&self, height: Height) -> Result<AnyClientState, Error> {
-        self.send(|reply_to| HandleInput::BuildClientState { height, reply_to })
+        self.send(|reply_to| ChainRequest::BuildClientState { height, reply_to })
     }
 
     fn build_consensus_state(&self, height: Height) -> Result<AnyConsensusState, Error> {
-        self.send(|reply_to| HandleInput::BuildConsensusState { height, reply_to })
+        self.send(|reply_to| ChainRequest::BuildConsensusState { height, reply_to })
     }
 
     fn build_connection_proofs_and_client_state(
@@ -246,7 +252,7 @@ impl ChainHandle for ProdChainHandle {
         height: Height,
     ) -> Result<(Option<AnyClientState>, Proofs), Error> {
         self.send(
-            |reply_to| HandleInput::BuildConnectionProofsAndClientState {
+            |reply_to| ChainRequest::BuildConnectionProofsAndClientState {
                 message_type,
                 connection_id: connection_id.clone(),
                 client_id: client_id.clone(),
@@ -262,7 +268,7 @@ impl ChainHandle for ProdChainHandle {
         channel_id: &ChannelId,
         height: Height,
     ) -> Result<Proofs, Error> {
-        self.send(|reply_to| HandleInput::BuildChannelProofs {
+        self.send(|reply_to| ChainRequest::BuildChannelProofs {
             port_id: port_id.clone(),
             channel_id: channel_id.clone(),
             height,
@@ -277,7 +283,7 @@ impl ChainHandle for ProdChainHandle {
         sequence: u64,
         height: Height,
     ) -> Result<(Vec<u8>, MerkleProof), Error> {
-        self.send(|reply_to| HandleInput::ProvenPacketCommitment {
+        self.send(|reply_to| ChainRequest::ProvenPacketCommitment {
             port_id: port_id.clone(),
             channel_id: channel_id.clone(),
             sequence,
@@ -290,17 +296,17 @@ impl ChainHandle for ProdChainHandle {
         &self,
         request: QueryPacketCommitmentsRequest,
     ) -> Result<(Vec<PacketAckCommitment>, Height), Error> {
-        self.send(|reply_to| HandleInput::QueryPacketCommitments { request, reply_to })
+        self.send(|reply_to| ChainRequest::QueryPacketCommitments { request, reply_to })
     }
 
     fn query_unreceived_packets(
         &self,
         request: QueryUnreceivedPacketsRequest,
     ) -> Result<Vec<u64>, Error> {
-        self.send(|reply_to| HandleInput::QueryUnreceivedPackets { request, reply_to })
+        self.send(|reply_to| ChainRequest::QueryUnreceivedPackets { request, reply_to })
     }
 
     fn query_txs(&self, request: QueryPacketEventDataRequest) -> Result<Vec<IBCEvent>, Error> {
-        self.send(|reply_to| HandleInput::QueryPacketEventData { request, reply_to })
+        self.send(|reply_to| ChainRequest::QueryPacketEventData { request, reply_to })
     }
 }