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

Discrepancy between keys add and keys new #2595

Closed
3 of 4 tasks
alexanderbez opened this issue Oct 25, 2018 · 15 comments
Closed
3 of 4 tasks

Discrepancy between keys add and keys new #2595

alexanderbez opened this issue Oct 25, 2018 · 15 comments
Assignees
Labels
C:CLI C:Keys Keybase, KMS and HSMs T: UX

Comments

@alexanderbez
Copy link
Contributor

alexanderbez commented Oct 25, 2018

The commands keys new (newly added) and keys add result in a somewhat confusing UX. I believe from #2091, we want to mark add as deprecated (which we can via Cobra). However, add has "recovery" functionality which new does not. How do we want to handle this?

Does it make sense to have have recovery functionality in keys new? Perhaps we can remove keys add all together and simply add a keys recover command.

/cc @ebuchman @jackzampolin


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez alexanderbez added C:CLI T: UX C:Keys Keybase, KMS and HSMs labels Oct 25, 2018
@alexanderbez alexanderbez changed the title Discrepancy between keys add and keys new. Discrepancy between keys add and keys new Oct 25, 2018
@alexanderbez alexanderbez self-assigned this Oct 25, 2018
@alessio
Copy link
Contributor

alessio commented Oct 25, 2018

In my opinion, we should have:

  • a generate-key command
  • a recovery command

The latter could be implemented just as a flag for the former.

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Oct 25, 2018

Why not then just add --recover to keys new and do away with keys add (which should be deprecated)?

@alessio
Copy link
Contributor

alessio commented Oct 25, 2018

I think generate is semantically more correct, though add does not bother me. Agreed on aggregating --recover 👍

@alexanderbez
Copy link
Contributor Author

Ok so rename new to generate and add the --recover flag? Thoughts @jackzampolin @ebuchman?

@jackzampolin
Copy link
Member

I would prefer not to use generate or new really. Unpopular opinion alert I think add describes the action best: you are adding keys to your database in all of the listed scenarios (--recover, generate/new). Thoughts?

@alexanderbez
Copy link
Contributor Author

Also, I don't think keys new prints the seed @ebuchman ?

@jackzampolin
Copy link
Member

@alexanderbez it does not.

@alexanderbez
Copy link
Contributor Author

Shouldn't it?

@jackzampolin
Copy link
Member

Yup!

@ebuchman
Copy link
Member

ebuchman commented Nov 6, 2018

Also, I don't think keys new prints the seed

Excellent and critical point! I don't have strong opinions here, other than that users should be able to enter mnemonic and bip39 passphrase interactively (ie. not recorded in shell history). As long as that's possible, I'm happy. Sorry for the mayhem on this one ...

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Nov 6, 2018

All good. I'll clean this up 👍

@alessio
Copy link
Contributor

alessio commented Nov 26, 2018

@jackzampolin dixit:

Unpopular opinion alert I think add describes the action best: you are adding keys to your database in all of the listed scenarios (--recover, generate/new). Thoughts?

Seconded. Let's keep add and drop new, shall we?

@alexanderbez
Copy link
Contributor Author

Actually I think the opposite? I think new is the newest command implemented by @ebuchman and we want to remove add. Also, new should support recovery.

@alessio
Copy link
Contributor

alessio commented Nov 26, 2018

How about removing add and renaming new to add?

@alexanderbez
Copy link
Contributor Author

I don't have strong opinions here.

@alessio alessio self-assigned this Nov 26, 2018
alessio pushed a commit that referenced this issue Nov 26, 2018
- Incorporate --recover in the new command.
- Code clean up

Closes: #2595
alessio pushed a commit that referenced this issue Nov 29, 2018
- Incorporate --recover in the new command.
- Code clean up

Closes: #2595
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:Keys Keybase, KMS and HSMs T: UX
Projects
None yet
Development

No branches or pull requests

4 participants