-
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
Give the user ability to opt out of get balance batch request for all loaded accounts #16746
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. |
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.
github diff checker is a bit confused on this file, I simply reordered the render functions on this file and added a new one
@@ -83,24 +85,26 @@ export default class SecurityTab extends PureComponent { | |||
); | |||
} | |||
|
|||
renderMetaMetricsOptIn() { | |||
renderIncomingTransactionsOptIn() { |
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.
github diff checker is a bit confused on this file, I simply reordered the render functions on this file and added a new one
f1e4b6f
to
342a8e5
Compare
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.
This looks good to me, tested locally and works well. Great work @pedronfigueiredo 🎉
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 👍
Marking it as a draft until tomorrow to get the final copy. |
@pedronfigueiredo by default we let contributors with merge access merge their own PRs, but sometimes they do get merged by others. Marking as draft is appropriate here due to incoming changes but you can also flag with DO-NOT-MERGE when necessary to avoid others merging fully approved PRs. Example: #12847 (at least it has been fully approved multiple times in the past :P ) |
e92ec03
342a8e5
to
e92ec03
Compare
e92ec03
to
10a0070
Compare
Builds ready [145bf9a]
Page Load Metrics (2427 ± 154 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
highlights:storybook
|
Builds ready [c4dd37e]
Page Load Metrics (1856 ± 91 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
highlights:storybook
|
Builds ready [59598bb]
Page Load Metrics (2583 ± 319 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
highlights:storybook
|
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.
🎉
Explanation
Adds ability for the user to opt-out of batching requests to get balances for all accounts in the wallet
Unit tests
Fixes [Bug]: Don't correlate accounts with RPC endpoint. #15169
Screenshots/Screencaps
Opting.out.of.batching.Network.Requests.for.user.privacy.1.mp4
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Board
label.In this case, a QA Engineer approval will be be required.