From 8467f4a62f437b1bd32a7880019bcaaa275e1ecc Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 30 Oct 2020 15:57:00 +0100 Subject: [PATCH 01/35] chore/error: remove from str conversion and add deprecation notifications --- client/service/src/client/wasm_override.rs | 18 +++---- client/sync-state-rpc/src/lib.rs | 14 ++--- primitives/blockchain/src/backend.rs | 4 +- primitives/blockchain/src/error.rs | 61 +++++++++++++++++----- 4 files changed, 64 insertions(+), 33 deletions(-) diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index 1025b9633887d..748588f939c3e 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -119,16 +119,16 @@ 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> { - let handle_err = |e: std::io::Error | -> sp_blockchain::Error { - sp_blockchain::Error::Msg(format!("{}", e.to_string())) + + let handle_err = { + let dir = dir.to_owned(); + move |e: std::io::Error | -> sp_blockchain::Error { + sp_blockchain::Error::WasmOverrideIo(dir, 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 +149,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) diff --git a/client/sync-state-rpc/src/lib.rs b/client/sync-state-rpc/src/lib.rs index fa433e5e31d2d..e1c70d3c8fa20 100644 --- a/client/sync-state-rpc/src/lib.rs +++ b/client/sync-state-rpc/src/lib.rs @@ -79,17 +79,13 @@ impl SyncStateRpcHandler fn build_sync_state(&self) -> Result, 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, diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index 1328dfb5752fc..4352a4de1d53a 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -172,7 +172,7 @@ pub trait Backend: HeaderBackend + HeaderMetadata: HeaderBackend + HeaderMetadata), + /// Blockchain error. #[error("Blockchain")] Blockchain(#[source] Box), + /// 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), + + #[error("Failed to get header for hash {0}")] + MissingHashInHeader(String), + + #[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. {0} is not a directory", .0.display())] + WasmOverrideNotADirectory(PathBuf), + + #[error("Duplicate WASM Runtimes found: \n{}\n", .0.join("\n") )] + DuplicateWasmRuntime(Vec), + /// A convenience variant for String + #[deprecated(note = "Introduce more typed error variants as needed")] #[error("{0}")] Msg(String), } -impl<'a> From<&'a str> for Error { - fn from(s: &'a str) -> Self { - Error::Msg(s.into()) - } -} - -impl From for Error { - fn from(s: String) -> Self { - Error::Msg(s) - } -} - impl From> for Error { fn from(e: Box) -> Self { Self::from_state(e) From 91744cd3743112e93d16bfafb34e363632a2f8ab Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 3 Nov 2020 13:08:52 +0100 Subject: [PATCH 02/35] fixup changes --- client/cli/src/error.rs | 12 ++++------- client/service/src/client/wasm_override.rs | 7 ++----- primitives/blockchain/src/error.rs | 23 ++++++++++++++++++---- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/client/cli/src/error.rs b/client/cli/src/error.rs index 36c963f3e8c97..042fc382b239b 100644 --- a/client/cli/src/error.rs +++ b/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), /// 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), } impl std::convert::From<&str> for Error { @@ -93,7 +89,7 @@ impl std::convert::From<&str> for Error { impl std::convert::From for Error { fn from(s: String) -> Error { - Error::Input(s.to_string()) + Error::Input(s) } } diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index 748588f939c3e..fea3e18e5a05a 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -120,11 +120,8 @@ where /// Returns a hashmap of the runtime version and wasm runtime code. fn scrape_overrides(dir: &Path, executor: &E) -> Result> { - let handle_err = { - let dir = dir.to_owned(); - move |e: std::io::Error | -> sp_blockchain::Error { - sp_blockchain::Error::WasmOverrideIo(dir, e) - } + let handle_err = |e: std::io::Error | -> sp_blockchain::Error { + sp_blockchain::Error::WasmOverrideIo(dir.to_owned(), e) }; if !dir.is_dir() { diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 9fedfebc9e40e..bf38a5ae31a09 100644 --- a/primitives/blockchain/src/error.rs +++ b/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; @@ -65,6 +65,7 @@ pub enum Error { ApplyExtrinsicFailed(#[from] ApplyExtrinsicFailed), /// Execution error. + // `inner` cannot be made member, since it lacks `std::error::Error` trait bounds. #[error("Execution failed: {0:?}")] Execution(Box), @@ -166,14 +167,16 @@ pub enum Error { WasmOverrideIo(PathBuf, #[source] std::io::Error), #[error("Overwriting WASM requires a directory where local \ - WASM is stored. {0} is not a directory", .0.display())] + WASM is stored. {} is not a directory", .0.display())] WasmOverrideNotADirectory(PathBuf), #[error("Duplicate WASM Runtimes found: \n{}\n", .0.join("\n") )] DuplicateWasmRuntime(Vec), - /// A convenience variant for String - #[deprecated(note = "Introduce more typed error variants as needed")] + #[error(transparent)] + Foreign(#[from] Box), + + /// Should be avoided if possible, use `Foreign` instead. #[error("{0}")] Msg(String), } @@ -190,6 +193,18 @@ impl From> for Error { } } +impl From 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) -> Self { From c1085ebf11c5cc9aa5dd42781f5d19199e7928f0 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 3 Nov 2020 16:04:28 +0100 Subject: [PATCH 03/35] fix test looking for gone ::Msg variant --- client/service/src/client/wasm_override.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index fea3e18e5a05a..1de9380472f77 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -233,12 +233,9 @@ 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::>(); - assert!(is_match.len() >= 1) + sp_blockchain::Error::DuplicateWasmRuntime(duplicates) => { + assert_eq!(duplicates.get(0).map(|x| x.as_str()), Some("test0.wasm")); + assert_eq!(duplicates.get(1).map(|x| x.as_str()), Some("test1.wasm")); }, _ => panic!("Test should end with Msg Error Variant") } From 524ee71788247cbe7b8e54175b8549d058272cdc Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 4 Nov 2020 11:03:33 +0100 Subject: [PATCH 04/35] another test fix --- client/service/src/client/wasm_override.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index 1de9380472f77..c57d2bef0d12c 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -234,8 +234,7 @@ mod tests { Err(e) => { match e { sp_blockchain::Error::DuplicateWasmRuntime(duplicates) => { - assert_eq!(duplicates.get(0).map(|x| x.as_str()), Some("test0.wasm")); - assert_eq!(duplicates.get(1).map(|x| x.as_str()), Some("test1.wasm")); + assert_eq!(duplicates.len(), 2); }, _ => panic!("Test should end with Msg Error Variant") } From 2a09d3c9d85ddcc685e754edb00fe6ba9ee4b2fa Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 4 Nov 2020 15:52:53 +0100 Subject: [PATCH 05/35] one is duplicate, the other is not, so duplicates reported are n-1 --- client/service/src/client/wasm_override.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index c57d2bef0d12c..4924e6d4c7e0a 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -234,7 +234,7 @@ mod tests { Err(e) => { match e { sp_blockchain::Error::DuplicateWasmRuntime(duplicates) => { - assert_eq!(duplicates.len(), 2); + assert_eq!(duplicates.len(), 1); }, _ => panic!("Test should end with Msg Error Variant") } From dbbd20f9518b7ab5959cf5737ba18753084d8458 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 10 Nov 2020 17:03:27 +0100 Subject: [PATCH 06/35] darn spaces Co-authored-by: Andronik Ordian --- client/service/src/client/wasm_override.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index 4924e6d4c7e0a..53985ee08585a 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -121,7 +121,7 @@ where fn scrape_overrides(dir: &Path, executor: &E) -> Result> { let handle_err = |e: std::io::Error | -> sp_blockchain::Error { - sp_blockchain::Error::WasmOverrideIo(dir.to_owned(), e) + sp_blockchain::Error::WasmOverrideIo(dir.to_owned(), e) }; if !dir.is_dir() { From 8d021f03b3f29f8e1da25c0948115f60e94e55ca Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 10 Nov 2020 20:12:28 +0100 Subject: [PATCH 07/35] remove pointless doc comments of error variants without any value --- Cargo.lock | 9 ++-- client/cli/src/error.rs | 33 ++++++------- client/service/Cargo.toml | 4 +- client/service/src/error.rs | 60 +++++++++++------------- client/transaction-pool/Cargo.toml | 2 +- client/transaction-pool/src/error.rs | 30 +++++------- primitives/blockchain/src/error.rs | 27 ----------- primitives/state-machine/src/error.rs | 8 ++-- primitives/transaction-pool/Cargo.toml | 3 +- primitives/transaction-pool/src/error.rs | 41 ++++++++-------- 10 files changed, 90 insertions(+), 127 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 450ea9f3852f1..4528eba0ba00f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1158,9 +1158,9 @@ checksum = "72aa14c04dfae8dd7d8a2b1cb7ca2152618cd01336dbfe704b8dcbf8d41dbd69" [[package]] name = "derive_more" -version = "0.99.9" +version = "0.99.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "298998b1cf6b5b2c8a7b023dfd45821825ce3ba8a8af55c921a0e734e4653f76" +checksum = "41cb0e6161ad61ed084a36ba71fbba9e3ac5aee3606fb607fe08da6acbcf3d8c" dependencies = [ "proc-macro2", "quote", @@ -7319,7 +7319,6 @@ name = "sc-service" version = "0.8.0" dependencies = [ "async-std", - "derive_more", "directories", "exit-future", "futures 0.1.29", @@ -7378,6 +7377,7 @@ dependencies = [ "substrate-test-runtime", "substrate-test-runtime-client", "tempfile", + "thiserror", "tokio 0.2.22", "tracing", "tracing-futures", @@ -7518,7 +7518,6 @@ name = "sc-transaction-pool" version = "2.0.0" dependencies = [ "assert_matches", - "derive_more", "futures 0.3.5", "futures-diagnose", "hex", @@ -7542,6 +7541,7 @@ dependencies = [ "substrate-prometheus-endpoint", "substrate-test-runtime-client", "substrate-test-runtime-transaction-pool", + "thiserror", "wasm-timer", ] @@ -8642,6 +8642,7 @@ dependencies = [ "sp-api", "sp-blockchain", "sp-runtime", + "thiserror", ] [[package]] diff --git a/client/cli/src/error.rs b/client/cli/src/error.rs index 042fc382b239b..5190cae2c2ff8 100644 --- a/client/cli/src/error.rs +++ b/client/cli/src/error.rs @@ -25,32 +25,32 @@ pub type Result = std::result::Result; /// 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, - /// 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. @@ -58,24 +58,25 @@ pub enum Error { /// 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), + /// Application specific error chain sequence forwarder. #[error(transparent)] Application(#[from] Box), diff --git a/client/service/Cargo.toml b/client/service/Cargo.toml index 3569b2e7e5853..4ad33ed24bfaf 100644 --- a/client/service/Cargo.toml +++ b/client/service/Cargo.toml @@ -24,7 +24,7 @@ wasmtime = [ test-helpers = [] [dependencies] -derive_more = "0.99.2" +thiserror = "1.0.21" futures01 = { package = "futures", version = "0.1.29" } futures = { version = "0.3.4", features = ["compat"] } jsonrpc-pubsub = "15.0" @@ -32,7 +32,7 @@ jsonrpc-core = "15.0" rand = "0.7.3" parking_lot = "0.10.0" lazy_static = "1.4.0" -log = "0.4.8" +log = "0.4.11" slog = { version = "2.5.2", features = ["nested-values"] } futures-timer = "3.0.1" wasm-timer = "0.2" diff --git a/client/service/src/error.rs b/client/service/src/error.rs index ffe1b39405501..07c87e194c449 100644 --- a/client/service/src/error.rs +++ b/client/service/src/error.rs @@ -27,25 +27,34 @@ use sp_blockchain; pub type Result = std::result::Result; /// Service errors. -#[derive(Debug, derive_more::Display, derive_more::From)] +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] pub enum Error { - /// Client error. - Client(sp_blockchain::Error), - /// IO error. - Io(std::io::Error), - /// Consensus error. - Consensus(sp_consensus::Error), - /// Network error. - Network(sc_network::error::Error), - /// Keystore error. - Keystore(sc_keystore::Error), - /// Best chain selection strategy is missing. - #[display(fmt="Best chain selection strategy (SelectChain) is not provided.")] + #[error(transparent)] + Client(#[from] sp_blockchain::Error), + + #[error(transparent)] + Io(#[from] std::io::Error), + + #[error(transparent)] + Consensus(#[from] sp_consensus::Error), + + #[error(transparent)] + Network(#[from] sc_network::error::Error), + + #[error(transparent)] + Keystore(#[from] sc_keystore::Error), + + #[error("Best chain selection strategy (SelectChain) is not provided.")] SelectChainRequired, - /// Tasks executor is missing. - #[display(fmt="Tasks executor hasn't been provided.")] + + #[error("Tasks executor hasn't been provided.")] TaskExecutorRequired, - /// Other error. + + #[error("Prometheus metrics error")] + Prometheus(#[from] prometheus_endpoint::PrometheusError), + + #[error("Other: {0}")] Other(String), } @@ -55,21 +64,8 @@ impl<'a> From<&'a str> for Error { } } -impl From for Error { - fn from(e: prometheus_endpoint::PrometheusError) -> Self { - Error::Other(format!("Prometheus error: {}", e)) - } -} - -impl std::error::Error for Error { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Error::Client(ref err) => Some(err), - Error::Io(ref err) => Some(err), - Error::Consensus(ref err) => Some(err), - Error::Network(ref err) => Some(err), - Error::Keystore(ref err) => Some(err), - _ => None, - } +impl<'a> From for Error { + fn from(s: String) -> Self { + Error::Other(s) } } diff --git a/client/transaction-pool/Cargo.toml b/client/transaction-pool/Cargo.toml index 5db37f5368387..a4d7bc685c99b 100644 --- a/client/transaction-pool/Cargo.toml +++ b/client/transaction-pool/Cargo.toml @@ -14,7 +14,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] codec = { package = "parity-scale-codec", version = "1.3.4" } -derive_more = "0.99.2" +thiserror = "1.0.21" futures = { version = "0.3.1", features = ["compat"] } futures-diagnose = "1.0" intervalier = "0.4.0" diff --git a/client/transaction-pool/src/error.rs b/client/transaction-pool/src/error.rs index c0f795df1801a..49fc433e320cc 100644 --- a/client/transaction-pool/src/error.rs +++ b/client/transaction-pool/src/error.rs @@ -24,30 +24,22 @@ use sp_transaction_pool::error::Error as TxPoolError; pub type Result = std::result::Result; /// Transaction pool error type. -#[derive(Debug, derive_more::Display, derive_more::From)] +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] pub enum Error { - /// Pool error. - Pool(TxPoolError), - /// Blockchain error. - Blockchain(sp_blockchain::Error), - /// Error while converting a `BlockId`. - #[from(ignore)] + #[error("Transaction pool error")] + Pool(#[from] TxPoolError), + + #[error("Blockchain error")] + Blockchain(#[from] sp_blockchain::Error), + + #[error("Block conversion error: {0}")] BlockIdConversion(String), - /// Error while calling the runtime api. - #[from(ignore)] + + #[error("Runtime error: {0}")] RuntimeApi(String), } -impl std::error::Error for Error { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Error::Pool(ref err) => Some(err), - Error::Blockchain(ref err) => Some(err), - Error::BlockIdConversion(_) => None, - Error::RuntimeApi(_) => None, - } - } -} impl sp_transaction_pool::error::IntoPoolError for Error { fn into_pool_error(self) -> std::result::Result { diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index bf38a5ae31a09..b75f72507a44f 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -48,112 +48,85 @@ pub enum ApplyExtrinsicFailed { #[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), - /// Blockchain error. #[error("Blockchain")] Blockchain(#[source] Box), - /// 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), diff --git a/primitives/state-machine/src/error.rs b/primitives/state-machine/src/error.rs index 0b02c68f79f5c..f20f9e530dc7f 100644 --- a/primitives/state-machine/src/error.rs +++ b/primitives/state-machine/src/error.rs @@ -32,18 +32,18 @@ impl Error for T {} /// would not be executed unless externalities were available. This is included for completeness, /// and as a transition away from the pre-existing framework. #[derive(Debug, Eq, PartialEq)] +#[allow(missing_docs)] #[cfg_attr(feature = "std", derive(thiserror::Error))] pub enum ExecutionError { - /// Backend error. #[cfg_attr(feature = "std", error("Backend error {0:?}"))] Backend(crate::DefaultError), - /// The entry `:code` doesn't exist in storage so there's no way we can execute anything. + #[cfg_attr(feature = "std", error("`:code` entry does not exist in storage"))] CodeEntryDoesNotExist, - /// Backend is incompatible with execution proof generation process. + #[cfg_attr(feature = "std", error("Unable to generate proof"))] UnableToGenerateProof, - /// Invalid execution proof. + #[cfg_attr(feature = "std", error("Invalid execution proof"))] InvalidProof, } diff --git a/primitives/transaction-pool/Cargo.toml b/primitives/transaction-pool/Cargo.toml index 57ba3a28ac3c1..030c2c4f17ac3 100644 --- a/primitives/transaction-pool/Cargo.toml +++ b/primitives/transaction-pool/Cargo.toml @@ -14,8 +14,9 @@ readme = "README.md" targets = ["x86_64-unknown-linux-gnu"] [dependencies] +thiserror = "1.0.21" codec = { package = "parity-scale-codec", version = "1.3.1", optional = true } -derive_more = { version = "0.99.2", optional = true } +derive_more = { version = "0.99.11", optional = true } futures = { version = "0.3.1", optional = true } log = { version = "0.4.8", optional = true } serde = { version = "1.0.101", features = ["derive"], optional = true} diff --git a/primitives/transaction-pool/src/error.rs b/primitives/transaction-pool/src/error.rs index 531b397cb946c..3fa4bb7bfc1e3 100644 --- a/primitives/transaction-pool/src/error.rs +++ b/primitives/transaction-pool/src/error.rs @@ -25,49 +25,48 @@ use sp_runtime::transaction_validity::{ pub type Result = std::result::Result; /// Transaction pool error type. -#[derive(Debug, derive_more::Display, derive_more::From)] +#[derive(Debug, thiserror::Error, derive_more::From)] +#[allow(missing_docs)] pub enum Error { - /// Transaction is not verifiable yet, but might be in the future. - #[display(fmt="Unknown transaction validity: {:?}", _0)] + #[error("Unknown transaction validity: {0:?}")] UnknownTransaction(UnknownTransaction), - /// Transaction is invalid. - #[display(fmt="Invalid transaction validity: {:?}", _0)] + + #[error("Invalid transaction validity: {0:?}")] InvalidTransaction(InvalidTransaction), + /// The transaction validity returned no "provides" tag. /// /// Such transactions are not accepted to the pool, since we use those tags /// to define identity of transactions (occupance of the same "slot"). - #[display(fmt="The transaction does not provide any tags, so the pool can't identify it.")] + #[error("Transaction does not provide any tags, so the pool can't identify it")] NoTagsProvided, - /// The transaction is temporarily banned. - #[display(fmt="Temporarily Banned")] + + #[error("Transaction temporarily Banned")] TemporarilyBanned, - /// The transaction is already in the pool. - #[display(fmt="[{:?}] Already imported", _0)] + + #[error("[{0:?}] Already imported")] AlreadyImported(Box), - /// The transaction cannot be imported cause it's a replacement and has too low priority. - #[display(fmt="Too low priority ({} > {})", old, new)] + + #[error("Too low priority ({} > {})", old, new)] TooLowPriority { /// Transaction already in the pool. old: Priority, /// Transaction entering the pool. new: Priority }, - /// Deps cycle detected and we couldn't import transaction. - #[display(fmt="Cycle Detected")] + #[error("Transaction with cyclic dependency")] CycleDetected, - /// Transaction was dropped immediately after it got inserted. - #[display(fmt="Transaction couldn't enter the pool because of the limit.")] + + #[error("Transaction couldn't enter the pool because of the limit")] ImmediatelyDropped, - /// Invalid block id. + + #[error("{0}")] InvalidBlockId(String), - /// The pool is not accepting future transactions. - #[display(fmt="The pool is not accepting future transactions")] + + #[error("The pool is not accepting future transactions")] RejectedFutureTransaction, } -impl std::error::Error for Error {} - /// Transaction pool error conversion. pub trait IntoPoolError: std::error::Error + Send + Sized { /// Try to extract original `Error` From 8b3f12b5193316148b87032f1a2acdf9ebbc3406 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 13 Nov 2020 18:23:21 +0100 Subject: [PATCH 08/35] low hanging fruits (for a tall person) --- client/consensus/slots/src/lib.rs | 9 +-- client/db/src/lib.rs | 12 ++-- client/light/src/call_executor.rs | 2 +- client/light/src/fetcher.rs | 14 ++--- client/network/src/on_demand_layer.rs | 12 ++-- client/service/src/error.rs | 3 + client/state-db/Cargo.toml | 3 +- client/sync-state-rpc/src/lib.rs | 4 +- .../api/proc-macro/src/decl_runtime_apis.rs | 8 +-- primitives/api/src/lib.rs | 4 +- primitives/blockchain/src/backend.rs | 4 +- primitives/blockchain/src/error.rs | 62 +++++++++++-------- 12 files changed, 72 insertions(+), 65 deletions(-) diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index 681d4a6273ed9..afee73a04889c 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -515,10 +515,11 @@ impl SlotDuration { } }?; - 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(), - )) + { + let slot_duration = slot_duration.slot_duration(); + if slot_duration == 0u64 { + return Err(sp_blockchain::Error::SlotDurationInvalid(slot_duration)) + } } Ok(slot_duration) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 8196a750557a8..255be152ca9dd 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -868,9 +868,7 @@ impl Backend { 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| sp_blockchain::Error::from( - format!("State database error: {:?}", e) - ); + let map_e = |e: sc_state_db::Error| sp_blockchain::Error::from_state_db(e); let state_db: StateDb<_, _> = StateDb::new( config.pruning.clone(), !config.source.supports_ref_counting(), @@ -1059,7 +1057,7 @@ impl Backend { trace!(target: "db", "Canonicalize block #{} ({:?})", new_canonical, hash); let commit = self.storage.state_db.canonicalize_block(&hash) - .map_err(|e: sc_state_db::Error| sp_blockchain::Error::from(format!("State database error: {:?}", e)))?; + .map_err(|e: sc_state_db::Error| sp_blockchain::Error::from_state_db(e))?; apply_state_commit(transaction, commit); }; @@ -1195,9 +1193,7 @@ impl Backend { number_u64, &pending_block.header.parent_hash(), changeset, - ).map_err(|e: sc_state_db::Error| - sp_blockchain::Error::from(format!("State database error: {:?}", e)) - )?; + ).map_err(|e: sc_state_db::Error| sp_blockchain::Error::from_state_db(e))?; apply_state_commit(&mut transaction, commit); // Check if need to finalize. Genesis is always finalized instantly. @@ -1352,7 +1348,7 @@ impl Backend { 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| sp_blockchain::Error::from(format!("State database error: {:?}", e)))?; + .map_err(|e: sc_state_db::Error| sp_blockchain::Error::from_state_db(e))?; apply_state_commit(transaction, commit); if !f_num.is_zero() { diff --git a/client/light/src/call_executor.rs b/client/light/src/call_executor.rs index fa0f02cd5aed9..d115391415dae 100644 --- a/client/light/src/call_executor.rs +++ b/client/light/src/call_executor.rs @@ -276,7 +276,7 @@ pub fn check_execution_proof_with_make_header( // TODO: Remove when solved: https://github.com/paritytech/substrate/issues/5047 let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&trie_backend); - let runtime_code = backend_runtime_code.runtime_code()?; + let runtime_code = backend_runtime_code.runtime_code().map_err(|_e| ClientError::RuntimeCodeMissing)?; execution_proof_check_on_trie_backend::( &trie_backend, diff --git a/client/light/src/fetcher.rs b/client/light/src/fetcher.rs index 33113c2fc7df0..60fce87b8d0c2 100644 --- a/client/light/src/fetcher.rs +++ b/client/light/src/fetcher.rs @@ -239,7 +239,7 @@ impl FetchChecker for LightDataChecker convert_hash(request.header.state_root()), remote_proof, request.keys.iter(), - ).map_err(Into::into) + ).map_err(|e| ClientError::from(e)) } fn check_read_child_proof( @@ -249,14 +249,14 @@ impl FetchChecker for LightDataChecker ) -> ClientResult, Option>>> { let child_info = match ChildType::from_prefixed_key(&request.storage_key) { Some((ChildType::ParentKeyId, storage_key)) => ChildInfo::new_default(storage_key), - None => return Err("Invalid child type".into()), + None => return Err(ClientError::InvalidChildType), }; read_child_proof_check::( convert_hash(request.header.state_root()), remote_proof, &child_info, request.keys.iter(), - ).map_err(Into::into) + ).map_err(|e| ClientError::from(e)) } fn check_execution_proof( @@ -292,10 +292,10 @@ impl FetchChecker for LightDataChecker if *request.header.extrinsics_root() == extrinsics_root { Ok(body) } else { - Err(format!("RemoteBodyRequest: invalid extrinsics root expected: {} but got {}", - *request.header.extrinsics_root(), - extrinsics_root, - ).into()) + Err(ClientError::ExtrinsicRootInvalid { + received: request.header.extrinsics_root().to_string(), + expected: extrinsics_root.to_string(), + }) } } diff --git a/client/network/src/on_demand_layer.rs b/client/network/src/on_demand_layer.rs index 084172ee57c4f..064277a6c40e0 100644 --- a/client/network/src/on_demand_layer.rs +++ b/client/network/src/on_demand_layer.rs @@ -65,7 +65,7 @@ impl FetchChecker for AlwaysBadChecker { _remote_header: Option, _remote_proof: StorageProof, ) -> Result { - Err(ClientError::Msg("AlwaysBadChecker".into())) + Err(ClientError::AlwaysBadChecker) } fn check_read_proof( @@ -73,7 +73,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteReadRequest, _remote_proof: StorageProof, ) -> Result,Option>>, ClientError> { - Err(ClientError::Msg("AlwaysBadChecker".into())) + Err(ClientError::AlwaysBadChecker) } fn check_read_child_proof( @@ -81,7 +81,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteReadChildRequest, _remote_proof: StorageProof, ) -> Result, Option>>, ClientError> { - Err(ClientError::Msg("AlwaysBadChecker".into())) + Err(ClientError::AlwaysBadChecker) } fn check_execution_proof( @@ -89,7 +89,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteCallRequest, _remote_proof: StorageProof, ) -> Result, ClientError> { - Err(ClientError::Msg("AlwaysBadChecker".into())) + Err(ClientError::AlwaysBadChecker) } fn check_changes_proof( @@ -97,7 +97,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteChangesRequest, _remote_proof: ChangesProof ) -> Result, u32)>, ClientError> { - Err(ClientError::Msg("AlwaysBadChecker".into())) + Err(ClientError::AlwaysBadChecker) } fn check_body_proof( @@ -105,7 +105,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteBodyRequest, _body: Vec ) -> Result, ClientError> { - Err(ClientError::Msg("AlwaysBadChecker".into())) + Err(ClientError::AlwaysBadChecker) } } diff --git a/client/service/src/error.rs b/client/service/src/error.rs index 07c87e194c449..624e69a3c0da9 100644 --- a/client/service/src/error.rs +++ b/client/service/src/error.rs @@ -54,6 +54,9 @@ pub enum Error { #[error("Prometheus metrics error")] Prometheus(#[from] prometheus_endpoint::PrometheusError), + #[error("Application")] + Application(#[from] Box), + #[error("Other: {0}")] Other(String), } diff --git a/client/state-db/Cargo.toml b/client/state-db/Cargo.toml index 4d3e736d9539e..18facd720db25 100644 --- a/client/state-db/Cargo.toml +++ b/client/state-db/Cargo.toml @@ -13,8 +13,9 @@ readme = "README.md" targets = ["x86_64-unknown-linux-gnu"] [dependencies] +thiserror = "1.0.21" parking_lot = "0.10.0" -log = "0.4.8" +log = "0.4.11" sc-client-api = { version = "2.0.0", path = "../api" } sp-core = { version = "2.0.0", path = "../../primitives/core" } codec = { package = "parity-scale-codec", version = "1.3.4", features = ["derive"] } diff --git a/client/sync-state-rpc/src/lib.rs b/client/sync-state-rpc/src/lib.rs index e1c70d3c8fa20..6d1fc3c5cdae3 100644 --- a/client/sync-state-rpc/src/lib.rs +++ b/client/sync-state-rpc/src/lib.rs @@ -79,13 +79,13 @@ impl SyncStateRpcHandler fn build_sync_state(&self) -> Result, 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::MissingHashInHeader(finalized_hash.to_string()))?; + .ok_or_else(|| sp_blockchain::Error::MissingHeader(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::MissingBlockWeightInHeader(finalized_hash.to_string()))?; + .ok_or_else(|| sp_blockchain::Error::LoadingBlockWeightFailed(finalized_hash.to_string()))?; Ok(sc_chain_spec::LightSyncState { finalized_block_header: finalized_header, diff --git a/primitives/api/proc-macro/src/decl_runtime_apis.rs b/primitives/api/proc-macro/src/decl_runtime_apis.rs index a628ade6f9b47..640bd543fc2de 100644 --- a/primitives/api/proc-macro/src/decl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/decl_runtime_apis.rs @@ -708,13 +708,7 @@ impl<'a> ToClientSideDecl<'a> { }, #crate_::NativeOrEncoded::Encoded(r) => { <#ret_type as #crate_::Decode>::decode(&mut &r[..]) - .map_err(|err| - format!( - "Failed to decode result of `{}`: {}", - #function_name, - err.what(), - ).into() - ) + .map_err(|err| { (#function_name, err).into() }) } } ) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 9dadce3b55452..a52fcdccc90dd 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -397,7 +397,7 @@ pub trait ConstructRuntimeApi> { #[cfg(feature = "std")] pub trait ApiErrorExt { /// Error type used by the runtime apis. - type Error: std::fmt::Debug + From; + type Error: std::fmt::Debug + From<(&'static str, codec::Error)>; } /// Extends the runtime api implementation with some common functionality. @@ -506,7 +506,7 @@ pub struct CallApiAtParams<'a, Block: BlockT, C, NC, Backend: StateBackend { /// Error type used by the implementation. - type Error: std::fmt::Debug + From; + type Error: std::fmt::Debug + From<(&'static str, codec::Error)>; /// The state backend that is used to store the block states. type StateBackend: StateBackend>; diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index 4352a4de1d53a..22035268805f7 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -172,7 +172,7 @@ pub trait Backend: HeaderBackend + HeaderMetadata: HeaderBackend + HeaderMetadata = result::Result; /// Error when the runtime failed to apply an extrinsic. #[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] pub enum ApplyExtrinsicFailed { /// The transaction cannot be included into the current block. /// @@ -36,12 +37,8 @@ pub enum ApplyExtrinsicFailed { #[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), + #[error("Application specific error")] + Application(#[source] Box), } /// Substrate Client error @@ -60,6 +57,12 @@ pub enum Error { #[error(transparent)] ApplyExtrinsicFailed(#[from] ApplyExtrinsicFailed), + #[error("Child type is invalid")] + InvalidChildType, + + #[error("RemoteBodyRequest: invalid extrinsics root expected: {expected} but got {received}")] + ExtrinsicRootInvalid{ received: String, expected: String}, + // `inner` cannot be made member, since it lacks `std::error::Error` trait bounds. #[error("Execution failed: {0:?}")] Execution(Box), @@ -97,8 +100,11 @@ pub enum Error { #[error("Error decoding call result of {0}")] CallResultDecode(&'static str, #[source] CodecError), - #[error("Error converting `{0}` between runtime and node")] - RuntimeParamConversion(String), + #[error("Decoding of {0} failed")] + RuntimeApiCodecError(&'static str, #[source] codec::Error), + + #[error("Runtime :code missing in storage")] + RuntimeCodeMissing, #[error("Changes tries are not supported by the runtime")] ChangesTriesNotSupported, @@ -131,10 +137,10 @@ pub enum Error { DatabaseError(#[from] sp_database::error::DatabaseError), #[error("Failed to get header for hash {0}")] - MissingHashInHeader(String), + MissingHeader(String), #[error("Failed to load the block weight for block {0}")] - MissingBlockWeightInHeader(String), + LoadingBlockWeightFailed(String), #[error("WASM override IO error")] WasmOverrideIo(PathBuf, #[source] std::io::Error), @@ -146,16 +152,21 @@ pub enum Error { #[error("Duplicate WASM Runtimes found: \n{}\n", .0.join("\n") )] DuplicateWasmRuntime(Vec), + #[error("State Database error: {0}")] + StateDatabase(String), + + #[error("Invalid value for slot_duration: the value ({0}) must be greater than 0.")] + SlotDurationInvalid(u64), + #[error(transparent)] - Foreign(#[from] Box), + Application(#[from] Box), - /// Should be avoided if possible, use `Foreign` instead. - #[error("{0}")] - Msg(String), + #[error("AlwaysBadChecker")] + AlwaysBadChecker, } -impl From> for Error { - fn from(e: Box) -> Self { +impl From> for Error { + fn from(e: Box) -> Self { Self::from_state(e) } } @@ -166,18 +177,12 @@ impl From> for Error { } } -impl From 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 From<(&'static str, codec::Error)> for Error { + fn from(x: (&'static str, codec::Error)) -> Self { + Self::RuntimeApiCodecError(x.0, x.1) } } - impl Error { /// Chain a blockchain error. pub fn from_blockchain(e: Box) -> Self { @@ -188,4 +193,11 @@ impl Error { pub fn from_state(e: Box) -> Self { Error::Execution(e) } + + /// Construct from a state db error. + // Can not be done directly, since that would make cargo run out of stack if + // `sc-state-db` is lib is added as dependency. + pub fn from_state_db(e: E) -> Self where E: std::fmt::Debug { + Error::StateDatabase(format!("{:?}", e)) + } } From 2dbc971934e8975b786ad068516899cf93dc89df Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Sat, 14 Nov 2020 11:47:38 +0100 Subject: [PATCH 09/35] moar error type variants --- Cargo.lock | 1 + client/basic-authorship/src/basic_authorship.rs | 5 +---- client/block-builder/src/lib.rs | 2 +- client/network/src/light_client_handler.rs | 2 +- client/rpc-servers/src/middleware.rs | 2 +- client/rpc/src/state/state_full.rs | 6 +++--- client/sync-state-rpc/src/lib.rs | 2 +- primitives/blockchain/Cargo.toml | 1 + primitives/blockchain/src/error.rs | 15 +++++++++++++++ 9 files changed, 25 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 25672704e4776..3551749bba966 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8154,6 +8154,7 @@ dependencies = [ name = "sp-blockchain" version = "2.0.0" dependencies = [ + "futures 0.3.5", "log", "lru 0.4.3", "parity-scale-codec", diff --git a/client/basic-authorship/src/basic_authorship.rs b/client/basic-authorship/src/basic_authorship.rs index 2fe7ba72ec7b9..8c022ef3a9741 100644 --- a/client/basic-authorship/src/basic_authorship.rs +++ b/client/basic-authorship/src/basic_authorship.rs @@ -208,10 +208,7 @@ impl sp_consensus::Proposer for })); async move { - match rx.await { - Ok(x) => x, - Err(err) => Err(sp_blockchain::Error::Msg(err.to_string())) - } + rx.await? }.boxed() } } diff --git a/client/block-builder/src/lib.rs b/client/block-builder/src/lib.rs index 8a38bb8478003..cc1431ea349bf 100644 --- a/client/block-builder/src/lib.rs +++ b/client/block-builder/src/lib.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: ::new(header, self.extrinsics), diff --git a/client/network/src/light_client_handler.rs b/client/network/src/light_client_handler.rs index e7c5e9c1c9b95..5a0b2bcd733bd 100644 --- a/client/network/src/light_client_handler.rs +++ b/client/network/src/light_client_handler.rs @@ -627,7 +627,7 @@ where let prefixed_key = PrefixedStorageKey::new_ref(&request.storage_key); let child_info = match ChildType::from_prefixed_key(prefixed_key) { Some((ChildType::ParentKeyId, storage_key)) => Ok(ChildInfo::new_default(storage_key)), - None => Err("Invalid child storage key".into()), + None => Err(sp_blockchain::Error::InvalidChildStorageKey), }; let proof = match child_info.and_then(|child_info| self.chain.read_child_proof( &BlockId::Hash(block), diff --git a/client/rpc-servers/src/middleware.rs b/client/rpc-servers/src/middleware.rs index 74139714c8cb7..439bd7d6f1b7f 100644 --- a/client/rpc-servers/src/middleware.rs +++ b/client/rpc-servers/src/middleware.rs @@ -48,7 +48,7 @@ impl RpcMetrics { &["protocol"] ).ok()?, r).ok()?, }) - }).ok_or(PrometheusError::Msg("Cannot register metric".to_string())) + }).ok_or(PrometheusError::RegisteringMetricFailed) } } diff --git a/client/rpc/src/state/state_full.rs b/client/rpc/src/state/state_full.rs index fda73cea27110..a1b9fbc4eebc5 100644 --- a/client/rpc/src/state/state_full.rs +++ b/client/rpc/src/state/state_full.rs @@ -541,7 +541,7 @@ impl ChildStateBackend for FullState ChildInfo::new_default(storage_key), - None => return Err("Invalid child storage key".into()), + None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client.child_storage_keys( &BlockId::Hash(block), @@ -563,7 +563,7 @@ impl ChildStateBackend for FullState ChildInfo::new_default(storage_key), - None => return Err("Invalid child storage key".into()), + None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client.child_storage( &BlockId::Hash(block), @@ -585,7 +585,7 @@ impl ChildStateBackend for FullState ChildInfo::new_default(storage_key), - None => return Err("Invalid child storage key".into()), + None => return Err(sp_blockchain::Error::InvalidChildStorageKey), }; self.client.child_storage_hash( &BlockId::Hash(block), diff --git a/client/sync-state-rpc/src/lib.rs b/client/sync-state-rpc/src/lib.rs index 6d1fc3c5cdae3..47b6b8b559d1e 100644 --- a/client/sync-state-rpc/src/lib.rs +++ b/client/sync-state-rpc/src/lib.rs @@ -120,5 +120,5 @@ impl SyncStateRpcApi for SyncStateRpcHandler } fn map_error(error: String) -> jsonrpc_core::Error { - Error(sp_blockchain::Error::Msg(error)).into() + Error(sp_blockchain::Error::JsonRpc(error)).into() } diff --git a/primitives/blockchain/Cargo.toml b/primitives/blockchain/Cargo.toml index f714aaaa1dae1..a516cc5a2a75b 100644 --- a/primitives/blockchain/Cargo.toml +++ b/primitives/blockchain/Cargo.toml @@ -18,6 +18,7 @@ log = "0.4.11" lru = "0.4.0" parking_lot = "0.10.0" thiserror = "1.0.21" +futures = "0.3" codec = { package = "parity-scale-codec", version = "1.3.1", default-features = false, features = ["derive"] } sp-consensus = { version = "0.8.0", path = "../consensus/common" } sp-runtime = { version = "2.0.0", path = "../runtime" } diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 1507a4f81b0a7..34f99814e6fe0 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -45,6 +45,12 @@ pub enum ApplyExtrinsicFailed { #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { + #[error("Cancelled oneshot channel {0}")] + OneShotCancelled(#[from] futures::channel::oneshot::Canceled), + + #[error("JsonRpc error: {0}")] + JsonRpc(String), + #[error(transparent)] Consensus(#[from] sp_consensus::Error), @@ -70,6 +76,15 @@ pub enum Error { #[error("Blockchain")] Blockchain(#[source] Box), + /// A error used by various storage subsystems. + /// + /// Eventually this will be replaced. + #[error("{0}")] + StorageChanges(sp_state_machine::DefaultError), + + #[error("Invalid child storage key")] + InvalidChildStorageKey, + #[error("Current state of blockchain has invalid authorities set")] InvalidAuthoritiesSet, From 3e94fa6b22152af56cee7d72a16124bd8bce2e98 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 17 Nov 2020 13:25:07 +0100 Subject: [PATCH 10/35] 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. --- Cargo.lock | 1 + client/rpc-servers/src/middleware.rs | 2 +- client/service/src/client/call_executor.rs | 22 +++++++++++++++++----- client/service/src/client/client.rs | 5 +++-- primitives/blockchain/src/error.rs | 10 ++++++++++ 5 files changed, 32 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3551749bba966..793d1a661f749 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7493,6 +7493,7 @@ dependencies = [ "parking_lot 0.10.2", "sc-client-api", "sp-core", + "thiserror", ] [[package]] diff --git a/client/rpc-servers/src/middleware.rs b/client/rpc-servers/src/middleware.rs index 439bd7d6f1b7f..5a72075ada1d7 100644 --- a/client/rpc-servers/src/middleware.rs +++ b/client/rpc-servers/src/middleware.rs @@ -48,7 +48,7 @@ impl RpcMetrics { &["protocol"] ).ok()?, r).ok()?, }) - }).ok_or(PrometheusError::RegisteringMetricFailed) + }).ok_or(PrometheusError::AlreadyReg) } } diff --git a/client/service/src/client/call_executor.rs b/client/service/src/client/call_executor.rs index 164976ecfe871..e89567e4d2134 100644 --- a/client/service/src/client/call_executor.rs +++ b/client/service/src/client/call_executor.rs @@ -137,7 +137,9 @@ where )?; let state = self.backend.state_at(*id)?; let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); - let runtime_code = self.check_override(state_runtime_code.runtime_code()?, id)?; + let runtime_code = state_runtime_code.runtime_code() + .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + let runtime_code = self.check_override(runtime_code, id)?; let return_data = StateMachine::new( &state, @@ -211,7 +213,10 @@ where let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&trie_state); // It is important to extract the runtime code here before we create the proof // recorder. - let runtime_code = self.check_override(state_runtime_code.runtime_code()?, at)?; + + let runtime_code = state_runtime_code.runtime_code() + .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + let runtime_code = self.check_override(runtime_code, at)?; let backend = sp_state_machine::ProvingBackend::new_with_recorder( trie_state, @@ -236,7 +241,9 @@ where }, None => { let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); - let runtime_code = self.check_override(state_runtime_code.runtime_code()?, at)?; + let runtime_code = state_runtime_code.runtime_code() + .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + let runtime_code = self.check_override(runtime_code, at)?; let mut state_machine = StateMachine::new( &state, @@ -273,7 +280,9 @@ where None, ); let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); - self.executor.runtime_version(&mut ext, &state_runtime_code.runtime_code()?) + let runtime_code = state_runtime_code.runtime_code() + .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + self.executor.runtime_version(&mut ext, &runtime_code) .map_err(|e| sp_blockchain::Error::VersionInvalid(format!("{:?}", e)).into()) } @@ -284,6 +293,9 @@ where method: &str, call_data: &[u8] ) -> Result<(Vec, StorageProof), sp_blockchain::Error> { + let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(trie_state); + let runtime_code = state_runtime_code.runtime_code() + .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; sp_state_machine::prove_execution_on_trie_backend::<_, _, NumberFor, _, _>( trie_state, overlay, @@ -291,7 +303,7 @@ where self.spawn_handle.clone(), method, call_data, - &sp_state_machine::backend::BackendRuntimeCode::new(trie_state).runtime_code()?, + &runtime_code, ) .map_err(Into::into) } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index d423fdee39b6c..50bdc7882ab1a 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -297,7 +297,8 @@ impl Client where config: ClientConfig, ) -> sp_blockchain::Result { if backend.blockchain().header(BlockId::Number(Zero::zero()))?.is_none() { - let genesis_storage = build_genesis_storage.build_storage()?; + let genesis_storage = build_genesis_storage.build_storage() + .map_err(|e| sp_blockchain::Error::Storage(e))?; let mut op = backend.begin_operation()?; backend.begin_state_operation(&mut op, BlockId::Hash(Default::default()))?; let state_root = op.reset_storage(genesis_storage)?; @@ -880,7 +881,7 @@ impl Client where &state, changes_trie_state.as_ref(), *parent_hash, - )?; + ).map_err(|e| sp_blockchain::Error::Storage(e))?; if import_block.header.state_root() != &gen_storage_changes.transaction_storage_root diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 34f99814e6fe0..607d6df30e6f5 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -178,6 +178,16 @@ pub enum Error { #[error("AlwaysBadChecker")] AlwaysBadChecker, + + // Should be removed/improved once + // the storage `fn`s returns typed errors. + #[error("Runtime code error: {0}")] + RuntimeCode(&'static str), + + // Should be removed/improved once + // the storage `fn`s returns typed errors. + #[error("Storage error: {0}")] + Storage(String), } impl From> for Error { From 666dd5f7afd74b5c91d33e52c7b07d99cffa51f7 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 17 Nov 2020 15:23:32 +0100 Subject: [PATCH 11/35] chore remove pointless error generic --- client/api/src/light.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/api/src/light.rs b/client/api/src/light.rs index 144851dac0075..3f98e5a4468f2 100644 --- a/client/api/src/light.rs +++ b/client/api/src/light.rs @@ -314,11 +314,9 @@ pub mod tests { pub type OkCallFetcher = Mutex>; - fn not_implemented_in_tests() -> Ready> - where - E: std::convert::From<&'static str>, + fn not_implemented_in_tests() -> Ready> { - futures::future::ready(Err("Not implemented on test node".into())) + futures::future::ready(Err(ClientError::Mock("Not implemented on test node"))) } impl Fetcher for OkCallFetcher { From 69e849bc862a7ff50aa7508bf5079d3252cfc52b Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 17 Nov 2020 15:23:53 +0100 Subject: [PATCH 12/35] fix test for mocks, add a bunch of non_exhaustive --- client/service/src/error.rs | 1 + .../api/proc-macro/src/mock_impl_runtime_apis.rs | 4 +++- primitives/api/test/tests/decl_and_impl.rs | 12 ++++++++---- primitives/blockchain/src/error.rs | 5 +++++ primitives/consensus/common/src/error.rs | 1 + 5 files changed, 18 insertions(+), 5 deletions(-) diff --git a/client/service/src/error.rs b/client/service/src/error.rs index 624e69a3c0da9..3515df78be876 100644 --- a/client/service/src/error.rs +++ b/client/service/src/error.rs @@ -29,6 +29,7 @@ pub type Result = std::result::Result; /// Service errors. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] +#[non_exhaustive] pub enum Error { #[error(transparent)] Client(#[from] sp_blockchain::Error), diff --git a/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs b/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs index 3e2fd42951b3c..bd8daaa8444f8 100644 --- a/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs @@ -69,7 +69,9 @@ fn implement_common_api_traits( ) -> Result { let crate_ = generate_crate_access(HIDDEN_INCLUDES_ID); - let error_type = error_type.map(|e| quote!(#e)).unwrap_or_else(|| quote!(String)); + let error_type = error_type + .map(|e| quote!(#e)) + .unwrap_or_else(|| quote!( (&'static str, #crate_::codec::Error) )); // Quote using the span from `error_type` to generate nice error messages when the type is // not implementing a trait or similar. diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index 594882baf1e34..350aca8a49f52 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -103,17 +103,18 @@ mock_impl_runtime_apis! { } #[advanced] - fn same_name(_: &BlockId) -> std::result::Result, String> { + fn same_name(_: &BlockId) -> std::result::Result, (&'static str, ::codec::Error)> { Ok(().into()) } #[advanced] - fn wild_card(at: &BlockId, _: u32) -> std::result::Result, String> { + fn wild_card(at: &BlockId, _: u32) -> std::result::Result, (&'static str, ::codec::Error)> { if let BlockId::Number(1337) = at { // yeah Ok(().into()) } else { - Err("Ohh noooo".into()) + let e = ::codec::Error::from("Ohh noooo"); + Err(("MockApi", e)) } } } @@ -197,5 +198,8 @@ fn mock_runtime_api_works_with_advanced() { Api::::same_name(&mock, &BlockId::Number(0)).unwrap(); mock.wild_card(&BlockId::Number(1337), 1).unwrap(); - assert_eq!(String::from("Ohh noooo"), mock.wild_card(&BlockId::Number(1336), 1).unwrap_err()); + assert_eq!( + ("MockApi", ::codec::Error::from("Ohh noooo")), + mock.wild_card(&BlockId::Number(1336), 1).unwrap_err() + ); } diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 607d6df30e6f5..8eaff7a276e71 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -44,6 +44,7 @@ pub enum ApplyExtrinsicFailed { /// Substrate Client error #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] +#[non_exhaustive] pub enum Error { #[error("Cancelled oneshot channel {0}")] OneShotCancelled(#[from] futures::channel::oneshot::Canceled), @@ -188,6 +189,10 @@ pub enum Error { // the storage `fn`s returns typed errors. #[error("Storage error: {0}")] Storage(String), + + /// Unit test helper for mocked modules. + #[error("Mock: {0}")] + Mock(&'static str), } impl From> for Error { diff --git a/primitives/consensus/common/src/error.rs b/primitives/consensus/common/src/error.rs index a21bcf6cca9b2..11b24d273d5ec 100644 --- a/primitives/consensus/common/src/error.rs +++ b/primitives/consensus/common/src/error.rs @@ -25,6 +25,7 @@ pub type Result = std::result::Result; /// Error type. #[derive(Debug, thiserror::Error)] +#[non_exhaustive] pub enum Error { /// Missing state at block with given descriptor. #[error("State unavailable at block {0}")] From d54cb37ce3d898352b892dc812ecd265de9a9f09 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 17 Nov 2020 15:41:29 +0100 Subject: [PATCH 13/35] max line width --- client/light/src/call_executor.rs | 3 ++- primitives/api/test/tests/decl_and_impl.rs | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/client/light/src/call_executor.rs b/client/light/src/call_executor.rs index d115391415dae..458ea2bd6b844 100644 --- a/client/light/src/call_executor.rs +++ b/client/light/src/call_executor.rs @@ -276,7 +276,8 @@ pub fn check_execution_proof_with_make_header( // TODO: Remove when solved: https://github.com/paritytech/substrate/issues/5047 let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&trie_backend); - let runtime_code = backend_runtime_code.runtime_code().map_err(|_e| ClientError::RuntimeCodeMissing)?; + let runtime_code = backend_runtime_code.runtime_code() + .map_err(|_e| ClientError::RuntimeCodeMissing)?; execution_proof_check_on_trie_backend::( &trie_backend, diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index 350aca8a49f52..b3989496e232f 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -103,12 +103,22 @@ mock_impl_runtime_apis! { } #[advanced] - fn same_name(_: &BlockId) -> std::result::Result, (&'static str, ::codec::Error)> { + fn same_name(_: &BlockId) -> + std::result::Result< + NativeOrEncoded<()>, + (&'static str, ::codec::Error) + > + { Ok(().into()) } #[advanced] - fn wild_card(at: &BlockId, _: u32) -> std::result::Result, (&'static str, ::codec::Error)> { + fn wild_card(at: &BlockId, _: u32) -> + std::result::Result< + NativeOrEncoded<()>, + (&'static str, ::codec::Error) + > + { if let BlockId::Number(1337) = at { // yeah Ok(().into()) From 66b69897b45c846c77a9a01bfdaa54a67333b88f Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 20 Nov 2020 12:16:04 +0100 Subject: [PATCH 14/35] test fixes due to error changes --- primitives/api/src/lib.rs | 11 ++++++----- primitives/api/test/tests/decl_and_impl.rs | 6 +++--- .../api/test/tests/ui/mock_only_one_error_type.stderr | 8 ++++---- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index a52fcdccc90dd..2a6efbcdefb9c 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -69,7 +69,7 @@ pub use sp_std::{slice, mem}; #[cfg(feature = "std")] use sp_std::result; #[doc(hidden)] -pub use codec::{Encode, Decode, DecodeLimit}; +pub use codec::{self, Encode, Decode, DecodeLimit}; use sp_core::OpaqueMetadata; #[cfg(feature = "std")] use std::{panic::UnwindSafe, cell::RefCell}; @@ -288,7 +288,7 @@ pub use sp_api_proc_macro::impl_runtime_apis; /// /// Sets the error type that is being used by the mock implementation. /// /// The error type is used by all runtime apis. It is only required to /// /// be specified in one trait implementation. -/// type Error = String; +/// type Error = (&'static str, codec::Error); /// /// fn build_block() -> Block { /// unimplemented!("Not Required in tests") @@ -315,6 +315,7 @@ pub use sp_api_proc_macro::impl_runtime_apis; /// # use sp_runtime::{traits::Block as BlockT, generic::BlockId}; /// # use sp_test_primitives::Block; /// # use sp_core::NativeOrEncoded; +/// # use codec; /// # /// # sp_api::decl_runtime_apis! { /// # /// Declare the api trait. @@ -331,15 +332,15 @@ pub use sp_api_proc_macro::impl_runtime_apis; /// /// sp_api::mock_impl_runtime_apis! { /// impl Balance for MockApi { -/// type Error = String; +/// type Error = (&'static str, codec::Error); /// #[advanced] -/// fn get_balance(&self, at: &BlockId) -> Result, String> { +/// fn get_balance(&self, at: &BlockId) -> Result, (&'static str, codec::Error)> { /// println!("Being called at: {}", at); /// /// Ok(self.balance.into()) /// } /// #[advanced] -/// fn set_balance(at: &BlockId, val: u64) -> Result, String> { +/// fn set_balance(at: &BlockId, val: u64) -> Result, (&'static str, codec::Error)> { /// if let BlockId::Number(1) = at { /// println!("Being called to set balance to: {}", val); /// } diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index b3989496e232f..e5b1143678689 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -106,7 +106,7 @@ mock_impl_runtime_apis! { fn same_name(_: &BlockId) -> std::result::Result< NativeOrEncoded<()>, - (&'static str, ::codec::Error) + (&'static str, codec::Error) > { Ok(().into()) @@ -116,14 +116,14 @@ mock_impl_runtime_apis! { fn wild_card(at: &BlockId, _: u32) -> std::result::Result< NativeOrEncoded<()>, - (&'static str, ::codec::Error) + (&'static str, codec::Error) > { if let BlockId::Number(1337) = at { // yeah Ok(().into()) } else { - let e = ::codec::Error::from("Ohh noooo"); + let e = codec::Error::from("Ohh noooo"); Err(("MockApi", e)) } } diff --git a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr index daac5674d6ffe..4ee80f75217b3 100644 --- a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr +++ b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr @@ -10,16 +10,16 @@ error: First error type was declared here. 17 | type Error = u32; | ^^^ -error[E0277]: the trait bound `u32: std::convert::From` is not satisfied +error[E0277]: the trait bound `u32: std::convert::From<(&'static str, sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::parity_scale_codec::Error)>` is not satisfied --> $DIR/mock_only_one_error_type.rs:17:16 | 17 | type Error = u32; - | ^^^ the trait `std::convert::From` is not implemented for `u32` + | ^^^ the trait `std::convert::From<(&'static str, sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::parity_scale_codec::Error)>` is not implemented for `u32` | ::: $WORKSPACE/primitives/api/src/lib.rs | - | type Error: std::fmt::Debug + From; - | ------------ required by this bound in `sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiErrorExt` + | type Error: std::fmt::Debug + From<(&'static str, codec::Error)>; + | ---------------------------------- required by this bound in `sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiErrorExt` | = help: the following implementations were found: > From 49fe4440d571582dbe85a642372ebf1f8c07f181 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 25 Nov 2020 16:58:56 +0100 Subject: [PATCH 15/35] fin --- Cargo.lock | 1 + client/consensus/slots/src/lib.rs | 2 +- client/transaction-pool/graph/Cargo.toml | 1 + client/transaction-pool/graph/src/error.rs | 24 +++++++++---------- .../test/tests/derive_no_bound_ui/eq.stderr | 5 ++++ primitives/api/test/tests/decl_and_impl.rs | 1 + .../ui/mock_advanced_block_id_by_value.rs | 2 +- .../ui/mock_advanced_block_id_by_value.stderr | 2 +- .../tests/ui/mock_advanced_missing_blockid.rs | 2 +- .../ui/mock_advanced_missing_blockid.stderr | 2 +- primitives/transaction-pool/Cargo.toml | 3 ++- primitives/transaction-pool/src/error.rs | 1 + 12 files changed, 28 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 42c16b9acf1ad..8d2f83c252a75 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7579,6 +7579,7 @@ dependencies = [ "sp-transaction-pool", "sp-utils", "substrate-test-runtime", + "thiserror", "wasm-timer", ] diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index 01f8af9cfb5bd..3c1de3ddc31b4 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -940,7 +940,7 @@ mod test { true, true, true, true, ]; - assert_eq!(backoff, expected); + assert_eq!(backoff.as_slice(), &expected[..]); } #[test] diff --git a/client/transaction-pool/graph/Cargo.toml b/client/transaction-pool/graph/Cargo.toml index c5850e765fcfa..94c80c6f298a2 100644 --- a/client/transaction-pool/graph/Cargo.toml +++ b/client/transaction-pool/graph/Cargo.toml @@ -14,6 +14,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] derive_more = "0.99.2" +thiserror = "1.0.21" futures = "0.3.4" log = "0.4.8" parking_lot = "0.10.0" diff --git a/client/transaction-pool/graph/src/error.rs b/client/transaction-pool/graph/src/error.rs index 392ddaa39be6f..b599715920be0 100644 --- a/client/transaction-pool/graph/src/error.rs +++ b/client/transaction-pool/graph/src/error.rs @@ -26,28 +26,29 @@ use sp_runtime::transaction_validity::{ pub type Result = std::result::Result; /// Transaction pool error type. -#[derive(Debug, derive_more::Display, derive_more::From)] +#[derive(Debug, thiserror::Error, derive_more::From)] +#[allow(missing_docs)] pub enum Error { /// Transaction is not verifiable yet, but might be in the future. - #[display(fmt="Unknown transaction validity: {:?}", _0)] + #[error("Unknown transaction validity: {0:?}")] UnknownTransaction(UnknownTransaction), /// Transaction is invalid. - #[display(fmt="Invalid transaction validity: {:?}", _0)] + #[error("Invalid transaction validity: {0:?}")] InvalidTransaction(InvalidTransaction), /// The transaction validity returned no "provides" tag. /// /// Such transactions are not accepted to the pool, since we use those tags /// to define identity of transactions (occupance of the same "slot"). - #[display(fmt="The transaction does not provide any tags, so the pool can't identify it.")] + #[error("The transaction does not provide any tags, so the pool can't identify it.")] NoTagsProvided, - /// The transaction is temporarily banned. - #[display(fmt="Temporarily Banned")] + + #[error("Temporarily Banned")] TemporarilyBanned, /// The transaction is already in the pool. - #[display(fmt="[{:?}] Already imported", _0)] + #[error("[{0:?}] Already imported")] AlreadyImported(Box), /// The transaction cannot be imported cause it's a replacement and has too low priority. - #[display(fmt="Too low priority ({} > {})", old, new)] + #[error("Too low priority ({0} > {1})", old, new)] TooLowPriority { /// Transaction already in the pool. old: Priority, @@ -55,17 +56,16 @@ pub enum Error { new: Priority }, /// Deps cycle detected and we couldn't import transaction. - #[display(fmt="Cycle Detected")] + #[error("Cycle Detected")] CycleDetected, /// Transaction was dropped immediately after it got inserted. - #[display(fmt="Transaction couldn't enter the pool because of the limit.")] + #[error("Transaction couldn't enter the pool because of the limit.")] ImmediatelyDropped, /// Invalid block id. + #[error("Invlaid block id: {0}")] InvalidBlockId(String), } -impl std::error::Error for Error {} - /// Transaction pool error conversion. pub trait IntoPoolError: ::std::error::Error + Send + Sized { /// Try to extract original `Error` diff --git a/frame/support/test/tests/derive_no_bound_ui/eq.stderr b/frame/support/test/tests/derive_no_bound_ui/eq.stderr index 08341c4d65ab5..d4e0ac455866d 100644 --- a/frame/support/test/tests/derive_no_bound_ui/eq.stderr +++ b/frame/support/test/tests/derive_no_bound_ui/eq.stderr @@ -4,4 +4,9 @@ error[E0277]: can't compare `Foo` with `Foo` 6 | struct Foo { | ^^^ no implementation for `Foo == Foo` | + ::: $RUST/src/libcore/cmp.rs + | + | pub trait Eq: PartialEq { + | --------------- required by this bound in `std::cmp::Eq` + | = help: the trait `std::cmp::PartialEq` is not implemented for `Foo` diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index e5b1143678689..3417d1da02bc6 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -23,6 +23,7 @@ use sp_runtime::{traits::{GetNodeBlockType, Block as BlockT}, generic::BlockId}; use sp_core::NativeOrEncoded; use substrate_test_runtime_client::runtime::Block; use sp_blockchain::Result; +use codec; /// The declaration of the `Runtime` type and the implementation of the `GetNodeBlockType` /// trait are done by the `construct_runtime!` macro in a real runtime. diff --git a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs index 1e71730cd0a17..b3fb38992a2ca 100644 --- a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs +++ b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs @@ -11,7 +11,7 @@ struct MockApi; sp_api::mock_impl_runtime_apis! { impl Api for MockApi { #[advanced] - fn test(&self, _: BlockId) -> Result, String> { + fn test(&self, _: BlockId) -> Result, (&'static str, codec::Error)> { Ok(().into()) } } diff --git a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr index efddce05f51b4..415e0c93b7b07 100644 --- a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr +++ b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr @@ -4,7 +4,7 @@ error: `BlockId` needs to be taken by reference and not by value! 11 | / sp_api::mock_impl_runtime_apis! { 12 | | impl Api for MockApi { 13 | | #[advanced] -14 | | fn test(&self, _: BlockId) -> Result, String> { +14 | | fn test(&self, _: BlockId) -> Result, (&'static str, codec::Error)> { ... | 17 | | } 18 | | } diff --git a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs index 407ea90ee882d..2c9b9fa72033d 100644 --- a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs +++ b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs @@ -11,7 +11,7 @@ struct MockApi; sp_api::mock_impl_runtime_apis! { impl Api for MockApi { #[advanced] - fn test(&self) -> Result, String> { + fn test(&self) -> Result, (&'static str, codec::Error)> { Ok(().into()) } } diff --git a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr index e7a66ebc5dba8..11c70e49c533d 100644 --- a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr +++ b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr @@ -1,5 +1,5 @@ error: If using the `advanced` attribute, it is required that the function takes at least one argument, the `BlockId`. --> $DIR/mock_advanced_missing_blockid.rs:14:3 | -14 | fn test(&self) -> Result, String> { +14 | fn test(&self) -> Result, (&'static str, codec::Error)> { | ^^ diff --git a/primitives/transaction-pool/Cargo.toml b/primitives/transaction-pool/Cargo.toml index 030c2c4f17ac3..4247e1a50c9bc 100644 --- a/primitives/transaction-pool/Cargo.toml +++ b/primitives/transaction-pool/Cargo.toml @@ -14,7 +14,7 @@ readme = "README.md" targets = ["x86_64-unknown-linux-gnu"] [dependencies] -thiserror = "1.0.21" +thiserror = { version = "1.0.21", optional = true } codec = { package = "parity-scale-codec", version = "1.3.1", optional = true } derive_more = { version = "0.99.11", optional = true } futures = { version = "0.3.1", optional = true } @@ -32,6 +32,7 @@ std = [ "futures", "log", "serde", + "thiserror", "sp-api/std", "sp-blockchain", "sp-runtime/std", diff --git a/primitives/transaction-pool/src/error.rs b/primitives/transaction-pool/src/error.rs index 3fa4bb7bfc1e3..e356df75908a7 100644 --- a/primitives/transaction-pool/src/error.rs +++ b/primitives/transaction-pool/src/error.rs @@ -60,6 +60,7 @@ pub enum Error { #[error("Transaction couldn't enter the pool because of the limit")] ImmediatelyDropped, + #[from(ignore)] #[error("{0}")] InvalidBlockId(String), From ff810078456a48471924d761d0a9387c73fea021 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 25 Nov 2020 19:25:41 +0100 Subject: [PATCH 16/35] error outputs... again --- frame/support/test/tests/derive_no_bound_ui/clone.stderr | 6 +++--- frame/support/test/tests/derive_no_bound_ui/eq.stderr | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/support/test/tests/derive_no_bound_ui/clone.stderr b/frame/support/test/tests/derive_no_bound_ui/clone.stderr index 4b9cccf0b0fa1..37e403c4e8f14 100644 --- a/frame/support/test/tests/derive_no_bound_ui/clone.stderr +++ b/frame/support/test/tests/derive_no_bound_ui/clone.stderr @@ -1,7 +1,7 @@ -error[E0277]: the trait bound `::C: std::clone::Clone` is not satisfied +error[E0277]: the trait bound `::C: Clone` is not satisfied --> $DIR/clone.rs:7:2 | 7 | c: T::C, - | ^ the trait `std::clone::Clone` is not implemented for `::C` + | ^ the trait `Clone` is not implemented for `::C` | - = note: required by `std::clone::Clone::clone` + = note: required by `clone` diff --git a/frame/support/test/tests/derive_no_bound_ui/eq.stderr b/frame/support/test/tests/derive_no_bound_ui/eq.stderr index d4e0ac455866d..a4a0fffd7bc0d 100644 --- a/frame/support/test/tests/derive_no_bound_ui/eq.stderr +++ b/frame/support/test/tests/derive_no_bound_ui/eq.stderr @@ -4,9 +4,9 @@ error[E0277]: can't compare `Foo` with `Foo` 6 | struct Foo { | ^^^ no implementation for `Foo == Foo` | - ::: $RUST/src/libcore/cmp.rs + ::: $RUST/core/src/cmp.rs | | pub trait Eq: PartialEq { - | --------------- required by this bound in `std::cmp::Eq` + | --------------- required by this bound in `Eq` | - = help: the trait `std::cmp::PartialEq` is not implemented for `Foo` + = help: the trait `PartialEq` is not implemented for `Foo` From 13ba47b7106ee5287d56af3314cf51744b0f2b37 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 11:29:37 +0100 Subject: [PATCH 17/35] undo stderr adjustments --- frame/support/test/tests/derive_no_bound_ui/clone.stderr | 6 +++--- frame/support/test/tests/derive_no_bound_ui/eq.stderr | 7 +------ 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/frame/support/test/tests/derive_no_bound_ui/clone.stderr b/frame/support/test/tests/derive_no_bound_ui/clone.stderr index 37e403c4e8f14..4b9cccf0b0fa1 100644 --- a/frame/support/test/tests/derive_no_bound_ui/clone.stderr +++ b/frame/support/test/tests/derive_no_bound_ui/clone.stderr @@ -1,7 +1,7 @@ -error[E0277]: the trait bound `::C: Clone` is not satisfied +error[E0277]: the trait bound `::C: std::clone::Clone` is not satisfied --> $DIR/clone.rs:7:2 | 7 | c: T::C, - | ^ the trait `Clone` is not implemented for `::C` + | ^ the trait `std::clone::Clone` is not implemented for `::C` | - = note: required by `clone` + = note: required by `std::clone::Clone::clone` diff --git a/frame/support/test/tests/derive_no_bound_ui/eq.stderr b/frame/support/test/tests/derive_no_bound_ui/eq.stderr index a4a0fffd7bc0d..08341c4d65ab5 100644 --- a/frame/support/test/tests/derive_no_bound_ui/eq.stderr +++ b/frame/support/test/tests/derive_no_bound_ui/eq.stderr @@ -4,9 +4,4 @@ error[E0277]: can't compare `Foo` with `Foo` 6 | struct Foo { | ^^^ no implementation for `Foo == Foo` | - ::: $RUST/core/src/cmp.rs - | - | pub trait Eq: PartialEq { - | --------------- required by this bound in `Eq` - | - = help: the trait `PartialEq` is not implemented for `Foo` + = help: the trait `std::cmp::PartialEq` is not implemented for `Foo` From 3dda02d5f896f4b66e461893fdfea05ce5a98e48 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 11:58:02 +0100 Subject: [PATCH 18/35] Update client/consensus/slots/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/consensus/slots/src/lib.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index 3c1de3ddc31b4..8e4b2eebfb2f9 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -532,11 +532,8 @@ impl SlotDuration { } }?; - { - let slot_duration = slot_duration.slot_duration(); - if slot_duration == 0u64 { - return Err(sp_blockchain::Error::SlotDurationInvalid(slot_duration)) - } + if slot_duration.slot_duration() == 0u64 { + return Err(sp_blockchain::Error::SlotDurationInvalid(slot_duration)) } Ok(slot_duration) From eb7805ba53905d0f81274db2db6f56d6f66457fe Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 12:03:04 +0100 Subject: [PATCH 19/35] remove closure clutter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/service/src/client/call_executor.rs | 10 +++++----- client/service/src/client/client.rs | 4 ++-- primitives/blockchain/src/error.rs | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/client/service/src/client/call_executor.rs b/client/service/src/client/call_executor.rs index e89567e4d2134..cd01a5877758d 100644 --- a/client/service/src/client/call_executor.rs +++ b/client/service/src/client/call_executor.rs @@ -138,7 +138,7 @@ where let state = self.backend.state_at(*id)?; let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); let runtime_code = state_runtime_code.runtime_code() - .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + .map_err(sp_blockchain::Error::RuntimeCode)?; let runtime_code = self.check_override(runtime_code, id)?; let return_data = StateMachine::new( @@ -215,7 +215,7 @@ where // recorder. let runtime_code = state_runtime_code.runtime_code() - .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + .map_err(sp_blockchain::Error::RuntimeCode)?; let runtime_code = self.check_override(runtime_code, at)?; let backend = sp_state_machine::ProvingBackend::new_with_recorder( @@ -242,7 +242,7 @@ where None => { let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); let runtime_code = state_runtime_code.runtime_code() - .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + .map_err(sp_blockchain::Error::RuntimeCode)?; let runtime_code = self.check_override(runtime_code, at)?; let mut state_machine = StateMachine::new( @@ -281,7 +281,7 @@ where ); let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); let runtime_code = state_runtime_code.runtime_code() - .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + .map_err(sp_blockchain::Error::RuntimeCode)?; self.executor.runtime_version(&mut ext, &runtime_code) .map_err(|e| sp_blockchain::Error::VersionInvalid(format!("{:?}", e)).into()) } @@ -295,7 +295,7 @@ where ) -> Result<(Vec, StorageProof), sp_blockchain::Error> { let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(trie_state); let runtime_code = state_runtime_code.runtime_code() - .map_err(|e| sp_blockchain::Error::RuntimeCode(e))?; + .map_err(sp_blockchain::Error::RuntimeCode)?; sp_state_machine::prove_execution_on_trie_backend::<_, _, NumberFor, _, _>( trie_state, overlay, diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 50bdc7882ab1a..8cf937cced3eb 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -298,7 +298,7 @@ impl Client where ) -> sp_blockchain::Result { if backend.blockchain().header(BlockId::Number(Zero::zero()))?.is_none() { let genesis_storage = build_genesis_storage.build_storage() - .map_err(|e| sp_blockchain::Error::Storage(e))?; + .map_err(sp_blockchain::Error::Storage)?; let mut op = backend.begin_operation()?; backend.begin_state_operation(&mut op, BlockId::Hash(Default::default()))?; let state_root = op.reset_storage(genesis_storage)?; @@ -881,7 +881,7 @@ impl Client where &state, changes_trie_state.as_ref(), *parent_hash, - ).map_err(|e| sp_blockchain::Error::Storage(e))?; + ).map_err(sp_blockchain::Error::Storage)?; if import_block.header.state_root() != &gen_storage_changes.transaction_storage_root diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 8eaff7a276e71..c1247b79ea4b0 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -68,7 +68,7 @@ pub enum Error { InvalidChildType, #[error("RemoteBodyRequest: invalid extrinsics root expected: {expected} but got {received}")] - ExtrinsicRootInvalid{ received: String, expected: String}, + ExtrinsicRootInvalid { received: String, expected: String }, // `inner` cannot be made member, since it lacks `std::error::Error` trait bounds. #[error("Execution failed: {0:?}")] From 536f11ef80e434c26e596de6b2a54835730cc297 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 14:21:51 +0100 Subject: [PATCH 20/35] more error types --- client/consensus/slots/Cargo.toml | 3 ++- client/consensus/slots/src/lib.rs | 16 +++++++++++++--- client/network/src/on_demand_layer.rs | 23 +++++++++++++++++------ 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/client/consensus/slots/Cargo.toml b/client/consensus/slots/Cargo.toml index d07ef49835b2b..e8bd1f33631ea 100644 --- a/client/consensus/slots/Cargo.toml +++ b/client/consensus/slots/Cargo.toml @@ -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" } diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index 8e4b2eebfb2f9..ab8fc16007ce9 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -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 { Checked(H, S), } + + +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error where T: SlotData + Clone + Debug + Send + Sync + 'static { + #[error("Slot duration is invalid: {0:?}")] + SlotDurationInvalid(SlotDuration), +} + /// 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 SlotData for SlotDuration { const SLOT_KEY: &'static [u8] = T::SLOT_KEY; } -impl SlotDuration { +impl SlotDuration { /// Either fetch the slot duration from disk or compute it from the /// genesis state. /// @@ -533,7 +543,7 @@ impl SlotDuration { }?; if slot_duration.slot_duration() == 0u64 { - return Err(sp_blockchain::Error::SlotDurationInvalid(slot_duration)) + return Err(sp_blockchain::Error::Application(Box::new(Error::SlotDurationInvalid(slot_duration)))) } Ok(slot_duration) diff --git a/client/network/src/on_demand_layer.rs b/client/network/src/on_demand_layer.rs index 064277a6c40e0..6e0add18adb02 100644 --- a/client/network/src/on_demand_layer.rs +++ b/client/network/src/on_demand_layer.rs @@ -51,6 +51,17 @@ pub struct OnDemand { requests_send: TracingUnboundedSender>, } + +#[derive(Debug, thiserror::Error)] +#[error("AlwaysBadChecker")] +struct ErrorAlwaysBadChecker; + +impl Into for ErrorAlwaysBadChecker { + fn into(self) -> ClientError { + ClientError::Application(Box::new(self)) + } +} + /// Dummy implementation of `FetchChecker` that always assumes that responses are bad. /// /// Considering that it is the responsibility of the client to build the fetcher, it can use this @@ -65,7 +76,7 @@ impl FetchChecker for AlwaysBadChecker { _remote_header: Option, _remote_proof: StorageProof, ) -> Result { - Err(ClientError::AlwaysBadChecker) + Err(ErrorAlwaysBadChecker.into()) } fn check_read_proof( @@ -73,7 +84,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteReadRequest, _remote_proof: StorageProof, ) -> Result,Option>>, ClientError> { - Err(ClientError::AlwaysBadChecker) + Err(ErrorAlwaysBadChecker.into()) } fn check_read_child_proof( @@ -81,7 +92,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteReadChildRequest, _remote_proof: StorageProof, ) -> Result, Option>>, ClientError> { - Err(ClientError::AlwaysBadChecker) + Err(ErrorAlwaysBadChecker.into()) } fn check_execution_proof( @@ -89,7 +100,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteCallRequest, _remote_proof: StorageProof, ) -> Result, ClientError> { - Err(ClientError::AlwaysBadChecker) + Err(ErrorAlwaysBadChecker.into()) } fn check_changes_proof( @@ -97,7 +108,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteChangesRequest, _remote_proof: ChangesProof ) -> Result, u32)>, ClientError> { - Err(ClientError::AlwaysBadChecker) + Err(ErrorAlwaysBadChecker.into()) } fn check_body_proof( @@ -105,7 +116,7 @@ impl FetchChecker for AlwaysBadChecker { _request: &RemoteBodyRequest, _body: Vec ) -> Result, ClientError> { - Err(ClientError::AlwaysBadChecker) + Err(ErrorAlwaysBadChecker.into()) } } From bc4095583cdd05647802d4edf937c258c4c9132b Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 14:23:26 +0100 Subject: [PATCH 21/35] introduce ApiError --- Cargo.lock | 2 ++ primitives/api/Cargo.toml | 2 ++ primitives/api/src/error.rs | 16 ++++++++++++++++ primitives/api/src/lib.rs | 17 +++++++++++------ primitives/api/test/tests/decl_and_impl.rs | 4 ++-- .../tests/ui/mock_advanced_block_id_by_value.rs | 2 +- .../ui/mock_advanced_block_id_by_value.stderr | 2 +- .../tests/ui/mock_advanced_missing_blockid.rs | 2 +- .../ui/mock_advanced_missing_blockid.stderr | 2 +- .../tests/ui/mock_only_one_error_type.stderr | 2 +- primitives/blockchain/src/error.rs | 10 ++-------- 11 files changed, 40 insertions(+), 21 deletions(-) create mode 100644 primitives/api/src/error.rs diff --git a/Cargo.lock b/Cargo.lock index 1b3196e141e63..e0cee22bff24d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6828,6 +6828,7 @@ dependencies = [ "sp-state-machine", "sp-trie", "substrate-test-runtime-client", + "thiserror", ] [[package]] @@ -7985,6 +7986,7 @@ dependencies = [ "sp-std", "sp-test-primitives", "sp-version", + "thiserror", ] [[package]] diff --git a/primitives/api/Cargo.toml b/primitives/api/Cargo.toml index a3c480e92135f..92bf9bea2bdc7 100644 --- a/primitives/api/Cargo.toml +++ b/primitives/api/Cargo.toml @@ -21,6 +21,7 @@ sp-runtime = { version = "2.0.0", default-features = false, path = "../runtime" sp-version = { version = "2.0.0", default-features = false, path = "../version" } sp-state-machine = { version = "0.8.0", optional = true, path = "../../primitives/state-machine" } hash-db = { version = "0.15.2", optional = true } +thiserror = { version = "1.0.21", optional = true } [dev-dependencies] sp-test-primitives = { version = "2.0.0", path = "../test-primitives" } @@ -35,4 +36,5 @@ std = [ "sp-state-machine", "sp-version/std", "hash-db", + "thiserror", ] diff --git a/primitives/api/src/error.rs b/primitives/api/src/error.rs new file mode 100644 index 0000000000000..092e177f2b2cb --- /dev/null +++ b/primitives/api/src/error.rs @@ -0,0 +1,16 @@ +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +struct Error { + tag: &'static str, + #[source] + error: codec::Error, +} + +impl From<(&'static str, codec::Error)> for Error { + fn from((tag, error): (&'static str, codec::Error)) -> Self { + Self { + tag, + error, + } + } +} diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 2a6efbcdefb9c..953263ad75e26 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -74,6 +74,11 @@ use sp_core::OpaqueMetadata; #[cfg(feature = "std")] use std::{panic::UnwindSafe, cell::RefCell}; +#[cfg(feature = "std")] +mod error; +#[cfg(feature = "std")] +pub use crate::error::Error as ApiError; + /// Maximum nesting level for extrinsics. pub const MAX_EXTRINSIC_DEPTH: u32 = 256; @@ -288,7 +293,7 @@ pub use sp_api_proc_macro::impl_runtime_apis; /// /// Sets the error type that is being used by the mock implementation. /// /// The error type is used by all runtime apis. It is only required to /// /// be specified in one trait implementation. -/// type Error = (&'static str, codec::Error); +/// type Error = ApiError; /// /// fn build_block() -> Block { /// unimplemented!("Not Required in tests") @@ -332,15 +337,15 @@ pub use sp_api_proc_macro::impl_runtime_apis; /// /// sp_api::mock_impl_runtime_apis! { /// impl Balance for MockApi { -/// type Error = (&'static str, codec::Error); +/// type Error = ApiError; /// #[advanced] -/// fn get_balance(&self, at: &BlockId) -> Result, (&'static str, codec::Error)> { +/// fn get_balance(&self, at: &BlockId) -> Result, ApiError> { /// println!("Being called at: {}", at); /// /// Ok(self.balance.into()) /// } /// #[advanced] -/// fn set_balance(at: &BlockId, val: u64) -> Result, (&'static str, codec::Error)> { +/// fn set_balance(at: &BlockId, val: u64) -> Result, ApiError> { /// if let BlockId::Number(1) = at { /// println!("Being called to set balance to: {}", val); /// } @@ -398,7 +403,7 @@ pub trait ConstructRuntimeApi> { #[cfg(feature = "std")] pub trait ApiErrorExt { /// Error type used by the runtime apis. - type Error: std::fmt::Debug + From<(&'static str, codec::Error)>; + type Error: std::fmt::Debug + From; } /// Extends the runtime api implementation with some common functionality. @@ -507,7 +512,7 @@ pub struct CallApiAtParams<'a, Block: BlockT, C, NC, Backend: StateBackend { /// Error type used by the implementation. - type Error: std::fmt::Debug + From<(&'static str, codec::Error)>; + type Error: std::fmt::Debug + From; /// The state backend that is used to store the block states. type StateBackend: StateBackend>; diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index 3417d1da02bc6..84ec3c3129885 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -107,7 +107,7 @@ mock_impl_runtime_apis! { fn same_name(_: &BlockId) -> std::result::Result< NativeOrEncoded<()>, - (&'static str, codec::Error) + ApiError > { Ok(().into()) @@ -117,7 +117,7 @@ mock_impl_runtime_apis! { fn wild_card(at: &BlockId, _: u32) -> std::result::Result< NativeOrEncoded<()>, - (&'static str, codec::Error) + ApiError > { if let BlockId::Number(1337) = at { diff --git a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs index b3fb38992a2ca..35dc814d1851b 100644 --- a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs +++ b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs @@ -11,7 +11,7 @@ struct MockApi; sp_api::mock_impl_runtime_apis! { impl Api for MockApi { #[advanced] - fn test(&self, _: BlockId) -> Result, (&'static str, codec::Error)> { + fn test(&self, _: BlockId) -> Result, ApiError> { Ok(().into()) } } diff --git a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr index 415e0c93b7b07..b4ee64a1bb7d1 100644 --- a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr +++ b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr @@ -4,7 +4,7 @@ error: `BlockId` needs to be taken by reference and not by value! 11 | / sp_api::mock_impl_runtime_apis! { 12 | | impl Api for MockApi { 13 | | #[advanced] -14 | | fn test(&self, _: BlockId) -> Result, (&'static str, codec::Error)> { +14 | | fn test(&self, _: BlockId) -> Result, ApiError> { ... | 17 | | } 18 | | } diff --git a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs index 2c9b9fa72033d..0f1b16fb8c057 100644 --- a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs +++ b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs @@ -11,7 +11,7 @@ struct MockApi; sp_api::mock_impl_runtime_apis! { impl Api for MockApi { #[advanced] - fn test(&self) -> Result, (&'static str, codec::Error)> { + fn test(&self) -> Result, ApiError> { Ok(().into()) } } diff --git a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr index 11c70e49c533d..605c4869a735b 100644 --- a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr +++ b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr @@ -1,5 +1,5 @@ error: If using the `advanced` attribute, it is required that the function takes at least one argument, the `BlockId`. --> $DIR/mock_advanced_missing_blockid.rs:14:3 | -14 | fn test(&self) -> Result, (&'static str, codec::Error)> { +14 | fn test(&self) -> Result, ApiError> { | ^^ diff --git a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr index 4ee80f75217b3..5ca1d556febd4 100644 --- a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr +++ b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr @@ -18,7 +18,7 @@ error[E0277]: the trait bound `u32: std::convert::From<(&'static str, sp_api_hid | ::: $WORKSPACE/primitives/api/src/lib.rs | - | type Error: std::fmt::Debug + From<(&'static str, codec::Error)>; + | type Error: std::fmt::Debug + From; | ---------------------------------- required by this bound in `sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiErrorExt` | = help: the following implementations were found: diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index c1247b79ea4b0..6c21cf6e6feb6 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -171,15 +171,9 @@ pub enum Error { #[error("State Database error: {0}")] StateDatabase(String), - #[error("Invalid value for slot_duration: the value ({0}) must be greater than 0.")] - SlotDurationInvalid(u64), - #[error(transparent)] Application(#[from] Box), - #[error("AlwaysBadChecker")] - AlwaysBadChecker, - // Should be removed/improved once // the storage `fn`s returns typed errors. #[error("Runtime code error: {0}")] @@ -207,8 +201,8 @@ impl From> for Error { } } -impl From<(&'static str, codec::Error)> for Error { - fn from(x: (&'static str, codec::Error)) -> Self { +impl From for Error { + fn from(x: ApiError) -> Self { Self::RuntimeApiCodecError(x.0, x.1) } } From 31f025966fc4e2f24b7a79d0420ecbc70bd17e94 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 14:55:19 +0100 Subject: [PATCH 22/35] extract Mock error --- Cargo.lock | 1 + client/api/Cargo.toml | 1 + client/api/src/light.rs | 12 +++++++++++- primitives/blockchain/src/error.rs | 4 ---- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e0cee22bff24d..2c4ef13acce76 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6565,6 +6565,7 @@ dependencies = [ "sp-version", "substrate-prometheus-endpoint", "substrate-test-runtime", + "thiserror", ] [[package]] diff --git a/client/api/Cargo.toml b/client/api/Cargo.toml index 7d770912f271f..07036bfb414a2 100644 --- a/client/api/Cargo.toml +++ b/client/api/Cargo.toml @@ -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" diff --git a/client/api/src/light.rs b/client/api/src/light.rs index 3f98e5a4468f2..e563eb5ffea50 100644 --- a/client/api/src/light.rs +++ b/client/api/src/light.rs @@ -312,11 +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 for MockError { + fn into(self) -> ClientError { + ClientError::Application(Box::new(self)) + } + } + pub type OkCallFetcher = Mutex>; fn not_implemented_in_tests() -> Ready> { - futures::future::ready(Err(ClientError::Mock("Not implemented on test node"))) + futures::future::ready(MockError.into()) } impl Fetcher for OkCallFetcher { diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 6c21cf6e6feb6..65504b27256fb 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -183,10 +183,6 @@ pub enum Error { // the storage `fn`s returns typed errors. #[error("Storage error: {0}")] Storage(String), - - /// Unit test helper for mocked modules. - #[error("Mock: {0}")] - Mock(&'static str), } impl From> for Error { From 717ec7dfee7b74ebaed12760ee6a3d3449014d07 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 14:55:30 +0100 Subject: [PATCH 23/35] ApiError refactor --- .../proc-macro/src/mock_impl_runtime_apis.rs | 2 +- primitives/api/src/error.rs | 16 ------------ primitives/api/src/lib.rs | 25 ++++++++++++++++--- .../tests/ui/mock_only_one_error_type.stderr | 6 ++--- 4 files changed, 25 insertions(+), 24 deletions(-) delete mode 100644 primitives/api/src/error.rs diff --git a/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs b/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs index bd8daaa8444f8..14cf47fc64b25 100644 --- a/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs @@ -71,7 +71,7 @@ fn implement_common_api_traits( let error_type = error_type .map(|e| quote!(#e)) - .unwrap_or_else(|| quote!( (&'static str, #crate_::codec::Error) )); + .unwrap_or_else(|| quote!( #crate_::ApiError ) ); // Quote using the span from `error_type` to generate nice error messages when the type is // not implementing a trait or similar. diff --git a/primitives/api/src/error.rs b/primitives/api/src/error.rs deleted file mode 100644 index 092e177f2b2cb..0000000000000 --- a/primitives/api/src/error.rs +++ /dev/null @@ -1,16 +0,0 @@ -#[derive(Debug, thiserror::Error)] -#[allow(missing_docs)] -struct Error { - tag: &'static str, - #[source] - error: codec::Error, -} - -impl From<(&'static str, codec::Error)> for Error { - fn from((tag, error): (&'static str, codec::Error)) -> Self { - Self { - tag, - error, - } - } -} diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 953263ad75e26..58c4cc46bdb19 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -74,10 +74,6 @@ use sp_core::OpaqueMetadata; #[cfg(feature = "std")] use std::{panic::UnwindSafe, cell::RefCell}; -#[cfg(feature = "std")] -mod error; -#[cfg(feature = "std")] -pub use crate::error::Error as ApiError; /// Maximum nesting level for extrinsics. pub const MAX_EXTRINSIC_DEPTH: u32 = 256; @@ -398,6 +394,27 @@ pub trait ConstructRuntimeApi> { fn construct_runtime_api<'a>(call: &'a C) -> ApiRef<'a, Self::RuntimeApi>; } +/// An error describing which API call failed. +#[cfg_attr(feature = "std", derive(Debug, thiserror::Error))] +#[cfg_attr(feature = "std", error("Failed to execute API call {tag}"))] +#[cfg(feature = "std")] +pub struct ApiError { + tag: &'static str, + #[source] + error: codec::Error, +} + +#[cfg(feature = "std")] +impl From<(&'static str, codec::Error)> for ApiError { + fn from((tag, error): (&'static str, crate::codec::Error)) -> Self { + Self { + tag, + error, + } + } +} + + /// Extends the runtime api traits with an associated error type. This trait is given as super /// trait to every runtime api trait. #[cfg(feature = "std")] diff --git a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr index 5ca1d556febd4..9f8ae58f2f691 100644 --- a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr +++ b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr @@ -10,16 +10,16 @@ error: First error type was declared here. 17 | type Error = u32; | ^^^ -error[E0277]: the trait bound `u32: std::convert::From<(&'static str, sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::parity_scale_codec::Error)>` is not satisfied +error[E0277]: the trait bound `u32: std::convert::From< sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiError>` is not satisfied --> $DIR/mock_only_one_error_type.rs:17:16 | 17 | type Error = u32; - | ^^^ the trait `std::convert::From<(&'static str, sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::parity_scale_codec::Error)>` is not implemented for `u32` + | ^^^ the trait `std::convert::From< sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiError>` is not implemented for `u32` | ::: $WORKSPACE/primitives/api/src/lib.rs | | type Error: std::fmt::Debug + From; - | ---------------------------------- required by this bound in `sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiErrorExt` + | -------------- required by this bound in `sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiErrorExt` | = help: the following implementations were found: > From b4c1348b77be5e808d933e51f2f45cf8ec228f11 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 16:18:31 +0100 Subject: [PATCH 24/35] even more error types --- Cargo.lock | 2 + client/api/src/light.rs | 2 +- client/service/src/client/wasm_override.rs | 41 +++++++++++++++---- .../api/proc-macro/src/decl_runtime_apis.rs | 2 +- primitives/api/src/lib.rs | 13 +++++- primitives/api/test/tests/decl_and_impl.rs | 6 +-- primitives/blockchain/Cargo.toml | 1 + primitives/blockchain/src/error.rs | 23 ++--------- 8 files changed, 56 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2c4ef13acce76..0fa32f3b8c390 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6898,6 +6898,7 @@ dependencies = [ "sp-runtime-interface", "sp-serializer", "sp-wasm-interface", + "thiserror", "wasmi", ] @@ -8111,6 +8112,7 @@ dependencies = [ "lru 0.6.1", "parity-scale-codec", "parking_lot 0.10.2", + "sp-api", "sp-consensus", "sp-database", "sp-runtime", diff --git a/client/api/src/light.rs b/client/api/src/light.rs index e563eb5ffea50..f9ba64544a8c0 100644 --- a/client/api/src/light.rs +++ b/client/api/src/light.rs @@ -326,7 +326,7 @@ pub mod tests { fn not_implemented_in_tests() -> Ready> { - futures::future::ready(MockError.into()) + futures::future::ready(Err(MockError.into())) } impl Fetcher for OkCallFetcher { diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index 53985ee08585a..a7dac9caaf27e 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -37,7 +37,8 @@ //! needed must be provided in the given directory. //! use std::{ - fs, collections::{HashMap, hash_map::DefaultHasher}, path::Path, + fs, collections::{HashMap, hash_map::DefaultHasher}, + path::{Path, PathBuf}, hash::Hasher as _, }; use sp_core::traits::FetchRuntimeCode; @@ -82,6 +83,30 @@ impl FetchRuntimeCode for WasmBlob { } } + +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum WasmOverrideError { + #[error("Failed to get runtime version: {0}")] + VersionInvalid(String), + + #[error("WASM override IO error")] + Io(PathBuf, #[source] std::io::Error), + + #[error("Overwriting WASM requires a directory where local \ + WASM is stored. {} is not a directory", .0.display())] + NotADirectory(PathBuf), + + #[error("Duplicate WASM Runtimes found: \n{}\n", .0.join("\n") )] + DuplicateRuntime(Vec), +} + +impl Into for WasmOverrideError { + fn into(self) -> sp_blockchain::Error { + sp_blockchain::Error::Application(Box::new(self)) + } +} + /// Scrapes WASM from a folder and returns WASM from that folder /// if the runtime spec version matches. #[derive(Clone, Debug)] @@ -121,11 +146,11 @@ where fn scrape_overrides(dir: &Path, executor: &E) -> Result> { let handle_err = |e: std::io::Error | -> sp_blockchain::Error { - sp_blockchain::Error::WasmOverrideIo(dir.to_owned(), e) + WasmOverrideError::Io(dir.to_owned(), e).into() }; if !dir.is_dir() { - return Err(sp_blockchain::Error::WasmOverrideNotADirectory(dir.to_owned())); + return Err(WasmOverrideError::NotADirectory(dir.to_owned()).into()); } let mut overrides = HashMap::new(); @@ -146,7 +171,7 @@ where } if !duplicates.is_empty() { - return Err(sp_blockchain::Error::DuplicateWasmRuntime(duplicates)); + return Err(WasmOverrideError::DuplicateRuntime(duplicates).into()); } Ok(overrides) @@ -159,7 +184,7 @@ where ) -> Result { let mut ext = BasicExternalities::default(); executor.runtime_version(&mut ext, &code.runtime_code(heap_pages)) - .map_err(|e| sp_blockchain::Error::VersionInvalid(format!("{:?}", e)).into()) + .map_err(|e| WasmOverrideError::VersionInvalid(format!("{:?}", e)).into()) } } @@ -231,9 +256,9 @@ mod tests { let scraped = WasmOverride::scrape_overrides(dir, exec); match scraped { - Err(e) => { - match e { - sp_blockchain::Error::DuplicateWasmRuntime(duplicates) => { + Err(sp_blockchain::Error::Application(e)) => { + match e.downcast_ref::() { + Some(WasmOverrideError::DuplicateRuntime(duplicates)) => { assert_eq!(duplicates.len(), 1); }, _ => panic!("Test should end with Msg Error Variant") diff --git a/primitives/api/proc-macro/src/decl_runtime_apis.rs b/primitives/api/proc-macro/src/decl_runtime_apis.rs index 640bd543fc2de..aebefe7ea03a4 100644 --- a/primitives/api/proc-macro/src/decl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/decl_runtime_apis.rs @@ -708,7 +708,7 @@ impl<'a> ToClientSideDecl<'a> { }, #crate_::NativeOrEncoded::Encoded(r) => { <#ret_type as #crate_::Decode>::decode(&mut &r[..]) - .map_err(|err| { (#function_name, err).into() }) + .map_err(|err| { #crate_::ApiError::new(#function_name, err).into() }) } } ) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 58c4cc46bdb19..7ee5a1f2bd4bf 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -395,7 +395,7 @@ pub trait ConstructRuntimeApi> { } /// An error describing which API call failed. -#[cfg_attr(feature = "std", derive(Debug, thiserror::Error))] +#[cfg_attr(feature = "std", derive(Debug, thiserror::Error, Eq, PartialEq))] #[cfg_attr(feature = "std", error("Failed to execute API call {tag}"))] #[cfg(feature = "std")] pub struct ApiError { @@ -406,7 +406,7 @@ pub struct ApiError { #[cfg(feature = "std")] impl From<(&'static str, codec::Error)> for ApiError { - fn from((tag, error): (&'static str, crate::codec::Error)) -> Self { + fn from((tag, error): (&'static str, codec::Error)) -> Self { Self { tag, error, @@ -414,6 +414,15 @@ impl From<(&'static str, codec::Error)> for ApiError { } } +#[cfg(feature = "std")] +impl ApiError { + pub fn new(tag: &'static str, error: codec::Error) -> Self { + Self { + tag, + error, + } + } +} /// Extends the runtime api traits with an associated error type. This trait is given as super /// trait to every runtime api trait. diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index 84ec3c3129885..a108d5cba27b6 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -17,6 +17,7 @@ use sp_api::{ RuntimeApiInfo, decl_runtime_apis, impl_runtime_apis, mock_impl_runtime_apis, + ApiError, ApiExt, }; use sp_runtime::{traits::{GetNodeBlockType, Block as BlockT}, generic::BlockId}; @@ -124,8 +125,7 @@ mock_impl_runtime_apis! { // yeah Ok(().into()) } else { - let e = codec::Error::from("Ohh noooo"); - Err(("MockApi", e)) + Err(ApiError::new("MockApi", codec::Error::from("Ohh noooo"))) } } } @@ -210,7 +210,7 @@ fn mock_runtime_api_works_with_advanced() { Api::::same_name(&mock, &BlockId::Number(0)).unwrap(); mock.wild_card(&BlockId::Number(1337), 1).unwrap(); assert_eq!( - ("MockApi", ::codec::Error::from("Ohh noooo")), + ApiError::new("MockApi", ::codec::Error::from("Ohh noooo")), mock.wild_card(&BlockId::Number(1336), 1).unwrap_err() ); } diff --git a/primitives/blockchain/Cargo.toml b/primitives/blockchain/Cargo.toml index d6997f7616354..3458b8c0846ba 100644 --- a/primitives/blockchain/Cargo.toml +++ b/primitives/blockchain/Cargo.toml @@ -24,3 +24,4 @@ sp-consensus = { version = "0.8.0", path = "../consensus/common" } sp-runtime = { version = "2.0.0", path = "../runtime" } sp-state-machine = { version = "0.8.0", path = "../state-machine" } sp-database = { version = "2.0.0", path = "../database" } +sp-api = { version = "2.0.0", path = "../api" } diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 65504b27256fb..14dfbacc674f9 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -17,11 +17,12 @@ //! Substrate client possible errors. -use std::{self, result, path::PathBuf}; +use std::{self, result}; use sp_state_machine; use sp_runtime::transaction_validity::TransactionValidityError; use sp_consensus; use codec::Error as CodecError; +use sp_api::ApiError; /// Client Result type alias pub type Result = result::Result; @@ -116,8 +117,8 @@ pub enum Error { #[error("Error decoding call result of {0}")] CallResultDecode(&'static str, #[source] CodecError), - #[error("Decoding of {0} failed")] - RuntimeApiCodecError(&'static str, #[source] codec::Error), + #[error(transparent)] + RuntimeApiCodecError(#[from] ApiError), #[error("Runtime :code missing in storage")] RuntimeCodeMissing, @@ -158,16 +159,6 @@ pub enum Error { #[error("Failed to load the block weight for block {0}")] LoadingBlockWeightFailed(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), - #[error("State Database error: {0}")] StateDatabase(String), @@ -197,12 +188,6 @@ impl From> for Error { } } -impl From for Error { - fn from(x: ApiError) -> Self { - Self::RuntimeApiCodecError(x.0, x.1) - } -} - impl Error { /// Chain a blockchain error. pub fn from_blockchain(e: Box) -> Self { From cc2353db988ab6591d55c6625efaa59564fd561b Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 16:43:02 +0100 Subject: [PATCH 25/35] the last for now --- client/executor/common/Cargo.toml | 1 + client/executor/common/src/error.rs | 89 +++++++++++++++-------------- client/executor/common/src/util.rs | 16 ++---- 3 files changed, 52 insertions(+), 54 deletions(-) diff --git a/client/executor/common/Cargo.toml b/client/executor/common/Cargo.toml index 64ed23598f47c..acc5e9fe57562 100644 --- a/client/executor/common/Cargo.toml +++ b/client/executor/common/Cargo.toml @@ -24,6 +24,7 @@ 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 = [] diff --git a/client/executor/common/src/error.rs b/client/executor/common/src/error.rs index caed63c183e68..caf6159da0727 100644 --- a/client/executor/common/src/error.rs +++ b/client/executor/common/src/error.rs @@ -25,92 +25,95 @@ use wasmi; pub type Result = std::result::Result; /// Error type. -#[derive(Debug, derive_more::Display, derive_more::From)] +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] pub enum Error { /// Unserializable Data - InvalidData(sp_serializer::Error), + #[error("Unserializable data encountered")] + InvalidData(#[from] sp_serializer::Error), /// Trap occurred during execution - Trap(wasmi::Trap), + #[error(transparent)] + Trap(#[from] wasmi::Trap), /// Wasmi loading/instantiating error - Wasmi(wasmi::Error), + #[error(transparent)] + Wasmi(#[from] wasmi::Error), /// Error in the API. Parameter is an error message. - #[from(ignore)] + #[error("API Error: {0}")] ApiError(String), /// Method is not found - #[display(fmt="Method not found: '{}'", _0)] - #[from(ignore)] + #[error("Method not found: '{0}'")] MethodNotFound(String), /// Code is invalid (expected single byte) - #[display(fmt="Invalid Code: {}", _0)] - #[from(ignore)] + #[error("Invalid Code: '{0}'")] InvalidCode(String), /// Could not get runtime version. - #[display(fmt="On-chain runtime does not specify version")] + #[error("On-chain runtime does not specify version")] VersionInvalid, /// Externalities have failed. - #[display(fmt="Externalities error")] + #[error("Externalities error")] Externalities, /// Invalid index. - #[display(fmt="Invalid index provided")] + #[error("Invalid index provided")] InvalidIndex, /// Invalid return type. - #[display(fmt="Invalid type returned (should be u64)")] + #[error("Invalid type returned (should be u64)")] InvalidReturn, /// Runtime failed. - #[display(fmt="Runtime error")] + #[error("Runtime error")] Runtime, /// Runtime panicked. - #[display(fmt="Runtime panicked: {}", _0)] - #[from(ignore)] + #[error("Runtime panicked: {0}")] RuntimePanicked(String), /// Invalid memory reference. - #[display(fmt="Invalid memory reference")] + #[error("Invalid memory reference")] InvalidMemoryReference, /// The runtime must provide a global named `__heap_base` of type i32 for specifying where the /// allocator is allowed to place its data. - #[display(fmt="The runtime doesn't provide a global named `__heap_base`")] + #[error("The runtime doesn't provide a global named `__heap_base`")] HeapBaseNotFoundOrInvalid, /// The runtime WebAssembly module is not allowed to have the `start` function. - #[display(fmt="The runtime has the `start` function")] + #[error("The runtime has the `start` function")] RuntimeHasStartFn, /// Some other error occurred + #[error("Other: {0}")] Other(String), /// Some error occurred in the allocator - #[display(fmt="Error in allocator: {}", _0)] - Allocator(sp_allocator::Error), + #[error("Allocation Error")] + Allocator(#[from] sp_allocator::Error), /// Execution of a host function failed. - #[display(fmt="Host function {} execution failed with: {}", _0, _1)] + #[error("Host function {0} execution failed with: {1}")] FunctionExecution(String, String), /// No table is present. /// /// Call was requested that requires table but none was present in the instance. - #[display(fmt="No table exported by wasm blob")] + #[error("No table exported by wasm blob")] NoTable, /// No table entry is present. /// /// Call was requested that requires specific entry in the table to be present. - #[display(fmt="No table entry with index {} in wasm blob exported table", _0)] - #[from(ignore)] + #[error("No table entry with index {0} in wasm blob exported table")] NoTableEntryWithIndex(u32), /// Table entry is not a function. - #[display(fmt="Table element with index {} is not a function in wasm blob exported table", _0)] - #[from(ignore)] + #[error("Table element with index {0} is not a function in wasm blob exported table")] TableElementIsNotAFunction(u32), /// Function in table is null and thus cannot be called. - #[display(fmt="Table entry with index {} in wasm blob is null", _0)] - #[from(ignore)] + #[error("Table entry with index {0} in wasm blob is null")] FunctionRefIsNull(u32), -} -impl std::error::Error for Error { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Error::InvalidData(ref err) => Some(err), - Error::Trap(ref err) => Some(err), - Error::Wasmi(ref err) => Some(err), - _ => None, - } - } + #[error(transparent)] + RuntimeConstruction(#[from] WasmError), + + #[error("Shared memory is not supported")] + SharedMemUnsupported, + + #[error("Imported globals are not supported yet")] + ImportedGlobalsUnsupported, + + #[error("initializer expression can have only up to 2 expressions in wasm 1.0")] + InitializerHasTooManyExpressions, + + #[error("Invalid initializer expression provided {0}")] + InvalidInitializerExpression(String), } impl wasmi::HostError for Error {} @@ -121,9 +124,9 @@ impl From<&'static str> for Error { } } -impl From for Error { - fn from(err: WasmError) -> Error { - Error::Other(err.to_string()) +impl From for Error { + fn from(err: String) -> Error { + Error::Other(err) } } @@ -151,3 +154,5 @@ pub enum WasmError { /// Other error happenend. Other(String), } + +impl std::error::Error for WasmError {} diff --git a/client/executor/common/src/util.rs b/client/executor/common/src/util.rs index 92a48e1401814..564f9dadcbec6 100644 --- a/client/executor/common/src/util.rs +++ b/client/executor/common/src/util.rs @@ -87,15 +87,12 @@ impl DataSegmentsSnapshot { let init_expr = match segment.offset() { Some(offset) => offset.code(), // Return if the segment is passive - None => return Err(Error::from("Shared memory is not supported".to_string())), + None => return Err(Error::SharedMemUnsupported), }; // [op, End] if init_expr.len() != 2 { - return Err(Error::from( - "initializer expression can have only up to 2 expressions in wasm 1.0" - .to_string(), - )); + return Err(Error::InitializerHasTooManyExpressions); } let offset = match &init_expr[0] { Instruction::I32Const(v) => *v as u32, @@ -106,15 +103,10 @@ impl DataSegmentsSnapshot { // At the moment of writing the Substrate Runtime Interface does not provide // any globals. There is nothing that prevents us from supporting this // if/when we gain those. - return Err(Error::from( - "Imported globals are not supported yet".to_string(), - )); + return Err(Error::ImportedGlobalsUnsupported); } insn => { - return Err(Error::from(format!( - "{:?} is not supported as initializer expression in wasm 1.0", - insn - ))) + return Err(Error::InvalidInitializerExpression(format!("{:?}", insn))) } }; From 1d364ace0e661820c11730a037bb1191638668e6 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 16:47:26 +0100 Subject: [PATCH 26/35] chore unused deps --- Cargo.lock | 2 -- client/executor/common/Cargo.toml | 2 -- client/executor/common/src/lib.rs | 1 + 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0fa32f3b8c390..58280e528b036 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6890,12 +6890,10 @@ name = "sc-executor-common" version = "0.8.0" dependencies = [ "derive_more", - "log", "parity-scale-codec", "parity-wasm 0.41.0", "sp-allocator", "sp-core", - "sp-runtime-interface", "sp-serializer", "sp-wasm-interface", "thiserror", diff --git a/client/executor/common/Cargo.toml b/client/executor/common/Cargo.toml index acc5e9fe57562..8501144a9a989 100644 --- a/client/executor/common/Cargo.toml +++ b/client/executor/common/Cargo.toml @@ -14,7 +14,6 @@ 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" } @@ -22,7 +21,6 @@ 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" diff --git a/client/executor/common/src/lib.rs b/client/executor/common/src/lib.rs index 7f3864e6152fb..df839d4ab6523 100644 --- a/client/executor/common/src/lib.rs +++ b/client/executor/common/src/lib.rs @@ -17,6 +17,7 @@ //! A set of common definitions that are needed for defining execution engines. #![warn(missing_docs)] +#![deny(unused_crate_dependencies)] pub mod error; pub mod sandbox; From 56a0b2d1776a1c93bd9ec7210d3c347378769f2d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 17:31:55 +0100 Subject: [PATCH 27/35] another extraction --- Cargo.lock | 1 + client/sync-state-rpc/Cargo.toml | 1 + client/sync-state-rpc/src/lib.rs | 40 ++++++++++++++++++++++-------- primitives/blockchain/src/error.rs | 5 ---- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 58280e528b036..cfe4306da5908 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7451,6 +7451,7 @@ dependencies = [ "serde_json", "sp-blockchain", "sp-runtime", + "thiserror", ] [[package]] diff --git a/client/sync-state-rpc/Cargo.toml b/client/sync-state-rpc/Cargo.toml index 8da372db94ffc..81204365d0821 100644 --- a/client/sync-state-rpc/Cargo.toml +++ b/client/sync-state-rpc/Cargo.toml @@ -13,6 +13,7 @@ readme = "README.md" targets = ["x86_64-unknown-linux-gnu"] [dependencies] +thiserror = "1.0.21" jsonrpc-core = "15.0" jsonrpc-core-client = "15.0" jsonrpc-derive = "15.0" diff --git a/client/sync-state-rpc/src/lib.rs b/client/sync-state-rpc/src/lib.rs index 47b6b8b559d1e..573610fb2f610 100644 --- a/client/sync-state-rpc/src/lib.rs +++ b/client/sync-state-rpc/src/lib.rs @@ -17,6 +17,8 @@ //! A RPC handler to create sync states for light clients. //! Currently only usable with BABE + GRANDPA. +#![deny(unused_crate_dependencies)] + use sp_runtime::traits::{Block as BlockT, NumberFor}; use sp_blockchain::HeaderBackend; use std::sync::Arc; @@ -28,12 +30,27 @@ type SharedAuthoritySet = sc_finality_grandpa::SharedAuthoritySet<::Hash, NumberFor>; type SharedEpochChanges = sc_consensus_epochs::SharedEpochChanges; -struct Error(sp_blockchain::Error); +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +enum Error { + #[error(transparent)] + Blockchain(#[from] sp_blockchain::Error), + + #[error("Failed to load the block weight for block {0:?}")] + LoadingBlockWeightFailed(::Hash), + + #[error("JsonRpc error: {0}")] + JsonRpc(String), +} -impl From for jsonrpc_core::Error { - fn from(error: Error) -> Self { +impl From> for jsonrpc_core::Error { + fn from(error: Error) -> Self { + let message = match error { + Error::JsonRpc(s) => s, + _ => error.to_string(), + }; jsonrpc_core::Error { - message: error.0.to_string(), + message, code: jsonrpc_core::ErrorCode::ServerError(1), data: None, } @@ -76,7 +93,7 @@ impl SyncStateRpcHandler } } - fn build_sync_state(&self) -> Result, sp_blockchain::Error> { + fn build_sync_state(&self) -> Result, 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::MissingHeader(finalized_hash.to_string()))?; @@ -85,7 +102,7 @@ impl SyncStateRpcHandler &*self.client, finalized_hash, )? - .ok_or_else(|| sp_blockchain::Error::LoadingBlockWeightFailed(finalized_hash.to_string()))?; + .ok_or_else(|| Error::LoadingBlockWeightFailed(finalized_hash))?; Ok(sc_chain_spec::LightSyncState { finalized_block_header: finalized_header, @@ -110,15 +127,16 @@ impl SyncStateRpcApi for SyncStateRpcHandler let mut chain_spec = self.chain_spec.cloned_box(); - let sync_state = self.build_sync_state().map_err(Error)?; + let sync_state = self.build_sync_state() + .map_err(map_error::>)?; chain_spec.set_light_sync_state(sync_state.to_serializable()); - let string = chain_spec.as_json(raw).map_err(map_error)?; + let string = chain_spec.as_json(raw).map_err(map_error::)?; - serde_json::from_str(&string).map_err(|err| map_error(err.to_string())) + serde_json::from_str(&string).map_err(|err| map_error::(err)) } } -fn map_error(error: String) -> jsonrpc_core::Error { - Error(sp_blockchain::Error::JsonRpc(error)).into() +fn map_error(error: S) -> jsonrpc_core::Error { + Error::::JsonRpc(error.to_string()).into() } diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index 14dfbacc674f9..c7180a61b00ca 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -50,9 +50,6 @@ pub enum Error { #[error("Cancelled oneshot channel {0}")] OneShotCancelled(#[from] futures::channel::oneshot::Canceled), - #[error("JsonRpc error: {0}")] - JsonRpc(String), - #[error(transparent)] Consensus(#[from] sp_consensus::Error), @@ -156,8 +153,6 @@ pub enum Error { #[error("Failed to get header for hash {0}")] MissingHeader(String), - #[error("Failed to load the block weight for block {0}")] - LoadingBlockWeightFailed(String), #[error("State Database error: {0}")] StateDatabase(String), From c0805940184a62cd9302603ad911c3591e70a60c Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 18:34:48 +0100 Subject: [PATCH 28/35] reduce should panic, due to extended error messages --- client/executor/src/integration_tests/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index c8b763a6b1936..08771847c25fd 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -523,7 +523,7 @@ fn offchain_http_should_work(wasm_method: WasmExecutionMethod) { #[test_case(WasmExecutionMethod::Interpreted)] #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))] -#[should_panic(expected = "Allocator ran out of space")] +#[should_panic] fn should_trap_when_heap_exhausted(wasm_method: WasmExecutionMethod) { let mut ext = TestExternalities::default(); From 077301e1a82f11a83c88b6533a3b7669befb653c Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 26 Nov 2020 19:10:26 +0100 Subject: [PATCH 29/35] error test happiness --- .../api/test/tests/ui/mock_advanced_block_id_by_value.rs | 1 + primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs | 1 + primitives/api/test/tests/ui/mock_only_one_error_type.stderr | 4 ++-- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs index 35dc814d1851b..fd654ffdc63d6 100644 --- a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs +++ b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.rs @@ -1,4 +1,5 @@ use substrate_test_runtime_client::runtime::Block; +use sp_api::ApiError; sp_api::decl_runtime_apis! { pub trait Api { diff --git a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs index 0f1b16fb8c057..a15ef133fa6c4 100644 --- a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs +++ b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.rs @@ -1,4 +1,5 @@ use substrate_test_runtime_client::runtime::Block; +use sp_api::ApiError; sp_api::decl_runtime_apis! { pub trait Api { diff --git a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr index 9f8ae58f2f691..82fd04e8c5e04 100644 --- a/primitives/api/test/tests/ui/mock_only_one_error_type.stderr +++ b/primitives/api/test/tests/ui/mock_only_one_error_type.stderr @@ -10,11 +10,11 @@ error: First error type was declared here. 17 | type Error = u32; | ^^^ -error[E0277]: the trait bound `u32: std::convert::From< sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiError>` is not satisfied +error[E0277]: the trait bound `u32: std::convert::From` is not satisfied --> $DIR/mock_only_one_error_type.rs:17:16 | 17 | type Error = u32; - | ^^^ the trait `std::convert::From< sp_api_hidden_includes_DECL_RUNTIME_APIS::sp_api::ApiError>` is not implemented for `u32` + | ^^^ the trait `std::convert::From` is not implemented for `u32` | ::: $WORKSPACE/primitives/api/src/lib.rs | From 0983c88e46f987e55d2b6f639900e28b79136b21 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 27 Nov 2020 06:55:55 +0100 Subject: [PATCH 30/35] shift error lines by one --- .../ui/mock_advanced_block_id_by_value.stderr | 14 +++++++------- .../tests/ui/mock_advanced_missing_blockid.stderr | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr index b4ee64a1bb7d1..47cd9e01d910f 100644 --- a/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr +++ b/primitives/api/test/tests/ui/mock_advanced_block_id_by_value.stderr @@ -1,13 +1,13 @@ error: `BlockId` needs to be taken by reference and not by value! - --> $DIR/mock_advanced_block_id_by_value.rs:11:1 + --> $DIR/mock_advanced_block_id_by_value.rs:12:1 | -11 | / sp_api::mock_impl_runtime_apis! { -12 | | impl Api for MockApi { -13 | | #[advanced] -14 | | fn test(&self, _: BlockId) -> Result, ApiError> { +12 | / sp_api::mock_impl_runtime_apis! { +13 | | impl Api for MockApi { +14 | | #[advanced] +15 | | fn test(&self, _: BlockId) -> Result, ApiError> { ... | -17 | | } -18 | | } +18 | | } +19 | | } | |_^ | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr index 605c4869a735b..87d3660316b1e 100644 --- a/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr +++ b/primitives/api/test/tests/ui/mock_advanced_missing_blockid.stderr @@ -1,5 +1,5 @@ error: If using the `advanced` attribute, it is required that the function takes at least one argument, the `BlockId`. - --> $DIR/mock_advanced_missing_blockid.rs:14:3 + --> $DIR/mock_advanced_missing_blockid.rs:15:3 | -14 | fn test(&self) -> Result, ApiError> { +15 | fn test(&self) -> Result, ApiError> { | ^^ From 0e9d6965fd4b8d2517c71c1d96db4c10f8772cc3 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 27 Nov 2020 11:34:16 +0100 Subject: [PATCH 31/35] doc tests --- primitives/api/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 7ee5a1f2bd4bf..524b8d53283ac 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -289,7 +289,7 @@ pub use sp_api_proc_macro::impl_runtime_apis; /// /// Sets the error type that is being used by the mock implementation. /// /// The error type is used by all runtime apis. It is only required to /// /// be specified in one trait implementation. -/// type Error = ApiError; +/// type Error = sp_api::ApiError; /// /// fn build_block() -> Block { /// unimplemented!("Not Required in tests") @@ -333,15 +333,15 @@ pub use sp_api_proc_macro::impl_runtime_apis; /// /// sp_api::mock_impl_runtime_apis! { /// impl Balance for MockApi { -/// type Error = ApiError; +/// type Error = sp_api::ApiError; /// #[advanced] -/// fn get_balance(&self, at: &BlockId) -> Result, ApiError> { +/// fn get_balance(&self, at: &BlockId) -> Result, Self::Error> { /// println!("Being called at: {}", at); /// /// Ok(self.balance.into()) /// } /// #[advanced] -/// fn set_balance(at: &BlockId, val: u64) -> Result, ApiError> { +/// fn set_balance(at: &BlockId, val: u64) -> Result, Self::Error> { /// if let BlockId::Number(1) = at { /// println!("Being called to set balance to: {}", val); /// } From 35d281489a5603e84fa00fd9b015501c3986acc5 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 27 Nov 2020 16:06:05 +0100 Subject: [PATCH 32/35] white space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/service/src/client/wasm_override.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index a7dac9caaf27e..b1d92fb1c553f 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -83,7 +83,6 @@ impl FetchRuntimeCode for WasmBlob { } } - #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum WasmOverrideError { From 13810ebd54bbc3bc7c5e7714fc94392122f403d6 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 27 Nov 2020 16:06:28 +0100 Subject: [PATCH 33/35] Into -> From MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/service/src/client/wasm_override.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index b1d92fb1c553f..ba76f7a0fcf29 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -100,9 +100,9 @@ pub enum WasmOverrideError { DuplicateRuntime(Vec), } -impl Into for WasmOverrideError { - fn into(self) -> sp_blockchain::Error { - sp_blockchain::Error::Application(Box::new(self)) +impl From for sp_blockchain::Error { + fn from(err: WasmOverrideError) -> Self { + Self::Application(Box::new(err)) } } From 6df60d097d40fd87cd7d626e4b636c18076f7525 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 27 Nov 2020 16:07:57 +0100 Subject: [PATCH 34/35] remove pointless codec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- primitives/api/test/tests/decl_and_impl.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/primitives/api/test/tests/decl_and_impl.rs b/primitives/api/test/tests/decl_and_impl.rs index a108d5cba27b6..be549d7b7f4cd 100644 --- a/primitives/api/test/tests/decl_and_impl.rs +++ b/primitives/api/test/tests/decl_and_impl.rs @@ -24,7 +24,6 @@ use sp_runtime::{traits::{GetNodeBlockType, Block as BlockT}, generic::BlockId}; use sp_core::NativeOrEncoded; use substrate_test_runtime_client::runtime::Block; use sp_blockchain::Result; -use codec; /// The declaration of the `Runtime` type and the implementation of the `GetNodeBlockType` /// trait are done by the `construct_runtime!` macro in a real runtime. From d0c0d242e015c8d66bba574df6ec8be26846aebb Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 27 Nov 2020 16:08:18 +0100 Subject: [PATCH 35/35] avoid pointless self import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- primitives/api/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/api/src/lib.rs b/primitives/api/src/lib.rs index 524b8d53283ac..96da63cf2e253 100644 --- a/primitives/api/src/lib.rs +++ b/primitives/api/src/lib.rs @@ -69,7 +69,7 @@ pub use sp_std::{slice, mem}; #[cfg(feature = "std")] use sp_std::result; #[doc(hidden)] -pub use codec::{self, Encode, Decode, DecodeLimit}; +pub use codec::{Encode, Decode, DecodeLimit}; use sp_core::OpaqueMetadata; #[cfg(feature = "std")] use std::{panic::UnwindSafe, cell::RefCell};