-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: integrate token list controller storage service #24019
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
base: main
Are you sure you want to change the base?
feat: integrate token list controller storage service #24019
Conversation
|
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. |
| + if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it"); | ||
| + return (kind === "a" ? f.call(receiver, value) : f ? f.value = value : state.set(receiver, value)), value; | ||
| +}; | ||
| +var _TokenListController_instances, _a, _TokenListController_mutex, _TokenListController_storageKeyPrefix, _TokenListController_getChainStorageKey, _TokenListController_intervalId, _TokenListController_intervalDelay, _TokenListController_cacheRefreshThreshold, _TokenListController_chainId, _TokenListController_abortController, _TokenListController_loadCacheFromStorage, _TokenListController_saveChainCacheToStorage, _TokenListController_migrateStateToStorage, _TokenListController_onNetworkControllerStateChange, _TokenListController_stopPolling, _TokenListController_startDeprecatedPolling; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patch file to be removed after release, this is just for testing
| async getAllKeys(namespace: string): Promise<string[]> { | ||
| // eslint-disable-next-line no-console | ||
| console.warn(`[StorageService DEBUG] getAllKeys called: ${namespace}`); | ||
| const startTime = performance.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perf logs to be cleaned up
app/store/persistConfig/index.ts
Outdated
| const totalDuration = performance.now() - startTime; | ||
|
|
||
| // Log performance for TokenListController specifically | ||
| if (controllerName === 'TokenListController') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to be reverted; only for testing
| return allKeys | ||
| const filteredKeys = allKeys | ||
| .filter((key) => key.startsWith(prefix)) | ||
| .map((key) => key.slice(prefix.length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filter to only keys that belong to the requested namespace
.yarn/patches/@metamask-assets-controllers-npm-94.0.0-0b6df6cf06.patch
Outdated
Show resolved
Hide resolved
dfa8049 to
070448e
Compare
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsThis PR introduces significant changes to the TokenListController, a core component in the Engine that manages token list caching across multiple chains. Key changes:
These changes are HIGH RISK because:
Selected tags rationale:
|
| + if (Object.keys(loadedCache).length > 0) { | ||
| + this.update((state) => { | ||
| + state.tokensChainsCache = loadedCache; | ||
| + }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Async cache load overwrites concurrent fetch data
The #loadCacheFromStorage method runs asynchronously in the constructor without using the mutex that protects fetchTokenList. When it completes, it performs a complete replacement of tokensChainsCache (state.tokensChainsCache = loadedCache) rather than merging. If fetchTokenList runs and updates state while the async load is in progress, those updates are overwritten when #loadCacheFromStorage completes with stale data. This can cause freshly fetched token data to be lost from state, potentially requiring redundant API calls or causing temporary UI inconsistencies.
Additional Locations (1)
|



Description
Do not merge until this is released MetaMask/core#7413
Performance Comparison: Per-Chain Token Cache Storage
This PR implements per-chain file storage for
tokensChainsCacheinTokenListController, replacing the single-file approach. Each chain's token list is now stored in a separate file, reducing write amplification during incremental updates.📊 Complete Performance Comparison
Cold Restart
Main is ~44ms faster on cold restart (single file read vs parallel reads + getAllKeys overhead)
Onboarding
This PR writes 57% less data and is 3x faster
Add New Chain (Monad)
This PR is 121x smaller and 197x faster!
Summary
📋 Captured Logs
This PR - Cold Restart
This PR - Onboarding
This PR - Add New Chain (Monad)
Main Branch - Cold Restart
Main Branch - Onboarding
Main Branch - Add New Chain (Monad)
🔧 Performance Logging Code (Main Branch)
The following code was added to
app/store/persistConfig/index.tsto capture performance metrics:Read Performance Logging (getAllPersistedState)
Write Performance Logging (createPersistController)
🔧 Performance Logging Code (This PR)
The following code was added to
app/core/Engine/controllers/storage-service-init.tsto capture performance metrics for the per-chain storage:getItem - Read Performance Logging
setItem - Write Performance Logging
getAllKeys - Key Enumeration Logging
Changelog
CHANGELOG entry: integrates per chain file save for tokenListController.
Related issues
Related: MetaMask/core#7413
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Switches TokenListController to per-chain StorageService-backed caching with init migration, per-chain saves, and messenger wiring.
tokensChainsCache.persisttofalse; introduce per-chain storage keystokensChainsCache:<chainId>._loadCacheFromStorage,_saveChainCacheToStorage,_migrateStateToStorage,_getChainStorageKey,_stopPolling.{ data, timestamp }and persist only the affected chain; handle empty responses similarly.StorageService.DataCache; includeStorageServiceactions in messenger types; makeclearingTokenListDataasync.persistedState.TokenListControllerto controller init.StorageService:getAllKeys|getItem|setItem|removeItemactions intoken-list-controllermessenger.@metamask/storage-serviceand apply patch to@metamask/assets-controllers; updateyarn.lock.Written by Cursor Bugbot for commit 070448e. This will update automatically on new commits. Configure here.