-
-
Notifications
You must be signed in to change notification settings - Fork 256
fix: improve add tokens through polling #7469
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?
Conversation
| this.#balanceFetchers = [ | ||
| ...(accountsApiChainIds().length > 0 && allowExternalServices() | ||
| ? [this.#createAccountsApiFetcher()] | ||
| : []), |
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.
we should not do this at the constructor level , this is init only when the worker restart , the new approach will have it changing even when the worker doesn't restart
| return { | ||
| // Dynamically check allowExternalServices() at call time, not just at construction time | ||
| supports: (chainId: ChainIdHex): boolean => | ||
| this.#allowExternalServices() && |
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.
we consider chains not supported for api fetch if basic func is off
| // This fixes a bug where the cache from construction time could be stale/empty | ||
| const { tokensChainsCache } = this.messenger.call( | ||
| 'TokenListController:getState', | ||
| ); |
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.
same here , we should fetch this value from the state to ensure we get last updated 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. |
Explanation
TokenBalancesControllerto evaluateallowExternalServicesdynamically instead of only at constructor time.#balanceFetchersarray was built once in the constructor, so changes toallowExternalServices()after initialization were not reflectedallowExternalServices()is stored as a function and evaluated dynamically in the fetcher'ssupportsmethodTokenDetectionControllermethodsaddDetectedTokensViaPollingandaddDetectedTokensViaWsto refresh token metadata cache before use#tokensChainsCachefrom construction timetokensChainsCachefromTokenListController:getStatebefore looking up token metadataExtension UI: MetaMask/metamask-extension#38841
Mobile UI: MetaMask/metamask-mobile#23864
References
Checklist
Note
Introduce onboarding-aware balance fetching, dynamic allowExternalServices checks, and fresh token metadata usage, with refactors and tests across balances and detection controllers.
isOnboardedoption;isActivenow requires unlocked + onboarded.supports()now evaluatesallowExternalServices()dynamically.isOnboardedoption; skiprefresh/syncBalanceWithAddresseswhen not onboarded.fromandtoaddresses on tx events.unprocessedChainIdsfallback; tighten typings, privatizesyncAccounts, safer event handling.tokensChainsCacheat call-time foraddDetectedTokensViaPolling/addDetectedTokensViaWs.CHANGELOG.mdwith added options and fixes; add mocks tests for Accounts API balances.Written by Cursor Bugbot for commit cb4a07d. This will update automatically on new commits. Configure here.