Fixing errors returned by sendTransaction* method family (#1665)

rpc/src/v1/impls/eth_signing.rs

@@ -83,17 +83,14 @@ impl<C, M> EthSigningQueueClient<C, M> where C: MiningBlockChainClient, M: Miner
 
 				if accounts.is_unlocked(request.from) {
 					let sender = request.from;
-					return match sign_and_dispatch(&*client, &*miner, request, &*accounts, sender) {
+					return sign_and_dispatch(&*client, &*miner, request, &*accounts, sender);
-						Ok(hash) => to_value(&hash),
-						_ => to_value(&RpcH256::default()),
-					}
 				}
 
 				let queue = take_weak!(self.queue);
 				fill_optional_fields(&mut request, &*client, &*miner);
 				let promise = queue.add_request(request);
 				f(promise)
-		})
+			})
 	}
 }
 
@@ -110,17 +107,17 @@ impl<C, M> EthSigning for EthSigningQueueClient<C, M>
 
 	fn send_transaction(&self, params: Params) -> Result<Value, Error> {
 		try!(self.active());
-		self.dispatch(params, |promise: ConfirmationPromise| {
+		self.dispatch(params, |promise| {
 			promise.wait_with_timeout().unwrap_or_else(|| to_value(&RpcH256::default()))
 		})
 	}
 
 	fn post_transaction(&self, params: Params) -> Result<Value, Error> {
 		try!(self.active());
-		self.dispatch(params, |promise: ConfirmationPromise| {
+		self.dispatch(params, |promise| {
-			let ret = to_value(&RpcU256::from(promise.id()));
+			let id = promise.id();
-			self.pending.lock().insert(promise.id(), promise);
+			self.pending.lock().insert(id, promise);
-			ret
+			to_value(&RpcU256::from(id))
 		})
 	}
 
@@ -192,11 +189,8 @@ impl<C, M> EthSigning for EthSigningUnsafeClient<C, M> where
 			.and_then(|(request, )| {
 				let request: TRequest = request.into();
 				let sender = request.from;
-				match sign_and_dispatch(&*take_weak!(self.client), &*take_weak!(self.miner), request, &*take_weak!(self.accounts), sender) {
+				sign_and_dispatch(&*take_weak!(self.client), &*take_weak!(self.miner), request, &*take_weak!(self.accounts), sender)
-					Ok(hash) => to_value(&hash),
+			})
-					_ => to_value(&RpcH256::default()),
-				}
-		})
 	}
 
 	fn post_transaction(&self, _: Params) -> Result<Value, Error> {

rpc/src/v1/impls/mod.rs

@@ -73,6 +73,7 @@ mod error_codes {
 	pub const UNKNOWN_ERROR: i64 = -32009;
 	pub const TRANSACTION_ERROR: i64 = -32010;
 	pub const ACCOUNT_LOCKED: i64 = -32020;
+	pub const PASSWORD_INVALID: i64 = -32021;
 	pub const SIGNER_DISABLED: i64 = -32030;
 }
 
@@ -109,7 +110,7 @@ fn unlock_sign_and_dispatch<C, M>(client: &C, miner: &M, request: TransactionReq
 	let signed_transaction = {
 		let t = prepare_transaction(client, miner, request);
 		let hash = t.hash();
-		let signature = try!(account_provider.sign_with_password(address, password, hash).map_err(signing_error));
+		let signature = try!(account_provider.sign_with_password(address, password, hash).map_err(password_error));
 		t.with_signature(signature)
 	};
 
@@ -147,6 +148,14 @@ fn signing_error(error: AccountError) -> Error {
 	}
 }
 
+fn password_error(error: AccountError) -> Error {
+	Error {
+		code: ErrorCode::ServerError(error_codes::PASSWORD_INVALID),
+		message: "Account password is invalid or account does not exist.".into(),
+		data: Some(Value::String(format!("{:?}", error))),
+	}
+}
+
 fn transaction_error(error: EthcoreError) -> Error {
 	use ethcore::error::TransactionError::*;
 

rpc/src/v1/impls/personal.rs

@@ -18,7 +18,7 @@
 use std::sync::{Arc, Weak};
 use jsonrpc_core::*;
 use v1::traits::Personal;
-use v1::types::{H160 as RpcH160, H256 as RpcH256, TransactionRequest};
+use v1::types::{H160 as RpcH160, TransactionRequest};
 use v1::impls::unlock_sign_and_dispatch;
 use v1::helpers::{TransactionRequest as TRequest};
 use ethcore::account_provider::AccountProvider;
@@ -101,10 +101,7 @@ impl<C: 'static, M: 'static> Personal for PersonalClient<C, M> where C: MiningBl
 				let sender = request.from;
 				let accounts = take_weak!(self.accounts);
 
-				match unlock_sign_and_dispatch(&*take_weak!(self.client), &*take_weak!(self.miner), request, &*accounts, sender, password) {
+				unlock_sign_and_dispatch(&*take_weak!(self.client), &*take_weak!(self.miner), request, &*accounts, sender, password)
-					Ok(hash) => Ok(hash),
+			})
-					_ => to_value(&RpcH256::default()),
-				}
-		})
 	}
 }

