fix: fix import ERC20 on network 1337

[sahar-fehri]

Explanation

This PR fixes importing an ERC20 token on chainId 1337.
This issue is on extension and Mobile.

When you run a local ganache node, you use testDapp to deploy a token, then you try to add the token to your wallet; and go back to home page but you wont be able to see the token.

The error starts on this line. It looks like it fails to pass that line to create the newEntry and push it to newTokens array.
The error is thrown on this line (See screenshot)

I see that tokenApi does not support chainId 1337 and the error is being thrown here https://github.com/consensys-vertical-apps/va-mmcx-token-api/blob/6c1b749c33a110f9dd8004dd283f6d9f7dcb4824/src/server.ts#L522

that is because networkConfig does not contain the 1337 chainId.

After testing also with Goerli testnet, where importing ERC20 works fine, i noticed that goerli is not supported on core so the fetchTokenMetadata threw here because isTokenDetectionSupportedForNetwork returned false

core/packages/assets-controllers/src/assetsUtil.ts

Line 156 in f8437e9

isTokenDetectionSupportedForNetwork(chainId) || chainId === GANACHE_CHAIN_ID

. I wasnt sure why we did not treat chainId 1337 same way as goerli.
So i removed the || chainId === GANACHE_CHAIN_ID check on the utils fct.

After the fix, i tested importing the ERC20, importing ERC721/1155, works fine.

Lemme know if anything! 🙏

Video Before

Screen.Recording.2024-01-12.at.18.08.29.mov

Video After

Screen.Recording.2024-01-12.at.17.16.10.mov

References

• Fixes [Bug] Cannot import an ERC20 token when on a network id 1337 metamask-mobile#6410

Changelog

@metamask/assets-controllers

• : Updates the fct on assetsUtils isTokenListSupportedForNetwork and removed the check for GANACHE_CHAIN_ID

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[mikesposito]

Changes look good, but I also do not have enough context on why GANACHE_CHAIN_ID is treated as an exception: perhaps it is a residual of what we had before the CoinGecko issue, or it is useful for e2e tests?

Were you able to run e2e tests on extension, using this PR's version of assets controllers?

[sahar-fehri]

Changes look good, but I also do not have enough context on why GANACHE_CHAIN_ID is treated as an exception: perhaps it is a residual of what we had before the CoinGecko issue, or it is useful for e2e tests?

Were you able to run e2e tests on extension, using this PR's version of assets controllers?

Hey @mikesposito ! I made a small update so my change does modify the isTokenListSupportedForNetwork fct cause it seems to be also used in TokensListController, noticed that the test test/e2e/tests/add-hide-token.spec.js was failing, but it passes now with latest update 🙏

[mcmire]

Sorry, I don't remember what was here before (it would be better in the future to not squash commits so we can always look back at your previous changes). If the goal of this function is to determine whether a chain supports token lists, what do you think about modifying the isTokenListSupportedForNetwork function to exclude the GANACHE_CHAIN_ID? Or did it cause the e2e test to fail as you mentioned? If that's the case, is there something we can do on the extension side to fix the test rather than making this code less cohesive?

[sahar-fehri]

Hello @mcmire , apologies was a bad idea to squash 🙏 ^^ But you are right, indeed the old change was to exclude GANACHE_CHAIN_ID from isTokenListSupportedForNetwork function. That caused the test test/e2e/tests/add-hide-token.spec.js to fail. I could see the reason why it was failing.

Because the isTokenListSupportedForNetwork was returning false after removing GANACHE_CHAIN_ID; the code went to this line

core/packages/assets-controllers/src/TokenListController.ts

Line 202 in 959b920

return;

instead of doing startPolling.

In our e2e tests we mock the following call https://github.com/MetaMask/metamask-extension/blob/5a968da2eeb6818247af982d58e00b14dc7fb72a/test/e2e/mock-e2e.js#L268 which would correspond to this part

core/packages/assets-controllers/src/TokenListController.ts

Line 287 in 959b920

