From 9c7395a6bef125ddd3b787e167abb131ef5ac60e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 20 Jul 2016 12:37:49 +0200 Subject: [PATCH] Fixing errors returned by sendTransaction* method family (#1665) --- rpc/src/v1/impls/eth_signing.rs | 24 ++++++++-------------- rpc/src/v1/impls/mod.rs | 11 +++++++++- rpc/src/v1/impls/personal.rs | 9 +++----- rpc/src/v1/impls/personal_signer.rs | 18 ++++++---------- rpc/src/v1/tests/mocked/eth.rs | 21 +++++++++++++++++++ rpc/src/v1/tests/mocked/personal.rs | 2 +- rpc/src/v1/tests/mocked/personal_signer.rs | 2 +- 7 files changed, 51 insertions(+), 36 deletions(-) diff --git a/rpc/src/v1/impls/eth_signing.rs b/rpc/src/v1/impls/eth_signing.rs index 331ce4deb..58b28473d 100644 --- a/rpc/src/v1/impls/eth_signing.rs +++ b/rpc/src/v1/impls/eth_signing.rs @@ -83,17 +83,14 @@ impl EthSigningQueueClient 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) { - Ok(hash) => to_value(&hash), - _ => to_value(&RpcH256::default()), - } + return sign_and_dispatch(&*client, &*miner, request, &*accounts, sender); } 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 EthSigning for EthSigningQueueClient fn send_transaction(&self, params: Params) -> Result { 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 { try!(self.active()); - self.dispatch(params, |promise: ConfirmationPromise| { - let ret = to_value(&RpcU256::from(promise.id())); - self.pending.lock().insert(promise.id(), promise); - ret + self.dispatch(params, |promise| { + let id = promise.id(); + self.pending.lock().insert(id, promise); + to_value(&RpcU256::from(id)) }) } @@ -192,11 +189,8 @@ impl EthSigning for EthSigningUnsafeClient 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) { - Ok(hash) => to_value(&hash), - _ => to_value(&RpcH256::default()), - } - }) + sign_and_dispatch(&*take_weak!(self.client), &*take_weak!(self.miner), request, &*take_weak!(self.accounts), sender) + }) } fn post_transaction(&self, _: Params) -> Result { diff --git a/rpc/src/v1/impls/mod.rs b/rpc/src/v1/impls/mod.rs index c66350fdd..2f45b5e56 100644 --- a/rpc/src/v1/impls/mod.rs +++ b/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(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::*; diff --git a/rpc/src/v1/impls/personal.rs b/rpc/src/v1/impls/personal.rs index d6286afb9..4430ab502 100644 --- a/rpc/src/v1/impls/personal.rs +++ b/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 Personal for PersonalClient 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) { - Ok(hash) => Ok(hash), - _ => to_value(&RpcH256::default()), - } - }) + unlock_sign_and_dispatch(&*take_weak!(self.client), &*take_weak!(self.miner), request, &*accounts, sender, password) + }) } } diff --git a/rpc/src/v1/impls/personal_signer.rs b/rpc/src/v1/impls/personal_signer.rs index d9c3be21d..823e20577 100644 --- a/rpc/src/v1/impls/personal_signer.rs +++ b/rpc/src/v1/impls/personal_signer.rs @@ -70,7 +70,7 @@ impl PersonalSigner for SignerClient 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 PersonalSigner for SignerClient where C: Mini } let sender = request.from; - - match unlock_sign_and_dispatch(&*client, &*miner, request, &*accounts, sender, pass) { - Ok(hash) => { - queue.request_confirmed(id, Ok(hash.clone())); - Some(to_value(&hash)) - }, - _ => None + let result = unlock_sign_and_dispatch(&*client, &*miner, request, &*accounts, sender, pass); + if let Ok(ref hash) = result { + queue.request_confirmed(id, Ok(hash.clone())); } - }) - .unwrap_or_else(|| { - to_value(&false) - }) + result + }).unwrap_or_else(|| Err(Error::invalid_params())) } ) } diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index 9e7331f00..ff53be976 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/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(); diff --git a/rpc/src/v1/tests/mocked/personal.rs b/rpc/src/v1/tests/mocked/personal.rs index 3b299728c..1aebde46b 100644 --- a/rpc/src/v1/tests/mocked/personal.rs +++ b/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())); } diff --git a/rpc/src/v1/tests/mocked/personal_signer.rs b/rpc/src/v1/tests/mocked/personal_signer.rs index ced9a228a..c3a0c070d 100644 --- a/rpc/src/v1/tests/mocked/personal_signer.rs +++ b/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()));