Add support for importing wallets using Electrum mnemonic phrases

Ghost commented

2023-10-18 20:37:20 +02:00

(Migrated from bitcoincashcode.org)

This is the format used by (older) Electron Cash wallets. It may be useful to users wishing to use their older EC wallets with Flowee Pay (such as myself).

Highlight of changes:

Import wallet UI now auto-detects whether it's Electrum or BIP39. Note that in some cases a valid BIP39 seed is also a valid Electrum seed. The user has a UI element checkbox for "forcing" Electrum interpretation in such cases.
Electrum seed phrases always use derivation path "m/" so this is forced on the user as the only path available in the Electrum case.
Wallet file format has a new tag to indicate whether the seed phrase is electrum or not. Only present in the electrum case, otherwise it's omitted from the serialized data.

Both desktop and mobile UIs were updated.

Note: This requires Flowee/thehub#5 be merged.

This is the format used by (older) Electron Cash wallets. It may be useful to users wishing to use their older EC wallets with Flowee Pay (such as myself). Highlight of changes: - Import wallet UI now auto-detects whether it's Electrum or BIP39. Note that in some cases a valid BIP39 seed is also a valid Electrum seed. The user has a UI element checkbox for "forcing" Electrum interpretation in such cases. - Electrum seed phrases always use derivation path "m/" so this is forced on the user as the only path available in the Electrum case. - Wallet file format has a new tag to indicate whether the seed phrase is electrum or not. Only present in the electrum case, otherwise it's omitted from the serialized data. Both desktop and mobile UIs were updated. Note: This requires Flowee/thehub#5 be merged.

Ghost commented

2023-10-18 22:22:55 +02:00

(Migrated from bitcoincashcode.org)

Looking at this from the UX point of view:

user imports his seed phrase. The software checks validity and gives a thumbs up. In your changes it now also checks for electrum and gives a slightly different thumbs up.
Either way, in 99.997% of the cases it automatically imports without user interaction.

It is helpful to show that it detects an electrum type, because if the imported one falls in the 'other' category where it doesn't get correctly detected the user needs to detect that and check the checkbox. For no other reason than to make sure that the derivation path is set propertly.

After import is done, I fail to see any reason why Flowee Pay needs to hold onto the 'is electrum' boolean. It already shares the derivation path on the backup screen, which is implicitly the result of that boolean.
The actual conversion from mnemonic to hdmasterkey doesn't need this bool.

The fact that this is now automatically detected means there is no need for the user to 'backup' this boolean. So why keep it in the wallet?

Looking at this from the UX point of view: user imports his seed phrase. The software checks validity and gives a thumbs up. In your changes it now also checks for electrum and gives a slightly different thumbs up. Either way, in 99.997% of the cases it automatically imports without user interaction. It is helpful to show that it detects an electrum type, because if the imported one falls in the 'other' category where it doesn't get correctly detected the user needs to detect that and check the checkbox. For no other reason than to make sure that the derivation path is set propertly. After import is done, I fail to see any reason why Flowee Pay needs to hold onto the 'is electrum' boolean. It already shares the derivation path on the backup screen, which is implicitly the result of that boolean. The actual conversion from mnemonic to hdmasterkey doesn't need this bool. The fact that this is now automatically detected means there is no need for the user to 'backup' this boolean. So why keep it in the wallet?

Ghost commented

2023-10-18 23:00:39 +02:00

(Migrated from bitcoincashcode.org)

Looking at changes in HDMasterkey, it looks like the boolean is needed to be persisted.

I did notice in Wallet_encryption.cpp that on line 250 there is another HDMasterKey::fromMnemonic() which may then need to have this bool added to the call.

Still, my UX point of view stands. I don't think that the UI has any need to show this boolean to the user after the import has completed.

Looking at changes in HDMasterkey, it looks like the boolean _is_ needed to be persisted. I did notice in Wallet_encryption.cpp that on line 250 there is another HDMasterKey::fromMnemonic() which may then need to have this bool added to the call. Still, my UX point of view stands. I don't think that the UI has any need to show this boolean to the user after the import has completed.

Ghost commented

2023-10-19 04:55:18 +02:00

(Migrated from bitcoincashcode.org)

After import is done, I fail to see any reason why Flowee Pay needs to hold onto the 'is electrum' boolean. It already shares the derivation path on the backup screen, which is implicitly the result of that boolean.
The actual conversion from mnemonic to hdmasterkey doesn't need this bool.

For symmetry and because knowing the format is a key parameter to being able to even use the seed in the first place. High-level view: wallet = restore(seed, format=bip39).

In the electrum case you definitely need to be told again your seed was Electrum seed. Otherwise you will have a sad/bad/surprising time.

Without telling the user what format the original seed was in, they may be surprised when they try that seed in other software and if fails (or worse, it happens to be pass the BIP-39 checksum and they get a different wallet on restore!!).

It's helpful to be told it again. Just as it's helpful to be told what derivation path they have, what their seed is in the first place, etc.

