cardano-foundation / cardano-wallet

HTTP server & command-line for managing UTxOs and HD wallets in Cardano.

Apache License 2.0

764 stars 211 forks source link

Implement master key generation & public id calculation for Byron random #1501

Closed KtorZ closed 4 years ago

KtorZ commented 4 years ago

Context

As a Daedalus developer,
I want to be able to associate a mnemonic phrase to its corresponding wallet id,
So that I can implement recurring mnemonic checks to make sure users haven't forgotten their mnemonic sentences.

Decision

Wallet id can be derived from a JavaScript library depending on their type (byron, icarus, ledger).

Acceptance Criteria

Wallet id can be derived from a JavaScript library depending on their type (byron, icarus, ledger).
Wallet id generation is documented

Development

https://github.com/input-output-hk/cardano-js/pull/27

QA

Generation method documented as part of Wallet Cryptography and Encoding
Golden test cases generated using cardano-wallet: https://github.com/input-output-hk/cardano-js/blob/dc505f29fba0bbb7a055d184ac3e99c998047b34/src/hd/index.spec.ts#L3-L22

piotr-iohk commented 4 years ago

Perhaps that's nitpicking but there could be some negative tests added. Like:

using faulty 12-length mnemonics (for Byron) and faulty 15-length (for Icarus)
using different than 12-length mnemonics (shorter and longer) (i.e. 12 for icarus, 15 for byron)
using non-English 12-length/15-length mnemonics... (respectively Byron and Icarus)
etc.

KtorZ commented 4 years ago

@piotr-iohk for points 1 and 3, I am assuming that the https://github.com/bitcoinjs/bip39 is handling those since this is purely mnemonic manipulation. Although the module API takes a list of string, passing anything in there that isn't a valid bip39 mnemonic will simply blow up or give improper result, but that's basically the caller's fault.

For point 2, there's in practice, nothing preventing you to generate a seed using the Byron/random generation method with a 24 words mnemonic for instance. That would be a stupid thing to do, but there's nothing wrong with that per se. The cardano-js is a private repository used only by Daedalus and I assume they'll not do stupid thing with this :) ... We could adopt a more defensive approach though by adding some explicit assert about the mnemonic length to make sure that no one will actually do these stupid things.

piotr-iohk commented 4 years ago

passing anything in there that isn't a valid bip39 mnemonic will simply blow up or give improper result, but that's basically the caller's fault.

Right, so by it's nature a negative test would assure it blows up in expected way. ;)

The cardano-js is a private repository used only by Daedalus...

Perhaps it is not necessary then, although it is always better to be more conservative. Anyway, that's why I was wondering if it is not nitpicking... :man_shrugging:

KtorZ commented 4 years ago

Right, so by it's nature a negative test would assure it blows up in expected way. ;)

KtorZ commented 4 years ago

If you want to PR this, feel free ^.^ .. I am not sure I'll do anything about it today