-
Notifications
You must be signed in to change notification settings - Fork 5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: erc20 token balances showing 0 (#29361)
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: MetaMask/core#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. [](https://codespaces.new/MetaMask/metamask-extension/pull/29361?quickstart=1) 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. <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> <!-- [screenshots/recordings] --> <!-- [screenshots/recordings] --> - [ ] 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. - [ ] 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.
- Loading branch information
](https://codespaces.new/MetaMask/metamask-extension/pull/29361?quickstart=1){kind=link}
Showing
6 changed files
with
342 additions
and
3 deletions.
There are no files selected for viewing
61 changes: 61 additions & 0 deletions
61
.yarn/patches/@metamask-assets-controllers-patch-d6ed5f8213.patch
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| }); | ||
| }); |
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import { hasProperty, isObject } from '@metamask/utils'; | ||
| import { cloneDeep } from 'lodash'; | ||
|
|
||
| type VersionedData = { | ||
| meta: { version: number }; | ||
| data: Record<string, unknown>; | ||
| }; | ||
|
|
||
| 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<VersionedData> { | ||
| const versionedData = cloneDeep(originalVersionedData); | ||
| versionedData.meta.version = version; | ||
| transformState(versionedData.data); | ||
| return versionedData; | ||
| } | ||
|
|
||
| function transformState(state: Record<string, unknown>): 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', | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains 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
This file contains 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
This file contains 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