* Use try!/map_err instead of match

* Use jsonrpc_core for serializing/deserializing rpc messages
* Factor out unwraps
* Remove mem::replace
* Add nicer formating of ConfirmationRequests

parity/configuration.rs

@@ -43,6 +43,8 @@ use presale::ImportWallet;
 use account::{AccountCmd, NewAccount, ImportAccounts, ImportFromGethAccounts};
 use snapshot::{self, SnapshotCommand};
 
+const AUTHCODE_FILENAME: &'static str = "authcodes";
+
 #[derive(Debug, PartialEq)]
 pub enum Cmd {
 	Run(RunCmd),
@@ -62,7 +64,7 @@ pub enum Cmd {
 		authfile: PathBuf
 	},
 	SignerReject {
-		id: usize,
+		id: Option<usize>,
 		port: u16,
 		authfile: PathBuf
 	},
@@ -124,15 +126,14 @@ impl Configuration {
 =======
 		} else if self.args.cmd_signer {
 			let mut authfile = PathBuf::from(signer_conf.signer_path);
-			authfile.push("authcodes");
+			authfile.push(AUTHCODE_FILENAME);
 
 			if self.args.cmd_new_token {
 				Cmd::SignerToken(dirs.signer)
 			} else if self.args.cmd_sign {
-				let pwfile = match self.args.flag_password.get(0) {
+				let pwfile = self.args.flag_password.get(0).map(|pwfile| {
-					Some(pwfile) => Some(PathBuf::from(pwfile)),
+					PathBuf::from(pwfile)
-					None => None,
+				});
-				};
 				Cmd::SignerSign {
 					id: self.args.arg_id,
 					pwfile: pwfile,
@@ -141,8 +142,7 @@ impl Configuration {
 				}
 			} else if self.args.cmd_reject  {
 				Cmd::SignerReject {
-					// id is a required field for this command
+					id: self.args.arg_id,
-					id: self.args.arg_id.unwrap(),
 					port: signer_conf.port,
 					authfile: authfile,
 				}

rpc/src/v1/types/confirmations.rs

@@ -41,16 +41,16 @@ impl From<helpers::ConfirmationRequest> for ConfirmationRequest {
 
 impl fmt::Display for ConfirmationRequest {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-		write!(f, "id: {:?}, {}", self.id, self.payload)
+		write!(f, "#{}: {}", self.id, self.payload)
 	}
 }
 
 impl fmt::Display for ConfirmationPayload {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-		match self {
+		match *self {
-			&ConfirmationPayload::Transaction(ref transaction)
+			ConfirmationPayload::Transaction(ref transaction)
 				=> write!(f, "{}", transaction),
-			&ConfirmationPayload::Sign(_) => write!(f, "TODO: data"),
+			ConfirmationPayload::Sign(_) => write!(f, "TODO: data"),
 		}
 	}
 }

rpc/src/v1/types/transaction_request.rs

@@ -44,10 +44,16 @@ pub struct TransactionRequest {
 
 impl fmt::Display for TransactionRequest {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-		write!(f, "{:?} from {:?} to {:?}",
+		let eth = self.value.unwrap_or(U256::from(0));
-			   self.value.unwrap_or(U256::from(0)),
+		match self.to {
-			   self.from,
+			Some(ref to) => write!(f, "{} Ether from {:?} to {:?}",
-			   self.to)
+							   eth.format_ether(),
+							   self.from,
+							   to),
+			None => write!(f, "{} Ether from {:?}",
+							   eth.format_ether(),
+							   self.from),
+		}
 	}
 }
 

rpc/src/v1/types/uint.rs

@@ -48,6 +48,25 @@ macro_rules! impl_uint {
 			}
 		}
 
+		impl $name {
+			/// Human readable formatting
+			pub fn format_ether(&self) -> String {
+				let divisor = $other::from(10u64.pow(18));
+				let ether = self.0 / divisor;
+				let rest = self.0 - ether * divisor;
+				let string = format!("{}.{}", ether, rest);
+				string.trim_right_matches('0')
+					.trim_right_matches('.')
+					.to_string()
+			}
+		}
+
+		impl fmt::Display for $name {
+			fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+				write!(f, "{}", self.0)
+			}
+		}
+
 		impl fmt::LowerHex for $name {
 			fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 				write!(f, "{:#x}", self.0)

rpc_cli/src/lib.rs

@@ -15,7 +15,7 @@ use std::fs::File;
 
 use futures::Future;
 
-fn sign_interactive(signer: &mut SignerRpc, pwd: &String, request: ConfirmationRequest)
+fn sign_interactive(signer: &mut SignerRpc, password: &str, request: ConfirmationRequest)
 {
 	print!("\n{}\nSign this transaction? (y)es/(N)o/(r)eject: ", request);
 	let _ = stdout().flush();
@@ -23,7 +23,7 @@ fn sign_interactive(signer: &mut SignerRpc, pwd: &String, request: ConfirmationR
 		Some(Ok(line)) => {
 			match line.to_lowercase().chars().nth(0) {
 				Some('y') => {
-					match sign_transaction(signer, request.id, pwd) {
+					match sign_transaction(signer, request.id, password) {
 						Ok(s) | Err(s) => println!("{}", s),
 					}
 				}
@@ -39,128 +39,129 @@ fn sign_interactive(signer: &mut SignerRpc, pwd: &String, request: ConfirmationR
 	}
 }
 
-fn sign_transactions(signer: &mut SignerRpc, pwd: String) -> Result<String, String> {
+fn sign_transactions(signer: &mut SignerRpc, password: String) -> Result<String, String> {
-	signer.requests_to_confirm().map(|reqs| {
+	try!(signer.requests_to_confirm().map(|reqs| {
 		match reqs {
+			Ok(ref reqs) if reqs.is_empty() => {
+				Ok("No transactions in signing queue".to_owned())
+			}
 			Ok(reqs) => {
-				if reqs.len() == 0 {
+				for r in reqs {
-					Ok("No transactions in signing queue".to_string())
+					sign_interactive(signer, &password, r)
-				} else {
-					for r in reqs {
-						sign_interactive(signer, &pwd, r)
-					}
-					Ok("".to_string())
 				}
+				Ok("".to_owned())
 			}
 			Err(err) => {
 				Err(format!("error: {:?}", err))
 			}
 		}
-	}).wait().unwrap()
+	}).map_err(|err| {
+		format!("{:?}", err)
+	}).wait())
 }
 
 fn list_transactions(signer: &mut SignerRpc) -> Result<String, String> {
-	signer.requests_to_confirm().map(|reqs| {
+	try!(signer.requests_to_confirm().map(|reqs| {
 		match reqs {
-			Ok(reqs) => {
+			Ok(ref reqs) if reqs.is_empty() => {
-				let mut s = "Transaction queue:".to_string();
+				Ok("No transactions in signing queue".to_owned())
-				if reqs.len() == 0 {
+			}
-					s = s + &"No transactions in signing queue";
+			Ok(ref reqs) => {
-				} else {
+				Ok(format!("Transaction queue:\n{}", reqs
-					for r in reqs {
+						   .iter()
-						s = s + &format!("\n{}", r);
+						   .map(|r| format!("{}", r))
-					}
+						   .collect::<Vec<String>>()
-				}
+						   .join("\n")))
-				Ok(s)
 			}
 			Err(err) => {
 				Err(format!("error: {:?}", err))
 			}
 		}
-	}).wait().unwrap()
+	}).map_err(|err| {
+		format!("{:?}", err)
+	}).wait())
 }
 
 fn sign_transaction(signer: &mut SignerRpc,
 					id: U256,
-					pwd: &String) -> Result<String, String> {
+					password: &str) -> Result<String, String> {
-	signer.confirm_request(id, None, &pwd).map(|res| {
+	try!(signer.confirm_request(id, None, password).map(|res| {
 		match res {
 			Ok(u) => Ok(format!("Signed transaction id: {:#x}", u)),
 			Err(e) => Err(format!("{:?}", e)),
 		}
-	}).wait().unwrap()
+	}).map_err(|err| {
+		format!("{:?}", err)
+	}).wait())
 }
 
 fn reject_transaction(signer: &mut SignerRpc,
 					  id: U256) -> Result<String, String> {
-	signer.reject_request(id).map(|res| {
+	try!(signer.reject_request(id).map(|res| {
 		match res {
 			Ok(true) => Ok(format!("Rejected transaction id {:#x}", id)),
 			Ok(false) => Err(format!("No such request")),
 			Err(e) => Err(format!("{:?}", e)),
 		}
-	}).wait().unwrap()
+	}).map_err(|err| {
+		format!("{:?}", err)
+	}).wait())
 }
 
 // cmds
 
 pub fn cmd_signer_list(signerport: u16,
 					   authfile: PathBuf) -> Result<String, String> {
-	match SignerRpc::new(&format!("ws://127.0.0.1:{}", signerport),
+	let mut signer = try!(SignerRpc::new(&format!("ws://127.0.0.1:{}", signerport), &authfile).map_err(|err| {
-						 &authfile) {
+		format!("{:?}", err)
-		Ok(mut signer) => {
+	}));
-			list_transactions(&mut signer)
+	list_transactions(&mut signer)
-		}
-		Err(e) => Err(format!("{:?}", e))
-	}
 }
 
-pub fn cmd_signer_reject(id: usize,
+pub fn cmd_signer_reject(id: Option<usize>, signerport: u16,
-						 signerport: u16,
 						 authfile: PathBuf) -> Result<String, String> {
-	match SignerRpc::new(&format!("ws://127.0.0.1:{}", signerport),
+	let id = try!(id.ok_or(format!("id required for signer reject")));
-						 &authfile) {
+	let mut signer = try!(SignerRpc::new(&format!("ws://127.0.0.1:{}", signerport), &authfile).map_err(|err| {
-		Ok(mut signer) => {
+		format!("{:?}", err)
-			reject_transaction(&mut signer, U256::from(id))
+	}));
-		},
+	reject_transaction(&mut signer, U256::from(id))
-		Err(e) => Err(format!("{:?}", e))
-	}
 }
 
 pub fn cmd_signer_sign(id: Option<usize>,
 					   pwfile: Option<PathBuf>,
 					   signerport: u16,
 					   authfile: PathBuf) -> Result<String, String> {
-	let pwd;
+	let password;
 	match pwfile {
 		Some(pwfile) => {
 			match File::open(pwfile) {
 				Ok(fd) => {
 					match BufReader::new(fd).lines().next() {
-						Some(Ok(line)) => pwd = line,
+						Some(Ok(line)) => password = line,
 						_ => return Err(format!("No password in file"))
 					}
 				},
-				Err(e) => return Err(format!("Could not open pwfile: {}", e))
+				Err(e) => return Err(format!("Could not open password file: {}", e))
 			}
 		}
 		None => {
-			pwd = rpassword::prompt_password_stdout("Password: ").unwrap();
+			password = match rpassword::prompt_password_stdout("Password: ") {
+				Ok(p) => p,
+				Err(e) => return Err(format!("{}", e)),
+			}
 		}
 	}
 
-	match SignerRpc::new(&format!("ws://127.0.0.1:{}", signerport),
+	let mut signer = try!(SignerRpc::new(&format!("ws://127.0.0.1:{}", signerport), &authfile).map_err(|err| {
-						 &authfile) {
+		format!("{:?}", err)
-		Ok(mut signer) => {
+	}));
-			match id {
+
-				Some(id) => {
+	match id {
-					sign_transaction(&mut signer, U256::from(id), &pwd)
+		Some(id) => {
-				},
+			sign_transaction(&mut signer, U256::from(id), &password)
-				None => {
+		},
-					sign_transactions(&mut signer, pwd)
+		None => {
-				}
+			sign_transactions(&mut signer, password)
-			}
 		}
-		Err(e) => return Err(format!("{:?}", e))
 	}
 }

rpc_client/Cargo.toml

@@ -8,6 +8,7 @@ version = "1.4.0"
 
 [dependencies]
 futures = "0.1"
+jsonrpc-core = "3.0.2"
 lazy_static = "0.2.1"
 matches = "0.1.2"
 rand = "0.3.14"
@@ -16,12 +17,6 @@ serde_json = "0.8"
 tempdir = "0.3.5"
 url = "1.2.0"
 ws = "0.5.3"
-
+ethcore-rpc = { path = "../rpc" }
-[dependencies.ethcore-rpc]
+ethcore-signer = { path = "../signer" }
-path = "../rpc"
+ethcore-util = { path = "../util" }
-
-[dependencies.ethcore-signer]
-path = "../signer"
-
-[dependencies.ethcore-util]
-path = "../util"

rpc_client/src/client.rs

@@ -4,7 +4,6 @@ use std::sync::{Arc, Mutex};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::collections::BTreeMap;
 use std::thread;
-use std::mem;
 use std::time;
 
 use std::path::PathBuf;
@@ -12,7 +11,7 @@ use util::Hashable;
 use url::Url;
 use std::fs::File;
 
-use ws::{connect,
+use ws::{self,
 		 Request,
 		 Handler,
 		 Sender,
@@ -25,12 +24,12 @@ use ws::{connect,
 use serde::Serialize;
 use serde::Deserialize;
 use serde::ser::Serializer;
-use serde_json::{from_str,
+use serde_json::{self as json,
-				 to_string,
-				 from_value,
 				 Value as JsonValue,
 				 Error as JsonError};
 
+//use jsonrpc_core::
+
 use futures::{BoxFuture, Canceled, Complete, Future, oneshot, done};
 
 /// The actual websocket connection handler, passed into the
@@ -65,9 +64,12 @@ impl Handler for RpcHandler {
     fn build_request(&mut self, url: &Url) -> WsResult<Request> {
 		match Request::from_url(url) {
 			Ok(mut r) => {
-				let timestamp = time::UNIX_EPOCH.elapsed().unwrap().as_secs();
+				let timestamp = try!(time::UNIX_EPOCH.elapsed().map_err(|err| {
-				let hashed = format!("{}:{}", self.auth_code, timestamp).sha3();
+					WsError::new(WsErrorKind::Internal, format!("{}", err))
-				let proto = format!("{:?}_{}", hashed, timestamp);
+				}));
+				let secs = timestamp.as_secs();
+				let hashed = format!("{}:{}", self.auth_code, secs).sha3();
+				let proto = format!("{:?}_{}", hashed, secs);
 				r.add_protocol(&proto);
 				Ok(r)
 			},
@@ -75,22 +77,25 @@ impl Handler for RpcHandler {
 		}
     }
     fn on_error(&mut self, err: WsError) {
-		match mem::replace(&mut self.complete, None) {
+		match self.complete.take() {
 			Some(c) => c.complete(Err(RpcError::WsError(err))),
-			None => println!("warning: unexpected error"),
+			None => println!("unexpected error: {}", err),
 		}
     }
     fn on_open(&mut self, _: Handshake) -> WsResult<()> {
-		match mem::replace(&mut self.complete, None) {
+		match (self.complete.take(), self.out.take()) {
-			Some(c) => c.complete(Ok(Rpc {
+			(Some(c), Some(out)) => {
-				out: mem::replace(&mut self.out, None).unwrap(),
+				c.complete(Ok(Rpc {
-				counter: AtomicUsize::new(0),
+					out: out,
-				pending: self.pending.clone(),
+					counter: AtomicUsize::new(0),
-			})),
+					pending: self.pending.clone(),
-			// Should not be reachable
+				}));
-			None => (),
+				Ok(())
+			},
+			_ => {
+				Err(WsError::new(WsErrorKind::Internal, format!("on_open called twice")))
+			}
 		}
-		Ok(())
 	}
     fn on_message(&mut self, msg: Message) -> WsResult<()> {
 		match parse_response(&msg.to_string()) {
@@ -153,16 +158,25 @@ impl Rpc {
 			Err(e) => return done(Ok(Err(e))).boxed(),
 			Ok(code) => {
 				let url = String::from(url);
+				// The ws::connect takes a FnMut closure,
+				// which means c cannot be moved into it,
+				// since it's consumed on complete.
+				// Therefore we wrap it in an option
+				// and pick it out once.
+				let mut once = Some(c);
 				thread::spawn(move || {
-					// mem:replace Option hack to move `c` out
+					let conn = ws::connect(url, |out| {
-					// of the FnMut closure
+						// this will panic if the closure
-					let mut swap = Some(c);
+						// is called twice, which it should never
-					match connect(url, |out| {
+						// be.
-						let c = mem::replace(&mut swap, None).unwrap();
+						let c = once.take().expect("connection closure called only once");
 						RpcHandler::new(out, code.clone(), c)
-					}) {
+					});
+					match conn {
 						Err(err) => {
-							let c = mem::replace(&mut swap, None).unwrap();
+							// since ws::connect is only called once, it cannot
+							// both fail and succeed.
+							let c = once.take().expect("connection closure called only once");
 							c.complete(Err(RpcError::WsError(err)));
 						},
 						// c will complete on the `on_open` event in the Handler
@@ -183,13 +197,13 @@ impl Rpc {
 		let id = self.counter.fetch_add(1, Ordering::Relaxed);
 		self.pending.insert(id, c);
 
-		let serialized = to_string(&RpcRequest::new(id, method, params)).unwrap();
+		let serialized = json::to_string(&RpcRequest::new(id, method, params)).unwrap();
 		let _ = self.out.send(serialized);
 
 		p.map(|result| {
 			match result {
 				Ok(json) => {
-					let t: T = try!(from_value(json));
+					let t: T = try!(json::from_value(json));
 					Ok(t)
 				},
 				Err(err) => Err(err)
@@ -281,7 +295,7 @@ impl From<Canceled> for RpcError {
 }
 
 fn parse_response(s: &str) -> (Option<usize>, Result<JsonValue, RpcError>) {
-	let mut json: JsonValue = match from_str(s) {
+	let mut json: JsonValue = match json::from_str(s) {
 		Err(e) => return (None, Err(RpcError::ParseError(e))),
 		Ok(json) => json,
 	};

rpc_client/src/lib.rs

@@ -12,6 +12,7 @@ extern crate serde;
 extern crate serde_json;
 extern crate rand;
 extern crate tempdir;
+extern crate jsonrpc_core;
 
 #[macro_use]
 extern crate lazy_static;
@@ -19,7 +20,7 @@ extern crate lazy_static;
 extern crate matches;
 
 #[cfg(test)]
-mod test {
+mod tests {
 	use futures::Future;
 	use std::path::PathBuf;
 
@@ -29,46 +30,40 @@ mod test {
 
 	#[test]
 	fn test_connection_refused() {
-		let (srv, port, tmpdir, _) = serve();
+		let (_srv, port, tmpdir, _) = serve();
 
 		let mut path = PathBuf::from(tmpdir.path());
 		path.push("authcodes");
 		let connect = Rpc::connect(&format!("ws://127.0.0.1:{}", port - 1), &path);
 
-		connect.map(|conn| {
+		let _ = connect.map(|conn| {
 			assert!(matches!(&conn, &Err(RpcError::WsError(_))));
 		}).wait();
-
-		drop(srv);
 	}
 
 	#[test]
 	fn test_authcode_fail() {
-		let (srv, port, _, _) = serve();
+		let (_srv, port, _, _) = serve();
 		let path = PathBuf::from("nonexist");
 
 		let connect = Rpc::connect(&format!("ws://127.0.0.1:{}", port), &path);
 
-		connect.map(|conn| {
+		let _ = connect.map(|conn| {
 			assert!(matches!(&conn, &Err(RpcError::NoAuthCode)));
 		}).wait();
-
-		drop(srv);
 	}
 
 	#[test]
 	fn test_authcode_correct() {
-		let (srv, port, tmpdir, _) = serve();
+		let (_srv, port, tmpdir, _) = serve();
 
 		let mut path = PathBuf::from(tmpdir.path());
 		path.push("authcodes");
 		let connect = Rpc::connect(&format!("ws://127.0.0.1:{}", port), &path);
 
-		connect.map(|conn| {
+		let _ = connect.map(|conn| {
 			assert!(conn.is_ok())
 		}).wait();
-
-		drop(srv);
 	}
 
 }

rpc_client/src/signer.rs

@@ -1,4 +1,3 @@
-
 use client::{Rpc, RpcError};
 use rpc::v1::types::{ConfirmationRequest,
 					 TransactionModification,
@@ -13,16 +12,12 @@ pub struct SignerRpc {
 
 impl SignerRpc {
 	pub fn new(url: &str, authfile: &PathBuf) -> Result<Self, RpcError> {
-		match Rpc::new(&url, authfile) {
+		Ok(SignerRpc { rpc: try!(Rpc::new(&url, authfile)) })
-			Ok(rpc) => Ok(SignerRpc { rpc: rpc }),
-			Err(e) => Err(e),
-		}
 	}
 	pub fn requests_to_confirm(&mut self) ->
 		BoxFuture<Result<Vec<ConfirmationRequest>, RpcError>, Canceled>
 	{
-		self.rpc.request::<Vec<ConfirmationRequest>>
+		self.rpc.request("personal_requestsToConfirm", vec![])
-			("personal_requestsToConfirm", vec![])
 	}
 	pub fn confirm_request(&mut self,
 						   id: U256,
@@ -30,7 +25,7 @@ impl SignerRpc {
 						   pwd: &str) ->
 		BoxFuture<Result<U256, RpcError>, Canceled>
 	{
-		self.rpc.request::<U256>("personal_confirmRequest", vec![
+		self.rpc.request("personal_confirmRequest", vec![
 			to_value(&format!("{:#x}", id)),
 			to_value(&TransactionModification { gas_price: new_gas_price }),
 			to_value(&pwd),
@@ -39,7 +34,7 @@ impl SignerRpc {
 	pub fn reject_request(&mut self, id: U256) ->
 		BoxFuture<Result<bool, RpcError>, Canceled>
 	{
-		self.rpc.request::<bool>("personal_rejectRequest", vec![
+		self.rpc.request("personal_rejectRequest", vec![
 			JsonValue::String(format!("{:#x}", id))
 		])
 	}