fix(test): fix HD Wallet message signing tests by borngraced · Pull Request #2474 · GLEECBTC/komodo-defi-framework

borngraced · 2025-05-29T18:47:43Z

No description provided.

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

mariocynicys · 2025-05-29T19:41:50Z

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

    // This should fail: signature was created with pubkey from m/49'/141'/0'/0/0,
    // but we are now resolving address from m/84'/141'/0'/0/0
    assert!(
-        !verify_response.result.is_valid,
-        "Expected verification to fail due to derivation path mismatch"
+        verify_response.result.is_valid,


checked the CI. it fails. and is_valid = false.
let's forget about the CI/test failure/passing now and keep the notion around the is_valid variable.

imo, i think is_valid is supposed to be false. i.e. if is_valid were true then this is a bug.

i will say why is that my belief and please tell me yours @borngraced:

we signed a message using pubkey = m/49'/141'/0'/0/0, and we are trying to verify it with pubkey = m/84'/141'/0'/0/0. and one can not sign a message and verify that signature from two different pubkeys. so i expect is_valid to be false (i.e., i see no bug).

actually I think I get your point 💯 and that's because of the flow of the test...I will modify and hope you understand better

at this point tbh my head is spinning hard. i would much rather prefer u to answer my question/tell me ur pov rather than change the test again :(

i will say why is that my belief and please tell me yours

I guess the test is too explicit and didn't capture what I wanted to portray in a dynamic way...

So the test is supposed to activate COIN, do some signing and verification which should work, then, deactivate theCOIN, and reactivate again with new derivation path(without stopping MM2).. do some signing and verification which should now fail assuming we reuse sam DB path in the test.

In conclusion, I think the test is not 100% doing what I want it to do.

Please recheck the test

shamardy

LGTM! Only one nit.

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

shamardy · 2025-05-29T23:47:37Z

@mariocynicys once all your comments are resolved plus this #2474 (comment) , please merge the PR after your approval.

mariocynicys

that's a starter quick review. since the test was ignored in the same commit it was modified in, there is no CI test runs of it.
so will unignore and try it locally and continue the review 👍

mariocynicys · 2025-05-30T05:28:33Z

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

 }

-/// Needs attention after [issue #2470](https://github.com/KomodoPlatform/komodo-defi-framework/issues/2470) is resolved.
+/// Should not fail after [issue #2470](https://github.com/KomodoPlatform/komodo-defi-framework/issues/2470) is resolved.


nit: let's reword this to "This test fails until issue ...... is resolved"

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

mariocynicys · 2025-05-30T09:00:10Z

nvm my previous comment about signing.
could u take a look at this commit 43454d2

it exploits the bug without signing and verifying.

borngraced · 2025-05-30T10:11:47Z

nvm my previous comments about signing. could u take a look at this commit 43454d2

it exploits the bug without signing and verifying.

It definitely makes more sense to write the test the same way it was discovered IMO..can just include assert_eq old and new addrees[0] rather than changing the test

mariocynicys · 2025-05-30T10:32:04Z

It definitely makes more sense to write the test the same way it was discovered IMO

i don't think we have to encode a complicated history of how the bug was discovered into the test (any first eye would be confused why we signing. let alone i was super confused to get started with this bug and i am supposedly following from the beginning).
the test should rather serve as a minimal reproducible example that demonstrate the bug. any extra routes are extra confusion to what this problem is related to.

can just include assert_eq old and new addrees[0] rather than changing the test

i think u mean assert_ne. then that's enough though to show the bug. the first panic gonna trigger anyway.
i mean we can also add a withdraw snippet to the test that exploits the bug. we could probably add a swap or two in addition to all of that to exploit the bug and tons of other things that the reader of the test will think they are related to the bug, but the bug in fact is solely in the HD Wallet code.

mariocynicys

Thanks for fixing this!
LGTM!

shamardy

Thanks a lot @borngraced @mariocynicys !

* 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 May 29, 2025 19:44

fix eth test and hd address conflicts

25d9527

rename hd address conflict test

dd32b85

borngraced requested a review from shamardy May 29, 2025 18:47

nit

fe27df9

mariocynicys reviewed May 29, 2025

View reviewed changes

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Show resolved Hide resolved

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Show resolved Hide resolved

allow test_hd_address_conflict_across_derivation_paths run

b881f25

mariocynicys reviewed May 29, 2025

View reviewed changes

borngraced added 2 commits May 29, 2025 23:31

improve conflict hd address unit test

ffb3b54

clippy

021b7b5

shamardy added priority: urgent Critical tasks requiring immediate action. 2.5.0-beta status: pending review labels May 29, 2025

shamardy previously approved these changes May 29, 2025

View reviewed changes

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Outdated Show resolved Hide resolved

re-use seednode_with_hd_account

e0bb2d9

borngraced dismissed shamardy’s stale review via e0bb2d9 May 29, 2025 23:54

mariocynicys reviewed May 30, 2025

View reviewed changes

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Outdated Show resolved Hide resolved

borngraced added 2 commits May 30, 2025 15:10

simplify hd address conflict test logics

823b67b

clippy

56f1b6c

mariocynicys approved these changes May 30, 2025

View reviewed changes

shamardy approved these changes May 31, 2025

View reviewed changes

shamardy changed the title ~~fix(test): fix eth hd message sign and verify test~~ fix(test): fix HD Wallet message signing tests May 31, 2025

shamardy merged commit 923eb0b into dev May 31, 2025
24 of 25 checks passed

shamardy deleted the hd-sign-msg-fix branch May 31, 2025 02:56

shamardy mentioned this pull request May 31, 2025

Switching HD derivation_paths causes address conflicts #2470

Open

shamardy mentioned this pull request Jun 16, 2025

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

Merged

Conversation

borngraced commented May 29, 2025

Uh oh!

Uh oh!

Uh oh!

mariocynicys May 29, 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!

borngraced May 29, 2025

Choose a reason for hiding this comment

Uh oh!

mariocynicys May 29, 2025

Choose a reason for hiding this comment

Uh oh!

borngraced May 29, 2025

Choose a reason for hiding this comment

Uh oh!

borngraced May 29, 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!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

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!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

mariocynicys May 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

borngraced commented May 30, 2025

Uh oh!

mariocynicys commented May 30, 2025

Uh oh!

mariocynicys 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mariocynicys May 29, 2025 •

edited

Loading

borngraced May 29, 2025 •

edited

Loading

shamardy commented May 29, 2025 •

edited

Loading

mariocynicys commented May 30, 2025 •

edited

Loading