Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: Disable account syncing in prod #27943

Merged
merged 2 commits into from
Oct 19, 2024
Merged

chore: Disable account syncing in prod #27943

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2e78ad
Disable account syncing in prod
danjm Oct 17, 2024
dbd02ca
Update app/scripts/metamask-controller.js
danjm Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/scripts/metamask-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import {
providerErrors,
} from '@metamask/rpc-errors';

import { isProduction } from '../../shared/modules/environment';
Copy link
Member

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


import { Mutex } from 'await-semaphore';
import log from 'loglevel';
import {
Expand Down Expand Up @@ -1561,7 +1563,7 @@ export default class MetamaskController extends EventEmitter {
},
},
env: {
isAccountSyncingEnabled: isManifestV3,
isAccountSyncingEnabled: !isProduction() && isManifestV3,,
Copy link
Member

Choose a reason for hiding this comment

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

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 = {

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on setting this to false instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

danjm marked this conversation as resolved.
Show resolved Hide resolved
},
messenger: this.controllerMessenger.getRestricted({
name: 'UserStorageController',
Expand Down
Loading