From 025244e8b23a020fa0e906acc37228cb728beb5e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= <tomusdrw@users.noreply.github.com>
Date: Tue, 24 Oct 2017 11:57:55 +0200
Subject: [PATCH] Return error on timed unlock attempt. (#6777)

---
 parity/rpc_apis.rs                  |  4 ++--
 rpc/src/v1/impls/personal.rs        | 17 ++++++++++-------
 rpc/src/v1/tests/mocked/personal.rs |  8 ++++----
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/parity/rpc_apis.rs b/parity/rpc_apis.rs
index 6064d3d2f..14ca9a999 100644
--- a/parity/rpc_apis.rs
+++ b/parity/rpc_apis.rs
@@ -292,7 +292,7 @@ impl FullDependencies {
 					}
 				},
 				Api::Personal => {
-					handler.extend_with(PersonalClient::new(&self.secret_store, dispatcher.clone(), self.geth_compatibility).to_delegate());
+					handler.extend_with(PersonalClient::new(self.secret_store.clone(), dispatcher.clone(), self.geth_compatibility).to_delegate());
 				},
 				Api::Signer => {
 					handler.extend_with(SignerClient::new(&self.secret_store, dispatcher.clone(), &self.signer_service, self.remote.clone()).to_delegate());
@@ -495,7 +495,7 @@ impl<C: LightChainClient + 'static> LightDependencies<C> {
 				},
 				Api::Personal => {
 					let secret_store = Some(self.secret_store.clone());
-					handler.extend_with(PersonalClient::new(&secret_store, dispatcher.clone(), self.geth_compatibility).to_delegate());
+					handler.extend_with(PersonalClient::new(secret_store, dispatcher.clone(), self.geth_compatibility).to_delegate());
 				},
 				Api::Signer => {
 					let secret_store = Some(self.secret_store.clone());
diff --git a/rpc/src/v1/impls/personal.rs b/rpc/src/v1/impls/personal.rs
index cfc7521d9..6ad2285f9 100644
--- a/rpc/src/v1/impls/personal.rs
+++ b/rpc/src/v1/impls/personal.rs
@@ -42,11 +42,11 @@ pub struct PersonalClient<D: Dispatcher> {
 
 impl<D: Dispatcher> PersonalClient<D> {
 	/// Creates new PersonalClient
-	pub fn new(store: &Option<Arc<AccountProvider>>, dispatcher: D, allow_perm_unlock: bool) -> Self {
+	pub fn new(accounts: Option<Arc<AccountProvider>>, dispatcher: D, allow_perm_unlock: bool) -> Self {
 		PersonalClient {
-			accounts: store.clone(),
-			dispatcher: dispatcher,
-			allow_perm_unlock: allow_perm_unlock,
+			accounts,
+			dispatcher,
+			allow_perm_unlock,
 		}
 	}
 
@@ -89,15 +89,18 @@ impl<D: Dispatcher + 'static> Personal for PersonalClient<D> {
 		};
 
 		let r = match (self.allow_perm_unlock, duration) {
-			(false, _) => store.unlock_account_temporarily(account, account_pass),
+			(false, None) => store.unlock_account_temporarily(account, account_pass),
+			(false, _) => return Err(errors::unsupported(
+				"Time-unlocking is only supported in --geth compatibility mode.",
+				Some("Restart your client with --geth flag or use personal_sendTransaction instead."),
+			)),
 			(true, Some(0)) => store.unlock_account_permanently(account, account_pass),
 			(true, Some(d)) => store.unlock_account_timed(account, account_pass, d * 1000),
 			(true, None) => store.unlock_account_timed(account, account_pass, 300_000),
 		};
 		match r {
 			Ok(_) => Ok(true),
-			// TODO [ToDr] Proper error here?
-			Err(_) => Ok(false),
+			Err(err) => Err(errors::account("Unable to unlock the account.", err)),
 		}
 	}
 
diff --git a/rpc/src/v1/tests/mocked/personal.rs b/rpc/src/v1/tests/mocked/personal.rs
index d34d734c0..f1a521f87 100644
--- a/rpc/src/v1/tests/mocked/personal.rs
+++ b/rpc/src/v1/tests/mocked/personal.rs
@@ -54,7 +54,7 @@ fn setup() -> PersonalTester {
 	let miner = miner_service();
 
 	let dispatcher = FullDispatcher::new(client, miner.clone());
-	let personal = PersonalClient::new(&opt_accounts, dispatcher, false);
+	let personal = PersonalClient::new(opt_accounts, dispatcher, false);
 
 	let mut io = IoHandler::default();
 	io.extend_with(personal.to_delegate());
@@ -178,7 +178,7 @@ fn sign_and_send_test(method: &str) {
 }
 
 #[test]
-fn should_unlock_account_temporarily() {
+fn should_unlock_not_account_temporarily_if_allow_perm_is_disabled() {
 	let tester = setup();
 	let address = tester.accounts.new_account("password123").unwrap();
 
@@ -192,10 +192,10 @@ fn should_unlock_account_temporarily() {
 		],
 		"id": 1
 	}"#;
-	let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#;
+	let response = r#"{"jsonrpc":"2.0","error":{"code":-32000,"message":"Time-unlocking is only supported in --geth compatibility mode.","data":"Restart your client with --geth flag or use personal_sendTransaction instead."},"id":1}"#;
 	assert_eq!(tester.io.handle_request_sync(&request), Some(response.into()));
 
-	assert!(tester.accounts.sign(address, None, Default::default()).is_ok(), "Should unlock account.");
+	assert!(tester.accounts.sign(address, None, Default::default()).is_err(), "Should not unlock account.");
 }
 
 #[test]