feat: implement remote feature flag controller #12427

joaoloureirop · 2024-11-26T09:52:43Z

Description

Introduction of @metamask/remote-feature-flag-controller library.

Remote feature flag controller manages data flow, retry policy, and cache expiry.
The controller consumer manages default values, data persistency, and data distribution.

See ADR for a in-depth description

Technical decisions

Controller init on `Engine.ts` with feature flags fetching only on cold app starts.

@metamask/remote-feature-flag-controller is only asked to fetch feature flags after its init in Engine.ts. Ensures feature flags are only fetched on cold app starts.

Fallback values

Default values are used when remote feature flags are undefined.
The fallback mechanism is implemented by each feature flag selector app/selectors/featureFlagsController/<featureFlagName>

In this PR we include minimumAppVersion selector, which manages the LD feature flag mobile-minimum-versions

One selector per each feature flag

LD feature flags can be boolean, number, strings on JSON objects.
We've decided to have each feature flag with its own selector

A feature flag selector contains:

state selectors for each feature flag value.
business logic
defaults for when feature flags values are undefined.
TS types.
unit tests and mocked data.

This architecture offers a clear separation between each feature flag and the logic behind it, allowing easier manipulation.
Code owners are assigned to each feature flag.

Related issues

Fixes: https://github.com/MetaMask/mobile-planning/issues/2054
Fixes: https://github.com/MetaMask/mobile-planning/issues/1975

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile 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.

allow remote feature flag controller symlink to get bundled by metro

github-actions · 2024-11-26T09:52:56Z

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.

app/core/Engine.ts

This reverts commit cdd4a58.

joaoloureirop · 2024-11-26T19:48:32Z

metro.config.js

+const path = require("path");
+
+const featureFlagModuleDir = path.resolve(__dirname, "../../core/feature-flags/packages/remote-feature-flag-controller");
+
+const extraNodeModules = {
+  "@metamask/remote-feature-flag-controller": featureFlagModuleDir,
+};
+
+const watchFolders = [
+  featureFlagModuleDir,
+];
+


To be removed once controller has been published to NPM

Will revert 00a3bd1

joaoloureirop · 2024-11-26T19:48:47Z

metro.config.js

+    watchFolders,
    resolver: {
      assetExts: assetExts.filter((ext) => ext !== 'svg'),
      sourceExts: [...sourceExts, 'svg', 'cjs', 'mjs'],
      resolverMainFields: ['sbmodern', 'react-native', 'browser', 'main'],
+      extraNodeModules: new Proxy (extraNodeModules, {
+        get: (target, name) =>
+          name in target ? target[name] : path.join(process.cwd(), `node_modules/${name}`),
+      }),
+      unstable_enableSymlinks: true,


To be removed once controller has been published to NPM

Will revert 00a3bd1

joaoloureirop · 2024-11-26T19:49:27Z

package.json

@@ -175,6 +175,7 @@
    "@metamask/react-native-payments": "^2.0.0",
    "@metamask/react-native-search-api": "1.0.1",
    "@metamask/react-native-webview": "^14.0.4",
+    "@metamask/remote-feature-flag-controller": "link:../../core/feature-flags/packages/remote-feature-flag-controller",


update once controller has been published to NPM

joaoloureirop · 2024-11-26T19:49:41Z

yarn.lock

+"@metamask/remote-feature-flag-controller@link:../../core/feature-flags/packages/remote-feature-flag-controller":
+  version "0.0.0"
+  uid ""
+
 "@metamask/[email protected]", "@metamask/rpc-errors@^6.0.0", "@metamask/rpc-errors@^6.2.1", "@metamask/rpc-errors@^6.3.1", "@metamask/rpc-errors@^7.0.0", "@metamask/rpc-errors@^7.0.1":


update once controller has been published to NPM

app/selectors/featureFlagController/index.ts

github-actions · 2024-11-27T13:58:06Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: ebc940e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/53e344f4-a5a0-43e1-8b09-0df88e619bf2

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

Prithpal-Sooriya · 2024-11-27T14:41:59Z

app/core/Engine/Engine.ts

@@ -933,8 +945,7 @@ export class Engine {
      encryptor,
      getMnemonic: getPrimaryKeyringMnemonic.bind(this),
      getFeatureFlags: () => ({
-        disableSnaps:
-          store.getState().settings.basicFunctionalityEnabled === false,
+        disableSnaps: !isBasicFunctionalityEnabled,


Cautious warning.
Before: it would recall getState() and pick out the basicFunctionalityEnabled property.
After: Now it is a constant. Meaning that is won't re-fire getState() to get the latest value.

So hypothetical, if we turned off basic functionality, and this snap getFeatureFlags is executed, it won't get the up-to-date field from state, but instead the constant when the app was initialised.

We could make the constant a function instead so we can evaluate lazily (once called) instead of just once at app init?

- const isBasicFunctionalityEnabled = ... + const isBasicFunctionalityEnabled = () => ...

Great catch, thank you!

addressed in 8014b45

This reverts commit b4f9cc8.

joaoloureirop · 2024-11-27T15:09:46Z

app/selectors/featureFlagController/minimumAppVersion/index.test.ts

+  it('should return feature flag object', () => {
+    const {
+      appMinimumBuild,
+      appleMinimumOS,
+      androidMinimumAPIVersion,
+    } = selectMobileMinimumVersions(mockedState as RootState);
+    expect(appleMinimumOS).toBeDefined();
+    expect(appMinimumBuild).toBeDefined();
+    expect(androidMinimumAPIVersion).toBeDefined();
+  });
+  it('should return fallback values', () => {
+    const {
+      appMinimumBuild,
+      appleMinimumOS,
+      androidMinimumAPIVersion,
+    } = selectMobileMinimumVersions(mockedEmptyFlagsState as RootState);
+    expect(appMinimumBuild).toBe(1024);
+    expect(appleMinimumOS).toBe(1025);
+    expect(androidMinimumAPIVersion).toBe(1026);


Test feature flag state selectors separate

replace hardcoded numbers with mocked values.

joaoloureirop · 2024-11-27T15:10:39Z

app/core/Engine/controllers/RemoteFeatureFlagController/index.ts

+} from './utils';
+
+
+const init = ({


add remote feature flag controller init unit tests

is this a TODO?

Thanks. Yes unit tests please!

app/reducers/index.ts

NicolasMassart

First pass of review

NicolasMassart · 2024-11-27T16:33:12Z

.github/CODEOWNERS

+app/core/Analytics/                                         @MetaMask/mobile-platform
+app/util/metrics/                                           @MetaMask/mobile-platform
+app/components/hooks/useMetrics/                            @MetaMask/mobile-platform
+app/selectors/featureFlagController/index.ts                @MetaMask/mobile-platform


S: maybe you can make the team own the whole app/selectors/featureFlagController directory instead of individual files?

We want app/selectors/featureFlagController/<featureFlagName> to be owned by other teams

I'll update the CODEOWNERS syntax to match files like featureFlagController/index.ts but no further nested files

addressed on b784e89

app/components/hooks/MinimumVersions/useMinimumVersions.tsx

NicolasMassart · 2024-11-27T17:14:01Z

app/components/hooks/MinimumVersions/useMinimumVersions.tsx

-    (state: RootState) => state.featureFlags,
-  );
+
+  const appMinimumBuild = useSelector((state: RootState) => selectAppMinimumBuild(state));


Q: not sure if this change makes the mock in the test obsolete and how this test doesn't need any changes.

Good catch, the tests for this hook need to be updated

I'm surprised the test is passing given the changes in the hook... so maybe rewrite the test in a more robust way. A change like this should make it fail otherwise it probably means it's not testing anything but the mocks...

NicolasMassart · 2024-11-27T17:21:25Z

app/core/Engine/controllers/RemoteFeatureFlagController/index.ts

+} from './utils';
+
+
+const init = ({


is this a TODO?

Co-authored-by: Nico MASSART <[email protected]>

NicolasMassart

Thanks for this awesome work @joaoloureirop!
Added some guideline suggestions and a few questions on some code parts I'm not sure to fully understand.

NicolasMassart · 2024-11-27T18:10:29Z

app/core/Engine/Engine.ts

@@ -266,6 +268,10 @@ export class Engine {
  ) {
    this.controllerMessenger = new ExtendedControllerMessenger();

+    // Basic Functionality toggle defaults to true
+    const getBasicFunctionalityToggleState = () =>


S: should we use a selector here?

NicolasMassart · 2024-11-27T18:19:32Z

app/core/Engine/Engine.ts

+      initialState,
+      controllerMessenger: this.controllerMessenger,
+      fetchFunction,
+      disabled: getBasicFunctionalityToggleState() === false


Q: why testing the exact boolean false equality here?
If the value is true, it's true.
If the value is false, it's true.
If the value eis anything like undefined or null, it will be true because of the getBasicFunctionalityToggleState default.
So if we have a false here, it's necessarily a real false as a value in the state. So the following would be enough:

Suggested change

disabled: getBasicFunctionalityToggleState() === false

disabled: !getBasicFunctionalityToggleState()

and same on line 949

NicolasMassart · 2024-11-27T18:21:37Z

app/core/Engine/controllers/RemoteFeatureFlagController/index.ts

+} from './utils';
+
+
+const init = ({


Thanks. Yes unit tests please!

NicolasMassart · 2024-11-27T18:24:57Z

app/selectors/featureFlagController/index.test.ts

+  });
+
+
+  it('should return feature flag initial state', () => {


Suggested change

it('should return feature flag initial state', () => {

it('returns feature flag initial state', () => {

NicolasMassart · 2024-11-27T18:25:47Z

app/selectors/featureFlagController/minimumAppVersion/index.test.ts

+    jest.clearAllMocks();
+  });
+
+  it('should return feature flag object', () => {


Suggested change

it('should return feature flag object', () => {

it('returns feature flag object', () => {

NicolasMassart · 2024-11-27T18:26:14Z

app/selectors/featureFlagController/minimumAppVersion/index.test.ts

+    expect(appMinimumBuild).toBeDefined();
+    expect(androidMinimumAPIVersion).toBeDefined();
+  });
+  it('should return fallback values', () => {


Suggested change

it('should return fallback values', () => {

it('returns fallback values', () => {

NicolasMassart · 2024-11-27T18:27:26Z

app/selectors/featureFlagController/minimumAppVersion/index.ts

+    appMinimumBuild: 1243,
+    appleMinimumOS: 6,
+    androidMinimumAPIVersion: 21,


S: should we have this in a constants file?

NicolasMassart · 2024-11-27T18:28:53Z

app/selectors/featureFlagController/minimumAppVersion/index.test.ts

+    expect(appMinimumBuild).toBeDefined();
+    expect(androidMinimumAPIVersion).toBeDefined();
+  });
+  it('should return fallback values', () => {


Q: we only have this test that tests the fallback values. How do we test the normal values? Could we have a mocked test for this?

NicolasMassart · 2024-11-27T18:30:57Z

app/store/migrations/061.test.ts

+  ];
+
+  for (const { errorMessage, scenario, state } of invalidStates) {
+    it(`should capture exception if ${scenario}`, async () => {


Suggested change

it(`should capture exception if ${scenario}`, async () => {

it(`captures exception if ${scenario}`, async () => {

NicolasMassart · 2024-11-27T18:31:25Z

app/store/migrations/061.test.ts

+    });
+  }
+
+  it('remove featureFlags property from redux state', async () => {


Suggested change

it('remove featureFlags property from redux state', async () => {

it('removes featureFlags property from redux state', async () => {

Cal-L

Left some comments

Cal-L · 2024-11-27T20:25:26Z

app/core/Engine/controllers/RemoteFeatureFlagController/index.ts

To keep things consistent, let's follow the pattern that resembles that of what is currently in Engine/controllers/accounts The directory name RemoteFeatureFlagController is fine. We can keep the index file that you have and use it to re-export utils and anything else in the future.

In this case, it's debatable whether or not we should just put all of the functions in index or utils file. IMO, creating the utils file and re-exporting via index allows us to be more flexible in the future, where index can export other things such as constants, etc.

Cal-L · 2024-11-27T20:26:21Z

app/core/Engine/controllers/RemoteFeatureFlagController/index.ts

+} from './utils';
+
+
+const init = ({


Rename to createRemoteFeatureFlagsController

Suggested change

const init = ({

const createRemoteFeatureFlagsController = ({

Move into utils file

Cal-L · 2024-11-27T20:27:19Z

app/core/Engine/controllers/RemoteFeatureFlagController/index.ts

+      allowedEvents: [],
+    }),
+    state: {
+      ...initialState.RemoteFeatureFlagController


No need to spread in an object. Just assign the value. Also pass initialState.RemoteFeatureFlagController into initialState instead of deconstructing from here

Cal-L · 2024-11-27T20:29:49Z

app/core/Engine/controllers/RemoteFeatureFlagController/index.ts

+}: RemoteFeatureFlagInitParamTypes) => {
+
+  const remoteFeatureFlagController = new RemoteFeatureFlagController({
+    messenger: controllerMessenger.getRestricted({


Move messenger creation back into Engine file since we as a platform team (at least) need to be aware of messenger related changes. You can reference how the accounts controller is set up.

Discussed with Mark a few days ago - Here's the PR that updated the accounts controller side - #12416

Cal-L · 2024-11-27T20:33:05Z

app/core/Engine/controllers/RemoteFeatureFlagController/types.ts

+import { ControllerMessenger, EngineState } from '../../types';
+
+export interface RemoteFeatureFlagInitParamTypes {
+  initialState: Partial<EngineState>;


Suggested change

initialState: Partial<EngineState>;

initialState: RemoteFeatureFlagControllerState;

Cal-L · 2024-11-27T20:33:29Z

app/core/Engine/controllers/RemoteFeatureFlagController/types.ts

+
+export interface RemoteFeatureFlagInitParamTypes {
+  initialState: Partial<EngineState>;
+  controllerMessenger: ControllerMessenger,


Suggested change

controllerMessenger: ControllerMessenger,

controllerMessenger: RemoteFeatureFlagControllerMessenger,

Cal-L · 2024-11-27T21:10:53Z

app/store/index.ts

@@ -87,11 +87,6 @@ const createStoreAndPersistor = async () => {
      basicFunctionalityEnabled:
        store.getState().settings.basicFunctionalityEnabled,
    });
-    // Fetch feature flags only if basic functionality is enabled


Let's confirm that the code above this is also no longer needed. Rehydration should automatically populate it already

Cal-L · 2024-11-27T21:14:26Z

app/store/migrations/061.ts

+  if (hasProperty(state, 'featureFlags')) {
+    delete state.featureFlags;
+  }
+  return state;


Let's update this migration to include the default state for RemoteFeatureFlagController to be safe

joaoloureirop added 4 commits November 22, 2024 11:40

setup metro bundler symlink resolution

00a3bd1

allow remote feature flag controller symlink to get bundled by metro

yarn setup fixup

b4f9cc8

metro bundler: bypass lint errors

cdd4a58

init RemoteFeatureFlagController

1787680

joaoloureirop self-assigned this Nov 26, 2024

joaoloureirop changed the title ~~feat: feature-flag-controller~~ feat: implement remote feature flag controller Nov 26, 2024

controller init state destruct

5299891

gauthierpetetin added the team-mobile-platform label Nov 26, 2024

joaoloureirop added 2 commits November 26, 2024 12:23

add feature flag selectors

6204914

ff(mobileMinimumVersion): set fallback values

6f8d208

joaoloureirop force-pushed the feat/feature-flags branch from 5e9991d to 6f8d208 Compare November 26, 2024 14:10

joaoloureirop added 5 commits November 26, 2024 14:13

remove feature flag saga / slice

608f05b

remote feature flags behing basic func toggle

a8ec092

fix controller state change event

01df041

get env vars to init feature flag controllers

c8c2af0

update min version fallback values

ca33006

joaoloureirop commented Nov 26, 2024

View reviewed changes

app/core/Engine.ts Outdated Show resolved Hide resolved

joaoloureirop added 4 commits November 26, 2024 19:32

remove controller messenger allowances

27b32a3

Revert "metro bundler: bypass lint errors"

d7aed0d

This reverts commit cdd4a58.

remove unused import

b80145d

fix lint errors

9a67929

joaoloureirop force-pushed the feat/feature-flags branch from 74b9b88 to 9a67929 Compare November 26, 2024 19:44

joaoloureirop added 2 commits November 26, 2024 19:46

remove unneded semicolon

934a13a

remove featureFlag initial root state

faa6cb0

joaoloureirop commented Nov 26, 2024

View reviewed changes

Cal-L reviewed Nov 26, 2024

View reviewed changes

app/selectors/featureFlagController/index.ts Outdated Show resolved Hide resolved

joaoloureirop added 5 commits November 27, 2024 12:25

set RemoteFeatureFlagController & minappversion flag codeowners

ba1802e

minimumAppVersion featureflag: improve types

5e97075

remove dead code

c838b21

add Feature flag controller logger

426655e

Merge branch 'main' into feat/feature-flags

ebc940e

joaoloureirop added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Nov 27, 2024

joaoloureirop marked this pull request as ready for review November 27, 2024 13:55

joaoloureirop requested review from a team as code owners November 27, 2024 13:55

joaoloureirop requested review from sethkfman and removed request for sethkfman November 27, 2024 13:55

joaoloureirop added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 27, 2024

joaoloureirop requested a review from Gudahtt November 27, 2024 14:32

Prithpal-Sooriya reviewed Nov 27, 2024

View reviewed changes

joaoloureirop added 2 commits November 27, 2024 14:54

fix basic functionality toggle

8014b45

Revert "yarn setup fixup"

9a35568

This reverts commit b4f9cc8.

joaoloureirop requested a review from NicolasMassart November 27, 2024 15:02

joaoloureirop commented Nov 27, 2024

View reviewed changes

app/reducers/index.ts Show resolved Hide resolved

joaoloureirop added 2 commits November 27, 2024 15:17

improve minimumAppVersion TS types

8c305c1

fix unit tests

c8495e5

NicolasMassart reviewed Nov 27, 2024

View reviewed changes

joaoloureirop and others added 3 commits November 27, 2024 18:13

migration: remove featureFlags property

d7d384c

codeowners: featureFlagController selector base files pattern

b784e89

update useMinimumVersions.tsx import paths

38292f7

Co-authored-by: Nico MASSART <[email protected]>

NicolasMassart reviewed Nov 27, 2024

View reviewed changes

Cal-L reviewed Nov 27, 2024

View reviewed changes

	disabled: getBasicFunctionalityToggleState() === false
	disabled: !getBasicFunctionalityToggleState()

	it('should return feature flag initial state', () => {
	it('returns feature flag initial state', () => {

	it('should return feature flag object', () => {
	it('returns feature flag object', () => {

	it('should return fallback values', () => {
	it('returns fallback values', () => {

	it(`should capture exception if ${scenario}`, async () => {
	it(`captures exception if ${scenario}`, async () => {

	it('remove featureFlags property from redux state', async () => {
	it('removes featureFlags property from redux state', async () => {

	const init = ({
	const createRemoteFeatureFlagsController = ({

	initialState: Partial<EngineState>;
	initialState: RemoteFeatureFlagControllerState;

	controllerMessenger: ControllerMessenger,
	controllerMessenger: RemoteFeatureFlagControllerMessenger,

feat: implement remote feature flag controller #12427

Are you sure you want to change the base?

feat: implement remote feature flag controller #12427

Conversation

joaoloureirop commented Nov 26, 2024 • edited Loading

Description

Technical decisions

Controller init on Engine.ts with feature flags fetching only on cold app starts.

Fallback values

One selector per each feature flag

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 27, 2024 • edited by metamaskbot Loading

Bitrise

Choose a reason for hiding this comment

Prithpal-Sooriya Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

joaoloureirop Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

joaoloureirop Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

joaoloureirop Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasMassart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasMassart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cal-L left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cal-L Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoloureirop commented Nov 26, 2024 •

edited

Loading

Controller init on `Engine.ts` with feature flags fetching only on cold app starts.

github-actions bot commented Nov 27, 2024 •

edited by metamaskbot

Loading

Prithpal-Sooriya Nov 27, 2024 •

edited

Loading

joaoloureirop Nov 27, 2024 •

edited

Loading

joaoloureirop Nov 27, 2024 •

edited

Loading

joaoloureirop Nov 27, 2024 •

edited

Loading

Cal-L Nov 27, 2024 •

edited

Loading