cleanup util::from_hex() (#3265)

* cleanup our from_hex()

* fix keychain tests

* add test coverage for empty hex string

api/src/foreign_rpc.rs

@@ -747,7 +747,7 @@ impl ForeignRpc for Foreign {
 	) -> Result<BlockHeaderPrintable, ErrorKind> {
 		let mut parsed_hash: Option<Hash> = None;
 		if let Some(hash) = hash {
-			let vec = util::from_hex(hash)
+			let vec = util::from_hex(&hash)
 				.map_err(|e| ErrorKind::Argument(format!("invalid block hash: {}", e)))?;
 			parsed_hash = Some(Hash::from_vec(&vec));
 		}
@@ -761,7 +761,7 @@ impl ForeignRpc for Foreign {
 	) -> Result<BlockPrintable, ErrorKind> {
 		let mut parsed_hash: Option<Hash> = None;
 		if let Some(hash) = hash {
-			let vec = util::from_hex(hash)
+			let vec = util::from_hex(&hash)
 				.map_err(|e| ErrorKind::Argument(format!("invalid block hash: {}", e)))?;
 			parsed_hash = Some(Hash::from_vec(&vec));
 		}

api/src/handlers/blocks_api.rs

@@ -47,7 +47,7 @@ impl HeaderHandler {
 			}
 		}
 		check_block_param(&input)?;
-		let vec = util::from_hex(input)
+		let vec = util::from_hex(&input)
 			.map_err(|e| ErrorKind::Argument(format!("invalid input: {}", e)))?;
 		let h = Hash::from_vec(&vec);
 		let header = w(&self.chain)?
@@ -153,7 +153,7 @@ impl BlockHandler {
 			}
 		}
 		check_block_param(&input)?;
-		let vec = util::from_hex(input)
+		let vec = util::from_hex(&input)
 			.map_err(|e| ErrorKind::Argument(format!("invalid input: {}", e)))?;
 		Ok(Hash::from_vec(&vec))
 	}

api/src/handlers/chain_api.rs

