From aad663e1146fd703d79898e35781fe9d35a9e729 Mon Sep 17 00:00:00 2001 From: NikVolf Date: Thu, 26 Dec 2019 19:54:22 +0300 Subject: [PATCH] Improve subkey error reporting. --- Cargo.lock | 1 + bin/utils/subkey/Cargo.toml | 1 + bin/utils/subkey/src/main.rs | 164 +++++++++++++++++++-------------- bin/utils/subkey/src/vanity.rs | 2 +- 4 files changed, 100 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f951617658729..af59d9ece1b82 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6714,6 +6714,7 @@ name = "subkey" version = "2.0.0" dependencies = [ "clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)", + "derive_more 0.99.2 (registry+https://github.com/rust-lang/crates.io-index)", "frame-system 2.0.0", "hex 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "hex-literal 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/bin/utils/subkey/Cargo.toml b/bin/utils/subkey/Cargo.toml index 6d687ce9109f2..c042b76f4a820 100644 --- a/bin/utils/subkey/Cargo.toml +++ b/bin/utils/subkey/Cargo.toml @@ -22,6 +22,7 @@ pallet-balances = { version = "2.0.0", path = "../../../frame/balances" } pallet-transaction-payment = { version = "2.0.0", path = "../../../frame/transaction-payment" } rpassword = "4.0.1" itertools = "0.8.2" +derive_more = { version = "0.99.2" } [features] bench = [] diff --git a/bin/utils/subkey/src/main.rs b/bin/utils/subkey/src/main.rs index f10c949dbec25..53488322549ea 100644 --- a/bin/utils/subkey/src/main.rs +++ b/bin/utils/subkey/src/main.rs @@ -31,7 +31,7 @@ use sp_core::{ }; use sp_runtime::{traits::{IdentifyAccount, Verify}, generic::Era}; use std::{ - convert::{TryInto, TryFrom}, io::{stdin, Read}, str::FromStr, path::PathBuf, fs, + convert::{TryInto, TryFrom}, io::{stdin, Read}, str::FromStr, path::PathBuf, fs, fmt, }; mod vanity; @@ -234,12 +234,12 @@ fn get_app<'a, 'b>(usage: &'a str) -> App<'a, 'b> { ]) } -fn main() { +fn main() -> Result<(), Error> { let usage = get_usage(); let matches = get_app(&usage).get_matches(); if matches.is_present("ed25519") { - return execute::(matches) + return execute::(matches); } if matches.is_present("secp256k1") { return execute::(matches) @@ -270,7 +270,24 @@ fn get_uri(match_name: &str, matches: &ArgMatches) -> String { } } -fn execute(matches: ArgMatches) +#[derive(derive_more::Display, derive_more::From)] +enum Error { + Static(&'static str), + Io(std::io::Error), + Formatted(String), +} + +impl fmt::Debug for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self, f) + } +} + +fn static_err(msg: &'static str) -> Result<(), Error> { + Err(Error::Static(msg)) +} + +fn execute(matches: ArgMatches) -> Result<(), Error> where SignatureOf: SignatureT, PublicOf: PublicT, @@ -279,28 +296,32 @@ where let password = matches.value_of("password"); let password = if password.is_some() && password_interactive { - panic!("`--password` given and `--password-interactive` selected!"); + return static_err("`--password` given and `--password-interactive` selected!"); } else if password_interactive { Some( - rpassword::read_password_from_tty(Some("Key password: ")) - .expect("Reads password from tty") + rpassword::read_password_from_tty(Some("Key password: "))? ) } else { password.map(Into::into) }; let password = password.as_ref().map(String::as_str); - let maybe_network: Option = matches.value_of("network").map(|network| { + let maybe_network: Option = match matches.value_of("network").map(|network| { network .try_into() - .expect("Invalid network name. See --help for available networks.") - }); + .map_err(|_| Error::Static("Invalid network name. See --help for available networks.")) + }) { + Some(Err(e)) => return Err(e), + Some(Ok(v)) => Some(v), + None => None, + }; + if let Some(network) = maybe_network { set_default_ss58_version(network); } match matches.subcommand() { ("generate", Some(matches)) => { - let mnemonic = generate_mnemonic(matches); + let mnemonic = generate_mnemonic(matches)?; C::print_from_uri(mnemonic.phrase(), password, maybe_network); } ("inspect", Some(matches)) => { @@ -309,19 +330,21 @@ where ("sign", Some(matches)) => { let suri = get_uri("suri", &matches); let should_decode = matches.is_present("hex"); - let message = read_message_from_stdin(should_decode); - let signature = do_sign::(&suri, message, password); + + let message = read_message_from_stdin(should_decode)?; + let signature = do_sign::(&suri, message, password)?; println!("{}", signature); } ("verify", Some(matches)) => { let uri = get_uri("uri", &matches); let should_decode = matches.is_present("hex"); - let message = read_message_from_stdin(should_decode); - let is_valid_signature = do_verify::(matches, &uri, message); + + let message = read_message_from_stdin(should_decode)?; + let is_valid_signature = do_verify::(matches, &uri, message)?; if is_valid_signature { println!("Signature verifies correctly."); } else { - println!("Signature invalid."); + return static_err("Signature invalid."); } } ("vanity", Some(matches)) => { @@ -329,17 +352,17 @@ where .value_of("pattern") .map(str::to_string) .unwrap_or_default(); - let result = vanity::generate_key::(&desired).expect("Key generation failed"); + let result = vanity::generate_key::(&desired)?; let formated_seed = format_seed::(result.seed); C::print_from_uri(&formated_seed, None, maybe_network); } ("transfer", Some(matches)) => { - let signer = read_pair::(matches.value_of("from"), password); - let index = read_required_parameter::(matches, "index"); - let genesis_hash = read_genesis_hash(matches); + let signer = read_pair::(matches.value_of("from"), password)?; + let index = read_required_parameter::(matches, "index")?; + let genesis_hash = read_genesis_hash(matches)?; let to: AccountId = read_account_id(matches.value_of("to")); - let amount = read_required_parameter::(matches, "amount"); + let amount = read_required_parameter::(matches, "amount")?; let function = Call::Balances(BalancesCall::transfer(to.into(), amount)); let extrinsic = create_extrinsic::(function, index, signer, genesis_hash); @@ -347,9 +370,9 @@ where print_extrinsic(extrinsic); } ("sign-transaction", Some(matches)) => { - let signer = read_pair::(matches.value_of("suri"), password); - let index = read_required_parameter::(matches, "nonce"); - let genesis_hash = read_genesis_hash(matches); + let signer = read_pair::(matches.value_of("suri"), password)?; + let index = read_required_parameter::(matches, "nonce")?; + let genesis_hash = read_genesis_hash(matches)?; let call = matches.value_of("call").expect("call is required; qed"); let function: Call = hex::decode(&call) @@ -363,81 +386,86 @@ where } _ => print_usage(&matches), } + + Ok(()) } /// Creates a new randomly generated mnemonic phrase. -fn generate_mnemonic(matches: &ArgMatches) -> Mnemonic { - let words = matches - .value_of("words") - .map(|x| usize::from_str(x).expect("Invalid number given for --words")) - .map(|x| { - MnemonicType::for_word_count(x) - .expect("Invalid number of words given for phrase: must be 12/15/18/21/24") - }) - .unwrap_or(MnemonicType::Words12); - Mnemonic::new(words, Language::English) -} - -fn do_sign(suri: &str, message: Vec, password: Option<&str>) -> String +fn generate_mnemonic(matches: &ArgMatches) -> Result { + let words = match matches.value_of("words") { + Some(words) => { + let num = usize::from_str(words).map_err(|_| Error::Static("Invalid number given for --words"))?; + MnemonicType::for_word_count(num) + .map_err(|_| Error::Static("Invalid number of words given for phrase: must be 12/15/18/21/24"))? + }, + None => MnemonicType::Words12, + }; + Ok(Mnemonic::new(words, Language::English)) +} + +fn do_sign(suri: &str, message: Vec, password: Option<&str>) -> Result where SignatureOf: SignatureT, PublicOf: PublicT, { - let pair = read_pair::(Some(suri), password); + let pair = read_pair::(Some(suri), password)?; let signature = pair.sign(&message); - format_signature::(&signature) + Ok(format_signature::(&signature)) } -fn do_verify(matches: &ArgMatches, uri: &str, message: Vec) -> bool +fn do_verify(matches: &ArgMatches, uri: &str, message: Vec) -> Result where SignatureOf: SignatureT, PublicOf: PublicT, { - let signature = read_signature::(matches); + + let signature = read_signature::(matches)?; let pubkey = read_public_key::(Some(uri)); - <::Pair as Pair>::verify(&signature, &message, &pubkey) + Ok(<::Pair as Pair>::verify(&signature, &message, &pubkey)) +} + +fn decode_hex>(message: T) -> Result, Error> { + hex::decode(message).map_err(|e| Error::Formatted(format!("Invalid hex ({})", e))) } -fn read_message_from_stdin(should_decode: bool) -> Vec { +fn read_message_from_stdin(should_decode: bool) -> Result, Error> { let mut message = vec![]; stdin() .lock() - .read_to_end(&mut message) - .expect("Error reading from stdin"); + .read_to_end(&mut message)?; if should_decode { - message = hex::decode(&message).expect("Invalid hex in message"); + message = decode_hex(&message)?; } - message + Ok(message) } -fn read_required_parameter(matches: &ArgMatches, name: &str) -> T where +fn read_required_parameter(matches: &ArgMatches, name: &str) -> Result where ::Err: std::fmt::Debug, { let str_value = matches .value_of(name) .expect("parameter is required; thus it can't be None; qed"); - str::parse::(str_value).unwrap_or_else(|_| - panic!("Invalid `{}' parameter; expecting an integer.", name) + str::parse::(str_value).map_err(|_| + Error::Formatted(format!("Invalid `{}' parameter; expecting an integer.", name)) ) } -fn read_genesis_hash(matches: &ArgMatches) -> H256 { +fn read_genesis_hash(matches: &ArgMatches) -> Result { let genesis_hash: Hash = match matches.value_of("genesis").unwrap_or("alex") { "elm" => hex!["10c08714a10c7da78f40a60f6f732cf0dba97acfb5e2035445b032386157d5c3"].into(), "alex" => hex!["dcd1346701ca8396496e52aa2785b1748deb6db09551b72159dcb3e08991025b"].into(), - h => hex::decode(h) - .ok() - .and_then(|x| Decode::decode(&mut &x[..]).ok()) + h => Decode::decode(&mut &decode_hex(h)?[..]) .expect("Invalid genesis hash or unrecognised chain identifier"), }; println!( "Using a genesis hash of {}", HexDisplay::from(&genesis_hash.as_ref()) ); - genesis_hash + Ok(genesis_hash) } -fn read_signature(matches: &ArgMatches) -> SignatureOf where +fn read_signature(matches: &ArgMatches) -> Result, Error> +where SignatureOf: SignatureT, PublicOf: PublicT, { @@ -445,19 +473,20 @@ fn read_signature(matches: &ArgMatches) -> SignatureOf where .value_of("sig") .expect("signature parameter is required; thus it can't be None; qed"); let mut signature = <::Pair as Pair>::Signature::default(); - let sig_data = hex::decode(sig_data).expect("signature is invalid hex"); + let sig_data = decode_hex(sig_data)?; if sig_data.len() != signature.as_ref().len() { - panic!( + return Err(Error::Formatted(format!( "signature has an invalid length. read {} bytes, expected {} bytes", sig_data.len(), signature.as_ref().len(), - ); + ))); } signature.as_mut().copy_from_slice(&sig_data); - signature + Ok(signature) } -fn read_public_key(matched_uri: Option<&str>) -> PublicOf where +fn read_public_key(matched_uri: Option<&str>) -> PublicOf +where PublicOf: PublicT, { let uri = matched_uri.expect("parameter is required; thus it can't be None; qed"); @@ -494,12 +523,12 @@ fn read_account_id(matched_uri: Option<&str>) -> AccountId { fn read_pair( matched_suri: Option<&str>, password: Option<&str>, -) -> ::Pair where +) -> Result<::Pair, Error> where SignatureOf: SignatureT, PublicOf: PublicT, { - let suri = matched_suri.expect("parameter is required; thus it can't be None; qed"); - C::pair_from_suri(suri, password) + let suri = matched_suri.ok_or(Error::Static("parameter is required; thus it can't be None; qed"))?; + Ok(C::pair_from_suri(suri, password)) } fn format_signature(signature: &SignatureOf) -> String { @@ -591,7 +620,7 @@ mod tests { let matches = app.clone().get_matches_from(arg_vec); let matches = matches.subcommand().1.unwrap(); - let mnemonic = generate_mnemonic(matches); + let mnemonic = generate_mnemonic(matches).expect("generate failed"); let (pair, seed) = <::Pair as Pair>::from_phrase(mnemonic.phrase(), password) @@ -601,14 +630,15 @@ mod tests { let seed = format_seed::(seed); let message = "Blah Blah\n".as_bytes().to_vec(); - let signature = do_sign::(&seed, message.clone(), password); + let signature = do_sign::(&seed, message.clone(), password).expect("signing failed"); // Verify the previous signature. let arg_vec = vec!["subkey", "verify", &signature[..], &public_key[..]]; let matches = get_app(&usage).get_matches_from(arg_vec); let matches = matches.subcommand().1.unwrap(); - assert!(do_verify::(matches, &public_key, message)); + + assert!(do_verify::(matches, &public_key, message).expect("verify failed")); } #[test] diff --git a/bin/utils/subkey/src/vanity.rs b/bin/utils/subkey/src/vanity.rs index 33e8559b1fbc4..ee5a2f2cce516 100644 --- a/bin/utils/subkey/src/vanity.rs +++ b/bin/utils/subkey/src/vanity.rs @@ -62,7 +62,7 @@ fn calculate_score(_desired: &str, key: &str) -> usize { 0 } -pub(super) fn generate_key(desired: &str) -> Result, &str> where +pub(super) fn generate_key(desired: &str) -> Result, &'static str> where PublicOf: PublicT, { if desired.is_empty() {