-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat(metrics): add eth-chainlist domain check to isPublicEndpointUrl cp-13.17.0 #39623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(metrics): add eth-chainlist domain check to isPublicEndpointUrl cp-13.17.0 #39623
Conversation
|
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. |
Builds ready [0f0a988]
UI Startup Metrics (1307 ± 109 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
0f0a988 to
2f1404a
Compare
Builds ready [2f1404a]
UI Startup Metrics (1319 ± 104 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
6e44c2c to
6dbed4f
Compare
6dbed4f to
f8c585b
Compare
…cp-13.17.0 (#39623) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** Add eth-chainlist domain checking to isPublicEndpointUrl to improve coverage for identifying public RPC endpoints in metrics. If a domain is defined in eth-chainlist (cached), it's considered public and safe to report. Initialization only happens in background context to avoid ~300KB memory footprint in UI. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/39623?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/WPC-354 ## **Manual testing steps** ```gherkin Feature: RPC domain analytics Scenario: Verify rpc_domain shows actual host when switching to public RPC via banner # Setup - Add Ink network with local RPC Given user navigates to Settings → Networks → Add Network And user adds Ink network (Chain ID: 57073) with local RPC endpoint: http://127.0.0.1:8545 And user also adds public RPC endpoint: https://rpc-qnd.inkonchain.com And user sets the local RPC as the default endpoint And user switches to Ink network # Trigger degraded state When user disconnects local RPC (or it becomes unavailable) And user waits for banner showing "Still connecting to Ink..." # Trigger RPC update from banner Then the "Update RPC" button appears on the banner When user clicks "Update RPC" on the banner And user is navigated to Edit Network screen And user switches default RPC to https://rpc-qnd.inkonchain.com # Verify analytics in Segment When user checks Segment dashboard for "Network Connection Banner RPC Updated" event Then the event property from_rpc_domain should be "custom" (local RPC is private) And the event property to_rpc_domain should be "rpc-qnd.inkonchain.com" (known public domain) Scenario: Verify rpc_domain for Infura networks using Switch to MetaMask default # Setup - Configure Arbitrum with local RPC Given user starts a local Ganache server: npx ganache --chain.chainId 42161 And user navigates to Settings → Networks → Arbitrum One And user adds a new RPC endpoint: http://127.0.0.1:8545 And user sets the local RPC as the default endpoint # Trigger degraded state When user stops the Ganache server (Ctrl+C) And user waits for banner showing "Still connecting to Arbitrum One..." # Switch to Infura via banner button Then the "Switch to MetaMask default RPC" button appears on the banner When user clicks "Switch to MetaMask default RPC" Then the toast "Updated to MetaMask default" appears # Verify analytics When user checks Segment for "Network Connection Banner Switch To MetaMask Default RPC Clicked" Then rpc_domain should be "custom" (the local RPC being switched from) ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes privacy/analytics sanitization for RPC URLs by expanding what’s considered “public” and moving the check behind a background RPC call; misclassification could leak domains to metrics or over-redact valid public endpoints. > > **Overview** > **Expands `isPublicEndpointUrl` to cover chainlist-known RPC domains.** The public-endpoint check is moved into `app/scripts/lib/util.ts`, now optionally allowing any hostname found in the cached safe chainlist while explicitly filtering out `localhost`, IP literals (v4/v6), and RFC 6761 special-use/reserved domains. > > **Routes public-endpoint checks through the background for UI metrics.** UI analytics in `useNetworkConnectionBanner` and the networks form now call `submitRequestToBackground('isPublicEndpointUrl', [url])` (with added error isolation), and the background controller exposes this method using the runtime Infura project id. Tests were updated accordingly, and `ip-regex` was added as a dependency. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit f5769e0. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Builds ready [f5769e0]
UI Startup Metrics (1312 ± 105 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…cp-13.17.0 (#39623) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** Add eth-chainlist domain checking to isPublicEndpointUrl to improve coverage for identifying public RPC endpoints in metrics. If a domain is defined in eth-chainlist (cached), it's considered public and safe to report. Initialization only happens in background context to avoid ~300KB memory footprint in UI. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/39623?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/WPC-354 ## **Manual testing steps** ```gherkin Feature: RPC domain analytics Scenario: Verify rpc_domain shows actual host when switching to public RPC via banner # Setup - Add Ink network with local RPC Given user navigates to Settings → Networks → Add Network And user adds Ink network (Chain ID: 57073) with local RPC endpoint: http://127.0.0.1:8545 And user also adds public RPC endpoint: https://rpc-qnd.inkonchain.com And user sets the local RPC as the default endpoint And user switches to Ink network # Trigger degraded state When user disconnects local RPC (or it becomes unavailable) And user waits for banner showing "Still connecting to Ink..." # Trigger RPC update from banner Then the "Update RPC" button appears on the banner When user clicks "Update RPC" on the banner And user is navigated to Edit Network screen And user switches default RPC to https://rpc-qnd.inkonchain.com # Verify analytics in Segment When user checks Segment dashboard for "Network Connection Banner RPC Updated" event Then the event property from_rpc_domain should be "custom" (local RPC is private) And the event property to_rpc_domain should be "rpc-qnd.inkonchain.com" (known public domain) Scenario: Verify rpc_domain for Infura networks using Switch to MetaMask default # Setup - Configure Arbitrum with local RPC Given user starts a local Ganache server: npx ganache --chain.chainId 42161 And user navigates to Settings → Networks → Arbitrum One And user adds a new RPC endpoint: http://127.0.0.1:8545 And user sets the local RPC as the default endpoint # Trigger degraded state When user stops the Ganache server (Ctrl+C) And user waits for banner showing "Still connecting to Arbitrum One..." # Switch to Infura via banner button Then the "Switch to MetaMask default RPC" button appears on the banner When user clicks "Switch to MetaMask default RPC" Then the toast "Updated to MetaMask default" appears # Verify analytics When user checks Segment for "Network Connection Banner Switch To MetaMask Default RPC Clicked" Then rpc_domain should be "custom" (the local RPC being switched from) ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes privacy/analytics sanitization for RPC URLs by expanding what’s considered “public” and moving the check behind a background RPC call; misclassification could leak domains to metrics or over-redact valid public endpoints. > > **Overview** > **Expands `isPublicEndpointUrl` to cover chainlist-known RPC domains.** The public-endpoint check is moved into `app/scripts/lib/util.ts`, now optionally allowing any hostname found in the cached safe chainlist while explicitly filtering out `localhost`, IP literals (v4/v6), and RFC 6761 special-use/reserved domains. > > **Routes public-endpoint checks through the background for UI metrics.** UI analytics in `useNetworkConnectionBanner` and the networks form now call `submitRequestToBackground('isPublicEndpointUrl', [url])` (with added error isolation), and the background controller exposes this method using the runtime Infura project id. Tests were updated accordingly, and `ip-regex` was added as a dependency. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit f5769e0. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Builds ready [7cb2c12]
UI Startup Metrics (1298 ± 111 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [ce53ef0]
UI Startup Metrics (1358 ± 152 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [14ec257]
UI Startup Metrics (1324 ± 136 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Add eth-chainlist domain checking to isPublicEndpointUrl to improve coverage for identifying public RPC endpoints in metrics. If a domain is defined in eth-chainlist (cached), it's considered public and safe to report.
Initialization only happens in background context to avoid ~300KB memory footprint in UI.
Changelog
CHANGELOG entry: null
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/WPC-354
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Medium risk because it changes privacy-sensitive URL sanitization used for metrics and introduces a new background↔UI call path; misclassification could leak or over-redact RPC domains in analytics.
Overview
Improves RPC-domain sanitization for metrics by moving
isPublicEndpointUrlintoapp/scripts/lib/util.tsand extending it to treat endpoints as public when their hostname appears in the cached safe chainlist, in addition to existing Infura/Quicknode/known-custom URL checks.To reduce privacy risk, chainlist-derived hostnames are filtered to exclude
localhost, any IPv4/IPv6 literal, and RFC 6761 special-use domains (newisSpecialUseDomain+ip-regexdependency). UI metrics paths (useNetworkConnectionBanner, networks form banner tracking) now callisPublicEndpointUrlviasubmitRequestToBackground('isPublicEndpointUrl')(with error isolation), and tests were updated/added accordingly while removing the oldshared/lib/network-utilsisPublicEndpointUrlcoverage.Written by Cursor Bugbot for commit 14ec257. This will update automatically on new commits. Configure here.