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

Fixing account naming #1810

Merged
merged 3 commits into from Aug 3, 2016
Merged

Fixing account naming #1810

merged 3 commits into from Aug 3, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Aug 2, 2016

  1. : is not valid part of filename on windows
  2. From<KeyFile> implementation removed and replaced with SafeAccount::from_file usage.

I'm still doing some more tests to confirm that those issues are solved:
Closes #1808
Closes #1809

@tomusdrw tomusdrw added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Aug 2, 2016
@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage decreased (-0.1%) to 86.871% when pulling 1aedeb7 on account-names into 1b507e0 on master.

@gavofyork
Copy link
Contributor

would be good to more or lesss copy geth's format and document our decision on the wiki:

UTC--2016-05-13T15-03-10.137084514Z--3e...41

that said, we might leave out the nanoseconds.

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Aug 3, 2016

Ok, should we have a migration for old keys? Or is it only for newly created?

@gavofyork
Copy link
Contributor

only for newly created - should only require alteration of the format line.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 3, 2016
@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Aug 3, 2016

Done. It required alterations of geth import logic as well (so that we don't duplicate account files).

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.02%) to 86.998% when pulling 5a01c10 on account-names into 1b507e0 on master.

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 3, 2016
keyfile_path.push(format!("{}-{}.json", keyfile.id, timestamp));
Some(keyfile_path)
let filename = account.filename.as_ref().cloned().unwrap_or_else(|| {
let timestamp = time::strftime("%Y-%m-%dT%H-%M-%S", &time::now_utc()).unwrap_or("???".to_owned());
Copy link
Contributor

Choose a reason for hiding this comment

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

are ?s a valid component of file names on all platforms?

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.04%) to 87.025% when pulling 1d28564 on account-names into 1b507e0 on master.

@gavofyork gavofyork merged commit 40a304b into master Aug 3, 2016
@gavofyork gavofyork deleted the account-names branch August 3, 2016 15:58
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

4 participants