fix(HD-Wallet): store & load correct HD Accounts #2537

mariocynicys · 2025-07-18T11:11:14Z

This PR makes it so that HDPathToCoin is a fieled of HDWalletCoinStorage and integrates it inside the databases.
HDWalletCoinStorage used to represent a selective set of the DB rows that match the same hd_wallet_rmd160 and coin (ticker).
This has changed to hd_wallet_rmd160 and purpose and coin_type (since purpose + coin_type define the coin uniquely*).
coin field is still stored in the DB but is not used. This is more for convenience.

This PR at the current state will purge your HD Wallet DB via a migration when run, so take a backup of it before running this code.
Will discuss a mitigation (easy for native, a little harder/tedious for wasm) for this side-effect in the review.

*: This isn't entirely true. All test coins use the same coin_type = 1.

How to test this (please backup your `MM2-shared.db` DB first):

Enable a coin in HD mode.
- A migration should be ran that will delete the old data for this coin. This means that a the coin will have no accounts and a new one will be generated.
- If using HW wallets, this means that the user will be prompted to share the xpub for the first account.
Any call that involves HD wallet storage should use the newly added purpose & coin_type in the DB selector/updater query (this doesn't really have any effect from the user facing side). So methods like create_new_account and generate_new_address should do such things.

supersedes #2482
closes #2470

as it's gonna be used as the identifier for the wallet instead of coin ticker

update the schema to include purpose and coin_type integers. currently also the same migration deletes the hd_account table where purpose=0 (and since purpose was just added, then the migration deletes all rows). we can rethink that before merging this PR. we can leave a leehway where marking purpose=0 means that this entry is carried over from an old mm2 version and we keep waiting until the user ever decides to enable the coin and we check the xpub and find it the one matching in the DB, only then do we assign the purpose and coin values to the correct values gotten from the recent enablement. this solution ofc doesn't work for HW wallets, otherwise we gonna ask HW wallets to derive xpubs all the time which is the bad UX we tryna avoid

this clears the object store, deletes the indexes and reinitailzes the new indexes with the proper fields using the same indexes names.

… hd_wallet_storage

mariocynicys · 2025-07-18T11:32:46Z

Quoting myself (from DMs):

second, we will use the fact that both coins got the same ticker name and delete all the entries for m/83'/0' (just liked the thing i did in the other PR about bad accounts; will check such inconsistent condition while loading the accounts; just to have a cleaner DB; but it ofc doesn't hurt if we keep those bad accounts inside the DB)

@shamardy this PR doesn't do that. I opted to not do that for perf reasons. not a strong opinion tho, so we can impl this if you want.

mariocynicys · 2025-07-24T19:26:14Z

heck no... not this

shamardy · 2025-07-30T13:45:24Z

This PR at the current state will purge your HD Wallet DB via a migration when run, so take a backup of it before running this code.
Will discuss a mitigation (easy for native, a little harder/tedious for wasm) for this side-effect in the review.

@mariocynicys can you please write a test case for @smk762 to test this and know how GUIs should handle this.

shamardy · 2025-07-30T13:46:14Z

*: This isn't entirely true. There is one case where two coins share purpose/coin_type derivation path, namely, BTC-testnet & LTC-testnet. I didn't want to involve the coin field as a selector just for this as this feels very redundant. The downside for that though, is that BTC-testnet and LTC-testnet are co-mingled among each other in the DB.
Just in case you are looking, no, we don't have LTC-testnet in coins file.

But all testnet coins should use the same coin_type so it's not only these 2.

mariocynicys · 2025-07-30T15:12:26Z

But all testnet coins should use the same coin_type so it's not only these 2.

thanks! didn't know that.
i will add the coin name back as part of the selection criterion in the queries then.

mariocynicys · 2025-07-31T13:53:35Z

discussed over DM with @shamardy:

we can set the purpose and coin_type to -1 be default and correct them when the coin is enabled and we have the seed (or user with HW wallet) at hand that we can derive the xpubs from.
we might wanna scan for the addresses in each account and if some account has some active addresses then we correct the purpose and coin_type for that account, otherwise, we delete that account.
this has a better UX properties for HW wallets, as they won't have to confirm every xpub in the DB but only the ones which have some addresses that have some balances.

same db space

this step is supposed to look for bad accounts and fix them to (replace them by) good accounts by setting the correct purpose and coin_types. a bad account is fixed into a good one only if the derived xpub matches the one stored in the bad account. if they don't match, this means the current purpose and coin_type of the coin don't match the ones who were used to create the bad account. this commit has compilation errors for multiple reasons. but still separating it away to be coherent. fixing the compilation errors in the next commits

as it has more (friendly) methods to deal with data inside the object store

because it was infunctionally written (doesn't wait on the IDBRequest to complete) and it already exists in IdbObjectStoreImpl that was added inside the table upgrader the previous commit

mariocynicys · 2025-08-18T13:18:25Z

im still working on creating the ability to do updates while on_upgarde_needed (analogous to running sql queries that does updates (sets some values) while migrating). so wasm migration isn't entirely done.

they keep spitting annoying warnings :/

this looks unsafe compilation-wise due to the Send + Sync unsafe impls on DbUpgrader. but in reality this should be fine since the upgrades are being ran in a single threaded fashion (one after the other)

shamardy

Next review iteration!

shamardy · 2025-08-27T15:29:23Z

mm2src/coins/hd_wallet/storage/sqlite_storage.rs

+const DB_VERSION: u32 = 1;
+
+const CREATE_MIGRATIONS_TABLE: &str =
+    "CREATE TABLE IF NOT EXISTS migration (current_migration INTEGER NOT_NULL UNIQUE);";


NOT_NULL should be NOT NULL

shamardy · 2025-08-27T15:44:03Z

mm2src/crypto/src/standard_hd_path.rs

+    pub fn default_for_test(coin_type: u32) -> HDPathToCoin {
+        HDPathToCoin {
+            value: Bip32PurposeValue {
+                purpose: Bip43Purpose::Bip84,


Why would Bip84 be default for tests?

I want some default value for tests to use it here: https://github.com/KomodoPlatform/komodo-defi-framework/blob/629fdeabb69e815bb9bd0078d731468f27c2b76b/mm2src/coins/hd_wallet/storage/mod.rs#L249

but also want the coin_type to be variable, that's why this doesn't fit as a impl Default.
i might just call it for_test instead?
as for why Bip84 specifically: it's not a default but maybe later we will add it as a parameter.

shamardy · 2025-08-27T16:05:07Z

mm2src/coins/hd_wallet/storage/wasm_storage.rs

+                    // FIXME: Here instead of clearing the table, you need to access the table and set every purpose and coin_type field to -1.
+                    // let items = table.object_store_impl.get_all_items().await; (we need this current method to be async)


When are you planning to work on this FIXME?

done :D
we can now do data-manipulation migrations on the fly through on_upgrade_needed
this could be utilized in saved_swaps 3819d8e (left a todo for it)

we also don't really need the Send, we just need the Sync to be able to Send a reference of DbUpgrader

add the datastore mutation code on upgrade callback. this still fails though because these calls aren't Send which on_upgrade_needed needs them to be

looks like we never needed the Send requirement from BoxFuture after all :)

including the Sync where the reference was needed to be Send

github-actions · 2025-11-30T12:01:15Z