Skip to content

perf(tron-wallet-snap): account-lifecycle fixes from createAccounts/import profiling - #149

Open
hmalik88 wants to merge 8 commits into
mainfrom
hm/mul-2015
Open

perf(tron-wallet-snap): account-lifecycle fixes from createAccounts/import profiling#149
hmalik88 wants to merge 8 commits into
mainfrom
hm/mul-2015

Conversation

@hmalik88

@hmalik88 hmalik88 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Account-lifecycle performance fixes from profiling account creation and bulk import

Context: how we found these

While investigating batch account creation performance (MUL-2015), we instrumented the snap two ways: a baseline branch with per-step timings inside createAccounts (state read, entropy fetch, derivation, merge, re-read), and a branch wrapping every snap handler invocation with duration logs. We then measured warm single account creation and a 151-account SRP import on a local Flask build with the snap file:-linked into the extension.

Three findings from that campaign drive this PR:

  1. createAccounts made five sequential extension RPC round trips, two of which were avoidable: the post-merge state re-read existed only because mergeKeyringAccounts returned void, and the existing-accounts read and entropy fetch ran sequentially despite being independent.
  2. The Bip44Discover path fetched entropy twice per call; once at the full derivation path just to compute an address for the on-chain activity check, then again at the coin-type path to create the account.
  3. Handler timings showed onSynchronizeSelectedAccounts running 3× concurrently at onboarding and 7× concurrently during the import, which is identical work, since the handler takes no parameters.

The same instrumentation also showed where the import time doesn't go: MetaMask imports all accounts as a single Bip44DeriveIndexRange call, so per-call createAccounts overhead barely registers on import (snap-side total ~3.5s either way). The import window is instead dominated by extension-side RPC queueing and post-import background work (tracked separately).

Changes

1. Fewer extension RPC round trips in keyring_createAccounts (5 → at most 4)

  • AccountsRepository.mergeKeyringAccounts now returns the merge result ({ merged, added }) instead of void, so createAccounts resolves persisted accounts; including conflict winners when a concurrent writer got there first, directly from the merge instead of re-reading state afterwards.
  • The existing-accounts read and the coin-type snap_getBip32Entropy fetch now run in parallel. Timings confirm the state read is fully hidden behind the entropy fetch.
  • Keeps the phase-timing instrumentation (readAndEntropyMs, deriveMs, mergeMs, totalMs) so future regressions are visible. Values are stringified because live objects become unexpandable in DevTools once the snap execution environment is torn down.
  • Trade-off: snap_getBip32Entropy is now called even when all requested indices already exist (+~5ms on that path). It only occurs on idempotent retries, and no new permissions are required.

2. Single entropy fetch during BIP-44 discovery (2 → 1)

Discovery now creates the coin-type deriver once, derives the activity-check address locally from the cached change node, and reuses the same deriver for account creation. This is address-equivalent: both the old full-path fetch and the deriver produce m/44'/195'/0'/0/i. New tests pin the single-fetch behavior and assert the probed address is the one persisted.

3. Coalesce concurrent account synchronization runs

Root cause of the 3×/7× duplicate syncs: every setSelectedAccounts call schedules a PT1S background event with no dedupe against already-pending ones, stacking on the PT60S cronjob and post-transaction refreshes. During onboarding/import the extension calls setSelectedAccounts repeatedly, so several identical sync runs fire within the same second. Each duplicate run fans out accounts × active networks of TronGrid requests, then duplicates state writes and AccountAssetListUpdated/AccountBalancesUpdated/AccountTransactionsUpdated keyring events back to the extension, when the extension RPC queue is most contended.

Fix: AccountsService.synchronize is wrapped in a keyed in-flight guard: concurrent calls for the same account set share one run; the next call after settlement starts a fresh one. All sync entry points (cronjob, SynchronizeSelectedAccounts, SynchronizeAccounts, SynchronizeAccount) funnel through this method. Tron's sync already swallows partial failures via Promise.allSettled, so sharing a run does not change error semantics.

