Security and Privacy Settings Re-org #16756

segun · 2022-12-01T10:25:46Z

Explanation

Reorganize settings so that all privacy related settings are easily discoverable, understood and toggleable.

Some settings (e.g Enhanced Token Detection) are in advanced tab, they should be moved to Security and Privacy Tab
There should also be a toggle to switch off/on all security and privacy toggles.

Fixes Reorganize settings so that all privacy related settings are easily discoverable, understood and toggleable #16727

Screenshots/Screencaps

Before

After

Manual Testing Steps

Launch MetaMask
Open Settings
Make sure all security and privacy settings (see Reorganize settings so that all privacy related settings are easily discoverable, understood and toggleable #16727) are under the Security and Privacy Tab
Make sure there's a checkbox on the tab to switch off/on all checkboxes in the tab

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

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.

github-actions · 2022-12-01T10:26:01Z

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.

metamaskbot · 2022-12-01T10:52:54Z

Builds ready [66c2577]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2170 ± 94 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	232	129	29	14
		domContentLoaded	1670	2538	2139	191	92
		load	1670	2538	2170	195	94
		domInteractive	1670	2538	2139	191	92

Bundle size diffs

background: 0 bytes
ui: 9 bytes
common: 0 bytes

highlights:

storybook

segun · 2022-12-01T12:12:10Z

ui/pages/settings/advanced-tab/advanced-tab.component.js

@@ -91,37 +89,6 @@ export default class AdvancedTab extends PureComponent {
    handleSettingsRefs(t, t('advanced'), this.settingsRefs);
  }

-  renderMobileSync() {


The changes in this file are huge because I have to re-arrange the renderXXX methods such that if follows a logical order of this.settingsRefs[XXX], so this.settingsRefs[0] comes before this.settingsRefs[14] for instance.

This will make adding/removing new renderXXX method in the future easier.

segun · 2022-12-01T12:13:59Z

ui/pages/settings/advanced-tab/advanced-tab.component.js

          </div>
        </div>
      </div>
    );
  }

-  renderTokenDetectionToggle() {


The only change logic-wise in this file, is that this method (and attendant variables) was removed and moved to security tab component.

segun · 2022-12-01T12:15:42Z

ui/pages/settings/security-tab/security-tab.component.js

@@ -83,6 +99,61 @@ export default class SecurityTab extends PureComponent {
    );
  }

