chore/error: remove from str conversion and add deprecation notificat…

client/cli/src/error.rs

@@ -47,9 +47,6 @@ pub enum Error {
 	/// Invalid listen multiaddress
 	#[error("Invalid listen multiaddress")]
 	InvalidListenMultiaddress,
-	/// Application specific error chain sequence forwarder.
-	#[error(transparent)]
-	Application(#[from] Box<dyn std::error::Error + Send + Sync + 'static>),
 	/// URI error.
 	#[error("Invalid URI; expecting either a secret URI or a public URI.")]
 	InvalidUri(crypto::PublicError),
@@ -79,10 +76,9 @@ pub enum 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),
+	/// Application specific error chain sequence forwarder.
+	#[error(transparent)]
+	Application(#[from] Box<dyn std::error::Error + Send + Sync + 'static>),
 }
 
 impl std::convert::From<&str> for Error {
@@ -93,7 +89,7 @@ impl std::convert::From<&str> for Error {
 
 impl std::convert::From<String> for Error {
 	fn from(s: String) -> Error {
-		Error::Input(s.to_string())
+		Error::Input(s)
 	}
 }
 

client/service/src/client/wasm_override.rs

@@ -119,16 +119,13 @@ where
 	/// Scrapes a folder for WASM runtimes.
 	/// Returns a hashmap of the runtime version and wasm runtime code.
 	fn scrape_overrides(dir: &Path, executor: &E) -> Result<HashMap<u32, WasmBlob>> {
+
 		let handle_err = |e: std::io::Error | -> sp_blockchain::Error {
-			sp_blockchain::Error::Msg(format!("{}", e.to_string()))
+				sp_blockchain::Error::WasmOverrideIo(dir.to_owned(), e)
 		};
 
 		if !dir.is_dir() {
-			return Err(sp_blockchain::Error::Msg(format!(
-				"Overwriting WASM requires a directory where \
-				 local WASM is stored. {:?} is not a directory",
-				 dir,
-			)));
+			return Err(sp_blockchain::Error::WasmOverrideNotADirectory(dir.to_owned()));
 		}
 
 		let mut overrides = HashMap::new();
@@ -149,9 +146,7 @@ where
 		}
 
 		if !duplicates.is_empty() {
-			let duplicate_file_list = duplicates.join("\n");
-			let msg = format!("Duplicate WASM Runtimes found: \n{}\n", duplicate_file_list);
-			return Err(sp_blockchain::Error::Msg(msg));
+			return Err(sp_blockchain::Error::DuplicateWasmRuntime(duplicates));
 		}
 
 		Ok(overrides)
@@ -238,12 +233,8 @@ mod tests {
 			match scraped {
 				Err(e) => {
 					match e {
-						sp_blockchain::Error::Msg(msg) => {
-							let is_match = msg
-								.matches("Duplicate WASM Runtimes found")
-								.map(ToString::to_string)
-								.collect::<Vec<String>>();
-							assert!(is_match.len() >= 1)
+						sp_blockchain::Error::DuplicateWasmRuntime(duplicates) => {
+							assert_eq!(duplicates.len(), 1);
 						},
 						_ => panic!("Test should end with Msg Error Variant")
 					}

client/sync-state-rpc/src/lib.rs

@@ -79,17 +79,13 @@ impl<TBl, TCl> SyncStateRpcHandler<TBl, TCl>
 	fn build_sync_state(&self) -> Result<sc_chain_spec::LightSyncState<TBl>, sp_blockchain::Error> {
 		let finalized_hash = self.client.info().finalized_hash;
 		let finalized_header = self.client.header(BlockId::Hash(finalized_hash))?
-			.ok_or_else(|| sp_blockchain::Error::Msg(
-				format!("Failed to get the header for block {:?}", finalized_hash)
-			))?;
+			.ok_or_else(|| sp_blockchain::Error::MissingHashInHeader(finalized_hash.to_string()))?;
 
 		let finalized_block_weight = sc_consensus_babe::aux_schema::load_block_weight(
-			&*self.client,
-			finalized_hash,
-		)?
-			.ok_or_else(|| sp_blockchain::Error::Msg(
-				format!("Failed to load the block weight for block {:?}", finalized_hash)
-			))?;
+				&*self.client,
+				finalized_hash,
+			)?
+			.ok_or_else(|| sp_blockchain::Error::MissingBlockWeightInHeader(finalized_hash.to_string()))?;
 
 		Ok(sc_chain_spec::LightSyncState {
 			finalized_block_header: finalized_header,

primitives/blockchain/src/backend.rs

@@ -172,7 +172,7 @@ pub trait Backend<Block: BlockT>: HeaderBackend<Block> + HeaderMetadata<Block, E
 			if let Some(max_number) = maybe_max_number {
 				loop {
 					let current_header = self.header(BlockId::Hash(current_hash.clone()))?
-						.ok_or_else(|| Error::from(format!("failed to get header for hash {}", current_hash)))?;
+						.ok_or_else(|| Error::MissingHashInHeader(current_hash.to_string()))?;
 
 					if current_header.number() <= &max_number {
 						best_hash = current_header.hash();
@@ -191,7 +191,7 @@ pub trait Backend<Block: BlockT>: HeaderBackend<Block> + HeaderMetadata<Block, E
 				}
 
 				let current_header = self.header(BlockId::Hash(current_hash.clone()))?
-					.ok_or_else(|| Error::from(format!("failed to get header for hash {}", current_hash)))?;
+					.ok_or_else(|| Error::MissingHashInHeader(current_hash.to_string()))?;
 
 				// stop search in this chain once we go below the target's block number
 				if current_header.number() < target_header.number() {

primitives/blockchain/src/error.rs

@@ -17,7 +17,7 @@
 
 //! Substrate client possible errors.
 
-use std::{self, result};
+use std::{self, result, path::PathBuf};
 use sp_state_machine;
 use sp_runtime::transaction_validity::TransactionValidityError;
 use sp_consensus;
@@ -35,110 +35,150 @@ pub enum ApplyExtrinsicFailed {
 	/// unappliable onto the current block.
 	#[error("Extrinsic is not valid: {0:?}")]
 	Validity(#[from] TransactionValidityError),
+
 	/// This is used for miscellaneous errors that can be represented by string and not handleable.
 	///
 	/// This will become obsolete with complete migration to v4 APIs.
+	#[deprecated(note = "Introduce more typed error variants as needed")]
 	#[error("Extrinsic failed: {0}")]
 	Msg(String),
 }
 
 /// Substrate Client error
 #[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
 pub enum Error {
 	/// Consensus Error
 	#[error(transparent)]
 	Consensus(#[from] sp_consensus::Error),
+
 	/// Backend error.
 	#[error("Backend error: {0}")]
 	Backend(String),
+
 	/// Unknown block.
 	#[error("UnknownBlock: {0}")]
 	UnknownBlock(String),
+
 	/// The `apply_extrinsic` is not valid due to the given `TransactionValidityError`.
 	#[error(transparent)]
 	ApplyExtrinsicFailed(#[from] ApplyExtrinsicFailed),
+
 	/// Execution error.
+	// `inner` cannot be made member, since it lacks `std::error::Error` trait bounds.
 	#[error("Execution failed: {0:?}")]
 	Execution(Box<dyn sp_state_machine::Error>),
+
 	/// Blockchain error.
 	#[error("Blockchain")]
 	Blockchain(#[source] Box<Error>),
+
 	/// Invalid authorities set received from the runtime.
 	#[error("Current state of blockchain has invalid authorities set")]
 	InvalidAuthoritiesSet,
+
 	/// Could not get runtime version.
 	#[error("Failed to get runtime version: {0}")]
 	VersionInvalid(String),
+
 	/// Genesis config is invalid.
 	#[error("Genesis config provided is invalid")]
 	GenesisInvalid,
+
 	/// Error decoding header justification.
 	#[error("error decoding justification for header")]
 	JustificationDecode,
+
 	/// Justification for header is correctly encoded, but invalid.
 	#[error("bad justification for header: {0}")]
 	BadJustification(String),
+
 	/// Not available on light client.
 	#[error("This method is not currently available when running in light client mode")]
 	NotAvailableOnLightClient,
+
 	/// Invalid remote CHT-based proof.
 	#[error("Remote node has responded with invalid header proof")]
 	InvalidCHTProof,
+
 	/// Remote fetch has been cancelled.
 	#[error("Remote data fetch has been cancelled")]
 	RemoteFetchCancelled,
+
 	/// Remote fetch has been failed.
 	#[error("Remote data fetch has been failed")]
 	RemoteFetchFailed,
+
 	/// Error decoding call result.
 	#[error("Error decoding call result of {0}")]
 	CallResultDecode(&'static str, #[source] CodecError),
+
 	/// Error converting a parameter between runtime and node.
 	#[error("Error converting `{0}` between runtime and node")]
 	RuntimeParamConversion(String),
+
 	/// Changes tries are not supported.
 	#[error("Changes tries are not supported by the runtime")]
 	ChangesTriesNotSupported,
+
 	/// Error reading changes tries configuration.
 	#[error("Error reading changes tries configuration")]
 	ErrorReadingChangesTriesConfig,
+
 	/// Key changes query has failed.
 	#[error("Failed to check changes proof: {0}")]
 	ChangesTrieAccessFailed(String),
+
 	/// Last finalized block not parent of current.
 	#[error("Did not finalize blocks in sequential order.")]
 	NonSequentialFinalization(String),
+
 	/// Safety violation: new best block not descendent of last finalized.
 	#[error("Potential long-range attack: block not in finalized chain.")]
 	NotInFinalizedChain,
+
 	/// Hash that is required for building CHT is missing.
 	#[error("Failed to get hash of block for building CHT")]
 	MissingHashRequiredForCHT,
+
 	/// Invalid calculated state root on block import.
 	#[error("Calculated state root does not match.")]
 	InvalidStateRoot,
+
 	/// Incomplete block import pipeline.
 	#[error("Incomplete block import pipeline.")]
 	IncompletePipeline,
+
+	/// Transaction pool initizliation is not complete.
 	#[error("Transaction pool not ready for block production.")]
 	TransactionPoolNotReady,
+
+	/// Database yielded an error.
 	#[error("Database")]
 	DatabaseError(#[from] sp_database::error::DatabaseError),
-	/// A convenience variant for String
-	#[error("{0}")]
-	Msg(String),
-}
 
-impl<'a> From<&'a str> for Error {
-	fn from(s: &'a str) -> Self {
-		Error::Msg(s.into())
-	}
-}
+	#[error("Failed to get header for hash {0}")]
+	MissingHashInHeader(String),
 
-impl From<String> for Error {
-	fn from(s: String) -> Self {
-		Error::Msg(s)
-	}
+	#[error("Failed to load the block weight for block {0}")]
+	MissingBlockWeightInHeader(String),
+
+	#[error("WASM override IO error")]
+	WasmOverrideIo(PathBuf, #[source] std::io::Error),
+
+	#[error("Overwriting WASM requires a directory where local \
+	WASM is stored. {} is not a directory", .0.display())]
+	WasmOverrideNotADirectory(PathBuf),
+
+	#[error("Duplicate WASM Runtimes found: \n{}\n", .0.join("\n") )]
+	DuplicateWasmRuntime(Vec<String>),
+
+	#[error(transparent)]
+	Foreign(#[from] Box<dyn std::error::Error + Send + Sync + 'static>),
+
+	/// Should be avoided if possible, use `Foreign` instead.
+	#[error("{0}")]
+	Msg(String),
 }
 
 impl From<Box<dyn sp_state_machine::Error + Send + Sync>> for Error {
@@ -153,6 +193,18 @@ impl From<Box<dyn sp_state_machine::Error>> for Error {
 	}
 }
 
+impl From<String> for Error {
+	fn from(msg: String) -> Self {
+		Self::Msg(msg)
+	}
+}
+impl From<&str> for Error {
+	fn from(msg: &str) -> Self {
+		Self::Msg(msg.to_owned())
+	}
+}
+
+
 impl Error {
 	/// Chain a blockchain error.
 	pub fn from_blockchain(e: Box<Error>) -> Self {