641 Validate Mnemonic Words on Wallet Import

[zmjohnso]

Fixes #641.

To check for each word, I simply added the bip39 javascript package to the project to get the full wordlists, then check that the mnemonic word input is contained within the list. I've defaulted the check against the English word list since it seems that the joinmarket api also defaults to the English words. There might be a better way to do this? I'm not exactly sure how the jm api is doing the check.

After:

[kristapsk]

I've defaulted the check against the English word list since it seems that the joinmarket api also defaults to the English words. There might be a better way to do this? I'm not exactly sure how the jm api is doing the check.

JoinMarket is hardcoded to English wordlist, it uses old code from Electrum. https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/239ac4689a61e769fa17f8e9ef2688b835d0748b/src/jmclient/old_mnemonic.py

[theborakompanioni]

Hey @zmjohnso! Nice 🚀

My 2 sats:

• With the current implementation, all wordlists will end up in the bundle, when only English is needed: What do you think about not using an external library and plainly copying the english wordlist?
• Let's not prevent users from using a word not included in the list - a word should not be seen as "invalid" just because it is not defined in bip39. Instead, a hint or warning (e.g. orange border) would be nice. Thoughts?

[zmjohnso]

Thanks for that reference @kristapsk! But I am still a bit confused how jm is doing the check, since that word list does not seem to contain everything in the bip39 wordlist, i.e., "abandon". I will try to understand this better. Thanks again.

@theborakompanioni Yes, I completely agree with your first point. I have added a hardcoded version of the bip39 wordlist and removed the bip39 package. On your second point: currently the user can still continue through the workflow to import a wallet even if the UI shows the word as "invalid". Then after reviewing the words and clicking the final "import" button, the jm api will throw an error if it is not a valid mnemonic phrase. I think this workflow currently makes sense as is. We should allow the user to continue and ultimately this just serves as a visual check if the user has a typo, but the final validation happens on the api.
So the api determines validity and I'm not sure a different/separate visual indicator is necessary. But definitely open to more thoughts on this.

[kristapsk]

Thanks for that reference @kristapsk! But I am still a bit confused how jm is doing the check, since that word list does not seem to contain everything in the bip39 wordlist, i.e., "abandon". I will try to understand this better. Thanks again.

From API perspective you either give completely correct world list or get error back.

[theborakompanioni]

@theborakompanioni Yes, I completely agree with your first point. I have added a hardcoded version of the bip39 wordlist and removed the bip39 package. On your second point: currently the user can still continue through the workflow to import a wallet even if the UI shows the word as "invalid". Then after reviewing the words and clicking the final "import" button, the jm api will throw an error if it is not a valid mnemonic phrase. I think this workflow currently makes sense as is. We should allow the user to continue and ultimately this just serves as a visual check if the user has a typo, but the final validation happens on the api. So the api determines validity and I'm not sure a different/separate visual indicator is necessary. But definitely open to more thoughts on this.

Very reasonable! 👍

Only little nit regarding the code is that now the isValid property of MnemonicWordInput does not work "as expected" and might surprise callers (i.e. validation is currently done outside the component and e.g. passing isValid := true can lead to the input ending up being displayed as "invalid"). What about adding an additional component Bip39MnemonicWordInput that decorates the MnemonicWordInput with validation - do you think that would make it clearer?

Otherwise, let's merge this soon! Thank you so much @zmjohnso Great work. Simple and clean.

[zmjohnso]

@theborakompanioni So I added the Bip39MnemonicWordInput decorator component which wraps MnemonicWordInput and just does the isValid check for if the value is in the bip39 word list. I think this is cleaner and allows the MnemonicWordInput to remain unchanged and thus callers of this component can expect the same behavior as before. I think this does make things a bit more clear, but please let me know if this isn't what you meant.

[theborakompanioni]

@theborakompanioni So I added the Bip39MnemonicWordInput decorator component which wraps MnemonicWordInput and just does the isValid check for if the value is in the bip39 word list. I think this is cleaner and allows the MnemonicWordInput to remain unchanged and thus callers of this component can expect the same behavior as before. I think this does make things a bit more clear, but please let me know if this isn't what you meant.

Made some small adaptions (taking into account the isValid given by the caller). Looks good to me! Will merge soon.