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

hdwallet-metamask's partial multi-account support uses a definitely-wrong derivation path #459

Open
mrnerdhair opened this issue Mar 14, 2022 · 0 comments
Labels
tech-debt A suboptimality that existed in code when it was written.

Comments

@mrnerdhair
Copy link
Contributor

While hdwallet-metamask does not fully support multiple accounts, its ethGetAccountPaths and describePath functions do, and assume the standard BIP44 derivation scheme (m/44'/60'/n'/0/0) even though MetaMask itself uses m/44'/60'/0'/0/n (unless it's being used with a hardware wallet, in which case all bets are off).

Options:

  • Update hdwallet-metamask to use Metamask's native account derivation scheme in these functions instead of BIP44. (This isn't perfect, but at least it won't be clearly wrong anymore.) (This is the approach taken by metamask: use metamask-style account path derivation #424.)
  • Return [] (or throw) from ethGetAccountPaths and throw in describePath to avoid exposing wrong functionality.
  • Support multiple accounts in MM "properly" somehow.
@mrnerdhair mrnerdhair added the tech-debt A suboptimality that existed in code when it was written. label Mar 14, 2022
@mrnerdhair mrnerdhair linked a pull request Mar 17, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt A suboptimality that existed in code when it was written.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant