Skip to content

fix: network verification for collision network + local network#23802

Merged
salimtb merged 7 commits intodevelopfrom
fix-collision-network-id
Apr 25, 2024
Merged

fix: network verification for collision network + local network#23802
salimtb merged 7 commits intodevelopfrom
fix-collision-network-id

Conversation

@salimtb
Copy link
Copy Markdown
Contributor

@salimtb salimtb commented Mar 29, 2024 •
edited
Loading

Description

here are the different fixes that this PR contains:

collision of two networks with the same chainId but with two different symbols ==> solution: manage this as a special case, add the other network and extend the list https://chainid.network/chains.json

scam warning displayed for networks running on localhost ==> exclude from network verification all networks running on localhost.

Open in GitHub Codespaces

Related issues

Fixes: #23242

Manual testing steps

  1. Go to add network page
  2. Go to custom add network form
  3. Add the network with chainID 78 and rpc https://rpc.wethio.io/ and symbol ZYN

Screenshots/Recordings

Before

Screenshot 2024-03-29 at 10 49 00 Screenshot 2024-03-29 at 10 49 18

After

Screenshot 2024-03-29 at 10 35 41

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • 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 format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • 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.

@salimtb salimtb requested a review from a team as a code owner March 29, 2024 09:37
@salimtb salimtb marked this pull request as draft March 29, 2024 09:37
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 29, 2024

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.

@salimtb salimtb added team-assets needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Mar 29, 2024
@salimtb salimtb force-pushed the fix-collision-network-id branch from d5ffde2 to 3d00c7a Compare March 29, 2024 09:50
@salimtb salimtb requested review from bergeron and sahar-fehri March 29, 2024 09:50
@salimtb salimtb self-assigned this Mar 29, 2024
@salimtb salimtb marked this pull request as ready for review March 29, 2024 09:51
@salimtb salimtb force-pushed the fix-collision-network-id branch from 3d00c7a to bc02fe5 Compare March 29, 2024 09:51
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Mar 29, 2024

Builds ready [bc02fe5]
Page Load Metrics (578 ± 426 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint60309995325
domContentLoaded94316105
load482334578887426
domInteractive94316105
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 729 Bytes (0.01%)
  • common: 272 Bytes (0.01%)

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.93750% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 67.50%. Comparing base (cc928f6) to head (d9a968c).
Report is 24 commits behind head on develop.

❗ Current head d9a968c differs from pull request most recent head e0dc2a5. Consider uploading reports for the commit e0dc2a5 to get more accurate results

Files Patch % Lines
...c-method-middleware/handlers/add-ethereum-chain.js 0.00% 7 Missing ⚠️
...ttings/networks-tab/networks-form/networks-form.js 92.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #23802      +/-   ##
===========================================
+ Coverage    67.47%   67.50%   +0.03%     
===========================================
  Files         1257     1258       +1     
  Lines        49228    49261      +33     
  Branches     12819    12829      +10     
===========================================
+ Hits         33216    33251      +35     
+ Misses       16012    16010       -2

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

@wethioproject
Copy link
Copy Markdown

wethioproject commented Apr 1, 2024

any update ?

@salimtb
Copy link
Copy Markdown
Contributor Author

salimtb commented Apr 3, 2024

any update ?

@wethioproject the PR is under review process and should be merged soon

@wethioproject
Copy link
Copy Markdown

wethioproject commented Apr 3, 2024 via email

@Jamesxik6 Jamesxik6 mentioned this pull request Apr 3, 2024
Closed
9 tasks
@salimtb salimtb requested a review from darkwing April 5, 2024 13:10
DDDDDanica
DDDDDanica previously approved these changes Apr 8, 2024
View reviewed changes
@DDDDDanica
Copy link
Copy Markdown
Contributor

DDDDDanica commented Apr 8, 2024

LGTM !Thanks for the fix !

darkwing
darkwing suggested changes Apr 8, 2024
View reviewed changes
@salimtb salimtb force-pushed the fix-collision-network-id branch 5 times, most recently from cc4aec2 to 378a5ff Compare April 9, 2024 17:14
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Apr 9, 2024

Builds ready [378a5ff]
Page Load Metrics (264 ± 284 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint64268914321
domContentLoaded95215105
load512168264591284
domInteractive95215105
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 24 Bytes (0.00%)
  • ui: 1.32 KiB (0.02%)
  • common: 310 Bytes (0.01%)

@salimtb salimtb requested review from DDDDDanica and darkwing April 10, 2024 08:45
@salimtb salimtb force-pushed the fix-collision-network-id branch from 49503b2 to c1ea75f Compare April 18, 2024 21:08
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Apr 18, 2024

Builds ready [c1ea75f]
Page Load Metrics (1506 ± 691 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint643461297134
domContentLoaded106119136
load51314515061440691
domInteractive106119136
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 24 Bytes (0.00%)
  • ui: 1.35 KiB (0.02%)
  • common: 348 Bytes (0.01%)

@wethioproject
Copy link
Copy Markdown

wethioproject commented Apr 22, 2024

Any eta to resolve issue ?

@salimtb salimtb force-pushed the fix-collision-network-id branch from c1ea75f to 958643a Compare April 22, 2024 13:59
@sahar-fehri
Copy link
Copy Markdown
Contributor

sahar-fehri commented Apr 22, 2024

LGTM!

sahar-fehri
sahar-fehri previously approved these changes Apr 22, 2024
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Apr 22, 2024

Builds ready [958643a]
Page Load Metrics (620 ± 533 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint70295994923
domContentLoaded10471594
load5531996201110533
domInteractive10471594
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 24 Bytes (0.00%)
  • ui: 1.35 KiB (0.02%)
  • common: 348 Bytes (0.01%)

@darkwing
Copy link
Copy Markdown
Contributor

darkwing commented Apr 22, 2024

Could we write an E2E test here to automate this? One basic network add and one collision case? This would make me much more comfortable.

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Apr 24, 2024

Builds ready [d9a968c]
Page Load Metrics (704 ± 534 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint59205863215
domContentLoaded9241242
load5029337041112534
domInteractive9241242
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 24 Bytes (0.00%)
  • ui: 1.38 KiB (0.02%)
  • common: 348 Bytes (0.01%)

darkwing
darkwing reviewed Apr 24, 2024
View reviewed changes
@salimtb salimtb force-pushed the fix-collision-network-id branch from 8c9a58b to e0dc2a5 Compare April 24, 2024 17:02
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Apr 24, 2024

Builds ready [e0dc2a5]
Page Load Metrics (744 ± 566 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint62154872110
domContentLoaded8431694
load5029217441179566
domInteractive8431694
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 1.38 KiB (0.02%)
  • common: 348 Bytes (0.01%)

@salimtb salimtb merged commit 6c465bc into develop Apr 25, 2024
@salimtb salimtb deleted the fix-collision-network-id branch April 25, 2024 13:32
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
@gauthierpetetin gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sahar-fehri sahar-fehri sahar-fehri approved these changes

@DDDDDanica DDDDDanica Awaiting requested review from DDDDDanica

@bergeron bergeron Awaiting requested review from bergeron

+1 more reviewer

@darkwing darkwing darkwing approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@salimtb salimtb

Labels

needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.0.0 Issue or pull request that will be included in release 12.0.0 team-assets

Projects

PR review queue
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Potential scam alert warning for Wethio network

8 participants

@salimtb @metamaskbot @wethioproject @DDDDDanica @sahar-fehri @darkwing @bergeron @gauthierpetetin