From 8ca381a9c2be3c2d332d41e33f973928d23b8113 Mon Sep 17 00:00:00 2001 From: Antioch Peverell Date: Tue, 10 Mar 2020 10:36:18 +0000 Subject: [PATCH] cleanup util::from_hex() (#3265) * cleanup our from_hex() * fix keychain tests * add test coverage for empty hex string --- api/src/foreign_rpc.rs | 4 +-- api/src/handlers/blocks_api.rs | 4 +-- api/src/handlers/chain_api.rs | 6 ++-- api/src/handlers/pool_api.rs | 2 +- api/src/handlers/transactions_api.rs | 6 ++-- api/src/handlers/utils.rs | 7 ++-- api/src/types.rs | 7 ++-- core/src/core/block.rs | 2 +- core/src/core/hash.rs | 2 +- core/src/core/id.rs | 2 +- core/src/core/merkle_proof.rs | 2 +- core/src/genesis.rs | 24 +++++--------- core/src/libtx/secp_ser.rs | 14 ++++---- keychain/src/base58.rs | 4 +-- keychain/src/extkey_bip32.rs | 6 ++-- keychain/src/mnemonic.rs | 4 +-- keychain/src/types.rs | 4 +-- util/src/hex.rs | 49 +++++++++++----------------- 18 files changed, 62 insertions(+), 87 deletions(-) diff --git a/api/src/foreign_rpc.rs b/api/src/foreign_rpc.rs index d1d057079..8020df941 100644 --- a/api/src/foreign_rpc.rs +++ b/api/src/foreign_rpc.rs @@ -747,7 +747,7 @@ impl ForeignRpc for Foreign { ) -> Result { let mut parsed_hash: Option = 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 { let mut parsed_hash: Option = 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)); } diff --git a/api/src/handlers/blocks_api.rs b/api/src/handlers/blocks_api.rs index 58528043a..aad4271d9 100644 --- a/api/src/handlers/blocks_api.rs +++ b/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)) } diff --git a/api/src/handlers/chain_api.rs b/api/src/handlers/chain_api.rs index 75abd2788..33b434d16 100644 --- a/api/src/handlers/chain_api.rs +++ b/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, max_height: Option, ) -> Result { - 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()); diff --git a/api/src/handlers/pool_api.rs b/api/src/handlers/pool_api.rs index db6757e6e..a869b3dc2 100644 --- a/api/src/handlers/pool_api.rs +++ b/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. diff --git a/api/src/handlers/transactions_api.rs b/api/src/handlers/transactions_api.rs index 585f22fa6..3df09f44c 100644 --- a/api/src/handlers/transactions_api.rs +++ b/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 { - 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)?; diff --git a/api/src/handlers/utils.rs b/api/src/handlers/utils.rs index 24097883e..ba0e78574 100644 --- a/api/src/handlers/utils.rs +++ b/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, id: &str, ) -> Result, 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 diff --git a/api/src/types.rs b/api/src/types.rs index 027d60ff7..65d2699d6 100644 --- a/api/src/types.rs +++ b/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 => { diff --git a/core/src/core/block.rs b/core/src/core/block.rs index fe1274e7e..adfee7898 100644 --- a/core/src/core/block.rs +++ b/core/src/core/block.rs @@ -363,7 +363,7 @@ impl BlockHeader { proof: Proof, ) -> Result { // 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)?; diff --git a/core/src/core/hash.rs b/core/src/core/hash.rs index 0b82769ab..fd886c407 100644 --- a/core/src/core/hash.rs +++ b/core/src/core/hash.rs @@ -80,7 +80,7 @@ impl Hash { /// Convert hex string back to hash. pub fn from_hex(hex: &str) -> Result { - 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)) } diff --git a/core/src/core/id.rs b/core/src/core/id.rs index 1e478a26b..1bc827530 100644 --- a/core/src/core/id.rs +++ b/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 { - 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)) } diff --git a/core/src/core/merkle_proof.rs b/core/src/core/merkle_proof.rs index 3cc8af03a..61d32852a 100644 --- a/core/src/core/merkle_proof.rs +++ b/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 { - 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) diff --git a/core/src/genesis.rs b/core/src/genesis.rs index 6288a6305..3101f0c83 100644 --- a/core/src/genesis.rs +++ b/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, diff --git a/core/src/libtx/secp_ser.rs b/core/src/libtx/secp_ser.rs index a29c21303..c5ce075d8 100644 --- a/core/src/libtx/secp_ser.rs +++ b/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| { 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::::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| { 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::::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| { 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| { 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::::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| 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| Ok(Commitment::from_vec(bytes.to_vec()))) } diff --git a/keychain/src/base58.rs b/keychain/src/base58.rs index ebe2d77a9..6429fd86b 100644 --- a/keychain/src/base58.rs +++ b/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()) ) } diff --git a/keychain/src/extkey_bip32.rs b/keychain/src/extkey_bip32.rs index d3412b30f..1db28f72a 100644 --- a/keychain/src/extkey_bip32.rs +++ b/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, &[], diff --git a/keychain/src/mnemonic.rs b/keychain/src/mnemonic.rs index bb9fe4bb8..5cf1b5d3e 100644 --- a/keychain/src/mnemonic.rs +++ b/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 ); } diff --git a/keychain/src/types.rs b/keychain/src/types.rs index a22ccfd51..8a5f20a71 100644 --- a/keychain/src/types.rs +++ b/keychain/src/types.rs @@ -191,7 +191,7 @@ impl Identifier { } pub fn from_hex(hex: &str) -> Result { - 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 { - let bytes = util::from_hex(hex.to_string()).unwrap(); + let bytes = util::from_hex(hex).unwrap(); Ok(BlindingFactor::from_slice(&bytes)) } diff --git a/util/src/hex.rs b/util/src/hex.rs index 615a0a6af..0e76086ea 100644 --- a/util/src/hex.rs +++ b/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) -> String { @@ -29,29 +28,16 @@ pub fn to_hex(bytes: Vec) -> String { } /// Decode a hex string into bytes. -pub fn from_hex(hex_str: String) -> Result, num::ParseIntError> { - if hex_str.len() % 2 == 1 { - // TODO: other way to instantiate a ParseIntError? - let err = ("QQQ").parse::(); - 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, 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::, _>>() -} - -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())); } }