-
-
Notifications
You must be signed in to change notification settings - Fork 277
Use new assets state in TransactionPayController (behind feature flag) #8163
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
base: main
Are you sure you want to change the base?
Changes from all commits
6e407d3
5b84ccc
1e09ec5
e8ab445
73586b3
3b760ee
3769141
dc87022
199802c
deb5f05
d883110
d8a0c2f
61e0890
20e274c
0f01832
6638faa
9b43494
61aec42
ab40146
fa786f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,8 @@ | |
| "@ethersproject/abi": "^5.7.0", | ||
| "@ethersproject/contracts": "^5.7.0", | ||
| "@ethersproject/providers": "^5.7.0", | ||
| "@metamask/app-metadata-controller": "^2.0.0", | ||
| "@metamask/assets-controller": "^2.3.0", | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assets-controller is the new controller that we are migrating towards and that will hold the state. Once the flag is fully turned on, we will come back here and delete all the old code and the old dependency. |
||
| "@metamask/assets-controllers": "^100.2.1", | ||
| "@metamask/base-controller": "^9.0.0", | ||
| "@metamask/bridge-controller": "^69.1.0", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |
| DEFAULT_RELAY_QUOTE_URL, | ||
| DEFAULT_SLIPPAGE, | ||
| DEFAULT_STRATEGY_ORDER, | ||
| getAssetsUnifyStateFeature, | ||
| getFallbackGas, | ||
| DEFAULT_RELAY_EXECUTE_URL, | ||
| getRelayOriginGasOverhead, | ||
|
|
@@ -38,8 +39,11 @@ const TOKEN_ADDRESS_DIFFERENT_MOCK = '0xdef789abc012' as Hex; | |
| const TOKEN_SPECIFIC_SLIPPAGE_MOCK = 0.02; | ||
|
|
||
| describe('Feature Flags Utils', () => { | ||
| const { messenger, getRemoteFeatureFlagControllerStateMock } = | ||
| getMessengerMock(); | ||
| const { | ||
| messenger, | ||
| getAppMetadataControllerStateMock, | ||
| getRemoteFeatureFlagControllerStateMock, | ||
| } = getMessengerMock(); | ||
|
|
||
| beforeEach(() => { | ||
| jest.resetAllMocks(); | ||
|
|
@@ -562,6 +566,111 @@ describe('Feature Flags Utils', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('getAssetsUnifyStateFeature', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is prime to convert into a test table/it.each 🤞🏾 Example Test Table / `it.each` describe('getAssetsUnifyStateFeature', () => {
const defaultAppMetadata = {
currentAppVersion: '',
previousAppVersion: '',
previousMigrationVersion: 0,
currentMigrationVersion: 0,
};
const failureCases = [
{
description: 'returns false when assetsUnifyState is not set',
remoteFeatureFlags: undefined,
appMetadata: undefined,
},
{
description: 'returns false when assetsUnifyState.enabled is false',
remoteFeatureFlags: {
assetsUnifyState: {
enabled: false,
featureVersion: '1',
minimumVersion: null,
},
},
appMetadata: undefined,
},
{
description:
'returns false when featureVersion does not match expected version',
remoteFeatureFlags: {
assetsUnifyState: {
enabled: true,
featureVersion: '2',
minimumVersion: null,
},
},
appMetadata: undefined,
},
{
description:
'returns false when app version does not satisfy minimumVersion',
remoteFeatureFlags: {
assetsUnifyState: {
enabled: true,
featureVersion: '1',
minimumVersion: '2.0.0',
},
},
appMetadata: {
...defaultAppMetadata,
currentAppVersion: '1.0.0',
},
},
];
const successCases = [
{
description: 'returns true when minimumVersion is null',
remoteFeatureFlags: {
assetsUnifyState: {
enabled: true,
featureVersion: '1',
minimumVersion: null,
},
},
appMetadata: undefined,
},
{
description: 'returns true when app version satisfies minimumVersion',
remoteFeatureFlags: {
assetsUnifyState: {
enabled: true,
featureVersion: '1',
minimumVersion: '1.0.0',
},
},
appMetadata: {
...defaultAppMetadata,
currentAppVersion: '2.0.0',
},
},
];
const arrangeMocks = (
remoteFeatureFlags: FeatureFlags | undefined,
appMetadata: AppMetadataControllerState | undefined,
): void => {
if (remoteFeatureFlags !== undefined) {
getRemoteFeatureFlagControllerStateMock.mockReturnValue({
...getDefaultRemoteFeatureFlagControllerState(),
remoteFeatureFlags,
});
}
if (appMetadata !== undefined) {
getAppMetadataControllerStateMock.mockReturnValue(appMetadata);
}
};
it.each(failureCases)(
'$description',
({ remoteFeatureFlags, appMetadata }: (typeof failureCases)[number]) => {
arrangeMocks(remoteFeatureFlags, appMetadata);
const result = getAssetsUnifyStateFeature(messenger);
expect(result).toBe(false);
},
);
it.each(successCases)(
'$description',
({ remoteFeatureFlags, appMetadata }: (typeof successCases)[number]) => {
arrangeMocks(remoteFeatureFlags, appMetadata);
const result = getAssetsUnifyStateFeature(messenger);
expect(result).toBe(true);
},
);
}); |
||
| it('returns false when assetsUnifyState is not set', () => { | ||
| const result = getAssetsUnifyStateFeature(messenger); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false when assetsUnifyState.enabled is false', () => { | ||
| getRemoteFeatureFlagControllerStateMock.mockReturnValue({ | ||
| ...getDefaultRemoteFeatureFlagControllerState(), | ||
| remoteFeatureFlags: { | ||
| assetsUnifyState: { | ||
| enabled: false, | ||
| featureVersion: '1', | ||
| minimumVersion: null, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const result = getAssetsUnifyStateFeature(messenger); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false when featureVersion does not match expected version', () => { | ||
| getRemoteFeatureFlagControllerStateMock.mockReturnValue({ | ||
| ...getDefaultRemoteFeatureFlagControllerState(), | ||
| remoteFeatureFlags: { | ||
| assetsUnifyState: { | ||
| enabled: true, | ||
| featureVersion: '2', | ||
| minimumVersion: null, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const result = getAssetsUnifyStateFeature(messenger); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true when minimumVersion is null', () => { | ||
| getRemoteFeatureFlagControllerStateMock.mockReturnValue({ | ||
| ...getDefaultRemoteFeatureFlagControllerState(), | ||
| remoteFeatureFlags: { | ||
| assetsUnifyState: { | ||
| enabled: true, | ||
| featureVersion: '1', | ||
| minimumVersion: null, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const result = getAssetsUnifyStateFeature(messenger); | ||
|
|
||
| expect(result).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false when app version does not satisfy minimumVersion', () => { | ||
| getRemoteFeatureFlagControllerStateMock.mockReturnValue({ | ||
| ...getDefaultRemoteFeatureFlagControllerState(), | ||
| remoteFeatureFlags: { | ||
| assetsUnifyState: { | ||
| enabled: true, | ||
| featureVersion: '1', | ||
| minimumVersion: '2.0.0', | ||
| }, | ||
| }, | ||
| }); | ||
| getAppMetadataControllerStateMock.mockReturnValue({ | ||
| currentAppVersion: '1.0.0', | ||
| previousAppVersion: '', | ||
| previousMigrationVersion: 0, | ||
| currentMigrationVersion: 0, | ||
| }); | ||
|
|
||
| const result = getAssetsUnifyStateFeature(messenger); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true when app version satisfies minimumVersion', () => { | ||
| getRemoteFeatureFlagControllerStateMock.mockReturnValue({ | ||
| ...getDefaultRemoteFeatureFlagControllerState(), | ||
| remoteFeatureFlags: { | ||
| assetsUnifyState: { | ||
| enabled: true, | ||
| featureVersion: '1', | ||
| minimumVersion: '1.0.0', | ||
| }, | ||
| }, | ||
| }); | ||
| getAppMetadataControllerStateMock.mockReturnValue({ | ||
| currentAppVersion: '2.0.0', | ||
| previousAppVersion: '', | ||
| previousMigrationVersion: 0, | ||
| currentMigrationVersion: 0, | ||
| }); | ||
|
|
||
| const result = getAssetsUnifyStateFeature(messenger); | ||
|
|
||
| expect(result).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getStrategyOrder', () => { | ||
| it('returns default strategy order when none is set', () => { | ||
| const strategyOrder = getStrategyOrder(messenger); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,10 @@ | ||
| import type { Hex } from '@metamask/utils'; | ||
| import { createModuleLogger } from '@metamask/utils'; | ||
| import { | ||
| createModuleLogger, | ||
| isValidSemVerRange, | ||
| isValidSemVerVersion, | ||
| satisfiesVersionRange, | ||
| } from '@metamask/utils'; | ||
| import { uniq } from 'lodash'; | ||
|
|
||
| import type { TransactionPayControllerMessenger } from '..'; | ||
|
|
@@ -297,6 +302,64 @@ export function getSlippage( | |
| return slippage; | ||
| } | ||
|
|
||
| /** | ||
| * Get the AssetsUnifyState feature flag state. | ||
| * | ||
| * @param messenger - Controller messenger. | ||
| * @returns True if the assets unify state feature is enabled, false otherwise. | ||
| */ | ||
| export function getAssetsUnifyStateFeature( | ||
| messenger: TransactionPayControllerMessenger, | ||
| ): boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QQ if we want to test this , what is the way to turn this to true ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently only controlled by remote feature flags, unsure how much work would be required if we want to force to true.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By doing black magic cjs updates in the client.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With that said, if that cjs change is done with a patch and directly at the feature flag controller, it can be used to test selector changes too. |
||
| const state = messenger.call('RemoteFeatureFlagController:getState'); | ||
| const assetsUnifyState = state.remoteFeatureFlags.assetsUnifyState as | ||
| | { | ||
| enabled: boolean; | ||
| featureVersion: string | null; | ||
| minimumVersion: string | null; | ||
| } | ||
| | undefined; | ||
|
|
||
| if (!assetsUnifyState?.enabled) { | ||
| return false; | ||
| } | ||
|
|
||
| const AssetsUnifyStateFeatureVersion = '1'; | ||
|
|
||
| return ( | ||
| assetsUnifyState.featureVersion === AssetsUnifyStateFeatureVersion && | ||
| hasMinimumRequiredVersion(messenger, assetsUnifyState.minimumVersion) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Check if the app version satisfies the minimum required version. | ||
| * | ||
| * @param messenger - Controller messenger. | ||
| * @param minRequiredVersion - The minimum required version. | ||
| * @returns True if the app version satisfies the minimum required version, false otherwise. | ||
| */ | ||
| function hasMinimumRequiredVersion( | ||
| messenger: TransactionPayControllerMessenger, | ||
| minRequiredVersion: string | null, | ||
| ): boolean { | ||
| if (!minRequiredVersion) { | ||
| return true; | ||
| } | ||
|
|
||
| const appVersion = messenger.call( | ||
| 'AppMetadataController:getState', | ||
| )?.currentAppVersion; | ||
|
|
||
| const semverRange = `>=${minRequiredVersion}`; | ||
|
|
||
| return ( | ||
| isValidSemVerVersion(appVersion) && | ||
| isValidSemVerRange(semverRange) && | ||
| satisfiesVersionRange(appVersion, semverRange) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Get a value from a record using a case-insensitive key lookup. | ||
| * | ||
|
|
||
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.
This dependency is needed for feature flags that have a minimum version requirement, as this package exposes the current app version to do the comparison.