FourthState / plasma-mvp-sidechain

Cosmos SDK (Tendermint consensus) side-chain implementation for plasma
Apache License 2.0
112 stars 35 forks source link

plasmacli key exporting (#40) #183

Closed wesgraham closed 4 years ago

wesgraham commented 4 years ago

Fixes #40

Export to file using ./plasmacli keys export

codecov-io commented 4 years ago

Codecov Report

Merging #183 into develop will increase coverage by 0.27%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #183      +/-   ##
===========================================
+ Coverage    51.75%   52.02%   +0.27%     
===========================================
  Files           28       28              
  Lines         1314     1359      +45     
===========================================
+ Hits           680      707      +27     
- Misses         529      538       +9     
- Partials       105      114       +9     

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 77fdee3...ba23add. Read the comment docs.

wesgraham commented 4 years ago

Can we adjust the import command, such that when given a file, it imports a geth keyfile instead of a raw private ke

Yes will work on now.

wesgraham commented 4 years ago

@colin-axner would appreciate your insights on normalizing everything. I kept the ability to import strictly a private key, but now the --file flag imports a geth-keyfile. Is there a better way to do this?

colin-axner commented 4 years ago

Can we adjust the import command, such that when given a file, it imports a geth keyfile instead of a raw private key file?

I think the default behavior should be using a raw private key file. From geth account import --help:

Imports an unencrypted private key from <keyfile> and creates a new account.
Prints the address.

Our default behavior should line up with geth imo, since that is probably the primary usage for importing/exporting keys.

Therefore we should add a separate flag such as encrypted with a short hand like -e or something which is a bool and then is treated as an encrypted keyfile.

Furthermore, the import command was implemented by someone else who had a need for it, so that suggests to me that importing a raw keyfile was particularly useful

Also from the geth command, so importing/exporting an encrypted keyfile is sugar for copy/paste into/from hidden directory:

Note:
As you can directly copy your encrypted accounts to another ethereum instance,
this import mechanism is not needed when you transfer an account between
nodes.
wesgraham commented 4 years ago

Therefore we should add a separate flag such as encrypted with a short hand like -e or something which is a bool and then is treated as an encrypted keyfile.

Got it - will add a -e flag for now. We can also remove this functionality if desired

hamdiallam commented 4 years ago

I was the one who originally implemented this i think. Importing a raw key file was definitely not used. i think we should default to geth's encrypted keyfile

colin-axner commented 4 years ago

I was the one who originally implemented this i think. Importing a raw key file was definitely not used. i think we should default to geth's encrypted keyfile

72 was the original pr. I don't have strong preference on the default, but both file types should be supported.

hamdiallam commented 4 years ago

I was the one who originally implemented this i think. Importing a raw key file was definitely not used. i think we should default to geth's encrypted keyfile

72 was the original pr. I don't have strong preference on the default, but both file types should be supported.

Ah Got it. sgtm

hamdiallam commented 4 years ago

Few things when testing. Usage gets displayed on error which is annoying. You can set cmd.SilenceUsage = true in the pre-run ( still want it false while parsing arguments )

  1. Entering the wrong password on export still results "Successfully exported". Created and empty file

  2. When entering the passphrase for the exported key. The second prompt should on say Repeat passphrase, instead of the same exact prompt again, like how importing does.

wesgraham commented 4 years ago
  • Entering the wrong password on export still results "Successfully exported". Created and empty file

I just tested this and wasn't able to recreate the issue. I think the error case added in export.go resolved this problem.

  • When entering the passphrase for the exported key. The second prompt should on say Repeat passphrase, instead of the same exact prompt again, like how importing doe

The current flow is to display "Enter passphrase:", then "Enter new passphrase for exported key:", then "Repeat passphrase:" Is this correct?