Skip to content

Conversation

@danroc
Copy link
Contributor

@danroc danroc commented Sep 11, 2025

Store the computed account address together with the HD node. This should prevent the need to recompute the address when listing accounts or finding an account by its address.

@danroc danroc changed the title feat(eth-hd-keyring): store computed account address perf(keyring-eth-hd): store computed account address Sep 11, 2025
@danroc danroc marked this pull request as ready for review September 11, 2025 09:58
@danroc danroc requested a review from a team as a code owner September 11, 2025 09:58
@danroc danroc requested a review from Copilot September 11, 2025 09:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes the HD keyring by pre-computing and storing Ethereum account addresses alongside HD keys to avoid repeated address calculations during account operations.

  • Store computed addresses in a new WalletData structure containing both HDKey and address
  • Eliminate redundant address computations in getAccounts(), removeAccount(), and #getWalletForAccount()
  • Simplify address lookups by using pre-stored values instead of deriving from public keys

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@danroc danroc requested a review from mikesposito September 11, 2025 10:08
Copy link
Contributor

@ccharly ccharly left a comment

Choose a reason for hiding this comment

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

LGTM! I'll wait for @mikesposito review too.

@danroc
Copy link
Contributor Author

danroc commented Sep 11, 2025

@metamaskbot publish-preview

@github-actions
Copy link

Preview builds have been published. See these instructions (from the core monorepo) for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/account-api": "0.12.0-ca7f2af",
  "@metamask-previews/keyring-api": "21.0.0-ca7f2af",
  "@metamask-previews/eth-hd-keyring": "12.1.0-ca7f2af",
  "@metamask-previews/eth-ledger-bridge-keyring": "11.1.2-ca7f2af",
  "@metamask-previews/eth-qr-keyring": "1.1.0-ca7f2af",
  "@metamask-previews/eth-simple-keyring": "10.0.0-ca7f2af",
  "@metamask-previews/eth-trezor-keyring": "9.0.0-ca7f2af",
  "@metamask-previews/keyring-internal-api": "9.0.0-ca7f2af",
  "@metamask-previews/keyring-internal-snap-client": "7.1.0-ca7f2af",
  "@metamask-previews/eth-snap-keyring": "17.1.0-ca7f2af",
  "@metamask-previews/keyring-snap-client": "8.0.0-ca7f2af",
  "@metamask-previews/keyring-snap-sdk": "7.0.0-ca7f2af",
  "@metamask-previews/keyring-utils": "3.1.0-ca7f2af"
}

Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

Great find, LGTM!

@danroc danroc added this pull request to the merge queue Sep 11, 2025
Merged via the queue into main with commit d0cce81 Sep 11, 2025
34 checks passed
@danroc danroc deleted the dr/store-account-address branch September 11, 2025 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants