chore/error: remove from str conversion and add deprecation notificat… (paritytech#7472)

* chore/error: remove from str conversion and add deprecation notifications

* fixup changes

* fix test looking for gone ::Msg variant

* another test fix

* one is duplicate, the other is not, so duplicates reported are n-1

* darn spaces

Co-authored-by: Andronik Ordian <[email protected]>

* remove pointless doc comments of error variants without any value

* low hanging fruits (for a tall person)

* moar error type variants

* avoid the storage modules for now

They are in need of a refactor, and the pain is rather large
removing all String error and DefaultError occurences.

* chore remove pointless error generic

* fix test for mocks, add a bunch of non_exhaustive

* max line width

* test fixes due to error changes

* fin

* error outputs... again

* undo stderr adjustments

* Update client/consensus/slots/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

* remove closure clutter

Co-authored-by: Bastian Köcher <[email protected]>

* more error types

* introduce ApiError

* extract Mock error

* ApiError refactor

* even more error types

* the last for now

* chore unused deps

* another extraction

* reduce should panic, due to extended error messages

* error test happiness

* shift error lines by one

* doc tests

* white space

Co-authored-by: Bastian Köcher <[email protected]>

* Into -> From

Co-authored-by: Bastian Köcher <[email protected]>

* remove pointless codec

Co-authored-by: Bastian Köcher <[email protected]>

* avoid pointless self import

Co-authored-by: Bastian Köcher <[email protected]>

Co-authored-by: Bernhard Schuster <[email protected]>
Co-authored-by: Andronik Ordian <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

Author: 4 people

Cargo.lock

@@ -46,3 +46,4 @@ prometheus-endpoint = { package = "substrate-prometheus-endpoint", version = "0.
 kvdb-memorydb = "0.7.0"
 sp-test-primitives = { version = "2.0.0", path = "../../primitives/test-primitives" }
 substrate-test-runtime = { version = "2.0.0", path = "../../test-utils/runtime"  }
+thiserror = "1.0.21"

client/api/Cargo.toml

@@ -312,13 +312,21 @@ pub mod tests {
 	use sp_test_primitives::{Block, Header, Extrinsic};
 	use super::*;
 
+	#[derive(Debug, thiserror::Error)]
+	#[error("Not implemented on test node")]
+	struct MockError;
+
+	impl Into<ClientError> for MockError {
+		fn into(self) -> ClientError {
+			ClientError::Application(Box::new(self))
+		}
+	}	
+	
 	pub type OkCallFetcher = Mutex<Vec<u8>>;
 
-	fn not_implemented_in_tests<T, E>() -> Ready<Result<T, E>>
-	where
-		E: std::convert::From<&'static str>,
+	fn not_implemented_in_tests<T>() -> Ready<Result<T, ClientError>>
 	{
-		futures::future::ready(Err("Not implemented on test node".into()))
+		futures::future::ready(Err(MockError.into()))
 	}
 
 	impl Fetcher<Block> for OkCallFetcher {

client/api/src/light.rs

@@ -208,10 +208,7 @@ impl<A, B, Block, C> sp_consensus::Proposer<Block> for
 		}));
 
 		async move {
-			match rx.await {
-				Ok(x) => x,
-				Err(err) => Err(sp_blockchain::Error::Msg(err.to_string()))
-			}
+			rx.await?
 		}.boxed()
 	}
 }

client/basic-authorship/src/basic_authorship.rs

@@ -212,7 +212,7 @@ where
 			&state,
 			changes_trie_state.as_ref(),
 			parent_hash,
