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 support #678

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

Add mnemonic support #678

wants to merge 15 commits into from

Conversation

palas
Copy link
Contributor

@palas palas commented Nov 12, 2024

Changelog

- description: |
    Added support for generating mnemonics and for deriving payment and stake keys from mnemonics.
  type:
  - feature

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

I would look at the tests. The hard-coded tests have been checked against existing wallets.
I have also tested that the derivation works by doing a prototype command in the cardano-cli.
Make sure you like the changes to the API. I have tried to keep them minimal.
Probably the most controversial bit are the instances I added, but they are not exposed. The reason I parametrised the index type is that for some types of keys that I haven't added yet (because I am waiting for a cardano-addresses release) the payment is not necessary, so I plan to set it to () in those.
Also make sure the documentation is good enough.

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

@palas palas force-pushed the add-mnemonic-support branch 2 times, most recently from 2d11500 to 07b6668 Compare November 13, 2024 11:20
@palas palas force-pushed the add-mnemonic-support branch 2 times, most recently from ba4773c to e24d64d Compare November 14, 2024 11:57
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.

I made a first pass. I want to look at it again before approving.

cardano-api/internal/Cardano/Api/Keys/Mnemonics.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Keys/Mnemonics.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Keys/Mnemonics.hs Outdated Show resolved Hide resolved
cardano-api/internal/Cardano/Api/Keys/Mnemonics.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

LGTM. I'm leaving approval to @Jimbo4350

cardano-api/internal/Cardano/Api/Keys/Mnemonics.hs Outdated Show resolved Hide resolved
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.

This LGTM however I think it may be better as two different type classes so we can avoid using () and an associated type.

What do you think about class SigningKeyFromRootKey keyrole and class IndexedSigningKeyFromRootKey keyrole?

After we decide on this, clean up the commit history 👍

@smelc
Copy link
Contributor

smelc commented Nov 18, 2024

This LGTM however I think it may be better as two different type classes so we can avoid using () and an associated type.

While I salute the effort to keep one interface that you did @palas, I agree with @Jimbo4350's suggestion here. I think it'll make the API clearer.

Also big kudos on the large amount of documentation written in the new code 👏

@palas
Copy link
Contributor Author

palas commented Nov 19, 2024

@Jimbo4350 @smelc So we would need two functions right? Another option I considered was having an algebraic data type as a parameter with all the info. Let me know if you prefer that.

@Jimbo4350
Copy link
Contributor

@Jimbo4350 @smelc So we would need two functions right? Another option I considered was having an algebraic data type as a parameter with all the info. Let me know if you prefer that.

I think two separate classes is better here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants