From 8596134c0f701d9fec07ccde38373b13467e1819 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 9 Dec 2016 08:31:58 +0000 Subject: [PATCH] Clearer updates handling --- ethcore/src/account_provider/mod.rs | 29 +++++++++++-------- ethstore/src/dir/disk.rs | 5 ++++ ethstore/src/dir/geth.rs | 4 +++ ethstore/src/dir/mod.rs | 1 + ethstore/src/dir/parity.rs | 4 +++ ethstore/src/ethstore.rs | 42 ++++++++++++++++++---------- ethstore/tests/util/transient_dir.rs | 4 +++ 7 files changed, 63 insertions(+), 26 deletions(-) diff --git a/ethcore/src/account_provider/mod.rs b/ethcore/src/account_provider/mod.rs index 7e0005c7a..bc7ffdfed 100644 --- a/ethcore/src/account_provider/mod.rs +++ b/ethcore/src/account_provider/mod.rs @@ -23,7 +23,7 @@ use self::stores::{AddressBook, DappsSettingsStore}; use std::fmt; use std::collections::HashMap; use std::time::{Instant, Duration}; -use util::{Mutex, RwLock, Itertools}; +use util::{RwLock, Itertools}; use ethstore::{SimpleSecretStore, SecretStore, Error as SSError, SafeAccount, EthStore, EthMultiStore, random_string}; use ethstore::dir::{KeyDirectory}; use ethstore::ethkey::{Address, Message, Public, Secret, Random, Generator}; @@ -83,7 +83,7 @@ impl KeyDirectory for NullDir { Ok(self.accounts.read().values().cloned().flatten().collect()) } - fn insert(&self, account: SafeAccount) -> Result { + fn update(&self, account: SafeAccount) -> Result { let mut lock = self.accounts.write(); let mut accounts = lock.entry(account.address.clone()).or_insert_with(Vec::new); // If the filename is the same we just need to replace the entry @@ -92,6 +92,13 @@ impl KeyDirectory for NullDir { Ok(account) } + fn insert(&self, account: SafeAccount) -> Result { + let mut lock = self.accounts.write(); + let mut accounts = lock.entry(account.address.clone()).or_insert_with(Vec::new); + accounts.push(account.clone()); + Ok(account) + } + fn remove(&self, account: &SafeAccount) -> Result<(), SSError> { let mut accounts = self.accounts.write(); let is_empty = if let Some(mut accounts) = accounts.get_mut(&account.address) { @@ -121,8 +128,8 @@ type AccountToken = String; /// Account management. /// Responsible for unlocking accounts. pub struct AccountProvider { - address_book: Mutex, - unlocked: Mutex>, + unlocked: RwLock>, + address_book: RwLock, dapps_settings: RwLock, /// Accounts on disk sstore: Box, @@ -134,7 +141,7 @@ impl AccountProvider { /// Creates new account provider. pub fn new(sstore: Box) -> Self { AccountProvider { - unlocked: Mutex::new(HashMap::new()), + unlocked: RwLock::new(HashMap::new()), address_book: RwLock::new(AddressBook::new(sstore.local_path().into())), dapps_settings: RwLock::new(DappsSettingsStore::new(sstore.local_path().into())), sstore: sstore, @@ -145,8 +152,8 @@ impl AccountProvider { /// Creates not disk backed provider. pub fn transient_provider() -> Self { AccountProvider { - address_book: Mutex::new(AddressBook::transient()), - unlocked: Mutex::new(HashMap::new()), + unlocked: RwLock::new(HashMap::new()), + address_book: RwLock::new(AddressBook::transient()), dapps_settings: RwLock::new(DappsSettingsStore::transient()), sstore: Box::new(EthStore::open(Box::new(NullDir::default())).expect("NullDir load always succeeds; qed")), transient_sstore: transient_sstore(), @@ -278,7 +285,7 @@ impl AccountProvider { let _ = try!(self.sstore.sign(&account, &password, &Default::default())); // check if account is already unlocked pernamently, if it is, do nothing - let mut unlocked = self.unlocked.lock(); + let mut unlocked = self.unlocked.write(); if let Some(data) = unlocked.get(&account) { if let Unlock::Perm = data.unlock { return Ok(()) @@ -295,7 +302,7 @@ impl AccountProvider { } fn password(&self, account: &Address) -> Result { - let mut unlocked = self.unlocked.lock(); + let mut unlocked = self.unlocked.write(); let data = try!(unlocked.get(account).ok_or(Error::NotUnlocked)).clone(); if let Unlock::Temp = data.unlock { unlocked.remove(account).expect("data exists: so key must exist: qed"); @@ -326,7 +333,7 @@ impl AccountProvider { /// Checks if given account is unlocked pub fn is_unlocked(&self, account: Address) -> bool { - let unlocked = self.unlocked.lock(); + let unlocked = self.unlocked.read(); unlocked.get(&account).is_some() } @@ -434,7 +441,7 @@ mod tests { assert!(ap.unlock_account_timed(kp.address(), "test1".into(), 60000).is_err()); assert!(ap.unlock_account_timed(kp.address(), "test".into(), 60000).is_ok()); assert!(ap.sign(kp.address(), None, Default::default()).is_ok()); - ap.unlocked.lock().get_mut(&kp.address()).unwrap().unlock = Unlock::Timed(Instant::now()); + ap.unlocked.write().get_mut(&kp.address()).unwrap().unlock = Unlock::Timed(Instant::now()); assert!(ap.sign(kp.address(), None, Default::default()).is_err()); } diff --git a/ethstore/src/dir/disk.rs b/ethstore/src/dir/disk.rs index 22093171e..8c1aeb48f 100644 --- a/ethstore/src/dir/disk.rs +++ b/ethstore/src/dir/disk.rs @@ -105,6 +105,11 @@ impl KeyDirectory for DiskDirectory { Ok(accounts) } + fn update(&self, account: SafeAccount) -> Result { + // Disk store handles updates correctly iff filename is the same + self.insert(account) + } + fn insert(&self, account: SafeAccount) -> Result { // transform account into key file let keyfile: json::KeyFile = account.clone().into(); diff --git a/ethstore/src/dir/geth.rs b/ethstore/src/dir/geth.rs index a5367f98d..60537f045 100644 --- a/ethstore/src/dir/geth.rs +++ b/ethstore/src/dir/geth.rs @@ -88,6 +88,10 @@ impl KeyDirectory for GethDirectory { self.dir.insert(account) } + fn update(&self, account: SafeAccount) -> Result { + self.dir.update(account) + } + fn remove(&self, account: &SafeAccount) -> Result<(), Error> { self.dir.remove(account) } diff --git a/ethstore/src/dir/mod.rs b/ethstore/src/dir/mod.rs index 0da4d71fb..e2c6c1117 100644 --- a/ethstore/src/dir/mod.rs +++ b/ethstore/src/dir/mod.rs @@ -29,6 +29,7 @@ pub enum DirectoryType { pub trait KeyDirectory: Send + Sync { fn load(&self) -> Result, Error>; fn insert(&self, account: SafeAccount) -> Result; + fn update(&self, account: SafeAccount) -> Result; fn remove(&self, account: &SafeAccount) -> Result<(), Error>; fn path(&self) -> Option<&PathBuf> { None } } diff --git a/ethstore/src/dir/parity.rs b/ethstore/src/dir/parity.rs index c5d0057d8..cc3bb5cd3 100644 --- a/ethstore/src/dir/parity.rs +++ b/ethstore/src/dir/parity.rs @@ -67,6 +67,10 @@ impl KeyDirectory for ParityDirectory { self.dir.insert(account) } + fn update(&self, account: SafeAccount) -> Result { + self.dir.update(account) + } + fn remove(&self, account: &SafeAccount) -> Result<(), Error> { self.dir.remove(account) } diff --git a/ethstore/src/ethstore.rs b/ethstore/src/ethstore.rs index 8cbce0f1c..158402a60 100644 --- a/ethstore/src/ethstore.rs +++ b/ethstore/src/ethstore.rs @@ -92,7 +92,7 @@ impl SecretStore for EthStore { let secret = try!(safe_account.crypto.secret(password).map_err(|_| Error::InvalidPassword)); safe_account.address = try!(KeyPair::from_secret(secret)).address(); let address = safe_account.address.clone(); - try!(self.store.save(safe_account)); + try!(self.store.import(safe_account)); Ok(address) } @@ -129,19 +129,21 @@ impl SecretStore for EthStore { } fn set_name(&self, address: &Address, name: String) -> Result<(), Error> { - let mut account = try!(self.get(address)); + let old = try!(self.get(address)); + let mut account = old.clone(); account.name = name; // save to file - self.store.save(account) + self.store.update(old, account) } fn set_meta(&self, address: &Address, meta: String) -> Result<(), Error> { - let mut account = try!(self.get(address)); + let old = try!(self.get(address)); + let mut account = old.clone(); account.meta = meta; // save to file - self.store.save(account) + self.store.update(old, account) } fn local_path(&self) -> String { @@ -213,20 +215,32 @@ impl EthMultiStore { } } - fn save(&self, account: SafeAccount) -> Result<(), Error> { - //save to file + fn import(&self, account: SafeAccount) -> Result<(), Error> { + // save to file let account = try!(self.dir.insert(account)); // update cache let mut cache = self.cache.write(); let mut accounts = cache.entry(account.address.clone()).or_insert_with(Vec::new); - // TODO [ToDr] That is crappy way of overcoming set_name, set_meta, etc. - // Avoid cloning instead! - accounts.retain(|acc| acc.filename != account.filename); accounts.push(account); Ok(()) } + fn update(&self, old: SafeAccount, new: SafeAccount) -> Result<(), Error> { + // save to file + let account = try!(self.dir.update(new)); + + // update cache + let mut cache = self.cache.write(); + let mut accounts = cache.entry(account.address.clone()).or_insert_with(Vec::new); + // Remove old account + accounts.retain(|acc| acc != &old); + // And push updated to the end + accounts.push(account); + Ok(()) + + } + } impl SimpleSecretStore for EthMultiStore { @@ -235,7 +249,7 @@ impl SimpleSecretStore for EthMultiStore { let id: [u8; 16] = Random::random(); let account = SafeAccount::create(&keypair, id, password, self.iterations, "".to_owned(), "{}".to_owned()); let address = account.address.clone(); - try!(self.save(account)); + try!(self.import(account)); Ok(address) } @@ -278,11 +292,9 @@ impl SimpleSecretStore for EthMultiStore { fn change_password(&self, address: &Address, old_password: &str, new_password: &str) -> Result<(), Error> { let accounts = try!(self.get(address)); for account in accounts { - // First remove - try!(self.remove_account(&address, old_password)); - // Then insert back with new password + // Change password let new_account = try!(account.change_password(old_password, new_password, self.iterations)); - try!(self.save(new_account)); + try!(self.update(account, new_account)); } Ok(()) } diff --git a/ethstore/tests/util/transient_dir.rs b/ethstore/tests/util/transient_dir.rs index 76010182e..f01c98063 100644 --- a/ethstore/tests/util/transient_dir.rs +++ b/ethstore/tests/util/transient_dir.rs @@ -63,6 +63,10 @@ impl KeyDirectory for TransientDir { self.dir.load() } + fn update(&self, account: SafeAccount) -> Result { + self.dir.update(account) + } + fn insert(&self, account: SafeAccount) -> Result { self.dir.insert(account) }