tx-pool: make error generic over hash

transaction-pool/src/error.rs

@@ -14,15 +14,11 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
-/// TODO: Consider making Hash generic for Error - legacy of error-chain
-/// So the hashes are converted to debug strings for easy display.
-type Hash = String;
-
 use std::{error, fmt, result};
 
 /// Transaction Pool Error
 #[derive(Debug)]
-pub enum Error {
+pub enum Error<Hash: fmt::Debug + fmt::LowerHex> {
 	/// Transaction is already imported
 	AlreadyImported(Hash),
 	/// Transaction is too cheap to enter the queue
@@ -32,25 +28,25 @@ pub enum Error {
 }
 
 /// Transaction Pool Result
-pub type Result<T> = result::Result<T, Error>;
+pub type Result<T, H> = result::Result<T, Error<H>>;
 
-impl fmt::Display for Error {
+impl<H: fmt::Debug + fmt::LowerHex> fmt::Display for Error<H> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 		match self {
 			Error::AlreadyImported(h) =>
-				write!(f, "[{}] already imported", h),
+				write!(f, "[{:?}] already imported", h),
 			Error::TooCheapToEnter(hash, min_score) =>
-				write!(f, "[{}] too cheap to enter the pool. Min score: {}", hash, min_score),
+				write!(f, "[{:x}] too cheap to enter the pool. Min score: {}", hash, min_score),
 			Error::TooCheapToReplace(old_hash, hash) =>
-				write!(f, "[{}] too cheap to replace: {}", hash, old_hash),
+				write!(f, "[{:x}] too cheap to replace: {:x}", hash, old_hash),
 		}
     }
 }
 
-impl error::Error for Error {}
+impl<H: fmt::Debug + fmt::LowerHex> error::Error for Error<H> {}
 
 #[cfg(test)]
-impl PartialEq for Error {
+impl<H: fmt::Debug + fmt::LowerHex> PartialEq for Error<H> where H: PartialEq {
 	fn eq(&self, other: &Self) -> bool {
 		use self::Error::*;
 

transaction-pool/src/listener.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::sync::Arc;
+use std::{fmt::{Debug, LowerHex}, sync::Arc};
 use error::Error;
 
 /// Transaction pool listener.
@@ -29,7 +29,7 @@ pub trait Listener<T> {
 
 	/// The transaction was rejected from the pool.
 	/// It means that it was too cheap to replace any transaction already in the pool.
-	fn rejected(&mut self, _tx: &Arc<T>, _reason: &Error) {}
+	fn rejected<H: Debug + LowerHex>(&mut self, _tx: &Arc<T>, _reason: &Error<H>) {}
 
 	/// The transaction was pushed out from the pool because of the limit.
 	fn dropped(&mut self, _tx: &Arc<T>, _by: Option<&T>) {}
@@ -58,7 +58,7 @@ impl<T, A, B> Listener<T> for (A, B) where
 		self.1.added(tx, old);
 	}
 
-	fn rejected(&mut self, tx: &Arc<T>, reason: &Error) {
+	fn rejected<H: Debug + LowerHex>(&mut self, tx: &Arc<T>, reason: &Error<H>) {
 		self.0.rejected(tx, reason);
 		self.1.rejected(tx, reason);
 	}

transaction-pool/src/pool.rs

@@ -133,11 +133,11 @@ impl<T, S, L> Pool<T, S, L> where
 	/// If any limit is reached the transaction with the lowest `Score` is evicted to make room.
 	///
 	/// The `Listener` will be informed on any drops or rejections.
-	pub fn import(&mut self, transaction: T) -> error::Result<Arc<T>> {
+	pub fn import(&mut self, transaction: T) -> error::Result<Arc<T>, T::Hash> {
 		let mem_usage = transaction.mem_usage();
 
 		if self.by_hash.contains_key(transaction.hash()) {
-			return Err(error::Error::AlreadyImported(format!("{:?}", transaction.hash())))
+			return Err(error::Error::AlreadyImported(transaction.hash().clone()))
 		}
 
 		self.insertion_id += 1;
@@ -204,12 +204,12 @@ impl<T, S, L> Pool<T, S, L> where
 				Ok(new.transaction)
 			},
 			AddResult::TooCheap { new, old } => {
-				let error = error::Error::TooCheapToReplace(format!("{:x}", old.hash()), format!("{:x}", new.hash()));
+				let error = error::Error::TooCheapToReplace(old.hash().clone(), new.hash().clone());
 				self.listener.rejected(&new, &error);
 				return Err(error)
 			},
 			AddResult::TooCheapToEnter(new, score) => {
-				let error = error::Error::TooCheapToEnter(format!("{:x}", new.hash()), format!("{:#x}", score));
+				let error = error::Error::TooCheapToEnter(new.hash().clone(), format!("{:#x}", score));
 				self.listener.rejected(&new, &error);
 				return Err(error)
 			}
@@ -283,12 +283,12 @@ impl<T, S, L> Pool<T, S, L> where
 	///
 	/// Returns `None` in case we couldn't decide if the transaction should replace the worst transaction or not.
 	/// In such case we will accept the transaction even though it is going to exceed the limit.
-	fn remove_worst(&mut self, transaction: &Transaction<T>) -> error::Result<Option<Transaction<T>>> {
+	fn remove_worst(&mut self, transaction: &Transaction<T>) -> error::Result<Option<Transaction<T>>, T::Hash> {
 		let to_remove = match self.worst_transactions.iter().next_back() {
 			// No elements to remove? and the pool is still full?
 			None => {
 				warn!("The pool is full but there are no transactions to remove.");
-				return Err(error::Error::TooCheapToEnter(format!("{:?}", transaction.hash()), "unknown".into()).into());
+				return Err(error::Error::TooCheapToEnter(transaction.hash().clone(), "unknown".into()))
 			},
 			Some(old) => match self.scoring.should_replace(&old.transaction, transaction) {
 				// We can't decide which of them should be removed, so accept both.
@@ -297,7 +297,7 @@ impl<T, S, L> Pool<T, S, L> where
 				scoring::Choice::ReplaceOld => Some(old.clone()),
 				// otherwise fail
 				scoring::Choice::RejectNew => {
-					return Err(error::Error::TooCheapToEnter(format!("{:?}", transaction.hash()), format!("{:#x}", old.score)).into())
+					return Err(error::Error::TooCheapToEnter(transaction.hash().clone(), format!("{:#x}", old.score)))
 				},
 			},
 		};

transaction-pool/src/tests/mod.rs

@@ -133,7 +133,7 @@ fn should_reject_if_above_count() {
 	// Reject second
 	let tx1 = b.tx().nonce(0).new();
 	let tx2 = b.tx().nonce(1).new();
-	let hash = format!("{:?}", tx2.hash());
+	let hash = tx2.hash.clone();
 	txq.import(tx1).unwrap();
 	assert_eq!(txq.import(tx2).unwrap_err(), error::Error::TooCheapToEnter(hash, "0x0".into()));
 	assert_eq!(txq.light_status().transaction_count, 1);
@@ -159,7 +159,7 @@ fn should_reject_if_above_mem_usage() {
 	// Reject second
 	let tx1 = b.tx().nonce(1).mem_usage(1).new();
 	let tx2 = b.tx().nonce(2).mem_usage(2).new();
-	let hash = format!("{:?}", tx2.hash());
+	let hash = tx2.hash.clone();
 	txq.import(tx1).unwrap();
 	assert_eq!(txq.import(tx2).unwrap_err(), error::Error::TooCheapToEnter(hash, "0x0".into()));
 	assert_eq!(txq.light_status().transaction_count, 1);
@@ -185,7 +185,7 @@ fn should_reject_if_above_sender_count() {
 	// Reject second
 	let tx1 = b.tx().nonce(1).new();
 	let tx2 = b.tx().nonce(2).new();
-	let hash = format!("{:x}", tx2.hash());
+	let hash = tx2.hash.clone();
 	txq.import(tx1).unwrap();
 	assert_eq!(txq.import(tx2).unwrap_err(), error::Error::TooCheapToEnter(hash, "0x0".into()));
 	assert_eq!(txq.light_status().transaction_count, 1);
@@ -195,7 +195,7 @@ fn should_reject_if_above_sender_count() {
 	// Replace first
 	let tx1 = b.tx().nonce(1).new();
 	let tx2 = b.tx().nonce(2).gas_price(2).new();
-	let hash = format!("{:x}", tx2.hash());
+	let hash = tx2.hash.clone();
 	txq.import(tx1).unwrap();
 	// This results in error because we also compare nonces
 	assert_eq!(txq.import(tx2).unwrap_err(), error::Error::TooCheapToEnter(hash, "0x0".into()));
@@ -625,6 +625,7 @@ fn should_import_even_if_sender_limit_is_reached() {
 mod listener {
 	use std::cell::RefCell;
 	use std::rc::Rc;
+	use std::fmt;
 
 	use super::*;
 
@@ -636,7 +637,7 @@ mod listener {
 			self.0.borrow_mut().push(if old.is_some() { "replaced" } else { "added" });
 		}
 
-		fn rejected(&mut self, _tx: &SharedTransaction, _reason: &error::Error) {
+		fn rejected<H: fmt::Debug + fmt::LowerHex>(&mut self, _tx: &SharedTransaction, _reason: &error::Error<H>) {
 			self.0.borrow_mut().push("rejected".into());
 		}