diff --git a/CHANGELOG.md b/CHANGELOG.md index 7af6ac5ccf..9b5c59ce3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,11 @@ ### IMPROVEMENTS - Mock chain (implementing IBC handlers) and integration against CLI ([#158]) +- Relayer tests for client update (ping pong) against MockChain ([#381]) + [#158]: https://github.com/informalsystems/ibc-rs/issues/158 +[#381]: https://github.com/informalsystems/ibc-rs/issues/381 ## v0.0.5 *December 2, 2020* diff --git a/modules/src/ics07_tendermint/client_def.rs b/modules/src/ics07_tendermint/client_def.rs index 941928bef5..cdeb60f1be 100644 --- a/modules/src/ics07_tendermint/client_def.rs +++ b/modules/src/ics07_tendermint/client_def.rs @@ -24,7 +24,8 @@ impl ClientDef for TendermintClient { ) -> Result<(Self::ClientState, Self::ConsensusState), Box> { if client_state.latest_height() >= header.height() { return Err( - "received header height is lower than (or equal to) client latest height".into(), + format!("received header height ({:?}) is lower than (or equal to) client latest height ({:?})", + header.height(), client_state.latest_height).into(), ); } diff --git a/relayer-cli/src/commands/tx/client.rs b/relayer-cli/src/commands/tx/client.rs index 4e838fea2e..df4dcf8abd 100644 --- a/relayer-cli/src/commands/tx/client.rs +++ b/relayer-cli/src/commands/tx/client.rs @@ -52,8 +52,9 @@ impl Runnable for TxCreateClientCmd { let (src_chain, _) = ChainRuntime::::spawn(src_chain_config).unwrap(); let (dst_chain, _) = ChainRuntime::::spawn(dst_chain_config).unwrap(); - let res: Result = build_create_client_and_send(dst_chain, src_chain, &opts) - .map_err(|e| Kind::Tx.context(e).into()); + let res: Result = + build_create_client_and_send(&dst_chain, &src_chain, &opts) + .map_err(|e| Kind::Tx.context(e).into()); match res { Ok(receipt) => status_ok!("Success", "client created: {:?}", receipt), @@ -101,8 +102,9 @@ impl Runnable for TxUpdateClientCmd { let (src_chain, _) = ChainRuntime::::spawn(src_chain_config).unwrap(); let (dst_chain, _) = ChainRuntime::::spawn(dst_chain_config).unwrap(); - let res: Result = build_update_client_and_send(dst_chain, src_chain, &opts) - .map_err(|e| Kind::Tx.context(e).into()); + let res: Result = + build_update_client_and_send(&dst_chain, &src_chain, &opts) + .map_err(|e| Kind::Tx.context(e).into()); match res { Ok(receipt) => status_ok!("Success client updated: {:?}", receipt), diff --git a/relayer-cli/src/commands/v0.rs b/relayer-cli/src/commands/v0.rs index 451020b136..4b69711683 100644 --- a/relayer-cli/src/commands/v0.rs +++ b/relayer-cli/src/commands/v0.rs @@ -41,21 +41,21 @@ pub fn v0_task(config: &Config) -> Result<(), BoxError> { let path = &conn.paths.clone().ok_or("No paths configured")?[0]; - let connection = ConnectionConfig::new(&conn.clone())?; - let channel = ChannelConfig::new(&connection, &path)?; + let connection_cfg = ConnectionConfig::new(&conn.clone())?; + let channel_cfg = ChannelConfig::new(&connection_cfg, &path)?; let src_chain_config = config .chains .clone() .into_iter() - .find(|c| c.id == connection.src().chain_id().clone()) + .find(|c| c.id == connection_cfg.src().chain_id().clone()) .ok_or("Configuration for source chain not found")?; let dst_chain_config = config .chains .clone() .into_iter() - .find(|c| c.id == connection.dst().chain_id().clone()) + .find(|c| c.id == connection_cfg.dst().chain_id().clone()) .ok_or("Configuration for source chain not found")?; let (src_chain_handle, _) = ChainRuntime::::spawn(src_chain_config)?; @@ -64,7 +64,7 @@ pub fn v0_task(config: &Config) -> Result<(), BoxError> { Ok(channel_relay( src_chain_handle, dst_chain_handle, - connection, - channel, + connection_cfg, + channel_cfg, )?) } diff --git a/relayer/src/chain/handle.rs b/relayer/src/chain/handle.rs index f64951e9ce..2665f6ad9d 100644 --- a/relayer/src/chain/handle.rs +++ b/relayer/src/chain/handle.rs @@ -184,22 +184,12 @@ pub trait ChainHandle: Clone + Send + Sync { /// Send a transaction with `msgs` to chain. fn send_tx(&self, proto_msgs: Vec) -> Result; - // Inclusion proofs - // It might be good to include an inclusion proof method which abstracts over the light client - // to prove that a piece of data is stored on the chain - - // fn get_header(&self, height: Height) -> Result; - fn get_minimal_set(&self, from: Height, to: Height) -> Result, Error>; fn get_signer(&self) -> Result; fn get_key(&self) -> Result; - // fn submit(&self, transaction: EncodedTransaction) -> Result<(), Error>; - - // fn create_packet(&self, event: IBCEvent) -> Result; - fn module_version(&self, port_id: &PortId) -> Result; fn query_latest_height(&self) -> Result; diff --git a/relayer/src/chain/mock.rs b/relayer/src/chain/mock.rs index b665675cda..39ef49f42e 100644 --- a/relayer/src/chain/mock.rs +++ b/relayer/src/chain/mock.rs @@ -137,10 +137,18 @@ impl Chain for MockChain { fn build_header( &self, - _trusted_light_block: Self::LightBlock, - _target_light_block: Self::LightBlock, + trusted_light_block: Self::LightBlock, + target_light_block: Self::LightBlock, ) -> Result { - unimplemented!() + Ok(Self::Header { + signed_header: target_light_block.signed_header.clone(), + validator_set: target_light_block.validators, + trusted_height: Height::new( + self.id().version(), + u64::from(trusted_light_block.signed_header.header.height), + ), + trusted_validator_set: trusted_light_block.validators, + }) } fn query_latest_height(&self) -> Result { diff --git a/relayer/src/channel.rs b/relayer/src/channel.rs index aa3148e6e2..913471d1f5 100644 --- a/relayer/src/channel.rs +++ b/relayer/src/channel.rs @@ -514,8 +514,8 @@ pub fn build_chan_try( // Build message to update client on destination let mut msgs = build_update_client( - dst_chain.clone(), - src_chain.clone(), + &dst_chain, + &src_chain, &dest_connection.client_id(), ics_target_height, )?; @@ -611,8 +611,8 @@ pub fn build_chan_ack( // Build message to update client on destination let mut msgs = build_update_client( - dst_chain.clone(), - src_chain.clone(), + &dst_chain, + &src_chain, &dest_connection.client_id(), ics_target_height, )?; @@ -694,8 +694,8 @@ pub fn build_chan_confirm( // Build message to update client on destination let mut msgs = build_update_client( - dst_chain.clone(), - src_chain.clone(), + &dst_chain, + &src_chain, &dest_connection.client_id(), ics_target_height, )?; diff --git a/relayer/src/connection.rs b/relayer/src/connection.rs index df024cae19..68542a039c 100644 --- a/relayer/src/connection.rs +++ b/relayer/src/connection.rs @@ -468,8 +468,8 @@ pub fn build_conn_try( // TODO - add check if it is required let src_client_target_height = dst_chain.query_latest_height()?; let client_msgs = build_update_client( - src_chain.clone(), - dst_chain.clone(), + &src_chain, + &dst_chain, &opts.src().client_id(), src_client_target_height, )?; @@ -479,8 +479,8 @@ pub fn build_conn_try( let ics_target_height = src_chain.query_latest_height()?; let mut msgs = build_update_client( - dst_chain.clone(), - src_chain.clone(), + &dst_chain, + &src_chain, &opts.dst().client_id(), ics_target_height, )?; @@ -568,8 +568,8 @@ pub fn build_conn_ack( // TODO - add check if it is required let src_client_target_height = dst_chain.query_latest_height()?; let client_msgs = build_update_client( - src_chain.clone(), - dst_chain.clone(), + &src_chain, + &dst_chain, &opts.src().client_id(), src_client_target_height, )?; @@ -579,8 +579,8 @@ pub fn build_conn_ack( let ics_target_height = src_chain.query_latest_height()?; let mut msgs = build_update_client( - dst_chain.clone(), - src_chain.clone(), + &dst_chain, + &src_chain, &opts.dst().client_id(), ics_target_height, )?; @@ -659,8 +659,8 @@ pub fn build_conn_confirm( let ics_target_height = src_chain.query_latest_height()?; let mut msgs = build_update_client( - dst_chain.clone(), - src_chain.clone(), + &dst_chain, + &src_chain, &opts.dst().client_id(), ics_target_height, )?; diff --git a/relayer/src/foreign_client.rs b/relayer/src/foreign_client.rs index cfc7285f61..f90582c5d8 100644 --- a/relayer/src/foreign_client.rs +++ b/relayer/src/foreign_client.rs @@ -1,9 +1,6 @@ use prost_types::Any; use thiserror::Error; -use ibc_proto::ibc::core::client::v1::MsgCreateClient as RawMsgCreateClient; -use ibc_proto::ibc::core::client::v1::MsgUpdateClient as RawMsgUpdateClient; - use ibc::ics02_client::header::Header; use ibc::ics02_client::msgs::create_client::MsgCreateAnyClient; use ibc::ics02_client::msgs::update_client::MsgUpdateAnyClient; @@ -12,6 +9,8 @@ use ibc::ics02_client::state::ConsensusState; use ibc::ics24_host::identifier::{ChainId, ClientId}; use ibc::tx_msg::Msg; use ibc::Height; +use ibc_proto::ibc::core::client::v1::MsgCreateClient as RawMsgCreateClient; +use ibc_proto::ibc::core::client::v1::MsgUpdateClient as RawMsgUpdateClient; use crate::chain::handle::ChainHandle; use crate::error::{Error, Kind}; @@ -27,7 +26,10 @@ pub enum ForeignClientError { #[derive(Clone, Debug)] pub struct ForeignClientConfig { + /// The identifier of the chain which hosts this client chain: ChainId, + + /// The client's identifier id: ClientId, } @@ -48,7 +50,7 @@ impl ForeignClientConfig { } } -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct ForeignClient { config: ForeignClientConfig, } @@ -60,8 +62,8 @@ impl ForeignClient { /// TODO: what are the pre-conditions for success? /// Is it enough to have a "live" handle to each of `dst_chain` and `src_chain` chains? pub fn new( - dst_chain: impl ChainHandle, - src_chain: impl ChainHandle, + dst_chain: &impl ChainHandle, + src_chain: &impl ChainHandle, config: ForeignClientConfig, ) -> Result { let done = '\u{1F36D}'; @@ -130,8 +132,8 @@ impl ForeignClient { } pub fn build_create_client( - dst_chain: impl ChainHandle, - src_chain: impl ChainHandle, + dst_chain: &impl ChainHandle, + src_chain: &impl ChainHandle, dst_client_id: &ClientId, ) -> Result { // Verify that the client has not been created already, i.e the destination chain does not @@ -163,18 +165,18 @@ pub fn build_create_client( } pub fn build_create_client_and_send( - dst_chain: impl ChainHandle, - src_chain: impl ChainHandle, + dst_chain: &impl ChainHandle, + src_chain: &impl ChainHandle, opts: &ForeignClientConfig, ) -> Result { - let new_msg = build_create_client(dst_chain.clone(), src_chain, opts.client_id())?; + let new_msg = build_create_client(dst_chain, src_chain, opts.client_id())?; Ok(dst_chain.send_tx(vec![new_msg.to_any::()])?) } pub fn build_update_client( - dst_chain: impl ChainHandle, - src_chain: impl ChainHandle, + dst_chain: &impl ChainHandle, + src_chain: &impl ChainHandle, dst_client_id: &ClientId, target_height: Height, ) -> Result, Error> { @@ -198,13 +200,13 @@ pub fn build_update_client( } pub fn build_update_client_and_send( - dst_chain: impl ChainHandle, - src_chain: impl ChainHandle, + dst_chain: &impl ChainHandle, + src_chain: &impl ChainHandle, opts: &ForeignClientConfig, ) -> Result { let new_msgs = build_update_client( - dst_chain.clone(), - src_chain.clone(), + dst_chain, + src_chain, opts.client_id(), src_chain.query_latest_height()?, )?; @@ -212,51 +214,202 @@ pub fn build_update_client_and_send( Ok(dst_chain.send_tx(new_msgs)?) } +/// 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`. #[cfg(test)] mod test { use std::str::FromStr; use ibc::ics24_host::identifier::ClientId; + use ibc::Height; + use crate::chain::handle::ChainHandle; use crate::chain::mock::test_utils::get_basic_chain_config; use crate::chain::mock::MockChain; use crate::chain::runtime::ChainRuntime; use crate::foreign_client::{ - build_create_client_and_send, build_update_client_and_send, ForeignClientConfig, + build_create_client_and_send, build_update_client_and_send, ForeignClient, + ForeignClientConfig, }; + /// Basic test for the `build_create_client_and_send` method. #[test] - fn test_build_create_client_and_send() { - let client_id = ClientId::from_str("client_on_a_forb").unwrap(); + fn create_client_and_send_method() { + let a_client_id = ClientId::from_str("client_on_a_forb").unwrap(); + let b_client_id = ClientId::from_str("client_on_b_fora").unwrap(); let a_cfg = get_basic_chain_config("chain_a"); let b_cfg = get_basic_chain_config("chain_b"); - let opts = ForeignClientConfig::new(&a_cfg.id, &client_id); + let a_opts = ForeignClientConfig::new(&a_cfg.id, &a_client_id); + let b_opts = ForeignClientConfig::new(&b_cfg.id, &b_client_id); let (a_chain, _) = ChainRuntime::::spawn(a_cfg).unwrap(); let (b_chain, _) = ChainRuntime::::spawn(b_cfg).unwrap(); - let res = build_create_client_and_send(a_chain, b_chain, &opts); + // Create the client on chain a + let res = build_create_client_and_send(&a_chain, &b_chain, &a_opts); + assert!( + res.is_ok(), + "build_create_client_and_send failed (chain a) with error {:?}", + res + ); + + // Double client creation should be forbidden. + let res = build_create_client_and_send(&a_chain, &b_chain, &a_opts); + assert!( + res.is_err(), + "build_create_client_and_send double client creation should have failed!", + ); + + // Create the client on chain b + let res = build_create_client_and_send(&b_chain, &a_chain, &b_opts); assert!( res.is_ok(), - "build_create_client_and_send failed with error {:?}", + "build_create_client_and_send failed (chain b) with error {:?}", + res + ); + + // Test double creation for chain b + let res = build_create_client_and_send(&b_chain, &a_chain, &b_opts); + assert!( + res.is_err(), + "build_create_client_and_send failed (chain b) with error {:?}", res ); } + /// Basic test for the `build_update_client_and_send` & `build_create_client_and_send` methods. #[test] - fn test_build_update_client_and_send() { - let client_id = ClientId::from_str("client_on_a_forb").unwrap(); + fn update_client_and_send_method() { let a_cfg = get_basic_chain_config("chain_a"); let b_cfg = get_basic_chain_config("chain_b"); - let opts = ForeignClientConfig::new(&a_cfg.id, &client_id); + let a_client_id = ClientId::from_str("client_on_a_forb").unwrap(); + let a_opts = ForeignClientConfig::new(&a_cfg.id, &a_client_id); + let b_client_id = ClientId::from_str("client_on_b_fora").unwrap(); + let b_opts = ForeignClientConfig::new(&b_cfg.id, &b_client_id); + + // The number of ping-pong iterations + let num_iterations = 3; let (a_chain, _) = ChainRuntime::::spawn(a_cfg).unwrap(); let (b_chain, _) = ChainRuntime::::spawn(b_cfg).unwrap(); - let res = build_update_client_and_send(a_chain, b_chain, &opts); + // This action should fail because no client exists (yet) + let res = build_update_client_and_send(&a_chain, &b_chain, &a_opts); assert!( res.is_err(), "build_update_client_and_send was supposed to fail (no client existed)" ); + + // Remember b's height. + let b_height_start = b_chain.query_latest_height().unwrap(); + + // Create a client on chain a + let res = build_create_client_and_send(&a_chain, &b_chain, &a_opts); + assert!( + res.is_ok(), + "build_create_client_and_send failed (chain a) with error {:?}", + res + ); + + // This should fail because the client on chain a already has the latest headers. Chain b, + // the source chain for the client on a, is at the same height where it was when the client + // was created, so an update should fail here. + let res = build_update_client_and_send(&a_chain, &b_chain, &a_opts); + + assert!( + res.is_err(), + "build_update_client_and_send was supposed to fail", + ); + let b_height_last = b_chain.query_latest_height().unwrap(); + assert_eq!(b_height_last, b_height_start); + + // Create a client on chain b + let res = build_create_client_and_send(&b_chain, &a_chain, &b_opts); + assert!( + res.is_ok(), + "build_create_client_and_send failed (chain b) with error {:?}", + res + ); + // Chain b should have advanced + let mut b_height_last = b_chain.query_latest_height().unwrap(); + assert_eq!(b_height_last, b_height_start.increment()); + + // Remember the current height of chain a + let mut a_height_last = a_chain.query_latest_height().unwrap(); + + // Now we can update both clients -- a ping pong, similar to ICS18 `client_update_ping_pong` + for _i in 1..num_iterations { + let res = build_update_client_and_send(&a_chain, &b_chain, &a_opts); + assert!( + res.is_ok(), + "build_update_client_and_send failed (chain a) with error: {:?}", + res + ); + let a_height_current = a_chain.query_latest_height().unwrap(); + a_height_last = a_height_last.increment(); + assert_eq!( + a_height_last, a_height_current, + "after client update, chain a did not advance" + ); + + // And also update the client on chain b. + let res = build_update_client_and_send(&b_chain, &a_chain, &b_opts); + assert!( + res.is_ok(), + "build_update_client_and_send failed (chain b) with error: {:?}", + res + ); + let b_height_current = b_chain.query_latest_height().unwrap(); + b_height_last = b_height_last.increment(); + assert_eq!( + b_height_last, b_height_current, + "after client update, chain b did not advance" + ); + } + } + + /// Tests for `ForeignClient::new()`. + #[test] + fn foreign_client_create() { + let a_cfg = get_basic_chain_config("chain_a"); + let b_cfg = get_basic_chain_config("chain_b"); + let a_client_id = ClientId::from_str("client_on_a_forb").unwrap(); + let a_opts = ForeignClientConfig::new(&a_cfg.id, &a_client_id); + let b_client_id = ClientId::from_str("client_on_b_fora").unwrap(); + let b_opts = ForeignClientConfig::new(&b_cfg.id, &b_client_id); + + let (a_chain, _) = ChainRuntime::::spawn(a_cfg).unwrap(); + let (b_chain, _) = ChainRuntime::::spawn(b_cfg).unwrap(); + + // Instantiate the foreign clients on the two chains. + let client_on_a = ForeignClient::new(&a_chain, &b_chain, a_opts); + assert!( + client_on_a.is_ok(), + "Client creation (on chain a) failed with error: {:?}", + client_on_a + ); + + let client_on_b = ForeignClient::new(&b_chain, &a_chain, b_opts); + assert!( + client_on_b.is_ok(), + "Client creation (on chain a) failed with error: {:?}", + client_on_b + ); + + // Now that the clients exists, we should be able to query its state + let b_client_state = b_chain.query_client_state(&b_client_id, Height::default()); + assert!( + b_client_state.is_ok(), + "Client query (on chain b) failed with error: {:?}", + b_client_state + ); + + let a_client_state = a_chain.query_client_state(&a_client_id, Height::default()); + assert!( + a_client_state.is_ok(), + "Client query (on chain a) failed with error: {:?}", + a_client_state + ); } } diff --git a/relayer/src/relay.rs b/relayer/src/relay.rs index 2bdb81ab39..40e04c34eb 100644 --- a/relayer/src/relay.rs +++ b/relayer/src/relay.rs @@ -15,15 +15,15 @@ pub fn channel_relay( ) -> Result<(), BoxError> { // Instantiate the foreign client on the source chain. let client_on_a = ForeignClient::new( - a_chain_handle.clone(), - b_chain_handle.clone(), + &a_chain_handle, + &b_chain_handle, ForeignClientConfig::new(conn_cfg.a_end().chain_id(), conn_cfg.a_end().client_id()), )?; // Instantiate the foreign client on the destination chain. let client_on_b = ForeignClient::new( - b_chain_handle.clone(), - a_chain_handle.clone(), + &b_chain_handle, + &a_chain_handle, ForeignClientConfig::new(conn_cfg.b_end().chain_id(), conn_cfg.b_end().client_id()), )?;