fix(assets-controller): token datasource filtering - #10172
fix(assets-controller): token datasource filtering#10172Prithpal-Sooriya wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
real api data of https://accounts.api.cx.metamask.io/v2/supportedNetworks at the time
There was a problem hiding this comment.
real api data of https://accounts.api.cx.metamask.io/v5/multiaccount/balances?accountIds=eip155%3A56%3A0xAAAAA at the time
There was a problem hiding this comment.
real data of https://tokens.api.cx.metamask.io/v2/supportedNetworks at the time
There was a problem hiding this comment.
real data of roughly https://token.api.cx.metamask.io/assets?assetIds=eip155:56/erc20:0xa7255c85232a42b5c602ed66c319da9af8433bb3&includeOccurrences=true at the time
There was a problem hiding this comment.
real data of https://token.api.cx.metamask.io/v1/suggestedOccurrenceFloors at the time
There was a problem hiding this comment.
real data of https://price.api.cx.metamask.io/v2/supportedNetworks at the time
There was a problem hiding this comment.
read data of https://price.api.cx.metamask.io/v3/spot-prices?assetIds= at the time
Prithpal-Sooriya
changed the title
There was a problem hiding this comment.
This file formulates the nock management.
There was a problem hiding this comment.
integration test utils
There was a problem hiding this comment.
integration test constants
| // Legitimate failing test, our middleware stack does not filter out spam asset prices! | ||
| // This does eventually get cleaned up during unlock cleanup, but worth flagging. | ||
| // eslint-disable-next-line jest/no-disabled-tests | ||
| it.skip('does not carry a price for the spam token', async () => { | ||
| const { response } = await runPipeline(buildEmptyAssetsState()); | ||
| expect( | ||
| getIgnoringCase(response.assetsPrice ?? {}, CDOGE_ASSET_ID_LOWERCASE), | ||
| ).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Mentioned in the PR recording, we don't prevent spam prices from being added due to our architecture...
Not a dealbreaker since we do end up cleaning during the unlock lifecycle.
use test tables for a cleaner test implementation
This commit cleans up the integration test for the BSC spam token filtering by removing unnecessary functions and simplifying the logic for balance and asset ID retrieval. The changes enhance code readability and maintainability while ensuring the test remains functional.
…oller-token-datasource-spam-bypass
| for (const accountBalances of Object.values(response.assetsBalance)) { | ||
| for (const assetId of Object.keys(accountBalances)) { | ||
| if (spamLowerIds.has(assetId.toLowerCase())) { | ||
| delete (accountBalances as Record<string, unknown>)[assetId]; |
There was a problem hiding this comment.
deletion is now using the exact keys from the object - avoids this assetId possible casing issues.
| if (response.assetsInfo) { | ||
| for (const assetId of Object.keys(response.assetsInfo)) { | ||
| if (spamLowerIds.has(assetId.toLowerCase())) { | ||
| delete response.assetsInfo[assetId as Caip19AssetId]; |
There was a problem hiding this comment.
deletion is now using the exact keys from the object - avoids this assetId possible casing issues.
| response.detectedAssets[accountId] = assetIds.filter( | ||
| (id) => !spamLowerIds.has(id.toLowerCase()), | ||
| ); |
There was a problem hiding this comment.
deletion is now using the exact keys from the object - avoids this assetId possible casing issues.
There was a problem hiding this comment.
The main fun integration test 😄
I think we can consider making a skill.
- Using extension open network tab and perform your actions.
- Export the network .har
- Have and agent (+ scripts) to create these mock fixtures for the integration test 🎉
Likewise you can also give the agent the account address, chain, and tokens you want to examine - it can make real API calls to then build fixtures.
| // Legitimate failing test, our middleware stack does not filter out spam | ||
| // asset prices! This does eventually get cleaned up during unlock cleanup, | ||
| // but worth flagging. | ||
| // eslint-disable-next-line jest/no-disabled-tests | ||
| it.skip('keeps the spam token out of prices', () => { | ||
| expect(PRICES.lookUp(response, CDOGE_ASSET_ID_LOWERCASE)).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Discussed in the PR walkthrough.
Because our prices are ran in parallel with TokenDataSource, the PriceDataSource doesn't really have information to do any cleanup.
Not a deal-breaker, we will perform cleanup during unlock.
Our messenger mocks were poor: - improper messenger registrations on global vs scoped messengers - hacks on overwriting messenger publish - mock subscriptions were out of order. I've cleaned up the messenger to correctly manage rootMessenger vs scopedMessenger actions and events. Also cleaned up the spam token integration messenger tests -- makes the integration test itself cleaner.
We have a code-smell, there should be no reason for our internal logic to call its own messenger to get state...
There was a problem hiding this comment.
sets up the messenger for these integration tests
| type MessengerWithPublish = { | ||
| publish: (event: string, ...args: unknown[]) => void; | ||
| registerActionHandler: ( | ||
| action: string, | ||
| handler: (...args: unknown[]) => unknown, | ||
| ) => void; | ||
| }; |
There was a problem hiding this comment.
Fixed test smell. Instead of hacking & injecting the messenger, use the events/actions on the global root messenger.
| rootMessenger: MockRootMessenger; | ||
| assetsControllerMessenger: AssetsControllerMessenger; | ||
| } { | ||
| const { delegateGetState = true } = options ?? {}; |
There was a problem hiding this comment.
Fixed test smell: none of our code should be referencing internal actions. Added a comment in the RPCDataSource test.
There was a problem hiding this comment.
Revamped our internal test utils.
Instead of test code smells (like promise flushing), please use the waitFor or waitUntilStable utilities.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bfc8864. Configure here.
| // TODO - code smell, why is our internal logic trying to call its own methods via messenger? | ||
| registerAssetsControllerStateMock( | ||
| assetsControllerMessenger, | ||
| actionHandlerOverrides?.['AssetsController:getState'] as | ||
| | (() => AssetsControllerState) | ||
| | undefined, | ||
| ); |
There was a problem hiding this comment.
Cleaned up test messenger.
As commented above, we shouldn't need to use the internal messenger to call the controller itself. Just use a callback and get state directly.
| const { rootMessenger, assetsControllerMessenger } = createMockMessengers({ | ||
| registerCustomRootActions: (messenger) => | ||
| registerStakedMessengerActions(messenger, { | ||
| enabledNetworkMap, | ||
| mockProvider, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Messenger mock cleanup.
There was a problem hiding this comment.
Main fix is in this file 🎉
There was a problem hiding this comment.
Gut logic out of AssetsController so we can simplify the controller and also test in isolation!
This builds the fast lane, we can move other pipelines to this folder too as a fast follow.
There was a problem hiding this comment.
Gutted pipeline execution from the AssetsController. The AssetsController should not have owned this responsibility.
This code for is a 1:1.
There was a problem hiding this comment.
Also created an integration test on the AssetsController too 🎉
| const { rootMessenger, assetsControllerMessenger } = createMockMessengers({ | ||
| registerCustomRootActions: (messenger) => | ||
| registerAssetsControllerActions(messenger, { | ||
| accounts, | ||
| enabledNetworkMap: { eip155: { '1': true, '10': true, '8453': true } }, | ||
| nativeAssetIdentifiers: SCAM_WALLET_NATIVE_ASSET_IDENTIFIERS, | ||
| remoteFeatureFlags, | ||
| }), |
There was a problem hiding this comment.
Messenger mock reformatting.
| const { rootMessenger, assetsControllerMessenger } = createMockMessengers({ | ||
| registerCustomRootActions: (messenger) => | ||
| registerAssetsControllerActions(messenger, { | ||
| accounts, | ||
| enabledNetworkMap: { eip155: { '1': true, '10': true } }, | ||
| nativeAssetIdentifiers: { 'eip155:1': MAINNET_NATIVE }, | ||
| remoteFeatureFlags, | ||
| }), |
There was a problem hiding this comment.
Messenger mock reformatting.
| const result = await chain({ | ||
| request, | ||
| response: initialResponse, | ||
| return executeAssetsPipeline({ |
There was a problem hiding this comment.
Gutted this execution logic to executeAssetsPipeline.ts
| const fastSources = buildFastFetchSources( | ||
| { | ||
| accountsApiDataSource: this.#accountsApiDataSource, | ||
| stakedBalanceDataSource: this.#stakedBalanceDataSource, | ||
| customAssetGraduationMiddleware: | ||
| this.#customAssetGraduationMiddleware, | ||
| this.#rpcFallbackMiddleware, | ||
| this.#detectionMiddleware, | ||
| createParallelMiddleware([ | ||
| this.#tokenDataSource, | ||
| this.#priceDataSource, | ||
| ]), | ||
| ] | ||
| : [this.#stakedBalanceDataSource, this.#detectionMiddleware]; | ||
| rpcFallbackMiddleware: this.#rpcFallbackMiddleware, | ||
| detectionMiddleware: this.#detectionMiddleware, | ||
| tokenDataSource: this.#tokenDataSource, | ||
| priceDataSource: this.#priceDataSource, | ||
| }, | ||
| { isBasicFunctionality: this.#isBasicFunctionality() }, | ||
| ); |
There was a problem hiding this comment.
Gutted the fast lane pipeline generation to buildFastFetchSources.ts
This allows us better isolation for testing, debugging, visibility, etc.
There was a problem hiding this comment.
We can do the other pipelines later, this PR is already too big.
| }, | ||
| "devDependencies": { | ||
| "@metamask/auto-changelog": "^6.1.0", | ||
| "@metamask/eth-json-rpc-provider": "^7.0.0", |
There was a problem hiding this comment.
Dev dependency only, aligned to best practice on RPC mocking.
…n_rpc_provider - Added eth_json_rpc_provider to the assets_controller diagram in README.md. - Updated tsconfig.build.json and tsconfig.json to include path for eth-json-rpc-provider.
Prithpal-Sooriya
changed the title
…oller-token-datasource-spam-bypass
…oller-token-datasource-spam-bypass
1 participant
Explanation
Added integration tests to prove hole in scam filtering system.
Added a simple patch to correctly filter out scam tokens.
Demo
https://www.loom.com/share/6c5bd9f2517b42d7920799616231d2fe
Code Walkthrough
https://www.loom.com/share/ef93d877b2d24d6f8d382f34ac7c3a3d
References
N/A
Checklist
Note
Medium Risk
Changes how spam tokens are removed from fetch responses before state is updated; incorrect matching could hide legitimate tokens or leave spam visible, though integration coverage targets the reported BSC case.
Overview
Fixes low-occurrence spam tokens (e.g. BNB Chain CDOGE) slipping into wallet state when asset IDs differ by checksum casing.
TokenDataSourcenow centralizes removal incleanResponseSpam, stripping filtered IDs from balances, metadata, and detected assets with case-insensitive matching instead of deleting balances by exact key only.The fast fetch lane is extracted into a
pipelinemodule (buildFastFetchSources,executeAssetsPipeline) so the same stack can run in integration tests without bootingAssetsController. New BSC wallet integration tests (pipeline + full controller) lock in the CDOGE behavior;it.failingtests document that prices are still not occurrence-filtered.Test helpers were refactored (
createMockMessengers, BSC fixtures,waitUntilStable) and@metamask/eth-json-rpc-providerwas added as a dev dependency for RPC mocks.Reviewed by Cursor Bugbot for commit 01e0cbb. Bugbot is set up for automated code reviews on this repo. Configure here.