fix(crypto): allow non bip39 mnemonics storage #2312

borngraced · 2025-01-08T13:57:46Z

Fixes an issue where passphrases were incorrectly validated as BIP39 mnemonics during storage decryption. Now, passphrases no longer require mnemonic validation.

…oPlatform/komodo-defi-framework into remove-unnecessary-mnemonic-checks

mariocynicys · 2025-01-08T15:42:30Z

for my money, we should do it the other way around. i.e. not allow a non bip-39 seed. and not support iguana in seed storage.
we should retire iguana already, it's very old for getting new world features :/

gcharang · 2025-01-08T15:51:19Z

we can't deprecate support for iguana seed phrases ever, imo

a user from last year can come back in 2030 and expect their seed phrase to work. I think we need to support at least wallet operations with iguana seed phrases forever.

maybe put iguana support behind a special flag/config option? and have the strict checks for valid bip39/bip44 seed phrases

borngraced · 2025-01-08T16:19:19Z

we can't deprecate support for iguana seed phrases ever, imo

a user from last year can come back in 2030 and expect their seed phrase to work. I think we need to support at least wallet operations with iguana seed phrases forever.

maybe put iguana support behind a special flag/config option? and have the strict checks for valid bip39/bip44 seed phrases

Yeah we could required an additional flag for seed_type ∈ {bip39, bip44, bip32, custom, ...}

This is out of scope for this PR anyways.

mariocynicys · 2025-01-08T16:19:27Z

a user from last year can come back in 2030 and expect their seed phrase to work. I think we need to support at least wallet operations with iguana seed phrases forever.

i would say such a user should dig up the internet for old clients that support iguana (or possibly build the old version themselves from source) to access their funds and send it over to an HD capable client, i.e. they self-migrate.
that's IMO is way more reasonable than having to maintain a very outdated standard and keep developing for their users.

borngraced · 2025-01-08T16:23:09Z

a user from last year can come back in 2030 and expect their seed phrase to work. I think we need to support at least wallet operations with iguana seed phrases forever.

i would say such a user should dig up the internet for old clients that support iguana (or possibly build the old version themselves from source) to access their funds and send it over to an HD capable client, i.e. they self-migrate. that's IMO is way more reasonable than having to maintain a very outdated standard and keep developing for their users.

While this won't be really helpful for most users who don't know what code is, this is still a good idea too.
We can provide a doc/guide how to do this if we end in this path of course

cipig · 2025-01-08T21:06:16Z

a user from last year can come back in 2030 and expect their seed phrase to work. I think we need to support at least wallet operations with iguana seed phrases forever.

i would say such a user should dig up the internet for old clients that support iguana (or possibly build the old version themselves from source) to access their funds and send it over to an HD capable client, i.e. they self-migrate. that's IMO is way more reasonable than having to maintain a very outdated standard and keep developing for their users.

Please don't. I want to be able to use my passphrase and also be able to import privkeys (WIFs), which now work fine, from all supported coins. I also don't want to move funds for 300 coins to some new addresses.

shamardy · 2025-01-09T11:20:41Z

for my money, we should do it the other way around. i.e. not allow a non bip-39 seed. and not support iguana in seed storage.

I guess making KDF generate bip39 mnemonics only is enough and for external seeds anything can be allowed. Allowing custom seeds with a config flag is a good idea but it's a breaking change.

shamardy · 2025-01-09T11:26:42Z

mm2src/crypto/src/mnemonic.rs

