feat: implement remote feature flag controller #12427
+429
−221
There are no files selected for viewing
Validating CODEOWNERS rules …
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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, | ||||||
|
|
@@ -79,6 +80,7 @@ | |||||
| LedgerMobileBridge, | ||||||
| LedgerTransportMiddleware, | ||||||
| } from '@metamask/eth-ledger-bridge-keyring'; | ||||||
| import RemoteFeatureFlagController from './controllers/RemoteFeatureFlagController'; | ||||||
| import { Encryptor, LEGACY_DERIVATION_OPTIONS } from '../Encryptor'; | ||||||
| import { | ||||||
| isMainnetByChainId, | ||||||
|
|
@@ -266,6 +268,10 @@ | |||||
| ) { | ||||||
| this.controllerMessenger = new ExtendedControllerMessenger(); | ||||||
|
|
||||||
| // Basic Functionality toggle defaults to true | ||||||
| const getBasicFunctionalityToggleState = () => | ||||||
|
There was a problem hiding this comment. S: should we use a selector here? |
||||||
| store.getState().settings?.basicFunctionalityEnabled ?? true; | ||||||
|
|
||||||
| const approvalController = new ApprovalController({ | ||||||
| messenger: this.controllerMessenger.getRestricted({ | ||||||
| name: 'ApprovalController', | ||||||
|
|
@@ -478,6 +484,13 @@ | |||||
| 'https://gas.api.cx.metamask.io/networks/<chain_id>/suggestedGasFees', | ||||||
| }); | ||||||
|
|
||||||
| const remoteFeatureFlagController = RemoteFeatureFlagController.init({ | ||||||
| initialState, | ||||||
| controllerMessenger: this.controllerMessenger, | ||||||
| fetchFunction, | ||||||
| disabled: getBasicFunctionalityToggleState() === false | ||||||
|
There was a problem hiding this comment. Q: why testing the exact boolean false equality here?
Suggested change
and same on line 949 |
||||||
| }); | ||||||
|
|
||||||
| const phishingController = new PhishingController({ | ||||||
| messenger: this.controllerMessenger.getRestricted({ | ||||||
| name: 'PhishingController', | ||||||
|
|
@@ -933,8 +946,7 @@ | |||||
| encryptor, | ||||||
| getMnemonic: getPrimaryKeyringMnemonic.bind(this), | ||||||
| getFeatureFlags: () => ({ | ||||||
| disableSnaps: getBasicFunctionalityToggleState() === false, | ||||||
| }), | ||||||
| }); | ||||||
|
|
||||||
|
|
@@ -1135,7 +1147,7 @@ | |||||
|
|
||||||
| return Boolean( | ||||||
| hasProperty(showIncomingTransactions, currentChainId) && | ||||||
| showIncomingTransactions?.[currentHexChainId], | ||||||
| ); | ||||||
| }, | ||||||
| updateTransactions: true, | ||||||
|
|
@@ -1387,6 +1399,7 @@ | |||||
| GasFeeController: gasFeeController, | ||||||
| ApprovalController: approvalController, | ||||||
| PermissionController: permissionController, | ||||||
| RemoteFeatureFlagController: remoteFeatureFlagController, | ||||||
| SelectedNetworkController: selectedNetworkController, | ||||||
| SignatureController: new SignatureController({ | ||||||
| messenger: this.controllerMessenger.getRestricted({ | ||||||
|
|
@@ -1466,9 +1479,9 @@ | |||||
|
|
||||||
| this.datamodel = new ComposableController( | ||||||
| // @ts-expect-error TODO: Filter out non-controller instances | ||||||
| controllers, | ||||||
|
Check failure on line 1482 in app/core/Engine/Engine.ts
|
||||||
| this.controllerMessenger, | ||||||
| ); | ||||||
|
|
||||||
| const { NftController: nfts } = this.context; | ||||||
|
|
||||||
|
|
@@ -1488,7 +1501,7 @@ | |||||
| (state: NetworkState) => { | ||||||
| if ( | ||||||
| state.networksMetadata[state.selectedNetworkClientId].status === | ||||||
| NetworkStatus.Available && | ||||||
| networkController.getNetworkClientById( | ||||||
| networkController?.state.selectedNetworkClientId, | ||||||
| ).configuration.chainId !== currentChainId | ||||||
|
|
@@ -1513,10 +1526,9 @@ | |||||
| } catch (error) { | ||||||
| console.error( | ||||||
| error, | ||||||
| `Network ID not changed, current chainId: ${networkController.getNetworkClientById( | ||||||
| networkController?.state.selectedNetworkClientId, | ||||||
| ).configuration.chainId | ||||||
| }`, | ||||||
| ); | ||||||
| } | ||||||
|
|
@@ -1718,7 +1730,7 @@ | |||||
| const decimalsToShow = (currentCurrency === 'usd' && 2) || undefined; | ||||||
| if ( | ||||||
| accountsByChainId?.[toHexadecimal(chainId)]?.[ | ||||||
| selectSelectedInternalAccountChecksummedAddress | ||||||
| ] | ||||||
| ) { | ||||||
| const balanceBN = hexToBN( | ||||||
|
|
@@ -1755,7 +1767,7 @@ | |||||
|
|
||||||
| const tokenBalances = | ||||||
| allTokenBalances?.[selectedInternalAccount.address as Hex]?.[ | ||||||
| chainId | ||||||
| ] ?? {}; | ||||||
| tokens.forEach( | ||||||
| (item: { address: string; balance?: string; decimals: number }) => { | ||||||
|
|
@@ -1766,9 +1778,9 @@ | |||||
| item.balance || | ||||||
| (item.address in tokenBalances | ||||||
| ? renderFromTokenMinimalUnit( | ||||||
| tokenBalances[item.address as Hex], | ||||||
| item.decimals, | ||||||
| ) | ||||||
| : undefined); | ||||||
| const tokenBalanceFiat = balanceToFiatNumber( | ||||||
| // TODO: Fix this by handling or eliminating the undefined case | ||||||
|
|
@@ -2061,6 +2073,7 @@ | |||||
| NetworkController, | ||||||
| PreferencesController, | ||||||
| PhishingController, | ||||||
| RemoteFeatureFlagController, | ||||||
| PPOMController, | ||||||
| TokenBalancesController, | ||||||
| TokenRatesController, | ||||||
|
|
@@ -2106,6 +2119,7 @@ | |||||
| KeyringController, | ||||||
| NetworkController, | ||||||
| PhishingController, | ||||||
| RemoteFeatureFlagController, | ||||||
| PPOMController, | ||||||
| PreferencesController, | ||||||
| TokenBalancesController, | ||||||
|
|
||||||
|
There was a problem hiding this comment. To keep things consistent, let's follow the pattern that resembles that of what is currently in There was a problem hiding this comment. 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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||
| import { | ||||||
| RemoteFeatureFlagController, | ||||||
| ClientConfigApiService, | ||||||
| ClientType, | ||||||
| } from '@metamask/remote-feature-flag-controller'; | ||||||
| import Logger from '../../../../util/Logger'; | ||||||
| import { RemoteFeatureFlagInitParamTypes } from './types'; | ||||||
| import { | ||||||
| getFeatureFlagAppEnvironment, | ||||||
| getFeatureFlagAppDistribution | ||||||
| } from './utils'; | ||||||
|
|
||||||
|
|
||||||
| const init = ({ | ||||||
|
There was a problem hiding this comment. add remote feature flag controller There was a problem hiding this comment. Thanks. Yes unit tests please! There was a problem hiding this comment. Rename to
Suggested change
|
||||||
| initialState, | ||||||
| controllerMessenger, | ||||||
| fetchFunction, | ||||||
| disabled, | ||||||
| }: RemoteFeatureFlagInitParamTypes) => { | ||||||
|
|
||||||
| const remoteFeatureFlagController = new RemoteFeatureFlagController({ | ||||||
| messenger: controllerMessenger.getRestricted({ | ||||||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. Discussed with Mark a few days ago - Here's the PR that updated the accounts controller side - #12416 |
||||||
| name: 'RemoteFeatureFlagController', | ||||||
| allowedActions: [], | ||||||
| allowedEvents: [], | ||||||
| }), | ||||||
| state: { | ||||||
| ...initialState.RemoteFeatureFlagController | ||||||
|
There was a problem hiding this comment. No need to spread in an object. Just assign the value. Also pass |
||||||
| }, | ||||||
| disabled, | ||||||
| clientConfigApiService: new ClientConfigApiService({ | ||||||
| fetch: fetchFunction, | ||||||
| config: { | ||||||
| client: ClientType.Mobile, | ||||||
| environment: getFeatureFlagAppEnvironment(), | ||||||
| distribution: getFeatureFlagAppDistribution(), | ||||||
| }, | ||||||
| }), | ||||||
| }); | ||||||
|
|
||||||
| if (disabled) { | ||||||
| Logger.log('Feature Flags Controller disabled'); | ||||||
| } else { | ||||||
| remoteFeatureFlagController.getRemoteFeatureFlags().then((featFlags) => { | ||||||
| Logger.log(`Received feature flags ${JSON.stringify(featFlags)}`); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| return remoteFeatureFlagController; | ||||||
| } | ||||||
|
|
||||||
| export default { | ||||||
| init | ||||||
| } | ||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,9 @@ | ||||||
| import { ControllerMessenger, EngineState } from '../../types'; | ||||||
|
|
||||||
| export interface RemoteFeatureFlagInitParamTypes { | ||||||
| initialState: Partial<EngineState>; | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| controllerMessenger: ControllerMessenger, | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| fetchFunction: typeof fetch, | ||||||
| disabled: boolean | ||||||
| } | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { | ||
| DistributionType, | ||
| EnvironmentType, | ||
| } from '@metamask/remote-feature-flag-controller'; | ||
|
|
||
| export const getFeatureFlagAppEnvironment = () => { | ||
| const env = process.env.METAMASK_ENVIRONMENT; | ||
| switch (env) { | ||
| case 'local': return EnvironmentType.Development; | ||
| case 'pre-release': return EnvironmentType.ReleaseCandidate; | ||
| case 'production': return EnvironmentType.Production; | ||
| default: return EnvironmentType.Development; | ||
| } | ||
| }; | ||
|
|
||
| export const getFeatureFlagAppDistribution = () => { | ||
| const dist = process.env.METAMASK_BUILD_TYPE; | ||
| switch (dist) { | ||
| case 'main': return DistributionType.Main; | ||
| case 'flask': return DistributionType.Flask; | ||
| default: return DistributionType.Main; | ||
| } | ||
| }; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: maybe you can make the team own the whole app/selectors/featureFlagController directory instead of individual files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want
app/selectors/featureFlagController/<featureFlagName>to be owned by other teamsI'll update the
CODEOWNERSsyntax to match files likefeatureFlagController/index.tsbut no further nested filesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed on b784e89