KomodoPlatform / komodo-defi-framework

This is the official Komodo DeFi Framework repository
https://komodoplatform.com/en/docs/komodo-defi-framework/
103 stars 94 forks source link

[bug] HD Wallet implementation #1838

Open ca333 opened 1 year ago

ca333 commented 1 year ago

The HD-Wallet implementation is flawed and does not align with the official BIP32/44 standards as we seem only incrementing / deriving on the address index level with a static (potentially config-derived?) account id.

https://github.com/KomodoPlatform/atomicDEX-API/blob/371595d6c0d322e677669544aa37ec6304140fe8/mm2src/crypto/src/standard_hd_path.rs#L18

expectation:

shamardy commented 1 year ago

Checklist:

shamardy commented 3 months ago

We've discussed several enhancements internally that can benefit the GUI team in their implementation of the HD wallet feature. Below are the proposed enhancements:

  1. Hide Addresses with Zero Balance:

    • Add a new parameter/setting to tell the API to hide addresses with a zero balance in the response.
  2. Enhanced Account Management:

    • For unused addresses, Trezor shows a property "transfers": 0. For used addresses, it displays something like:
      {
      "transfers": 5,
      "balance": "7282",
      "sent": "14746",
      "received": "22028"
      }

      Something similar to this should be added to the responses.

  3. Hide Certain Addresses:

    • Add a feature to hide specific addresses as per user preference. This can be done in the GUI instead.
  4. Public Key Exposure:

    • Regarding the PR #2053, which can expose pubkeys to Electrum servers, we should consider not enabling legacy P2PK address checking/spending by default. For HD wallet, there will be no Legacy P2PK balance in HD addresses probably as P2PK was probably deprecated before HD wallets were proposed.
  5. Account Number and Address Scanning:

    • Implement addresses based on account number increasing and add address scanning based on the account part of the derivation path. This is needed to be compatible with parts of Trezor's EVM implementation.
    • Reference:
      We believe Ethereum should use the SEP-0005 
      scheme for everything, because it is account-based, rather than UTXO-based. 
      Unfortunately, many Ethereum tools (MEW, Metamask) do not use such a scheme 
      and set account = 0 and then iterate the address index. For compatibility, 
      we allow this scheme as well.
    • SEP-0005: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0005.md
    • Ledger also uses an account-based path for Ethereum (with all 5 levels, levels 4 and 5 are always 0). Users sometimes struggle to switch from Ledger to Trezor completely. We can support multiple derivation paths in coin configs to allow users to choose.
  6. Concurrent Address Scanning:

    • Implement concurrent address scanning to improve performance.