search

IntersectMBO / cardano-api

Cardano API
Apache License 2.0
26 stars 23 forks source link

Add mnemonic support #678

Open palas opened 2 weeks ago

palas commented 2 weeks ago

Changelog

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

Context

Depends on release of cardano-addresses (https://github.com/IntersectMBO/cardano-addresses/pull/279) Allows https://github.com/IntersectMBO/cardano-cli/pull/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-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

smelc commented 1 week ago

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 :clap:

palas commented 1 week ago

@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 commented 1 week ago

@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.