@@ -308,7 +308,7 @@ impl OutputHandler {
 		let query = must_get_query!(req);
 		let params = QueryParams::from(query);
 		params.process_multival_param("id", |id| {
-			if let Ok(x) = util::from_hex(String::from(id)) {
+			if let Ok(x) = util::from_hex(id) {
 				commitments.push(Commitment::from_vec(x));
 			}
 		});
@@ -392,7 +392,7 @@ impl KernelHandler {
 			.rsplit('/')
 			.next()
 			.ok_or_else(|| ErrorKind::RequestError("missing excess".into()))?;
-		let excess = util::from_hex(excess.to_owned())
+		let excess = util::from_hex(excess)
 			.map_err(|_| ErrorKind::RequestError("invalid excess hex".into()))?;
 		if excess.len() != 33 {
 			return Err(ErrorKind::RequestError("invalid excess length".into()).into());
@@ -444,7 +444,7 @@ impl KernelHandler {
 		min_height: Option<u64>,
 		max_height: Option<u64>,
 	) -> Result<LocatedTxKernel, Error> {
-		let excess = util::from_hex(excess)
+		let excess = util::from_hex(&excess)
 			.map_err(|_| ErrorKind::RequestError("invalid excess hex".into()))?;
 		if excess.len() != 33 {
 			return Err(ErrorKind::RequestError("invalid excess length".into()).into());

api/src/handlers/pool_api.rs

@@ -109,7 +109,7 @@ async fn update_pool(
 	let fluff = params.get("fluff").is_some();
 
 	let wrapper: TxWrapper = parse_body(req).await?;
-	let tx_bin = util::from_hex(wrapper.tx_hex)
+	let tx_bin = util::from_hex(&wrapper.tx_hex)
 		.map_err(|e| ErrorKind::RequestError(format!("Bad request: {}", e)))?;
 
 	// All wallet api interaction explicitly uses protocol version 1 for now.

api/src/handlers/transactions_api.rs

@@ -120,10 +120,8 @@ impl TxHashSetHandler {
 	// return a dummy output with merkle proof for position filled out
 	// (to avoid having to create a new type to pass around)
 	fn get_merkle_proof_for_output(&self, id: &str) -> Result<OutputPrintable, Error> {
-		let c = util::from_hex(String::from(id)).context(ErrorKind::Argument(format!(
-			"Not a valid commitment: {}",
-			id
-		)))?;
+		let c = util::from_hex(id)
+			.map_err(|_| ErrorKind::Argument(format!("Not a valid commitment: {}", id)))?;
 		let commit = Commitment::from_vec(c);
 		let chain = w(&self.chain)?;
 		let output_pos = chain.get_output_pos(&commit).context(ErrorKind::NotFound)?;

api/src/handlers/utils.rs

@@ -19,7 +19,6 @@ use crate::rest::*;
 use crate::types::*;
 use crate::util;
 use crate::util::secp::pedersen::Commitment;
-use failure::ResultExt;
 use std::sync::{Arc, Weak};
 
 // All handlers use `Weak` references instead of `Arc` to avoid cycles that
@@ -35,10 +34,8 @@ fn get_unspent(
 	chain: &Arc<chain::Chain>,
 	id: &str,
 ) -> Result<Option<(CommitPos, OutputIdentifier)>, Error> {
-	let c = util::from_hex(String::from(id)).context(ErrorKind::Argument(format!(
-		"Not a valid commitment: {}",
-		id
-	)))?;
+	let c = util::from_hex(id)
+		.map_err(|_| ErrorKind::Argument(format!("Not a valid commitment: {}", id)))?;
 	let commit = Commitment::from_vec(c);
 
 	// We need the features here to be able to generate the necessary hash

api/src/types.rs

@@ -246,7 +246,7 @@ impl<'de> serde::de::Visitor<'de> for PrintableCommitmentVisitor {
 	{
 		Ok(PrintableCommitment {
 			commit: pedersen::Commitment::from_vec(
-				util::from_hex(String::from(v)).map_err(serde::de::Error::custom)?,
+				util::from_hex(v).map_err(serde::de::Error::custom)?,
 			),
 		})
 	}
@@ -337,7 +337,7 @@ impl OutputPrintable {
 			.clone()
 			.ok_or_else(|| ser::Error::HexError("output range_proof missing".to_string()))?;
 
-		let p_vec = util::from_hex(proof_str)
+		let p_vec = util::from_hex(&proof_str)
 			.map_err(|_| ser::Error::HexError("invalid output range_proof".to_string()))?;
 		let mut p_bytes = [0; util::secp::constants::MAX_PROOF_SIZE];
 		for i in 0..p_bytes.len() {
@@ -421,8 +421,7 @@ impl<'de> serde::de::Deserialize<'de> for OutputPrintable {
 							no_dup!(commit);
 
 							let val: String = map.next_value()?;
-							let vec =
-								util::from_hex(val.clone()).map_err(serde::de::Error::custom)?;
+							let vec = util::from_hex(&val).map_err(serde::de::Error::custom)?;
 							commit = Some(pedersen::Commitment::from_vec(vec));
 						}
 						Field::Spent => {

core/src/core/block.rs

@@ -363,7 +363,7 @@ impl BlockHeader {
 		proof: Proof,
 	) -> Result<Self, Error> {
 		// Convert hex pre pow string
-		let mut header_bytes = from_hex(pre_pow)
+		let mut header_bytes = from_hex(&pre_pow)
 			.map_err(|e| Error::Serialization(ser::Error::HexError(e.to_string())))?;
 		// Serialize and append serialized nonce and proof
 		serialize_default(&mut header_bytes, &nonce)?;

core/src/core/hash.rs

@@ -80,7 +80,7 @@ impl Hash {
 
 	/// Convert hex string back to hash.
 	pub fn from_hex(hex: &str) -> Result<Hash, Error> {
-		let bytes = util::from_hex(hex.to_string())
+		let bytes = util::from_hex(hex)
 			.map_err(|_| Error::HexError(format!("failed to decode {}", hex)))?;
 		Ok(Hash::from_vec(&bytes))
 	}

core/src/core/id.rs

@@ -115,7 +115,7 @@ impl ShortId {
 
 	/// Reconstructs a switch commit hash from a hex string.
 	pub fn from_hex(hex: &str) -> Result<ShortId, ser::Error> {
-		let bytes = util::from_hex(hex.to_string())
+		let bytes = util::from_hex(hex)
 			.map_err(|_| ser::Error::HexError("short_id from_hex error".to_string()))?;
 		Ok(ShortId::from_bytes(&bytes))
 	}

core/src/core/merkle_proof.rs

@@ -84,7 +84,7 @@ impl MerkleProof {
 
 	/// Convert hex string representation back to a Merkle proof instance
 	pub fn from_hex(hex: &str) -> Result<MerkleProof, String> {
-		let bytes = util::from_hex(hex.to_string()).unwrap();
+		let bytes = util::from_hex(hex).unwrap();
 		let res = ser::deserialize_default(&mut &bytes[..])
 			.map_err(|_| "failed to deserialize a Merkle Proof".to_string())?;
 		Ok(res)

core/src/genesis.rs

@@ -92,10 +92,8 @@ pub fn genesis_floo() -> core::Block {
 	let kernel = core::TxKernel {
 		features: core::KernelFeatures::Coinbase,
 		excess: Commitment::from_vec(
-			util::from_hex(
-				"08df2f1d996cee37715d9ac0a0f3b13aae508d1101945acb8044954aee30960be9".to_string(),
-			)
-			.unwrap(),
+			util::from_hex("08df2f1d996cee37715d9ac0a0f3b13aae508d1101945acb8044954aee30960be9")
+				.unwrap(),
 		),
 		excess_sig: Signature::from_raw_data(&[
 			25, 176, 52, 246, 172, 1, 12, 220, 247, 111, 73, 101, 13, 16, 157, 130, 110, 196, 123,
@@ -108,10 +106,8 @@ pub fn genesis_floo() -> core::Block {
 	let output = core::Output {
 		features: core::OutputFeatures::Coinbase,
 		commit: Commitment::from_vec(
-			util::from_hex(
-				"08c12007af16d1ee55fffe92cef808c77e318dae70c3bc70cb6361f49d517f1b68".to_string(),
-			)
-			.unwrap(),
+			util::from_hex("08c12007af16d1ee55fffe92cef808c77e318dae70c3bc70cb6361f49d517f1b68")
+				.unwrap(),
 		),
 		proof: RangeProof {
 			plen: SINGLE_BULLET_PROOF_SIZE,
@@ -208,10 +204,8 @@ pub fn genesis_main() -> core::Block {
 	let kernel = core::TxKernel {
 		features: core::KernelFeatures::Coinbase,
 		excess: Commitment::from_vec(
-			util::from_hex(
-				"096385d86c5cfda718aa0b7295be0adf7e5ac051edfe130593a2a257f09f78a3b1".to_string(),
-			)
-			.unwrap(),
+			util::from_hex("096385d86c5cfda718aa0b7295be0adf7e5ac051edfe130593a2a257f09f78a3b1")
+				.unwrap(),
 		),
 		excess_sig: Signature::from_raw_data(&[
 			80, 208, 41, 171, 28, 224, 250, 121, 60, 192, 213, 232, 111, 199, 111, 105, 18, 22, 54,
@@ -224,10 +218,8 @@ pub fn genesis_main() -> core::Block {
 	let output = core::Output {
 		features: core::OutputFeatures::Coinbase,
 		commit: Commitment::from_vec(
-			util::from_hex(
-				"08b7e57c448db5ef25aa119dde2312c64d7ff1b890c416c6dda5ec73cbfed2edea".to_string(),
-			)
-			.unwrap(),
+			util::from_hex("08b7e57c448db5ef25aa119dde2312c64d7ff1b890c416c6dda5ec73cbfed2edea")
+				.unwrap(),
 		),
 		proof: RangeProof {
 			plen: SINGLE_BULLET_PROOF_SIZE,

core/src/libtx/secp_ser.rs

@@ -44,7 +44,7 @@ pub mod pubkey_serde {
 		let static_secp = static_secp_instance();
 		let static_secp = static_secp.lock();
 		String::deserialize(deserializer)
-			.and_then(|string| from_hex(string).map_err(|err| Error::custom(err.to_string())))
+			.and_then(|string| from_hex(&string).map_err(|err| Error::custom(err.to_string())))
 			.and_then(|bytes: Vec<u8>| {
 				PublicKey::from_slice(&static_secp, &bytes)
 					.map_err(|err| Error::custom(err.to_string()))
@@ -81,7 +81,7 @@ pub mod option_sig_serde {
 		let static_secp = static_secp_instance();
 		let static_secp = static_secp.lock();
 		Option::<String>::deserialize(deserializer).and_then(|res| match res {
-			Some(string) => from_hex(string)
+			Some(string) => from_hex(&string)
 				.map_err(|err| Error::custom(err.to_string()))
 				.and_then(|bytes: Vec<u8>| {
 					let mut b = [0u8; 64];
@@ -123,7 +123,7 @@ pub mod option_seckey_serde {
 		let static_secp = static_secp_instance();
 		let static_secp = static_secp.lock();
 		Option::<String>::deserialize(deserializer).and_then(|res| match res {
-			Some(string) => from_hex(string)
+			Some(string) => from_hex(&string)
 				.map_err(|err| Error::custom(err.to_string()))
 				.and_then(|bytes: Vec<u8>| {
 					let mut b = [0u8; 32];
@@ -161,7 +161,7 @@ pub mod sig_serde {
 		let static_secp = static_secp_instance();
 		let static_secp = static_secp.lock();
 		String::deserialize(deserializer)
-			.and_then(|string| from_hex(string).map_err(|err| Error::custom(err.to_string())))
+			.and_then(|string| from_hex(&string).map_err(|err| Error::custom(err.to_string())))
 			.and_then(|bytes: Vec<u8>| {
 				let mut b = [0u8; 64];
 				b.copy_from_slice(&bytes[0..64]);
@@ -195,7 +195,7 @@ pub mod option_commitment_serde {
 		D: Deserializer<'de>,
 	{
 		Option::<String>::deserialize(deserializer).and_then(|res| match res {
-			Some(string) => from_hex(string)
+			Some(string) => from_hex(&string)
 				.map_err(|err| Error::custom(err.to_string()))
 				.and_then(|bytes: Vec<u8>| Ok(Some(Commitment::from_vec(bytes.to_vec())))),
 			None => Ok(None),
@@ -221,7 +221,7 @@ where
 	use serde::de::{Error, IntoDeserializer};
 
 	let val = String::deserialize(deserializer)
-		.and_then(|string| from_hex(string).map_err(|err| Error::custom(err.to_string())))?;
+		.and_then(|string| from_hex(&string).map_err(|err| Error::custom(err.to_string())))?;
 	RangeProof::deserialize(val.into_deserializer())
 }
 
@@ -232,7 +232,7 @@ where
 {
 	use serde::de::Error;
 	String::deserialize(deserializer)
-		.and_then(|string| from_hex(string).map_err(|err| Error::custom(err.to_string())))
+		.and_then(|string| from_hex(&string).map_err(|err| Error::custom(err.to_string())))
 		.and_then(|bytes: Vec<u8>| Ok(Commitment::from_vec(bytes.to_vec())))
 }
 

keychain/src/base58.rs

@@ -388,7 +388,7 @@ mod tests {
 		assert_eq!(&_encode_slice(&[0, 0, 0, 0, 13, 36][..]), "1111211");
 
 		// Addresses
-		let addr = from_hex("00f8917303bfa8ef24f292e8fa1419b20460ba064d".to_owned()).unwrap();
+		let addr = from_hex("00f8917303bfa8ef24f292e8fa1419b20460ba064d").unwrap();
 		assert_eq!(
 			&check_encode_slice(&addr[..]),
 			"1PfJpZsjreyVrqeoAfabrRwwjQyoSQMmHH"
@@ -410,7 +410,7 @@ mod tests {
 		// Addresses
 		assert_eq!(
 			from_check("1PfJpZsjreyVrqeoAfabrRwwjQyoSQMmHH").ok(),
-			Some(from_hex("00f8917303bfa8ef24f292e8fa1419b20460ba064d".to_owned()).unwrap())
+			Some(from_hex("00f8917303bfa8ef24f292e8fa1419b20460ba064d").unwrap())
 		)
 	}
 

keychain/src/extkey_bip32.rs

@@ -790,7 +790,7 @@ mod tests {
 	#[test]
 	fn test_vector_1() {
 		let secp = Secp256k1::new();
-		let seed = from_hex("000102030405060708090a0b0c0d0e0f".to_owned()).unwrap();
+		let seed = from_hex("000102030405060708090a0b0c0d0e0f").unwrap();
 
 		// m
 		test_path(&secp, &seed, &[],
@@ -826,7 +826,7 @@ mod tests {
 	#[test]
 	fn test_vector_2() {
 		let secp = Secp256k1::new();
-		let seed = from_hex("fffcf9f6f3f0edeae7e4e1dedbd8d5d2cfccc9c6c3c0bdbab7b4b1aeaba8a5a29f9c999693908d8a8784817e7b7875726f6c696663605d5a5754514e4b484542".to_owned()).unwrap();
+		let seed = from_hex("fffcf9f6f3f0edeae7e4e1dedbd8d5d2cfccc9c6c3c0bdbab7b4b1aeaba8a5a29f9c999693908d8a8784817e7b7875726f6c696663605d5a5754514e4b484542").unwrap();
 
 		// m
 		test_path(&secp, &seed, &[],
@@ -862,7 +862,7 @@ mod tests {
 	#[test]
 	fn test_vector_3() {
 		let secp = Secp256k1::new();
-		let seed = from_hex("4b381541583be4423346c643850da4b320e46a87ae3d2a4e6da11eba819cd4acba45d239319ac14f863b8d5ab5a0d0c64d2e8a1e7d1457df2e5a3c51c73235be".to_owned()).unwrap();
+		let seed = from_hex("4b381541583be4423346c643850da4b320e46a87ae3d2a4e6da11eba819cd4acba45d239319ac14f863b8d5ab5a0d0c64d2e8a1e7d1457df2e5a3c51c73235be").unwrap();
 
 		// m
 		test_path(&secp, &seed, &[],

keychain/src/mnemonic.rs

@@ -316,10 +316,10 @@ mod tests {
 			);
 			assert_eq!(
 				to_entropy(t.mnemonic).unwrap().to_vec(),
-				from_hex(t.entropy.to_string()).unwrap()
+				from_hex(t.entropy).unwrap()
 			);
 			assert_eq!(
-				from_entropy(&from_hex(t.entropy.to_string()).unwrap()).unwrap(),
+				from_entropy(&from_hex(t.entropy).unwrap()).unwrap(),
 				t.mnemonic
 			);
 		}

keychain/src/types.rs

@@ -191,7 +191,7 @@ impl Identifier {
 	}
 
 	pub fn from_hex(hex: &str) -> Result<Identifier, Error> {
-		let bytes = util::from_hex(hex.to_string()).unwrap();
+		let bytes = util::from_hex(hex).unwrap();
 		Ok(Identifier::from_bytes(&bytes))
 	}
 
@@ -293,7 +293,7 @@ impl BlindingFactor {
 	}
 
 	pub fn from_hex(hex: &str) -> Result<BlindingFactor, Error> {
-		let bytes = util::from_hex(hex.to_string()).unwrap();
+		let bytes = util::from_hex(hex).unwrap();
 		Ok(BlindingFactor::from_slice(&bytes))
 	}
 

util/src/hex.rs

@@ -17,7 +17,6 @@
 /// provide easy hex encoding, hex is a bit in limbo right now in Rust-
 /// land. It's simple enough that we can just have our own.
 use std::fmt::Write;
-use std::num;
 
 /// Encode the provided bytes into a hex string
 pub fn to_hex(bytes: Vec<u8>) -> String {
@@ -29,29 +28,16 @@ pub fn to_hex(bytes: Vec<u8>) -> String {
 }
 
 /// Decode a hex string into bytes.
-pub fn from_hex(hex_str: String) -> Result<Vec<u8>, num::ParseIntError> {
-	if hex_str.len() % 2 == 1 {
-		// TODO: other way to instantiate a ParseIntError?
-		let err = ("QQQ").parse::<u64>();
-		if let Err(e) = err {
-			return Err(e);
-		}
-	}
-	let hex_trim = if &hex_str[..2] == "0x" {
-		hex_str[2..].to_owned()
+pub fn from_hex(hex: &str) -> Result<Vec<u8>, String> {
+	let hex = hex.trim().trim_start_matches("0x");
+	if hex.len() % 2 != 0 {
+		Err(hex.to_string())
 	} else {
-		hex_str
-	};
-	split_n(&hex_trim.trim()[..], 2)
-		.iter()
-		.map(|b| u8::from_str_radix(b, 16))
-		.collect::<Result<Vec<u8>, _>>()
-}
-
-fn split_n(s: &str, n: usize) -> Vec<&str> {
-	(0..=(s.len() - n + 1) / 2)
-		.map(|i| &s[2 * i..2 * i + n])
-		.collect()
+		(0..hex.len())
+			.step_by(2)
+			.map(|i| u8::from_str_radix(&hex[i..i + 2], 16).map_err(|_| hex.to_string()))
+			.collect()
+	}
 }
 
 #[cfg(test)]
@@ -67,14 +53,17 @@ mod test {
 
 	#[test]
 	fn test_from_hex() {
-		assert_eq!(from_hex("00000000".to_string()).unwrap(), vec![0, 0, 0, 0]);
+		assert_eq!(from_hex(""), Ok(vec![]));
+		assert_eq!(from_hex("00000000"), Ok(vec![0, 0, 0, 0]));
+		assert_eq!(from_hex("0a0b0c0d"), Ok(vec![10, 11, 12, 13]));
+		assert_eq!(from_hex("000000ff"), Ok(vec![0, 0, 0, 255]));
+		assert_eq!(from_hex("0x000000ff"), Ok(vec![0, 0, 0, 255]));
+		assert_eq!(from_hex("0x000000fF"), Ok(vec![0, 0, 0, 255]));
+		assert_eq!(from_hex("0x000000fg"), Err("000000fg".to_string()));
 		assert_eq!(
-			from_hex("0a0b0c0d".to_string()).unwrap(),
-			vec![10, 11, 12, 13]
-		);
-		assert_eq!(
-			from_hex("000000ff".to_string()).unwrap(),
-			vec![0, 0, 0, 255]
+			from_hex("not a hex string"),
+			Err("not a hex string".to_string())
 		);
+		assert_eq!(from_hex("0"), Err("0".to_string()));
 	}
 }