perf: replace createDeepEqualSelector with createSelector in account selectors - #40859
Merged
Conversation
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Contributor
✨ Files requiring CODEOWNER review ✨🔑 @MetaMask/accounts-engineers (1 files, +68 -51)
👨🔧 @MetaMask/core-extension-ux (1 files, +1 -1)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
hmalik88
marked this pull request as draft
March 12, 2026 20:23
Contributor
Builds ready [9612fa3]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
…electors relying on getInternalAccountsFromGroupById
Contributor
Builds ready [b5d447b]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
Contributor
Builds ready [0050ae1]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
hmalik88
commented
Mar 17, 2026
| }, | ||
| }; | ||
| const store = configureStore(merge(defaultState, state)); | ||
| const store = configureStore(merge({}, defaultState, state)); |
Contributor
Author
There was a problem hiding this comment.
merge(defaultState, state) mutates defaultState in-place. Since defaultState.metamask.keyrings holds the same reference as mockState.metamask.keyrings, this mutation leaks across tests. With getMetaMaskHdKeyrings now a memoized createSelector, the selector compares the keyrings array by reference — same reference means cache hit, so it returns stale results from a previous test even though the array contents were mutated. Using merge({}, defaultState, state) deep-clones into a fresh target so each test gets independent state.
hmalik88
marked this pull request as ready for review
March 17, 2026 02:25
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Contributor
Builds ready [3b6a671]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
ccharly
reviewed
Mar 18, 2026
Contributor
Builds ready [ce60d1b]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
david0xd
approved these changes
Mar 18, 2026
Contributor
Builds ready [ed6a18b]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
NidhiKJha
approved these changes
Mar 18, 2026
metamaskbot
added
the
release-13.24.0
Contributor
🧪 Validation RunVerdict: ✅ nothing moved — Claim: replacing Note Trial run of the MetaMask evidence skills — Swapping a deep-equality memo for a reference-equality one can raise recomputations, because a structurally-identical-but-new input now counts as a change. At the parent commit:
|
| Condition | Calls | Recomputations |
|---|---|---|
| Identical state reference | 5 | 1 |
Fresh metamask slice, unrelated field |
5 | 1 |
pinnedAccountList changed (a real input) |
5 | 6 |
Correctness: the returned value is identical across all calls above, so the count
measures memoisation rather than a change in behaviour.
$ git checkout --detach 5befef4073be906ee07d6b15108616ac6bc61f2a && yarn jest ui/selectors/multichain-accounts/__recompute_probe__.test.ts
RECOMPUTE_PROBE identical=1 unrelated=1 inputChanged=6 n=5 valueStable=true
Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Time: 4.309 s, estimated 9 sProduced by selector-recompute.sh via reselect's own counter; the probe is generated, run, and kept beside this artifact. head 5befef4073be906ee07d6b15108616ac6bc61f2a · 0 tracked changes · node v24.13.1. Run: https://github.com/MajorLift/metamask-skills/actions/runs/30765011348 — logs and artifacts attached there.
At the merge commit:
getWalletsWithAccounts recomputation count
Verdict: narrowed — unrelated writes cost nothing
| Condition | Calls | Recomputations |
|---|---|---|
| Identical state reference | 5 | 1 |
Fresh metamask slice, unrelated field |
5 | 1 |
pinnedAccountList changed (a real input) |
5 | 6 |
Correctness: the returned value is identical across all calls above, so the count
measures memoisation rather than a change in behaviour.
$ git checkout --detach ab2ea6ff92a25b044f97f7ab97e305e752c3cfe3 && yarn jest ui/selectors/multichain-accounts/__recompute_probe__.test.ts
RECOMPUTE_PROBE identical=1 unrelated=1 inputChanged=6 n=5 valueStable=true
Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Time: 8.835 sProduced by selector-recompute.sh via reselect's own counter; the probe is generated, run, and kept beside this artifact. head ab2ea6ff92a25b044f97f7ab97e305e752c3cfe3 · 0 tracked changes · node v24.13.1. Run: https://github.com/MajorLift/metamask-skills/actions/runs/30765011348 — logs and artifacts attached there.
Follows from the arms above
- Identical at both commits: one recomputation, and unrelated writes cost nothing. The swap did not move the count.
valueStable=truein both, so the counts measure memoisation rather than a change in what the selector returns.- This is the outcome the lane is least likely to produce and most needed for calibration: a check that only ever reports problems cannot be read as evidence when it reports none.
Open for review: one fixture, one perturbed key. getWalletsWithAccounts composes six input selectors, and this exercises the shapes the fixture reaches — a state this fixture does not construct could still distinguish the two memo strategies. The eight sibling selectors converted in the same change were not measured.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Account selectors across
multi-srp.ts,selectors.js, andaccount-tree.tswere usingcreateDeepEqualSelector, which uses lodashisEqualto recursively compare all input selector results on every call to decide whether to recompute. For account data, this is unnecessary overhead.Account objects come from immer-backed controllers. Immer uses structural sharing — when data hasn't changed, the reference is identical (===). This means the deep traversal was doing O(accounts × fields) work per call just to confirm what a reference check would confirm in O(1).
This PR replaces
createDeepEqualSelectorwith the most appropriate selector creator for each case:createSelector— for selectors whose inputs come entirely from immer state, where reference equality is sufficientcreateParameterizedSelector— for selectors that take a parameter (e.g. an account ID or address) and are called simultaneously for multiple values in list renders, requiring an LRU cache rather than a single cached resultcreateParameterizedShallowEqualSelector— for parameterized selectors whose inputs include arrays derived from upstream selectors. Shallow equality compares array elements by reference one level deep, which is correct since the elements are immer-backed objects. This handles edge cases where an upstream selector produces a new array containing the same element references, avoiding unnecessary recomputation without the cost of deep traversalTwo selectors were intentionally left as
createDeepEqualSelector:getAccountGroupsByScopesandgetAccountGroupsByAddress, because they take array arguments from callers rather than from immer state. Deep equality is legitimately useful there to avoid recomputation when the content is the same but the reference differs.Changes
ui/selectors/multi-srp/multi-srp.tsSelector:
isPrimaryHdAndFirstPartySnapAccountcreateDeepEqualSelector→createSelectorSelector:
getShouldShowSeedPhraseRemindercreateDeepEqualSelector→createSelectorui/selectors/selectors.jsSelector:
getMetaMaskHdKeyringscreateSelectorisPrimaryHdAndFirstPartySnapAccountSelector:
getAllPermittedAccountscreateParameterizedSelector(20)getCaipAccountIdsFromCaip25CaveatValue(...)or[]— new array every call regardless of state changegetOrderedConnectedAccountsForActiveTaband up togetWalletsWithAccountsSelector:
getOrderedConnectedAccountsForActiveTabcreateDeepEqualSelector→createSelectorgetAllPermittedAccountsfixSelector:
getUpdatedAndSortedAccountsWithCaipAccountIdcreateDeepEqualSelector→createSelectorgetUpdatedAndSortedAccounts) is immer-backed and already memoizedui/selectors/multichain-accounts/account-tree.tsSelector:
getWalletsWithAccountscreateDeepEqualSelector→createSelectorgetOrderedConnectedAccountsForActiveTab(now stable)Selector:
getNormalizedGroupsMetadatacreateDeepEqualSelector→createSelectorSelector:
getAllAccountGroupscreateDeepEqualSelector→createSelectorSelector:
getMultichainAccountGroups/getSingleAccountGroupscreateDeepEqualSelector→createSelectorgetAllAccountGroups(now stable)Selector:
getAccountGroupWithInternalAccountscreateDeepEqualSelector→createSelectorSelector:
getMultichainAccountsToScopesMapcreateDeepEqualSelector→createSelectorSelector:
getSelectedAccountGroupcreateDeepEqualSelector→createSelectorSelector:
getInternalAccountsFromGroupByIdcreateSelector(size 1) →createParameterizedSelector(20)groupIdcall in list rendersSelector:
getWalletIdAndNameByAccountAddresscreateDeepEqualSelector→createParameterizedSelector(20)Selector:
getMultichainAccountGroupByIdcreateDeepEqualSelector→createParameterizedSelector(5)Selector:
getCaip25IdByAccountGroupAndScopecreateDeepEqualSelector→createParameterizedSelector(5)+ input refactor{accountGroup, scope}into one object (new ref every call) — split into two pass-through input selectorsSelector:
getInternalAccountByGroupAndCaipcreateDeepEqualSelector→createParameterizedSelector(10)+ input refactorSelector:
getInternalAccountBySelectedAccountGroupAndCaipcreateDeepEqualSelector→createParameterizedSelector(10)Selector:
getInternalAccountListSpreadByScopesByGroupIdcreateDeepEqualSelector→createParameterizedShallowEqualSelector(20)getInternalAccountsFromGroupById— shallow equality handles the case where the array is new but elements are same immer referencesSelector:
getNetworkAddressCountcreateDeepEqualSelector→createParameterizedSelector(5)getInternalAccountListSpreadByScopesByGroupIdwhich is now stableSelector:
getIconSeedAddressByAccountGroupIdcreateDeepEqualSelector→createParameterizedShallowEqualSelector(20)getInternalAccountListSpreadByScopesByGroupId— array input, shallow equality is appropriateSelector:
getDefaultScopeAndAddressByAccountGroupIdcreateDeepEqualSelector→createParameterizedSelector(20)getInternalAccountListSpreadByScopesByGroupIdChangelog
CHANGELOG entry: null
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/6591
Manual testing steps
Screenshots/Recordings
N/A
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Touches core account/permission selectors used widely in rendering; changes are mostly memoization semantics, but incorrect parameterization or cache sizing could cause subtle stale data or UI update issues.
Overview
Reduces selector recomputation overhead in account- and multichain-related flows by replacing
createDeepEqualSelectorusages withcreateSelectorwhere referential equality is sufficient.Introduces/expands use of LRU-cached parameterized selectors (and shallow-equal variants) for selectors called with many different
address/groupId/scopeparameters in list renders, including refactors to avoid passing newly-created parameter objects that defeat memoization.Updates
getMetaMaskHdKeyringsandgetAllPermittedAccountsto be memoized selectors (including per-origin caching), and adjusts an account menu test store setup to avoid mutating default state duringmerge.Written by Cursor Bugbot for commit ed6a18b. This will update automatically on new commits. Configure here.