Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Key files include timestamp in name. #1700

Merged
merged 7 commits into from Jul 25, 2016
Merged

Conversation

gavofyork
Copy link
Contributor

Introduce timestamp into new key files; keep filename around, so
that we don't accidentally duplicate keys.

Introduce timestamp into new key files; keep filename around, so
that we don't accidentally duplicate keys.
@gavofyork gavofyork added the A0-pleasereview 🤓 Pull request needs code review. label Jul 23, 2016
fn insert(&self, account: SafeAccount) -> Result<(), Error> {
self.dir.insert(account)
fn insert(&self, account: SafeAccount) -> Result<SafeAccount, Error> {
self.dir.insert(account.clone())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clone unnecessary?

@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage increased (+0.09%) to 75.545% when pulling 3544258 on timestamp-in-key-filenames into 247495f on master.

@@ -15,6 +15,8 @@ rustc-serialize = "0.3"
rust-crypto = "0.2.36"
tiny-keccak = "1.0"
docopt = { version = "0.6", optional = true }
time = "0.1.34"
log = "0.3"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used? I cant't see any logging added

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 24, 2016
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 24, 2016
@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage increased (+0.1%) to 75.642% when pulling fee830b on timestamp-in-key-filenames into 8574bfd on master.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 25, 2016
@gavofyork gavofyork merged commit 435ba18 into master Jul 25, 2016
@gavofyork gavofyork deleted the timestamp-in-key-filenames branch July 25, 2016 08:45
@@ -35,16 +36,17 @@ pub struct SafeAccount {
pub version: Version,
pub address: Address,
pub crypto: Crypto,
pub path: Option<PathBuf>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about just storing a filename: Option<String> instead? It will allow migrations between two DiskDirectories - right know accounts with path.is_some() can basically come from anywhere on the disk and whole DiskDirectory is kind of useless and misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a potential improvement, sure, but not strictly necessary for this feature.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants