Fix for relayer::error displays (#559)

* Include cause in Failed errors (#555). Double-checking 'required' tag.

* Finished adding 'required' tag to commands

* Last nits around error messages.

* Changelog

* Change doctests in conclude to ignore

* Remove Option from chain_id parameter in client queries

Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>

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])
   - Implement commands for channel close init and confirm ([#538])
   - Implement command to perform the handshake for a new channel ([#557])
   - Query all connections command ([#553])
@@ -71,6 +72,7 @@
 [#537]: https://github.com/informalsystems/ibc-rs/issues/537
 [#538]: https://github.com/informalsystems/ibc-rs/issues/538
 [#540]: https://github.com/informalsystems/ibc-rs/issues/540
+[#555]: https://github.com/informalsystems/ibc-rs/issues/555
 [#554]: https://github.com/informalsystems/ibc-rs/issues/554
 [#553]: https://github.com/informalsystems/ibc-rs/issues/553
 [#557]: https://github.com/informalsystems/ibc-rs/issues/557

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<ChainId>,
 
-    #[options(free, help = "identifier of the port to query")]
+    #[options(free, required, help = "identifier of the port to query")]
     port_id: Option<String>,
 
-    #[options(free, help = "identifier of the channel to query")]
+    #[options(free, required, help = "identifier of the channel to query")]
     channel_id: Option<String>,
 
     #[options(help = "height of the state to query", short = "h")]
@@ -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

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

@@ -19,19 +19,25 @@ 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.
+//  see the tx/client.rs or tx/packet.rs for simpler approaches.
+
 /// Query client state command
 #[derive(Clone, Command, Debug, Options)]
 pub struct QueryClientStateCmd {
-    #[options(free, help = "identifier of the chain to query")]
-    chain_id: Option<ChainId>,
+    #[options(free, required, help = "identifier of the chain to query")]
+    chain_id: ChainId,
 
-    #[options(free, help = "identifier of the client to query")]
+    #[options(free, required, help = "identifier of the client to query")]
     client_id: Option<String>,
 
     #[options(help = "the chain height which this query should reflect", short = "h")]
     height: Option<u64>,
 
-    #[options(help = "whether proof is required", short = "p")]
+    #[options(
+        help = "whether proof is required; default: false (no proof)",
+        short = "p"
+    )]
     proof: Option<bool>,
 }
 
@@ -95,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")]
-    chain_id: Option<ChainId>,
+    #[options(free, required, help = "identifier of the chain to query")]
+    chain_id: ChainId,
 
-    #[options(free, help = "identifier of the client to query")]
+    #[options(free, required, help = "identifier of the client to query")]
     client_id: Option<String>,
 
-    #[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<u64>,
 
-    #[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<u64>,
 
     #[options(help = "the chain height which this query should reflect", short = "h")]
@@ -192,16 +206,13 @@ impl Runnable for QueryClientConsensusCmd {
 }
 
 fn validate_common_options(
-    chain_id: &Option<ChainId>,
+    chain_id: &ChainId,
     client_id: &Option<String>,
     config: &Config,
 ) -> Result<(ChainConfig, ClientId), String> {
-    let chain_id = chain_id
-        .clone()
-        .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()
@@ -215,10 +226,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")]
-    chain_id: Option<ChainId>,
+    #[options(free, required, help = "identifier of the chain to query")]
+    chain_id: ChainId,
 
-    #[options(free, help = "identifier of the client to query")]
+    #[options(free, required, help = "identifier of the client to query")]
     client_id: Option<String>,
 
     #[options(help = "the chain height which this query should reflect", short = "h")]
@@ -236,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(|| "missing chain configuration".to_string())?;
+            .find_chain(&self.chain_id)
+            .ok_or_else(|| format!("chain '{}' not found in configuration file", self.chain_id))?;
 
         let client_id = self
             .client_id
@@ -300,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,
@@ -318,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,
@@ -386,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),
         };
@@ -403,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,

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<ChainId>,
 
-    #[options(free, help = "identifier of the connection to query")]
+    #[options(free, required, help = "identifier of the connection to query")]
     connection_id: Option<String>,
 
     #[options(help = "height of the state to query", short = "h")]
     height: Option<u64>,
 
-    #[options(help = "whether proof is required", short = "p")]
+    #[options(
+        help = "whether proof is required; default: false (no proof)",
+        short = "p"
+    )]
     proof: Option<bool>,
 }
 
@@ -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
@@ -99,10 +102,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<ChainId>,
 
-    #[options(free, help = "identifier of the connection to query")]
+    #[options(free, required, help = "identifier of the connection to query")]
     connection_id: Option<String>,
 }
 
@@ -122,7 +125,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

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,

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,21 +21,33 @@
 //! 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();
 //! ```
 //!
+//! #### 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:
+//! ```ignore
+//! #[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
+//! ```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();

relayer/src/channel.rs

@@ -30,7 +30,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}")]

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}")]