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: implement remote feature flag controller #12427

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
00a3bd1
setup metro bundler symlink resolution
joaoloureirop Nov 22, 2024
b4f9cc8
yarn setup fixup
joaoloureirop Nov 22, 2024
cdd4a58
metro bundler: bypass lint errors
joaoloureirop Nov 22, 2024
1787680
init RemoteFeatureFlagController
joaoloureirop Nov 22, 2024
5299891
controller init state destruct
joaoloureirop Nov 26, 2024
6204914
add feature flag selectors
joaoloureirop Nov 26, 2024
6f8d208
ff(mobileMinimumVersion): set fallback values
joaoloureirop Nov 26, 2024
608f05b
remove feature flag saga / slice
joaoloureirop Nov 26, 2024
a8ec092
remote feature flags behing basic func toggle
joaoloureirop Nov 26, 2024
01df041
fix controller state change event
joaoloureirop Nov 26, 2024
c8c2af0
get env vars to init feature flag controllers
joaoloureirop Nov 26, 2024
ca33006
update min version fallback values
joaoloureirop Nov 26, 2024
27b32a3
remove controller messenger allowances
joaoloureirop Nov 26, 2024
d7aed0d
Revert "metro bundler: bypass lint errors"
joaoloureirop Nov 26, 2024
b80145d
remove unused import
joaoloureirop Nov 26, 2024
9a67929
fix lint errors
joaoloureirop Nov 26, 2024
934a13a
remove unneded semicolon
joaoloureirop Nov 26, 2024
faa6cb0
remove featureFlag initial root state
joaoloureirop Nov 26, 2024
20c3b42
Merge branch 'main' into feat/feature-flags
joaoloureirop Nov 27, 2024
d22d1cb
Engine: split Feature flag controller
joaoloureirop Nov 27, 2024
ba1802e
set RemoteFeatureFlagController & minappversion flag codeowners
joaoloureirop Nov 27, 2024
5e97075
minimumAppVersion featureflag: improve types
joaoloureirop Nov 27, 2024
c838b21
remove dead code
joaoloureirop Nov 27, 2024
426655e
add Feature flag controller logger
joaoloureirop Nov 27, 2024
ebc940e
Merge branch 'main' into feat/feature-flags
joaoloureirop Nov 27, 2024
8014b45
fix basic functionality toggle
joaoloureirop Nov 27, 2024
9a35568
Revert "yarn setup fixup"
joaoloureirop Nov 27, 2024
8c305c1
improve minimumAppVersion TS types
joaoloureirop Nov 27, 2024
c8495e5
fix unit tests
joaoloureirop Nov 27, 2024
d7d384c
migration: remove featureFlags property
joaoloureirop Nov 27, 2024
b784e89
codeowners: featureFlagController selector base files pattern
joaoloureirop Nov 27, 2024
38292f7
update useMinimumVersions.tsx import paths
joaoloureirop Nov 27, 2024
7661b90
update feature flags format
joaoloureirop Nov 28, 2024
f5b754f
review round 1
joaoloureirop Nov 28, 2024
7bdcc81
remove unused value selector
joaoloureirop Nov 28, 2024
e54d05d
Revert "setup metro bundler symlink resolution"
joaoloureirop Nov 29, 2024
b4fb866
get controller from registry
joaoloureirop Nov 29, 2024
f20e31a
fix feature flag types
joaoloureirop Nov 29, 2024
6f709eb
remove trycatch block from controller init
joaoloureirop Nov 29, 2024
82e2c88
feature flag controller: rename fetch arg
joaoloureirop Nov 29, 2024
54424a6
remove fetch argument
joaoloureirop Nov 29, 2024
96ca13b
add selector unit tests
joaoloureirop Nov 29, 2024
b5d9a6a
revert ios changes
joaoloureirop Nov 29, 2024
e12bfb6
remove any from test types
joaoloureirop Nov 29, 2024
50795f0
add basic functionality selector
joaoloureirop Nov 29, 2024
0126119
move default values into consts file
joaoloureirop Nov 29, 2024
674962d
fix mock import
joaoloureirop Nov 29, 2024
d451f5c
mock engine selector
joaoloureirop Nov 29, 2024
5cfbd03
chore: add unit tests to feature flags (#12503)
tommasini Nov 29, 2024
9cd1bf0
Merge branch 'main' into feat/feature-flags
joaoloureirop Nov 29, 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
37 changes: 20 additions & 17 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,22 @@
app/component-library/ @MetaMask/design-system-engineers

# Platform Team
.github/CODEOWNERS @MetaMask/mobile-platform
patches/ @MetaMask/mobile-platform
app/core/Engine/Engine.ts @MetaMask/mobile-platform
app/core/Engine/Engine.test.ts @MetaMask/mobile-platform
app/core/Engine/index.ts @MetaMask/mobile-platform
app/core/Engine/types.ts @MetaMask/mobile-platform
app/core/Analytics/ @MetaMask/mobile-platform
app/util/metrics/ @MetaMask/mobile-platform
app/components/hooks/useMetrics/ @MetaMask/mobile-platform
app/store/migrations/ @MetaMask/mobile-platform
bitrise.yml @MetaMask/mobile-platform
yarn.lock @MetaMask/mobile-platform
ios/Podfile.lock @MetaMask/mobile-platform
.github/CODEOWNERS @MetaMask/mobile-platform
patches/ @MetaMask/mobile-platform
app/core/Engine/Engine.ts @MetaMask/mobile-platform
app/core/Engine/Engine.test.ts @MetaMask/mobile-platform
app/core/Engine/index.ts @MetaMask/mobile-platform
app/core/Engine/types.ts @MetaMask/mobile-platform
app/core/Engine/controllers/RemoteFeatureFlagController/ @MetaMask/mobile-platform
app/core/Analytics/ @MetaMask/mobile-platform
app/util/metrics/ @MetaMask/mobile-platform
app/components/hooks/useMetrics/ @MetaMask/mobile-platform
app/selectors/featureFlagController/* @MetaMask/mobile-platform
app/selectors/featureFlagController/minimumAppVersion/ @MetaMask/mobile-platform
app/store/migrations/ @MetaMask/mobile-platform
bitrise.yml @MetaMask/mobile-platform
yarn.lock @MetaMask/mobile-platform
ios/Podfile.lock @MetaMask/mobile-platform

# Ramps Team
app/components/UI/Ramp/ @MetaMask/ramp
Expand Down Expand Up @@ -53,10 +56,10 @@ app/components/UI/Swaps @MetaMask/swaps-engineers
app/components/Views/Notifications @MetaMask/notifications
app/components/Views/Settings/NotificationsSettings @MetaMask/notifications
app/components/UI/Notifications @MetaMask/notifications
app/reducers/notification @MetaMask/notifications
app/actions/notification @MetaMask/notifications
app/selectors/notification @MetaMask/notifications
app/util/notifications @MetaMask/notifications
app/reducers/notification @MetaMask/notifications
app/actions/notification @MetaMask/notifications
app/selectors/notification @MetaMask/notifications
app/util/notifications @MetaMask/notifications
app/store/util/notifications @MetaMask/notifications

# LavaMoat Team
Expand Down
29 changes: 24 additions & 5 deletions app/components/hooks/MinimumVersions/useMinimumVersions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,21 @@ describe('useMinimumVersions', () => {
jest.clearAllMocks();
(useNavigation as jest.Mock).mockReturnValue(mockNavigation);
});

it('requires update only if automaticSecurityChecksEnabled', () => {
(useSelector as jest.Mock).mockImplementation(() => ({
security: { automaticSecurityChecksEnabled: false },
featureFlags: {
featureFlags: { mobileMinimumVersions: { appMinimumBuild: 100 } },
engine: {
backgroundState: {
RemoteFeatureFlagController: {
remoteFeatureFlags: {
mobileMinimumVersions: {
appMinimumBuild: 100,
appleMinimumOS: 100,
androidMinimumAPIVersion: 100,
},
},
},
},
},
}));

Expand All @@ -54,8 +63,18 @@ describe('useMinimumVersions', () => {
it('requires update only if currentBuildNumber is lower than appMinimumBuild', () => {
(useSelector as jest.Mock).mockImplementation(() => ({
security: { automaticSecurityChecksEnabled: true },
featureFlags: {
featureFlags: { mobileMinimumVersions: { appMinimumBuild: 100 } },
engine: {
backgroundState: {
RemoteFeatureFlagController: {
remoteFeatureFlags: {
mobileMinimumVersions: {
appMinimumBuild: 100,
appleMinimumOS: 100,
androidMinimumAPIVersion: 100,
},
},
},
},
},
}));

Expand Down
14 changes: 6 additions & 8 deletions app/components/hooks/MinimumVersions/useMinimumVersions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,20 @@ import { createUpdateNeededNavDetails } from '../../UI/UpdateNeeded/UpdateNeeded
import { useSelector } from 'react-redux';
import { useNavigation } from '@react-navigation/native';
import { InteractionManager } from 'react-native';
import { FeatureFlagsState } from '../../../core/redux/slices/featureFlags';
import { SecurityState } from '../../../../app/reducers/security';
import { RootState } from '../../../../app/reducers';
import { SecurityState } from '../../../reducers/security';
import { RootState } from '../../../reducers';
import { selectAppMinimumBuild } from '../../../selectors/featureFlagController/minimumAppVersion';

const useMinimumVersions = () => {
const { automaticSecurityChecksEnabled }: SecurityState = useSelector(
(state: RootState) => state.security,
);
const { featureFlags }: FeatureFlagsState = useSelector(
(state: RootState) => state.featureFlags,
);

const appMinimumBuild = useSelector((state: RootState) => selectAppMinimumBuild(state));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

const currentBuildNumber = Number(getBuildNumber());
const navigation = useNavigation();
const shouldTriggerUpdateFlow =
automaticSecurityChecksEnabled &&
featureFlags?.mobileMinimumVersions?.appMinimumBuild > currentBuildNumber;
automaticSecurityChecksEnabled && appMinimumBuild > currentBuildNumber;

useEffect(() => {
if (shouldTriggerUpdateFlow) {
Expand Down
5 changes: 4 additions & 1 deletion app/core/Engine/Engine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ jest.mock('../../store', () => ({
jest.mock('../../selectors/smartTransactionsController', () => ({
selectShouldUseSmartTransaction: jest.fn().mockReturnValue(false),
}));

jest.mock('../../selectors/settings', () => ({
selectBasicFunctionalityEnabled: jest.fn().mockReturnValue(true),
}));
describe('Engine', () => {
it('should expose an API', () => {
const engine = Engine.init({});
Expand All @@ -37,6 +39,7 @@ describe('Engine', () => {
expect(engine.context).toHaveProperty('NetworkController');
expect(engine.context).toHaveProperty('PhishingController');
expect(engine.context).toHaveProperty('PreferencesController');
expect(engine.context).toHaveProperty('RemoteFeatureFlagController');
expect(engine.context).toHaveProperty('SignatureController');
expect(engine.context).toHaveProperty('TokenBalancesController');
expect(engine.context).toHaveProperty('TokenRatesController');
Expand Down
43 changes: 30 additions & 13 deletions app/core/Engine/Engine.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable @typescript-eslint/no-shadow */
import Crypto from 'react-native-quick-crypto';
import { scrypt } from 'react-native-fast-crypto';

import {
AccountTrackerController,
AssetsContractController,
Expand Down Expand Up @@ -153,6 +154,7 @@
} from './controllers/accounts/constants';
import { AccountsControllerMessenger } from '@metamask/accounts-controller';
import { createAccountsController } from './controllers/accounts/utils';
import { createRemoteFeatureFlagController } from './controllers/RemoteFeatureFlagController';
import { captureException } from '@sentry/react-native';
import { lowerCase } from 'lodash';
import {
Expand All @@ -161,6 +163,7 @@
} from '../../core/redux/slices/inpageProvider';
import SmartTransactionsController from '@metamask/smart-transactions-controller';
import { getAllowedSmartTransactionsChainIds } from '../../../app/constants/smartTransactions';
import { selectBasicFunctionalityEnabled } from '../../selectors/settings';
import { selectShouldUseSmartTransaction } from '../../selectors/smartTransactionsController';
import { selectSwapsChainFeatureFlags } from '../../reducers/swaps';
import { SmartTransactionStatuses, ClientId } from '@metamask/smart-transactions-controller/dist/types';
Expand Down Expand Up @@ -264,6 +267,9 @@
) {
this.controllerMessenger = new ExtendedControllerMessenger();

const isBasicFunctionalityToggleEnabled = () =>
selectBasicFunctionalityEnabled(store.getState());

const approvalController = new ApprovalController({
messenger: this.controllerMessenger.getRestricted({
name: 'ApprovalController',
Expand Down Expand Up @@ -476,6 +482,16 @@
'https://gas.api.cx.metamask.io/networks/<chain_id>/suggestedGasFees',
});

const remoteFeatureFlagController = createRemoteFeatureFlagController({
state: initialState.RemoteFeatureFlagController,
messenger: this.controllerMessenger.getRestricted({
name: 'RemoteFeatureFlagController',
allowedActions: [],
allowedEvents: [],
}),
disabled: !isBasicFunctionalityToggleEnabled(),
});

const phishingController = new PhishingController({
messenger: this.controllerMessenger.getRestricted({
name: 'PhishingController',
Expand Down Expand Up @@ -924,8 +940,7 @@
encryptor,
getMnemonic: getPrimaryKeyringMnemonic.bind(this),
getFeatureFlags: () => ({
disableSnaps:
store.getState().settings.basicFunctionalityEnabled === false,
disableSnaps: !isBasicFunctionalityToggleEnabled(),
}),
});

Expand Down Expand Up @@ -1126,7 +1141,7 @@

return Boolean(
hasProperty(showIncomingTransactions, currentChainId) &&
showIncomingTransactions?.[currentHexChainId],
showIncomingTransactions?.[currentHexChainId],
);
},
updateTransactions: true,
Expand Down Expand Up @@ -1383,6 +1398,7 @@
GasFeeController: gasFeeController,
ApprovalController: approvalController,
PermissionController: permissionController,
RemoteFeatureFlagController: remoteFeatureFlagController,
SelectedNetworkController: selectedNetworkController,
SignatureController: new SignatureController({
messenger: this.controllerMessenger.getRestricted({
Expand Down Expand Up @@ -1484,7 +1500,7 @@
(state: NetworkState) => {
if (
state.networksMetadata[state.selectedNetworkClientId].status ===
NetworkStatus.Available &&
NetworkStatus.Available &&
networkController.getNetworkClientById(
networkController?.state.selectedNetworkClientId,
).configuration.chainId !== currentChainId
Expand All @@ -1509,10 +1525,9 @@
} catch (error) {
console.error(
error,
`Network ID not changed, current chainId: ${
networkController.getNetworkClientById(
networkController?.state.selectedNetworkClientId,
).configuration.chainId
`Network ID not changed, current chainId: ${networkController.getNetworkClientById(
networkController?.state.selectedNetworkClientId,
).configuration.chainId
}`,
);
}
Expand Down Expand Up @@ -1714,7 +1729,7 @@
const decimalsToShow = (currentCurrency === 'usd' && 2) || undefined;
if (
accountsByChainId?.[toHexadecimal(chainId)]?.[
selectSelectedInternalAccountChecksummedAddress
selectSelectedInternalAccountChecksummedAddress
]
) {
const balanceBN = hexToBN(
Expand Down Expand Up @@ -1751,7 +1766,7 @@

const tokenBalances =
allTokenBalances?.[selectedInternalAccount.address as Hex]?.[
chainId
chainId
] ?? {};
tokens.forEach(
(item: { address: string; balance?: string; decimals: number }) => {
Expand All @@ -1762,9 +1777,9 @@
item.balance ||
(item.address in tokenBalances
? renderFromTokenMinimalUnit(
tokenBalances[item.address as Hex],
item.decimals,
)
tokenBalances[item.address as Hex],
item.decimals,
)
: undefined);
const tokenBalanceFiat = balanceToFiatNumber(
// TODO: Fix this by handling or eliminating the undefined case
Expand Down Expand Up @@ -1847,12 +1862,12 @@
const { tokenBalances } = backgroundState.TokenBalancesController;

let tokenFound = false;
tokenLoop: for (const chains of Object.values(tokenBalances)) {

Check warning on line 1865 in app/core/Engine/Engine.ts

View workflow job for this annotation

GitHub Actions / scripts (lint)

Unexpected labeled statement
for (const tokens of Object.values(chains)) {
for (const balance of Object.values(tokens)) {
if (!isZero(balance)) {
tokenFound = true;
break tokenLoop;

Check warning on line 1870 in app/core/Engine/Engine.ts

View workflow job for this annotation

GitHub Actions / scripts (lint)

Unexpected label in break statement
}
}
}
Expand Down Expand Up @@ -2057,6 +2072,7 @@
NetworkController,
PreferencesController,
PhishingController,
RemoteFeatureFlagController,
PPOMController,
TokenBalancesController,
TokenRatesController,
Expand Down Expand Up @@ -2102,6 +2118,7 @@
KeyringController,
NetworkController,
PhishingController,
RemoteFeatureFlagController,
PPOMController,
PreferencesController,
TokenBalancesController,
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed on f5b754f

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe better to name this directory platform instead to preserve the team grouping, so that we can include other controllers here too in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm concerned about the potential tech debt we add when the team's name-based namings enter the codebase.

See another discussion around this topic.

Copy link
Member

@Gudahtt Gudahtt Nov 29, 2024

Choose a reason for hiding this comment

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

Well, we could think of it as "feature areas" instead perhaps, which roughly correlate to teams but are more long-lasting.

The purpose (as I understood it) of moving controller-related code to dedicated files was that it allowed us to assign more appropriate code owners (i.e. teams) and reduce code review burden and regression risk that comes from modifying core modules like Engine.ts.

But the problem is that there are very few changes that affect just a controller. If we co-located multiple controllers owned by the same team, it would be more effective in minimizing cross-team impact/reviews for changes to how controllers interact with each other. If each controller is separate, then their interactions would be via Engine.ts still so we don't get that benefit.

Single-controller directories don't seem helpful to me in reducing review burden or regression risk.

(Note: still not a blocker for me, just explaining my reasoning)

Copy link
Contributor Author

@joaoloureirop joaoloureirop Nov 29, 2024

Choose a reason for hiding this comment

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

I recognize the benefits of organizing the codebase in a feature-based structure. It helps with ownership and better outlines the relationship between components.

Namings that carry meaning for the codebase / product are long-lasting and do not need to be revisited if there's any maintainer's restructuring.

Responsibility areas can then be attributed to each team.

I'll leave it as is for now and follow up with an update when there's a decision on feature area names.

Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { createRemoteFeatureFlagController } from './utils';

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { RemoteFeatureFlagControllerMessenger, RemoteFeatureFlagControllerState } from '@metamask/remote-feature-flag-controller';

export interface RemoteFeatureFlagInitParamTypes {
state?: RemoteFeatureFlagControllerState;
messenger: RemoteFeatureFlagControllerMessenger,
disabled: boolean
}

Loading
Loading