chore: Disable account syncing in prod by danjm · Pull Request #27943 · MetaMask/metamask-extension

danjm · 2024-10-17T19:12:56Z

Description

This PR disables account syncing in prod until we have e2e tests and documentation of proper testing

Related issues

Fixes:

Manual testing steps

yarn build
Account syncing should not occur

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

github-actions · 2024-10-17T19:13:10Z

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.

Prithpal-Sooriya

LGTM. The isProduction check is nice!

Gudahtt · 2024-10-17T19:23:50Z

app/scripts/metamask-controller.js

      },
      env: {
-        isAccountSyncingEnabled: isManifestV3,
+        isAccountSyncingEnabled: !isProduction() && isManifestV3,,


Hmm. This isProduction function does not effectively check that the current build is a production build.

It only checks that this is not "development" or "testing":

metamask-extension/shared/modules/environment.ts

Line 5 in 9716e94

process.env.METAMASK_ENVIRONMENT !== ENVIRONMENT.DEVELOPMENT &&

But there are many other environments:

metamask-extension/development/build/constants.js

Line 18 in 9716e94

const ENVIRONMENT = {

This will be effective in disabling the check in production. But it will also disable it in any non-test builds produced on CircleCI, including release candidate builds, which would be confusing during release QA.

Thoughts on setting this to false instead?

But it will also disable it in any non-test builds produced on CircleCI, including release candidate builds, which would be confusing during release QA.

Isn't that what we want, so that when v12.5.0 is tested, the feature is disabled?

Fair enough, yes we would want those to be as production-like as possible. But our QA team uses a lot of different build types interchangeably, including development builds, so it would still be confusing.

What was the reason for leaving it enabled for development and e2e testing?

What was the reason for leaving it enabled for development and e2e testing?

I was just thinking not to inhibit the ongoing QA work. It can be more efficient to work with development builds while writing tests or doing exploratory testing.

app/scripts/metamask-controller.js

Gudahtt · 2024-10-18T20:23:08Z

app/scripts/metamask-controller.js

Needs to be re-ordered, causing a lint error

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

metamaskbot · 2024-10-18T22:33:28Z

Builds ready [dbd02ca]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1864 ± 111 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	244	2563	1731	503	241
		domContentLoaded	1667	2550	1838	226	108
		load	1685	2565	1864	230	111
		domInteractive	19	194	59	44	21
		backgroundConnect	9	69	27	18	9
		firstReactRender	48	202	102	40	19
		getState	4	67	12	16	7
		initialActions	0	1	0	0	0
		loadScripts	1142	2056	1374	221	106
		setupStore	11	103	44	31	15
		uiStartup	1835	2748	2090	252	121

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

background: 155 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

Cherry-picks #27943 ## **Description** This PR disables account syncing in prod until we have e2e tests and documentation of proper testing. There were no merge conflicts when picking these commits. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27982?quickstart=1) ## **Related issues** Fixes: #27943 ## **Manual testing steps** 1. yarn build 2. Account syncing should not occur ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **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/develop/.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/develop/.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. Co-authored-by: Dan J Miller <danjm.com@gmail.com> Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Disable account syncing in prod

b2e78ad

danjm requested a review from a team as a code owner October 17, 2024 19:12

Prithpal-Sooriya previously approved these changes Oct 17, 2024

View reviewed changes

danjm added team-extension-platform Extension Platform team team-notifications-deprecated DEPRECATED: please use "team-assets" instead labels Oct 17, 2024

danjm changed the title ~~Disable account syncing in prod~~ chore: Disable account syncing in prod Oct 17, 2024

Gudahtt reviewed Oct 17, 2024

View reviewed changes

Gudahtt reviewed Oct 18, 2024

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

danjm dismissed Prithpal-Sooriya’s stale review via 5af0105 October 18, 2024 19:49

Gudahtt reviewed Oct 18, 2024

View reviewed changes

app/scripts/metamask-controller.js Outdated

Copy link
Copy Markdown

Member

Gudahtt Oct 18, 2024

Choose a reason for hiding this comment

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

Needs to be re-ordered, causing a lint error

Update app/scripts/metamask-controller.js

dbd02ca

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

danjm force-pushed the disable-account-syncing-prod branch from 5af0105 to dbd02ca Compare October 18, 2024 22:01

darkwing approved these changes Oct 19, 2024

View reviewed changes

sleepytanya approved these changes Oct 19, 2024

View reviewed changes

danjm added this pull request to the merge queue Oct 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 19, 2024

danjm added this pull request to the merge queue Oct 19, 2024

Merged via the queue into develop with commit 6794a10 Oct 19, 2024

danjm deleted the disable-account-syncing-prod branch October 19, 2024 05:28

github-actions bot locked and limited conversation to collaborators Oct 19, 2024

metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Oct 19, 2024

gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.8.0 Issue or pull request that will be included in release 12.8.0 labels Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Disable account syncing in prod#27943

chore: Disable account syncing in prod#27943
danjm merged 2 commits intodevelopfrom
disable-account-syncing-prod

danjm commented Oct 17, 2024 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Oct 17, 2024

Uh oh!

Prithpal-Sooriya left a comment

Uh oh!

Gudahtt Oct 17, 2024

Uh oh!

Gudahtt Oct 17, 2024

Uh oh!

Gudahtt Oct 17, 2024

Uh oh!

danjm Oct 17, 2024

Uh oh!

Gudahtt Oct 18, 2024

Uh oh!

danjm Oct 18, 2024

Uh oh!

Uh oh!

Gudahtt Oct 18, 2024

Uh oh!

metamaskbot commented Oct 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

danjm commented Oct 17, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Oct 17, 2024

Uh oh!

Prithpal-Sooriya left a comment

Choose a reason for hiding this comment

Uh oh!

Gudahtt Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Gudahtt Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Gudahtt Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

danjm Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Gudahtt Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

danjm Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gudahtt Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Oct 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

danjm commented Oct 17, 2024 •

edited by github-actions bot

Loading