From 6fdb952d223eb8a83d0917141a7bcd378bf3145e Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 26 Oct 2022 14:18:16 +0200 Subject: [PATCH 01/33] Refactor wallet_enable --- packages/controllers/jest.config.js | 8 +- .../src/snaps/SnapController.test.ts | 78 +++++------ .../controllers/src/snaps/SnapController.ts | 129 +++++++++--------- 3 files changed, 102 insertions(+), 113 deletions(-) diff --git a/packages/controllers/jest.config.js b/packages/controllers/jest.config.js index 0daf9d7cc5..51d53b1ae1 100644 --- a/packages/controllers/jest.config.js +++ b/packages/controllers/jest.config.js @@ -8,10 +8,10 @@ module.exports = { coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'], coverageThreshold: { global: { - branches: 85.74, - functions: 95.3, - lines: 94.84, - statements: 94.93, + branches: 85.91, + functions: 95.34, + lines: 94.85, + statements: 94.95, }, }, projects: [ diff --git a/packages/controllers/src/snaps/SnapController.test.ts b/packages/controllers/src/snaps/SnapController.test.ts index e634d353a5..fc8b621e90 100644 --- a/packages/controllers/src/snaps/SnapController.test.ts +++ b/packages/controllers/src/snaps/SnapController.test.ts @@ -15,7 +15,7 @@ import { SnapCaveatType, } from '@metamask/snap-utils'; import { Crypto } from '@peculiar/webcrypto'; -import { EthereumRpcError, ethErrors, serializeError } from 'eth-rpc-errors'; +import { ethErrors } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; @@ -651,13 +651,13 @@ describe('SnapController', () => { it('throws an error on invalid semver range during installSnaps', async () => { const controller = getSnapController(); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: { version: 'foo' }, - }); - - expect(result).toMatchObject({ - [MOCK_SNAP_ID]: { error: expect.any(EthereumRpcError) }, - }); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: 'foo' }, + }), + ).rejects.toThrow( + 'The "version" field must be a valid SemVer version range if specified. Received: "foo"', + ); }); it('reuses an already installed Snap if it satisfies the requested SemVer range', async () => { @@ -721,17 +721,12 @@ describe('SnapController', () => { }; }); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: {}, - }); - - const { code, message } = serializeError( - ethErrors.provider.userRejectedRequest(), - ); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: {}, + }), + ).rejects.toThrow('User rejected the request.'); - expect(result).toStrictEqual({ - [MOCK_SNAP_ID]: { error: expect.objectContaining({ code, message }) }, - }); expect(controller.get(MOCK_SNAP_ID)).toBeUndefined(); }); @@ -783,13 +778,11 @@ describe('SnapController', () => { const expectedSnapObject = getTruncatedSnap(); - expect( - await snapController.installSnaps(MOCK_ORIGIN, { + await expect( + snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }), - ).toStrictEqual({ - [MOCK_SNAP_ID]: { error: serializeError(new Error('foo')) }, - }); + ).rejects.toThrow('foo'); expect(messengerCallMock).toHaveBeenCalledTimes(3); expect(messengerCallMock).toHaveBeenNthCalledWith( @@ -2286,13 +2279,12 @@ describe('SnapController', () => { return false; }); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [snapId]: {}, - }); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [snapId]: {}, + }), + ).rejects.toThrow('Invalid snap id. Unknown prefix. Received: "foo"'); - expect(result).toStrictEqual({ - [snapId]: { error: expect.any(EthereumRpcError) }, - }); expect(callActionMock).toHaveBeenCalledTimes(1); expect(callActionMock).toHaveBeenCalledWith( 'PermissionController:hasPermission', @@ -2435,9 +2427,11 @@ describe('SnapController', () => { getSnapControllerOptions({ messenger }), ); + const manifest = getSnapManifest(); + await controller.add({ id: MOCK_SNAP_ID, - manifest: getSnapManifest(), + manifest, origin: MOCK_ORIGIN, sourceCode: DEFAULT_SNAP_BUNDLE, }); @@ -2458,9 +2452,13 @@ describe('SnapController', () => { sourceCode: DEFAULT_SNAP_BUNDLE, })); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: { version: newVersionRange }, - }); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: newVersionRange }, + }), + ).rejects.toThrow( + `Snap "${MOCK_SNAP_ID}@${manifest.version}" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, + ); expect(callActionMock).toHaveBeenCalledTimes(1); expect(callActionMock).toHaveBeenCalledWith( @@ -2470,9 +2468,6 @@ describe('SnapController', () => { ); expect(fetchSnapMock).toHaveBeenCalledTimes(1); expect(fetchSnapMock).toHaveBeenCalledWith(MOCK_SNAP_ID, newVersionRange); - expect(result).toStrictEqual({ - [MOCK_SNAP_ID]: { error: expect.any(EthereumRpcError) }, - }); }); it('returns an error when a throw happens inside an update', async () => { @@ -2506,9 +2501,11 @@ describe('SnapController', () => { throw new Error('foo'); }); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: { version: newVersionRange }, - }); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: newVersionRange }, + }), + ).rejects.toThrow('foo'); expect(callActionMock).toHaveBeenCalledTimes(1); expect(callActionMock).toHaveBeenCalledWith( @@ -2518,9 +2515,6 @@ describe('SnapController', () => { ); expect(fetchSnapMock).toHaveBeenCalledTimes(1); expect(fetchSnapMock).toHaveBeenCalledWith(MOCK_SNAP_ID, newVersionRange); - expect(result).toStrictEqual({ - [MOCK_SNAP_ID]: { error: expect.anything() }, - }); }); }); diff --git a/packages/controllers/src/snaps/SnapController.ts b/packages/controllers/src/snaps/SnapController.ts index eaffb8a797..4d5b5632ef 100644 --- a/packages/controllers/src/snaps/SnapController.ts +++ b/packages/controllers/src/snaps/SnapController.ts @@ -63,7 +63,7 @@ import { assertExhaustive, } from '@metamask/utils'; import { createMachine, interpret, StateMachine } from '@xstate/fsm'; -import { ethErrors, serializeError } from 'eth-rpc-errors'; +import { ethErrors } from 'eth-rpc-errors'; import type { Patch } from 'immer'; import { nanoid } from 'nanoid'; @@ -1492,44 +1492,56 @@ export class SnapController extends BaseController< ): Promise { const result: InstallSnapsResult = {}; - await Promise.all( - Object.entries(requestedSnaps).map( - async ([snapId, { version: rawVersion }]) => { - const version = resolveVersion(rawVersion); - const permissionName = getSnapPermissionName(snapId); - - if (!isValidSnapVersionRange(version)) { - result[snapId] = { - error: ethErrors.rpc.invalidParams( + const snapIds = Object.keys(requestedSnaps); + + // Existing snaps may need to be updated + const pendingUpdates = snapIds.filter((snapId) => this.has(snapId)); + + // Non-existing snaps will need to be installed + const pendingInstalls = snapIds.filter( + (snapId) => !pendingUpdates.includes(snapId), + ); + + try { + await Promise.all( + Object.entries(requestedSnaps).map( + async ([snapId, { version: rawVersion }]) => { + const version = resolveVersion(rawVersion); + const permissionName = getSnapPermissionName(snapId); + + if (!isValidSnapVersionRange(version)) { + throw ethErrors.rpc.invalidParams( `The "version" field must be a valid SemVer version range if specified. Received: "${version}".`, - ), - }; - return; - } + ); + } + + if ( + !(await this.messagingSystem.call( + 'PermissionController:hasPermission', + origin, + permissionName, + )) + ) { + throw ethErrors.provider.unauthorized( + `Not authorized to install snap "${snapId}". Request the permission for the snap before attempting to install it.`, + ); + } - if ( - await this.messagingSystem.call( - 'PermissionController:hasPermission', + result[snapId] = await this.processRequestedSnap( origin, - permissionName, - ) - ) { - // Attempt to install and run the snap, storing any errors that - // occur during the process. - result[snapId] = { - ...(await this.processRequestedSnap(origin, snapId, version)), - }; - } else { - // only allow the installation of permitted snaps - result[snapId] = { - error: ethErrors.provider.unauthorized( - `Not authorized to install snap "${snapId}". Request the permission for the snap before attempting to install it.`, - ), - }; - } - }, - ), - ); + snapId, + version, + ); + }, + ), + ); + } catch (err) { + const installed = pendingInstalls.filter((snapId) => this.has(snapId)); + this.removeSnaps(installed); + // TODO: Handle update roll backs + + throw err; + } return result; } @@ -1547,15 +1559,7 @@ export class SnapController extends BaseController< snapId: SnapId, versionRange: string, ): Promise { - try { - validateSnapId(snapId); - } catch (err) { - return { - error: ethErrors.rpc.invalidParams( - `"${snapId}" is not a valid snap id.`, - ), - }; - } + validateSnapId(snapId); const existingSnap = this.getTruncated(snapId); // For devX we always re-install local snaps. @@ -1565,30 +1569,21 @@ export class SnapController extends BaseController< } if (this._featureFlags.dappsCanUpdateSnaps === true) { - try { - const updateResult = await this.updateSnap( - origin, - snapId, - versionRange, + const updateResult = await this.updateSnap( + origin, + snapId, + versionRange, + ); + if (updateResult === null) { + throw ethErrors.rpc.invalidParams( + `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, ); - if (updateResult === null) { - return { - error: ethErrors.rpc.invalidParams( - `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, - ), - }; - } - return updateResult; - } catch (err) { - return { error: serializeError(err) }; } - } else { - return { - error: ethErrors.rpc.invalidParams( - `Version mismatch with already installed snap. ${snapId}@${existingSnap.version} doesn't satisfy requested version ${versionRange}`, - ), - }; + return updateResult; } + throw ethErrors.rpc.invalidParams( + `Version mismatch with already installed snap. ${snapId}@${existingSnap.version} doesn't satisfy requested version ${versionRange}`, + ); } // Existing snaps must be stopped before overwriting @@ -1620,7 +1615,7 @@ export class SnapController extends BaseController< this.removeSnap(snapId); } - return { error: serializeError(err) }; + throw err; } } From b597ce5efb18033e350332da984171676a96819a Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Sun, 27 Nov 2022 18:44:34 -0600 Subject: [PATCH 02/33] add initial rollback code --- .../controllers/src/snaps/SnapController.ts | 116 +++++++++++++++++- 1 file changed, 115 insertions(+), 1 deletion(-) diff --git a/packages/controllers/src/snaps/SnapController.ts b/packages/controllers/src/snaps/SnapController.ts index 8673a02016..f6c4b55608 100644 --- a/packages/controllers/src/snaps/SnapController.ts +++ b/packages/controllers/src/snaps/SnapController.ts @@ -9,6 +9,7 @@ import { HasPermission, HasPermissions, PermissionsRequest, + RequestedPermissions, RestrictedControllerMessenger, RevokeAllPermissions, RevokePermissionForAllSubjects, @@ -62,6 +63,7 @@ import { timeSince, assert, assertExhaustive, + NonEmptyArray, } from '@metamask/utils'; import { createMachine, interpret, StateMachine } from '@xstate/fsm'; import { ethErrors } from 'eth-rpc-errors'; @@ -69,6 +71,7 @@ import type { Patch } from 'immer'; import { nanoid } from 'nanoid'; import { caveatMappers } from '@metamask/rpc-methods'; +import { enablePatches } from 'immer'; import { forceStrict, validateMachine } from '../fsm'; import { ExecuteSnapAction, @@ -85,6 +88,7 @@ import { fetchNpmSnap } from './utils'; import { Timer } from './Timer'; +enablePatches(); export const controllerName = 'SnapController'; // TODO: Figure out how to name these @@ -206,6 +210,16 @@ export type PersistedSnapControllerState = SnapControllerState & { snapStates: Record; }; +type RollbackSnapshot = { + statePatches: Patch[]; + sourceCode: string | null; + permissions: { + revoked: unknown; + granted: unknown[]; + requestData: unknown; + }; +}; + // Controller Messenger Actions /** @@ -554,6 +568,7 @@ type SetSnapArgs = Omit & { manifest: SnapManifest; sourceCode: string; svgIcon?: string; + isUpdate?: boolean; }; const defaultState: SnapControllerState = { @@ -614,6 +629,8 @@ export class SnapController extends BaseController< private _snapsRuntimeData: Map; + private _rollbackSnapshots: Map; + private _getAppKey: GetAppKey; private _timeoutForLastRequestStatus?: number; @@ -708,6 +725,7 @@ export class SnapController extends BaseController< this._onOutboundRequest = this._onOutboundRequest.bind(this); this._onOutboundResponse = this._onOutboundResponse.bind(this); this._snapsRuntimeData = new Map(); + this._rollbackSnapshots = new Map(); this._pollForLastRequestStatus(); @@ -1541,6 +1559,7 @@ export class SnapController extends BaseController< async ([snapId, { version: rawVersion }]) => { const version = resolveVersion(rawVersion); const permissionName = getSnapPermissionName(snapId); + const isUpdate = pendingUpdates.includes(snapId); if (!isValidSnapVersionRange(version)) { throw ethErrors.rpc.invalidParams( @@ -1560,6 +1579,15 @@ export class SnapController extends BaseController< ); } + if (isUpdate) { + let rollbackSnapshot = this.getRollbackSnapshot(snapId); + if (rollbackSnapshot === undefined) { + const prevSourceCode = this.getRuntimeExpect(snapId).sourceCode; + rollbackSnapshot = this.createRollbackSnapshot(snapId); + rollbackSnapshot.sourceCode = prevSourceCode; + } + } + result[snapId] = await this.processRequestedSnap( origin, snapId, @@ -1737,8 +1765,14 @@ export class SnapController extends BaseController< manifest: newSnap.manifest, sourceCode: newSnap.sourceCode, versionRange: newVersionRange, + isUpdate: true, }); + // record revokePermissions to later grant for update rollback + // and record approvedNewPermissions to later revoke for update rollback + // when rolling back check if the permissions exist as the update might + // have errored out before the permissions were assigned + const unusedPermissionsKeys = Object.keys(unusedPermissions); if (isNonEmptyArray(unusedPermissionsKeys)) { await this.messagingSystem.call( @@ -1757,6 +1791,16 @@ export class SnapController extends BaseController< }); } + const rollbackSnapshot = this.getRollbackSnapshot( + snapId, + ) as RollbackSnapshot; + rollbackSnapshot.permissions.revoked = unusedPermissions; + rollbackSnapshot.permissions.granted = Object.keys(approvedNewPermissions); + rollbackSnapshot.permissions.requestData = requestData; + + // add a try/catch block here to catch an error if the snap does not start in update situations + // we might need to add a flag that delineates between installs/updates calling this methodor ac + await this._startSnap({ snapId, sourceCode: newSnap.sourceCode }); const truncatedSnap = this.getTruncatedExpect(snapId); @@ -1930,6 +1974,7 @@ export class SnapController extends BaseController< sourceCode, svgIcon, versionRange = DEFAULT_REQUESTED_SNAP_VERSION, + isUpdate = false, } = args; assertIsSnapManifest(manifest); @@ -1991,10 +2036,19 @@ export class SnapController extends BaseController< delete snap.blockInformation; // store the snap back in state - this.update((state: any) => { + const [, , inversePatches] = this.update((state: any) => { state.snaps[snapId] = snap; }); + // checking for isUpdate here as this function is also used in + // the install flow, we do not care to create snapsshots for installs + if (isUpdate) { + const rollbackSnapshot = this.getRollbackSnapshot( + snapId, + ) as RollbackSnapshot; + rollbackSnapshot.statePatches = inversePatches; + } + const runtime = this.getRuntimeExpect(snapId); runtime.sourceCode = sourceCode; @@ -2386,6 +2440,66 @@ export class SnapController extends BaseController< } } + private getRollbackSnapshot(snapId: SnapId): RollbackSnapshot | undefined { + return this._rollbackSnapshots.get(snapId); + } + + private createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { + assert( + this._rollbackSnapshots.get(snapId) === undefined, + new Error(`Snap "${snapId}" rollback snapshot already exists`), + ); + + this._rollbackSnapshots.set(snapId, { + statePatches: [], + sourceCode: '', + permissions: { revoked: null, granted: null, requestData: null }, + }); + return this.getRollbackSnapshot(snapId) as RollbackSnapshot; + } + + private async rollbackSnap(snapId: SnapId) { + // 1. First check if the rollbackSnapshot exists, if it doesn't throw an error + // 2. Then you can begin to pipe in statePatches, sourceCode and permissions into the appropriate functions + // 3. Make the appropriate checks to make sure you are + const rollbackSnapshot = this.getRollbackSnapshot(snapId); + if (!rollbackSnapshot) { + throw new Error('A snapshot does not exist for this snap'); + } + + const { statePatches, sourceCode, permissions } = rollbackSnapshot; + + if (statePatches?.length) { + this.applyPatches(rollbackSnapshot.statePatches); + } + + if (sourceCode) { + const runtime = this.getRuntimeExpect(snapId); + runtime.sourceCode = rollbackSnapshot.sourceCode; + } + + if (permissions.revoked && Object.keys(permissions.revoked).length) { + await this.messagingSystem.call('PermissionController:grantPermissions', { + approvedPermissions: rollbackSnapshot.permissions + .revoked as RequestedPermissions, + subject: { origin: snapId }, + requestData: rollbackSnapshot.permissions.requestData as Record< + string, + unknown + >, + }); + } + + if (permissions.granted?.length) { + await this.messagingSystem.call( + 'PermissionController:revokePermissions', + { + [snapId]: permissions.granted as NonEmptyArray, + }, + ); + } + } + private getRuntime(snapId: SnapId): SnapRuntimeData | undefined { return this._snapsRuntimeData.get(snapId); } From b9c68cc09be1186d01c15af0844cafa80966ed9e Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 29 Nov 2022 12:46:14 -0500 Subject: [PATCH 03/33] more updates --- .../controllers/src/snaps/SnapController.ts | 61 +++++++++++++------ .../controllers/src/test-utils/controller.ts | 1 + 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/packages/controllers/src/snaps/SnapController.ts b/packages/controllers/src/snaps/SnapController.ts index f6c4b55608..5ab7e56e1a 100644 --- a/packages/controllers/src/snaps/SnapController.ts +++ b/packages/controllers/src/snaps/SnapController.ts @@ -218,6 +218,7 @@ type RollbackSnapshot = { granted: unknown[]; requestData: unknown; }; + newVersion: string; }; // Controller Messenger Actions @@ -397,6 +398,13 @@ export type SnapUpdated = { payload: [snap: TruncatedSnap, oldVersion: string]; }; +/** + * Emitted when a snap is rolled back. + */ +export type SnapRolledback = { + type: `${typeof controllerName}:snapRolledback`; + payload: [snap: TruncatedSnap, failedVersion: string]; +}; /** * Emitted when a Snap is terminated. This is different from the snap being * stopped as it can also be triggered when a snap fails initialization. @@ -414,6 +422,7 @@ export type SnapControllerEvents = | SnapStateChange | SnapUnblocked | SnapUpdated + | SnapRolledback | SnapTerminated; export type AllowedActions = @@ -1585,6 +1594,9 @@ export class SnapController extends BaseController< const prevSourceCode = this.getRuntimeExpect(snapId).sourceCode; rollbackSnapshot = this.createRollbackSnapshot(snapId); rollbackSnapshot.sourceCode = prevSourceCode; + rollbackSnapshot.newVersion = version; + } else { + console.error('This snap is already being updated.'); } } @@ -1599,7 +1611,11 @@ export class SnapController extends BaseController< } catch (err) { const installed = pendingInstalls.filter((snapId) => this.has(snapId)); this.removeSnaps(installed); - // TODO: Handle update roll backs + const snapshottedSnaps = [...this._rollbackSnapshots.keys()]; + const snapsToRollback = pendingUpdates.filter((snapId) => + snapshottedSnaps.includes(snapId), + ); + this.rollbackSnaps(snapsToRollback); throw err; } @@ -1768,11 +1784,6 @@ export class SnapController extends BaseController< isUpdate: true, }); - // record revokePermissions to later grant for update rollback - // and record approvedNewPermissions to later revoke for update rollback - // when rolling back check if the permissions exist as the update might - // have errored out before the permissions were assigned - const unusedPermissionsKeys = Object.keys(unusedPermissions); if (isNonEmptyArray(unusedPermissionsKeys)) { await this.messagingSystem.call( @@ -1798,10 +1809,11 @@ export class SnapController extends BaseController< rollbackSnapshot.permissions.granted = Object.keys(approvedNewPermissions); rollbackSnapshot.permissions.requestData = requestData; - // add a try/catch block here to catch an error if the snap does not start in update situations - // we might need to add a flag that delineates between installs/updates calling this methodor ac - - await this._startSnap({ snapId, sourceCode: newSnap.sourceCode }); + try { + await this._startSnap({ snapId, sourceCode: newSnap.sourceCode }); + } catch { + throw new Error('Snap crashed on update.'); + } const truncatedSnap = this.getTruncatedExpect(snapId); @@ -2447,24 +2459,22 @@ export class SnapController extends BaseController< private createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { assert( this._rollbackSnapshots.get(snapId) === undefined, - new Error(`Snap "${snapId}" rollback snapshot already exists`), + new Error(`Snap "${snapId}" rollback snapshot already exists.`), ); this._rollbackSnapshots.set(snapId, { statePatches: [], sourceCode: '', - permissions: { revoked: null, granted: null, requestData: null }, + permissions: { revoked: null, granted: [], requestData: null }, + newVersion: '', }); return this.getRollbackSnapshot(snapId) as RollbackSnapshot; } private async rollbackSnap(snapId: SnapId) { - // 1. First check if the rollbackSnapshot exists, if it doesn't throw an error - // 2. Then you can begin to pipe in statePatches, sourceCode and permissions into the appropriate functions - // 3. Make the appropriate checks to make sure you are const rollbackSnapshot = this.getRollbackSnapshot(snapId); if (!rollbackSnapshot) { - throw new Error('A snapshot does not exist for this snap'); + throw new Error('A snapshot does not exist for this snap.'); } const { statePatches, sourceCode, permissions } = rollbackSnapshot; @@ -2480,8 +2490,7 @@ export class SnapController extends BaseController< if (permissions.revoked && Object.keys(permissions.revoked).length) { await this.messagingSystem.call('PermissionController:grantPermissions', { - approvedPermissions: rollbackSnapshot.permissions - .revoked as RequestedPermissions, + approvedPermissions: permissions.revoked as RequestedPermissions, subject: { origin: snapId }, requestData: rollbackSnapshot.permissions.requestData as Record< string, @@ -2498,6 +2507,22 @@ export class SnapController extends BaseController< }, ); } + + await this.stopSnap(snapId, SnapStatusEvents.Stop); + + const truncatedSnap = this.getTruncatedExpect(snapId); + + this.messagingSystem.publish( + 'SnapController:snapRolledback', + truncatedSnap, + rollbackSnapshot.newVersion, + ); + } + + private async rollbackSnaps(snapIds: SnapId[]) { + for (const snapId of snapIds) { + this.rollbackSnap(snapId); + } } private getRuntime(snapId: SnapId): SnapRuntimeData | undefined { diff --git a/packages/controllers/src/test-utils/controller.ts b/packages/controllers/src/test-utils/controller.ts index de873a4dda..36384316ec 100644 --- a/packages/controllers/src/test-utils/controller.ts +++ b/packages/controllers/src/test-utils/controller.ts @@ -45,6 +45,7 @@ export const getSnapControllerMessenger = ( 'SnapController:snapUpdated', 'SnapController:snapRemoved', 'SnapController:stateChange', + 'SnapController:snapRolledback', ], allowedActions: [ 'ApprovalController:addRequest', From a2ed49606e9cfbcd3d4aba45e9d3e07230da90ba Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Wed, 30 Nov 2022 17:58:54 -0500 Subject: [PATCH 04/33] add test --- packages/snaps-controllers/package.json | 2 +- .../src/snaps/SnapController.test.ts | 70 ++++++++++++++++++- .../src/snaps/SnapController.ts | 7 +- yarn.lock | 14 +++- 4 files changed, 85 insertions(+), 8 deletions(-) diff --git a/packages/snaps-controllers/package.json b/packages/snaps-controllers/package.json index 042172a31e..ce91a4e2f7 100644 --- a/packages/snaps-controllers/package.json +++ b/packages/snaps-controllers/package.json @@ -32,7 +32,7 @@ }, "dependencies": { "@metamask/approval-controller": "^1.0.0", - "@metamask/base-controller": "^1.0.0", + "@metamask/base-controller": "^1.1.0", "@metamask/browser-passworder": "^4.0.2", "@metamask/object-multiplex": "^1.1.0", "@metamask/permission-controller": "^1.0.0", diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 33af58910f..6b4c9b49c0 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -26,7 +26,7 @@ import { MOCK_SNAP_ID, } from '@metamask/snaps-utils/test-utils'; import { Crypto } from '@peculiar/webcrypto'; -import { ethErrors } from 'eth-rpc-errors'; +import { EthereumRpcError, ethErrors } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; @@ -2407,7 +2407,7 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: { version: newVersionRange }, }), ).rejects.toThrow( - `Snap "${MOCK_SNAP_ID}@${manifest.version}" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, + `Snap "${MOCK_SNAP_ID}@0.9.0" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, ); expect(messenger.call).toHaveBeenCalledTimes(1); @@ -2453,6 +2453,72 @@ describe('SnapController', () => { expect(fetchSnapMock).toHaveBeenCalledTimes(1); expect(fetchSnapMock).toHaveBeenCalledWith(MOCK_SNAP_ID, newVersionRange); }); + + it('rollsback any updates & installs made during a failure scenario', async () => { + const snapId1 = 'npm:@metamask/example-snap1'; + const snapId2 = 'npm:@metamask/example-snap2'; + const snapId3 = 'npm:@metamask/example-snap3'; + const newVersion = '1.0.1'; + const messenger = getSnapControllerMessenger(); + const controller = getSnapController( + getSnapControllerOptions({ messenger }), + ); + + const fetchSnapMock = jest + .spyOn(controller as any, 'fetchSnap') + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: newVersion }), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: newVersion }), + sourceCode: 'foo', + }), + ); + + await controller.installSnaps(MOCK_ORIGIN, { [snapId1]: {} }); + await controller.installSnaps(MOCK_ORIGIN, { [snapId2]: {} }); + await controller.stopSnap(snapId1); + await controller.stopSnap(snapId2); + + expect(controller.get(snapId1)).toBeDefined(); + expect(controller.get(snapId2)).toBeDefined(); + + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [snapId3]: {}, + [snapId1]: { version: newVersion }, + [snapId2]: { version: newVersion }, + }), + ).rejects.toThrow(`Snap ${snapId2} crashed with updated source code.`); + + expect(fetchSnapMock).toHaveBeenCalledTimes(5); + + expect(controller.get(snapId3)).toBeUndefined(); + expect(controller.get(snapId1)?.manifest.version).toBe('1.0.0'); + expect(controller.get(snapId2)?.manifest.version).toBe('1.0.0'); + }); }); describe('updateSnap', () => { diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index f24e09f5b2..4100eeef3a 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1685,6 +1685,7 @@ export class SnapController extends BaseController< snapId, versionRange, ); + console.log('update result is:', updateResult); if (updateResult === null) { throw ethErrors.rpc.invalidParams( `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, @@ -1843,7 +1844,7 @@ export class SnapController extends BaseController< try { await this.#startSnap({ snapId, sourceCode: newSnap.sourceCode }); } catch { - throw new Error('Snap crashed on update.'); + throw new Error(`Snap ${snapId} crashed with updated source code.`); } const truncatedSnap = this.getTruncatedExpect(snapId); @@ -2081,12 +2082,12 @@ export class SnapController extends BaseController< delete snap.blockInformation; // store the snap back in state - const [, , inversePatches] = this.update((state: any) => { + const { inversePatches } = this.update((state: any) => { state.snaps[snapId] = snap; }); // checking for isUpdate here as this function is also used in - // the install flow, we do not care to create snapsshots for installs + // the install flow, we do not care to create snapshots for installs if (isUpdate) { const rollbackSnapshot = this.#getRollbackSnapshot( snapId, diff --git a/yarn.lock b/yarn.lock index 61265a8579..c5bfdf6618 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2470,7 +2470,17 @@ __metadata: languageName: node linkType: hard -"@metamask/base-controller@npm:^1.0.0, @metamask/base-controller@npm:~1.0.0": +"@metamask/base-controller@npm:^1.1.0": + version: 1.1.0 + resolution: "@metamask/base-controller@npm:1.1.0" + dependencies: + "@metamask/controller-utils": ~1.0.0 + immer: ^9.0.6 + checksum: 87f9a78bc2d7111d37fbddbf7ec08d856e18bc64d35a8ac77f9a8c1ed96f8b70c17ebd29931d01a77fe574850fd73467df7826bfbd56ebbd8dac7a4a554207ea + languageName: node + linkType: hard + +"@metamask/base-controller@npm:~1.0.0": version: 1.0.0 resolution: "@metamask/base-controller@npm:1.0.0" dependencies: @@ -2817,7 +2827,7 @@ __metadata: "@lavamoat/allow-scripts": ^2.0.3 "@metamask/approval-controller": ^1.0.0 "@metamask/auto-changelog": ^3.1.0 - "@metamask/base-controller": ^1.0.0 + "@metamask/base-controller": ^1.1.0 "@metamask/browser-passworder": ^4.0.2 "@metamask/eslint-config": ^11.0.0 "@metamask/eslint-config-jest": ^11.0.0 From ee1f5bf165b526b9592154f03b2bb8b882a06c79 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Wed, 30 Nov 2022 20:22:29 -0500 Subject: [PATCH 05/33] refactors --- .../src/snaps/SnapController.test.ts | 2 +- .../snaps-controllers/src/snaps/SnapController.ts | 12 ++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 6b4c9b49c0..49d07dab39 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2407,7 +2407,7 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: { version: newVersionRange }, }), ).rejects.toThrow( - `Snap "${MOCK_SNAP_ID}@0.9.0" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, + `Snap "${MOCK_SNAP_ID}@1.0.0" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, ); expect(messenger.call).toHaveBeenCalledTimes(1); diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 4100eeef3a..4115e82507 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1593,7 +1593,7 @@ export class SnapController extends BaseController< if (error) { throw ethErrors.rpc.invalidParams( - `The "version" field must be a valid SemVer version range if specified. Received: "${version}".`, + `The "version" field must be a valid SemVer version range if specified. Received: "${rawVersion}".`, ); } @@ -1685,7 +1685,6 @@ export class SnapController extends BaseController< snapId, versionRange, ); - console.log('update result is:', updateResult); if (updateResult === null) { throw ethErrors.rpc.invalidParams( `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, @@ -2554,22 +2553,19 @@ export class SnapController extends BaseController< const { statePatches, sourceCode, permissions } = rollbackSnapshot; if (statePatches?.length) { - this.applyPatches(rollbackSnapshot.statePatches); + this.applyPatches(statePatches); } if (sourceCode) { const runtime = this.#getRuntimeExpect(snapId); - runtime.sourceCode = rollbackSnapshot.sourceCode; + runtime.sourceCode = sourceCode; } if (permissions.revoked && Object.keys(permissions.revoked).length) { this.messagingSystem.call('PermissionController:grantPermissions', { approvedPermissions: permissions.revoked as RequestedPermissions, subject: { origin: snapId }, - requestData: rollbackSnapshot.permissions.requestData as Record< - string, - unknown - >, + requestData: permissions.requestData as Record, }); } From 4e8ff8df8f23cab23b540259d86f57720654035d Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Wed, 30 Nov 2022 20:49:09 -0500 Subject: [PATCH 06/33] fixed some more errors --- .../snaps-controllers/src/snaps/SnapController.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 4115e82507..fab36a7257 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1836,9 +1836,13 @@ export class SnapController extends BaseController< const rollbackSnapshot = this.#getRollbackSnapshot( snapId, ) as RollbackSnapshot; - rollbackSnapshot.permissions.revoked = unusedPermissions; - rollbackSnapshot.permissions.granted = Object.keys(approvedNewPermissions); - rollbackSnapshot.permissions.requestData = requestData; + if (rollbackSnapshot !== undefined) { + rollbackSnapshot.permissions.revoked = unusedPermissions; + rollbackSnapshot.permissions.granted = Object.keys( + approvedNewPermissions, + ); + rollbackSnapshot.permissions.requestData = requestData; + } try { await this.#startSnap({ snapId, sourceCode: newSnap.sourceCode }); @@ -2091,7 +2095,9 @@ export class SnapController extends BaseController< const rollbackSnapshot = this.#getRollbackSnapshot( snapId, ) as RollbackSnapshot; - rollbackSnapshot.statePatches = inversePatches; + if (rollbackSnapshot !== undefined) { + rollbackSnapshot.statePatches = inversePatches; + } } const runtime = this.#getRuntimeExpect(snapId); From 012d75150810a7d09a29a5bf67b3e91be411c49d Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Wed, 30 Nov 2022 21:13:04 -0500 Subject: [PATCH 07/33] fixed another failing test --- packages/snaps-controllers/src/snaps/SnapController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index fab36a7257..c2090c513d 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -77,7 +77,7 @@ import { timeSince, } from '@metamask/utils'; import { createMachine, interpret, StateMachine } from '@xstate/fsm'; -import { ethErrors, serializeError } from 'eth-rpc-errors'; +import { ethErrors } from 'eth-rpc-errors'; import type { Patch } from 'immer'; import { enablePatches } from 'immer'; import { nanoid } from 'nanoid'; @@ -1726,7 +1726,7 @@ export class SnapController extends BaseController< await this.removeSnap(snapId); } - return { error: serializeError(error) }; + throw error; } } From e54aec8541affc6d4a4f5321f3361626a6e2a750 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 1 Dec 2022 10:27:19 +0100 Subject: [PATCH 08/33] Fix test --- .../controllers/src/test-utils/controller.ts | 267 ------------------ .../src/snaps/SnapController.test.ts | 5 +- .../src/test-utils/controller.ts | 12 + 3 files changed, 14 insertions(+), 270 deletions(-) delete mode 100644 packages/controllers/src/test-utils/controller.ts diff --git a/packages/controllers/src/test-utils/controller.ts b/packages/controllers/src/test-utils/controller.ts deleted file mode 100644 index 36384316ec..0000000000 --- a/packages/controllers/src/test-utils/controller.ts +++ /dev/null @@ -1,267 +0,0 @@ -import { ControllerMessenger } from '@metamask/controllers'; -import { getPersistedSnapObject } from '@metamask/snap-utils/test-utils'; -import { - AllowedActions, - AllowedEvents, - CheckSnapBlockListArg, - PersistedSnapControllerState, - SnapController, - SnapControllerActions, - SnapControllerEvents, - SnapEndowments, -} from '../snaps'; -import { - CronjobControllerActions, - CronjobControllerEvents, -} from '../cronjob/CronjobController'; -import { getNodeEES, getNodeEESMessenger } from './execution-environment'; - -export const getControllerMessenger = () => - new ControllerMessenger< - SnapControllerActions | AllowedActions, - SnapControllerEvents | AllowedEvents - >(); - -export const getSnapControllerMessenger = ( - messenger: ReturnType< - typeof getControllerMessenger - > = getControllerMessenger(), - mocked = true, -) => { - const m = messenger.getRestricted< - 'SnapController', - SnapControllerActions['type'] | AllowedActions['type'], - SnapControllerEvents['type'] | AllowedEvents['type'] - >({ - name: 'SnapController', - allowedEvents: [ - 'ExecutionService:unhandledError', - 'ExecutionService:outboundRequest', - 'ExecutionService:outboundResponse', - 'SnapController:snapAdded', - 'SnapController:snapBlocked', - 'SnapController:snapInstalled', - 'SnapController:snapUnblocked', - 'SnapController:snapUpdated', - 'SnapController:snapRemoved', - 'SnapController:stateChange', - 'SnapController:snapRolledback', - ], - allowedActions: [ - 'ApprovalController:addRequest', - 'ExecutionService:executeSnap', - 'ExecutionService:terminateAllSnaps', - 'ExecutionService:terminateSnap', - 'ExecutionService:handleRpcRequest', - 'PermissionController:getEndowments', - 'PermissionController:hasPermission', - 'PermissionController:hasPermissions', - 'PermissionController:getPermissions', - 'PermissionController:grantPermissions', - 'PermissionController:revokeAllPermissions', - 'SnapController:get', - 'SnapController:handleRequest', - 'SnapController:getSnapState', - 'SnapController:has', - 'SnapController:updateSnapState', - 'SnapController:clearSnapState', - 'SnapController:updateBlockedSnaps', - 'SnapController:enable', - 'SnapController:disable', - 'SnapController:remove', - 'SnapController:getAll', - 'SnapController:getPermitted', - 'SnapController:install', - 'SnapController:removeSnapError', - 'SnapController:incrementActiveReferences', - 'SnapController:decrementActiveReferences', - ], - }); - - if (mocked) { - jest.spyOn(m, 'call').mockImplementation((method, ...args) => { - // Return false for long-running by default, and true for everything else. - if ( - method === 'PermissionController:hasPermission' && - args[1] === SnapEndowments.LongRunning - ) { - return false; - } else if (method === 'ApprovalController:addRequest') { - return (args[0] as any).requestData; - } - return true; - }); - } - return m; -}; - -export type SnapControllerConstructorParams = ConstructorParameters< - typeof SnapController ->[0]; - -export type PartialSnapControllerConstructorParams = Partial< - Omit[0], 'state'> & { - state: Partial; - } ->; - -export const getSnapControllerOptions = ( - opts?: PartialSnapControllerConstructorParams, -) => { - const options = { - environmentEndowmentPermissions: [], - closeAllConnections: jest.fn(), - getAppKey: jest - .fn() - .mockImplementation((snapId, appKeyType) => `${appKeyType}:${snapId}`), - messenger: getSnapControllerMessenger(), - featureFlags: { dappsCanUpdateSnaps: true }, - checkBlockList: jest - .fn() - .mockImplementation(async (snaps: CheckSnapBlockListArg) => { - return Object.keys(snaps).reduce( - (acc, snapId) => ({ ...acc, [snapId]: { blocked: false } }), - {}, - ); - }), - state: undefined, - ...opts, - } as SnapControllerConstructorParams; - - options.state = { - snaps: {}, - snapErrors: {}, - snapStates: {}, - ...options.state, - }; - return options; -}; - -export type GetSnapControllerWithEESOptionsParam = Omit< - PartialSnapControllerConstructorParams, - 'messenger' -> & { rootMessenger?: ReturnType }; - -export const getSnapControllerWithEESOptions = ( - opts: GetSnapControllerWithEESOptionsParam = {}, -) => { - const { rootMessenger = getControllerMessenger() } = opts; - const snapControllerMessenger = getSnapControllerMessenger( - rootMessenger, - false, - ); - const originalCall = snapControllerMessenger.call.bind( - snapControllerMessenger, - ); - - jest - .spyOn(snapControllerMessenger, 'call') - .mockImplementation((method, ...args) => { - // Mock long running permission, call actual implementation for everything else - if ( - method === 'PermissionController:hasPermission' && - args[1] === SnapEndowments.LongRunning - ) { - return false; - } - return originalCall(method, ...args); - }); - - return { - environmentEndowmentPermissions: [], - closeAllConnections: jest.fn(), - getAppKey: jest - .fn() - .mockImplementation((snapId, appKeyType) => `${appKeyType}:${snapId}`), - messenger: snapControllerMessenger, - ...opts, - rootMessenger, - } as SnapControllerConstructorParams & { - rootMessenger: ReturnType; - }; -}; - -export const getSnapController = (options = getSnapControllerOptions()) => { - return new SnapController(options); -}; - -export const getSnapControllerWithEES = ( - options = getSnapControllerWithEESOptions(), - service?: ReturnType, -) => { - const _service = - service ?? getNodeEES(getNodeEESMessenger(options.rootMessenger)); - const controller = new SnapController(options); - return [controller, _service] as const; -}; - -export const getPersistedSnapsState = ( - ...snaps: PersistedSnapControllerState['snaps'][string][] -): PersistedSnapControllerState['snaps'] => { - return (snaps.length > 0 ? snaps : [getPersistedSnapObject()]).reduce( - (snapsState, snapObject) => { - snapsState[snapObject.id] = snapObject; - return snapsState; - }, - {} as PersistedSnapControllerState['snaps'], - ); -}; - -// Mock controller messenger for Cronjob Controller -export const getRootCronjobControllerMessenger = () => - new ControllerMessenger< - CronjobControllerActions | AllowedActions, - CronjobControllerEvents | AllowedEvents - >(); - -export const getRestrictedCronjobControllerMessenger = ( - messenger: ReturnType< - typeof getRootCronjobControllerMessenger - > = getRootCronjobControllerMessenger(), - mocked = true, -) => { - const m = messenger.getRestricted< - 'CronjobController', - CronjobControllerActions['type'] | AllowedActions['type'], - CronjobControllerEvents['type'] | AllowedEvents['type'] - >({ - name: 'CronjobController', - allowedEvents: [ - 'ExecutionService:unhandledError', - 'ExecutionService:outboundRequest', - 'ExecutionService:outboundResponse', - 'SnapController:snapAdded', - 'SnapController:snapBlocked', - 'SnapController:snapInstalled', - 'SnapController:snapUnblocked', - 'SnapController:snapUpdated', - 'SnapController:snapRemoved', - ], - allowedActions: [ - 'ApprovalController:addRequest', - 'ExecutionService:executeSnap', - 'ExecutionService:terminateAllSnaps', - 'ExecutionService:terminateSnap', - 'ExecutionService:handleRpcRequest', - 'PermissionController:getEndowments', - 'PermissionController:hasPermission', - 'PermissionController:hasPermissions', - 'PermissionController:getPermissions', - 'SnapController:handleRequest', - ], - }); - - if (mocked) { - jest.spyOn(m, 'call').mockImplementation((method, ...args) => { - // Return false for long-running by default, and true for everything else. - if ( - method === 'PermissionController:hasPermission' && - args[1] === SnapEndowments.LongRunning - ) { - return false; - } - return true; - }); - } - return m; -}; diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 49d07dab39..17ac63e3e7 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2459,9 +2459,8 @@ describe('SnapController', () => { const snapId2 = 'npm:@metamask/example-snap2'; const snapId3 = 'npm:@metamask/example-snap3'; const newVersion = '1.0.1'; - const messenger = getSnapControllerMessenger(); - const controller = getSnapController( - getSnapControllerOptions({ messenger }), + const [controller] = getSnapControllerWithEES( + getSnapControllerWithEESOptions(), ); const fetchSnapMock = jest diff --git a/packages/snaps-controllers/src/test-utils/controller.ts b/packages/snaps-controllers/src/test-utils/controller.ts index 961c2a92eb..609bca66f2 100644 --- a/packages/snaps-controllers/src/test-utils/controller.ts +++ b/packages/snaps-controllers/src/test-utils/controller.ts @@ -205,6 +205,7 @@ export const getSnapControllerMessenger = ( 'SnapController:snapUpdated', 'SnapController:snapRemoved', 'SnapController:stateChange', + 'SnapController:snapRolledback', ], allowedActions: [ 'ApprovalController:addRequest', @@ -301,11 +302,22 @@ export const getSnapControllerWithEESOptions = ({ const snapControllerMessenger = getSnapControllerMessenger(rootMessenger); return { + featureFlags: { dappsCanUpdateSnaps: true }, environmentEndowmentPermissions: [], closeAllConnections: jest.fn(), getAppKey: jest .fn() .mockImplementation((snapId, appKeyType) => `${appKeyType}:${snapId}`), + checkBlockList: jest + .fn() + .mockImplementation(async (snaps: CheckSnapBlockListArg) => + Promise.resolve( + Object.keys(snaps).reduce( + (acc, snapId) => ({ ...acc, [snapId]: { blocked: false } }), + {}, + ), + ), + ), messenger: snapControllerMessenger, rootMessenger, ...options, From 9e18b1756549efd0a9197cb21e5301527d68ebf2 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Thu, 1 Dec 2022 11:41:53 -0500 Subject: [PATCH 09/33] updated test --- .../snaps-controllers/src/snaps/SnapController.test.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 17ac63e3e7..c4b9976eff 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2458,8 +2458,9 @@ describe('SnapController', () => { const snapId1 = 'npm:@metamask/example-snap1'; const snapId2 = 'npm:@metamask/example-snap2'; const snapId3 = 'npm:@metamask/example-snap3'; + const oldVersion = '1.0.0'; const newVersion = '1.0.1'; - const [controller] = getSnapControllerWithEES( + const [controller, service] = getSnapControllerWithEES( getSnapControllerWithEESOptions(), ); @@ -2515,8 +2516,11 @@ describe('SnapController', () => { expect(fetchSnapMock).toHaveBeenCalledTimes(5); expect(controller.get(snapId3)).toBeUndefined(); - expect(controller.get(snapId1)?.manifest.version).toBe('1.0.0'); - expect(controller.get(snapId2)?.manifest.version).toBe('1.0.0'); + expect(controller.get(snapId1)?.manifest.version).toBe(oldVersion); + expect(controller.get(snapId2)?.manifest.version).toBe(oldVersion); + + controller.destroy(); + await service.terminateAllSnaps(); }); }); From 9c13101337483e98224acc532f02d688c341bac8 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Thu, 1 Dec 2022 11:59:33 -0500 Subject: [PATCH 10/33] reverted some inadvertent changes --- .../src/snaps/SnapController.test.ts | 14 ++++++-------- .../snaps-controllers/src/snaps/SnapController.ts | 10 +--------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index c4b9976eff..22c2f4871d 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -26,7 +26,7 @@ import { MOCK_SNAP_ID, } from '@metamask/snaps-utils/test-utils'; import { Crypto } from '@peculiar/webcrypto'; -import { EthereumRpcError, ethErrors } from 'eth-rpc-errors'; +import { ethErrors } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; @@ -2242,13 +2242,11 @@ describe('SnapController', () => { getSnapControllerOptions({ messenger }), ); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [snapId]: {}, - }); - - expect(result).toStrictEqual({ - [snapId]: { error: expect.any(EthereumRpcError) }, - }); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [snapId]: {}, + }), + ).rejects.toThrow('Invalid snap id. Unknown prefix. Received: "foo"'); expect(messenger.call).toHaveBeenCalledTimes(1); expect(messenger.call).toHaveBeenCalledWith( 'PermissionController:hasPermission', diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index c2090c513d..22e54566f6 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1662,15 +1662,7 @@ export class SnapController extends BaseController< snapId: SnapId, versionRange: SemVerRange, ): Promise { - try { - validateSnapId(snapId); - } catch (error) { - return { - error: ethErrors.rpc.invalidParams( - `"${snapId}" is not a valid snap id.`, - ), - }; - } + validateSnapId(snapId); const existingSnap = this.getTruncated(snapId); // For devX we always re-install local snaps. From 020c1c9b820442442f63fecfb03ec3c164da32d4 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Thu, 1 Dec 2022 12:17:23 -0500 Subject: [PATCH 11/33] removed unnecessary immer usage --- packages/snaps-controllers/src/snaps/SnapController.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 22e54566f6..fec905853f 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -79,7 +79,6 @@ import { import { createMachine, interpret, StateMachine } from '@xstate/fsm'; import { ethErrors } from 'eth-rpc-errors'; import type { Patch } from 'immer'; -import { enablePatches } from 'immer'; import { nanoid } from 'nanoid'; import { forceStrict, validateMachine } from '../fsm'; @@ -102,7 +101,6 @@ import { RequestQueue } from './RequestQueue'; import { Timer } from './Timer'; import { fetchNpmSnap } from './utils'; -enablePatches(); export const controllerName = 'SnapController'; // TODO: Figure out how to name these From 5b886728126185f02a35c1581a154454331fa66f Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Mon, 5 Dec 2022 21:13:46 -0500 Subject: [PATCH 12/33] address PR comments --- .../src/snaps/SnapController.test.ts | 83 ++++++++++++++++++- .../src/snaps/SnapController.ts | 50 ++++++----- 2 files changed, 108 insertions(+), 25 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 22c2f4871d..04b6a0cc31 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -10,6 +10,7 @@ import { getSnapSourceShasum, HandlerType, SemVerVersion, + SemVerRange, SnapCaveatType, SnapManifest, SnapStatus, @@ -2452,7 +2453,7 @@ describe('SnapController', () => { expect(fetchSnapMock).toHaveBeenCalledWith(MOCK_SNAP_ID, newVersionRange); }); - it('rollsback any updates & installs made during a failure scenario', async () => { + it('rolls back any updates & installs made during a failure scenario', async () => { const snapId1 = 'npm:@metamask/example-snap1'; const snapId2 = 'npm:@metamask/example-snap2'; const snapId3 = 'npm:@metamask/example-snap3'; @@ -2520,6 +2521,82 @@ describe('SnapController', () => { controller.destroy(); await service.terminateAllSnaps(); }); + + it('will not create snapshots for already installed snaps that have invalid requested ranges', async () => { + const snapId1 = 'npm:@metamask/example-snap1'; + const snapId2 = 'npm:@metamask/example-snap2'; + const snapId3 = 'npm:@metamask/example-snap3'; + const oldVersion = '1.0.0'; + const newVersion = '1.0.1'; + const olderVersion = '0.9.0'; + const options = getSnapControllerWithEESOptions(); + const { messenger } = options; + const [controller, service] = getSnapControllerWithEES(options); + + const listener = jest.fn(); + messenger.subscribe('SnapController:snapRolledback' as any, listener); + + const fetchSnapMock = jest + .spyOn(controller as any, 'fetchSnap') + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: olderVersion }), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: newVersion }), + sourceCode: 'foo', + }), + ); + + await controller.installSnaps(MOCK_ORIGIN, { [snapId1]: {} }); + await controller.installSnaps(MOCK_ORIGIN, { [snapId2]: {} }); + await controller.stopSnap(snapId1); + await controller.stopSnap(snapId2); + + expect(controller.get(snapId1)).toBeDefined(); + expect(controller.get(snapId2)).toBeDefined(); + + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [snapId3]: {}, + [snapId1]: { version: olderVersion }, + [snapId2]: { version: newVersion }, + }), + ).rejects.toThrow( + `Snap "${snapId1}@${oldVersion}" is already installed, couldn't update to a version inside requested "${olderVersion}" range.`, + ); + + expect(fetchSnapMock).toHaveBeenCalledTimes(5); + + expect(controller.get(snapId3)).toBeUndefined(); + expect(controller.get(snapId1)?.manifest.version).toBe(oldVersion); + expect(controller.get(snapId2)?.manifest.version).toBe(oldVersion); + expect(listener).toHaveBeenCalledTimes(1); + + controller.destroy(); + await service.terminateAllSnaps(); + }); }); describe('updateSnap', () => { @@ -2542,9 +2619,9 @@ describe('SnapController', () => { controller.updateSnap( MOCK_ORIGIN, MOCK_SNAP_ID, - 'this is not a version', + 'this is not a version' as SemVerRange, ), - ).rejects.toThrow('Received invalid snap version range'); + ).rejects.toThrow('Received invalid Snap version range.'); }); it('throws an error if the new version of the snap is blocked', async () => { diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index fec905853f..87558cc690 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -30,6 +30,7 @@ import { getSnapPermissionName, getSnapPrefix, gtVersion, + gtRange, InstallSnapsResult, isValidSemVerRange, LOCALHOST_HOSTNAMES, @@ -659,7 +660,7 @@ export class SnapController extends BaseController< // This property cannot be hash private yet because of tests. private readonly snapsRuntimeData: Map; - private readonly _rollbackSnapshots: Map; + #rollbackSnapshots: Map; #getAppKey: GetAppKey; @@ -753,7 +754,7 @@ export class SnapController extends BaseController< this._onUnhandledSnapError = this._onUnhandledSnapError.bind(this); this._onOutboundRequest = this._onOutboundRequest.bind(this); this._onOutboundResponse = this._onOutboundResponse.bind(this); - this._rollbackSnapshots = new Map(); + this.#rollbackSnapshots = new Map(); this.snapsRuntimeData = new Map(); this.#pollForLastRequestStatus(); @@ -1611,7 +1612,7 @@ export class SnapController extends BaseController< const isUpdate = pendingUpdates.includes(snapId); - if (isUpdate) { + if (isUpdate && this.#isValidUpdate(snapId, version)) { let rollbackSnapshot = this.#getRollbackSnapshot(snapId); if (rollbackSnapshot === undefined) { const prevSourceCode = @@ -1632,10 +1633,11 @@ export class SnapController extends BaseController< }, ), ); + snapIds.forEach((snapId) => this.#rollbackSnapshots.delete(snapId)); } catch (error) { const installed = pendingInstalls.filter((snapId) => this.has(snapId)); await this.removeSnaps(installed); - const snapshottedSnaps = [...this._rollbackSnapshots.keys()]; + const snapshottedSnaps = [...this.#rollbackSnapshots.keys()]; const snapsToRollback = pendingUpdates.filter((snapId) => snapshottedSnaps.includes(snapId), ); @@ -1712,9 +1714,6 @@ export class SnapController extends BaseController< return truncated; } catch (error) { console.error(`Error when adding snap.`, error); - if (this.has(snapId)) { - await this.removeSnap(snapId); - } throw error; } @@ -1740,16 +1739,10 @@ export class SnapController extends BaseController< async updateSnap( origin: string, snapId: ValidatedSnapId, - newVersionRange: string = DEFAULT_REQUESTED_SNAP_VERSION, + newVersionRange: SemVerRange = DEFAULT_REQUESTED_SNAP_VERSION, ): Promise { const snap = this.getExpect(snapId); - if (!isValidSemVerRange(newVersionRange)) { - throw new Error( - `Received invalid snap version range: "${newVersionRange}".`, - ); - } - const newSnap = await this.fetchSnap(snapId, newVersionRange); const newVersion = newSnap.manifest.version; if (!gtVersion(newVersion, snap.version)) { @@ -2137,9 +2130,7 @@ export class SnapController extends BaseController< versionRange: string, ): Promise { if (!isValidSemVerRange(versionRange)) { - throw new Error( - `Received invalid Snap version range: "${versionRange}".`, - ); + throw new Error('Received invalid Snap version range.'); } const { manifest, sourceCode, svgIcon } = await fetchNpmSnap( @@ -2522,7 +2513,7 @@ export class SnapController extends BaseController< } #getRollbackSnapshot(snapId: SnapId): RollbackSnapshot | undefined { - return this._rollbackSnapshots.get(snapId); + return this.#rollbackSnapshots.get(snapId); } #createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { @@ -2531,7 +2522,7 @@ export class SnapController extends BaseController< new Error(`Snap "${snapId}" rollback snapshot already exists.`), ); - this._rollbackSnapshots.set(snapId, { + this.#rollbackSnapshots.set(snapId, { statePatches: [], sourceCode: '', permissions: { revoked: null, granted: [], requestData: null }, @@ -2546,6 +2537,8 @@ export class SnapController extends BaseController< throw new Error('A snapshot does not exist for this snap.'); } + await this.stopSnap(snapId, SnapStatusEvents.Stop); + const { statePatches, sourceCode, permissions } = rollbackSnapshot; if (statePatches?.length) { @@ -2571,8 +2564,6 @@ export class SnapController extends BaseController< }); } - await this.stopSnap(snapId, SnapStatusEvents.Stop); - const truncatedSnap = this.getTruncatedExpect(snapId); this.messagingSystem.publish( @@ -2580,12 +2571,13 @@ export class SnapController extends BaseController< truncatedSnap, rollbackSnapshot.newVersion, ); + + this.#rollbackSnapshots.delete(snapId); } async #rollbackSnaps(snapIds: SnapId[]) { for (const snapId of snapIds) { await this.#rollbackSnap(snapId); - this._rollbackSnapshots.delete(snapId); } } @@ -2662,4 +2654,18 @@ export class SnapController extends BaseController< return { newPermissions, unusedPermissions, approvedPermissions }; } + + #isValidUpdate(snapId: SnapId, newVersionRange: SemVerRange) { + const existingSnap = this.getExpect(snapId); + + if (satisfiesVersionRange(existingSnap.version, newVersionRange)) { + return false; + } + + if (gtRange(existingSnap.version, newVersionRange)) { + return false; + } + + return true; + } } From 76a9474937a49e58f5bf91958bcf31cd829a6397 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Mon, 5 Dec 2022 21:24:23 -0500 Subject: [PATCH 13/33] add semver util --- packages/snaps-utils/src/versions.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/snaps-utils/src/versions.ts b/packages/snaps-utils/src/versions.ts index 8fcd36553a..f8b1a936ae 100644 --- a/packages/snaps-utils/src/versions.ts +++ b/packages/snaps-utils/src/versions.ts @@ -1,6 +1,7 @@ import { assertStruct, Json } from '@metamask/utils'; import { gt as gtSemver, + gtr, maxSatisfying as maxSatisfyingSemver, satisfies as satisfiesSemver, valid as validSemVerVersion, @@ -141,6 +142,17 @@ export function gtVersion( return gtSemver(version1, version2); } +/** + * Checks whether a SemVer version is greater than all possibilities in a range. + * + * @param version - A SemvVer version. + * @param range - The range to check against. + * @returns `version > range`. + */ +export function gtRange(version: SemVerVersion, range: SemVerRange): boolean { + return gtr(version, range); +} + /** * Returns whether a SemVer version satisfies a SemVer range. * From c358a1fa8bae122d149a619d1771c292612c6262 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 26 Oct 2022 14:18:16 +0200 Subject: [PATCH 14/33] Refactor wallet_enable --- .../src/snaps/SnapController.test.ts | 85 +++++------- .../src/snaps/SnapController.ts | 130 +++++++++--------- 2 files changed, 97 insertions(+), 118 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 76edaeccfc..b515fb35d1 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -29,7 +29,7 @@ import { } from '@metamask/snaps-utils/test-utils'; import { AssertionError } from '@metamask/utils'; import { Crypto } from '@peculiar/webcrypto'; -import { EthereumRpcError, ethErrors, serializeError } from 'eth-rpc-errors'; +import { ethErrors } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; @@ -622,13 +622,13 @@ describe('SnapController', () => { it('throws an error on invalid semver range during installSnaps', async () => { const controller = getSnapController(); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: { version: 'foo' }, - }); - - expect(result).toMatchObject({ - [MOCK_SNAP_ID]: { error: expect.any(EthereumRpcError) }, - }); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: 'foo' }, + }), + ).rejects.toThrow( + 'The "version" field must be a valid SemVer version range if specified. Received: "foo"', + ); }); it('reuses an already installed Snap if it satisfies the requested SemVer range', async () => { @@ -682,17 +682,12 @@ describe('SnapController', () => { return true; }); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: {}, - }); - - const { code, message } = serializeError( - ethErrors.provider.userRejectedRequest(), - ); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: {}, + }), + ).rejects.toThrow('User rejected the request.'); - expect(result).toStrictEqual({ - [MOCK_SNAP_ID]: { error: expect.objectContaining({ code, message }) }, - }); expect(controller.get(MOCK_SNAP_ID)).toBeUndefined(); }); @@ -737,13 +732,11 @@ describe('SnapController', () => { const expectedSnapObject = getTruncatedSnap(); - expect( - await snapController.installSnaps(MOCK_ORIGIN, { + await expect( + snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }), - ).toStrictEqual({ - [MOCK_SNAP_ID]: { error: serializeError(new Error('foo')) }, - }); + ).rejects.toThrow('foo'); expect(messengerCallMock).toHaveBeenCalledTimes(3); expect(messengerCallMock).toHaveBeenNthCalledWith( @@ -2220,20 +2213,11 @@ describe('SnapController', () => { const controller = getSnapController( getSnapControllerOptions({ messenger }), ); - - const result = await controller.installSnaps(MOCK_ORIGIN, { - [snapId]: {}, - }); - - expect(result).toStrictEqual({ - [snapId]: { error: expect.any(EthereumRpcError) }, - }); - expect(messenger.call).toHaveBeenCalledTimes(1); - expect(messenger.call).toHaveBeenCalledWith( - 'PermissionController:hasPermission', - MOCK_ORIGIN, - expect.anything(), - ); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [snapId]: {}, + }), + ).rejects.toThrow('Invalid snap id. Unknown prefix. Received: "foo"'); }); it('updates a snap', async () => { @@ -2376,9 +2360,14 @@ describe('SnapController', () => { }), ); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: { version: newVersionRange }, - }); + const manifest = getSnapManifest(); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: newVersionRange }, + }), + ).rejects.toThrow( + `Snap "${MOCK_SNAP_ID}@${manifest.version}" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, + ); expect(messenger.call).toHaveBeenCalledTimes(1); expect(messenger.call).toHaveBeenCalledWith( @@ -2391,10 +2380,6 @@ describe('SnapController', () => { MOCK_SNAP_ID, expect.objectContaining({ versionRange: newVersionRange }), ); - - expect(result).toStrictEqual({ - [MOCK_SNAP_ID]: { error: expect.any(EthereumRpcError) }, - }); }); it('returns an error when a throw happens inside an update', async () => { @@ -2417,9 +2402,11 @@ describe('SnapController', () => { }), ); - const result = await controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: { version: newVersionRange }, - }); + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: newVersionRange }, + }), + ).rejects.toThrow('foo'); expect(messenger.call).toHaveBeenCalledTimes(1); expect(messenger.call).toHaveBeenCalledWith( @@ -2432,10 +2419,6 @@ describe('SnapController', () => { MOCK_SNAP_ID, expect.objectContaining({ versionRange: newVersionRange }), ); - - expect(result).toStrictEqual({ - [MOCK_SNAP_ID]: { error: expect.anything() }, - }); }); }); diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 1f44aec48f..4a4206c33c 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -71,7 +71,7 @@ import { timeSince, } from '@metamask/utils'; import { createMachine, interpret, StateMachine } from '@xstate/fsm'; -import { ethErrors, serializeError } from 'eth-rpc-errors'; +import { ethErrors } from 'eth-rpc-errors'; import type { Patch } from 'immer'; import { nanoid } from 'nanoid'; @@ -1551,43 +1551,56 @@ export class SnapController extends BaseController< ): Promise { const result: InstallSnapsResult = {}; - await Promise.all( - Object.entries(requestedSnaps).map( - async ([snapId, { version: rawVersion }]) => { - const [error, version] = resolveVersionRange(rawVersion); - if (error) { - result[snapId] = { - error: ethErrors.rpc.invalidParams( + const snapIds = Object.keys(requestedSnaps); + + // Existing snaps may need to be updated + const pendingUpdates = snapIds.filter((snapId) => this.has(snapId)); + + // Non-existing snaps will need to be installed + const pendingInstalls = snapIds.filter( + (snapId) => !pendingUpdates.includes(snapId), + ); + + try { + await Promise.all( + Object.entries(requestedSnaps).map( + async ([snapId, { version: rawVersion }]) => { + const [error, version] = resolveVersionRange(rawVersion); + if (error) { + throw ethErrors.rpc.invalidParams( `The "version" field must be a valid SemVer version range if specified. Received: "${version}".`, - ), - }; - return; - } - const permissionName = getSnapPermissionName(snapId); + ); + } + + const permissionName = getSnapPermissionName(snapId); + + if ( + !this.messagingSystem.call( + 'PermissionController:hasPermission', + origin, + permissionName, + ) + ) { + throw ethErrors.provider.unauthorized( + `Not authorized to install snap "${snapId}". Request the permission for the snap before attempting to install it.`, + ); + } - if ( - this.messagingSystem.call( - 'PermissionController:hasPermission', + result[snapId] = await this.processRequestedSnap( origin, - permissionName, - ) - ) { - // Attempt to install and run the snap, storing any errors that - // occur during the process. - result[snapId] = { - ...(await this.processRequestedSnap(origin, snapId, version)), - }; - } else { - // only allow the installation of permitted snaps - result[snapId] = { - error: ethErrors.provider.unauthorized( - `Not authorized to install snap "${snapId}". Request the permission for the snap before attempting to install it.`, - ), - }; - } - }, - ), - ); + snapId, + version, + ); + }, + ), + ); + } catch (error) { + const installed = pendingInstalls.filter((snapId) => this.has(snapId)); + await this.removeSnaps(installed); + // TODO: Handle update roll backs + + throw error; + } return result; } @@ -1605,15 +1618,7 @@ export class SnapController extends BaseController< snapId: SnapId, versionRange: SemVerRange, ): Promise { - try { - validateSnapId(snapId); - } catch (error) { - return { - error: ethErrors.rpc.invalidParams( - `"${snapId}" is not a valid snap id.`, - ), - }; - } + validateSnapId(snapId); const location = this.#detectSnapLocation(snapId, { versionRange }); @@ -1625,31 +1630,22 @@ export class SnapController extends BaseController< } if (this.#featureFlags.dappsCanUpdateSnaps === true) { - try { - const updateResult = await this.updateSnap( - origin, - snapId, - versionRange, - location, + const updateResult = await this.updateSnap( + origin, + snapId, + versionRange, + location, + ); + if (updateResult === null) { + throw ethErrors.rpc.invalidParams( + `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, ); - if (updateResult === null) { - return { - error: ethErrors.rpc.invalidParams( - `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, - ), - }; - } - return updateResult; - } catch (error) { - return { error: serializeError(error) }; } - } else { - return { - error: ethErrors.rpc.invalidParams( - `Version mismatch with already installed snap. ${snapId}@${existingSnap.version} doesn't satisfy requested version ${versionRange}`, - ), - }; + return updateResult; } + throw ethErrors.rpc.invalidParams( + `Version mismatch with already installed snap. ${snapId}@${existingSnap.version} doesn't satisfy requested version ${versionRange}`, + ); } // Existing snaps must be stopped before overwriting @@ -1681,7 +1677,7 @@ export class SnapController extends BaseController< await this.removeSnap(snapId); } - return { error: serializeError(error) }; + throw error; } } From f2a54e1f8f6b7f94788ccd44420f25ba9252528d Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Sun, 27 Nov 2022 18:44:34 -0600 Subject: [PATCH 15/33] add initial rollback code --- .../src/snaps/SnapController.ts | 121 +++++++++++++++++- 1 file changed, 115 insertions(+), 6 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 4a4206c33c..3166f2df52 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -13,6 +13,7 @@ import { HasPermissions, PermissionConstraint, PermissionsRequest, + RequestedPermissions, RevokeAllPermissions, RevokePermissionForAllSubjects, RevokePermissions, @@ -69,10 +70,12 @@ import { isValidJson, Json, timeSince, + NonEmptyArray, } from '@metamask/utils'; import { createMachine, interpret, StateMachine } from '@xstate/fsm'; import { ethErrors } from 'eth-rpc-errors'; import type { Patch } from 'immer'; +import { enablePatches } from 'immer'; import { nanoid } from 'nanoid'; import { forceStrict, validateMachine } from '../fsm'; @@ -95,6 +98,7 @@ import { detectSnapLocation, SnapLocation } from './location'; import { RequestQueue } from './RequestQueue'; import { Timer } from './Timer'; +enablePatches(); export const controllerName = 'SnapController'; // TODO: Figure out how to name these @@ -218,6 +222,16 @@ export type PersistedSnapControllerState = SnapControllerState & { snapStates: Record; }; +type RollbackSnapshot = { + statePatches: Patch[]; + sourceCode: string | null; + permissions: { + revoked: unknown; + granted: unknown[]; + requestData: unknown; + }; +}; + // Controller Messenger Actions /** @@ -568,6 +582,7 @@ type SetSnapArgs = Omit & { */ // TODO(ritave): Used only for validation in #set, should be moved elsewhere. versionRange?: SemVerRange; + isUpdate?: boolean; }; const defaultState: SnapControllerState = { @@ -640,6 +655,8 @@ export class SnapController extends BaseController< #getAppKey: GetAppKey; + private readonly _rollbackSnapshots: Map; + #timeoutForLastRequestStatus?: number; #statusMachine!: StateMachine.Machine< @@ -733,7 +750,7 @@ export class SnapController extends BaseController< this._onOutboundRequest = this._onOutboundRequest.bind(this); this._onOutboundResponse = this._onOutboundResponse.bind(this); this.snapsRuntimeData = new Map(); - + this._rollbackSnapshots = new Map(); this.#pollForLastRequestStatus(); /* eslint-disable @typescript-eslint/unbound-method */ @@ -1586,6 +1603,17 @@ export class SnapController extends BaseController< ); } + const isUpdate = pendingUpdates.includes(snapId); + if (isUpdate) { + let rollbackSnapshot = this.getRollbackSnapshot(snapId); + if (rollbackSnapshot === undefined) { + const prevSourceCode = + this.#getRuntimeExpect(snapId).sourceCode; + rollbackSnapshot = this.createRollbackSnapshot(snapId); + rollbackSnapshot.sourceCode = prevSourceCode; + } + } + result[snapId] = await this.processRequestedSnap( origin, snapId, @@ -1771,8 +1799,14 @@ export class SnapController extends BaseController< manifest: newSnap.manifest, files: newSnap.files, versionRange: newVersionRange, + isUpdate: true, }); + // record revokePermissions to later grant for update rollback + // and record approvedNewPermissions to later revoke for update rollback + // when rolling back check if the permissions exist as the update might + // have errored out before the permissions were assigned + const unusedPermissionsKeys = Object.keys(unusedPermissions); if (isNonEmptyArray(unusedPermissionsKeys)) { this.messagingSystem.call('PermissionController:revokePermissions', { @@ -1788,6 +1822,16 @@ export class SnapController extends BaseController< }); } + const rollbackSnapshot = this.getRollbackSnapshot( + snapId, + ) as RollbackSnapshot; + rollbackSnapshot.permissions.revoked = unusedPermissions; + rollbackSnapshot.permissions.granted = Object.keys(approvedNewPermissions); + rollbackSnapshot.permissions.requestData = requestData; + + // add a try/catch block here to catch an error if the snap does not start in update situations + // we might need to add a flag that delineates between installs/updates calling this methodor ac + const sourceCode = newSnap.files .find( (file) => @@ -1795,10 +1839,8 @@ export class SnapController extends BaseController< ) ?.toString(); assert(sourceCode !== undefined); - await this.#startSnap({ - snapId, - sourceCode, - }); + + await this.#startSnap({ snapId, sourceCode }); const truncatedSnap = this.getTruncatedExpect(snapId); this.messagingSystem.publish( @@ -1959,6 +2001,7 @@ export class SnapController extends BaseController< manifest, files, versionRange = DEFAULT_REQUESTED_SNAP_VERSION, + isUpdate = false, } = args; assertIsSnapManifest(manifest.result); @@ -2020,10 +2063,19 @@ export class SnapController extends BaseController< delete snap.blockInformation; // store the snap back in state - this.update((state: any) => { + const [, , inversePatches] = this.update((state: any) => { state.snaps[snapId] = snap; }); + // checking for isUpdate here as this function is also used in + // the install flow, we do not care to create snapsshots for installs + if (isUpdate) { + const rollbackSnapshot = this.getRollbackSnapshot( + snapId, + ) as RollbackSnapshot; + rollbackSnapshot.statePatches = inversePatches; + } + const runtime = this.#getRuntimeExpect(snapId); runtime.sourceCode = sourceCode; @@ -2388,6 +2440,63 @@ export class SnapController extends BaseController< } } + private getRollbackSnapshot(snapId: SnapId): RollbackSnapshot | undefined { + return this._rollbackSnapshots.get(snapId); + } + + private createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { + assert( + this._rollbackSnapshots.get(snapId) === undefined, + new Error(`Snap "${snapId}" rollback snapshot already exists`), + ); + + this._rollbackSnapshots.set(snapId, { + statePatches: [], + sourceCode: '', + permissions: { revoked: null, granted: null, requestData: null }, + }); + return this.getRollbackSnapshot(snapId) as RollbackSnapshot; + } + + private async rollbackSnap(snapId: SnapId) { + // 1. First check if the rollbackSnapshot exists, if it doesn't throw an error + // 2. Then you can begin to pipe in statePatches, sourceCode and permissions into the appropriate functions + // 3. Make the appropriate checks to make sure you are + const rollbackSnapshot = this.getRollbackSnapshot(snapId); + if (!rollbackSnapshot) { + throw new Error('A snapshot does not exist for this snap'); + } + + const { statePatches, sourceCode, permissions } = rollbackSnapshot; + + if (statePatches?.length) { + this.applyPatches(rollbackSnapshot.statePatches); + } + + if (sourceCode) { + const runtime = this.#getRuntimeExpect(snapId); + runtime.sourceCode = rollbackSnapshot.sourceCode; + } + + if (permissions.revoked && Object.keys(permissions.revoked).length) { + this.messagingSystem.call('PermissionController:grantPermissions', { + approvedPermissions: rollbackSnapshot.permissions + .revoked as RequestedPermissions, + subject: { origin: snapId }, + requestData: rollbackSnapshot.permissions.requestData as Record< + string, + unknown + >, + }); + } + + if (permissions.granted?.length) { + this.messagingSystem.call('PermissionController:revokePermissions', { + [snapId]: permissions.granted as NonEmptyArray, + }); + } + } + #getRuntime(snapId: SnapId): SnapRuntimeData | undefined { return this.snapsRuntimeData.get(snapId); } From bdfb081d4482fde1c14f93378a2e62ac94afcc2e Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 29 Nov 2022 12:46:14 -0500 Subject: [PATCH 16/33] more updates --- .../src/snaps/SnapController.ts | 58 ++++++++++++++----- .../src/test-utils/controller.ts | 1 + 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 3166f2df52..91b0b7f56f 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -230,6 +230,7 @@ type RollbackSnapshot = { granted: unknown[]; requestData: unknown; }; + newVersion: string; }; // Controller Messenger Actions @@ -409,6 +410,13 @@ export type SnapUpdated = { payload: [snap: TruncatedSnap, oldVersion: string]; }; +/** + * Emitted when a snap is rolled back. + */ +export type SnapRolledback = { + type: `${typeof controllerName}:snapRolledback`; + payload: [snap: TruncatedSnap, failedVersion: string]; +}; /** * Emitted when a Snap is terminated. This is different from the snap being * stopped as it can also be triggered when a snap fails initialization. @@ -426,6 +434,7 @@ export type SnapControllerEvents = | SnapStateChange | SnapUnblocked | SnapUpdated + | SnapRolledback | SnapTerminated; export type AllowedActions = @@ -1611,6 +1620,9 @@ export class SnapController extends BaseController< this.#getRuntimeExpect(snapId).sourceCode; rollbackSnapshot = this.createRollbackSnapshot(snapId); rollbackSnapshot.sourceCode = prevSourceCode; + rollbackSnapshot.newVersion = version; + } else { + console.error('This snap is already being updated.'); } } @@ -1625,7 +1637,11 @@ export class SnapController extends BaseController< } catch (error) { const installed = pendingInstalls.filter((snapId) => this.has(snapId)); await this.removeSnaps(installed); - // TODO: Handle update roll backs + const snapshottedSnaps = [...this._rollbackSnapshots.keys()]; + const snapsToRollback = pendingUpdates.filter((snapId) => + snapshottedSnaps.includes(snapId), + ); + await this.rollbackSnaps(snapsToRollback); throw error; } @@ -1802,11 +1818,6 @@ export class SnapController extends BaseController< isUpdate: true, }); - // record revokePermissions to later grant for update rollback - // and record approvedNewPermissions to later revoke for update rollback - // when rolling back check if the permissions exist as the update might - // have errored out before the permissions were assigned - const unusedPermissionsKeys = Object.keys(unusedPermissions); if (isNonEmptyArray(unusedPermissionsKeys)) { this.messagingSystem.call('PermissionController:revokePermissions', { @@ -1840,7 +1851,11 @@ export class SnapController extends BaseController< ?.toString(); assert(sourceCode !== undefined); - await this.#startSnap({ snapId, sourceCode }); + try { + await this.#startSnap({ snapId, sourceCode }); + } catch { + throw new Error('Snap crashed on update.'); + } const truncatedSnap = this.getTruncatedExpect(snapId); this.messagingSystem.publish( @@ -2447,24 +2462,22 @@ export class SnapController extends BaseController< private createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { assert( this._rollbackSnapshots.get(snapId) === undefined, - new Error(`Snap "${snapId}" rollback snapshot already exists`), + new Error(`Snap "${snapId}" rollback snapshot already exists.`), ); this._rollbackSnapshots.set(snapId, { statePatches: [], sourceCode: '', - permissions: { revoked: null, granted: null, requestData: null }, + permissions: { revoked: null, granted: [], requestData: null }, + newVersion: '', }); return this.getRollbackSnapshot(snapId) as RollbackSnapshot; } private async rollbackSnap(snapId: SnapId) { - // 1. First check if the rollbackSnapshot exists, if it doesn't throw an error - // 2. Then you can begin to pipe in statePatches, sourceCode and permissions into the appropriate functions - // 3. Make the appropriate checks to make sure you are const rollbackSnapshot = this.getRollbackSnapshot(snapId); if (!rollbackSnapshot) { - throw new Error('A snapshot does not exist for this snap'); + throw new Error('A snapshot does not exist for this snap.'); } const { statePatches, sourceCode, permissions } = rollbackSnapshot; @@ -2480,8 +2493,7 @@ export class SnapController extends BaseController< if (permissions.revoked && Object.keys(permissions.revoked).length) { this.messagingSystem.call('PermissionController:grantPermissions', { - approvedPermissions: rollbackSnapshot.permissions - .revoked as RequestedPermissions, + approvedPermissions: permissions.revoked as RequestedPermissions, subject: { origin: snapId }, requestData: rollbackSnapshot.permissions.requestData as Record< string, @@ -2495,6 +2507,22 @@ export class SnapController extends BaseController< [snapId]: permissions.granted as NonEmptyArray, }); } + + await this.stopSnap(snapId, SnapStatusEvents.Stop); + + const truncatedSnap = this.getTruncatedExpect(snapId); + + this.messagingSystem.publish( + 'SnapController:snapRolledback', + truncatedSnap, + rollbackSnapshot.newVersion, + ); + } + + private async rollbackSnaps(snapIds: SnapId[]) { + for (const snapId of snapIds) { + await this.rollbackSnap(snapId); + } } #getRuntime(snapId: SnapId): SnapRuntimeData | undefined { diff --git a/packages/snaps-controllers/src/test-utils/controller.ts b/packages/snaps-controllers/src/test-utils/controller.ts index 6bc009e560..68a10728f5 100644 --- a/packages/snaps-controllers/src/test-utils/controller.ts +++ b/packages/snaps-controllers/src/test-utils/controller.ts @@ -186,6 +186,7 @@ export const getSnapControllerMessenger = ( 'SnapController:snapUpdated', 'SnapController:snapRemoved', 'SnapController:stateChange', + 'SnapController:snapRolledback', ], allowedActions: [ 'ApprovalController:addRequest', From 4b9902cd63a676993b128f0a60647c9f10123c6a Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Wed, 30 Nov 2022 17:58:54 -0500 Subject: [PATCH 17/33] add test --- .../src/snaps/SnapController.test.ts | 70 ++++++++++++++++++- .../src/snaps/SnapController.ts | 7 +- 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index b515fb35d1..68a8bc4bb6 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -29,7 +29,7 @@ import { } from '@metamask/snaps-utils/test-utils'; import { AssertionError } from '@metamask/utils'; import { Crypto } from '@peculiar/webcrypto'; -import { ethErrors } from 'eth-rpc-errors'; +import { EthereumRpcError, ethErrors } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; @@ -2366,7 +2366,7 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: { version: newVersionRange }, }), ).rejects.toThrow( - `Snap "${MOCK_SNAP_ID}@${manifest.version}" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, + `Snap "${MOCK_SNAP_ID}@0.9.0" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, ); expect(messenger.call).toHaveBeenCalledTimes(1); @@ -2420,6 +2420,72 @@ describe('SnapController', () => { expect.objectContaining({ versionRange: newVersionRange }), ); }); + + it('rollsback any updates & installs made during a failure scenario', async () => { + const snapId1 = 'npm:@metamask/example-snap1'; + const snapId2 = 'npm:@metamask/example-snap2'; + const snapId3 = 'npm:@metamask/example-snap3'; + const newVersion = '1.0.1'; + const messenger = getSnapControllerMessenger(); + const controller = getSnapController( + getSnapControllerOptions({ messenger }), + ); + + const fetchSnapMock = jest + .spyOn(controller as any, 'fetchSnap') + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: newVersion }), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: newVersion }), + sourceCode: 'foo', + }), + ); + + await controller.installSnaps(MOCK_ORIGIN, { [snapId1]: {} }); + await controller.installSnaps(MOCK_ORIGIN, { [snapId2]: {} }); + await controller.stopSnap(snapId1); + await controller.stopSnap(snapId2); + + expect(controller.get(snapId1)).toBeDefined(); + expect(controller.get(snapId2)).toBeDefined(); + + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [snapId3]: {}, + [snapId1]: { version: newVersion }, + [snapId2]: { version: newVersion }, + }), + ).rejects.toThrow(`Snap ${snapId2} crashed with updated source code.`); + + expect(fetchSnapMock).toHaveBeenCalledTimes(5); + + expect(controller.get(snapId3)).toBeUndefined(); + expect(controller.get(snapId1)?.manifest.version).toBe('1.0.0'); + expect(controller.get(snapId2)?.manifest.version).toBe('1.0.0'); + }); }); describe('updateSnap', () => { diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 91b0b7f56f..b21fa978ea 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1680,6 +1680,7 @@ export class SnapController extends BaseController< versionRange, location, ); + console.log('update result is:', updateResult); if (updateResult === null) { throw ethErrors.rpc.invalidParams( `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, @@ -1854,7 +1855,7 @@ export class SnapController extends BaseController< try { await this.#startSnap({ snapId, sourceCode }); } catch { - throw new Error('Snap crashed on update.'); + throw new Error(`Snap ${snapId} crashed with updated source code.`); } const truncatedSnap = this.getTruncatedExpect(snapId); @@ -2078,12 +2079,12 @@ export class SnapController extends BaseController< delete snap.blockInformation; // store the snap back in state - const [, , inversePatches] = this.update((state: any) => { + const { inversePatches } = this.update((state: any) => { state.snaps[snapId] = snap; }); // checking for isUpdate here as this function is also used in - // the install flow, we do not care to create snapsshots for installs + // the install flow, we do not care to create snapshots for installs if (isUpdate) { const rollbackSnapshot = this.getRollbackSnapshot( snapId, From 221aef4ced49992a2514f9dab6a47182bb5e2067 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Wed, 30 Nov 2022 20:22:29 -0500 Subject: [PATCH 18/33] refactors --- .../src/snaps/SnapController.test.ts | 2 +- .../snaps-controllers/src/snaps/SnapController.ts | 12 ++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 68a8bc4bb6..3a0a76ca91 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2366,7 +2366,7 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: { version: newVersionRange }, }), ).rejects.toThrow( - `Snap "${MOCK_SNAP_ID}@0.9.0" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, + `Snap "${MOCK_SNAP_ID}@1.0.0" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, ); expect(messenger.call).toHaveBeenCalledTimes(1); diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index b21fa978ea..a571aa8820 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1594,7 +1594,7 @@ export class SnapController extends BaseController< const [error, version] = resolveVersionRange(rawVersion); if (error) { throw ethErrors.rpc.invalidParams( - `The "version" field must be a valid SemVer version range if specified. Received: "${version}".`, + `The "version" field must be a valid SemVer version range if specified. Received: "${rawVersion}".`, ); } @@ -1680,7 +1680,6 @@ export class SnapController extends BaseController< versionRange, location, ); - console.log('update result is:', updateResult); if (updateResult === null) { throw ethErrors.rpc.invalidParams( `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, @@ -2484,22 +2483,19 @@ export class SnapController extends BaseController< const { statePatches, sourceCode, permissions } = rollbackSnapshot; if (statePatches?.length) { - this.applyPatches(rollbackSnapshot.statePatches); + this.applyPatches(statePatches); } if (sourceCode) { const runtime = this.#getRuntimeExpect(snapId); - runtime.sourceCode = rollbackSnapshot.sourceCode; + runtime.sourceCode = sourceCode; } if (permissions.revoked && Object.keys(permissions.revoked).length) { this.messagingSystem.call('PermissionController:grantPermissions', { approvedPermissions: permissions.revoked as RequestedPermissions, subject: { origin: snapId }, - requestData: rollbackSnapshot.permissions.requestData as Record< - string, - unknown - >, + requestData: permissions.requestData as Record, }); } From 43dfe1b045afa8589eaeba099bedca803ac18134 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Wed, 30 Nov 2022 20:49:09 -0500 Subject: [PATCH 19/33] fixed some more errors --- .../snaps-controllers/src/snaps/SnapController.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index a571aa8820..f2184e5e0c 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1836,9 +1836,13 @@ export class SnapController extends BaseController< const rollbackSnapshot = this.getRollbackSnapshot( snapId, ) as RollbackSnapshot; - rollbackSnapshot.permissions.revoked = unusedPermissions; - rollbackSnapshot.permissions.granted = Object.keys(approvedNewPermissions); - rollbackSnapshot.permissions.requestData = requestData; + if (rollbackSnapshot !== undefined) { + rollbackSnapshot.permissions.revoked = unusedPermissions; + rollbackSnapshot.permissions.granted = Object.keys( + approvedNewPermissions, + ); + rollbackSnapshot.permissions.requestData = requestData; + } // add a try/catch block here to catch an error if the snap does not start in update situations // we might need to add a flag that delineates between installs/updates calling this methodor ac @@ -2088,7 +2092,9 @@ export class SnapController extends BaseController< const rollbackSnapshot = this.getRollbackSnapshot( snapId, ) as RollbackSnapshot; - rollbackSnapshot.statePatches = inversePatches; + if (rollbackSnapshot !== undefined) { + rollbackSnapshot.statePatches = inversePatches; + } } const runtime = this.#getRuntimeExpect(snapId); From 8d8502b53337baba2601cf1b304a58c2a7ae69c9 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 1 Dec 2022 10:27:19 +0100 Subject: [PATCH 20/33] Fix test --- .../src/snaps/SnapController.test.ts | 5 ++--- .../snaps-controllers/src/test-utils/controller.ts | 11 +++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 3a0a76ca91..192353f5a0 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2426,9 +2426,8 @@ describe('SnapController', () => { const snapId2 = 'npm:@metamask/example-snap2'; const snapId3 = 'npm:@metamask/example-snap3'; const newVersion = '1.0.1'; - const messenger = getSnapControllerMessenger(); - const controller = getSnapController( - getSnapControllerOptions({ messenger }), + const [controller] = getSnapControllerWithEES( + getSnapControllerWithEESOptions(), ); const fetchSnapMock = jest diff --git a/packages/snaps-controllers/src/test-utils/controller.ts b/packages/snaps-controllers/src/test-utils/controller.ts index 68a10728f5..7946c54dc1 100644 --- a/packages/snaps-controllers/src/test-utils/controller.ts +++ b/packages/snaps-controllers/src/test-utils/controller.ts @@ -283,11 +283,22 @@ export const getSnapControllerWithEESOptions = ({ const snapControllerMessenger = getSnapControllerMessenger(rootMessenger); return { + featureFlags: { dappsCanUpdateSnaps: true }, environmentEndowmentPermissions: [], closeAllConnections: jest.fn(), getAppKey: jest .fn() .mockImplementation((snapId, appKeyType) => `${appKeyType}:${snapId}`), + checkBlockList: jest + .fn() + .mockImplementation(async (snaps: CheckSnapBlockListArg) => + Promise.resolve( + Object.keys(snaps).reduce( + (acc, snapId) => ({ ...acc, [snapId]: { blocked: false } }), + {}, + ), + ), + ), messenger: snapControllerMessenger, rootMessenger, ...options, From 5722caf6876cb79967f32efca981a3ec6f56770f Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Thu, 1 Dec 2022 11:41:53 -0500 Subject: [PATCH 21/33] updated test --- .../snaps-controllers/src/snaps/SnapController.test.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 192353f5a0..be34ecdb54 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2425,8 +2425,9 @@ describe('SnapController', () => { const snapId1 = 'npm:@metamask/example-snap1'; const snapId2 = 'npm:@metamask/example-snap2'; const snapId3 = 'npm:@metamask/example-snap3'; + const oldVersion = '1.0.0'; const newVersion = '1.0.1'; - const [controller] = getSnapControllerWithEES( + const [controller, service] = getSnapControllerWithEES( getSnapControllerWithEESOptions(), ); @@ -2482,8 +2483,11 @@ describe('SnapController', () => { expect(fetchSnapMock).toHaveBeenCalledTimes(5); expect(controller.get(snapId3)).toBeUndefined(); - expect(controller.get(snapId1)?.manifest.version).toBe('1.0.0'); - expect(controller.get(snapId2)?.manifest.version).toBe('1.0.0'); + expect(controller.get(snapId1)?.manifest.version).toBe(oldVersion); + expect(controller.get(snapId2)?.manifest.version).toBe(oldVersion); + + controller.destroy(); + await service.terminateAllSnaps(); }); }); From ba3830c75328d63b6dffbaa95ab42f34c38695da Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Thu, 1 Dec 2022 11:59:33 -0500 Subject: [PATCH 22/33] reverted some inadvertent changes --- packages/snaps-controllers/src/snaps/SnapController.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index be34ecdb54..553636ff7f 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -29,7 +29,7 @@ import { } from '@metamask/snaps-utils/test-utils'; import { AssertionError } from '@metamask/utils'; import { Crypto } from '@peculiar/webcrypto'; -import { EthereumRpcError, ethErrors } from 'eth-rpc-errors'; +import { ethErrors } from 'eth-rpc-errors'; import fetchMock from 'jest-fetch-mock'; import { createAsyncMiddleware, JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; From 531d530f9f60e346da327838aba9765a2324c2b5 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Thu, 1 Dec 2022 12:17:23 -0500 Subject: [PATCH 23/33] removed unnecessary immer usage --- packages/snaps-controllers/src/snaps/SnapController.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index f2184e5e0c..a131fdd768 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -75,7 +75,6 @@ import { import { createMachine, interpret, StateMachine } from '@xstate/fsm'; import { ethErrors } from 'eth-rpc-errors'; import type { Patch } from 'immer'; -import { enablePatches } from 'immer'; import { nanoid } from 'nanoid'; import { forceStrict, validateMachine } from '../fsm'; @@ -98,7 +97,6 @@ import { detectSnapLocation, SnapLocation } from './location'; import { RequestQueue } from './RequestQueue'; import { Timer } from './Timer'; -enablePatches(); export const controllerName = 'SnapController'; // TODO: Figure out how to name these From df30e50a47bd10cafdda0c10992ef9bc87065d83 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Mon, 5 Dec 2022 21:13:46 -0500 Subject: [PATCH 24/33] address PR comments --- .../src/snaps/SnapController.test.ts | 83 ++++++++++++++++++- .../src/snaps/SnapController.ts | 66 +++++++++------ 2 files changed, 121 insertions(+), 28 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 553636ff7f..4f795a4c51 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -11,6 +11,7 @@ import { HandlerType, SemVerRange, SemVerVersion, + SemVerRange, SnapCaveatType, SnapStatus, VirtualFile, @@ -2421,7 +2422,7 @@ describe('SnapController', () => { ); }); - it('rollsback any updates & installs made during a failure scenario', async () => { + it('rolls back any updates & installs made during a failure scenario', async () => { const snapId1 = 'npm:@metamask/example-snap1'; const snapId2 = 'npm:@metamask/example-snap2'; const snapId3 = 'npm:@metamask/example-snap3'; @@ -2489,6 +2490,82 @@ describe('SnapController', () => { controller.destroy(); await service.terminateAllSnaps(); }); + + it('will not create snapshots for already installed snaps that have invalid requested ranges', async () => { + const snapId1 = 'npm:@metamask/example-snap1'; + const snapId2 = 'npm:@metamask/example-snap2'; + const snapId3 = 'npm:@metamask/example-snap3'; + const oldVersion = '1.0.0'; + const newVersion = '1.0.1'; + const olderVersion = '0.9.0'; + const options = getSnapControllerWithEESOptions(); + const { messenger } = options; + const [controller, service] = getSnapControllerWithEES(options); + + const listener = jest.fn(); + messenger.subscribe('SnapController:snapRolledback' as any, listener); + + const fetchSnapMock = jest + .spyOn(controller as any, 'fetchSnap') + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest(), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: olderVersion }), + sourceCode: DEFAULT_SNAP_BUNDLE, + }), + ) + .mockImplementationOnce(async () => + Promise.resolve({ + manifest: getSnapManifest({ version: newVersion }), + sourceCode: 'foo', + }), + ); + + await controller.installSnaps(MOCK_ORIGIN, { [snapId1]: {} }); + await controller.installSnaps(MOCK_ORIGIN, { [snapId2]: {} }); + await controller.stopSnap(snapId1); + await controller.stopSnap(snapId2); + + expect(controller.get(snapId1)).toBeDefined(); + expect(controller.get(snapId2)).toBeDefined(); + + await expect( + controller.installSnaps(MOCK_ORIGIN, { + [snapId3]: {}, + [snapId1]: { version: olderVersion }, + [snapId2]: { version: newVersion }, + }), + ).rejects.toThrow( + `Snap "${snapId1}@${oldVersion}" is already installed, couldn't update to a version inside requested "${olderVersion}" range.`, + ); + + expect(fetchSnapMock).toHaveBeenCalledTimes(5); + + expect(controller.get(snapId3)).toBeUndefined(); + expect(controller.get(snapId1)?.manifest.version).toBe(oldVersion); + expect(controller.get(snapId2)?.manifest.version).toBe(oldVersion); + expect(listener).toHaveBeenCalledTimes(1); + + controller.destroy(); + await service.terminateAllSnaps(); + }); }); describe('updateSnap', () => { @@ -2511,9 +2588,9 @@ describe('SnapController', () => { controller.updateSnap( MOCK_ORIGIN, MOCK_SNAP_ID, - 'this is not a version', + 'this is not a version' as SemVerRange, ), - ).rejects.toThrow('Received invalid snap version range'); + ).rejects.toThrow('Received invalid Snap version range.'); }); it('throws an error if the new version of the snap is blocked', async () => { diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index a131fdd768..ee7b2c99ec 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -29,6 +29,7 @@ import { fromEntries, getSnapPermissionName, gtVersion, + gtRange, InstallSnapsResult, isValidSemVerRange, PersistedSnap, @@ -660,9 +661,9 @@ export class SnapController extends BaseController< // This property cannot be hash private yet because of tests. private readonly snapsRuntimeData: Map; - #getAppKey: GetAppKey; + #rollbackSnapshots: Map; - private readonly _rollbackSnapshots: Map; + #getAppKey: GetAppKey; #timeoutForLastRequestStatus?: number; @@ -756,8 +757,8 @@ export class SnapController extends BaseController< this._onUnhandledSnapError = this._onUnhandledSnapError.bind(this); this._onOutboundRequest = this._onOutboundRequest.bind(this); this._onOutboundResponse = this._onOutboundResponse.bind(this); + this.#rollbackSnapshots = new Map(); this.snapsRuntimeData = new Map(); - this._rollbackSnapshots = new Map(); this.#pollForLastRequestStatus(); /* eslint-disable @typescript-eslint/unbound-method */ @@ -1611,12 +1612,13 @@ export class SnapController extends BaseController< } const isUpdate = pendingUpdates.includes(snapId); - if (isUpdate) { - let rollbackSnapshot = this.getRollbackSnapshot(snapId); + + if (isUpdate && this.#isValidUpdate(snapId, version)) { + let rollbackSnapshot = this.#getRollbackSnapshot(snapId); if (rollbackSnapshot === undefined) { const prevSourceCode = this.#getRuntimeExpect(snapId).sourceCode; - rollbackSnapshot = this.createRollbackSnapshot(snapId); + rollbackSnapshot = this.#createRollbackSnapshot(snapId); rollbackSnapshot.sourceCode = prevSourceCode; rollbackSnapshot.newVersion = version; } else { @@ -1632,14 +1634,15 @@ export class SnapController extends BaseController< }, ), ); + snapIds.forEach((snapId) => this.#rollbackSnapshots.delete(snapId)); } catch (error) { const installed = pendingInstalls.filter((snapId) => this.has(snapId)); await this.removeSnaps(installed); - const snapshottedSnaps = [...this._rollbackSnapshots.keys()]; + const snapshottedSnaps = [...this.#rollbackSnapshots.keys()]; const snapsToRollback = pendingUpdates.filter((snapId) => snapshottedSnaps.includes(snapId), ); - await this.rollbackSnaps(snapsToRollback); + await this.#rollbackSnaps(snapsToRollback); throw error; } @@ -1715,9 +1718,6 @@ export class SnapController extends BaseController< return truncated; } catch (error) { console.error(`Error when adding snap.`, error); - if (this.has(snapId)) { - await this.removeSnap(snapId); - } throw error; } @@ -1831,7 +1831,7 @@ export class SnapController extends BaseController< }); } - const rollbackSnapshot = this.getRollbackSnapshot( + const rollbackSnapshot = this.#getRollbackSnapshot( snapId, ) as RollbackSnapshot; if (rollbackSnapshot !== undefined) { @@ -2087,7 +2087,7 @@ export class SnapController extends BaseController< // checking for isUpdate here as this function is also used in // the install flow, we do not care to create snapshots for installs if (isUpdate) { - const rollbackSnapshot = this.getRollbackSnapshot( + const rollbackSnapshot = this.#getRollbackSnapshot( snapId, ) as RollbackSnapshot; if (rollbackSnapshot !== undefined) { @@ -2459,31 +2459,33 @@ export class SnapController extends BaseController< } } - private getRollbackSnapshot(snapId: SnapId): RollbackSnapshot | undefined { - return this._rollbackSnapshots.get(snapId); + #getRollbackSnapshot(snapId: SnapId): RollbackSnapshot | undefined { + return this.#rollbackSnapshots.get(snapId); } - private createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { + #createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { assert( - this._rollbackSnapshots.get(snapId) === undefined, + this.#rollbackSnapshots.get(snapId) === undefined, new Error(`Snap "${snapId}" rollback snapshot already exists.`), ); - this._rollbackSnapshots.set(snapId, { + this.#rollbackSnapshots.set(snapId, { statePatches: [], sourceCode: '', permissions: { revoked: null, granted: [], requestData: null }, newVersion: '', }); - return this.getRollbackSnapshot(snapId) as RollbackSnapshot; + return this.#getRollbackSnapshot(snapId) as RollbackSnapshot; } - private async rollbackSnap(snapId: SnapId) { - const rollbackSnapshot = this.getRollbackSnapshot(snapId); + async #rollbackSnap(snapId: SnapId) { + const rollbackSnapshot = this.#getRollbackSnapshot(snapId); if (!rollbackSnapshot) { throw new Error('A snapshot does not exist for this snap.'); } + await this.stopSnap(snapId, SnapStatusEvents.Stop); + const { statePatches, sourceCode, permissions } = rollbackSnapshot; if (statePatches?.length) { @@ -2509,8 +2511,6 @@ export class SnapController extends BaseController< }); } - await this.stopSnap(snapId, SnapStatusEvents.Stop); - const truncatedSnap = this.getTruncatedExpect(snapId); this.messagingSystem.publish( @@ -2518,11 +2518,13 @@ export class SnapController extends BaseController< truncatedSnap, rollbackSnapshot.newVersion, ); + + this.#rollbackSnapshots.delete(snapId); } - private async rollbackSnaps(snapIds: SnapId[]) { + async #rollbackSnaps(snapIds: SnapId[]) { for (const snapId of snapIds) { - await this.rollbackSnap(snapId); + await this.#rollbackSnap(snapId); } } @@ -2599,4 +2601,18 @@ export class SnapController extends BaseController< return { newPermissions, unusedPermissions, approvedPermissions }; } + + #isValidUpdate(snapId: SnapId, newVersionRange: SemVerRange) { + const existingSnap = this.getExpect(snapId); + + if (satisfiesVersionRange(existingSnap.version, newVersionRange)) { + return false; + } + + if (gtRange(existingSnap.version, newVersionRange)) { + return false; + } + + return true; + } } From c35c39f1de66bfffeb9e955043b163544c5353e8 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Mon, 5 Dec 2022 21:24:23 -0500 Subject: [PATCH 25/33] add semver util --- packages/snaps-utils/src/versions.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/snaps-utils/src/versions.ts b/packages/snaps-utils/src/versions.ts index 8fcd36553a..f8b1a936ae 100644 --- a/packages/snaps-utils/src/versions.ts +++ b/packages/snaps-utils/src/versions.ts @@ -1,6 +1,7 @@ import { assertStruct, Json } from '@metamask/utils'; import { gt as gtSemver, + gtr, maxSatisfying as maxSatisfyingSemver, satisfies as satisfiesSemver, valid as validSemVerVersion, @@ -141,6 +142,17 @@ export function gtVersion( return gtSemver(version1, version2); } +/** + * Checks whether a SemVer version is greater than all possibilities in a range. + * + * @param version - A SemvVer version. + * @param range - The range to check against. + * @returns `version > range`. + */ +export function gtRange(version: SemVerVersion, range: SemVerRange): boolean { + return gtr(version, range); +} + /** * Returns whether a SemVer version satisfies a SemVer range. * From 35934be3bf513c6d553bd29eed235d32b3a44fa4 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Tue, 6 Dec 2022 10:39:09 +0100 Subject: [PATCH 26/33] Stop running snap installs in parallel --- .../src/snaps/SnapController.test.ts | 4 +- .../src/snaps/SnapController.ts | 93 +++++++++---------- 2 files changed, 48 insertions(+), 49 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 4f795a4c51..9fcec6eb7e 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2556,12 +2556,12 @@ describe('SnapController', () => { `Snap "${snapId1}@${oldVersion}" is already installed, couldn't update to a version inside requested "${olderVersion}" range.`, ); - expect(fetchSnapMock).toHaveBeenCalledTimes(5); + expect(fetchSnapMock).toHaveBeenCalledTimes(4); expect(controller.get(snapId3)).toBeUndefined(); expect(controller.get(snapId1)?.manifest.version).toBe(oldVersion); expect(controller.get(snapId2)?.manifest.version).toBe(oldVersion); - expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledTimes(0); controller.destroy(); await service.terminateAllSnaps(); diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index ee7b2c99ec..dce67dd5fd 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1587,53 +1587,52 @@ export class SnapController extends BaseController< ); try { - await Promise.all( - Object.entries(requestedSnaps).map( - async ([snapId, { version: rawVersion }]) => { - const [error, version] = resolveVersionRange(rawVersion); - if (error) { - throw ethErrors.rpc.invalidParams( - `The "version" field must be a valid SemVer version range if specified. Received: "${rawVersion}".`, - ); - } - - const permissionName = getSnapPermissionName(snapId); - - if ( - !this.messagingSystem.call( - 'PermissionController:hasPermission', - origin, - permissionName, - ) - ) { - throw ethErrors.provider.unauthorized( - `Not authorized to install snap "${snapId}". Request the permission for the snap before attempting to install it.`, - ); - } - - const isUpdate = pendingUpdates.includes(snapId); - - if (isUpdate && this.#isValidUpdate(snapId, version)) { - let rollbackSnapshot = this.#getRollbackSnapshot(snapId); - if (rollbackSnapshot === undefined) { - const prevSourceCode = - this.#getRuntimeExpect(snapId).sourceCode; - rollbackSnapshot = this.#createRollbackSnapshot(snapId); - rollbackSnapshot.sourceCode = prevSourceCode; - rollbackSnapshot.newVersion = version; - } else { - console.error('This snap is already being updated.'); - } - } - - result[snapId] = await this.processRequestedSnap( - origin, - snapId, - version, - ); - }, - ), - ); + for (const entry of Object.entries(requestedSnaps)) { + const snapId = entry[0]; + const { version: rawVersion } = entry[1]; + + const [error, version] = resolveVersionRange(rawVersion); + + if (error) { + throw ethErrors.rpc.invalidParams( + `The "version" field must be a valid SemVer version range if specified. Received: "${rawVersion}".`, + ); + } + + const permissionName = getSnapPermissionName(snapId); + + if ( + !this.messagingSystem.call( + 'PermissionController:hasPermission', + origin, + permissionName, + ) + ) { + throw ethErrors.provider.unauthorized( + `Not authorized to install snap "${snapId}". Request the permission for the snap before attempting to install it.`, + ); + } + + const isUpdate = pendingUpdates.includes(snapId); + + if (isUpdate && this.#isValidUpdate(snapId, version)) { + let rollbackSnapshot = this.#getRollbackSnapshot(snapId); + if (rollbackSnapshot === undefined) { + const prevSourceCode = this.#getRuntimeExpect(snapId).sourceCode; + rollbackSnapshot = this.#createRollbackSnapshot(snapId); + rollbackSnapshot.sourceCode = prevSourceCode; + rollbackSnapshot.newVersion = version; + } else { + console.error('This snap is already being updated.'); + } + } + + result[snapId] = await this.processRequestedSnap( + origin, + snapId, + version, + ); + } snapIds.forEach((snapId) => this.#rollbackSnapshots.delete(snapId)); } catch (error) { const installed = pendingInstalls.filter((snapId) => this.has(snapId)); From ad502f7d45d08e73731042eadf6b8f535555ba6a Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Tue, 6 Dec 2022 11:21:05 +0100 Subject: [PATCH 27/33] Fix tests after rebase --- packages/snaps-controllers/jest.config.js | 2 +- .../src/snaps/SnapController.test.ts | 126 ++++++++---------- 2 files changed, 55 insertions(+), 73 deletions(-) diff --git a/packages/snaps-controllers/jest.config.js b/packages/snaps-controllers/jest.config.js index 3200d016cc..8d1cf377b5 100644 --- a/packages/snaps-controllers/jest.config.js +++ b/packages/snaps-controllers/jest.config.js @@ -5,7 +5,7 @@ const baseConfig = require('../../jest.config.base'); module.exports = deepmerge(baseConfig, { coverageThreshold: { global: { - branches: 90.85, + branches: 90.81, functions: 97.45, lines: 97.45, statements: 97.45, diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 9fcec6eb7e..b16aa086ea 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -11,7 +11,6 @@ import { HandlerType, SemVerRange, SemVerVersion, - SemVerRange, SnapCaveatType, SnapStatus, VirtualFile, @@ -2361,7 +2360,6 @@ describe('SnapController', () => { }), ); - const manifest = getSnapManifest(); await expect( controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: { version: newVersionRange }, @@ -2428,43 +2426,43 @@ describe('SnapController', () => { const snapId3 = 'npm:@metamask/example-snap3'; const oldVersion = '1.0.0'; const newVersion = '1.0.1'; - const [controller, service] = getSnapControllerWithEES( - getSnapControllerWithEESOptions(), - ); - const fetchSnapMock = jest - .spyOn(controller as any, 'fetchSnap') - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ version: newVersion }), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), + const manifest = getSnapManifest(); + const detect = jest + .fn() + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ version: newVersion }), + }), ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ version: newVersion }), - sourceCode: 'foo', - }), + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ + version: newVersion, + shasum: getSnapSourceShasum('foo'), + }), + files: [ + new VirtualFile({ + value: 'foo', + path: manifest.source.location.npm.filePath, + }), + new VirtualFile({ + value: DEFAULT_SNAP_ICON, + path: manifest.source.location.npm.iconPath, + }), + ], + }), ); + const [controller, service] = getSnapControllerWithEES( + getSnapControllerWithEESOptions({ detectSnapLocation: detect }), + ); + await controller.installSnaps(MOCK_ORIGIN, { [snapId1]: {} }); await controller.installSnaps(MOCK_ORIGIN, { [snapId2]: {} }); await controller.stopSnap(snapId1); @@ -2481,7 +2479,7 @@ describe('SnapController', () => { }), ).rejects.toThrow(`Snap ${snapId2} crashed with updated source code.`); - expect(fetchSnapMock).toHaveBeenCalledTimes(5); + expect(detect).toHaveBeenCalledTimes(5); expect(controller.get(snapId3)).toBeUndefined(); expect(controller.get(snapId1)?.manifest.version).toBe(oldVersion); @@ -2498,46 +2496,28 @@ describe('SnapController', () => { const oldVersion = '1.0.0'; const newVersion = '1.0.1'; const olderVersion = '0.9.0'; - const options = getSnapControllerWithEESOptions(); + + const detect = jest + .fn() + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: getSnapManifest({ version: olderVersion }), + }), + ); + + const options = getSnapControllerWithEESOptions({ + detectSnapLocation: detect, + }); const { messenger } = options; const [controller, service] = getSnapControllerWithEES(options); const listener = jest.fn(); messenger.subscribe('SnapController:snapRolledback' as any, listener); - const fetchSnapMock = jest - .spyOn(controller as any, 'fetchSnap') - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest(), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ version: olderVersion }), - sourceCode: DEFAULT_SNAP_BUNDLE, - }), - ) - .mockImplementationOnce(async () => - Promise.resolve({ - manifest: getSnapManifest({ version: newVersion }), - sourceCode: 'foo', - }), - ); - await controller.installSnaps(MOCK_ORIGIN, { [snapId1]: {} }); await controller.installSnaps(MOCK_ORIGIN, { [snapId2]: {} }); await controller.stopSnap(snapId1); @@ -2556,7 +2536,7 @@ describe('SnapController', () => { `Snap "${snapId1}@${oldVersion}" is already installed, couldn't update to a version inside requested "${olderVersion}" range.`, ); - expect(fetchSnapMock).toHaveBeenCalledTimes(4); + expect(detect).toHaveBeenCalledTimes(4); expect(controller.get(snapId3)).toBeUndefined(); expect(controller.get(snapId1)?.manifest.version).toBe(oldVersion); @@ -2590,7 +2570,9 @@ describe('SnapController', () => { MOCK_SNAP_ID, 'this is not a version' as SemVerRange, ), - ).rejects.toThrow('Received invalid Snap version range.'); + ).rejects.toThrow( + 'Received invalid snap version range: "this is not a version"', + ); }); it('throws an error if the new version of the snap is blocked', async () => { From d77296c0828e320411933a4da05a14f286bc16e7 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 6 Dec 2022 09:12:42 -0500 Subject: [PATCH 28/33] address more PR comments --- .../src/snaps/SnapController.test.ts | 8 +- .../src/snaps/SnapController.ts | 77 +++++++++++++++---- packages/snaps-utils/src/snaps.ts | 2 +- packages/snaps-utils/src/versions.ts | 4 +- 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index a72468df75..6c16bbe84f 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -627,7 +627,7 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: { version: 'foo' }, }), ).rejects.toThrow( - 'The "version" field must be a valid SemVer version range if specified. Received: "foo"', + 'The "version" field must be a valid SemVer version range if specified. Received: "foo".', ); }); @@ -2217,7 +2217,7 @@ describe('SnapController', () => { controller.installSnaps(MOCK_ORIGIN, { [snapId]: {}, }), - ).rejects.toThrow('Invalid snap id. Unknown prefix. Received: "foo"'); + ).rejects.toThrow('Invalid snap id. Unknown prefix. Received: "foo".'); }); it('updates a snap', async () => { @@ -2420,7 +2420,7 @@ describe('SnapController', () => { ); }); - it('rolls back any updates & installs made during a failure scenario', async () => { + it('rolls back any updates and installs made during a failure scenario', async () => { const snapId1 = 'npm:@metamask/example-snap1'; const snapId2 = 'npm:@metamask/example-snap2'; const snapId3 = 'npm:@metamask/example-snap3'; @@ -2571,7 +2571,7 @@ describe('SnapController', () => { 'this is not a version' as SemVerRange, ), ).rejects.toThrow( - 'Received invalid snap version range: "this is not a version"', + 'Received invalid snap version range: "this is not a version".', ); }); diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 32107b9940..5b8abc4588 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1588,10 +1588,9 @@ export class SnapController extends BaseController< ); try { - for (const entry of Object.entries(requestedSnaps)) { - const snapId = entry[0]; - const { version: rawVersion } = entry[1]; - + for (const [snapId, { version: rawVersion }] of Object.entries( + requestedSnaps, + )) { const [error, version] = resolveVersionRange(rawVersion); if (error) { @@ -1624,7 +1623,7 @@ export class SnapController extends BaseController< rollbackSnapshot.sourceCode = prevSourceCode; rollbackSnapshot.newVersion = version; } else { - console.error('This snap is already being updated.'); + throw new Error('This snap is already being updated.'); } } @@ -1683,13 +1682,13 @@ export class SnapController extends BaseController< ); if (updateResult === null) { throw ethErrors.rpc.invalidParams( - `Snap "${snapId}@${existingSnap.version}" is already installed, couldn't update to a version inside requested "${versionRange}" range.`, + `Snap "${snapId}@${existingSnap.version}" is already installed. Couldn't update to a version inside requested "${versionRange}" range.`, ); } return updateResult; } throw ethErrors.rpc.invalidParams( - `Version mismatch with already installed snap. ${snapId}@${existingSnap.version} doesn't satisfy requested version ${versionRange}`, + `Version mismatch with already installed snap. ${snapId}@${existingSnap.version} doesn't satisfy requested version ${versionRange}.`, ); } @@ -1842,9 +1841,6 @@ export class SnapController extends BaseController< rollbackSnapshot.permissions.requestData = requestData; } - // add a try/catch block here to catch an error if the snap does not start in update situations - // we might need to add a flag that delineates between installs/updates calling this methodor ac - const sourceCode = newSnap.files .find( (file) => @@ -2087,9 +2083,7 @@ export class SnapController extends BaseController< // checking for isUpdate here as this function is also used in // the install flow, we do not care to create snapshots for installs if (isUpdate) { - const rollbackSnapshot = this.#getRollbackSnapshot( - snapId, - ) as RollbackSnapshot; + const rollbackSnapshot = this.#getRollbackSnapshot(snapId); if (rollbackSnapshot !== undefined) { rollbackSnapshot.statePatches = inversePatches; } @@ -2459,10 +2453,24 @@ export class SnapController extends BaseController< } } + /** + * Retrieves the rollback snapshot of a snap. + * + * @param snapId - The snap id. + * @returns A `RollbackSnapshot` or `undefined` if one doesn't exist. + */ #getRollbackSnapshot(snapId: SnapId): RollbackSnapshot | undefined { return this.#rollbackSnapshots.get(snapId); } + /** + * Creates a `RollbackSnapshot` that is used to help ensure + * atomicity in multiple snap updates. + * + * @param snapId - The snap id. + * @throws {@link Error}. If the snap exists before creation or if creation fails. + * @returns A `RollbackSnapshot`. + */ #createRollbackSnapshot(snapId: SnapId): RollbackSnapshot { assert( this.#rollbackSnapshots.get(snapId) === undefined, @@ -2475,9 +2483,27 @@ export class SnapController extends BaseController< permissions: { revoked: null, granted: [], requestData: null }, newVersion: '', }); - return this.#getRollbackSnapshot(snapId) as RollbackSnapshot; + + const newRollbackSnapshot = this.#rollbackSnapshots.get(snapId); + + assert( + newRollbackSnapshot !== undefined, + new Error(`Snapshot creation failed for ${snapId}.`), + ); + return newRollbackSnapshot; } + /** + * Rolls back a snap to its previous state, permissions + * and source code based on the `RollbackSnapshot` that + * is captured during the update process. After rolling back, + * the function also emits an event indicating that the + * snap has been rolled back and it clears the snapshot + * for that snap. + * + * @param snapId - The snap id. + * @throws {@link Error}. If a snapshot does not exist. + */ async #rollbackSnap(snapId: SnapId) { const rollbackSnapshot = this.#getRollbackSnapshot(snapId); if (!rollbackSnapshot) { @@ -2522,6 +2548,12 @@ export class SnapController extends BaseController< this.#rollbackSnapshots.delete(snapId); } + /** + * Iterates through an array of snap ids + * and calls `rollbackSnap` on them. + * + * @param snapIds - An array of snap ids. + */ async #rollbackSnaps(snapIds: SnapId[]) { for (const snapId of snapIds) { await this.#rollbackSnap(snapId); @@ -2602,7 +2634,22 @@ export class SnapController extends BaseController< return { newPermissions, unusedPermissions, approvedPermissions }; } - #isValidUpdate(snapId: SnapId, newVersionRange: SemVerRange) { + /** + * Checks if a snap will pass version validation checks + * with the new version range that is requested. The first + * check that is done is to check if the existing snap version + * falls inside the requested range, if it does we want to return + * false because we do not care to create a rollback snapshot in + * that scenario. The second check is to ensure that the current + * snap version is not greater than all possible versions in + * the requested version range, if it is then we also want + * to return false in that scenario. + * + * @param snapId - The snap id. + * @param newVersionRange - The new version range being requsted. + * @returns `true` if validation checks pass and `false` if they do not. + */ + #isValidUpdate(snapId: SnapId, newVersionRange: SemVerRange): boolean { const existingSnap = this.getExpect(snapId); if (satisfiesVersionRange(existingSnap.version, newVersionRange)) { diff --git a/packages/snaps-utils/src/snaps.ts b/packages/snaps-utils/src/snaps.ts index bcd80b1691..18ae1b4e50 100644 --- a/packages/snaps-utils/src/snaps.ts +++ b/packages/snaps-utils/src/snaps.ts @@ -306,7 +306,7 @@ export function validateSnapId( } } - throw new Error(`Invalid snap id. Unknown prefix. Received: "${snapId}"`); + throw new Error(`Invalid snap id. Unknown prefix. Received: "${snapId}".`); } /** diff --git a/packages/snaps-utils/src/versions.ts b/packages/snaps-utils/src/versions.ts index f8b1a936ae..4f839d9859 100644 --- a/packages/snaps-utils/src/versions.ts +++ b/packages/snaps-utils/src/versions.ts @@ -1,7 +1,7 @@ import { assertStruct, Json } from '@metamask/utils'; import { gt as gtSemver, - gtr, + gtr as gtrSemver, maxSatisfying as maxSatisfyingSemver, satisfies as satisfiesSemver, valid as validSemVerVersion, @@ -150,7 +150,7 @@ export function gtVersion( * @returns `version > range`. */ export function gtRange(version: SemVerVersion, range: SemVerRange): boolean { - return gtr(version, range); + return gtrSemver(version, range); } /** From 185d8ac32e4f2124a48e30b78482294832524297 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 6 Dec 2022 09:22:07 -0500 Subject: [PATCH 29/33] fixed some tests --- packages/snaps-controllers/src/snaps/SnapController.test.ts | 4 ++-- packages/snaps-controllers/src/snaps/SnapController.ts | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 6c16bbe84f..6a4506f52a 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -2365,7 +2365,7 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: { version: newVersionRange }, }), ).rejects.toThrow( - `Snap "${MOCK_SNAP_ID}@1.0.0" is already installed, couldn't update to a version inside requested "${newVersionRange}" range.`, + `Snap "${MOCK_SNAP_ID}@1.0.0" is already installed. Couldn't update to a version inside requested "${newVersionRange}" range.`, ); expect(messenger.call).toHaveBeenCalledTimes(1); @@ -2533,7 +2533,7 @@ describe('SnapController', () => { [snapId2]: { version: newVersion }, }), ).rejects.toThrow( - `Snap "${snapId1}@${oldVersion}" is already installed, couldn't update to a version inside requested "${olderVersion}" range.`, + `Snap "${snapId1}@${oldVersion}" is already installed. Couldn't update to a version inside requested "${olderVersion}" range.`, ); expect(detect).toHaveBeenCalledTimes(4); diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 5b8abc4588..f57dbba11c 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -72,7 +72,6 @@ import { Json, NonEmptyArray, timeSince, - NonEmptyArray, } from '@metamask/utils'; import { createMachine, interpret, StateMachine } from '@xstate/fsm'; import { ethErrors } from 'eth-rpc-errors'; From 3aad81dd8d1736ff0f2ac85f9c7c5c5a812b6676 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 6 Dec 2022 10:18:58 -0500 Subject: [PATCH 30/33] removed casting and added gtRange test --- .../src/snaps/SnapController.ts | 4 +--- packages/snaps-utils/src/versions.test.ts | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 2d97b284ee..b39ba5de13 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -1829,9 +1829,7 @@ export class SnapController extends BaseController< }); } - const rollbackSnapshot = this.#getRollbackSnapshot( - snapId, - ) as RollbackSnapshot; + const rollbackSnapshot = this.#getRollbackSnapshot(snapId); if (rollbackSnapshot !== undefined) { rollbackSnapshot.permissions.revoked = unusedPermissions; rollbackSnapshot.permissions.granted = Object.keys( diff --git a/packages/snaps-utils/src/versions.test.ts b/packages/snaps-utils/src/versions.test.ts index 7874a0e46b..de2330e819 100644 --- a/packages/snaps-utils/src/versions.test.ts +++ b/packages/snaps-utils/src/versions.test.ts @@ -8,6 +8,7 @@ import { DEFAULT_REQUESTED_SNAP_VERSION, getTargetVersion, gtVersion, + gtRange, isValidSemVerRange, isValidSemVerVersion, resolveVersionRange, @@ -178,6 +179,26 @@ describe('gtVersion', () => { }); }); +describe('gtRange', () => { + it('supports regular versions', () => { + expect(gtRange('1.2.3' as SemVerVersion, '^1.0.0' as SemVerRange)).toBe( + false, + ); + + expect( + gtRange('2.0.0' as SemVerVersion, '>1.0.0 <2.0.0' as SemVerRange), + ).toBe(true); + + expect(gtRange('1.2.0' as SemVerVersion, '~1.2.0' as SemVerRange)).toBe( + false, + ); + + expect(gtRange('1.5.0' as SemVerVersion, '<1.5.0' as SemVerRange)).toBe( + true, + ); + }); +}); + describe('getTargetVersion', () => { it('supports *', () => { expect( From 7399f571060b21fa23c03ad65e4b0b7ecc73a9b5 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 6 Dec 2022 11:09:47 -0500 Subject: [PATCH 31/33] jest config update --- packages/snaps-utils/jest.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-utils/jest.config.js b/packages/snaps-utils/jest.config.js index e1cb61249e..2870c39352 100644 --- a/packages/snaps-utils/jest.config.js +++ b/packages/snaps-utils/jest.config.js @@ -18,7 +18,7 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 94.36, + branches: 94.62, functions: 98.92, lines: 98.99, statements: 98.99, From 54cbb63638fbd49f17e8e23f33aa975516464a7f Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 6 Dec 2022 11:19:59 -0500 Subject: [PATCH 32/33] update threshold --- packages/snaps-utils/jest.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-utils/jest.config.js b/packages/snaps-utils/jest.config.js index 2870c39352..ad3a10c0f3 100644 --- a/packages/snaps-utils/jest.config.js +++ b/packages/snaps-utils/jest.config.js @@ -18,7 +18,7 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 94.62, + branches: 94.6, functions: 98.92, lines: 98.99, statements: 98.99, From 4b71e5e14fc1cb5e279d422e424446651c3b0e75 Mon Sep 17 00:00:00 2001 From: hmalik88 Date: Tue, 6 Dec 2022 11:35:29 -0500 Subject: [PATCH 33/33] update grammar --- packages/snaps-controllers/src/snaps/SnapController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index b39ba5de13..784e718ffa 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -2635,11 +2635,11 @@ export class SnapController extends BaseController< * Checks if a snap will pass version validation checks * with the new version range that is requested. The first * check that is done is to check if the existing snap version - * falls inside the requested range, if it does we want to return + * falls inside the requested range. If it does, we want to return * false because we do not care to create a rollback snapshot in * that scenario. The second check is to ensure that the current * snap version is not greater than all possible versions in - * the requested version range, if it is then we also want + * the requested version range. If it is, then we also want * to return false in that scenario. * * @param snapId - The snap id.