Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-key import feature #1201

Merged
merged 16 commits into from
Aug 12, 2021
Merged

Multi-key import feature #1201

merged 16 commits into from
Aug 12, 2021

Conversation

leviathanbeak
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • instead of reading the key from the command argument, we either read the path to the key, or if there is no path provided we read it from stdin (the same way lotus does it)

Reference issue to close (if applicable)
Closes #1129

Other information and links

Copy link
Contributor

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

I really like it! Thanks so much for your continued involvement on the project.

I left a couple nits, but while you're at it, do you think you could make it so this error returns a code other than 0?
Screenshot from 2021-08-05 08-24-46

Do you think you could also make these work? I'm a little rusty with my bash shell, but I think this is how you do this, right?
Screenshot from 2021-08-05 08-48-21

Key import from path doesn't seem to decode correctly? Or maybe it's string trim, not sure. Either way, it doesn't work.
Screenshot from 2021-08-05 08-56-09

From a user experience standpoint, I think we should also make it also print out the key on a successful import so the user knows which key they just imported. Right now no message is returned other than exit code 0.

Also, do you think you could make it trim the string input? If you accidentally add extra space to stdin when copy/pasting, that might be nice.

In summary:

  • Fix qualified std import nit
  • Make key import work with stdin in addition to read_password (if possible)
  • Fix path import decoding issue
  • Fix "key already exists" error code so returns other than 0
  • Print out key name after succesful import
  • Trim string input

I know it's a lot more work than originally specified, but that'd really help us out. That's whole point, right? And thanks again for your contributions so far!

forest/src/cli/wallet_cmd.rs Outdated Show resolved Hide resolved
forest/src/cli/wallet_cmd.rs Outdated Show resolved Hide resolved
@leviathanbeak
Copy link
Contributor Author

@cryptoquick thanks for the detailed review, I didn't expect it to be that easy anyway, will improve it

@cryptoquick
Copy link
Contributor

Haha, great!

@leviathanbeak
Copy link
Contributor Author

leviathanbeak commented Aug 12, 2021

In summary:

  • Fix qualified std import nit
  • Make key import work with stdin in addition to read_password (if possible)

fixed this by only updating rpassword lib, it was very, very outdated

  • Fix path import decoding issue

added trimming - should fix all the problems

  • Fix "key already exists" error code so returns other than 0

as discussed, the problem was in 2 places, fixed it

  • Print out key name after succesful import

added println!(key)

  • Trim string input

Copy link
Contributor

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

Good catch on the outdated rpassword crate! Just tried it out, it all works great. Thank you so much for your solid contributions!

Also, as mentioned in #fil-forest-dev in the Slack, we decided the error code change isn't needed for this PR.

@ec2 ec2 merged commit a134d5e into ChainSafe:main Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multi-key imports from file/stdin
3 participants