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.

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

feat(27255): allow local modification for remote feature flags #29696

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ notes.txt
.metamaskrc
.metamaskprodrc

# Manifest customizated configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Manifest customizated configuration
# Customized manifest configuration

.manifest-flags.json

# Test results
test-results/

Expand Down
4 changes: 4 additions & 0 deletions .manifest-flags.json.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"remoteFeatureFlags": {
}
}
11 changes: 11 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@ If you are not a MetaMask Internal Developer, or are otherwise developing on a f
- If debugging MetaMetrics, you'll need to add a value for `SEGMENT_WRITE_KEY` [Segment write key](https://segment.com/docs/connections/find-writekey/), see [Developing on MetaMask - Segment](./development/README.md#segment).
- If debugging unhandled exceptions, you'll need to add a value for `SENTRY_DSN` [Sentry Dsn](https://docs.sentry.io/product/sentry-basics/dsn-explainer/), see [Developing on MetaMask - Sentry](./development/README.md#sentry).
- Optionally, replace the `PASSWORD` value with your development wallet password to avoid entering it each time you open the app.
- Duplicate `manifest-flags.json.dist` within the root and rename it to `manifest-flags.json` by running `cp .manifest-flags.json{.dist,}`. This file is used to add flags to `.manifest.json` build files for the extension. You can add flags to the file to be used in the build process, for example:
Copy link
Contributor

@HowardBraham HowardBraham Jan 17, 2025

Choose a reason for hiding this comment

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

No action required yet, but when we figure out what to call the file, we have to be consistent everywhere. Sometimes in the code and documents it has a dot in front, and sometimes it doesn't.

This is an optional step (not everyone who develops locally has to do this immediately) and that should be specified.

```json
{
"remoteFeatureFlags": {
"testFlagForThreshold": {
"name": "test-flag",
"value": "test-value"
}
}
}
```
- Run `yarn install` to install the dependencies.
- Build the project to the `./dist/` folder with `yarn dist` (for Chromium-based browsers) or `yarn dist:mv2` (for Firefox)

Expand Down
2 changes: 1 addition & 1 deletion app/scripts/lib/setupSentry.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import * as Sentry from '@sentry/browser';
import { logger } from '@sentry/utils';
import browser from 'webextension-polyfill';
import { isManifestV3 } from '../../../shared/modules/mv3.utils';
import { getManifestFlags } from '../../../shared/lib/manifestFlags';
import extractEthjsErrorMessage from './extractEthjsErrorMessage';
import { getManifestFlags } from './manifestFlags';
import { filterEvents } from './sentry-filter-events';

const projectLogger = createProjectLogger('sentry');
Expand Down
27 changes: 26 additions & 1 deletion development/build/manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,29 @@ const { getEnvironment, getBuildName } = require('./utils');

module.exports = createManifestTasks;

async function loadManifestFlags() {
try {
return JSON.parse(
await fs.readFile(
path.join(__dirname, '../../.manifest-flags.json'),
'utf8',
),
);
} catch (error) {
return { remoteFeatureFlags: {} };
}
}

// Initialize with default value
let manifestFlags = { remoteFeatureFlags: {} };

// Load flags asynchronously
loadManifestFlags().then((flags) => {
manifestFlags = flags;
});

module.exports = createManifestTasks;

Comment on lines +21 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

While I don't think the race condition this introduces will ever show its face because the rest of the build process is slow, it's still adding a race condition, which generally makes me uncomfortable. It also hides parsing errors the dev may have made in their .manifest-flags.json file. I think reading synchronously is fine.

function createManifestTasks({
browserPlatforms,
browserVersionMap,
Expand Down Expand Up @@ -47,8 +70,10 @@ function createManifestTasks({
browserVersionMap[platform],
await getBuildModifications(buildType, platform),
customArrayMerge,
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Add customized local manifestFlags to manifest file (old build system)

_flags: manifestFlags,
},
);

modifyNameAndDescForNonProd(result);

const dir = path.join('.', 'dist', platform);
Expand Down
2 changes: 1 addition & 1 deletion development/lib/get-manifest-flag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { exec as callbackExec } from 'node:child_process';
import { hasProperty } from '@metamask/utils';
import { merge } from 'lodash';

import type { ManifestFlags } from '../../app/scripts/lib/manifestFlags';
import type { ManifestFlags } from '../../shared/lib/manifestFlags';

const exec = promisify(callbackExec);
const PR_BODY_FILEPATH = path.resolve(
Expand Down
54 changes: 53 additions & 1 deletion development/webpack/test/plugins.ManifestPlugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@
function runTest(baseManifest: Combination<typeof manifestMatrix>) {
const manifest = baseManifest as unknown as chrome.runtime.Manifest;
const hasTabsPermission = (manifest.permissions || []).includes('tabs');
const transform = transformManifest(args);
const transform = transformManifest(args, false);

if (args.test && hasTabsPermission) {
it("throws in test mode when manifest already contains 'tabs' permission", () => {
Expand Down Expand Up @@ -281,4 +281,56 @@
}
}
});

describe('manifest flags in development mode', () => {
const testManifest = {} as chrome.runtime.Manifest;
const mockFlags = { remoteFeatureFlags: { testFlag: true } };

it('adds manifest flags in development mode', () => {
const transform = transformManifest({ lockdown: true, test: false }, true);

Check failure on line 290 in development/webpack/test/plugins.ManifestPlugin.test.ts

View workflow job for this annotation

GitHub Actions / Test lint / Test lint

Replace `{·lockdown:·true,·test:·false·},·true` with `⏎········{·lockdown:·true,·test:·false·},⏎········true,⏎······`
assert(transform, 'transform should be truthy');

// Mock fs.readFileSync
const fs = require('fs');
const originalReadFileSync = fs.readFileSync;
fs.readFileSync = () => JSON.stringify(mockFlags);

try {
const transformed = transform(testManifest, 'chrome');
assert.deepStrictEqual(
transformed._flags,
mockFlags,
'manifest should have flags in development mode'

Check failure on line 303 in development/webpack/test/plugins.ManifestPlugin.test.ts

View workflow job for this annotation

GitHub Actions / Test lint / Test lint

Insert `,`
);
} finally {
// Restore original readFileSync
fs.readFileSync = originalReadFileSync;
}
});

it('handles missing manifest flags file', () => {
const transform = transformManifest({ lockdown: true, test: false }, true);

Check failure on line 312 in development/webpack/test/plugins.ManifestPlugin.test.ts

View workflow job for this annotation

GitHub Actions / Test lint / Test lint

Replace `{·lockdown:·true,·test:·false·},·true` with `⏎········{·lockdown:·true,·test:·false·},⏎········true,⏎······`
assert(transform, 'transform should be truthy');

// Mock fs.readFileSync to throw ENOENT
const fs = require('fs');
const originalReadFileSync = fs.readFileSync;
fs.readFileSync = () => {
const error = new Error('ENOENT');
throw error;
};

try {
const transformed = transform(testManifest, 'chrome');
assert.deepStrictEqual(
transformed._flags,
{ remoteFeatureFlags: {} },
'manifest should have default flags when file is missing'

Check failure on line 328 in development/webpack/test/plugins.ManifestPlugin.test.ts

View workflow job for this annotation

GitHub Actions / Test lint / Test lint

Insert `,`
);
} finally {
// Restore original readFileSync
fs.readFileSync = originalReadFileSync;
}
});
});
});
5 changes: 4 additions & 1 deletion development/webpack/test/webpack.config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ ${Object.entries(env)
manifest_version: 3,
name: 'name',
version: '1.2.3',
_flags: {
remoteFeatureFlags: {},
},
content_scripts: [
{
js: ['scripts/contentscript.js', 'scripts/inpage.js'],
Expand Down Expand Up @@ -231,7 +234,7 @@ ${Object.entries(env)
assert.deepStrictEqual(manifestPlugin.options.description, null);
assert.deepStrictEqual(manifestPlugin.options.zip, true);
assert(manifestPlugin.options.zipOptions, 'Zip options should be present');
assert.strictEqual(manifestPlugin.options.transform, undefined);
assert.deepStrictEqual(manifestPlugin.options.transform, undefined);

const progressPlugin = instance.options.plugins.find(
(plugin) => plugin && plugin.constructor.name === 'ProgressPlugin',
Expand Down
37 changes: 36 additions & 1 deletion development/webpack/utils/plugins/ManifestPlugin/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@
* @param args
* @param args.lockdown
* @param args.test
* @param isDevelopment
* @returns a function that will transform the manifest JSON object
* @throws an error if the manifest already contains the "tabs" permission and
* `test` is `true`
*/
export function transformManifest(args: { lockdown: boolean; test: boolean }) {
export function transformManifest(
args: { lockdown: boolean; test: boolean },
isDevelopment: boolean,
) {
const transforms: ((manifest: chrome.runtime.Manifest) => void)[] = [];

function removeLockdown(browserManifest: chrome.runtime.Manifest) {
Expand All @@ -29,6 +33,37 @@ export function transformManifest(args: { lockdown: boolean; test: boolean }) {
transforms.push(removeLockdown);
}

/**
davidmurdoch marked this conversation as resolved.
Show resolved Hide resolved
* This function sets predefined flags in the manifest's _flags property
* that are stored in the .manifest-flags.json file.
*
* @param browserManifest - The Chrome extension manifest object to modify
*/
function addManifestFlags(browserManifest: chrome.runtime.Manifest) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Add customized local manifestFlags to manifest file (webpack build system)

let manifestFlags = { remoteFeatureFlags: {} };

try {
const fs = require('fs');
const manifestFlagsContent = fs.readFileSync(
'.manifest-flags.json',
'utf8',
);
manifestFlags = JSON.parse(manifestFlagsContent);
} catch (error: unknown) {
// Only ignore the error if the file doesn't exist
if (error instanceof Error && error.message !== 'ENOENT') {
throw error;
}
}

browserManifest._flags = manifestFlags;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of reading in the .manifest-flags.json file at the top level scope, I think it's best to just try to read it synchronously here.

Also, hiding JSON.parse errors from the end user by catching them will hide formatting errors from the developer, which I don't think they'd like. Probably better to only try/catch the attempt to readFileSync, and then only ignore the caught error if the file does not exist (the error.code property will be "ENOENT"); all other errors readFileSync can throw the developer will probably need to know about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add sync reading + error catching here a5ea54f

}

if (isDevelopment) {
// Add manifest flags only for development builds
transforms.push(addManifestFlags);
}

function addTabsPermission(browserManifest: chrome.runtime.Manifest) {
if (browserManifest.permissions) {
if (browserManifest.permissions.includes('tabs')) {
Expand Down
2 changes: 1 addition & 1 deletion development/webpack/webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ const plugins: WebpackPluginInstance[] = [
version: version.version,
versionName: version.versionName,
browsers: args.browser,
transform: transformManifest(args),
transform: transformManifest(args, isDevelopment),
zip: args.zip,
...(args.zip
? {
Expand Down
12 changes: 12 additions & 0 deletions app/scripts/lib/manifestFlags.ts → shared/lib/manifestFlags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,18 @@ export type ManifestFlags = {
*/
forceEnable?: boolean;
};
/**
* Feature flags to control business logic behavior
*/
remoteFeatureFlags?: {
/**
* A test remote featureflag for threshold
*/
testFlagForThreshold: {
name: string;
value: string;
};
Comment on lines +70 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use better types here? maybe something like testFlagForThreshold: Record<string, any> ( or maybe unknown instead of any, but that may complicate some things).

Or if you really want to go the extra mile, you can define the type as any valid JSON:

type JSONValue =
  | string
  | number
  | boolean
  | null
  | JSONValue[]
  | { [key: string]: JSONValue };

};
};

// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -- you can't extend a type, we want this to be an interface
Expand Down
18 changes: 18 additions & 0 deletions test/e2e/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,21 @@ export enum ACCOUNT_TYPE {
/* Meta metricsId generated by generateMetaMetricsId */
export const MOCK_META_METRICS_ID =
'0x86bacb9b2bf9a7e8d2b147eadb95ac9aaa26842327cd24afc8bd4b3c1d136420';

/* Mock remote feature flags response */
export const MOCK_REMOTE_FEATURE_FLAGS_RESPONSE = {
feature1: true,
feature2: false,
feature3: {
name: 'groupC',
value: 'valueC',
},
};

/* Mock customized remote feature flags response*/
export const MOCK_CUSTOMIZED_REMOTE_FEATURE_FLAGS = {
feature3: {
name: 'groupA',
value: 'valueA',
},
};
23 changes: 23 additions & 0 deletions test/e2e/page-objects/pages/developer-options-page.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { strict as assert } from 'assert';
import { Driver } from '../../webdriver/driver';
import {
MOCK_REMOTE_FEATURE_FLAGS_RESPONSE,
MOCK_CUSTOMIZED_REMOTE_FEATURE_FLAGS,
} from '../../constants';

class DevelopOptions {
private readonly driver: Driver;
Expand All @@ -12,6 +17,9 @@ class DevelopOptions {
css: 'h4',
};

private readonly developerOptionsRemoteFeatureFlagsState: string =
'[data-testid="developer-options-remote-feature-flags"]';

constructor(driver: Driver) {
this.driver = driver;
}
Expand All @@ -33,6 +41,21 @@ class DevelopOptions {
console.log('Generate a page crash in Developer option page');
await this.driver.clickElement(this.generatePageCrashButton);
}

async validateRemoteFeatureFlagState(): Promise<void> {
console.log('Validate remote feature flags state in Developer option page');
const element = await this.driver.findElement(
this.developerOptionsRemoteFeatureFlagsState,
);
const remoteFeatureFlagsState = await element.getText();
assert.equal(
remoteFeatureFlagsState,
JSON.stringify({
...MOCK_REMOTE_FEATURE_FLAGS_RESPONSE,
...MOCK_CUSTOMIZED_REMOTE_FEATURE_FLAGS,
}),
);
}
}

export default DevelopOptions;
2 changes: 1 addition & 1 deletion test/e2e/set-manifest-flags.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import fs from 'fs';
import { merge } from 'lodash';
import { ManifestFlags } from '../../app/scripts/lib/manifestFlags';
import { ManifestFlags } from '../../shared/lib/manifestFlags';
import { fetchManifestFlagsFromPRAndGit } from '../../development/lib/get-manifest-flag';

export const folder = `dist/${process.env.SELENIUM_BROWSER}`;
Expand Down
39 changes: 37 additions & 2 deletions test/e2e/tests/remote-feature-flag/remote-feature-flag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,14 @@ import { getCleanAppState, withFixtures } from '../../helpers';
import FixtureBuilder from '../../fixture-builder';
import { TestSuiteArguments } from '../confirmations/transactions/shared';
import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow';
import { MOCK_META_METRICS_ID } from '../../constants';
import { MOCK_REMOTE_FEATURE_FLAGS_RESPONSE } from './mock-data';
import HeaderNavbar from '../../page-objects/pages/header-navbar';
import SettingsPage from '../../page-objects/pages/settings/settings-page';
import DevelopOptions from '../../page-objects/pages/developer-options-page';
import {
MOCK_CUSTOMIZED_REMOTE_FEATURE_FLAGS,
MOCK_META_METRICS_ID,
MOCK_REMOTE_FEATURE_FLAGS_RESPONSE,
} from '../../constants';

describe('Remote feature flag', function (this: Suite) {
it('should be fetched with threshold value when basic functionality toggle is on', async function () {
Expand Down Expand Up @@ -45,4 +51,33 @@ describe('Remote feature flag', function (this: Suite) {
},
);
});

it('offers the option to pass into manifest file for developers along with original response', async function () {
await withFixtures(
{
fixtures: new FixtureBuilder()
.withMetaMetricsController({
metaMetricsId: MOCK_META_METRICS_ID,
participateInMetaMetrics: true,
})
.build(),
manifestFlags: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Usage in e2e test: override remoteFeatureFlags and assert the result to be MOCK_CUSTOMIZED_REMOTE_FEATURE_FLAGS

remoteFeatureFlags: MOCK_CUSTOMIZED_REMOTE_FEATURE_FLAGS,
},
title: this.test?.fullTitle(),
},
async ({ driver }: TestSuiteArguments) => {
await loginWithBalanceValidation(driver);
const headerNavbar = new HeaderNavbar(driver);
await headerNavbar.openSettingsPage();
const settingsPage = new SettingsPage(driver);
await settingsPage.check_pageIsLoaded();
await settingsPage.goToDevelopOptionSettings();

const developOptionsPage = new DevelopOptions(driver);
await developOptionsPage.check_pageIsLoaded();
await developOptionsPage.validateRemoteFeatureFlagState();
},
);
});
});
3 changes: 1 addition & 2 deletions ui/helpers/utils/mm-lazy.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
// eslint-disable-next-line import/no-restricted-paths
import { getManifestFlags } from '../../../app/scripts/lib/manifestFlags';
import { getManifestFlags } from '../../../shared/lib/manifestFlags';
import { endTrace, trace, TraceName } from '../../../shared/lib/trace';

type DynamicImportType = () => Promise<{ default: React.ComponentType }>;
Expand Down
Loading
Loading