Problem:(fix #827)no import/export for non-HD keys #1106

linfeng-crypto · 2020-02-19T08:56:48Z

Solution:

add export method to backup a wallet, and import method to store the wallet again, both in client-rpc and client-cli
add unit test

yihuang · 2020-02-19T09:13:32Z

client-cli/src/command/wallet_command.rs

+            parse(from_os_str),
+            help = "file to dump wallet"
+        )]
+        file: PathBuf,


I suggest to treat a special value "-" as stdout and make it the default value, so the command can work with pipes in shell.

yihuang · 2020-02-19T09:13:49Z

client-cli/src/command/wallet_command.rs

+            parse(from_os_str),
+            help = "file stored the wallet info"
+        )]
+        file: PathBuf,


I suggest to treat a special value "-" as stdin and make it the default value, so the command can work with pipes in shell.

Currently, I serialized the wallet info as multiline json string using the serde_json::to_string_pretty.

If the wallet info inputted from the terminal, I have to serialize the wallet info into one line json string. By the way, the output is colored and there is a waring messge like Warning! CRYPTO_CHAIN_ID environment variable is not set. Setting network to devnet and network-id to 0. I don't think the the pipes can work well.

I mean something like this:

let output = if name == "-" { std::io::stdout } else { File::open(name)... }; output.write_all(...);

If warnings are going to stderr, then it's ok, otherwise, it's indeed problematic.

- is too short , "console"?

- is too short , "console"?

- is like a conventional name for this.

How does the command work with -, like this?

cat wallet_file | ./client-cli wallet import -

This is one way to use it, but only some developers know it.

How does the command work with -, like this?

cat wallet_file | ./client-cli wallet import -

Yes, with pipes it can work with other tools together, for example, compression tool, assuming stdin/stdout is the default:

$ ./client-cli wallet export --all | gzip > wallets.export.gz $ zcat wallets.export.gz | ./client-cli wallet import

yihuang · 2020-02-19T09:16:19Z

We had an issue before to import/export all wallets at once, which is closed for duplication of this one, so I presume this one should support import/export all wallets at once?

tomtau · 2020-02-19T09:32:17Z

We had an issue before to import/export all wallets at once, which is closed for duplication of this one, so I presume this one should support import/export all wallets at once?

@leejw51crypto is this one ok as is, or should it take say several names of wallets, or a flag --all and ask for individual details (and then dump all in one file or ask for several ?)

codecov · 2020-02-19T09:36:05Z

Codecov Report

Merging #1106 into master will decrease coverage by 1.07%.
The diff coverage is 36.78%.

@@            Coverage Diff            @@
##           master   #1106      +/-   ##
=========================================
- Coverage   65.57%   64.5%   -1.08%     
=========================================
  Files         145     149       +4     
  Lines       19041   19677     +636     
=========================================
+ Hits        12487   12692     +205     
- Misses       6554    6985     +431

Impacted Files	Coverage Δ
client-rpc/src/rpc/sync_rpc.rs	`0% <ø> (ø)`	⬆️
client-rpc/src/rpc/multisig_rpc.rs	`31.27% <ø> (ø)`	⬆️
client-rpc/src/rpc/staking_rpc.rs	`0% <ø> (ø)`	⬆️
client-rpc/src/server.rs	`6.36% <ø> (-4.85%)`	⬇️
client-cli/src/command/wallet_command.rs	`0% <0%> (ø)`	⬆️
chain-tx-validation/src/lib.rs	`70.3% <100%> (-0.13%)`	⬇️
client-core/src/wallet.rs	`100% <100%> (ø)`
client-core/src/service/wallet_service.rs	`78.5% <4.76%> (-8.1%)`	⬇️
client-core/src/wallet/default_wallet_client.rs	`42.51% <85.71%> (+4.24%)`	⬆️
client-rpc/src/rpc/wallet_rpc.rs	`65.09% <94.87%> (+4.37%)`	⬆️
... and 26 more

leejw51crypto · 2020-02-19T10:01:54Z

import and export all wallets would be better
dump all to one file also using one password?
if decoding fails, we can ask again

consider that wallets can be over 100 or thousands

tomtau · 2020-02-19T10:07:43Z

import and export all wallets would be better
dump all to one file also using one password?
if decoding fails, we can ask again

consider that wallets can be over 100 or thousands

will anyone have client-cli with that many wallets? there'll be other issues arising for that, so it can be left for now
each wallet may a different passphrase

leejw51crypto · 2020-02-19T10:08:44Z

yes, that's possible scenario for special use case
a few hundreds are common , i think

yihuang · 2020-02-20T05:06:21Z

Is it better to let export/import the encrypted data as is, not to decrypt and re-encrypt? This will work for multiple wallets with different passphrases.
And it's safer because the information is not decrypted?

linfeng-crypto · 2020-02-20T07:39:59Z

