Clearer updates handling

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<SafeAccount, SSError> {
+	fn update(&self, account: SafeAccount) -> Result<SafeAccount, SSError> {
 		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<SafeAccount, SSError> {
+		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<AddressBook>,
+	unlocked: RwLock<HashMap<Address, AccountData>>,
-	unlocked: Mutex<HashMap<Address, AccountData>>,
+	address_book: RwLock<AddressBook>,
 	dapps_settings: RwLock<DappsSettingsStore>,
 	/// Accounts on disk
 	sstore: Box<SecretStore>,
@@ -134,7 +141,7 @@ impl AccountProvider {
 	/// Creates new account provider.
 	pub fn new(sstore: Box<SecretStore>) -> 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: RwLock::new(HashMap::new()),
-			unlocked: Mutex::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<String, Error> {
-		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());
 	}
 

ethstore/src/dir/disk.rs

@@ -105,6 +105,11 @@ impl KeyDirectory for DiskDirectory {
 		Ok(accounts)
 	}
 
+	fn update(&self, account: SafeAccount) -> Result<SafeAccount, Error> {
+		// Disk store handles updates correctly iff filename is the same
+		self.insert(account)
+	}
+
 	fn insert(&self, account: SafeAccount) -> Result<SafeAccount, Error> {
 		// transform account into key file
 		let keyfile: json::KeyFile = account.clone().into();

ethstore/src/dir/geth.rs

@@ -88,6 +88,10 @@ impl KeyDirectory for GethDirectory {
 		self.dir.insert(account)
 	}
 
+	fn update(&self, account: SafeAccount) -> Result<SafeAccount, Error> {
+		self.dir.update(account)
+	}
+
 	fn remove(&self, account: &SafeAccount) -> Result<(), Error> {
 		self.dir.remove(account)
 	}

ethstore/src/dir/mod.rs

@@ -29,6 +29,7 @@ pub enum DirectoryType {
 pub trait KeyDirectory: Send + Sync {
 	fn load(&self) -> Result<Vec<SafeAccount>, Error>;
 	fn insert(&self, account: SafeAccount) -> Result<SafeAccount, Error>;
+	fn update(&self, account: SafeAccount) -> Result<SafeAccount, Error>;
 	fn remove(&self, account: &SafeAccount) -> Result<(), Error>;
 	fn path(&self) -> Option<&PathBuf> { None }
 }

ethstore/src/dir/parity.rs

@@ -67,6 +67,10 @@ impl KeyDirectory for ParityDirectory {
 		self.dir.insert(account)
 	}
 
+	fn update(&self, account: SafeAccount) -> Result<SafeAccount, Error> {
+		self.dir.update(account)
+	}
+
 	fn remove(&self, account: &SafeAccount) -> Result<(), Error> {
 		self.dir.remove(account)
 	}

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> {
+	fn import(&self, account: SafeAccount) -> Result<(), Error> {
-		//save to file
+		// 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
+			// Change password
-			try!(self.remove_account(&address, old_password));
-			// Then insert back with new 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(())
 	}

ethstore/tests/util/transient_dir.rs

@@ -63,6 +63,10 @@ impl KeyDirectory for TransientDir {
 		self.dir.load()
 	}
 
+	fn update(&self, account: SafeAccount) -> Result<SafeAccount, Error> {
+		self.dir.update(account)
+	}
+
 	fn insert(&self, account: SafeAccount) -> Result<SafeAccount, Error> {
 		self.dir.insert(account)
 	}