feat(wallet): implement HD multi-address support for message signing by borngraced · Pull Request #2432 · GLEECBTC/komodo-defi-framework

borngraced · 2025-04-29T18:42:25Z

#2406
This PR allows users with HD wallet to sign messages from any derived address using derivation_path

sign_message rpc response params

Example before

{
  "coin": "ETH",
  "message": "hello world",
}

Example New

{
  "coin": "ETH",
  "message": "hello world"
}

Example New(HD account)

{
  "coin": "ETH",
  "message": "hello world",
  "address": {
    "derivation_path": "m/44'/0'/0'/0/2"
  }
}

OR

{
  "coin": "ETH",
  "message": "hello world",
  "address": {
    "account_id": 0,
    "chain": "External",
    "address_id": 1
  }
}

…r HDwallet, completed eth impl

mm2src/coins/lp_coins.rs

mariocynicys · 2025-04-30T18:01:18Z

mm2src/mm2_main/Cargo.toml

 sepolia-taker-swap-v2-tests = []
 test-ext-api = ["trading_api/test-ext-api"]
-new-db-arch = [] # A temporary feature to integrate the new db architecture incrementally
+new-db-arch = ["mm2_core/new-db-arch"] # A temporary feature to integrate the new db architecture incrementally


nit: this change is up #2398. you could omit it here if u want (to avoid conflicts)

mariocynicys · 2025-04-30T18:26:55Z

mm2src/coins/lp_coins.rs

