atomicDEX-API icon indicating copy to clipboard operation
atomicDEX-API copied to clipboard

HD Wallet implementation

Open ca333 opened this issue 2 years ago • 2 comments

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:

  • multi-wallet support on seed level where each seed = one wallet. (already supported)
  • multi-account support: each asset allows the creation of multiple accounts (for clean "purpose" isolation). i.e. If I create a new account, the account ID needs incremented.
  • multi-address support: each account can then have multiple addresses.
  • if I create a multi-account/address wallet on any other BIP32/44 wallet (Trezor, etc.) I expect to see the same pub-keys for all accounts/addresses as in the corresponding ADEX wallet (with same seed).
  • implementation needs to align with official BIP32/44 standards

ca333 avatar May 26 '23 06:05 ca333

Checklist:

  • [x] https://github.com/KomodoPlatform/komodo-defi-framework/pull/1933#discussion_r1286632901
  • [x] https://github.com/KomodoPlatform/komodo-defi-framework/pull/1933#discussion_r1286632901
  • [ ] https://github.com/KomodoPlatform/komodo-defi-framework/pull/1962#discussion_r1557513950
  • [ ] https://github.com/KomodoPlatform/komodo-defi-framework/pull/1933#discussion_r1289560423
  • [ ] https://github.com/KomodoPlatform/komodo-defi-framework/pull/1933#discussion_r1293013105
  • [ ] https://github.com/KomodoPlatform/komodo-defi-framework/pull/1933#pullrequestreview-1604776313
  • [ ] https://github.com/KomodoPlatform/komodo-defi-framework/pull/2013#discussion_r1407988271
  • [ ] https://github.com/KomodoPlatform/komodo-defi-framework/pull/2013#discussion_r1414267228
  • [ ] Make sure that swap restarts work right when activating a coin with a different address. after https://github.com/KomodoPlatform/komodo-defi-framework/pull/2093. A new method similar to coins_needed_for_kickstart should show what address to enable for swaps on restart.
  • [ ] Add more HD wallet tests using geth dev node once.
  • [ ] Add tests for HD balance streaming
  • [ ] Add test for multiple addresses nonce lock
  • [ ] Extract pubkey methods and additional enhancements.
  • [ ] Move mm2src/coins/coin_balance.rs and related HD wallet balance implementations to hd_wallet mod.
  • [ ] Create hd_wallet crate once the code is completely decoupled from coins crate code.
    • [ ] Add crate features for HW wallet, balances, withdraw and serde support. Trezor should be separate from hd_wallet crate and implement the HW traits.
  • [ ] EVM Transaction History
  • [ ] NFT HD wallet
  • [ ] Add an optional Nonce and from address to sign_raw_transaction methods.
  • [ ] Balance stream for single address or all addresses (reduces cost if a GUI wants to stream balance for only one address)
  • [ ] Implement HD wallet functionalities for more coins, this will require new activation methods.
  • [ ] Show enabled address in responses.
  • [ ] Add option to get total balance of all addresses in addition to page_balance in account_balance response.

shamardy avatar Aug 11 '23 22:08 shamardy

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.

shamardy avatar Jul 19 '24 00:07 shamardy