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

Add mnemonic sentence support #975

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add mnemonic sentence support #975

wants to merge 14 commits into from

Conversation

palas
Copy link
Contributor

@palas palas commented Nov 20, 2024

Changelog

- description: |
    Added support for mnemonic sentence generation and extended key derivation from mnemonic sentences.
  type:
  - feature

Context

This PR is associated to: IntersectMBO/cardano-api#678.
It is part of an effort to integrate cardano-addresses functionalities into the cardano-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

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a 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.

cardano-cli/src/Cardano/CLI/Commands/Key.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Commands/Key.hs Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Commands/Key.hs Outdated Show resolved Hide resolved
cardano-cli/test/cardano-cli-golden/files/golden/help.cli Outdated Show resolved Hide resolved
cardano-cli/test/cardano-cli-golden/files/golden/help.cli Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Options/Key.hs Outdated Show resolved Hide resolved
@@ -205,6 +207,19 @@ runNonExtendedKeyCmd
writeLazyByteStringFile vkf' $
textEnvelopeToJSON descr vk

runGenerateMnemonicCmd :: Cmd.KeyGenerateMnemonicCmdArgs -> ExceptT KeyCmdError IO ()
Copy link
Contributor

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?

cardano-cli/src/Cardano/CLI/Run/Key.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Run/Key.hs Outdated Show resolved Hide resolved
@@ -1,6 +1,8 @@
Usage: cardano-cli key
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@palas palas added next-update Needs a dependency to be updated to be merged and removed next-update Needs a dependency to be updated to be merged labels Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants