-
Notifications
You must be signed in to change notification settings - Fork 25
Add mnemonic support #678
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
Conversation
4ab91a9 to
43b369e
Compare
2d11500 to
07b6668
Compare
e24d64d to
0d56679
Compare
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 made a first pass. I want to look at it again before approving.
0d56679 to
3192ab8
Compare
6f196f5 to
0d0b98a
Compare
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. I'm leaving approval to @Jimbo4350
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.
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 👍
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 👏 |
|
@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. |
46e4f70 to
98c6372
Compare
98c6372 to
72761e4
Compare
|
This PR is stale because it has been open 45 days with no activity. |
72761e4 to
94a7857
Compare
94a7857 to
e058da8
Compare
e058da8 to
bc7c5f0
Compare
Changelog
Context
Depends on release of
cardano-addresses(IntersectMBO/cardano-addresses#279)Allows IntersectMBO/cardano-cli#975.
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-addressesrelease) the payment is not necessary, so I plan to set it to()in those.Also make sure the documentation is good enough.
Checklist