Summary

Problem Definition

@ancazamfir: In general, error handling and details are inconsistent across the CLIs, in case of errors sometimes we just exit with Error: Hermes command exited with an error… in other cases ERROR generic error…. We wrap_err or .map_err(|e| BaseError::generic(e) or .map_err(|e| BaseError::relayer(e). Error strings are sometimes capitalized, sometimes not. I know it sounds minor at this stage but the UX is not great. We should set some guidelines and fix these at some point.

Proposal

Error messages

The text should be matter of fact and avoid capitalization and periods, unless multiple sentences are needed:

error: the fobrulator needs to be krontrificated
       ^

When code or an identifier must appear in a message or label, it should be surrounded with backticks:

error: failed to query client state for client `foo` on chain `ibc-0`

Error context

If possible, context should be added to errors that can be raised in the CLI, ie. if one is calling a fallible function or method from a CLI command, one should call .wrap_err("...") or .wrap_err_with(|| format!("...", args...)) on the result of the call in order to provide a human-readable explanation of what went wrong.

For instance, instead of

let clients = chain.query_client_states().await?;

Let's do:

let clients = chain.query_client_states().await
    .wrap_err("Failed to query clients")?;

or alternatively:

let clients = chain.query_client_states().await
    .wrap_err_with(|| format!("failed to query clients hosted on chain {chain_id}"))?;

Note: wrap_err and wrap_err_with are made available with:

use oneline_eyre::eyre::Context;

In some cases, one must wrap the returned error in another error type, for example when calling methods on a ChainHandle within .with_blocking_chain_handle.
In this situation, one should wrap the error using .map_err(|e| CosmosChain::raise_err(e)) if within a non-generic context or .map_err(|e| Chain::raise_err(e)) within a generic context where Chain is a generic type parameter. This might require a CanRaiserError<E> constraint on the Chain type parameter where E is the type of error returned by the original call.

For instance, instead of:

chain.with_blocking_chain_handle(move |handle| {
      let something = handle.fallible_call()
          .map_err(|e| BaseError::relayer(e))?;
      Ok(something)
  })
  .await?;

let's do

chain.with_blocking_chain_handle(move |handle| {
      let something = handle.fallible_call()
          .map_err(|e| Chain::raise_error(e))?;
      Ok(something)
  })
  .await?;

TODO: How to best add context in that case? Perhaps with:

chain.with_blocking_chain_handle(move |handle| {
      let something = handle.fallible_call()
          .map_err(|e| Chain::raise_error(e.wrap_err("failed to do X")))?;
      Ok(something)
  })
  .await?;

Acceptance Criteria

The guidelines are implemented across all CLIs.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned