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

Close after importing keys from geth #2464

Merged
merged 3 commits into from Oct 5, 2016
Merged

Close after importing keys from geth #2464

merged 3 commits into from Oct 5, 2016

Conversation

svyatonik
Copy link
Collaborator

@svyatonik svyatonik commented Oct 4, 2016 •
edited

closes ethcore/parity#1918

@parity-cla-bot
Copy link

It looks like @svyatonik hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.ethcore.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Ethcore CLA Bot

@svyatonik
Copy link
Collaborator Author

[clabot:check]

@svyatonik svyatonik closed this Oct 4, 2016
@parity-cla-bot
Copy link

It looks like @svyatonik signed our Contributor License Agreement. 👍

Many thanks,

Ethcore CLA Bot

@svyatonik svyatonik changed the title closes ethcore/parity#1918 Close after importing keys from geth Oct 4, 2016
@svyatonik svyatonik reopened this Oct 4, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 4, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling abc5db0 on svyatonik:master into * on ethcore:master*.

rphmeier
rphmeier reviewed Oct 4, 2016
View reviewed changes
parity/account.rs
use ethcore::ethstore::Error;

let dir = Box::new(try!(keys_dir(i.to)));
let secret_store = EthStore::open(dir).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe try!(EthStore::open(dir).map_err(|e| format!("Error opening key store: {}", e)))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy-pasted this line from other functions above (new, list) :) Just tried to mimic the code-style from previous contributors. Ok - will fix this for these functions too

rphmeier
rphmeier reviewed Oct 4, 2016
View reviewed changes
parity/account.rs
pub struct ImportFromGethAccounts {
pub testnet: bool,
pub to: String,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

documentation on this struct and fields would be a bonus :)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling abc5db0 on svyatonik:master into * on ethcore:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0e8dda7 on svyatonik:master into * on ethcore:master*.

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 4, 2016
@rphmeier
Copy link
Contributor

rphmeier commented Oct 4, 2016

LGTM, but haven't tested it yet.

@NikVolf NikVolf added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Oct 4, 2016
@NikVolf
Copy link
Contributor

NikVolf commented Oct 4, 2016 •
edited 

cargo run -- account import ./ethstore/tests/res/geth_keystore/
    Finished debug [unoptimized + debuginfo] target(s) in 0.1 secs
     Running `target/debug/parity account import ./ethstore/tests/res/geth_keystore/`
3

just types 3
or is it as designed?

@svyatonik
Copy link
Collaborator Author

This PR is not about --account import - it's about --import-geth-keys

@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 5, 2016
@NikVolf
Copy link
Contributor

NikVolf commented Oct 5, 2016

tested import, great job

@NikVolf NikVolf merged commit 02c04a3 into openethereum:master Oct 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@svyatonik @parity-cla-bot @coveralls @rphmeier @NikVolf @arkpar