-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add mnemonic sentence support #975
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I want feedback from the rest of the team before we approve and merge.
@@ -205,6 +207,19 @@ runNonExtendedKeyCmd | |||
writeLazyByteStringFile vkf' $ | |||
textEnvelopeToJSON descr vk | |||
|
|||
runGenerateMnemonicCmd :: Cmd.KeyGenerateMnemonicCmdArgs -> ExceptT KeyCmdError IO () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when writing to file we should at least set restrictive permissions. Can you get in touch with the wallet team and ask them what precautions they took when generating mnemonics?
@@ -1,6 +1,8 @@ | |||
Usage: cardano-cli key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we should have a new command root-key
that has these commands underneath. We need to make a clear distinction between the root key and the derived keys. Putting the commands under key
makes this distinction a little less clear IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far we are not really letting the user handle root keys, we are deriving normal extended keys directly from the mnemonic. But if we plan on adding support for deriving root keys too, and/or other commands specific for root keys then it would make sense.
Changelog
Context
This PR is associated to: IntersectMBO/cardano-api#678.
It is part of an effort to integrate
cardano-addresses
functionalities into thecardano-cli
.How to trust this PR
I would look mainly at the logic, the changes in the help golden files, and probably testing it is the best way.
Checklist