Is it better to let export/import the encrypted data as is, not to decrypt and re-encrypt? This will work for multiple wallets with different passphrases.
And it's safer because the information is not decrypted?

I think it's user's response to encrypt data and keep the file safe, just like the mnemonics. It's not the client's business.

leejw51crypto · 2020-02-20T08:06:48Z

client-cli/src/command/wallet_command.rs

+        #[structopt(
+            name = "from_file",
+            short = "f",
+            long = "from_file",


how about just from

I think it's ok -- if it's too long, one can just use -f ... -t ...?

leejw51crypto · 2020-02-20T08:07:12Z

client-cli/src/command/wallet_command.rs

+        #[structopt(
+            name = "to_file",
+            short = "t",
+            long = "to_file",


#1106 (comment)

leejw51crypto · 2020-02-20T08:15:12Z

also addresses, roothashes(mutlsig addresses) need to export, import

yihuang · 2020-02-20T08:26:12Z

Is it better to let export/import the encrypted data as is, not to decrypt and re-encrypt? This will work for multiple wallets with different passphrases.
And it's safer because the information is not decrypted?

I think it's user's response to encrypt data and keep the file safe, just like the mnemonics. It's not the client's business.

I mean if we don't decrypt the information, we can simply:

client-cli export --all

No need to worry about different passphrases of different wallets.

linfeng-crypto · 2020-02-21T01:49:18Z

Is it better to let export/import the encrypted data as is, not to decrypt and re-encrypt? This will work for multiple wallets with different passphrases.
And it's safer because the information is not decrypted?

I think it's user's response to encrypt data and keep the file safe, just like the mnemonics. It's not the client's business.

I mean if we don't decrypt the information, we can simply:
client-cli export --all
No need to worry about different passphrases of different wallets.

sure, to get the private key, the workflow is wallet_name -> public key -> private key, but we should give a enckey to get the public key, or we have to store the un-encrypted mapwallet_name->pubkey, is it safe?

yihuang · 2020-02-21T03:04:46Z

Is it better to let export/import the encrypted data as is, not to decrypt and re-encrypt? This will work for multiple wallets with different passphrases.
And it's safer because the information is not decrypted?

I think it's user's response to encrypt data and keep the file safe, just like the mnemonics. It's not the client's business.

I mean if we don't decrypt the information, we can simply:
client-cli export --all
No need to worry about different passphrases of different wallets.
sure, to get the private key, the workflow is wallet_name -> public key -> private key, but we should give a enckey to get the public key, or we have to store the un-encrypted mapwallet_name->pubkey, is it safe?

Yeah, that's a block, the structure of the key service is to be changed in #914, after that it probably will be changed to wallet_name -> key_pairs.
Before that, I guess we just have to stick to the current solution?

linfeng-crypto · 2020-02-21T03:24:59Z

Is it better to let export/import the encrypted data as is, not to decrypt and re-encrypt? This will work for multiple wallets with different passphrases.
And it's safer because the information is not decrypted?

I think it's user's response to encrypt data and keep the file safe, just like the mnemonics. It's not the client's business.

I mean if we don't decrypt the information, we can simply:
client-cli export --all
No need to worry about different passphrases of different wallets.
sure, to get the private key, the workflow is wallet_name -> public key -> private key, but we should give a enckey to get the public key, or we have to store the un-encrypted mapwallet_name->pubkey, is it safe?
Yeah, that's a block, the structure of the key service is to be changed in #914, after that it probably will be changed to wallet_name -> key_pairs.
Before that, I guess we just have to stick to the current solution?

sure

tomtau

a small clippy suggestion:

error: single-character string constant used as pattern
1564   --> client-cli/src/command/wallet_command.rs:196:41
1565    |
1566196 |                 for name in names.split(",") {
1567    |                                         ^^^ help: try using a char instead: `','`
1568    |
1569    = note: `-D clippy::single-char-pattern` implied by `-D warnings`
1570    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
1571
1572error: aborting due to previous error

could you squash the commits?

tomtau · 2020-02-21T09:27:29Z

client-core/src/service/wallet_service.rs

+    #[serde(deserialize_with = "deserde_from_str", serialize_with = "serde_to_str")]
+    pub private_key: PrivateKey,
+    /// passphrase used when import wallet
+    pub passphrase: String,


String -> SecUtf8?

Or Option, as it's optional (the client would ask for it if not provided?)?

tomtau

two small comments:

type of passphrase would be better Option<SecUtf8> rather than String to make it more concrete
export json settings can be extracted out into a struct / data type

tomtau · 2020-02-21T09:37:10Z

client-cli/src/command/wallet_command.rs

+                let wallet_settings: Value = serde_json::from_str(&settings)
+                    .chain(|| (ErrorKind::InvalidInput, "Invalid wallet info"))?;
+                let wallet_settings = wallet_settings.as_array().chain(|| {
+                    (
+                        ErrorKind::InvalidInput,
+                        "from_file should contains a list of setting",
+                    )
+                })?;
+                for setting in wallet_settings {
+                    let name = setting["name"]
+                        .as_str()
+                        .chain(|| (ErrorKind::InvalidInput, "expect name"))?;
+                    let key = setting["auth_token"]
+                        .as_str()
+                        .chain(|| (ErrorKind::InvalidInput, "Expect auth_token"))?;
+                    let enckey = parse_hex_enckey(&key).map_err(|_e| {
+                        error(&format!("error auth_token of wallet {}", name));
+                    });


probably giving it a data type "Vec" with serde implementations would be less error-prone ?

calvinaco · 2020-02-24T02:54:44Z

client-cli/src/command/wallet_command.rs

+            parse(from_os_str),
+            help = "file stored the wallet info"
+        )]
+        file: PathBuf,


