-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: issue where setNetworkClientIdForDomain
was called without checking whether the origin was eligible for setting its own network
#26323
Conversation
…ng whether the origin was eligible for setting its own network
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. |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -100,6 +101,7 @@ export const NetworkListMenu = ({ onClose }) => { | |||
const selectedTabOrigin = useSelector(getOriginOfCurrentTab); | |||
const useRequestQueue = useSelector(getUseRequestQueue); | |||
const networkConfigurations = useSelector(getNetworkConfigurations); | |||
const domains = useSelector(getAllDomains); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how important this is, but somewhere else in code i saw setNetworkClientIdForDomain guarded by hasPermissions instead of checking if the origin has a networkClientId set. The controller itself checks hasPermissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should always be in sync and perhaps we should change the controller to look at domains state since the state of which permissions are the threshhold may change
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26323 +/- ##
===========================================
+ Coverage 70.04% 70.05% +0.01%
===========================================
Files 1411 1411
Lines 50031 50035 +4
Branches 13812 13815 +3
===========================================
+ Hits 35042 35048 +6
+ Misses 14989 14987 -2 ☔ View full report in Codecov by Sentry. |
Builds ready [5a94d97]
Page Load Metrics (291 ± 262 ms)
Bundle size diffs
|
Description
This PR fixes a issue where
setNetworkClientIdForDomain
was frequently called with origins that had no account permissions yet (which is the thresshold we currently set for giving site's their own network) resulting in a large number of silent errors in the background that are clogging up sentry.Related issues
Fixes: https://metamask.sentry.io/issues/5659582204/?environment=production&project=273505&qu%5B%E2%80%A6%5Derrer=issue-stream&sort=freq&statsPeriod=90d&stream_index=1
Manual testing steps
N/A
Screenshots/Recordings
Before
N/A
After
N/A
Pre-merge author checklist
Pre-merge reviewer checklist