Skip to content

fix(hw-wallet): avoid calling get_enabled_address in trezor-based coin init#2504

Merged
shamardy merged 6 commits intostagingfrom
hotfix-2502
Jun 28, 2025
Merged

fix(hw-wallet): avoid calling get_enabled_address in trezor-based coin init#2504
shamardy merged 6 commits intostagingfrom
hotfix-2502

Conversation

@mariocynicys
Copy link
Copy Markdown
Collaborator

@mariocynicys mariocynicys commented Jun 26, 2025

this makes it so we don't call get_enabled_address before initializing the coin, but rather after we made sure we initialized the account and address that the enabled address belongs to.
recently spent outputs is set at the beginning with a dummy output script and then changed to the correct one corresponding to the enabled address once we got it from the hardware wallet.

fixes #2502

@mariocynicys
Copy link
Copy Markdown
Collaborator Author

@takenagain could u please test if this works as expected?

also that the recently spent output script is set correctly (by adding a log line to mm2src/coins/utxo/utxo_common.rs - fn received_enabled_address_from_hw_wallet() that logs the address).

@shamardy
Copy link
Copy Markdown
Collaborator

@mariocynicys this PR should be based on the staging branch

@mariocynicys mariocynicys changed the base branch from dev to staging June 26, 2025 07:37
@mariocynicys mariocynicys changed the title hotfix(hw-wallet): avoid calling get_enabled_address in UTxO trezor-based initializaiton fix(hw-wallet): avoid calling get_enabled_address in UTxO trezor-based initializaiton Jun 26, 2025
@mariocynicys mariocynicys changed the title fix(hw-wallet): avoid calling get_enabled_address in UTxO trezor-based initializaiton fix(hw-wallet): avoid calling get_enabled_address in trezor-based coin init Jun 26, 2025
@mariocynicys mariocynicys added status: pending review priority: urgent Critical tasks requiring immediate action. labels Jun 26, 2025
@takenagain
Copy link
Copy Markdown

Activation is stuck in RequestingWalletBalance status for DOC.

{
  "mmrpc": "2.0",
  "result": {
    "status": "InProgress",
    "details": "RequestingWalletBalance"
  },
  "id": null
}

I added additional logging statements to mm2src/coins/coin_balance.rs -> fn enable_hd_wallet, and it appears to get stuck in get_enabled_address function. More specifically, it seems to be the get_account method in mm2src/coins/hd_wallet/mod.rs.

It has not reached received_enabled_address_from_hw_wallet in my testing so far.

See kdf.log with additional debug statements for DOC activation followed by MARTY activation attempt.

kdf.log without any local changes

RPC execution order
  1. task::init_trezor::init
  2. task::init_trezor::user_action
  3. task::init_trezor::status
  4. task::enable_utxo::init
  5. task::enable_utxo::status

Run command:

MM_LOG=./kdf.log RUST_LOG=debug cargo run
DOC enable request

Limited to SSL & TCP electrums in testing.

{
  "userpass": "{{userpass}}",
  "mmrpc": "2.0",
  "method": "task::enable_utxo::init",
  "params": {
    "ticker": "DOC",
    "activation_params": {
      "mode": {
        "rpc": "Electrum",
        "rpc_data": {
          "servers": [
            {
              "url": "electrum1.cipig.net:10020",
              "contact": [
                {
                  "email": "cipi@komodoplatform.com"
                },
                {
                  "discord": "cipi#4502"
                }
              ],
              "protocol": "TCP"
            },
            {
              "url": "electrum1.cipig.net:20020",
              "protocol": "SSL",
              "contact": [
                {
                  "email": "cipi@komodoplatform.com"
                },
                {
                  "discord": "cipi#4502"
                }
              ]
            },
            {
              "url": "electrum2.cipig.net:10020",
              "contact": [
                {
                  "email": "cipi@komodoplatform.com"
                },
                {
                  "discord": "cipi#4502"
                }
              ],
              "protocol": "TCP"
            },
            {
              "url": "electrum2.cipig.net:20020",
              "protocol": "SSL",
              "contact": [
                {
                  "email": "cipi@komodoplatform.com"
                },
                {
                  "discord": "cipi#4502"
                }
              ]
            },
            {
              "url": "electrum3.cipig.net:10020",
              "contact": [
                {
                  "email": "cipi@komodoplatform.com"
                },
                {
                  "discord": "cipi#4502"
                }
              ],
              "protocol": "TCP"
            },
            {
              "url": "electrum3.cipig.net:20020",
              "protocol": "SSL",
              "contact": [
                {
                  "email": "cipi@komodoplatform.com"
                },
                {
                  "discord": "cipi#4502"
                }
              ]
            }
              ]
            }
          ]
        }
      },
      "tx_history": true,
      "priv_key_policy": "Trezor",
      "gap_limit": 20,
      "scan_policy": "scan_if_new_wallet",
      "min_addresses_number": 3
    }
  }
}

@mariocynicys
Copy link
Copy Markdown
Collaborator Author

@takenagain thanks for the investigation and logs, that came in handy :)

the issue seemed to be a deadlock due to issuing this get_enabled_address call while the accounts map was still locked after. now i let it drop early before this call. things should be fine now 🤞

@takenagain
Copy link
Copy Markdown

Thanks for the quick fix @mariocynicys!

