Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] HD Wallet implementation #1838

Open
ca333 opened this issue May 26, 2023 · 2 comments
Open

[bug] HD Wallet implementation #1838

ca333 opened this issue May 26, 2023 · 2 comments
Assignees
Labels
blocked requires additional attention bug Something isn't working High priority

Comments

@ca333
Copy link

ca333 commented May 26, 2023

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
@shamardy
Copy link
Collaborator

shamardy commented Aug 11, 2023

Checklist:

@shamardy
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked requires additional attention bug Something isn't working High priority
Projects
None yet
Development

No branches or pull requests

2 participants