Skip to content

Commit beeab74

Browse files
authored
feat!: track analytics for OTA Snap updates (#4049)
## Changes - Emit `SnapController:snapUpdated` in `#handleRegistryUpdate()` after each successful OTA update. As a side effect, this also fixes missing notifications to `CronjobController` (cronjob re-registration) and `WebSocketService` (connection teardown) on OTA updates. - Add `ota: boolean` to the `SnapControllerSnapUpdatedEvent` payload to distinguish OTA updates from user-initiated ones. - Update the `snapUpdated` analytics subscriber to track the `Snap Updated` event for OTA updates (previously all preinstalled Snap updates were silently skipped). - Add `ota`, `client_version`, and `client_type` properties to the `Snap Updated` analytics event. - Add `clientConfig: ClientConfig` to `SnapControllerArgs` (same shape as `SnapRegistryControllerArgs`) to give the controller access to the client type and version. ## Breaking changes - The `SnapController` constructor now requires a `clientConfig` parameter. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Breaking constructor and messenger event contract changes require host app updates; analytics and lifecycle side effects on OTA updates are new behavior for preinstalled snaps. > > **Overview** > **`SnapController:snapUpdated`** gains a fifth payload field **`ota`** so listeners can tell over-the-air registry updates apart from user-driven ones. After a successful preinstalled snap OTA in **`#handleRegistryUpdate`**, the controller now **publishes** that event with **`ota: true`** (and **`preinstalled: true`**), which also drives **cronjob re-registration** and **WebSocket teardown** that previously did not run on OTA. > > The **`Snap Updated`** analytics handler no longer skips every preinstalled update: it still skips when **`preinstalled && !ota`**, but **records OTA** with **`ota: true`**. All tracked **`Snap Updated`** events now include **`ota`**, **`client_version`**, and **`client_type`** from a new required constructor arg **`clientConfig`** (same **`ClientConfig`** shape as the registry controller). > > **Breaking:** **`SnapController`** must be constructed with **`clientConfig`**, and any code that publishes or asserts on **`snapUpdated`** must pass the **`ota`** boolean. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7ea3aad. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 4755cab commit beeab74

6 files changed

Lines changed: 127 additions & 11 deletions

File tree

packages/snaps-controllers/src/cronjob/CronjobController.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -779,6 +779,7 @@ describe('CronjobController', () => {
779779
snapInfo.version,
780780
MOCK_ORIGIN,
781781
false,
782+
false,
782783
);
783784

784785
expect(cronjobController.state.events).toStrictEqual({

packages/snaps-controllers/src/snaps/SnapController.test.tsx

Lines changed: 89 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6651,6 +6651,7 @@ describe('SnapController', () => {
66516651
'1.0.0',
66526652
METAMASK_ORIGIN,
66536653
true,
6654+
false,
66546655
);
66556656

66566657
const result = await snapController.handleRequest({
@@ -9327,6 +9328,7 @@ describe('SnapController', () => {
93279328
'1.0.0',
93289329
MOCK_ORIGIN,
93299330
false,
9331+
false,
93309332
);
93319333

93329334
controller.destroy();
@@ -10537,23 +10539,25 @@ describe('SnapController', () => {
1053710539
'0.9.0',
1053810540
MOCK_ORIGIN,
1053910541
false,
10542+
false,
1054010543
);
1054110544

1054210545
expect(options.messenger.call).toHaveBeenCalledWith(
1054310546
'AnalyticsController:trackEvent',
1054410547
{
1054510548
name: 'Snap Updated',
10549+
/* eslint-disable @typescript-eslint/naming-convention */
1054610550
properties: {
10547-
// eslint-disable-next-line @typescript-eslint/naming-convention
1054810551
snap_id: MOCK_SNAP_ID,
10549-
// eslint-disable-next-line @typescript-eslint/naming-convention
1055010552
old_version: '0.9.0',
10551-
// eslint-disable-next-line @typescript-eslint/naming-convention
1055210553
new_version: '1.0.0',
1055310554
origin: MOCK_ORIGIN,
10554-
// eslint-disable-next-line @typescript-eslint/naming-convention
1055510555
snap_category: null,
10556+
ota: false,
10557+
client_version: '1.0.0',
10558+
client_type: 'extension',
1055610559
},
10560+
/* eslint-enable @typescript-eslint/naming-convention */
1055710561
sensitiveProperties: {},
1055810562
saveDataRecording: false,
1055910563
hasProperties: true,
@@ -10578,6 +10582,7 @@ describe('SnapController', () => {
1057810582
'0.9.0',
1057910583
MOCK_ORIGIN,
1058010584
true,
10585+
false,
1058110586
);
1058210587

1058310588
expect(options.messenger.call).not.toHaveBeenCalledWith(
@@ -11330,6 +11335,84 @@ describe('SnapController', () => {
1133011335
snapController.destroy();
1133111336
});
1133211337

11338+
it('tracks `Snap Updated` with `ota: true` when a preinstalled Snap is OTA-updated', async () => {
11339+
const rootMessenger = getRootMessenger();
11340+
const registry = new MockSnapRegistryController(rootMessenger);
11341+
11342+
rootMessenger.registerActionHandler(
11343+
'PermissionController:getPermissions',
11344+
() => ({
11345+
[SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION,
11346+
[SnapEndowments.LifecycleHooks]: MOCK_LIFECYCLE_HOOKS_PERMISSION,
11347+
}),
11348+
);
11349+
11350+
const snapId = 'npm:@metamask/jsx-example-snap' as SnapId;
11351+
11352+
const mockSnap = getPersistedSnapObject({
11353+
id: snapId,
11354+
preinstalled: true,
11355+
});
11356+
11357+
const updateVersion = '1.2.1';
11358+
11359+
registry.resolveVersion.mockResolvedValue(updateVersion);
11360+
const fetchFunction = jest.fn().mockResolvedValueOnce({
11361+
// eslint-disable-next-line no-restricted-globals
11362+
headers: new Headers({ 'content-length': '5477' }),
11363+
ok: true,
11364+
body: Readable.toWeb(
11365+
createReadStream(
11366+
path.resolve(
11367+
__dirname,
11368+
`../../test/fixtures/metamask-jsx-example-snap-${updateVersion}.tgz`,
11369+
),
11370+
),
11371+
),
11372+
});
11373+
11374+
const options = getSnapControllerOptions({
11375+
rootMessenger,
11376+
state: {
11377+
snaps: getPersistedSnapsState(mockSnap),
11378+
},
11379+
fetchFunction,
11380+
featureFlags: {
11381+
autoUpdatePreinstalledSnaps: true,
11382+
},
11383+
});
11384+
11385+
const snapController = await getSnapController(options);
11386+
11387+
await snapController.updateRegistry();
11388+
await waitForStateChange(options.messenger);
11389+
await sleep(100);
11390+
11391+
expect(options.messenger.call).toHaveBeenCalledWith(
11392+
'AnalyticsController:trackEvent',
11393+
{
11394+
name: 'Snap Updated',
11395+
/* eslint-disable @typescript-eslint/naming-convention */
11396+
properties: {
11397+
snap_id: snapId,
11398+
old_version: mockSnap.version,
11399+
new_version: updateVersion,
11400+
origin: METAMASK_ORIGIN,
11401+
snap_category: null,
11402+
ota: true,
11403+
client_version: '1.0.0',
11404+
client_type: 'extension',
11405+
},
11406+
/* eslint-enable @typescript-eslint/naming-convention */
11407+
sensitiveProperties: {},
11408+
saveDataRecording: false,
11409+
hasProperties: true,
11410+
},
11411+
);
11412+
11413+
snapController.destroy();
11414+
});
11415+
1133311416
it('retries updating preinstalled Snaps', async () => {
1133411417
const rootMessenger = getRootMessenger();
1133511418
const registry = new MockSnapRegistryController(rootMessenger);
@@ -13437,6 +13520,7 @@ describe('SnapController', () => {
1343713520
'0.9.0',
1343813521
MOCK_ORIGIN,
1343913522
false,
13523+
false,
1344013524
);
1344113525

1344213526
await new Promise((resolve) => setTimeout(resolve, 10));
@@ -13544,6 +13628,7 @@ describe('SnapController', () => {
1354413628
'0.9.0',
1354513629
MOCK_ORIGIN,
1354613630
false,
13631+
false,
1354713632
);
1354813633
await new Promise((resolve) => setTimeout(resolve, 10));
1354913634

packages/snaps-controllers/src/snaps/SnapController.ts

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@ import {
162162
import type { SnapLocation } from './location';
163163
import { detectSnapLocation } from './location';
164164
import type {
165+
ClientConfig,
165166
SnapRegistryControllerGetAction,
166167
SnapRegistryControllerGetMetadataAction,
167168
SnapRegistryControllerResolveVersionAction,
@@ -460,6 +461,7 @@ export type SnapControllerSnapUpdatedEvent = {
460461
oldVersion: string,
461462
origin: string,
462463
preinstalled: boolean,
464+
ota: boolean,
463465
];
464466
};
465467

@@ -699,6 +701,11 @@ export type SnapControllerArgs = {
699701
* @returns A promise that resolves when onboarding is complete.
700702
*/
701703
ensureOnboardingComplete: () => Promise<void>;
704+
705+
/**
706+
* The client configuration, containing the client type and version.
707+
*/
708+
clientConfig: ClientConfig;
702709
};
703710

704711
type AddSnapArgs = {
@@ -787,6 +794,8 @@ export class SnapController extends BaseController<
787794

788795
readonly #clientCryptography: CryptographicFunctions | undefined;
789796

797+
readonly #clientConfig: ClientConfig;
798+
790799
readonly #detectSnapLocation: typeof detectSnapLocation;
791800

792801
readonly #snapsRuntimeData: Map<SnapId, SnapRuntimeData>;
@@ -831,6 +840,7 @@ export class SnapController extends BaseController<
831840
getFeatureFlags = () => ({}),
832841
clientCryptography,
833842
ensureOnboardingComplete,
843+
clientConfig,
834844
}: SnapControllerArgs) {
835845
super({
836846
messenger,
@@ -911,6 +921,7 @@ export class SnapController extends BaseController<
911921
this.#getMnemonicSeed = getMnemonicSeed;
912922
this.#getFeatureFlags = getFeatureFlags;
913923
this.#clientCryptography = clientCryptography;
924+
this.#clientConfig = clientConfig;
914925
this.#preinstalledSnaps = preinstalledSnaps;
915926
this._onUnhandledSnapError = this._onUnhandledSnapError.bind(this);
916927
this._onOutboundRequest = this._onOutboundRequest.bind(this);
@@ -978,7 +989,7 @@ export class SnapController extends BaseController<
978989

979990
this.messenger.subscribe(
980991
'SnapController:snapUpdated',
981-
(snap, oldVersion, origin, preinstalled) => {
992+
(snap, oldVersion, origin, preinstalled, ota) => {
982993
this.#callLifecycleHook(origin, snap.id, HandlerType.OnUpdate).catch(
983994
(error) => {
984995
logError(
@@ -989,7 +1000,7 @@ export class SnapController extends BaseController<
9891000
},
9901001
);
9911002

992-
if (preinstalled) {
1003+
if (preinstalled && !ota) {
9931004
return;
9941005
}
9951006

@@ -999,17 +1010,18 @@ export class SnapController extends BaseController<
9991010
);
10001011
this.messenger.call('AnalyticsController:trackEvent', {
10011012
name: 'Snap Updated',
1013+
/* eslint-disable @typescript-eslint/naming-convention */
10021014
properties: {
1003-
// eslint-disable-next-line @typescript-eslint/naming-convention
10041015
snap_id: snap.id,
1005-
// eslint-disable-next-line @typescript-eslint/naming-convention
10061016
old_version: oldVersion,
1007-
// eslint-disable-next-line @typescript-eslint/naming-convention
10081017
new_version: snap.version,
10091018
origin,
1010-
// eslint-disable-next-line @typescript-eslint/naming-convention
10111019
snap_category: snapMetadata?.category ?? null,
1020+
ota,
1021+
client_version: this.#clientConfig.version,
1022+
client_type: this.#clientConfig.type,
10121023
},
1024+
/* eslint-enable @typescript-eslint/naming-convention */
10131025
sensitiveProperties: {},
10141026
saveDataRecording: false,
10151027
hasProperties: true,
@@ -1485,6 +1497,7 @@ export class SnapController extends BaseController<
14851497
existingSnap.version,
14861498
METAMASK_ORIGIN,
14871499
true,
1500+
false,
14881501
);
14891502
} else if (!isMissingSource) {
14901503
this.messenger.publish(
@@ -1641,6 +1654,7 @@ export class SnapController extends BaseController<
16411654
resolvedVersion !== preinstalledVersionRange &&
16421655
gtVersion(resolvedVersion as unknown as SemVerVersion, snap.version)
16431656
) {
1657+
const oldVersion = snap.version;
16441658
const location = this.#detectSnapLocation(snap.id, {
16451659
versionRange: resolvedVersion,
16461660
fetch: this.#fetchFunction,
@@ -1655,6 +1669,15 @@ export class SnapController extends BaseController<
16551669
versionRange: resolvedVersion,
16561670
automaticUpdate: true,
16571671
});
1672+
1673+
this.messenger.publish(
1674+
'SnapController:snapUpdated',
1675+
this.#getTruncatedSnapExpect(snap.id),
1676+
oldVersion,
1677+
ORIGIN_METAMASK,
1678+
true,
1679+
true,
1680+
);
16581681
}
16591682
}),
16601683
);
@@ -2836,6 +2859,7 @@ export class SnapController extends BaseController<
28362859
oldVersion,
28372860
origin,
28382861
false,
2862+
false,
28392863
),
28402864
);
28412865

packages/snaps-controllers/src/snaps/registry/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
export type {
2+
ClientConfig,
23
SnapRegistryControllerActions,
34
SnapRegistryControllerEvents,
45
SnapRegistryControllerArgs,

packages/snaps-controllers/src/test-utils/controller.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ import {
4949
InMemoryStorageAdapter,
5050
StorageService,
5151
} from '@metamask/storage-service';
52-
import type { Json } from '@metamask/utils';
52+
import type { Json, SemVerVersion } from '@metamask/utils';
5353

5454
import { MOCK_CRONJOB_PERMISSION } from './cronjob';
5555
import { getNodeEES, getNodeEESMessenger } from './execution-environment';
@@ -607,6 +607,10 @@ export const getSnapControllerOptions = ({
607607
clientCryptography: {},
608608
encryptor: getSnapControllerEncryptor(),
609609
ensureOnboardingComplete: jest.fn().mockResolvedValue(undefined),
610+
clientConfig: {
611+
type: 'extension',
612+
version: '1.0.0' as SemVerVersion,
613+
},
610614
...opts,
611615
} as SnapControllerConstructorParamsWithStorage & {
612616
rootMessenger: ReturnType<typeof getRootMessenger>;

packages/snaps-controllers/src/websocket/WebSocketService.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,7 @@ describe('WebSocketService', () => {
488488
'1.0.0',
489489
MOCK_ORIGIN,
490490
false,
491+
false,
491492
);
492493

493494
expect(

0 commit comments

Comments
 (0)