diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.tsx b/packages/snaps-controllers/src/snaps/SnapController.test.tsx index e75b5cf2f0..174f263ff6 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.tsx +++ b/packages/snaps-controllers/src/snaps/SnapController.test.tsx @@ -634,11 +634,9 @@ describe('SnapController', () => { }), ); - await snapController.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + await snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); expect(messenger.call).toHaveBeenNthCalledWith( 4, @@ -727,11 +725,9 @@ describe('SnapController', () => { }), ); - await snapController.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + await snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); expect(messenger.call).toHaveBeenNthCalledWith( 6, @@ -808,11 +804,9 @@ describe('SnapController', () => { }), ); - await snapController.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + await snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); expect(messenger.call).toHaveBeenNthCalledWith( 6, @@ -1275,8 +1269,6 @@ describe('SnapController', () => { }), ); - const authorizeSpy = jest.spyOn(controller as any, 'authorize'); - await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: { version: '>0.9.0 <1.1.0' }, }); @@ -1284,7 +1276,6 @@ describe('SnapController', () => { const newSnap = controller.get(MOCK_SNAP_ID); expect(newSnap).toStrictEqual(getSnapObject()); - expect(authorizeSpy).not.toHaveBeenCalled(); expect(messenger.call).not.toHaveBeenCalled(); controller.destroy(); @@ -1360,7 +1351,8 @@ describe('SnapController', () => { }); it('removes a snap that errors during installation after being added', async () => { - const messenger = getSnapControllerMessenger(); + const rootMessenger = getControllerMessenger(); + const messenger = getSnapControllerMessenger(rootMessenger); const snapController = getSnapController( getSnapControllerOptions({ messenger, @@ -1370,28 +1362,31 @@ describe('SnapController', () => { const messengerCallMock = jest.spyOn(messenger, 'call'); - jest - .spyOn(snapController as any, 'authorize') - .mockImplementationOnce(() => { - throw new Error('foo'); - }); + rootMessenger.registerActionHandler( + 'ApprovalController:updateRequestState', + (request) => { + approvalControllerMock.updateRequestStateAndReject.bind( + approvalControllerMock, + )(request); + }, + ); await expect( snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }), - ).rejects.toThrow('foo'); + ).rejects.toThrow('User rejected the request.'); - expect(messengerCallMock).toHaveBeenCalledTimes(9); + expect(messengerCallMock).toHaveBeenCalledTimes(10); expect(messengerCallMock).toHaveBeenNthCalledWith( - 4, + 5, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), requestState: { loading: false, - error: 'foo', + error: 'User rejected the request.', type: SNAP_APPROVAL_INSTALL, }, }), @@ -5444,13 +5439,12 @@ describe('SnapController', () => { }), ); - const authorizeSpy = jest.spyOn(snapController as any, 'authorize'); const result = await snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }); expect(result).toStrictEqual({ [MOCK_SNAP_ID]: truncatedSnap }); - expect(authorizeSpy).not.toHaveBeenCalled(); + expect(messenger.call).not.toHaveBeenCalled(); snapController.destroy(); }); @@ -6887,12 +6881,6 @@ describe('SnapController', () => { }, ]; - const snapControllerOptions = getSnapControllerWithEESOptions({ - preinstalledSnaps, - rootMessenger, - }); - const [snapController] = getSnapControllerWithEES(snapControllerOptions); - // After install the snap should have permissions rootMessenger.registerActionHandler( 'PermissionController:getPermissions', @@ -6909,12 +6897,17 @@ describe('SnapController', () => { manifest: manifest.result, }); + const snapControllerOptions = getSnapControllerWithEESOptions({ + preinstalledSnaps, + rootMessenger, + detectSnapLocation, + }); + const [snapController] = getSnapControllerWithEES(snapControllerOptions); + await expect( - snapController.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ), + snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }), ).rejects.toThrow('Preinstalled Snaps cannot be manually updated.'); snapController.destroy(); @@ -7663,11 +7656,9 @@ describe('SnapController', () => { () => ({}), ); - await snapController.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + await snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); expect(messenger.call).toHaveBeenNthCalledWith( 7, @@ -7740,11 +7731,9 @@ describe('SnapController', () => { }), ); - await snapController.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + await snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); expect(messenger.call).toHaveBeenNthCalledWith( 7, @@ -8462,46 +8451,7 @@ describe('SnapController', () => { snapController.destroy(); }); - describe('updateSnap', () => { - it('throws an error for non installed snap', async () => { - const detectSnapLocation = loopbackDetect(); - const controller = getSnapController(); - - await expect(async () => - controller.updateSnap( - MOCK_ORIGIN, - MOCK_LOCAL_SNAP_ID, - detectSnapLocation(), - ), - ).rejects.toThrow(`Snap "${MOCK_LOCAL_SNAP_ID}" not found.`); - - controller.destroy(); - }); - - it('throws an error if the specified SemVer range is invalid', async () => { - const detectSnapLocation = loopbackDetect(); - const controller = getSnapController( - getSnapControllerOptions({ - state: { - snaps: getPersistedSnapsState(), - }, - }), - ); - - await expect( - controller.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - 'this is not a version' as SemVerRange, - ), - ).rejects.toThrow( - 'Received invalid snap version range: "this is not a version".', - ); - - controller.destroy(); - }); - + describe('Updating Snaps', () => { it("throws an error if new version doesn't match version range", async () => { const { manifest } = await getMockSnapFilesWithUpdatedChecksum({ manifest: getSnapManifest({ @@ -8530,13 +8480,9 @@ describe('SnapController', () => { const newSnap = controller.get(MOCK_SNAP_ID); await expect( - async () => - await controller.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - '1.2.0', - ), + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.2.0' }, + }), ).rejects.toThrow( `Version mismatch. Manifest for "npm:@metamask/example-snap" specifies version "1.1.0" which doesn't satisfy requested version range "1.2.0".`, ); @@ -8573,7 +8519,9 @@ describe('SnapController', () => { }); await expect( - controller.updateSnap(MOCK_ORIGIN, MOCK_SNAP_ID, detectSnapLocation()), + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }), ).rejects.toThrow('Cannot install version "1.1.0" of snap'); controller.destroy(); @@ -8608,15 +8556,13 @@ describe('SnapController', () => { const newSnap = controller.get(MOCK_SNAP_ID); - const errorMessage = `Snap "${MOCK_SNAP_ID}@${snap.version}" is already installed. Couldn't update to a version inside requested "*" range.`; + const errorMessage = `Snap "${MOCK_SNAP_ID}@${snap.version}" is already installed. Couldn't update to a version inside requested "0.9.0" range.`; await expect( async () => - await controller.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ), + await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '0.9.0' }, + }), ).rejects.toThrow(rpcErrors.invalidParams(errorMessage)); expect(publishSpy).toHaveBeenCalledWith( 'SnapController:snapInstallStarted', @@ -8668,17 +8614,15 @@ describe('SnapController', () => { messenger.subscribe('SnapController:snapUpdated', onSnapUpdated); - const result = await controller.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + const result = await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); const newSnapTruncated = controller.getTruncated(MOCK_SNAP_ID); const newSnap = controller.get(MOCK_SNAP_ID); - expect(result).toStrictEqual(newSnapTruncated); + expect(result).toStrictEqual({ [MOCK_SNAP_ID]: newSnapTruncated }); expect(newSnap?.version).toBe('1.1.0'); expect(newSnap?.versionHistory).toStrictEqual([ { @@ -8769,7 +8713,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 21, + 19, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -8819,17 +8763,15 @@ describe('SnapController', () => { }), ); - const result = await controller.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + const result = await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); const newSnapTruncated = controller.getTruncated(MOCK_SNAP_ID); const newSnap = controller.get(MOCK_SNAP_ID); - expect(result).toStrictEqual(newSnapTruncated); + expect(result).toStrictEqual({ [MOCK_SNAP_ID]: newSnapTruncated }); expect(newSnap?.version).toBe('1.1.0'); expect(newSnap?.versionHistory).toStrictEqual([ { @@ -8873,11 +8815,9 @@ describe('SnapController', () => { const stopSnapSpy = jest.spyOn(controller as any, 'stopSnap'); - await controller.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); const isRunning = controller.isRunning(MOCK_SNAP_ID); @@ -8964,7 +8904,7 @@ describe('SnapController', () => { ); expect(callActionSpy).toHaveBeenNthCalledWith( - 12, + 10, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -9043,7 +8983,9 @@ describe('SnapController', () => { ); await expect( - controller.updateSnap(MOCK_ORIGIN, MOCK_SNAP_ID, detectSnapLocation()), + controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }), ).rejects.toThrow('User rejected the request.'); const newSnap = controller.get(MOCK_SNAP_ID); @@ -9230,7 +9172,9 @@ describe('SnapController', () => { await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {} }); await controller.stopSnap(MOCK_SNAP_ID); - await controller.updateSnap(MOCK_ORIGIN, MOCK_SNAP_ID, detect()); + await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); expect(callActionSpy).toHaveBeenCalledTimes(23); @@ -9333,7 +9277,7 @@ describe('SnapController', () => { ); expect(callActionSpy).toHaveBeenNthCalledWith( - 23, + 21, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -9595,7 +9539,9 @@ describe('SnapController', () => { ); await snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {} }); - await snapController.updateSnap(MOCK_ORIGIN, MOCK_SNAP_ID, detect()); + await snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); snapController.destroy(); }); @@ -9622,11 +9568,9 @@ describe('SnapController', () => { }), ); - await controller.updateSnap( - MOCK_ORIGIN, - MOCK_SNAP_ID, - detectSnapLocation(), - ); + await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.2.0' }, + }); const newSnap = controller.get(MOCK_SNAP_ID); expect(newSnap?.version).toBe('1.2.0'); diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index f490c70793..fe5951ef25 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -84,7 +84,6 @@ import { assertIsSnapManifest, assertIsValidSnapId, DEFAULT_ENDOWMENTS, - DEFAULT_REQUESTED_SNAP_VERSION, encodeAuxiliaryFile, HandlerType, isOriginAllowed, @@ -131,7 +130,6 @@ import { hasProperty, inMilliseconds, isNonEmptyArray, - isValidSemVerRange, satisfiesVersionRange, timeSince, createDeferredPromise, @@ -2650,7 +2648,7 @@ export class SnapController extends BaseController< pendingInstalls.push(snapId); } - result[snapId] = await this.processRequestedSnap( + result[snapId] = await this.#processRequestedSnap( origin, snapId, location, @@ -2704,10 +2702,7 @@ export class SnapController extends BaseController< * @param versionRange - The semver range of the snap to install. * @returns The resulting snap object, or an error if something went wrong. */ - // TODO: Either fix this lint violation or explain why it's necessary to - // ignore. - // eslint-disable-next-line no-restricted-syntax - private async processRequestedSnap( + async #processRequestedSnap( origin: string, snapId: SnapId, location: SnapLocation, @@ -2721,18 +2716,12 @@ export class SnapController extends BaseController< return existingSnap; } - return await this.updateSnap( + return await this.#updateSnap({ origin, snapId, location, versionRange, - // Since we are requesting an update from within processRequestedSnap, - // we disable the emitting of the snapUpdated event and rely on the caller - // to publish this event after the update is complete. - // This is necessary as installSnaps may be installing multiple snaps - // and we don't want to emit events prematurely. - false, - ); + }); } this.#assertCanInstallSnaps(); @@ -2768,7 +2757,7 @@ export class SnapController extends BaseController< versionRange, }); - await this.authorize(snapId, pendingApproval); + await this.#authorize(snapId, pendingApproval); pendingApproval = this.#createApproval({ origin, @@ -2867,20 +2856,24 @@ export class SnapController extends BaseController< * If the original version of the snap was blocked and the update succeeded, * the snap will be unblocked and enabled before it is restarted. * - * @param origin - The origin requesting the snap update. - * @param snapId - The id of the Snap to be updated. - * @param location - The location implementation of the snap. - * @param newVersionRange - A semver version range in which the maximum version will be chosen. - * @param emitEvent - An optional boolean flag to indicate whether this update should emit an event. + * @param options - An options bag. + * @param options.origin - The origin requesting the snap update. + * @param options.snapId - The id of the Snap to be updated. + * @param options.location - The location implementation of the snap. + * @param options.versionRange - A semver version range in which the maximum version will be chosen. * @returns The snap metadata if updated, `null` otherwise. */ - async updateSnap( - origin: string, - snapId: SnapId, - location: SnapLocation, - newVersionRange: string = DEFAULT_REQUESTED_SNAP_VERSION, - emitEvent = true, - ): Promise { + async #updateSnap({ + origin, + snapId, + location, + versionRange, + }: { + origin: string; + snapId: SnapId; + location: SnapLocation; + versionRange: SemVerRange; + }): Promise { this.#assertCanInstallSnaps(); this.#assertCanUsePlatform(); @@ -2890,12 +2883,6 @@ export class SnapController extends BaseController< throw new Error('Preinstalled Snaps cannot be manually updated.'); } - if (!isValidSemVerRange(newVersionRange)) { - throw new Error( - `Received invalid snap version range: "${newVersionRange}".`, - ); - } - let pendingApproval = this.#createApproval({ origin, snapId, @@ -2920,13 +2907,13 @@ export class SnapController extends BaseController< const newVersion = manifest.version; if (!gtVersion(newVersion, snap.version)) { throw rpcErrors.invalidParams( - `Snap "${snapId}@${snap.version}" is already installed. Couldn't update to a version inside requested "${newVersionRange}" range.`, + `Snap "${snapId}@${snap.version}" is already installed. Couldn't update to a version inside requested "${versionRange}" range.`, ); } - if (!satisfiesVersionRange(newVersion, newVersionRange)) { + if (!satisfiesVersionRange(newVersion, versionRange)) { throw new Error( - `Version mismatch. Manifest for "${snapId}" specifies version "${newVersion}" which doesn't satisfy requested version range "${newVersionRange}".`, + `Version mismatch. Manifest for "${snapId}" specifies version "${newVersion}" which doesn't satisfy requested version range "${versionRange}".`, ); } @@ -3024,16 +3011,6 @@ export class SnapController extends BaseController< const truncatedSnap = this.getTruncatedExpect(snapId); - if (emitEvent) { - this.messagingSystem.publish( - 'SnapController:snapUpdated', - truncatedSnap, - snap.version, - origin, - false, - ); - } - this.#updateApproval(pendingApproval.id, { loading: false, type: SNAP_APPROVAL_UPDATE, @@ -3419,14 +3396,11 @@ export class SnapController extends BaseController< * Initiates a request for the given snap's initial permissions. * Must be called in order. See processRequestedSnap. * - * This function is not hash private yet because of tests. - * * @param snapId - The id of the Snap. * @param pendingApproval - Pending approval to update. * @returns The snap's approvedPermissions. */ - // eslint-disable-next-line no-restricted-syntax - private async authorize( + async #authorize( snapId: SnapId, pendingApproval: PendingApproval, ): Promise {