Skip to content

fix: rpcIdentifierUtility client side grouping before emitting CustomRPC event#26266

Merged
gambinish merged 22 commits intodevelopfrom
fix/mmassets-298_capture-custom-rpc-url
Aug 16, 2024
Merged

fix: rpcIdentifierUtility client side grouping before emitting CustomRPC event#26266
gambinish merged 22 commits intodevelopfrom
fix/mmassets-298_capture-custom-rpc-url

Conversation

@gambinish
Copy link
Copy Markdown
Member

@gambinish gambinish commented Jul 31, 2024 •
edited by github-actions bot
Loading

Description

Rather than arbitrarily sending sha256 hashes over the wire, we should instead handle the lookup/grouping of custom rpcs client-side, and then send over the meaningful resource identifiers of commonly used.

Benefits are:

  1. No post processing necessary
  2. Bypasses the need to hash at all, since rpcIdentifierUtility should only return the rpc host when there's a match (not for private eth nodes, for instance)

Note:

  1. useSafeChains hook is inspired by to be merged implementation here

Open in GitHub Codespaces

Related issues

Fixes: MMASSETS-298

Manual testing steps

  1. Go to settings and add a Custom network from the custom network form
  2. Event will be emitted with sensitiveProperties once network gets added

Screenshots/Recordings

N/A UX behavior should be the same

Pre-merge author checklist

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 31, 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.

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.12%. Comparing base (2d63c0e) to head (b19fa22).

Files Patch % Lines
...ings/networks-tab/networks-form/use-safe-chains.ts 89.47% 2 Missing ⚠️
...ttings/networks-tab/networks-form/networks-form.js 85.71% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##           develop   #26266   +/-   ##
========================================
  Coverage    70.12%   70.12%           
========================================
  Files         1428     1429    +1     
  Lines        50089    50102   +13     
  Branches     13894    13895    +1     
========================================
+ Hits         35120    35130   +10     
- Misses       14969    14972    +3

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

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jul 31, 2024

Builds ready [47d0752]
Page Load Metrics (459 ± 358 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint703531488842
domContentLoaded9156414321
load422445459745358
domInteractive9156414321
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 663 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@gambinish gambinish marked this pull request as ready for review July 31, 2024 21:45
@gambinish gambinish requested a review from a team as a code owner July 31, 2024 21:45
@gambinish gambinish requested review from bergeron and legobeat July 31, 2024 21:48
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 1, 2024

Builds ready [16fc788]
Page Load Metrics (66 ± 19 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint61206973818
domContentLoaded8128273416
load39184664019
domInteractive8128273416
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 653 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@legobeat legobeat force-pushed the fix/mmassets-298_capture-custom-rpc-url branch from 16fc788 to 1783cff Compare August 11, 2024 22:15
legobeat
legobeat previously approved these changes Aug 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@legobeat legobeat requested a review from a team August 11, 2024 22:16
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 14, 2024

Builds ready [816a642]
Page Load Metrics (158 ± 179 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint72133102199
domContentLoaded116123115
load481784158374179
domInteractive116123115
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 653 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

bergeron
bergeron reviewed Aug 14, 2024
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 14, 2024

Builds ready [6ab96a4]
Page Load Metrics (224 ± 238 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint653691026330
domContentLoaded168527168
load461904224495238
domInteractive168527178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 623 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@gambinish gambinish requested review from bergeron and legobeat August 14, 2024 22:26
@gambinish
Copy link
Copy Markdown
Member Author

gambinish commented Aug 14, 2024

Re-requesting after merging develop branch. Needs to be re-approved 🙏

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 15, 2024

Builds ready [c601154]
Page Load Metrics (440 ± 339 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint872941274521
domContentLoaded126733178
load542008440707339
domInteractive126733178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 623 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@legobeat legobeat requested a review from a team August 15, 2024 11:13
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 15, 2024

Builds ready [f2604af]
Page Load Metrics (1254 ± 541 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint793261647536
domContentLoaded1180342010
load52297912541127541
domInteractive1179342010
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 623 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Aug 16, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 16, 2024

Builds ready [b19fa22]
Page Load Metrics (357 ± 300 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint932861283919
domContentLoaded167135147
load741990357624300
domInteractive167135147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 623 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@gambinish gambinish merged commit 7198656 into develop Aug 16, 2024
@gambinish gambinish deleted the fix/mmassets-298_capture-custom-rpc-url branch August 16, 2024 15:50
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 16, 2024
@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@salimtb salimtb salimtb approved these changes

@bergeron bergeron Awaiting requested review from bergeron

+1 more reviewer

@legobeat legobeat legobeat approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-12.4.0 Issue or pull request that will be included in release 12.4.0 team-assets

Projects

PR review queue
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gambinish @metamaskbot @bergeron @salimtb @legobeat @gauthierpetetin