rpc/src/v1/impls/personal_signer.rs

@@ -70,7 +70,7 @@ impl<C: 'static, M: 'static> PersonalSigner for SignerClient<C, M> where C: Mini
 				let queue = take_weak!(self.queue);
 				let client = take_weak!(self.client);
 				let miner = take_weak!(self.miner);
-				queue.peek(&id).and_then(|confirmation| {
+				queue.peek(&id).map(|confirmation| {
 					let mut request = confirmation.transaction;
 					// apply modification
 					if let Some(gas_price) = modification.gas_price {
@@ -78,18 +78,12 @@ impl<C: 'static, M: 'static> PersonalSigner for SignerClient<C, M> where C: Mini
 					}
 
 					let sender = request.from;
-
+					let result = unlock_sign_and_dispatch(&*client, &*miner, request, &*accounts, sender, pass);
-					match unlock_sign_and_dispatch(&*client, &*miner, request, &*accounts, sender, pass) {
+					if let Ok(ref hash) = result {
-						Ok(hash) => {
+						queue.request_confirmed(id, Ok(hash.clone()));
-							queue.request_confirmed(id, Ok(hash.clone()));
-							Some(to_value(&hash))
-						},
-						_ => None
 					}
-				})
+					result
-				.unwrap_or_else(|| {
+				}).unwrap_or_else(|| Err(Error::invalid_params()))
-					to_value(&false)
-				})
 			}
 		)
 	}

rpc/src/v1/tests/mocked/eth.rs

@@ -609,6 +609,27 @@ fn rpc_eth_send_transaction() {
 	assert_eq!(tester.io.handle_request(&request), Some(response));
 }
 
+#[test]
+fn rpc_eth_send_transaction_error() {
+	let tester = EthTester::default();
+	let address = tester.accounts_provider.new_account("").unwrap();
+	let request = r#"{
+		"jsonrpc": "2.0",
+		"method": "eth_sendTransaction",
+		"params": [{
+			"from": ""#.to_owned() + format!("0x{:?}", address).as_ref() + r#"",
+			"to": "0xd46e8dd67c5d32be8058bb8eb970870f07244567",
+			"gas": "0x76c0",
+			"gasPrice": "0x9184e72a000",
+			"value": "0x9184e72a"
+		}],
+		"id": 1
+	}"#;
+
+	let response = r#"{"jsonrpc":"2.0","error":{"code":-32020,"message":"Your account is locked. Unlock the account via CLI, personal_unlockAccount or use Trusted Signer.","data":"NotUnlocked"},"id":1}"#;
+	assert_eq!(tester.io.handle_request(&request), Some(response.into()));
+}
+
 #[test]
 fn rpc_eth_send_raw_transaction() {
 	let tester = EthTester::default();

rpc/src/v1/tests/mocked/personal.rs

@@ -135,7 +135,7 @@ fn sign_and_send_transaction_with_invalid_password() {
 		"id": 1
 	}"#;
 
-	let response = r#"{"jsonrpc":"2.0","result":"0x0000000000000000000000000000000000000000000000000000000000000000","id":1}"#;
+	let response = r#"{"jsonrpc":"2.0","error":{"code":-32021,"message":"Account password is invalid or account does not exist.","data":"SStore(InvalidPassword)"},"id":1}"#;
 
 	assert_eq!(tester.io.handle_request(request.as_ref()), Some(response.into()));
 }

rpc/src/v1/tests/mocked/personal_signer.rs

@@ -132,7 +132,7 @@ fn should_not_remove_transaction_if_password_is_invalid() {
 
 	// when
 	let request = r#"{"jsonrpc":"2.0","method":"personal_confirmTransaction","params":["0x01",{},"xxx"],"id":1}"#;
-	let response = r#"{"jsonrpc":"2.0","result":false,"id":1}"#;
+	let response = r#"{"jsonrpc":"2.0","error":{"code":-32021,"message":"Account password is invalid or account does not exist.","data":"SStore(InvalidAccount)"},"id":1}"#;
 
 	// then
 	assert_eq!(tester.io.handle_request(&request), Some(response.to_owned()));