-		)?;
+		).map_err(|e| sp_blockchain::Error::StorageChanges(e))?;
 
 		Ok(BuiltBlock {
 			block: <Block as BlockT>::new(header, self.extrinsics),

client/block-builder/src/lib.rs

@@ -25,64 +25,61 @@ pub type Result<T> = std::result::Result<T, Error>;
 
 /// Error type for the CLI.
 #[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
 pub enum Error {
-	/// Io error
 	#[error(transparent)]
 	Io(#[from] std::io::Error),
-	/// Cli error
+
 	#[error(transparent)]
 	Cli(#[from] structopt::clap::Error),
-	/// Service error
+
 	#[error(transparent)]
 	Service(#[from] sc_service::Error),
-	/// Client error
+
 	#[error(transparent)]
 	Client(#[from] sp_blockchain::Error),
-	/// scale codec error
+
 	#[error(transparent)]
 	Codec(#[from] parity_scale_codec::Error),
-	/// Input error
+
 	#[error("Invalid input: {0}")]
 	Input(String),
-	/// 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),
-	/// 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")]
+
+	#[error("Invalid hexadecimal string 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 +90,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/cli/src/error.rs

@@ -31,7 +31,8 @@ sp-inherents = { version = "2.0.0", path = "../../../primitives/inherents" }
 futures = "0.3.4"
 futures-timer = "3.0.1"
 parking_lot = "0.10.0"
-log = "0.4.8"
+log = "0.4.11"
+thiserror = "1.0.21"
 
 [dev-dependencies]
 substrate-test-runtime-client = { version = "2.0.0", path = "../../../test-utils/runtime/client" }

client/consensus/slots/Cargo.toml

@@ -20,7 +20,8 @@
 //! time during which certain events can and/or must occur.  This crate
 //! provides generic functionality for slots.
 
-#![forbid(unsafe_code, missing_docs)]
+#![forbid(unsafe_code)]
+#![deny(missing_docs)]
 
 mod slots;
 mod aux_schema;
@@ -470,6 +471,15 @@ pub enum CheckedHeader<H, S> {
 	Checked(H, S),
 }
 
+
+
+#[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
+pub enum Error<T> where T: SlotData + Clone + Debug + Send + Sync + 'static {
+	#[error("Slot duration is invalid: {0:?}")]
+	SlotDurationInvalid(SlotDuration<T>),
+}
+
 /// A slot duration. Create with `get_or_compute`.
 // The internal member should stay private here to maintain invariants of
 // `get_or_compute`.
@@ -494,7 +504,7 @@ impl<T: SlotData + Clone> SlotData for SlotDuration<T> {
 	const SLOT_KEY: &'static [u8] = T::SLOT_KEY;
 }
 
-impl<T: Clone> SlotDuration<T> {
+impl<T: Clone + Send + Sync + 'static> SlotDuration<T> {
 	/// Either fetch the slot duration from disk or compute it from the
 	/// genesis state.
 	///
@@ -532,10 +542,8 @@ impl<T: Clone> SlotDuration<T> {
 			}
 		}?;
 
-		if slot_duration.slot_duration() == 0 {
-			return Err(sp_blockchain::Error::Msg(
-				"Invalid value for slot_duration: the value must be greater than 0.".into(),
-			))
+		if slot_duration.slot_duration() == 0u64 {
+			return Err(sp_blockchain::Error::Application(Box::new(Error::SlotDurationInvalid(slot_duration))))
 		}
 
 		Ok(slot_duration)
@@ -939,7 +947,7 @@ mod test {
 			true, true, true, true,
 	];
 
-		assert_eq!(backoff, expected);
+		assert_eq!(backoff.as_slice(), &expected[..]);
 	}
 
 	#[test]

client/consensus/slots/src/lib.rs

@@ -891,9 +891,7 @@ impl<Block: BlockT> Backend<Block> {
 		let is_archive_pruning = config.pruning.is_archive();
 		let blockchain = BlockchainDb::new(db.clone())?;
 		let meta = blockchain.meta.clone();
-		let map_e = |e: sc_state_db::Error<io::Error>| sp_blockchain::Error::from(
-			format!("State database error: {:?}", e)
-		);
+		let map_e = |e: sc_state_db::Error<io::Error>| sp_blockchain::Error::from_state_db(e);
 		let state_db: StateDb<_, _> = StateDb::new(
 			config.pruning.clone(),
 			!config.source.supports_ref_counting(),
@@ -1082,7 +1080,7 @@ impl<Block: BlockT> Backend<Block> {
 
 			trace!(target: "db", "Canonicalize block #{} ({:?})", new_canonical, hash);
 			let commit = self.storage.state_db.canonicalize_block(&hash)
-				.map_err(|e: sc_state_db::Error<io::Error>| sp_blockchain::Error::from(format!("State database error: {:?}", e)))?;
+				.map_err(|e: sc_state_db::Error<io::Error>| sp_blockchain::Error::from_state_db(e))?;
 			apply_state_commit(transaction, commit);
 		};
 
@@ -1212,9 +1210,7 @@ impl<Block: BlockT> Backend<Block> {
 					number_u64,
 					&pending_block.header.parent_hash(),
 					changeset,
-				).map_err(|e: sc_state_db::Error<io::Error>|
-					sp_blockchain::Error::from(format!("State database error: {:?}", e))
-				)?;
+				).map_err(|e: sc_state_db::Error<io::Error>| sp_blockchain::Error::from_state_db(e))?;
 				apply_state_commit(&mut transaction, commit);
 
 				// Check if need to finalize. Genesis is always finalized instantly.
@@ -1379,7 +1375,7 @@ impl<Block: BlockT> Backend<Block> {
 			transaction.set_from_vec(columns::META, meta_keys::FINALIZED_BLOCK, lookup_key);
 
 			let commit = self.storage.state_db.canonicalize_block(&f_hash)
-				.map_err(|e: sc_state_db::Error<io::Error>| sp_blockchain::Error::from(format!("State database error: {:?}", e)))?;
+				.map_err(|e: sc_state_db::Error<io::Error>| sp_blockchain::Error::from_state_db(e))?;
 			apply_state_commit(transaction, commit);
 
 			if !f_num.is_zero() {

client/db/src/lib.rs

@@ -14,16 +14,15 @@ readme = "README.md"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
-log = "0.4.8"
 derive_more = "0.99.2"
 parity-wasm = "0.41.0"
 codec = { package = "parity-scale-codec", version = "1.3.4" }
 wasmi = "0.6.2"
 sp-core = { version = "2.0.0", path = "../../../primitives/core" }
 sp-allocator = { version = "2.0.0", path = "../../../primitives/allocator" }
 sp-wasm-interface = { version = "2.0.0", path = "../../../primitives/wasm-interface" }
-sp-runtime-interface = { version = "2.0.0", path = "../../../primitives/runtime-interface" }
 sp-serializer = { version = "2.0.0", path = "../../../primitives/serializer" }
+thiserror = "1.0.21"
 
 [features]
 default = []