+    HdWallet {
+        coin: String,
+        message: String,
+        derivation_path: RpcDerivationPath,


there is part of the derivation path which should be static per coin (and defined in the coins file).
we might wanna get the derivation path this way: https://github.com/KomodoPlatform/komodo-defi-framework/blob/d323812a5100b3a96f152bfcbc7ac38e86b828bc/mm2src/coins/rpc_command/account_balance.rs#L19-L20

I might disagree with this. KDF already returns full derivation path for every addresses which GUI can just pass to this param. Moreover, if I go with your suggestion, I will also need to make sign_message into an asynchronous function(no big deal tho) and make the fn do more work.

If I see a better reason not to require the full derivation_path I will rework it ofc.

#2406 (comment)

if we wanna keep it like this, we at least wanna perform a check (e.g.) to make sure we are not signing from a private key that isn't standard to this coin. people make mistakes.

done 032faff

mariocynicys · 2025-04-30T18:37:24Z

mm2src/coins/lp_coins.rs

+#[serde(tag = "derivation_method", rename_all = "snake_case")]
+pub enum SignatureRequest {
+    Iguana {
+        coin: String,
+        message: String,
+    },
+    HdWallet {
+        coin: String,
+        message: String,
+        derivation_path: RpcDerivationPath,
+    },
 }


does it make sense to tell the caller to state whether they want the signing with iguana or HD wallet given that KDF was initialized exclusively with either iguana or HD wallet?

i mean, if i initialized KDF with enable_hd: true, signing requests should implicitly be HD based (as signing with iguana should error anyway and vice versa), or let's say, signing should depend on the activated coin (assuming we can have one coin activated in HD mode while another activated in another mode (e.g. pubkey-only)).

what im saying is, the derivation_method field shouldn't exist.

valid point 👍🏾

done 032faff

onur-ozkan

LGTM other than some minor notes

mm2src/coins/eth.rs

mm2src/coins/siacoin.rs

mm2src/coins/utxo/utxo_common.rs

mariocynicys · 2025-05-02T14:49:45Z

mm2src/coins/eth.rs

+
+        let signature = match derivation_path {
+            Some(derivation_path) => {
+                let expected_coin_type = self.priv_key_policy.path_to_coin_or_err()?.coin_type();


could we check the whole path to coin instead of just the coin type?

thanks

what about this tho:

let expected_path = self.priv_key_policy.path_to_coin_or_err()?; let rpc_path = HDPathToCoin::try_from(derivation_path.clone())?; if expected_path != &rpc_path { let error = format!( "Derivation path '{:?}' must be equal to '{:?}'", derivation_path, expected_path ); return MmError::err(SignatureError::InvalidRequest(error)); };

but looks like the tail applies a strict length check: https://github.com/KomodoPlatform/komodo-defi-framework/blob/187284d0b1e07ef7d9c243cb35dba210063c2b4d/mm2src/crypto/src/bip32_child.rs#L233

we can trim the derivation path before HDPathToCoin::try_from or use StandardHDPath but add a method to it that gets the HDPathToCoin from self.

shamardy

One comment at this stage that will change things a bit. Will review more once this is addressed.

mm2src/coins/lp_coins.rs

smk762

Confirmed working for LTC, LTC-segwit, and ETH. Confirmed param omission defaults to 0 index address. Confirmed param inclusion validates with related index address.

Prior segwit issues determined to be a result of derivation path data in MM2.db not being updated to match the coins file when it was updated. Issue unlikely to affect non-dev users.

shamardy

Thanks for the fixes! Only nits left, will merge once fixed.

mm2src/coins/lp_coins.rs

mm2src/coins/eth.rs

mm2src/coins/hd_wallet/mod.rs

shamardy · 2025-05-27T23:23:43Z

mm2src/coins/siacoin.rs

+    fn sign_message_hash(&self, _message: &str) -> Option<[u8; 32]> { None }

-    fn sign_message(&self, _message: &str) -> SignatureResult<String> { unimplemented!() }
+    fn sign_message(&self, _message: &str, _account: Option<HDAddressSelector>) -> SignatureResult<String> {
+        MmError::err(SignatureError::InternalError("Not implemented".into()))
+    }

    fn verify_message(&self, _signature: &str, _message: &str, _address: &str) -> VerificationResult<bool> {
-        unimplemented!()
+        MmError::err(VerificationError::InternalError("Not implemented".into()))


I guess we will need those once SIA implementation is completed. Can you please open an issue for both this and tendermint and any other missing implementation @borngraced

mm2src/coins/utxo/utxo_common.rs

mm2src/mm2_test_helpers/src/for_tests.rs

shamardy · 2025-05-28T11:39:08Z

@smk762 account in SignatureRequest was renamed to address

dimxy

LGTM.

Couple of notes for future:

I'd suggest we try to avoid optional params like in this fn
fn sign_message(&self, message: &str, address: Option<HDAddressSelector>)
in favour of explicit params, to avoid hidden behaviour what make the whole api less transparent IMO. So in this case params could be enum { Iguana, HDWallet {address: HDAddressSelector }

Also let's avoid general purpose errors with attached lengthy descriptions like: SignatureError::InvalidRequest("You need to enable kdf with enable_hd to sign messages with a specific account/address".to_string()) in favour of error codes, for e.g. SignatureError::HDWalletNotEnabled

shamardy · 2025-05-29T17:34:13Z

I'd suggest we try to avoid optional params like in this fn
fn sign_message(&self, message: &str, address: Option<HDAddressSelector>)
in favour of explicit params, to avoid hidden behaviour what make the whole api less transparent IMO. So in this case params could be enum { Iguana, HDWallet {address: HDAddressSelector }

Also let's avoid general purpose errors with attached lengthy descriptions like: SignatureError::InvalidRequest("You need to enable kdf with enable_hd to sign messages with a specific account/address".to_string()) in favour of error codes, for e.g. SignatureError::HDWalletNotEnabled

Totally agree on both

shamardy · 2025-05-29T17:40:30Z

@borngraced please fix this opening comment #2432 (comment) even post merge

borngraced · 2025-05-29T17:51:53Z

@borngraced please fix this opening comment #2432 (comment) even post merge

updated. thanks!

fixes two tests that were introduced in #2432. 1- fixes `test_sign_verify_message_eth_with_derivation_path` by including a json parameter that was missing. 2- fixes `test_hd_address_conflict_across_derivation_paths` as it was not functioning correctly because it never re-used the same DB path, which is a critical step to show that the bug exists. this makes it so we reuse the same DB path for the second mm2 instance. the test was also simplified to omit message signing as it was not critical part of the test.

* dev: feat(walletconnect): add WalletConnect v2 support for EVM and Cosmos (#2223) feat(ibc-routing-part-1): supporting entire Cosmos network for swaps (#2459) fix(test): fix HD Wallet message signing tests (#2474) improvement(builds): enable static CRT linking for MSVC builds (#2464) feat(wallet): implement HD multi-address support for message signing (#2432)

* lr-swap-wip: (37 commits) fix custom token error name fix getting chain_id from protocol_data refactor (review): use dedicated large error cfg, add new fn to FromApiValueError, fix TODO, use experimental namespace for lr rpc, more Ticker alias feat(walletconnect): add WalletConnect v2 support for EVM and Cosmos (GLEECBTC#2223) feat(ibc-routing-part-1): supporting entire Cosmos network for swaps (GLEECBTC#2459) fix(test): fix HD Wallet message signing tests (GLEECBTC#2474) improvement(builds): enable static CRT linking for MSVC builds (GLEECBTC#2464) feat(wallet): implement HD multi-address support for message signing (GLEECBTC#2432) fix(p2pk): validate expected pubkey correctly for p2pk inputs (GLEECBTC#2408) chore(docs): update old urls referencing atomicdex or old docs pages (GLEECBTC#2428) improvement(p2p): remove hardcoded seeds (GLEECBTC#2439) fix(evm-api): find enabled erc20 token using platform ticker (GLEECBTC#2445) chore(docs): add DeepWiki badge to README (GLEECBTC#2463) chore(core): organize deps using workspace.dependencies (GLEECBTC#2449) feat(db-arch): more dbdir to address_dir replacements (GLEECBTC#2398) chore(build-artifacts): remove duplicated mm2 build artifacts (GLEECBTC#2448) feat(pubkey-banning): expirable bans (GLEECBTC#2455) fix(eth-balance-events): serialize eth address using AddrToString (GLEECBTC#2440) chore(deps): remove base58 and replace it completely with bs58 (GLEECBTC#2427) feat(tron): initial groundwork for full TRON integration (GLEECBTC#2425) ...

* dev: (30 commits) chore(core): replace hash_raw_entry with stable entry() API (GLEECBTC#2473) chore(core): adapt `MmError` and usages for compatibility with new rustc versions (GLEECBTC#2443) feat(wallet): add `delete_wallet` RPC (GLEECBTC#2497) chore(release): add changelog entries for v2.5.0-beta (GLEECBTC#2494) chore(release): bump kdf version to 2.5.0-beta (GLEECBTC#2492) feat(tests): zcoin unit test to validate dex fee (GLEECBTC#2460) fix(zcoin): correctly track unconfirmed z-coin notes (GLEECBTC#2331) improvement(orders): remove BTC specific volume from min_trading_vol logic (GLEECBTC#2483) feat(ibc-routing-part-2): supporting entire Cosmos network for swaps (GLEECBTC#2476) fix(startup): don't initialize WalletConnect during startup (GLEECBTC#2485) fix(dns): better ip resolution (GLEECBTC#2487) improvement(event-streaming): strong type streamer IDs (GLEECBTC#2441) bump timed-map to `1.4.1` (GLEECBTC#2481) improvement(RPC): unified interface for legacy and current RPC interfaces (GLEECBTC#2450) improvement(tendermint): `tendermint_tx_internal_id` helper (GLEECBTC#2438) feat(walletconnect): add WalletConnect v2 support for EVM and Cosmos (GLEECBTC#2223) feat(ibc-routing-part-1): supporting entire Cosmos network for swaps (GLEECBTC#2459) fix(test): fix HD Wallet message signing tests (GLEECBTC#2474) improvement(builds): enable static CRT linking for MSVC builds (GLEECBTC#2464) feat(wallet): implement HD multi-address support for message signing (GLEECBTC#2432) ... # Conflicts: # mm2src/coins/qrc20.rs # mm2src/mm2_main/src/lp_swap/maker_swap.rs

borngraced added 2 commits April 29, 2025 16:41

refactor sign_message and enable sign message for specific address fo…

c5ccb30

…r HDwallet, completed eth impl

implement support for utxo

7c61845

borngraced self-assigned this Apr 29, 2025

borngraced added the status: in progress label Apr 29, 2025

borngraced added 3 commits April 30, 2025 02:37

add unit test for eth

529f2a6

minor changes to eth test

8a42b0e

implement utxo unit test

b23b8fc

borngraced added status: pending review and removed status: in progress labels Apr 30, 2025

onur-ozkan previously approved these changes Apr 30, 2025

View reviewed changes

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved

apply onur's patch

d323812

borngraced dismissed onur-ozkan’s stale review via d323812 April 30, 2025 15:34

mariocynicys reviewed Apr 30, 2025

View reviewed changes

onur-ozkan previously approved these changes May 1, 2025

View reviewed changes

mm2src/coins/eth.rs Outdated Show resolved Hide resolved

mm2src/coins/siacoin.rs Outdated Show resolved Hide resolved

mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved

fix review notes and validate coin type

ad7e0b5

borngraced dismissed onur-ozkan’s stale review via ad7e0b5 May 1, 2025 20:51

borngraced added 3 commits May 1, 2025 21:51

Merge branch 'dev' into hd-multi-addr-msg-sign

032faff

impl Display for Bip32Deri..

6d4332e

nits

9e9081e

mariocynicys reviewed May 2, 2025

View reviewed changes

borngraced added 2 commits May 3, 2025 17:07

validate comps that makes up path_to_coin

187284d

impl path_to_coin method and use for rpc derivation_path validation

b10fd57

shamardy added the priority: urgent Critical tasks requiring immediate action. label May 6, 2025

shamardy reviewed May 7, 2025

View reviewed changes

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved

smk762 mentioned this pull request May 7, 2025

Update/kdf/message signing hd GLEECBTC/komodo-docs-mdx#487

Merged

borngraced added status: in progress and removed status: pending review labels May 7, 2025

borngraced added 3 commits May 7, 2025 14:59

Merge branch 'dev' into hd-multi-addr-msg-sign

5fcef99

rename WithdrawFrom and reuse in SignatureRequest -< message signing

791c64b

fmt

8489bb0

smk762 self-requested a review May 27, 2025 12:54

smk762 previously approved these changes May 27, 2025

View reviewed changes

apply segwit sign-verify patch by Omar

935409d

borngraced dismissed smk762’s stale review via 935409d May 27, 2025 13:07

cargo fmt

9f0bde4

borngraced added status: pending review and removed status: in progress labels May 27, 2025

borngraced added 2 commits May 27, 2025 21:31

impl unit test for address conflict caused by derivation_paths

8180da3

clippy

4d91373

shamardy previously approved these changes May 27, 2025

View reviewed changes

shamardy added the 2.5.0-beta label May 28, 2025

fix naming and minor changes

860c208

borngraced dismissed shamardy’s stale review via 860c208 May 28, 2025 09:38

borngraced force-pushed the hd-multi-addr-msg-sign branch from 48a64cf to 860c208 Compare May 28, 2025 09:58

update lp_coins sign_message

3ade61b

dimxy approved these changes May 29, 2025

View reviewed changes

borngraced mentioned this pull request May 29, 2025

Switching HD derivation_paths causes address conflicts #2470

Open

shamardy approved these changes May 29, 2025

View reviewed changes

shamardy merged commit b5e034a into dev May 29, 2025
20 of 24 checks passed

shamardy deleted the hd-multi-addr-msg-sign branch May 29, 2025 17:41

shamardy mentioned this pull request Jun 16, 2025

chore(release): v2.5.0-beta #2491

Merged

shamardy mentioned this pull request Jul 4, 2025

Feat(KYC/AML): HD multi-address support for message signing #2406

Closed

Conversation

borngraced commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example before

Example New

Example New(HD account)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smk762 left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy commented May 28, 2025

Uh oh!

dimxy left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy commented May 29, 2025

Uh oh!

Uh oh!

borngraced commented May 29, 2025

Uh oh!

Reviewers

Assignees

Labels

borngraced commented Apr 29, 2025 •

edited

Loading

borngraced May 1, 2025 •

edited

Loading

borngraced May 1, 2025 •

edited

Loading

borngraced Apr 30, 2025 •

edited

Loading

shamardy commented May 29, 2025 •

edited

Loading