-
Notifications
You must be signed in to change notification settings - Fork 23
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
base: master
Are you sure you want to change the base?
Add mnemonic support #678
Conversation
4ab91a9
to
43b369e
Compare
2d11500
to
07b6668
Compare
ba4773c
to
e24d64d
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
Co-authored-by: Mateusz Galazyn <[email protected]>
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
Changelog
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