fix(assets-controller): implement spam asset cleanup functionality - #9973
Open
Prithpal-Sooriya wants to merge 22 commits into
Open
fix(assets-controller): implement spam asset cleanup functionality#9973Prithpal-Sooriya wants to merge 22 commits into
Prithpal-Sooriya wants to merge 22 commits into
Conversation
Adds spam cleanup for users. This is because our external sources can fail and add in spam: - API filters can loosen, and add in spam tokens. - WS has no filtering right now, so airdrops are added to the wallet. This safeguards ourselves by adding additional cleanup on startup/unlock. We need a longer term discussion & decision with Assets team to highlight this cleanup issue and building a trustless system.
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
…)/add-asset-cleanup-process-on-unlock Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # packages/assets-controller/CHANGELOG.md
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
Author
|
@cursor /babysit fix CI |
|
Taking a look!   |
- Add unit tests for cleanSpamAssets covering candidate filtering, spam removal, API failures, and occurrence floor fallback - Add AssetsController integration tests for unlock-triggered cleanup - Fix import order in healAssetsInfoMetadata.ts for lint:misc:check Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
- Unit test via healAssetsInfoMetadata.test.ts - More ingrained integration test via AssetsController.spam-cleanup.test.ts Also created some test mocks and utils via __fixtures__
There were small changes made to mocks... meh not a big deal.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
… cleanup The previous implementation of #runSpamCleanup would replace the assetsInfo and assetsBalance state wholesale with a snapshot taken before awaiting the Token API. This caused any legitimate updates to those state slices that occurred concurrently to be dropped. This commit updates the logic to compute the set of removed spam assets and delete only those specific assets from the current state, preserving any concurrent additions or modifications. Co-authored-by: Cursor <cursoragent@cursor.com>
…ock' of github.com:MetaMask/core into fix(assets-controller)/add-asset-cleanup-process-on-unlock
- Re-apply selective spam deletion in #runSpamCleanup so concurrent state updates during the Token API await are not overwritten - Bump eslint suppression count for AssetsController stateChange usage - Fix formatting in spam-cleanup test file Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
…date Mutate the Immer draft in place with type assertions instead of spreading assetsInfo/assetsBalance, matching the pattern used in handleAssetsUpdate. Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
This was an edge-case caught from E2E tests on clients.
…ock' of github.com:MetaMask/core into fix(assets-controller)/add-asset-cleanup-process-on-unlock
Trigger spam cleanup when UI, keyring, and account-tree preconditions are first satisfied, not only on unlock. Update spam-cleanup tests to activate the full lifecycle and relax assertions affected by concurrent tracking. Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
…ock' of github.com:MetaMask/core into fix(assets-controller)/add-asset-cleanup-process-on-unlock
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.
Reviewed by Cursor Bugbot for commit c2d9d46. Configure here.
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
| return assetIds.filter((assetId) => { | ||
| const chainReference = assetId.split(':')[1]?.split('/')[0] ?? ''; | ||
| const floor = floors[chainReference] ?? DEFAULT_OCCURRENCE_FLOOR; | ||
| return (occurrencesByLowerId.get(assetId.toLowerCase()) ?? 0) < floor; |
Contributor
Author
There was a problem hiding this comment.
Correction we are using fail closed logic.
If, for whatever reason, the token API has not returned a token, well assume that it is zero and filter it out.
A bit aggressive but this protects us for edge cases where the token API does not have this data.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Explanation
Adds spam cleanup for users. This is additional filtering to ensure we are protected if any external system adds in spam (WS, API, Migration).
This is because our external sources can fail and add in spam:
This safeguards ourselves by adding additional cleanup on startup/unlock. We need a longer term discussion & decision with Assets team to highlight this cleanup issue and building a trustless system.
Builds:
References
https://consensyssoftware.atlassian.net/browse/ASSETS-3905
Checklist
Note
Medium Risk
Unlock now mutates persisted asset state using Token API occurrence heuristics, which can remove legitimate thinly-listed tokens and depends on API availability at unlock time.
Overview
Runs a Token API–backed spam sweep when the wallet unlocks, so airdrop junk that slipped in via websocket, loosened API filters, or migrations is dropped from persisted
assetsInfoandassetsBalancewithout waiting for the next balance fetch.On
KeyringController:unlock,AssetsControllernow callscleanSpamAssets(only when the keyring is unlocked and basic functionality is on). That helper picks sweepable ERC-20s on Accounts-API chains (skipping default-tracked tokens, user-importedcustomAssets, native/non-EVM assets), loads suggested occurrence floors and/v3/assetsoccurrence counts in batches of 50, and removes tokens below the per-chain floor. API failures are fail-closed (state unchanged, optionalcaptureException); successful sweeps apply via a singleupdatepatch. Prices, preferences, and imported tokens are left alone.Adds
nock-based unit/integration coverage (AssetsController.spam-cleanup.test.ts, expandedhealAssetsInfoMetadatatests) plus shared test fixtures and mock-messenger helpers for unlock-driven flows.Reviewed by Cursor Bugbot for commit e68ab14. Bugbot is set up for automated code reviews on this repo. Configure here.