-    cross_test!(test_mnemonic_with_last_byte_zero, {
-        let mnemonic = "tank abandon bind salon remove wisdom net size aspect direct source fossil\0".to_string();
-        let password = "password";
+    cross_test!(test_encrypt_decrypt_words, {
+        let mnemonic = "Helloworld";
+        let password = "Helloworld";

        // Encrypt the mnemonic
-        let encrypted_data = encrypt_mnemonic(&mnemonic, password);
+        let encrypted_data = encrypt_mnemonic(mnemonic, password);
        assert!(encrypted_data.is_ok());
        let encrypted_data = encrypted_data.unwrap();

        // Decrypt the mnemonic
        let decrypted_mnemonic = decrypt_mnemonic(&encrypted_data, password);
-        assert!(decrypted_mnemonic.is_err());
+        assert!(decrypted_mnemonic.is_ok());
+        let decrypted_mnemonic = decrypted_mnemonic.unwrap();

-        // Verify that the error is due to parsing and not padding
-        assert!(decrypted_mnemonic
-            .unwrap_err()
-            .to_string()
-            .contains("mnemonic contains an unknown word (word 11)"));


I would like to keep this test as it was as it covers the case of making sure padding works fine. You can use Mnemonic::parse_normalized to make it work as before.

shamardy · 2025-01-09T11:50:56Z

Allowing custom seeds with a config flag is a good idea but it's a breaking change.

After some discussions with @mariocynicys , we should do that and also enable HD wallet by default even if it's a breaking change. We don't want new iguana users in the future. We can work on that on another PR.

laruh · 2025-01-09T11:56:20Z

Allowing custom seeds with a config flag is a good idea but it's a breaking change.

After some discussions with @mariocynicys , we should do that and also enable HD wallet by default even if it's a breaking change. We don't want new iguana users in the future. We can work on that on another PR.

Do we have export priv key rpc and such feature in mobile/web apps?
I remember the message that desktop has it.
What I want to tell kdf and apps on all platforms should provide export-import priv key functionality.

shamardy · 2025-01-09T12:03:00Z

What I want to tell kdf and apps on all platforms should provide export-import priv key functionality.

We don't have it when hd wallet is enabled, we should add it as any number of custom addresses unrelated to the HD tree of accounts and addresses. Can you please open an issue for it @laruh?

laruh · 2025-01-09T12:13:30Z

What I want to tell kdf and apps on all platforms should provide export-import priv key functionality.

We don't have it when hd wallet is enabled, we should add it as any number of custom addresses unrelated to the HD tree of accounts and addresses. Can you please open an issue for it @laruh?

Could you tell methods which export-import privkey for Iguana please? I cant find it in dispatcher files.
UPD: found show_priv_key in legacy. "import" is left

shamardy · 2025-01-09T12:44:33Z

Could you tell methods which export-import privkey for Iguana please? I cant find it in dispatcher files.
UPD: found show_priv_key in legacy. "import" is left

There is no per coin import of private keys since iguana only supports one address per coin from the seed mm2 started with.

shamardy

LGTM! Only one nit.

mm2src/crypto/src/mnemonic.rs

* dev: (35 commits) fix(crypto): allow non bip39 mnemonics storage (KomodoPlatform#2312) fix(legacy_swap): check for existing maker/taker payment before timeout (KomodoPlatform#2283) feat(tendermint): validators RPC (KomodoPlatform#2310) chore(CI): validate Cargo lock file (KomodoPlatform#2309) test(P2P): add test for peer time sync validation (KomodoPlatform#2304) fix mm2_p2p dev build (KomodoPlatform#2311) update Cargo.lock (KomodoPlatform#2308) chore(CI): unlock wasm-pack version (KomodoPlatform#2307) add `wasm` feature on WASM for timed-map (KomodoPlatform#2306) replace broken rpc link (KomodoPlatform#2305) chore(eth-websocket): remove some unnecessary wrappers (KomodoPlatform#2291) improvement(CI): switch to proper rust caching (KomodoPlatform#2303) fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls (KomodoPlatform#2295) chore(ci): Update docker build for wasm (KomodoPlatform#2294) chore(p2p): follow-up nits (KomodoPlatform#2302) feat(p2p): ensure time synchronization in the network (KomodoPlatform#2255) bump libp2p (KomodoPlatform#2296) chore(adex-cli): use "Komodo DeFi Framework" name in adex_cli (KomodoPlatform#2290) chore(ctx): replace gstuff constructible with oncelock (KomodoPlatform#2267) don't rely on core (KomodoPlatform#2289) ...

borngraced added 3 commits January 8, 2025 14:29

allow an words to be used for mnemonics

31efc2c

allow an words to be used for mnemonic

5b0ea49

Merge branch 'remove-unnecessary-mnemonic-checks' of github.com:Komod…

0288d6b

…oPlatform/komodo-defi-framework into remove-unnecessary-mnemonic-checks

borngraced requested a review from shamardy January 8, 2025 13:57

borngraced self-assigned this Jan 8, 2025

borngraced added the status: pending review label Jan 8, 2025

shamardy reviewed Jan 9, 2025

View reviewed changes

laruh mentioned this pull request Jan 9, 2025

Export-Import priv key functionality for HD mode #2314

Open

revert test_mnemonic_with_last_byte_zero

488b919

shamardy previously approved these changes Jan 9, 2025

View reviewed changes

mm2src/crypto/src/mnemonic.rs Outdated Show resolved Hide resolved

rename test_encrypt_decrypt_words test name

d52cf50

borngraced dismissed shamardy’s stale review via d52cf50 January 9, 2025 14:53

shamardy approved these changes Jan 9, 2025

View reviewed changes

shamardy changed the title ~~fix(seed-storage): passphrase doesn't have to be mnemonic validated~~ fix(crypto): allow non bip39 mnemonics storage Jan 9, 2025

shamardy merged commit 405bcb7 into dev Jan 9, 2025
20 of 25 checks passed

shamardy deleted the remove-unnecessary-mnemonic-checks branch January 9, 2025 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(crypto): allow non bip39 mnemonics storage #2312

fix(crypto): allow non bip39 mnemonics storage #2312

borngraced commented Jan 8, 2025

mariocynicys commented Jan 8, 2025

gcharang commented Jan 8, 2025 •

edited

Loading

borngraced commented Jan 8, 2025 •

edited

Loading

mariocynicys commented Jan 8, 2025

borngraced commented Jan 8, 2025 •

edited

Loading

cipig commented Jan 8, 2025

shamardy commented Jan 9, 2025

shamardy Jan 9, 2025

borngraced Jan 9, 2025

shamardy commented Jan 9, 2025

laruh commented Jan 9, 2025

shamardy commented Jan 9, 2025 •

edited

Loading

laruh commented Jan 9, 2025 •

edited

Loading

shamardy commented Jan 9, 2025

shamardy left a comment

fix(crypto): allow non bip39 mnemonics storage #2312

fix(crypto): allow non bip39 mnemonics storage #2312

Conversation

borngraced commented Jan 8, 2025

mariocynicys commented Jan 8, 2025

gcharang commented Jan 8, 2025 • edited Loading

borngraced commented Jan 8, 2025 • edited Loading

mariocynicys commented Jan 8, 2025

borngraced commented Jan 8, 2025 • edited Loading

cipig commented Jan 8, 2025

shamardy commented Jan 9, 2025

shamardy Jan 9, 2025

Choose a reason for hiding this comment

borngraced Jan 9, 2025

Choose a reason for hiding this comment

shamardy commented Jan 9, 2025

laruh commented Jan 9, 2025

shamardy commented Jan 9, 2025 • edited Loading

laruh commented Jan 9, 2025 • edited Loading

shamardy commented Jan 9, 2025

shamardy left a comment

Choose a reason for hiding this comment

gcharang commented Jan 8, 2025 •

edited

Loading

borngraced commented Jan 8, 2025 •

edited

Loading

borngraced commented Jan 8, 2025 •

edited

Loading

shamardy commented Jan 9, 2025 •

edited

Loading

laruh commented Jan 9, 2025 •

edited

Loading