Confirmed that it works as expected via Postman and the GUI with Trezor, HD, and non-HD (all expected addresses and balances discovered).

kdf.log - the log in received_enabled_address_from_hw_wallet is tagged with, "Enabled address for ".

Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is a hotfix, all comments are for future reference.

Comment on lines +511 to +528
if coin.is_trezor() {
let enabled_address =
hd_wallet
.get_enabled_address()
.await
.ok_or(EnableCoinBalanceError::NewAddressDerivingError(
NewAddressDerivingError::Internal(
"Couldn't find enabled address after it has already been enabled".to_string(),
),
))?;
coin.received_enabled_address_from_hw_wallet(enabled_address)
.await
.map_err(|e| {
EnableCoinBalanceError::NewAddressDerivingError(NewAddressDerivingError::Internal(format!(
"Coin rejected the enabled address derived from the hardware wallet: {}",
e
)))
})?;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is repeated, you can refactor it in the other open PR as I am gonna merge this now

Comment on lines +235 to +236
/// Informs the coin of the enabled address provided/derived by the hardware wallet.
async fn received_enabled_address_from_hw_wallet(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have this in a separate trait in the future to start separating specific HW wallet code from the more general HD wallet code. This is not at all a priority at the current stage but we need it mentioned somewhere in our issues / checklists or at least a todo.

@shamardy shamardy merged commit a25aea6 into staging Jun 28, 2025
17 of 24 checks passed
@shamardy shamardy deleted the hotfix-2502 branch June 28, 2025 02:26
mariocynicys added a commit that referenced this pull request Jul 3, 2025
this was fixed in another PR, namely: #2504
dimxy pushed a commit that referenced this pull request Jul 13, 2025
* dev:
  fix(eth): Correctly implement ETH max withdrawal logic (#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args #2215 (#2510)
  feat(orderbook): expirable maker orders (#2516)
  improvement(eth): drop parity support (#2527)
  chore(release): finalize changelog for v2.5.0-beta (#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (#2444)
  feat(swap): rpc to find best swap with liquidity routing for ask (#2362)
  fix(kdf_walletconnect): apply explicit MmError mapping (#2514)
  fix(walletconnect): centralize connection and retry logic (#2508)
  fix(hw-wallet): avoid calling `get_enabled_address` in trezor-based coin init (#2504)
dimxy pushed a commit to dimxy/komodo-defi-framework that referenced this pull request Jul 16, 2025
* refactor-gas-fee-policy: (30 commits)
  revert change in test_enable_custom_erc20
  fix(test) disable get_swap_gas_fee_policy for tests (no platform coin in tests)
  revert tokio test and use block_on for eth docker test
  Refactor gas fee code: get max_eth_tx_type from platform coin, add gas fee adjust coin params, rename "set_swap_transaction_fee_policy" rpc to "set_swap_gas_fee_policy", force set_swap_gas_fee_policy to store policy in the platform coin
  fix(eth): Propagate structured EIP-1559 fee errors (GLEECBTC#2532)
  fix(eth): Correctly implement ETH max withdrawal logic (GLEECBTC#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args GLEECBTC#2215 (GLEECBTC#2510)
  feat(orderbook): expirable maker orders (GLEECBTC#2516)
  improvement(eth): drop parity support (GLEECBTC#2527)
  chore(release): finalize changelog for v2.5.0-beta (GLEECBTC#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (GLEECBTC#2444)
  feat(swap): rpc to find best swap with liquidity routing for ask (GLEECBTC#2362)
  fix(kdf_walletconnect): apply explicit MmError mapping (GLEECBTC#2514)
  fix(walletconnect): centralize connection and retry logic (GLEECBTC#2508)
  fix(hw-wallet): avoid calling `get_enabled_address` in trezor-based coin init (GLEECBTC#2504)
  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)
  ...

# Conflicts:
#	mm2src/coins/eth.rs
#	mm2src/coins/eth/eth_swap_v2/eth_maker_swap_v2.rs
#	mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs
#	mm2src/coins/eth/eth_utils.rs
#	mm2src/coins/eth/eth_withdraw.rs
#	mm2src/coins/eth/v2_activation.rs
#	mm2src/coins/lp_coins.rs
#	mm2src/coins/qrc20.rs
#	mm2src/coins/qrc20/swap.rs
#	mm2src/coins_activation/src/platform_coin_with_tokens.rs
#	mm2src/coins_activation/src/token.rs
#	mm2src/mm2_main/src/lp_swap/check_balance.rs
#	mm2src/mm2_main/src/lp_swap/maker_swap.rs
#	mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs
#	mm2src/mm2_main/src/rpc/lp_commands/ext_api/ext_api_types.rs
#	mm2src/mm2_main/src/rpc/lp_commands/mod.rs
#	mm2src/mm2_main/src/rpc/lp_commands/one_inch/errors.rs
#	mm2src/mm2_main/src/rpc/lp_commands/one_inch/rpcs.rs
#	mm2src/mm2_main/src/rpc/lp_commands/tokens.rs
#	mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
#	mm2src/trading_api/src/one_inch_api/classic_swap_types.rs
#	mm2src/trading_api/src/one_inch_api/client.rs
#	mm2src/trading_api/src/one_inch_api/portfolio_types.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: urgent Critical tasks requiring immediate action. status: pending review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants