From 021a2657b5fe0f958bf6aa5c57f121d37193e5a4 Mon Sep 17 00:00:00 2001 From: Brian Bergeron Date: Thu, 19 Dec 2024 12:10:15 -0800 Subject: [PATCH] fix: erc20 token balances showing 0 (#29361) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Fixes an issue where erc20 token balances were incorrectly showing 0. On the repro we have, we noticed a token in state with address `0x0000000000000000000000000000000000000000` on mainnet, which is not a valid erc20 token. This caused the multicall to revert, preventing other balances from updating. There's a fix in the controller here: https://github.com/MetaMask/core/pull/5083 which will fall back to parallel `balanceOf` calls if the multicall reverts. And we're also doing a migration here to remove zero address tokens on mainnet. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29361?quickstart=1) ## **Related issues** ## **Manual testing steps** The current version of the wallet should not allow importing an invalid erc20 address through any mechanism, so not easy to reproduce naturally. The migration can be tested by checking out an older version like `git checkout v12.9.0 `, upgrading to this branch, verifying the migration ran in background logs, and that your tokens remain. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- ...-assets-controllers-patch-d6ed5f8213.patch | 61 ++++++ app/scripts/migrations/133.2.test.ts | 185 ++++++++++++++++++ app/scripts/migrations/133.2.ts | 53 +++++ app/scripts/migrations/index.js | 1 + package.json | 2 +- yarn.lock | 43 +++- 6 files changed, 342 insertions(+), 3 deletions(-) create mode 100644 .yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch create mode 100644 app/scripts/migrations/133.2.test.ts create mode 100644 app/scripts/migrations/133.2.ts diff --git a/.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch b/.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch new file mode 100644 index 000000000000..02e6d3f694e5 --- /dev/null +++ b/.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch @@ -0,0 +1,61 @@ +diff --git a/dist/multicall.cjs b/dist/multicall.cjs +index bf9aa5e86573fc1651f421cc0b64f5af121c3ab2..43a0531ed86cd3ee1774dcda3f990dd40f7f52de 100644 +--- a/dist/multicall.cjs ++++ b/dist/multicall.cjs +@@ -342,9 +342,22 @@ const multicallOrFallback = async (calls, chainId, provider, maxCallsPerMultical + return []; + } + const multicallAddress = MULTICALL_CONTRACT_BY_CHAINID[chainId]; +- return await (multicallAddress +- ? multicall(calls, multicallAddress, provider, maxCallsPerMulticall) +- : fallback(calls, maxCallsParallel)); ++ if (multicallAddress) { ++ try { ++ return await multicall(calls, multicallAddress, provider, maxCallsPerMulticall); ++ } ++ catch (error) { ++ // Fallback only on revert ++ // https://docs.ethers.org/v5/troubleshooting/errors/#help-CALL_EXCEPTION ++ if (!error || ++ typeof error !== 'object' || ++ !('code' in error) || ++ error.code !== 'CALL_EXCEPTION') { ++ throw error; ++ } ++ } ++ } ++ return await fallback(calls, maxCallsParallel); + }; + exports.multicallOrFallback = multicallOrFallback; + //# sourceMappingURL=multicall.cjs.map +\ No newline at end of file +diff --git a/dist/multicall.mjs b/dist/multicall.mjs +index 8fbe0112303d5df1d868e0357a9d31e43a3b6cf9..860dfdbddd813659cb2be5f7faed5d4016db5966 100644 +--- a/dist/multicall.mjs ++++ b/dist/multicall.mjs +@@ -339,8 +339,21 @@ export const multicallOrFallback = async (calls, chainId, provider, maxCallsPerM + return []; + } + const multicallAddress = MULTICALL_CONTRACT_BY_CHAINID[chainId]; +- return await (multicallAddress +- ? multicall(calls, multicallAddress, provider, maxCallsPerMulticall) +- : fallback(calls, maxCallsParallel)); ++ if (multicallAddress) { ++ try { ++ return await multicall(calls, multicallAddress, provider, maxCallsPerMulticall); ++ } ++ catch (error) { ++ // Fallback only on revert ++ // https://docs.ethers.org/v5/troubleshooting/errors/#help-CALL_EXCEPTION ++ if (!error || ++ typeof error !== 'object' || ++ !('code' in error) || ++ error.code !== 'CALL_EXCEPTION') { ++ throw error; ++ } ++ } ++ } ++ return await fallback(calls, maxCallsParallel); + }; + //# sourceMappingURL=multicall.mjs.map +\ No newline at end of file diff --git a/app/scripts/migrations/133.2.test.ts b/app/scripts/migrations/133.2.test.ts new file mode 100644 index 000000000000..18251d8f4b2b --- /dev/null +++ b/app/scripts/migrations/133.2.test.ts @@ -0,0 +1,185 @@ +import { migrate, version } from './133.2'; + +const oldVersion = 133.1; + +describe(`migration #${version}`, () => { + it('updates the version metadata', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.meta).toStrictEqual({ version }); + }); + + it('does nothing if theres no tokens controller state defined', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if theres empty tokens controller state', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + TokensController: {}, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if theres empty tokens controller state for allTokens', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: {}, + }, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if theres empty tokens controller state for mainnet', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: { + '0x1': {}, + }, + }, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('Does nothing if theres no tokens with empty address', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: { + '0x1': { + '0x123': [ + { address: '0x1', symbol: 'TOKEN1', decimals: 18 }, + { address: '0x2', symbol: 'TOKEN2', decimals: 18 }, + ], + '0x123456': [ + { address: '0x3', symbol: 'TOKEN3', decimals: 18 }, + { address: '0x4', symbol: 'TOKEN4', decimals: 18 }, + ], + }, + }, + }, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('Removes tokens with empty address', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: { + '0x1': { + '0x123': [ + { + address: '0x0000000000000000000000000000000000000000', + symbol: 'eth', + decimals: 18, + }, + { address: '0x2', symbol: 'TOKEN2', decimals: 18 }, + ], + }, + }, + }, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual({ + TokensController: { + allTokens: { + '0x1': { + '0x123': [{ address: '0x2', symbol: 'TOKEN2', decimals: 18 }], + }, + }, + }, + }); + }); + + it('Removes tokens with empty address across multiple accounts', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: { + '0x1': { + '0x123': [ + { + address: '0x0000000000000000000000000000000000000000', + symbol: 'eth', + decimals: 18, + }, + { address: '0x2', symbol: 'TOKEN2', decimals: 18 }, + ], + '0x456': [ + { + address: '0x0000000000000000000000000000000000000000', + symbol: 'eth', + decimals: 18, + }, + { address: '0x3', symbol: 'TOKEN3', decimals: 18 }, + ], + '0x789': [{ address: '0x4', symbol: 'TOKEN4', decimals: 18 }], + }, + }, + }, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual({ + TokensController: { + allTokens: { + '0x1': { + '0x123': [{ address: '0x2', symbol: 'TOKEN2', decimals: 18 }], + '0x456': [{ address: '0x3', symbol: 'TOKEN3', decimals: 18 }], + '0x789': [{ address: '0x4', symbol: 'TOKEN4', decimals: 18 }], + }, + }, + }, + }); + }); + + it('Does not change state on chains other than mainnet', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: { + '0x999': { + '0x123': [ + { + address: '0x0000000000000000000000000000000000000000', + symbol: 'eth', + decimals: 18, + }, + { address: '0x2', symbol: 'TOKEN2', decimals: 18 }, + ], + }, + }, + }, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); +}); diff --git a/app/scripts/migrations/133.2.ts b/app/scripts/migrations/133.2.ts new file mode 100644 index 000000000000..6ad8ff888cfd --- /dev/null +++ b/app/scripts/migrations/133.2.ts @@ -0,0 +1,53 @@ +import { hasProperty, isObject } from '@metamask/utils'; +import { cloneDeep } from 'lodash'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +export const version = 133.2; + +/** + * This migration removes tokens on mainnet with the + * zero address, since this is not a valid erc20 token. + * + * @param originalVersionedData - Versioned MetaMask extension state, exactly + * what we persist to disk. + * @returns Updated versioned MetaMask extension state. + */ +export async function migrate( + originalVersionedData: VersionedData, +): Promise { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +function transformState(state: Record): void { + if ( + !hasProperty(state, 'TokensController') || + !isObject(state.TokensController) || + !isObject(state.TokensController.allTokens) + ) { + return; + } + + const chainIds = ['0x1']; + + for (const chainId of chainIds) { + const allTokensOnChain = state.TokensController.allTokens[chainId]; + + if (isObject(allTokensOnChain)) { + for (const [account, tokens] of Object.entries(allTokensOnChain)) { + if (Array.isArray(tokens)) { + allTokensOnChain[account] = tokens.filter( + (token) => + token?.address !== '0x0000000000000000000000000000000000000000', + ); + } + } + } + } +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 6673d1e62d2f..8700b833d8de 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -156,6 +156,7 @@ const migrations = [ require('./132'), require('./133'), require('./133.1'), + require('./133.2'), require('./134'), ]; diff --git a/package.json b/package.json index 71373b3f12c7..4534c2c15c2f 100644 --- a/package.json +++ b/package.json @@ -286,7 +286,7 @@ "@metamask/address-book-controller": "^6.0.0", "@metamask/announcement-controller": "^7.0.0", "@metamask/approval-controller": "^7.0.0", - "@metamask/assets-controllers": "patch:@metamask/assets-controllers@npm%3A45.1.0#~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch", + "@metamask/assets-controllers": "patch:@metamask/assets-controllers@patch%3A@metamask/assets-controllers@npm%253A45.1.0%23~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch%3A%3Aversion=45.1.0&hash=cfcadc#~/.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch", "@metamask/base-controller": "^7.0.0", "@metamask/bitcoin-wallet-snap": "^0.8.2", "@metamask/browser-passworder": "^4.3.0", diff --git a/yarn.lock b/yarn.lock index d10097abd78e..ed1c8a1de350 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4983,7 +4983,7 @@ __metadata: languageName: node linkType: hard -"@metamask/assets-controllers@patch:@metamask/assets-controllers@npm%3A45.1.0#~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch": +"@metamask/assets-controllers@patch:@metamask/assets-controllers@npm%3A45.1.0#~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch::version=45.1.0&hash=cfcadc": version: 45.1.0 resolution: "@metamask/assets-controllers@patch:@metamask/assets-controllers@npm%3A45.1.0#~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch::version=45.1.0&hash=cfcadc" dependencies: @@ -5022,6 +5022,45 @@ __metadata: languageName: node linkType: hard +"@metamask/assets-controllers@patch:@metamask/assets-controllers@patch%3A@metamask/assets-controllers@npm%253A45.1.0%23~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch%3A%3Aversion=45.1.0&hash=cfcadc#~/.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch": + version: 45.1.0 + resolution: "@metamask/assets-controllers@patch:@metamask/assets-controllers@patch%3A@metamask/assets-controllers@npm%253A45.1.0%23~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch%3A%3Aversion=45.1.0&hash=cfcadc#~/.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch::version=45.1.0&hash=4e79dd" + dependencies: + "@ethereumjs/util": "npm:^8.1.0" + "@ethersproject/abi": "npm:^5.7.0" + "@ethersproject/address": "npm:^5.7.0" + "@ethersproject/bignumber": "npm:^5.7.0" + "@ethersproject/contracts": "npm:^5.7.0" + "@ethersproject/providers": "npm:^5.7.0" + "@metamask/abi-utils": "npm:^2.0.3" + "@metamask/base-controller": "npm:^7.0.2" + "@metamask/contract-metadata": "npm:^2.4.0" + "@metamask/controller-utils": "npm:^11.4.3" + "@metamask/eth-query": "npm:^4.0.0" + "@metamask/metamask-eth-abis": "npm:^3.1.1" + "@metamask/polling-controller": "npm:^12.0.1" + "@metamask/rpc-errors": "npm:^7.0.1" + "@metamask/utils": "npm:^10.0.0" + "@types/bn.js": "npm:^5.1.5" + "@types/uuid": "npm:^8.3.0" + async-mutex: "npm:^0.5.0" + bn.js: "npm:^5.2.1" + cockatiel: "npm:^3.1.2" + immer: "npm:^9.0.6" + lodash: "npm:^4.17.21" + multiformats: "npm:^13.1.0" + single-call-balance-checker-abi: "npm:^1.0.0" + uuid: "npm:^8.3.2" + peerDependencies: + "@metamask/accounts-controller": ^20.0.0 + "@metamask/approval-controller": ^7.0.0 + "@metamask/keyring-controller": ^19.0.0 + "@metamask/network-controller": ^22.0.0 + "@metamask/preferences-controller": ^15.0.0 + checksum: 10/26260472e67d0995b3730870fed99ba081c421ea64e8ca70f02ca8184fb9350fd2c607b75f45507743ba73d7336e831cc55a7aaf9a32f569a58eb7abb9275451 + languageName: node + linkType: hard + "@metamask/auto-changelog@npm:^2.1.0": version: 2.6.1 resolution: "@metamask/auto-changelog@npm:2.6.1" @@ -26540,7 +26579,7 @@ __metadata: "@metamask/announcement-controller": "npm:^7.0.0" "@metamask/api-specs": "npm:^0.9.3" "@metamask/approval-controller": "npm:^7.0.0" - "@metamask/assets-controllers": "patch:@metamask/assets-controllers@npm%3A45.1.0#~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch" + "@metamask/assets-controllers": "patch:@metamask/assets-controllers@patch%3A@metamask/assets-controllers@npm%253A45.1.0%23~/.yarn/patches/@metamask-assets-controllers-npm-45.1.0-d914c453f0.patch%3A%3Aversion=45.1.0&hash=cfcadc#~/.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch" "@metamask/auto-changelog": "npm:^2.1.0" "@metamask/base-controller": "npm:^7.0.0" "@metamask/bitcoin-wallet-snap": "npm:^0.8.2"