fetchTokenListByChainId(

if we did startPolling and went to execute fetchTokenList.

So on the UI part, we endup with an empty state.tokenList, and the test fail because it was looking for the BAT token to click on it and import it.

By leaving the GANACHE_CHAIN_ID in the isTokenListSupportedForNetwork, it will trigger populating the tokenList;
but i agree it makes the code look a bit less cohesive when we add it in fetchTokenMetadata instead.

Looks like for the test to pass, the isTokenListSupportedForNetwork must return true for ganache ID. But at the same time, the ganache ID needs to be excluded for fetchTokenMetadata so the user is able to see the token imported...

[sahar-fehri]

I have made an update so the fct isTokenListSupportedForNetwork does not consider the GanacheID as supported; (since the call inside fetchTokenMetadata to tokenApi would fail because token api does not support it anyway)

but also so that does not stop the test from doing the startPolling..

[mcmire]

Hi @sahar-fehri. Sorry it's taken me a while to look at this.

I took some time to test the original issue and look at the code. I believe I found exactly what you found, but just for clarity, this seems to be the sequence of events:

• The user adds the token to their wallet through a dapp, which calls wallet_watchAsset on the provider and to MetaMask.
• This causes the addToken method on TokensController to get called.
• This method calls fetchTokenMetadata, also on TokensController.
• This method calls fetchTokenMetadata in assetsUtil.
• This function attempts to request information about the token from the Token API. However, because the Token API doesn't support chain 1337, the request fails and an error is thrown.
• fetchTokenMetadata may throw an error if isTokenListSupportedForNetwork returns false. If this were to happen, then the TokensController version of fetchTokenMetadata would catch this error and return undefined. addToken expects this to happen and continues on without issue.

As for the failing extension e2e test — "Add existing token using search renders the balance for the chosen token" — I looked at why this was failing too. This test seems to be trying to simulate production, because if you do remove GANACHE_CHAIN_ID from isTokenListSupportedForNetwork, then it removes the option to search for tokens from the UI. The way that the extension has gotten around this is with a check in import-tokens-modal.js, which turns it on in e2e tests.

I see why we make an exception for e2e tests here — we need some way of testing the search box — but I wonder if the way to do this is to use a custom chain ID for Ganache (not 1337, in other words) so that isTokenListSupportedForNetwork returns true. Perhaps we will need to extract this test to a different file so that this is easier, I'm not sure.

To recap, I think we just need to remove GANACHE_CHAIN_ID check from isTokenListSupportedForNetwork here. That path seems to make the most sense to me, but at the same time I think we should update the e2e test so that it better simulates production instead of localhost.

What do you think? Would that work?

[sahar-fehri]

Hi @sahar-fehri. Sorry it's taken me a while to look at this.

I took some time to test the original issue and look at the code. I believe I found exactly what you found, but just for clarity, this seems to be the sequence of events:

• The user adds the token to their wallet through a dapp, which calls wallet_watchAsset on the provider and to MetaMask.
• This causes the addToken method on TokensController to get called.
• This method calls fetchTokenMetadata, also on TokensController.
• This method calls fetchTokenMetadata in assetsUtil.
• This function attempts to request information about the token from the Token API. However, because the Token API doesn't support chain 1337, the request fails and an error is thrown.
• fetchTokenMetadata may throw an error if isTokenListSupportedForNetwork returns false. If this were to happen, then the TokensController version of fetchTokenMetadata would catch this error and return undefined. addToken expects this to happen and continues on without issue.

As for the failing extension e2e test — "Add existing token using search renders the balance for the chosen token" — I looked at why this was failing too. This test seems to be trying to simulate production, because if you do remove GANACHE_CHAIN_ID from isTokenListSupportedForNetwork, then it removes the option to search for tokens from the UI. The way that the extension has gotten around this is with a check in import-tokens-modal.js, which turns it on in e2e tests.

I see why we make an exception for e2e tests here — we need some way of testing the search box — but I wonder if the way to do this is to use a custom chain ID for Ganache (not 1337, in other words) so that isTokenListSupportedForNetwork returns true. Perhaps we will need to extract this test to a different file so that this is easier, I'm not sure.

To recap, I think we just need to remove GANACHE_CHAIN_ID check from isTokenListSupportedForNetwork here. That path seems to make the most sense to me, but at the same time I think we should update the e2e test so that it better simulates production instead of localhost.

What do you think? Would that work?

Thnx @mcmire 🙏 It makes sense to update the e2e test to use mainnet instead of ganache since tokenApi does not support it in the first place; pushed a small update to this and updated the e2e test on extension : MetaMask/metamask-extension#22880

[mcmire]

I hadn't realized you'd already made the change I requested. Sorry for the delay. This looks good to me!