The guard is a new InFlightCoalescer class in @metamask/snap-networks-utils, exported from a new ./dedupe entry point (same subpath-export pattern as ./logger), will be used to fix a similar issue in the Bitcoin snap in a follow-up PR.

4. Remove the unreachable v1 account-creation path

AccountsService.create was not exposed by any handler, the keyring handler only routes keyring_createAccounts (the extension uses the batch method even for single manual adds), so it was dead code. It was also a latent bug: it emitted the v1 AccountCreated lifecycle event, which keyring-v2 clients reject (this snap declares endowment:keyring.capabilities, making it v2), and its rollback-on-emission-failure would then delete the just-persisted account. Removed along with its private dependency chain, which nothing else used: deriveAccount, the lowest-unused-index helpers (utils/getLowestUnusedIndex.ts), and the CreateAccountOptions type (services/accounts/types.ts). Note that the three data-update events (AccountAssetListUpdated/AccountBalancesUpdated/AccountTransactionsUpdated) are still supported for v2 snaps and remain untouched.

Measurements

Local Flask build, snap file:-linked into the extension:

Scenario Baseline This PR
Warm single account creation (quiet) ~22 ms ~18.5 ms (−4 ms, ~18%)
151-account bulk import (snap-side total) ~3.49 s ~3.2–7.4 s depending on contention (see below)
BIP-44 discovery call 2 entropy RPCs 1 entropy RPC (~10 ms saved per call)
Already-exists retry +5 ms (speculative entropy fetch; hidden whenever the state read is slower)
Selected-accounts sync during onboarding/import 3–7 concurrent duplicate runs 1 shared run

Warm phase profile for reference: state read ~4 ms, entropy fetch 7–13 ms, derivation ~0.8 ms/account, merge 3–9 ms quiet (25–33 ms under extension-side contention, where all phases inflate together 3–5×).

Where the bulk-import time actually goes

Phase timings for the single bulk createAccounts call ({from: 1, to: 151}, 151 accounts created) during a 152-account SRP import, same build, with MetaMetrics on vs off:

Phase MetaMetrics on MetaMetrics off
readAndEntropy (one parallel state-read + entropy round trip) 3,547 ms 3,116 ms
derive (all 151 accounts, local CKD) 64 ms 116 ms
merge (one persist round trip) 3,764 ms 15 ms
total 7,377 ms 3,249 ms

Snap compute is ~100 ms either way; >95% of the call is extension RPC queue wait, which is why the round-trip reduction in this PR is the right shape of fix even though it cannot rescue import latency by itself. The on/off comparison isolates the MetaMetrics proof-of-ownership fan-out (~150 concurrent per-account signProofOfOwnership requests, fired by the profile-metrics controller as the imported accounts get registered client-side) at ~4.1 s of queue pressure on this one call; most visibly on the persist, which takes 15 ms on a quiet queue. The remaining ~3.1 s on the first round trip is cross-snap import contention (Solana/Bitcoin snaps importing the same range concurrently over the same transport). Both are tracked as follow-ups.

The extension also re-issues the same bulk range once (~3–10 s later); the idempotent path returns created: 0 in ~25 ms.

Testing

  • New tests: single-entropy-fetch behavior on both discovery outcomes; sync coalescing (concurrent same-accounts calls share one run, sequential calls re-run, different account sets don't coalesce); InFlightCoalescer unit tests at 100% coverage (sharing, settlement reset, key independence, rejection propagation).
  • Tests covering the removed create/deriveAccount paths were deleted with them; full suites pass for both packages.

References

N/A

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

@hmalik88
hmalik88 marked this pull request as ready for review August 14, 2026 10:58
@hmalik88
hmalik88 requested a review from a team as a code owner August 14, 2026 10:58
@hmalik88
hmalik88 deployed to default-branch August 14, 2026 10:58 — with GitHub Actions Active
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.

1 participant