Move ethcore::Error to error_chain (openethereum#8386)

* WIP * Convert Ethcore error to use error_chain * Use error_chain for ImportError and BlockImportError * Fix error pattern matches for error_chain in miner * Implement explicit From for AccountsError * Fix pattern matches for ErrorKinds * Handle ethcore error_chain in light client * Explicitly define Result type to avoid shadowing * Fix remaining Error pattern matches * Fix tab space formatting * Helps if the tests compile * Fix error chain matching after merge
VladLupashevskyi · May 23, 2018 · 23a7dba · 23a7dba
1 parent 9ca84a8
commit 23a7dba
Show file tree

Hide file tree

Showing 27 changed files with 246 additions and 271 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/ethcore/Cargo.toml b/ethcore/Cargo.toml
@@ -20,6 +20,7 @@ fetch = { path = "../util/fetch" }
 hashdb = { path = "../util/hashdb" }
 memorydb = { path = "../util/memorydb" }
 patricia-trie = { path = "../util/patricia_trie" }
+error-chain = { version = "0.11", default-features = false }
 ethcore-io = { path = "../util/io" }
 ethcore-logger = { path = "../logger" }
 ethcore-miner = { path = "../miner" }

diff --git a/ethcore/light/Cargo.toml b/ethcore/light/Cargo.toml
@@ -35,6 +35,7 @@ keccak-hash = { path = "../../util/hash" }
 triehash = { path = "../../util/triehash" }
 kvdb = { path = "../../util/kvdb" }
 memory-cache = { path = "../../util/memory_cache" }
+error-chain = { version = "0.11", default-features = false }
 
 [dev-dependencies]
 kvdb-memorydb = { path = "../../util/kvdb-memorydb" }

diff --git a/ethcore/light/src/client/header_chain.rs b/ethcore/light/src/client/header_chain.rs
@@ -31,7 +31,7 @@ use std::sync::Arc;
 use cht;
 
 use ethcore::block_status::BlockStatus;
-use ethcore::error::{Error, BlockImportError, BlockError};
+use ethcore::error::{Error, ErrorKind, BlockImportError, BlockImportErrorKind, BlockError};
 use ethcore::encoded;
 use ethcore::header::Header;
 use ethcore::ids::BlockId;
@@ -260,7 +260,7 @@ impl HeaderChain {
 			let best_block = {
 				let era = match candidates.get(&curr.best_num) {
 					Some(era) => era,
-					None => return Err(Error::Database("Database corrupt: highest block referenced but no data.".into())),
+					None => bail!(ErrorKind::Database("Database corrupt: highest block referenced but no data.".into())),
 				};
 
 				let best = &era.candidates[0];
@@ -332,8 +332,7 @@ impl HeaderChain {
 
 		// instantiate genesis epoch data if it doesn't exist.
 		if let None = chain.db.get(col, LAST_CANONICAL_TRANSITION)? {
-			let genesis_data = spec.genesis_epoch_data()
-				.map_err(|s| Error::Database(s.into()))?;
+			let genesis_data = spec.genesis_epoch_data()?;
 
 			{
 				let mut batch = chain.db.transaction();
@@ -411,7 +410,7 @@ impl HeaderChain {
 							.and_then(|entry| entry.candidates.iter().find(|c| c.hash == parent_hash))
 							.map(|c| c.total_difficulty)
 							.ok_or_else(|| BlockError::UnknownParent(parent_hash))
-							.map_err(BlockImportError::Block)?
+							.map_err(BlockImportErrorKind::Block)?
 					};
 
 				parent_td + *header.difficulty()
@@ -580,7 +579,7 @@ impl HeaderChain {
 					} else {
 						let msg = format!("header of block #{} not found in DB ; database in an \
 											inconsistent state", h_num);
-						return Err(Error::Database(msg.into()));
+						bail!(ErrorKind::Database(msg.into()));
 					};
 
 					let decoded = header.decode();
@@ -590,7 +589,7 @@ impl HeaderChain {
 							.ok_or_else(|| {
 								let msg = format!("entry for era #{} not found in DB ; database \
 													in an inconsistent state", h_num);
-								Error::Database(msg.into())
+								ErrorKind::Database(msg.into())
 							})?;
 						::rlp::decode(&bytes)
 					};
@@ -600,7 +599,7 @@ impl HeaderChain {
 						.ok_or_else(|| {
 							let msg = "no candidate matching block found in DB ; database in an \
 										inconsistent state";
-							Error::Database(msg.into())
+							ErrorKind::Database(msg.into())
 						})?
 						.total_difficulty;
 

diff --git a/ethcore/light/src/lib.rs b/ethcore/light/src/lib.rs
@@ -80,6 +80,8 @@ extern crate keccak_hash as hash;
 extern crate triehash;
 extern crate kvdb;
 extern crate memory_cache;
+#[macro_use]
+extern crate error_chain;
 
 #[cfg(test)]
 extern crate kvdb_memorydb;

diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs
@@ -49,7 +49,7 @@ use client::{
 };
 use encoded;
 use engines::{EthEngine, EpochTransition};
-use error::{ImportError, ExecutionError, CallError, BlockError, ImportResult, Error as EthcoreError};
+use error::{ImportErrorKind, BlockImportErrorKind, ExecutionError, CallError, BlockError, ImportResult, Error as EthcoreError};
 use vm::{EnvInfo, LastHashes};
 use evm::Schedule;
 use executive::{Executive, Executed, TransactOptions, contract_address};
@@ -1417,11 +1417,11 @@ impl ImportBlock for Client {
 
 		{
 			if self.chain.read().is_known(&unverified.hash()) {
-				return Err(BlockImportError::Import(ImportError::AlreadyInChain));
+				bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain));
 			}
 			let status = self.block_status(BlockId::Hash(unverified.parent_hash()));
 			if status == BlockStatus::Unknown || status == BlockStatus::Pending {
-				return Err(BlockImportError::Block(BlockError::UnknownParent(unverified.parent_hash())));
+				bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(unverified.parent_hash())));
 			}
 		}
 		Ok(self.importer.block_queue.import(unverified)?)
@@ -1432,11 +1432,11 @@ impl ImportBlock for Client {
 			// check block order
 			let header = view!(BlockView, &block_bytes).header_view();
 			if self.chain.read().is_known(&header.hash()) {
-				return Err(BlockImportError::Import(ImportError::AlreadyInChain));
+				bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain));
 			}
 			let status = self.block_status(BlockId::Hash(header.parent_hash()));
 			if  status == BlockStatus::Unknown || status == BlockStatus::Pending {
-				return Err(BlockImportError::Block(BlockError::UnknownParent(header.parent_hash())));
+				bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(header.parent_hash())));
 			}
 		}
 

