Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for relayer::error displays #559

Merged
merged 8 commits into from
Jan 28, 2021
Merged

Fix for relayer::error displays #559

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
72242cf
Include cause in Failed errors (#555). Double-checking 'required' tag.
adizere Jan 26, 2021
f3fba1b
Finished adding 'required' tag to commands
adizere Jan 26, 2021
ffefc34
Last nits around error messages.
adizere Jan 27, 2021
b278859
Changelog
adizere Jan 27, 2021
53374bb
Merge branch 'master' into adi/555_err
adizere Jan 27, 2021
4d78ed0
Change doctests in conclude to ignore
adizere Jan 27, 2021
2ec0e85
Merge branch 'master' into adi/555_err
ancazamfir Jan 28, 2021
ecb5318
Remove Option from chain_id parameter in client queries
ancazamfir Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])

Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions relayer-cli/src/commands/query/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -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
Expand Down
38 changes: 26 additions & 12 deletions relayer-cli/src/commands/query/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +22 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because gumdrop requires types used in options to have a Default instance: https://gist.github.com/romac/23db89d7c5971b0e591a11286fc259a6

While this makes sense for options which are not marked required, it's beyond me why the requirement subsists for required options.

As such, I am not even sure we need the required annotation since we end up checking for the Option to be defined anyway but I guess it does not hurt if the error message thrown by gumdrop is good enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but we have a Default instance for ChainId. Then yeah we don't need the option and can instead just use required.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added Default -s for everything when we started to use types in the CLI options. I think we should consider moving to something else in V1 (maybe directly gumdrop) as we need more flexibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As such, I am not even sure we need the required annotation since we end up checking for the Option to be defined anyway but I guess it does not hurt if the error message thrown by gumdrop is good enough.

The error message from gumdrop is a bit.. underwhelming. But it helps still. It says only:

error: missing required free argument

Was looking at ways to parametrize this, but did not find anything.

The way I understand it, with 'required' we now have two levels of checks:

  1. gumdrop enforces some (any) value to be present
  2. in our validate_options methods we parse and do additional checks on the field.

We added Default -s for everything when we started to use types in the CLI options. I think we should consider moving to something else in V1 (maybe directly gumdrop) as we need more flexibility.

Sounds like a plan.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue to continue discussion, if necessary, and track this

#573

ancazamfir marked this conversation as resolved.
Show resolved Hide resolved

/// 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<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>,
}

Expand Down Expand Up @@ -97,16 +103,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<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")]
Expand Down Expand Up @@ -202,10 +216,10 @@ 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(|| "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()
Expand All @@ -219,10 +233,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<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")]
Expand All @@ -246,7 +260,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
Expand Down
17 changes: 10 additions & 7 deletions relayer-cli/src/commands/query/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
}

Expand All @@ -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
Expand Down Expand Up @@ -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<ChainId>,

#[options(free, help = "identifier of the connection to query")]
#[options(free, required, help = "identifier of the connection to query")]
connection_id: Option<String>,
}

Expand All @@ -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
Expand Down
9 changes: 5 additions & 4 deletions relayer-cli/src/commands/tx/transfer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
12 changes: 12 additions & 0 deletions relayer-cli/src/conclude.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
adizere marked this conversation as resolved.
Show resolved Hide resolved
//! #[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
Expand Down
2 changes: 1 addition & 1 deletion relayer/src/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")]
Expand Down
4 changes: 2 additions & 2 deletions relayer/src/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")]
Expand Down