> After import is done, I fail to see any reason why Flowee Pay needs to hold onto the 'is electrum' boolean. It already shares the derivation path on the backup screen, which is implicitly the result of that boolean. > The actual conversion from mnemonic to hdmasterkey doesn't need this bool. For symmetry and because knowing the format is a key parameter to being able to even use the seed in the first place. High-level view: `wallet = restore(seed, format=bip39)`. In the electrum case you definitely need to be told again your seed was Electrum seed. Otherwise you will have a sad/bad/surprising time. Without telling the user what format the original seed was in, they may be surprised when they try that seed in other software and if fails (or worse, it happens to be pass the BIP-39 checksum and they get a different wallet on restore!!). It's helpful to be told it again. Just as it's helpful to be told what derivation path they have, what their seed is in the first place, etc.

Ghost commented

2023-10-19 05:07:12 +02:00

(Migrated from bitcoincashcode.org)

By the way if you want to try an electrum seed with the UI here is one:

walnut ginger silent client kitchen wife old close round hire danger scene

It even has history on it on BCH mainnet...

I don't have any seeds ready that I can share that happen to also match BIP39 checksums though.. but if you want one I can prepare one for you...

By the way if you want to try an electrum seed with the UI here is one: `walnut ginger silent client kitchen wife old close round hire danger scene` It even has history on it on BCH mainnet... I don't have any seeds ready that I can share that happen to also match BIP39 checksums though.. but if you want one I can prepare one for you...

Ghost commented

2023-10-19 17:19:35 +02:00

(Migrated from bitcoincashcode.org)

I did notice in Wallet_encryption.cpp that on line 250 there is another HDMasterKey::fromMnemonic() which may then need to have this bool added to the call.

Yep you're right. Fixed.

> I did notice in Wallet_encryption.cpp that on line 250 there is another HDMasterKey::fromMnemonic() which may then need to have this bool added to the call. > Yep you're right. Fixed.

Ghost commented

2023-10-19 17:21:45 +02:00

(Migrated from bitcoincashcode.org)

Ok, pushed a new commit that implements your requested changes + also uses new API (enum for MnemonicType), etc.

Note we are persisting that enum's value directly to the wallet now, no longer a bool. What do you think about that?

Ok, pushed a new commit that implements your requested changes + also uses new API (enum for MnemonicType), etc. Note we are persisting that enum's value directly to the wallet now, no longer a bool. What do you think about that?

Ghost commented

2023-10-19 17:27:42 +02:00

(Migrated from bitcoincashcode.org)

Force-pushed a rebase to master (to take in the latest fixes).

Ghost commented

2023-10-23 17:06:21 +02:00

(Migrated from bitcoincashcode.org)

Note we are persisting that enum's value directly to the wallet now, no longer a bool. What do you think about that?

Using one extra byte in the save-file in order to make the code clearer to read, sounds good to me.

> Note we are persisting that enum's value directly to the wallet now, no longer a bool. What do you think about that? Using one extra byte in the save-file in order to make the code clearer to read, sounds good to me.

Ghost commented

2023-10-24 08:56:02 +02:00

(Migrated from bitcoincashcode.org)

Note we are persisting that enum's value directly to the wallet now, no longer a bool. What do you think about that?

Using one extra byte in the save-file in order to make the code clearer to read, sounds good to me.

I definitely agree enums are better in general for this -- was just concerned about safety in the code like if someone modifies the enum values or ordering... maybe they need to be documented with a comment and also hard-coded as:

/// Note these exact values get serialized to the wallet file in Flowee Pay so don't modify their values, ... etc..
enum MnemonicType {
    BIIP39Mnemonic = 0,
    ElectrumMnemonic = 1,
};

> > Note we are persisting that enum's value directly to the wallet now, no longer a bool. What do you think about that? > > Using one extra byte in the save-file in order to make the code clearer to read, sounds good to me. I definitely agree enums are better in general for this -- was just concerned about safety in the code like if someone modifies the enum values or ordering... maybe they need to be documented with a comment and also hard-coded as: ```c++ /// Note these exact values get serialized to the wallet file in Flowee Pay so don't modify their values, ... etc.. enum MnemonicType { BIIP39Mnemonic = 0, ElectrumMnemonic = 1, }; ```

Ghost commented

2023-10-24 21:08:03 +02:00

(Migrated from bitcoincashcode.org)

Great idea, done;

TheHub: https://codeberg.org/Flowee/thehub/commit/1f57aef0df84bd8b9cdd0e96c0110f96881cf640
Pay: https://codeberg.org/Flowee/pay/commit/1cdd10ac6699e84272fb98b9c4e20a3e4149786d

Great idea, done; TheHub: https://codeberg.org/Flowee/thehub/commit/1f57aef0df84bd8b9cdd0e96c0110f96881cf640 Pay: https://codeberg.org/Flowee/pay/commit/1cdd10ac6699e84272fb98b9c4e20a3e4149786d

Ghost commented

2023-10-25 05:21:26 +02:00

(Migrated from bitcoincashcode.org)

Great idea, done;

TheHub: https://codeberg.org/Flowee/thehub/commit/1f57aef0df84bd8b9cdd0e96c0110f96881cf640
Pay: https://codeberg.org/Flowee/pay/commit/1cdd10ac6699e84272fb98b9c4e20a3e4149786d

Looks good to me!

> Great idea, done; > > TheHub: https://codeberg.org/Flowee/thehub/commit/1f57aef0df84bd8b9cdd0e96c0110f96881cf640 > Pay: https://codeberg.org/Flowee/pay/commit/1cdd10ac6699e84272fb98b9c4e20a3e4149786d Looks good to me!

Add support for importing wallets using Electrum mnemonic phrases #22