Interface improvements

[andrewkozlik]

There are some technical improvements to the interface that we might want to consider based on my experience with integrating SLIP39 into Electrum. Below is a list of the modifications that I made in the Electrum implementation. Come to think of it, I actually took some of the code from the Trezor implementation.

1. I bundled everything into one file, taking only the functions which are needed for seed recovery.
   • I didn't want to introduce too many new files into Electrum. After all, in Trezor we also have most of the stuff in one file.
   • I didn't want to introduce dead code (Electrum will not support generating SLIP-39 shares).
2. Used Electrum's native Wordlist module to load and use the wordlist. IIRC Electrum uses this for word hints, so I didn't want to load the wordlist into memory twice. Also added a get_wordlist() function so that the wordlist is loaded only when needed.
3. Rewrote most strings to use the electrum.i18n _() function, but now I see I missed a few spots. Also shortened some error strings so that they would fit more nicely into the UI.
4. Renamed MnemonicError to Slip39Error, which seems more accurate.
5. Introduced an EncryptedSeed class and moved the decrypt function there. The recover_ems() function returns EncryptedSeed, which also carries information about the identifier and interation exponent. This is easier to work with if you want to allow the user to supply the passprase later.
6. recover_ems discards groups that don't meet the member threshold and allows extra shares. In python-shamir-mnemonic if the user provides extra shares, then we fail, which seems like a pointless hassle.
7. Introduced a process_mnemonics() function which formulates the status of the mnemonics and recovers the EMS if all required mnemonics are provided. Pretty much what cli.py does.
8. In some places we used threshold, which is ambiguous. Changed to member_threshold.

Not sure whether it makes sense to do anything about 1, 2 and 3 here, but I think the remaining points would be useful to apply here too. Introducing process_mnemonics() seems handy if we can do it in a non-application-specific manner.

[matejcik]

i'm implementing a RecoveryState class which is an abstraction out of cli.py that is supposed to be reusable in the Qt component. This should resolve (7)

I would not touch MnemonicError, seeing as someone might already be using it?

(5), (6) and (8) seem reasonable. In addition to (5), I would also change the signature of recover_ems to accept Share objects instead of mnemonic strings

[andrewkozlik]

i'm implementing a RecoveryState class which is an abstraction out of cli.py that is supposed to be reusable in the Qt component. This should resolve (7)

Sounds good!

I would not touch MnemonicError, seeing as someone might already be using it?

I suppose we can leave it. But does it make a difference if we are already making changes to the other parts of the interface?

(5), (6) and (8) seem reasonable. In addition to (5), I would also change the signature of recover_ems to accept Share objects instead of mnemonic strings

I'll take care of (6) and (8) and look at (5).

[matejcik]

I suppose we can leave it. But does it make a difference if we are already making changes to the other parts of the interface?

we really aren't; the original generate_mnemonics and combine_mnemonics APIs are unchanged from the start

[matejcik]

note that the version that is currently out is 0.1.0 which is the old single-file implementation. recover_ems was not released yet

[andrewkozlik]

(8) is done, created a branch: https://github.com/trezor/python-shamir-mnemonic/tree/interface-improvements

I just noticed a bug in the CLI:

$ shamir recover
Enter a recovery share: credit deal academic agency earth slim manual prune fraction training cleanup intimate blue senior vampire tackle disease laundry class standard

⛬ 1 of 2 shares needed from group credit deal academic
Enter a recovery share: shadow pistol academic always adequate wildlife fancy gross oasis cylinder mustang wrist rescue view short owner flip making coding armed
ERROR: This mnemonic is not part of the current set. Please try again.

✓ 2 of 2 shares needed from group shadow pistol academic
ERROR: Invalid set of mnemonics. All mnemonics must begin with the same 2 words.
Recovery failed

This is clearly wrong:

✓ 2 of 2 shares needed from group shadow pistol academic

Also, the 26EC unicode character which is user to indicate "in progress" doesn't show up on my Ubuntu installation. Perhaps a more universally used symbol would be more preferable.

[matejcik]

Perhaps a more universally used symbol would be more preferable.

i'm open to suggestions :) "done" and "not done" is pretty clear but "in progress" ?

[andrewkozlik]

Perhaps a more universally used symbol would be more preferable.

i'm open to suggestions :) "done" and "not done" is pretty clear but "in progress" ?

In Electrum I used 26ab (medium black circle) in orange color. I am pretty much open to anything as long as the symbol is available in the majority of system fonts.