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.

Sign up for GitHub

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(3742): Feature Flag Values with Scope Based on threshold #5051

Merged
merged 5 commits into from
Dec 12, 2024
Merged

feat(3742): Feature Flag Values with Scope Based on threshold #5051

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2122627
feat(3742): Feature Flag Values with Scope Based on threshold
DDDDDanica Dec 10, 2024
f2bc3bf
feat(3744): use UUIDV4 as callback of metricsId
DDDDDanica Dec 11, 2024
93d8351
feat(3742): Modify MOCK_METRIC_ID to be uuidv4
DDDDDanica Dec 12, 2024
63a7274
feat(3742): typo fix for generateDeterministicRandomNumber function s…
DDDDDanica Dec 12, 2024
1f94695
feat(3742): replace metaMetricId to a promise getMetaMetricId
DDDDDanica Dec 12, 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
4 changes: 2 additions & 2 deletions packages/remote-feature-flag-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@
"dependencies": {
"@metamask/base-controller": "^7.0.2",
"@metamask/utils": "^10.0.0",
"@noble/hashes": "^1.4.0",
"cockatiel": "^3.1.2"
"cockatiel": "^3.1.2",
"uuid": "^8.3.2"
},
"devDependencies": {
"@lavamoat/allow-scripts": "^3.0.4",
Expand Down
11 changes: 4 additions & 7 deletions packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const MOCK_FLAGS_WITH_THRESHOLD = {
],
};

const MOCK_METRICS_ID = '0x1234567890abcdef';
const MOCK_METRICS_ID = 'f9e8d7c6-b5a4-3210-9876-543210fedcba';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a valid UUIDv4, this is UUIDv3

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 ! addressed 93d8351


/**
* Creates a controller instance with default parameters for testing
Expand Down Expand Up @@ -72,12 +72,6 @@ function createController(
}

describe('RemoteFeatureFlagController', () => {
beforeEach(() => {
jest
.spyOn(userSegmentationUtils, 'generateFallbackMetaMetricsId')
.mockReturnValue(MOCK_METRICS_ID);
});

describe('constructor', () => {
it('initializes with default state', () => {
const controller = createController();
Expand Down Expand Up @@ -304,6 +298,9 @@ describe('RemoteFeatureFlagController', () => {
});

it('uses a fallback metaMetricsId if none is provided', async () => {
jest
.spyOn(userSegmentationUtils, 'generateFallbackMetaMetricsId')
.mockReturnValue(MOCK_METRICS_ID);
const clientConfigApiService = buildClientConfigApiService({
remoteFeatureFlags: MOCK_FLAGS_WITH_THRESHOLD,
});
Expand Down
33 changes: 8 additions & 25 deletions packages/remote-feature-flag-controller/src/utils/user-segmentation-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { webcrypto } from 'crypto';
import { validate as uuidValidate, version as uuidVersion } from 'uuid';

import {
generateDeterministicRandomNumber,
Expand All @@ -7,10 +7,10 @@ import {
} from './user-segmentation-utils';

const MOCK_METRICS_IDS = [
'0x1234567890abcdef',
NicolasMassart marked this conversation as resolved.
Show resolved Hide resolved
'0xdeadbeefdeadbeef',
'0xabcdef0123456789',
'0xfedcba9876543210',
'123e4567-e89b-12d3-a456-426614174000',
'987fcdeb-51a2-3c4b-9876-543210fedcba',
'a1b2c3d4-e5f6-7890-abcd-ef1234567890',
'f9e8d7c6-b5a4-3210-9876-543210fedcba',
];
Copy link
Contributor

Choose a reason for hiding this comment

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

these are not valid UUIDv4.


const MOCK_FEATURE_FLAGS = {
Expand Down Expand Up @@ -77,27 +77,10 @@ describe('user-segmentation-utils', () => {
});

describe('generateFallbackMetaMetricsId', () => {
beforeAll(() => {
// Set up crypto for tests
Object.defineProperty(global, 'crypto', {
value: webcrypto,
writable: true,
configurable: true,
});
});

it('returns a properly formatted hex string', () => {
it('returns a valid uuidv4', () => {
const result = generateFallbackMetaMetricsId();
expect(result.startsWith('0x')).toBe(true);
expect(result).toHaveLength(66);
expect(result.slice(2)).toMatch(/^[0-9a-f]+$/u);
});

it('generates unique values for each revoke', () => {
const result1 = generateFallbackMetaMetricsId();
const result2 = generateFallbackMetaMetricsId();

expect(result1).not.toBe(result2);
expect(uuidValidate(result)).toBe(true);
expect(uuidVersion(result)).toBe(4);
});
});
});
12 changes: 4 additions & 8 deletions packages/remote-feature-flag-controller/src/utils/user-segmentation-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import type { Json } from '@metamask/utils';
import { sha256 } from '@noble/hashes/sha256';
import { bytesToHex } from '@noble/hashes/utils';
import { v4 as uuidV4 } from 'uuid';

import type { FeatureFlagScopeValue } from '../remote-feature-flag-controller-types';

Expand Down Expand Up @@ -42,12 +41,9 @@ export const isFeatureFlagWithScopeValue = (
};

/**
* Generates a SHA-256 hash to use as a fallback metaMetricsId
* @returns A 32-byte hex string prefixed with '0x'
* Generates UUIDv4 as a fallback metaMetricsId
* @returns A UUIDv4 string
*/
export function generateFallbackMetaMetricsId(): string {
Copy link
Member

Choose a reason for hiding this comment

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

This is non-deterministic, and therefore unsuitable for determining whether a feature flag should be enabled or not

Copy link
Member

Choose a reason for hiding this comment

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

I guess the best way of handling this would be to accept a callback for generating the metametrics ID? Ideally we'd use a messenger action, but that would be tricky because of the separate metrics implementations, so a callback should work instead.

Copy link
Member

Choose a reason for hiding this comment

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

Or as you've just pointed out in the extension PR @DDDDDanica , we can rely on getMetaMetricsId to always return a value (i.e. to generate if it's missing). But we'll still want to remove this fallback if we're expecting the callback to always return a value, and we'll want to update the type of the callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed in slack thread, we will remove the callback and add this logic to client side if undefined 🙏🏻

const random = new Uint8Array(32);
crypto.getRandomValues(random);
const hash = sha256(random);
return `0x${bytesToHex(hash)}`;
return uuidV4();
}
2 changes: 1 addition & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3512,7 +3512,6 @@ __metadata:
"@metamask/base-controller": "npm:^7.0.2"
"@metamask/controller-utils": "npm:^11.4.4"
"@metamask/utils": "npm:^10.0.0"
"@noble/hashes": "npm:^1.4.0"
"@types/jest": "npm:^27.4.1"
cockatiel: "npm:^3.1.2"
deepmerge: "npm:^4.2.2"
Expand All @@ -3522,6 +3521,7 @@ __metadata:
typedoc: "npm:^0.24.8"
typedoc-plugin-missing-exports: "npm:^2.0.0"
typescript: "npm:~5.2.2"
uuid: "npm:^8.3.2"
languageName: unknown
linkType: soft

Expand Down
Loading