As far as I can tell, this change is functionally equivalent, as the NETWORK_DID_CHANGE event is only emitted at the time that the providerStore is updated.

Nit: the code comment for this method could be slightly modified. Subscription to NETWORK_DID_CHANGE events or something like that.

reworded comment in the latest commit

Nit: I think the account variable might be more appropriately name address or accountAddress, which better conveys what that variable is.

done in the latest

Any reason to put this in its own if block instead of add it as a case to the above switch?

Oh, because you need the above for loop to complete before the below logic can properly migrate account tokens for custom rpcs that share a chain id with one of the default networks

Given this logic, it seems the following could happen:

• user currently has token ABC on mainnet
• user currently has token DEF on a custom rpc X, which has the same chainId as mainnet
• user currently has tokens GHI on a custom rpc Y, which has a different chainId than mainnet
• so currently, on mainnet, the user will only see ABC, but on both X and Y the user will see both DEF and GHI (because all tokens are stored by provider type, and X and Y have the same provider type NETWORK_TYPE_RPC)
• after this migration, the user will (as desired) continue to see ABC on mainnet and will see ABC on X (via ...newAccountTokens[account][detail.chainId]), and (as desired) will see DEF on mainnet and X, BUT will also see GHI on mainnet and X (via ...accountTokens[account][NETWORK_TYPE_RPC])

I think we should avoid this if possible, but I am not sure that it is possible...

or am I misunderstanding somehow?

If I am not misunderstanding, the only way to achieve a distinction of NETWORK_TYPE_RPC tokens by chainId would be to check on chain data via a network request. That is, we could check if the user has any of the token each of the networks with the chain id. Not sure if this is feasbile within our migration or desirable given network load.

Perhaps we need to add precision to what we are keying by here: key by a combination of provider type and chain id

key by a combination of provider type and chain id

This would result in the following:

• the migration would not change the current tokens of default networks
• all custom networks would continue to have all the tokens they already do
• custom networks and default networks that share chainIds would NOT have each others tokens as a result of the migration
• post migration, newly added tokens will be added network by network, and even if two networks share a chainId, adding a token to one will not add a token to another (unless we add further code that ensures that)

I might suggest a simpler solution that gets more to the heart of #8668 (treating custom networks with the chainId 0x1 as mainnet) - For this migration prefer the tokens listed under mainnet for '0x1'. So the following would happen:

1. For all custom networks the user has added that don't share a chainId with a default MetaMask chain, they will continue to have all tokens listed under 'rpc' provider type.
2. For all default networks the user tokens will be unchanged.
3. For any custom network that shares a chainId with a default network, they will inherit the default network's tokens and those will be shared across them.

If we want to preserve the ability to have different tokens on different networks (which is sort of going against the spirit of #8668 in my opinion) that share the same chainId, then I'm fine with @danjm 's suggestion.

@MetaMask/extension-devs anyone else have opinions?

If I am not misunderstanding, the only way to achieve a distinction of NETWORK_TYPE_RPC tokens by chainId would be to check on chain data via a network request. That is, we could check if the user has any of the token each of the networks with the chain id. Not sure if this is feasbile within our migration or desirable given network load.

i'd like to point out that we don't currently do this, any token a user adds for a custom network is added to all custom networks. I think it's out of scope to consider this for this migration and puts the extension in a better place to handle it in the future by isolating tokens added per chain.

@brad-decker The simpler solution you suggested sounds good. One remaining question about this case:

For any custom network that shares a chainId with a default network, they will inherit the default network's tokens and those will be shared across them.

Will such custom networks also continue to have all other tokens that are currently under the rpc provider type. Or are you suggesting that, after the migration, these custom networks will only have the tokens that are from the default network they share a chainId with?

Given the following current state:

const frequentRpcListDetail = [
	{
		chainId: '0x1',
        networkName: 'mainnet custom endpoint'
	},
    {
		chainId: '0xabc',
        networkName: 'non default network'
    }
];
const accounTokens = {
  '0x1111': {
    'mainnet': ['X', 'Y', 'Z'],
    'rpc': ['A', 'B', 'C', 'Z'],
   }
};

the resulting new structure of accountTokens would be:

accounTokens = {
  '0x1111': {
    '0x1': ['X', 'Y', 'Z'],
    '0xabc': ['A', 'B', 'C', 'Z'],
   }
};

so previously the user connected to their custom mainnet endpoint and added 'Z' token, which is also in the default mainnet. presumably for the purposes of tracking that mainnet token on their custom network. They also added 'A', 'B', 'C' tokens for the '0xabc' chain. After the migration the custom network and mainnet will have identical token lists and will remain in sync, including hiding tokens. The custom network with a non default network chain id will remain in the same state it was in prior to the migration with the added benefit that hiding tokens on that network will not hide the tokens elsewhere and adding new tokens to it will not add them elsewhere (for custom RPCs anyways)

This logic was implemented in the latest commit

This is subject to similar concerns that I raised above.