diff --git a/ethcore/src/client/mod.rs b/ethcore/src/client/mod.rs
@@ -48,7 +48,7 @@ pub use types::call_analytics::CallAnalytics;
 pub use executive::{Executed, Executive, TransactOptions};
 pub use vm::{LastHashes, EnvInfo};
 
-pub use error::{BlockImportError, TransactionImportError};
+pub use error::{BlockImportError, BlockImportErrorKind, TransactionImportError};
 pub use verification::VerifierType;
 
 mod traits;

diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs
@@ -1339,7 +1339,7 @@ mod tests {
 	use transaction::{Action, Transaction};
 	use engines::{Seal, Engine, EngineError, EthEngine};
 	use engines::validator_set::TestSet;
-	use error::Error;
+	use error::{Error, ErrorKind};
 	use super::{AuthorityRoundParams, AuthorityRound, EmptyStep, SealedEmptyStep};
 
 	#[test]
@@ -1842,7 +1842,7 @@ mod tests {
 		]);
 
 		assert!(match engine.verify_block_family(&header, &parent_header) {
-			Err(Error::Engine(EngineError::InsufficientProof(ref s)))
+			Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _))
 				if s.contains("invalid step") => true,
 			_ => false,
 		});
@@ -1856,7 +1856,7 @@ mod tests {
 		]);
 
 		assert!(match engine.verify_block_family(&header, &parent_header) {
-			Err(Error::Engine(EngineError::InsufficientProof(ref s)))
+			Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _))
 				if s.contains("invalid empty step proof") => true,
 			_ => false,
 		});
@@ -1871,7 +1871,7 @@ mod tests {
 		]);
 
 		assert!(match engine.verify_block_family(&header, &parent_header) {
-			Err(Error::Engine(EngineError::InsufficientProof(ref s)))
+			Err(Error(ErrorKind::Engine(EngineError::InsufficientProof(ref s)), _))
 				if s.contains("invalid empty step proof") => true,
 			_ => false,
 		});

diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs
@@ -37,7 +37,7 @@ pub use self::tendermint::Tendermint;
 
 use std::sync::{Weak, Arc};
 use std::collections::{BTreeMap, HashMap};
-use std::fmt;
+use std::{fmt, error};
 
 use self::epoch::PendingTransition;
 
@@ -102,6 +102,12 @@ impl fmt::Display for EngineError {
 	}
 }
 
+impl error::Error for EngineError {
+	fn description(&self) -> &str {
+		"Engine error"
+	}
+}
+
 /// Seal type.
 #[derive(Debug, PartialEq, Eq)]
 pub enum Seal {

diff --git a/ethcore/src/engines/tendermint/mod.rs b/ethcore/src/engines/tendermint/mod.rs
@@ -768,7 +768,7 @@ mod tests {
 	use ethereum_types::Address;
 	use bytes::Bytes;
 	use block::*;
-	use error::{Error, BlockError};
+	use error::{Error, ErrorKind, BlockError};
 	use header::Header;
 	use client::ChainInfo;
 	use miner::MinerService;
@@ -855,7 +855,7 @@ mod tests {
 		let verify_result = engine.verify_block_basic(&header);
 
 		match verify_result {
-			Err(Error::Block(BlockError::InvalidSealArity(_))) => {},
+			Err(Error(ErrorKind::Block(BlockError::InvalidSealArity(_)), _)) => {},
 			Err(_) => { panic!("should be block seal-arity mismatch error (got {:?})", verify_result); },
 			_ => { panic!("Should be error, got Ok"); },
 		}
@@ -885,7 +885,7 @@ mod tests {
 		header.set_seal(seal);
 		// Bad proposer.
 		match engine.verify_block_external(&header) {
-			Err(Error::Engine(EngineError::NotProposer(_))) => {},
+			Err(Error(ErrorKind::Engine(EngineError::NotProposer(_)), _)) => {},
 			_ => panic!(),
 		}
 
@@ -895,7 +895,7 @@ mod tests {
 		header.set_seal(seal);
 		// Not authority.
 		match engine.verify_block_external(&header) {
-			Err(Error::Engine(EngineError::NotAuthorized(_))) => {},
+			Err(Error(ErrorKind::Engine(EngineError::NotAuthorized(_)), _)) => {},
 			_ => panic!(),
 		};
 		engine.stop();
@@ -925,7 +925,7 @@ mod tests {
 
 		// One good signature is not enough.
 		match engine.verify_block_external(&header) {
-			Err(Error::Engine(EngineError::BadSealFieldSize(_))) => {},
+			Err(Error(ErrorKind::Engine(EngineError::BadSealFieldSize(_)), _)) => {},
 			_ => panic!(),
 		}
 
@@ -945,7 +945,7 @@ mod tests {
 
 		// One good and one bad signature.
 		match engine.verify_block_external(&header) {
-			Err(Error::Engine(EngineError::NotAuthorized(_))) => {},
+			Err(Error(ErrorKind::Engine(EngineError::NotAuthorized(_)), _)) => {},
 			_ => panic!(),
 		};
 		engine.stop();
@@ -1092,15 +1092,15 @@ mod tests {
 					} else if *s == signature0 {
 						Ok(voter)
 					} else {
-						Err(Error::Ethkey(EthkeyError::InvalidSignature))
+						Err(ErrorKind::Ethkey(EthkeyError::InvalidSignature).into())
 					}
 				}
 			},
 		};
 
 		// One good signature is not enough.
 		match epoch_verifier.verify_light(&header) {
-			Err(Error::Engine(EngineError::BadSealFieldSize(_))) => {},
+			Err(Error(ErrorKind::Engine(EngineError::BadSealFieldSize(_)), _)) => {},
 			_ => panic!(),
 		}
 
@@ -1117,7 +1117,7 @@ mod tests {
 
 		// One good and one bad signature.
 		match epoch_verifier.verify_light(&header) {
-			Err(Error::Ethkey(EthkeyError::InvalidSignature)) => {},
+			Err(Error(ErrorKind::Ethkey(EthkeyError::InvalidSignature), _)) => {},
 			_ => panic!(),
 		};