fix: Key the address book by chain ID instead of network ID

[Gudahtt]

Development & PR Process

1. Follow MetaMask Mobile Coding Standards
2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
3. Add needs-dev-review label when work is completed
4. Add the appropiate QA label when dev review is completed
   • needs-qa: PR requires manual QA.
   • No QA/E2E only: PR does not require any manual QA effort. Prior to merging, ensure that you have successful end-to-end test runs in Bitrise.
   • Spot check on release build: PR does not require feature QA but needs non-automated verification. In the description section, provide test scenarios. Add screenshots, and or recordings of what was tested.
5. Add QA Passed label when QA has signed off (Only required if the PR was labeled with needs-qa)
6. Add your team's label, i.e. label starting with team- (or external-contributor label if your not a MetaMask employee)

Description

The AddressBookController stores addresses by chain ID, but we had been storing and retrieving them using the network ID instead in many places. It has been updated to consistently use the chain ID for all address book access.

Existing address book entries have been migrated to be grouped by chain ID. For all known networks, we attempt to map the network ID to a chain ID that exists as a locally configured or built-in network. In cases where multiple matches are found, the entries are duplicated on each matching chain. Address book entries that don't correspond with any local networks are discarded.

Screenshots / Recordings

Before

In this recording, I create a contact on ETC and show that it's also suggested on Ethereum Mainnet

https://recordit.co/KrZMjEYKb4

After

In this recording, I am running a test build on this branch with the same wallet state as for the "Before" recording, so the test ETC contact entry is still there. I show that it was duplicated onto both Ethereum Mainnet and ETC. Then I create a new contact entry and show that it's correctly saved just for the current chain.

https://recordit.co/AIzatKvKXr

Issue

This relates to https://github.com/MetaMask/mobile-planning/issues/1226

Checklist

• There is a related GitHub issue
• Tests are included if applicable
• Any added code is fully documented

[codecov-commenter]

Codecov Report

Patch coverage: 74.73% and project coverage change: +0.14% 🎉

Comparison is base (ad9793b) 34.43% compared to head (c055ef7) 34.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7197      +/-   ##
==========================================
+ Coverage   34.43%   34.58%   +0.14%     
==========================================
  Files        1016     1016              
  Lines       27089    27150      +61     
  Branches     2206     2206              
==========================================
+ Hits         9329     9389      +60     
- Misses      17267    17268       +1     
  Partials      493      493              

Files Changed	Coverage Δ
.../UI/ApproveTransactionReview/AddNickname/index.tsx	7.01% <ø> (ø)
app/components/Views/ApproveView/Approve/index.js	3.50% <ø> (ø)
app/components/Views/Send/index.js	3.72% <0.00%> (ø)
app/components/Views/SendFlow/SendTo/index.js	4.16% <0.00%> (ø)
...nents/Views/Settings/Contacts/ContactForm/index.js	6.59% <0.00%> (ø)
app/components/Views/Settings/Contacts/index.js	13.33% <0.00%> (ø)
app/reducers/user/index.js	30.00% <ø> (ø)
app/util/address/index.js	51.00% <0.00%> (ø)
app/util/transactions/index.js	69.04% <0.00%> (ø)
...ddToAddressBookWrapper/AddToAddressBookWrapper.tsx	73.91% <50.00%> (ø)
... and 4 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Gudahtt]

Migrating this data will be tricky. I wrote a script to process https://chainid.network/chains.json looking for cases where the network ID doesn't match the chain ID. Here is the script:

const { inspect } = require('util');
const chains = require('./chains.json');

const networkIdToChain = {};

for (const chain of chains) {
  if (chain.chainId === undefined) {
    throw new Error(`Missing chain ID for ${inspect(chain)}`)
  }
  if (chain.networkId === undefined) {
    throw new Error(`Missing network ID for ${inspect(chain)}`)
  }
  networkIdToChain[chain.networkId] = [
    ...(networkIdToChain[chain.networkId] || []),
    chain.chainId,
  ];
}

for (const [networkId, chainIds] of Object.entries(networkIdToChain)) {
  if (chainIds.length === 1 && String(networkId) === String(chainIds[0])) {
    continue;
  }

  console.log(`networkId: ${networkId}, chainIds: ${inspect(chainIds)}`)
}

And here are the results:

networkId: 0, chainIds: [ 24, 211, 980, 989 ]
networkId: 1, chainIds: [
        1,        2,      61,
      101,      138,     262,
      820,     1856,    1898,
    31102,    43113,   71393,
   103090,   201018,  201030,
   210425,   420666, 2203181,
  2206132, 20180430
]
networkId: 2, chainIds: [ 9, 62, 821 ]
networkId: 7, chainIds: [ 7, 63 ]
networkId: 10, chainIds: [ 10, 2415 ]
networkId: 21, chainIds: [ 21, 2138 ]
networkId: 79, chainIds: [ 79, 20729 ]
networkId: 1000, chainIds: [ 500, 1000 ]
networkId: 1001, chainIds: [ 501, 1001 ]
networkId: 1024, chainIds: [ 520, 1024 ]
networkId: 1230, chainIds: [ 1230, 12306 ]
networkId: 2048, chainIds: [ 1202, 2048 ]
networkId: 2221, chainIds: [ 222, 2221 ]
networkId: 3344, chainIds: [ 67588 ]
networkId: 37129, chainIds: [ 24484 ]
networkId: 37480, chainIds: [ 24734 ]
networkId: 48501, chainIds: [ 85449 ]
networkId: 103090, chainIds: [ 420420 ]
networkId: 11235813, chainIds: [ 1620 ]

[hesterbruikman]

This PR opts to show both addresses in case of chain conflicts. There is a known and current risk with this approach:

WHEN the user selects an address from the contact list in the send flow
AND that address exists on a duplicate chain
AND this is not the intended destination chain
THEN the user might send funds to an address on the wrong chain

This risk exists in production today. To unblock controller work, next steps agreed on:

1. Release with duplicate addresses
2. Add a warning icon to addresses that had chain conflicts
3. Add tooltip/new design pattern to verify the chain (I don't know if we have actions on addresses today, e.g. remove, but we need something like this)

[Gudahtt]

@SocketSecurity ignore-all

False positive, the new dependencies detected are not new

[Gudahtt]

I have updated the migration to store a record of all ambiguous addresses that have been duplicated across chains. I am awaiting further clarification on the next requirements.

[Gudahtt]

2. Add a warning icon to addresses that had chain conflicts
3. Add tooltip/new design pattern to verify the chain (I don't know if we have actions on addresses today, e.g. remove, but we need something like this)

These steps are being discussed, so I have marked this as DO-NOT-MERGE for now. But aside from that, the PR is ready for review

[Cal-L]

LGTM. LMK if you need another review once there's more movement on UI dependencies.

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[Gudahtt]

This has been rebased to resolve conflicts (the migration was moved from 22 to 23 because a migration 22 was added on main)

[Gudahtt]

The PR description has been updated with recordings

[Cal-L]

LGTM. Migration works when upgrading from old version to new. Existing contacts show up the same as they did before. Ambiguous addresses are populated in redux. Adding contacts post fix does not cross contaminate across chains. Nice.

[Cal-L]

Removing do not merge label for now since we will handle the UI improvement in a separate task. Follow thread here - https://consensys.slack.com/archives/CBW7S9FSN/p1694536315593899

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

74.5% Coverage
0.0% Duplication

[cortisiko]

I did a manual regression on this PR with an emphasis on the testing contract nicknames functionality: contract nicknames should appear in the address book but not in the send flow.

✅