chore: integrate storageService with tokenListController #7413
Conversation
| tokensChainsCache: { | ||
| includeInStateLogs: false, | ||
| persist: true, | ||
| persist: false, // Persisted separately via StorageService |
There was a problem hiding this comment.
Making this false to block disk writes
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…oading and saving
…tchTokenList calls
| error, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Loaded storage data immediately re-persisted during initialization
Medium Severity
When #loadCacheFromStorage() loads data from storage and calls this.update(), it triggers #onCacheChanged via the subscription. At that moment, #previousTokensChainsCache is still {} (the initial value), so all loaded chains are detected as "new" and added to #changedChainsToPersist. Line 264 sets #previousTokensChainsCache only AFTER #loadCacheFromStorage() returns, but by then persistence is already scheduled. This causes all data just loaded from storage to be written back 500ms later, defeating the write amplification optimization that this PR aims to achieve.
Additional Locations (1)
| override destroy(): void { | ||
| super.destroy(); | ||
| this.stopPolling(); | ||
| this.#stopPolling(); |
There was a problem hiding this comment.
Debounce timer not cleared in destroy method
Medium Severity
The destroy() method stops polling but doesn't clear #persistDebounceTimer. If there's a pending debounced persistence operation when destroy() is called, the timer will still fire and attempt to call #persistChangedChains(), which uses this.messenger.call(...). This could cause errors or unexpected behavior after the controller is destroyed. The clearingTokenListData() method correctly clears this timer, suggesting the same cleanup should happen in destroy().
| error, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Loaded cache data immediately scheduled for re-persistence
Medium Severity
The comment at line 263 states "Initialize previous cache to prevent re-persisting loaded data" but this doesn't work as intended. When #loadCacheFromStorage() calls this.update() at line 397, it triggers the TokenListController:stateChange subscription synchronously. The callback #onCacheChanged executes while #previousTokensChainsCache is still {} (the initial value), causing all loaded chains to be detected as "new" and added to #changedChainsToPersist. Line 264 executes too late—after the debounced persistence is already scheduled. This causes unnecessary writes as all loaded cache data is immediately re-persisted to storage.
| this.#persistDebounceTimer = undefined; | ||
| } | ||
| this.#changedChainsToPersist.clear(); | ||
| this.#previousTokensChainsCache = {}; |
There was a problem hiding this comment.
Race between in-progress persistence and clearing operations
Medium Severity
If #persistChangedChains() is already executing (timer fired, async operation started) when clearingTokenListData() is called, clearing the debounce timer has no effect. The #persistChangedChains method copies chains to persist and then performs async setItem calls, while clearingTokenListData performs async removeItem calls. These operations can interleave, causing some chains to be re-saved after being removed. This results in data persisting in storage despite the user expecting it to be cleared.
Additional Locations (1)
| override destroy(): void { | ||
| super.destroy(); | ||
| this.stopPolling(); | ||
| this.#stopPolling(); |
There was a problem hiding this comment.
Debounce timer not cleared on destroy causes post-destruction errors
Medium Severity
The destroy() method does not clear #persistDebounceTimer. If there's a pending debounce when destroy() is called, the timer continues running and will fire after destruction, attempting to call this.messenger.call() on a potentially destroyed messenger. This inconsistency is evident since clearingTokenListData() properly clears this timer at lines 646-649, but destroy() does not. While errors are caught in #saveChainCacheToStorage, this creates a resource leak and may cause logged errors after destruction.
There was a problem hiding this comment.
this is fixed in destroy() it already has
if (this.#persistDebounceTimer) {
clearTimeout(this.#persistDebounceTimer);
this.#persistDebounceTimer = undefined;
}
| error, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Loaded cache unnecessarily re-persisted during initialization
Low Severity
The comment at line 263 claims "Initialize previous cache to prevent re-persisting loaded data," but this doesn't work as intended. When #loadCacheFromStorage() calls this.update(), the subscription fires #onCacheChanged() while #previousTokensChainsCache is still empty. This marks all loaded chains as "changed" and schedules a debounce. Setting #previousTokensChainsCache at line 264 happens after #loadCacheFromStorage() returns, but the debounce is already scheduled. When the timer fires, all chains just loaded from storage are unnecessarily re-persisted to the same storage. The #changedChainsToPersist set needs to be cleared after loading completes.
| error, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Loaded cache data unnecessarily re-persisted during initialization
Medium Severity
When initialize() is called and storage contains cached data, the loaded data is unnecessarily re-persisted to storage. The #previousTokensChainsCache is initialized to {} in the class field, and the state change subscription is set up in the constructor. When #loadCacheFromStorage() calls this.update(), it triggers #onCacheChanged, which compares against the empty #previousTokensChainsCache and detects all loaded chains as "new", scheduling them for persistence. The assignment at line 264 happens too late — the debounced persist has already been scheduled. This causes redundant storage writes on every initialization, undermining the performance optimization this PR aims to achieve.
Additional Locations (1)
|
@metamaskbot publish-preview |
| // Only remove loaded chains, not chains from initial state that need first persist | ||
| for (const chainId of Object.keys(loadedCache) as Hex[]) { | ||
| this.#changedChainsToPersist.delete(chainId); | ||
| } |
There was a problem hiding this comment.
Initialization removes chains from persistence queue incorrectly
Medium Severity
The #loadCacheFromStorage method removes ALL chains in loadedCache from #changedChainsToPersist, but loadedCache contains all chains from storage, not just chains that were actually added to state. When a chain exists in both initial state and storage, the update() call preserves the initial state's data (correctly), but the chain is still removed from the persistence queue (incorrectly). This means initial state data that should be persisted to storage is silently dropped if the same chain also exists in storage with older data.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| * @returns A promise that resolves when initialization is complete. | ||
| */ | ||
| async initialize(): Promise<void> { | ||
| this.#initializationPromise = this.#initializeFromStorage(); |
There was a problem hiding this comment.
Why are we wrapping loadCacheFromStorage with initializeFromStorage? Can't we assign this.#loadCacheFromStorage() directly here?
There was a problem hiding this comment.
We can yes; i missed cleaning this up; i think i was calling other functions initially
Description
Optimizes TokenListController storage to reduce write amplification by persisting tokensChainsCache via StorageService using per-chain files instead of a single monolithic state property.
Mobile: MetaMask/metamask-mobile#24019
Extension: MetaMask/metamask-extension#39250
Related: https://github.com/MetaMask/metamask-mobile/pull/22943/files
Related: https://github.com/MetaMask/decisions/pull/110
Related: #7192
Explanation
The tokensChainsCache (~5MB total, containing token lists for all chains) was persisted as part of the controller state. Every time a single chain's token list was updated (~100-500KB), the entire ~5MB cache was rewritten to disk, causing:
Solution
Per-Chain File Storage:
Each chain's cache is now stored in a separate file (e.g., tokensChainsCache:0x1, tokensChainsCache:0x89)
Only the updated chain (~100-500KB) is written on each token fetch, reducing write operations by ~90-95%
All chains are loaded in parallel at startup to maintain compatibility with TokenDetectionController
Key Changes:
References
Checklist
Note
Introduces per-chain persistence for
TokenListControllerviaStorageService, reducing write amplification and refining startup behavior.TokenListControllernow requiresawait controller.initialize();tokensChainsCachemetadata set topersist: falsetokensChainsCacheper chain using keys liketokensChainsCache:<chainId>; load all chains in parallel on init; skip redundant re-persistenceclearingTokenListData()is now async and removes all per-chain cache files before clearing stateDataCache; removes mutex-based lockingstart/restart/stop@metamask/storage-servicedependency and tsconfig references; updates CHANGELOG with breaking note and dependency bumpsWritten by Cursor Bugbot for commit 009e026. This will update automatically on new commits. Configure here.