+  renderIncomingTransactionsOptIn() {


I had to make the same arrangement for renderXXX methods here like I did in advanced tab components.

metamaskbot · 2022-12-01T12:57:50Z

Builds ready [bd0900b]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2164 ± 115 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	153	118	18	9
		domContentLoaded	1622	2665	2139	243	117
		load	1622	2665	2164	240	115
		domInteractive	1622	2665	2139	243	117

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 193 bytes
ui: 1425 bytes
common: 359 bytes

highlights:

storybook

kevinghim · 2022-12-06T18:20:57Z

@segun please remove the Toggle All section

metamaskbot · 2022-12-08T17:21:07Z

Builds ready [b4e5797]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2528 ± 268 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	101	1284	300	402	193
		domContentLoaded	1887	3844	2508	560	269
		load	1887	3844	2528	559	268
		domInteractive	1887	3844	2508	560	269

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 223 bytes
ui: 6342 bytes
common: 773 bytes

highlights:

storybook

metamaskbot · 2022-12-13T18:34:35Z

Builds ready [abb3b72]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1928 ± 56 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	97	179	118	21	10
		domContentLoaded	1556	2117	1921	121	58
		load	1556	2117	1928	117	56
		domInteractive	1556	2116	1921	121	58

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 223 bytes
ui: 7552 bytes
common: 773 bytes

highlights:

storybook

danjm · 2022-12-14T12:17:38Z

app/scripts/controllers/preferences.js

@@ -35,6 +35,7 @@ export default class PreferencesController {
      useNonceField: false,
      usePhishDetect: true,
      dismissSeedBackUpReminder: false,
+      showGasFeeEstimationBuySwapTokens: true,


We can remove the toggle that controls this boolean from this PR

danjm · 2022-12-14T12:18:13Z

app/scripts/metamask-controller.js

@@ -1637,6 +1637,10 @@ export default class MetamaskController extends EventEmitter {
      setUseTokenDetection: preferencesController.setUseTokenDetection.bind(
        preferencesController,
      ),
+      setShowGasFeeEstimationBuySwapTokens:


We can remove the toggle that this function controls from this PR

danjm · 2022-12-14T12:19:37Z

ui/pages/settings/security-tab/security-tab.component.js

@@ -167,7 +230,218 @@ export default class SecurityTab extends PureComponent {
    );
  }

-  renderMultiAccountBalanceCheckerOptIn() {
+  renderShowGasFeeEstimationBuySwapTokensToggle() {


Let's remove this from this PR. We will add this in a future PR, when we are ready to deal with those features being toggled off.

danjm

Code looks good. Only change needed is to remove the toggle and associated code, which I commented on

Signed-off-by: Akintayo A. Olusegun <[email protected]> Toggle all security & privacy settings by a single checkbox Signed-off-by: Akintayo A. Olusegun <[email protected]> i18n Signed-off-by: Akintayo A. Olusegun <[email protected]> Global Settings toggle tests Signed-off-by: Akintayo A. Olusegun <[email protected]> Add new privacy settings show balance and price checker Signed-off-by: Akintayo A. Olusegun <[email protected]> New settings renderShowGasFeeEstimationBuySwapTokensToggle Signed-off-by: Akintayo A. Olusegun <[email protected]> Some css changes Signed-off-by: Akintayo A. Olusegun <[email protected]> Add other privacy links. Signed-off-by: Akintayo A. Olusegun <[email protected]> more i18n Signed-off-by: Akintayo A. Olusegun <[email protected]> Add custom network Signed-off-by: Akintayo A. Olusegun <[email protected]> Add custom IPFS gateway Signed-off-by: Akintayo A. Olusegun <[email protected]> Autodetect tokens Signed-off-by: Akintayo A. Olusegun <[email protected]> Batch account balance requests Signed-off-by: Akintayo A. Olusegun <[email protected]> Renamed batch account balance checker to useMultiAccountBalanceChecker Signed-off-by: Akintayo A. Olusegun <[email protected]> Tests Signed-off-by: Akintayo A. Olusegun <[email protected]> Fixtures buikder change Signed-off-by: Akintayo A. Olusegun <[email protected]> Fix rebase duplicate methods. Signed-off-by: Akintayo A. Olusegun <[email protected]> Added the learn more link Signed-off-by: Akintayo A. Olusegun <[email protected]> Remove Toggle All Signed-off-by: Akintayo A. Olusegun <[email protected]> Remove preferences we don't already have Signed-off-by: Akintayo A. Olusegun <[email protected]> Auto Detect NFTs Signed-off-by: Akintayo A. Olusegun <[email protected]> Make tests pass Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fixes Signed-off-by: Akintayo A. Olusegun <[email protected]> Fix locales. Signed-off-by: Akintayo A. Olusegun <[email protected]> Move settings to security section Signed-off-by: Akintayo A. Olusegun <[email protected]> Git ignore Signed-off-by: Akintayo A. Olusegun <[email protected]>

Signed-off-by: Akintayo A. Olusegun <[email protected]>

metamaskbot · 2022-12-15T00:08:45Z

Builds ready [1515943]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2149 ± 105 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	105	181	147	22	11
		domContentLoaded	1714	2591	2145	218	105
		load	1715	2591	2149	218	105
		domInteractive	1714	2591	2145	218	105

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 bytes
ui: 5853 bytes
common: 530 bytes

highlights:

storybook

danjm

LGTM

darkwing · 2022-12-15T20:38:25Z

ui/pages/settings/security-tab/security-tab.component.js

+          {this.renderAutoDectectTokensToggle()}
+          {this.renderBatchAccountBalanceRequestsToggle()}
+          {this.renderCollectibleDetectionToggle()}
+        </div>
        {this.renderMetaMetricsOptIn()}


I believe we're missing the title Usage above this line.

@darkwing if you mean the <span className="settings-page__security-tab-sub-header">, it was decided that we should leave this like that.

segun requested a review from a team as a code owner December 1, 2022 10:25

segun requested a review from pedronfigueiredo December 1, 2022 10:25

segun marked this pull request as draft December 1, 2022 10:26

segun mentioned this pull request Dec 1, 2022

Reorganize settings so that all privacy related settings are easily discoverable, understood and toggleable #16727

Closed

segun commented Dec 1, 2022

View reviewed changes

segun self-assigned this Dec 1, 2022

segun marked this pull request as ready for review December 1, 2022 12:24

segun added area-privacy team-extension-client labels Dec 1, 2022

hilvmason removed the team-extension-client label Dec 2, 2022

segun force-pushed the dev-segun-security-privacy-settings branch 2 times, most recently from dc7e3c7 to ac8b530 Compare December 4, 2022 22:07

segun mentioned this pull request Dec 4, 2022

Toggle option to enable/disable balance and Token rate checking for using third-party API #16772

Merged

8 tasks

kevinghim mentioned this pull request Dec 6, 2022

Add privacy notice to "Show Incoming Transactions" setting #8806

Closed

segun force-pushed the dev-segun-security-privacy-settings branch 2 times, most recently from 7d17714 to b4e5797 Compare December 8, 2022 16:03

PeterYinusa added this to the v10.24.0 milestone Dec 8, 2022

segun force-pushed the dev-segun-security-privacy-settings branch 2 times, most recently from e2a2875 to f8c7afd Compare December 12, 2022 17:35

danjm reviewed Dec 14, 2022

View reviewed changes

danjm requested changes Dec 14, 2022

View reviewed changes

segun added 4 commits December 14, 2022 23:20

Fix linter error

5db0273

Signed-off-by: Akintayo A. Olusegun <[email protected]>

fix tests

5c774e8

Signed-off-by: Akintayo A. Olusegun <[email protected]>

Remove showGasFeeEstimationBuySwapTokens toggle

085ac44

Signed-off-by: Akintayo A. Olusegun <[email protected]>

segun force-pushed the dev-segun-security-privacy-settings branch from abb3b72 to 085ac44 Compare December 14, 2022 23:20

Remove un-used messages.

1515943

Signed-off-by: Akintayo A. Olusegun <[email protected]>

danjm approved these changes Dec 15, 2022

View reviewed changes

darkwing requested changes Dec 15, 2022

View reviewed changes

darkwing approved these changes Dec 19, 2022

View reviewed changes

segun merged commit 13de51e into develop Dec 19, 2022

segun deleted the dev-segun-security-privacy-settings branch December 19, 2022 17:46

github-actions bot locked and limited conversation to collaborators Dec 19, 2022

PeterYinusa added the rc-cherry-picked label Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security and Privacy Settings Re-org #16756

Security and Privacy Settings Re-org #16756

segun commented Dec 1, 2022 •

edited

Loading

github-actions bot commented Dec 1, 2022

metamaskbot commented Dec 1, 2022

segun Dec 1, 2022 •

edited

Loading

segun Dec 1, 2022

segun Dec 1, 2022

metamaskbot commented Dec 1, 2022

kevinghim commented Dec 6, 2022

metamaskbot commented Dec 8, 2022

metamaskbot commented Dec 13, 2022

danjm Dec 14, 2022

danjm Dec 14, 2022

danjm Dec 14, 2022

danjm left a comment

metamaskbot commented Dec 15, 2022

danjm left a comment

darkwing Dec 15, 2022

segun Dec 16, 2022

Security and Privacy Settings Re-org #16756

Security and Privacy Settings Re-org #16756

Conversation

segun commented Dec 1, 2022 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Dec 1, 2022

metamaskbot commented Dec 1, 2022

highlights:

segun Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

segun Dec 1, 2022

Choose a reason for hiding this comment

segun Dec 1, 2022

Choose a reason for hiding this comment

metamaskbot commented Dec 1, 2022

highlights:

kevinghim commented Dec 6, 2022

metamaskbot commented Dec 8, 2022

highlights:

metamaskbot commented Dec 13, 2022

highlights:

danjm Dec 14, 2022

Choose a reason for hiding this comment

danjm Dec 14, 2022

Choose a reason for hiding this comment

danjm Dec 14, 2022

Choose a reason for hiding this comment

danjm left a comment

Choose a reason for hiding this comment

metamaskbot commented Dec 15, 2022

highlights:

danjm left a comment

Choose a reason for hiding this comment

darkwing Dec 15, 2022

Choose a reason for hiding this comment

segun Dec 16, 2022

Choose a reason for hiding this comment

segun commented Dec 1, 2022 •

edited

Loading

segun Dec 1, 2022 •

edited

Loading