How does the command work with -, like this?

cat wallet_file | ./client-cli wallet import -

client-core/src/service/wallet_service.rs

Solution: * add export/import wallet, the dumpped info include private key, wallet name, and all information in `Wallet` struct * in client-cli, can dump many wallets at onece by a setting file which include wallet name and auth-token * the dumpped wallet information is not encrypted, so carefule backup it.

tomtau · 2020-02-24T07:32:10Z

bors r+

bors · 2020-02-24T08:13:30Z

Build succeeded

linfeng-crypto requested review from calvinaco, devashishdxt and tomtau as code owners February 19, 2020 08:56

tomtau requested a review from yihuang February 19, 2020 09:08

yihuang reviewed Feb 19, 2020

View reviewed changes

tomtau requested review from yihuang and leejw51crypto February 19, 2020 09:27

linfeng-crypto force-pushed the issue-827 branch from 2edb931 to 955faa9 Compare February 20, 2020 04:25

leejw51crypto reviewed Feb 20, 2020

View reviewed changes

linfeng-crypto force-pushed the issue-827 branch from 955faa9 to a14349a Compare February 20, 2020 08:12

linfeng-crypto force-pushed the issue-827 branch from a14349a to d9c6437 Compare February 21, 2020 01:39

linfeng-crypto force-pushed the issue-827 branch from d9c6437 to b7a9d30 Compare February 21, 2020 02:50

yihuang approved these changes Feb 21, 2020

View reviewed changes

linfeng-crypto force-pushed the issue-827 branch from b7a9d30 to b450160 Compare February 21, 2020 03:15

tomtau suggested changes Feb 21, 2020

View reviewed changes

linfeng-crypto force-pushed the issue-827 branch from b450160 to 2788661 Compare February 21, 2020 07:57

tomtau reviewed Feb 21, 2020

View reviewed changes

tomtau suggested changes Feb 21, 2020

View reviewed changes

calvinaco reviewed Feb 24, 2020

View reviewed changes

linfeng-crypto force-pushed the issue-827 branch from 8c065d1 to a595649 Compare February 24, 2020 05:34

calvinaco approved these changes Feb 24, 2020

View reviewed changes

bors bot merged commit 5608759 into crypto-com:master Feb 24, 2020

Problem:(fix #827)no import/export for non-HD keys #1106

Problem:(fix #827)no import/export for non-HD keys #1106

Conversation

linfeng-crypto commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linfeng-crypto Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

yihuang Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

leejw51crypto Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

yihuang commented Feb 19, 2020

tomtau commented Feb 19, 2020

codecov bot commented Feb 19, 2020 • edited Loading

Codecov Report

leejw51crypto commented Feb 19, 2020 • edited Loading

tomtau commented Feb 19, 2020

leejw51crypto commented Feb 19, 2020 • edited Loading

yihuang commented Feb 20, 2020 • edited Loading

linfeng-crypto commented Feb 20, 2020

leejw51crypto Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto commented Feb 20, 2020

yihuang commented Feb 20, 2020

linfeng-crypto commented Feb 21, 2020 • edited Loading

yihuang commented Feb 21, 2020

linfeng-crypto commented Feb 21, 2020

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau commented Feb 24, 2020

bors bot commented Feb 24, 2020

Build succeeded

linfeng-crypto Feb 19, 2020 •

edited

Loading

yihuang Feb 20, 2020 •

edited

Loading

leejw51crypto Feb 20, 2020 •

edited

Loading

yihuang Feb 24, 2020 •

edited

Loading

codecov bot commented Feb 19, 2020 •

edited

Loading

leejw51crypto commented Feb 19, 2020 •

edited

Loading

leejw51crypto commented Feb 19, 2020 •

edited

Loading

yihuang commented Feb 20, 2020 •

edited

Loading

leejw51crypto Feb 20, 2020 •

edited

Loading

linfeng-crypto commented Feb 21, 2020 •

edited

Loading