[Bug]: Metamask switches chain instead of adding a new chain.

[starkbamse]

Describe the bug

If a user has chain A from provider B but wants to add chain A from provider C because they bought a premium RPC url, wallet_addEthereumChain RPC request results in MetaMask iOS to switch chain and not add the new chain.

Expected behavior

I expect that wallet_addEthereumChain is honored as a request to add a new chain even though another RPC with the same chain id is present.

Screenshots/Recordings

No response

Steps to reproduce

Example for binance smart chain:

• Add default RPC for binance smart chain
• Go to 1rpc.io and add their BSC url.
• Metamask will say that the dapp requests to switch chain whilst actually it is requesting to add a new chain.

Error messages or log output

No response

Version

7.20.0 (1308)

Build type

None

Device

iPhone 12 Pro

Operating system

iOS

Additional context

No response

Severity

Problematic for RPC providers wanting to offer their customers a quick way to add their RPC url's.

[vandan]

Confirmed that there is an issue in the logic here: https://github.com/MetaMask/metamask-mobile/blob/cd353bb94584b5995fc93e7ac0394cd1916f7292/app/core/RPCMethods/wallet_addEthereumChain.js#L120-L122 Should behave similar to the logic in the Extension.

[vandan]

Re-assigning to the mobile platform team

[adonesky1]

Existing entry here should match on rpcUrl not chainId

[starkbamse]

Re-assigning to the mobile platform team

Thanks, is there an ETA for issues like this? Could I submit a PR with a fix myself if team is too busy?

[starkbamse]

Any update @vandan @adonesky1

[starkbamse]

@MarioAslau Hello, I saw you approved my PR: https://github.com/MetaMask/metamask-mobile/pull/9541

Glad to see this being looked at! For some reason the CI still fails even though I did sign the "CLA". I cannot seem to try again because the GitHub bot locked the conversation.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

[starkbamse]

Guys can we please get a resolution on this @MarioAslau @vandan @adonesky1 @gauthierpetetin

[gauthierpetetin]

Hi @starkbamse , we plan to work on it starting next week. We'll let you know once we have more visibility with regards to resolution.

[starkbamse]

Hi @starkbamse , we plan to work on it starting next week. We'll let you know once we have more visibility with regards to resolution.

Hello, thanks for the response! Just to mention, I included a PR for this here: #9541 should fix the issue.

Essentially what is needed is an update to the logic of checking for an existing entry. Instead of checking for an existing entry via chain id we should factor in the RPC url. That is, if the RPC url differs, that should result in existingEntry equating to false.

[gauthierpetetin]

Thanks lot for your contribution, we'll review that. cc @kylanhurt for visibility

[kylanhurt]

@starkbamse How's it going? I've been assigned to this task and I've noticed some minor issues with the "Add Custom Network" process, but let's start off with the bug you submitted. When I use a fresh install of MetaMask and follow your steps to reproduce, it allows me to add the second chain.

https://github.com/user-attachments/assets/36b8f152-fd00-465a-955f-e901998aa111

I will admit that the bottom modal is a bit confusing because while a BNB chain already exists, it isn't asking us to cancel adding the second chain config but rather asking if we want to switch to the second chain now that it has been added. The Close button does not cancel the addition of the second chain, just adds it without switching to that network.

Now, an area where it DOES have a bug is that once the chains show up in the dropdown it displays which is "Active" via chainId so that is an area that we could fix but that is different from your bug report.

Can you still reproduce the bug you see? Would be great if you could post a screen capture video so that we can be sure we are trying to fix the right issue.

[starkbamse]

Hello @kylanhurt. Reviewing the footage that you have provided, there seems to be some misunderstanding. In the footage that you have provided you are attempting to add the RPC url manually, what this issue regards is how MetaMask mobile responds to the wallet_addEthereumChain RPC call.

To reproduce the issue you must first:

1. Add the standard BSC network to your wallet.
2. Switch back to the ethereum mainnet. (We do this to trigger the popup from MetaMask, why there is no popup when pressing the wallet icon is probably another issue. MetaMask does not respect wallet_addEthereumNetwork RPC calls when user is on the same network which doesn't adhere to EIP. Nevertheless, one thing at a time.)
3. Visit https://1rpc.io
4. Search for BSC chain and press the little wallet icon: On desktop this triggers a wallet_addEthereumChain RPC call which is respected by MetaMask. However in MetaMask mobile this is not treated as a separate chain because in app/core/RPCMethods/wallet_addEthereumChain.js:

 const existingEntry = Object.entries(networkConfigurations).find(
    ([, networkConfiguration]) => networkConfiguration.chainId === _chainId,
  );

Since we already added the default BSC chain before existingEntry resolves to false, whilst it should resolve to true because we are attempting to add 1RPC's BSC chain configuration which is not the same as default BSC RPC configuration. In essence, MetaMask should always prompt the user if they want to add a new chain if the request wallet_addEthereumChain is received. For switching chain we have the wallet_switchEthereumChain. Luckily, the problem is fixable with minimal effort:

Should be something like:

const existingEntry = Object.entries(networkConfigurations).find(
([, networkConfiguration]) => {
    let isArray=Array.isArray(networkConfiguration.rpcUrls);
    if(!isArray) return false;
    return networkConfiguration.rpcUrls.includes(firstValidRPCUrl),
  }
);

This actually differs from my PR #9541 because I realized that if the isArray check is done simultaneously as the includes check and if the rpcUrls is not actually an array it will throw an error.

To conclude the logic for MetaMask mobile to check whether or not this is an existing entry should not be based on chain id but rather the RPC url contained within that id.

Below you can find footage of my replication of the bug that is indeed still present. Motivation?. Different RPC providers differ in their rate limitations/privacy policies. Users should have full convenience when it comes to changing/adding RPC urls with the appropriate warnings of course.

https://github.com/user-attachments/assets/67a58f91-116a-4fab-a9de-fdd304815f8b

[starkbamse]

@kylanhurt I have updated my PR #9541 with https://github.com/MetaMask/metamask-mobile/pull/9541/commits/5d9768b56da048fcde4566c951e78b478f0081bc that validates the array prior to calling the includes() method. For some reason, the PR is locked to collaborators only which is why the CLA bot fails.

[kylanhurt]

@starkbamse Ok that makes a lot more sense. Sorry for the confusion! I'll let you know if we need any more info from you.

[kylanhurt]

@starkbamse Could you do me a favor and pull the latest from our repo's main branch into your branch? Also, do me a favor and make sure that when two chains with the same ID exist, that the UI properly conveys which chain that user is currently on (ie consider using rpcUrl in lieu of chainId).

I am specifically referring to the following file: app/components/Views/NetworkSelector/NetworkSelector.tsx where you see the <Cell /> property isSelected

Edit: also, if you're too busy then if you could give me write access on your repo / branch then I could submit the changes a bit faster. Your choice.

[starkbamse]

@starkbamse Could you do me a favor and pull the latest from our repo's main branch into your branch? Also, do me a favor and make sure that when two chains with the same ID exist, that the UI properly conveys which chain that user is currently on (ie consider using rpcUrl in lieu of chainId).

I am specifically referring to the following file: app/components/Views/NetworkSelector/NetworkSelector.tsx where you see the <Cell /> property isSelected

Edit: also, if you're too busy then if you could give me write access on your repo / branch then I could submit the changes a bit faster. Your choice.

Atm the UI conveys which chain user is on via the name of the chain that user specifies. Example: 1RPC BNB Chain.

I'll give you write access, one moment.

[starkbamse]

@kylanhurt My branch is up-to-date with main.

[starkbamse]

@kylanhurt I've sent you an invite.

[starkbamse]

@kylanhurt I believe the relevant parts were: renderRpcNetworks, which are the custom RPC networks within the app. Edit: pushed my changes, apologize for the multiple messages :)

[kylanhurt]

@starkbamse After talking with our team, it looks like major changes are being made to the way that MetaMask handles chain configurations. Our Assets team, specifically, is responsible for the Network Controller and they are targeting to have this functionality in the next version of the Network Controller. Since their team has taken the time to fully think out the repercussions of this improved functionality, I think it's best that we stay out of their way and let them implement it how they see fit.

We appreciate your contribution to our project and encourage you to submit PRs for any other bugs you see within MetaMask. Thank you.