improve error handling in cli (#7436)

* other error variant should carry a dyn Error

* introduce thiserror for error derive, add explicit error variants, cleanup dependencies

* cleanup handle dev-deps of sc-cli too

* Update client/cli/src/error.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bernhard Schuster <bernhard@parity.io>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/client/cli/src/commands/insert.rs

@@ -62,7 +62,7 @@ impl InsertCmd {
 	pub fn run(&self) -> Result<(), Error> {
 		let suri = utils::read_uri(self.suri.as_ref())?;
 		let base_path = self.shared_params.base_path.as_ref()
-			.ok_or_else(|| Error::Other("please supply base path".into()))?;
+			.ok_or_else(|| Error::MissingBasePath)?;
 
 		let (keystore, public) = match self.keystore_params.keystore_config(base_path)? {
 			KeystoreConfig::Path { path, password } => {
@@ -70,20 +70,19 @@ impl InsertCmd {
 					self.crypto_scheme.scheme,
 					to_vec(&suri, password.clone())
 				)?;
-				let keystore: SyncCryptoStorePtr = Arc::new(LocalKeystore::open(path, password)
-					.map_err(|e| format!("{}", e))?);
+				let keystore: SyncCryptoStorePtr = Arc::new(LocalKeystore::open(path, password)?);
 				(keystore, public)
 			},
 			_ => unreachable!("keystore_config always returns path and password; qed")
 		};
 
 		let key_type = KeyTypeId::try_from(self.key_type.as_str())
-			.map_err(|_| {
-				Error::Other("Cannot convert argument to keytype: argument should be 4-character string".into())
+			.map_err(|_e| {
+				Error::KeyTypeInvalid
 			})?;
 
 		SyncCryptoStore::insert_unknown(&*keystore, key_type, &suri, &public[..])
-			.map_err(|e| Error::Other(format!("{:?}", e)))?;
+			.map_err(|_| Error::KeyStoreOperation)?;
 
 		Ok(())
 	}

substrate/client/cli/src/commands/utils.rs

@@ -246,8 +246,7 @@ pub fn decode_hex<T: AsRef<[u8]>>(message: T) -> Result<Vec<u8>, Error> {
 	if message[..2] == [b'0', b'x'] {
 		message = &message[2..]
 	}
-	hex::decode(message)
-		.map_err(|e| Error::Other(format!("Invalid hex ({})", e)))
+	Ok(hex::decode(message)?)
 }
 
 /// checks if message is Some, otherwise reads message from stdin and optionally decodes hex

substrate/client/cli/src/commands/verify.rs

@@ -77,27 +77,25 @@ fn verify<Pair>(sig_data: Vec<u8>, message: Vec<u8>, uri: &str) -> error::Result
 {
 	let mut signature = Pair::Signature::default();
 	if sig_data.len() != signature.as_ref().len() {
-		return Err(error::Error::Other(format!(
-			"signature has an invalid length. read {} bytes, expected {} bytes",
-			sig_data.len(),
-			signature.as_ref().len(),
-		)));
+		return Err(
+			error::Error::SignatureInvalidLength {
+				read: sig_data.len(),
+				expected: signature.as_ref().len(),
+			}
+		);
 	}
 	signature.as_mut().copy_from_slice(&sig_data);
 
 	let pubkey = if let Ok(pubkey_vec) = hex::decode(uri) {
 		Pair::Public::from_slice(pubkey_vec.as_slice())
 	} else {
-		Pair::Public::from_string(uri)
-			.map_err(|_| {
-				error::Error::Other(format!("Invalid URI; expecting either a secret URI or a public URI."))
-			})?
+		Pair::Public::from_string(uri)?
 	};
 
 	if Pair::verify(&signature, &message, &pubkey) {
 		println!("Signature verifies correctly.");
 	} else {
-		return Err(error::Error::Other("Signature invalid.".into()))
+		return Err(error::Error::SignatureInvalid)
 	}
 
 	Ok(())

substrate/client/cli/src/error.rs

@@ -18,61 +18,87 @@
 
 //! Initialization errors.
 
-
+use sp_core::crypto;
 
 /// Result type alias for the CLI.
 pub type Result<T> = std::result::Result<T, Error>;
 
 /// Error type for the CLI.
-#[derive(Debug, derive_more::Display, derive_more::From)]
+#[derive(Debug, thiserror::Error)]
 pub enum Error {
 	/// Io error
-	Io(std::io::Error),
+	#[error(transparent)]
+	Io(#[from] std::io::Error),
 	/// Cli error
-	Cli(structopt::clap::Error),
+	#[error(transparent)]
+	Cli(#[from] structopt::clap::Error),
 	/// Service error
-	Service(sc_service::Error),
+	#[error(transparent)]
+	Service(#[from] sc_service::Error),
 	/// Client error
-	Client(sp_blockchain::Error),
+	#[error(transparent)]
+	Client(#[from] sp_blockchain::Error),
 	/// scale codec error
-	Codec(parity_scale_codec::Error),
+	#[error(transparent)]
+	Codec(#[from] parity_scale_codec::Error),
 	/// Input error
-	#[from(ignore)]
+	#[error("Invalid input: {0}")]
 	Input(String),
 	/// Invalid listen multiaddress
-	#[display(fmt="Invalid listen multiaddress")]
-	#[from(ignore)]
+	#[error("Invalid listen multiaddress")]
 	InvalidListenMultiaddress,
-	/// Other uncategorized error.
-	#[from(ignore)]
+	/// Application specific error chain sequence forwarder.
+	#[error(transparent)]
+	Application(#[from] Box<dyn std::error::Error>),
+	/// URI error.
+	#[error("Invalid URI; expecting either a secret URI or a public URI.")]
+	InvalidUri(crypto::PublicError),
+	/// Signature length mismatch.
+	#[error("Signature has an invalid length. Read {read} bytes, expected {expected} bytes")]
+	SignatureInvalidLength {
+		/// Amount of signature bytes read.
+		read: usize,
+		/// Expected number of signature bytes.
+		expected: usize,
+	},
+	/// Missing base path argument.
+	#[error("The base path is missing, please provide one")]
+	MissingBasePath,
+	/// Unknown key type specifier or missing key type specifier.
+	#[error("Unknown key type, must be a known 4-character sequence")]
+	KeyTypeInvalid,
+	/// Signature verification failed.
+	#[error("Signature verification failed")]
+	SignatureInvalid,
+	/// Storing a given key failed.
+	#[error("Key store operation failed")]
+	KeyStoreOperation,
+	/// An issue with the underlying key storage was encountered.
+	#[error("Key storage issue encountered")]
+	KeyStorage(#[from] sc_keystore::Error),
+	/// Bytes are not decodable when interpreted as hexadecimal string.
+	#[error("Invalid hex base data")]
+	HexDataConversion(#[from] hex::FromHexError),
+	/// Shortcut type to specify types on the fly, discouraged.
+	#[deprecated = "Use `Forwarded` with an error type instead."]
+	#[error("Other: {0}")]
 	Other(String),
 }
 
-/// Must be implemented explicitly because `derive_more` won't generate this
-/// case due to conflicting derive for `Other(String)`.
-impl std::convert::From<String> for Error {
-	fn from(s: String) -> Error {
-		Error::Input(s)
-	}
-}
-
 impl std::convert::From<&str> for Error {
 	fn from(s: &str) -> Error {
 		Error::Input(s.to_string())
 	}
 }
 
-impl std::error::Error for Error {
-	fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-		match self {
-			Error::Io(ref err) => Some(err),
-			Error::Cli(ref err) => Some(err),
-			Error::Service(ref err) => Some(err),
-			Error::Client(ref err) => Some(err),
-			Error::Codec(ref err) => Some(err),
-			Error::Input(_) => None,
-			Error::InvalidListenMultiaddress => None,
-			Error::Other(_) => None,
-		}
+impl std::convert::From<String> for Error {
+	fn from(s: String) -> Error {
+		Error::Input(s.to_string())
+	}
+}
+
+impl std::convert::From<crypto::PublicError> for Error {
+	fn from(e: crypto::PublicError) -> Error {
+		Error::InvalidUri(e)
 	}
 }

substrate/client/cli/src/lib.rs

@@ -20,6 +20,8 @@
 
 #![warn(missing_docs)]
 #![warn(unused_extern_crates)]
+#![warn(unused_imports)]
+#![warn(unused_crate_dependencies)]
 
 pub mod arg_enums;
 mod commands;