From 72242cf152dfedd65e99af05d66ae8abd2fa6532 Mon Sep 17 00:00:00 2001 From: Adi Seredinschi Date: Tue, 26 Jan 2021 18:46:31 +0100 Subject: [PATCH 1/6] Include cause in Failed errors (#555). Double-checking 'required' tag. --- relayer-cli/src/commands/query/channel.rs | 6 +++--- relayer-cli/src/commands/query/client.rs | 4 ++-- relayer-cli/src/commands/tx/transfer.rs | 9 +++++---- relayer-cli/src/conclude.rs | 12 ++++++++++++ relayer/src/channel.rs | 2 +- relayer/src/connection.rs | 4 ++-- 6 files changed, 25 insertions(+), 12 deletions(-) diff --git a/relayer-cli/src/commands/query/channel.rs b/relayer-cli/src/commands/query/channel.rs index 31db1dfe84..b53b574e03 100644 --- a/relayer-cli/src/commands/query/channel.rs +++ b/relayer-cli/src/commands/query/channel.rs @@ -18,13 +18,13 @@ use crate::prelude::*; #[derive(Clone, Command, Debug, Options)] pub struct QueryChannelEndCmd { - #[options(free, help = "identifier of the chain to query")] + #[options(free, required, help = "identifier of the chain to query")] chain_id: Option, - #[options(free, help = "identifier of the port to query")] + #[options(free, required, help = "identifier of the port to query")] port_id: Option, - #[options(free, help = "identifier of the channel to query")] + #[options(free, required, help = "identifier of the channel to query")] channel_id: Option, #[options(help = "height of the state to query", short = "h")] diff --git a/relayer-cli/src/commands/query/client.rs b/relayer-cli/src/commands/query/client.rs index 168b260bab..151fefcb5c 100644 --- a/relayer-cli/src/commands/query/client.rs +++ b/relayer-cli/src/commands/query/client.rs @@ -22,10 +22,10 @@ use crate::prelude::*; /// Query client state command #[derive(Clone, Command, Debug, Options)] pub struct QueryClientStateCmd { - #[options(free, help = "identifier of the chain to query")] + #[options(free, required, help = "identifier of the chain to query")] chain_id: Option, - #[options(free, help = "identifier of the client to query")] + #[options(free, required, help = "identifier of the client to query")] client_id: Option, #[options(help = "the chain height which this query should reflect", short = "h")] diff --git a/relayer-cli/src/commands/tx/transfer.rs b/relayer-cli/src/commands/tx/transfer.rs index 1adea1cd0d..1b41224be9 100644 --- a/relayer-cli/src/commands/tx/transfer.rs +++ b/relayer-cli/src/commands/tx/transfer.rs @@ -17,20 +17,21 @@ use crate::prelude::*; #[derive(Clone, Command, Debug, Options)] pub struct TxRawSendPacketCmd { - #[options(free, help = "identifier of the source chain")] + #[options(free, required, help = "identifier of the source chain")] src_chain_id: ChainId, - #[options(free, help = "identifier of the destination chain")] + #[options(free, required, help = "identifier of the destination chain")] dest_chain_id: ChainId, - #[options(free, help = "identifier of the source port")] + #[options(free, required, help = "identifier of the source port")] src_port_id: PortId, - #[options(free, help = "identifier of the source channel")] + #[options(free, required, help = "identifier of the source channel")] src_channel_id: ChannelId, #[options( free, + required, help = "amount of coins (samoleans, by default) to send (e.g. `100000`)" )] amount: u64, diff --git a/relayer-cli/src/conclude.rs b/relayer-cli/src/conclude.rs index 900fa79ddb..3299741f8a 100644 --- a/relayer-cli/src/conclude.rs +++ b/relayer-cli/src/conclude.rs @@ -26,6 +26,18 @@ //! Output::error(format!("{}", e)).exit(); //! ``` //! +//! #### Note: +//! The resulting output that this approach generates is determined by the 'error' annotation given +//! to the error object `Kind::Query`. If this error object comprises any positional arguments, +//! e.g. as achieved by `Query(String, String)`, then it is important to cover these arguments +//! in the `error` annotation, for instance: +//! ```compile_fail +//! #[derive(Debug, Error)] +//! pub enum Kind { +//! #[error("failed with underlying causes: {0}, {1}")] +//! Query(String, String), // ... +//! ``` +//! //! - Exit from a query/tx with success: //! //! ```compile_fail diff --git a/relayer/src/channel.rs b/relayer/src/channel.rs index 793803521a..505c2d135c 100644 --- a/relayer/src/channel.rs +++ b/relayer/src/channel.rs @@ -25,7 +25,7 @@ use crate::relay::MAX_ITER; #[derive(Debug, Error)] pub enum ChannelError { - #[error("failed")] + #[error("failed with underlying cause: {0}")] Failed(String), #[error("failed during an operation on client ({0}) hosted by chain ({1}) with error: {2}")] diff --git a/relayer/src/connection.rs b/relayer/src/connection.rs index 74acb2a830..a65ef74b65 100644 --- a/relayer/src/connection.rs +++ b/relayer/src/connection.rs @@ -25,10 +25,10 @@ use crate::relay::MAX_ITER; #[derive(Debug, Error)] pub enum ConnectionError { - #[error("Failed")] + #[error("failed with underlying cause: {0}")] Failed(String), - #[error("constructor parameters do not match")] + #[error("constructor parameters do not match: underlying error: {0}")] ConstructorFailed(String), #[error("failed during a query to chain id {0} due to underlying error: {1}")] From f3fba1b9058bc4ef3783544f77459191c68b0242 Mon Sep 17 00:00:00 2001 From: Adi Seredinschi Date: Tue, 26 Jan 2021 19:01:29 +0100 Subject: [PATCH 2/6] Finished adding 'required' tag to commands --- relayer-cli/src/commands/query/client.rs | 30 ++++++++++++++------ relayer-cli/src/commands/query/connection.rs | 17 ++++++----- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/relayer-cli/src/commands/query/client.rs b/relayer-cli/src/commands/query/client.rs index 151fefcb5c..1d1a96ab6b 100644 --- a/relayer-cli/src/commands/query/client.rs +++ b/relayer-cli/src/commands/query/client.rs @@ -19,6 +19,7 @@ use crate::conclude::Output; use crate::error::{Error, Kind}; use crate::prelude::*; +// TODO: refactor commands: why is chain_id an `Option`? simpler to give it `ChainId` type. /// Query client state command #[derive(Clone, Command, Debug, Options)] pub struct QueryClientStateCmd { @@ -31,7 +32,10 @@ pub struct QueryClientStateCmd { #[options(help = "the chain height which this query should reflect", short = "h")] height: Option, - #[options(help = "whether proof is required", short = "p")] + #[options( + help = "whether proof is required; default: false (no proof)", + short = "p" + )] proof: Option, } @@ -97,16 +101,24 @@ impl Runnable for QueryClientStateCmd { /// Query client consensus command #[derive(Clone, Command, Debug, Options)] pub struct QueryClientConsensusCmd { - #[options(free, help = "identifier of the chain to query")] + #[options(free, required, help = "identifier of the chain to query")] chain_id: Option, - #[options(free, help = "identifier of the client to query")] + #[options(free, required, help = "identifier of the client to query")] client_id: Option, - #[options(free, help = "epoch of the client's consensus state to query")] + #[options( + free, + required, + help = "epoch of the client's consensus state to query" + )] consensus_epoch: Option, - #[options(free, help = "height of the client's consensus state to query")] + #[options( + free, + required, + help = "height of the client's consensus state to query" + )] consensus_height: Option, #[options(help = "the chain height which this query should reflect", short = "h")] @@ -205,7 +217,7 @@ fn validate_common_options( .ok_or_else(|| "missing chain parameter".to_string())?; let chain_config = config .find_chain(&chain_id) - .ok_or_else(|| "missing chain in configuration".to_string())?; + .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; let client_id = client_id .as_ref() @@ -219,10 +231,10 @@ fn validate_common_options( /// Query client connections command #[derive(Clone, Command, Debug, Options)] pub struct QueryClientConnectionsCmd { - #[options(free, help = "identifier of the chain to query")] + #[options(free, required, help = "identifier of the chain to query")] chain_id: Option, - #[options(free, help = "identifier of the client to query")] + #[options(free, required, help = "identifier of the client to query")] client_id: Option, #[options(help = "the chain height which this query should reflect", short = "h")] @@ -246,7 +258,7 @@ impl QueryClientConnectionsCmd { .ok_or_else(|| "missing chain identifier".to_string())?; let chain_config = config .find_chain(&chain_id) - .ok_or_else(|| "missing chain configuration".to_string())?; + .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; let client_id = self .client_id diff --git a/relayer-cli/src/commands/query/connection.rs b/relayer-cli/src/commands/query/connection.rs index 77b099b396..713c30b238 100644 --- a/relayer-cli/src/commands/query/connection.rs +++ b/relayer-cli/src/commands/query/connection.rs @@ -17,16 +17,19 @@ use crate::prelude::*; #[derive(Clone, Command, Debug, Options)] pub struct QueryConnectionEndCmd { - #[options(free, help = "identifier of the chain to query")] + #[options(free, required, help = "identifier of the chain to query")] chain_id: Option, - #[options(free, help = "identifier of the connection to query")] + #[options(free, required, help = "identifier of the connection to query")] connection_id: Option, #[options(help = "height of the state to query", short = "h")] height: Option, - #[options(help = "whether proof is required", short = "p")] + #[options( + help = "whether proof is required; default: false (no proof)", + short = "p" + )] proof: Option, } @@ -49,7 +52,7 @@ impl QueryConnectionEndCmd { let chain_config = config .find_chain(&chain_id) - .ok_or_else(|| "missing chain configuration".to_string())?; + .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; let connection_id = self .connection_id @@ -101,10 +104,10 @@ impl Runnable for QueryConnectionEndCmd { /// `cargo run --bin relayer -- -c simple_config.toml query connection channels ibc-0 connection-0` #[derive(Clone, Command, Debug, Options)] pub struct QueryConnectionChannelsCmd { - #[options(free, help = "identifier of the chain to query")] + #[options(free, required, help = "identifier of the chain to query")] chain_id: Option, - #[options(free, help = "identifier of the connection to query")] + #[options(free, required, help = "identifier of the connection to query")] connection_id: Option, } @@ -124,7 +127,7 @@ impl QueryConnectionChannelsCmd { .ok_or_else(|| "no chain chain identifier provided".to_string())?; let chain_config = config .find_chain(&chain_id) - .ok_or_else(|| "missing chain configuration for the given chain id".to_string())?; + .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; let connection_id = self .connection_id From ffefc34c148bd9e56874ffa9250242a438012716 Mon Sep 17 00:00:00 2001 From: Adi Seredinschi Date: Wed, 27 Jan 2021 10:17:00 +0100 Subject: [PATCH 3/6] Last nits around error messages. --- relayer-cli/src/commands/query/channel.rs | 2 +- relayer-cli/src/commands/query/client.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/relayer-cli/src/commands/query/channel.rs b/relayer-cli/src/commands/query/channel.rs index b53b574e03..fd4653ce7d 100644 --- a/relayer-cli/src/commands/query/channel.rs +++ b/relayer-cli/src/commands/query/channel.rs @@ -53,7 +53,7 @@ impl QueryChannelEndCmd { .ok_or_else(|| "missing chain identifier".to_string())?; let chain_config = config .find_chain(&chain_id) - .ok_or_else(|| "missing chain configuration".to_string())?; + .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; let port_id = self .port_id diff --git a/relayer-cli/src/commands/query/client.rs b/relayer-cli/src/commands/query/client.rs index 1d1a96ab6b..90b948d969 100644 --- a/relayer-cli/src/commands/query/client.rs +++ b/relayer-cli/src/commands/query/client.rs @@ -20,6 +20,8 @@ use crate::error::{Error, Kind}; use crate::prelude::*; // TODO: refactor commands: why is chain_id an `Option`? simpler to give it `ChainId` type. +// see the tx/client.rs or tx/packet.rs for simpler approaches. + /// Query client state command #[derive(Clone, Command, Debug, Options)] pub struct QueryClientStateCmd { @@ -214,7 +216,7 @@ fn validate_common_options( ) -> Result<(ChainConfig, ClientId), String> { let chain_id = chain_id .clone() - .ok_or_else(|| "missing chain parameter".to_string())?; + .ok_or_else(|| "missing chain identifier".to_string())?; let chain_config = config .find_chain(&chain_id) .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; From b278859874fa598a65564be1efcddbdcbca7d8a7 Mon Sep 17 00:00:00 2001 From: Adi Seredinschi Date: Wed, 27 Jan 2021 10:19:41 +0100 Subject: [PATCH 4/6] Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2b371ad2f..291b3f625a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - [relayer-cli] - Implement command to query the channels associated with a connection ([#505]) - JSON output for queries and txs ([#500]) + - Added 'required' annotation for CLIs queries & txs; better error display ([#555]) - [relayer] - Added retry mechanism, restructured relayer ([#519]) @@ -59,6 +60,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 +[#555]: https://github.com/informalsystems/ibc-rs/issues/555 ## v0.0.6 From 4d78ed07a784c91c06d02a9c8e1e8ff6c5d592cc Mon Sep 17 00:00:00 2001 From: Adi Seredinschi Date: Wed, 27 Jan 2021 15:39:08 +0100 Subject: [PATCH 5/6] Change doctests in conclude to ignore --- relayer-cli/src/conclude.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/relayer-cli/src/conclude.rs b/relayer-cli/src/conclude.rs index 3299741f8a..de7fcf8f85 100644 --- a/relayer-cli/src/conclude.rs +++ b/relayer-cli/src/conclude.rs @@ -2,14 +2,14 @@ //! from a CLI command. The main use-case for this module is to provide a consistent output for //! queries and transactions. //! -//! The examples below rely on crate-private methods (for this reason, doctests do not compile). +//! The examples below rely on crate-private methods (for this reason, doctests are ignored). //! They are intended for contributors to crate `relayer-cli`, and _not_ for users of this binary. //! //! ## Examples on how to use the quick-access constructors: //! //! - Exit from a query/tx with a `String` error: //! -//! ```compile_fail +//! ```ignore //! let e = String::from("error message"); //! Output::error(e).exit(); //! // or as an alternative: @@ -21,7 +21,7 @@ //! better to simplify the output and only write out the chain of error sources, which we can //! achieve with `format!("{}", e)`. The complete solution is as follows: //! -//! ```compile_fail +//! ```ignore //! let e: Error = Kind::Query.into(); //! Output::error(format!("{}", e)).exit(); //! ``` @@ -31,7 +31,7 @@ //! to the error object `Kind::Query`. If this error object comprises any positional arguments, //! e.g. as achieved by `Query(String, String)`, then it is important to cover these arguments //! in the `error` annotation, for instance: -//! ```compile_fail +//! ```ignore //! #[derive(Debug, Error)] //! pub enum Kind { //! #[error("failed with underlying causes: {0}, {1}")] @@ -40,14 +40,14 @@ //! //! - Exit from a query/tx with success: //! -//! ```compile_fail +//! ```ignore //! let cs = ChannelEnd::default(); //! Output::success(cs).exit(); //! ``` //! //! - Exit from a query/tx with success and multiple objects in the result: //! -//! ```compile_fail +//! ```ignore //! let h = Height::default(); //! let end = ConnectionEnd::default(); //! Output::success(h).with_result(end).exit(); From ecb53189a66496c714ebec00f1769132776e191f Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Thu, 28 Jan 2021 13:03:08 +0100 Subject: [PATCH 6/6] Remove Option from chain_id parameter in client queries --- relayer-cli/src/commands/query/client.rs | 43 ++++++------------------ 1 file changed, 10 insertions(+), 33 deletions(-) diff --git a/relayer-cli/src/commands/query/client.rs b/relayer-cli/src/commands/query/client.rs index b6c05f2430..acc193ee9b 100644 --- a/relayer-cli/src/commands/query/client.rs +++ b/relayer-cli/src/commands/query/client.rs @@ -26,7 +26,7 @@ use crate::prelude::*; #[derive(Clone, Command, Debug, Options)] pub struct QueryClientStateCmd { #[options(free, required, help = "identifier of the chain to query")] - chain_id: Option, + chain_id: ChainId, #[options(free, required, help = "identifier of the client to query")] client_id: Option, @@ -102,7 +102,7 @@ impl Runnable for QueryClientStateCmd { #[derive(Clone, Command, Debug, Options)] pub struct QueryClientConsensusCmd { #[options(free, required, help = "identifier of the chain to query")] - chain_id: Option, + chain_id: ChainId, #[options(free, required, help = "identifier of the client to query")] client_id: Option, @@ -206,13 +206,10 @@ impl Runnable for QueryClientConsensusCmd { } fn validate_common_options( - chain_id: &Option, + chain_id: &ChainId, client_id: &Option, config: &Config, ) -> Result<(ChainConfig, ClientId), String> { - let chain_id = chain_id - .clone() - .ok_or_else(|| "missing chain identifier".to_string())?; let chain_config = config .find_chain(&chain_id) .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; @@ -230,7 +227,7 @@ fn validate_common_options( #[derive(Clone, Command, Debug, Options)] pub struct QueryClientConnectionsCmd { #[options(free, required, help = "identifier of the chain to query")] - chain_id: Option, + chain_id: ChainId, #[options(free, required, help = "identifier of the client to query")] client_id: Option, @@ -250,13 +247,9 @@ impl QueryClientConnectionsCmd { &self, config: &Config, ) -> Result<(ChainConfig, QueryClientConnectionsOptions), String> { - let chain_id = self - .chain_id - .clone() - .ok_or_else(|| "missing chain identifier".to_string())?; let chain_config = config - .find_chain(&chain_id) - .ok_or_else(|| format!("chain '{}' not found in configuration file", chain_id))?; + .find_chain(&self.chain_id) + .ok_or_else(|| format!("chain '{}' not found in configuration file", self.chain_id))?; let client_id = self .client_id @@ -314,7 +307,7 @@ mod tests { #[test] fn parse_query_state_parameters() { let default_params = QueryClientStateCmd { - chain_id: Some("ibc-0".to_string().parse().unwrap()), + chain_id: "ibc-0".to_string().parse().unwrap(), client_id: Some("ibconeclient".to_string().parse().unwrap()), height: None, proof: None, @@ -332,18 +325,10 @@ mod tests { params: default_params.clone(), want_pass: true, }, - Test { - name: "No chain specified".to_string(), - params: QueryClientStateCmd { - chain_id: None, - ..default_params.clone() - }, - want_pass: false, - }, Test { name: "Chain not configured".to_string(), params: QueryClientStateCmd { - chain_id: Some("notibc0oribc1".to_string().parse().unwrap()), + chain_id: "notibc0oribc1".to_string().parse().unwrap(), ..default_params.clone() }, want_pass: false, @@ -400,7 +385,7 @@ mod tests { #[test] fn parse_query_client_connections_parameters() { let default_params = QueryClientConnectionsCmd { - chain_id: Some("ibc-0".to_string().parse().unwrap()), + chain_id: "ibc-0".to_string().parse().unwrap(), client_id: Some("clientidone".to_string().parse().unwrap()), height: Some(4), }; @@ -417,18 +402,10 @@ mod tests { params: default_params.clone(), want_pass: true, }, - Test { - name: "No chain specified".to_string(), - params: QueryClientConnectionsCmd { - chain_id: None, - ..default_params.clone() - }, - want_pass: false, - }, Test { name: "Chain not configured".to_string(), params: QueryClientConnectionsCmd { - chain_id: Some("notibc0oribc1".to_string().parse().unwrap()), + chain_id: "notibc0oribc1".to_string().parse().unwrap(), ..default_params.clone() }, want_pass: false,