fix!: Always consume latest registry in updateRegistry

packages/snaps-controllers/src/snaps/SnapController-method-action-types.ts

@@ -21,7 +21,7 @@ export type SnapControllerInitAction = {
 /**
  * Trigger an update of the registry.
  *
- * As a side-effect of this, preinstalled Snaps may be updated and Snaps may be blocked/unblocked.
+ * As a side-effect, this will _always_ check if preinstalled Snaps can be updated and whether any Snaps need to be blocked/unblocked.
  */
 export type SnapControllerUpdateRegistryAction = {
   type: `SnapController:updateRegistry`;

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

@@ -10826,6 +10826,107 @@ describe('SnapController', () => {
       snapController.destroy();
     });
 
+    it('retries updating preinstalled Snaps', async () => {
+      const rootMessenger = getRootMessenger();
+      const registry = new MockSnapRegistryController(rootMessenger);
+
+      // Simulate previous permissions, some of which will be removed
+      rootMessenger.registerActionHandler(
+        'PermissionController:getPermissions',
+        () => {
+          return {
+            [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION,
+            [SnapEndowments.LifecycleHooks]: MOCK_LIFECYCLE_HOOKS_PERMISSION,
+          };
+        },
+      );
+
+      const snapId = 'npm:@metamask/jsx-example-snap' as SnapId;
+
+      const mockSnap = getPersistedSnapObject({
+        id: snapId,
+        preinstalled: true,
+      });
+
+      // Indicate no database update performed
+      registry.requestUpdate.mockImplementation(() => {
+        rootMessenger.publish('SnapRegistryController:registryUpdated', false);
+      });
+
+      const updateVersion = '1.2.1';
+
+      registry.resolveVersion.mockResolvedValue(updateVersion);
+      const fetchFunction = jest.fn().mockResolvedValueOnce({
+        // eslint-disable-next-line no-restricted-globals
+        headers: new Headers({ 'content-length': '5477' }),
+        ok: true,
+        body: Readable.toWeb(
+          createReadStream(
+            path.resolve(
+              __dirname,
+              `../../test/fixtures/metamask-jsx-example-snap-${updateVersion}.tgz`,
+            ),
+          ),
+        ),
+      });
+
+      const options = getSnapControllerOptions({
+        rootMessenger,
+        state: {
+          snaps: getPersistedSnapsState(mockSnap),
+        },
+        fetchFunction,
+        featureFlags: {
+          autoUpdatePreinstalledSnaps: true,
+        },
+      });
+
+      const snapController = await getSnapController(options);
+
+      await snapController.updateRegistry();
+      await waitForStateChange(options.messenger);
+      await sleep(100);
+
+      const updatedSnap = snapController.getSnap(snapId);
+      assert(updatedSnap);
+
+      expect(updatedSnap.version).toStrictEqual(updateVersion);
+      expect(updatedSnap.preinstalled).toBe(true);
+
+      expect(options.messenger.call).toHaveBeenNthCalledWith(
+        7,
+        'StorageService:setItem',
+        controllerName,
+        snapId,
+        { sourceCode: expect.any(String) },
+      );
+
+      expect(options.messenger.call).toHaveBeenNthCalledWith(
+        9,
+        'PermissionController:revokePermissions',
+        { [snapId]: [SnapEndowments.Rpc, SnapEndowments.LifecycleHooks] },
+      );
+
+      expect(options.messenger.call).toHaveBeenNthCalledWith(
+        10,
+        'PermissionController:grantPermissions',
+        {
+          approvedPermissions: {
+            'endowment:rpc': {
+              caveats: [{ type: 'rpcOrigin', value: { dapps: true } }],
+            },
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            snap_dialog: {},
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            snap_manageState: {},
+          },
+          subject: { origin: snapId },
+        },
+      );
+
+      snapController.destroy();
+    });
+
     it('does not update preinstalled Snaps when the feature flag is off', async () => {
       const rootMessenger = getRootMessenger();
       const registry = new MockSnapRegistryController(rootMessenger);

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

@@ -167,7 +167,7 @@ import type {
   SnapRegistryControllerRequestUpdateAction,
   SnapRegistryInfo,
   SnapRegistryRequest,
-  SnapRegistryControllerStateChangeEvent,
+  SnapRegistryControllerRegistryUpdatedEvent,
 } from './registry';
 import { SnapRegistryStatus } from './registry';
 import { getRunnableSnaps } from './selectors';
@@ -547,7 +547,7 @@ type AllowedEvents =
   | SnapControllerSnapInstalledEvent
   | SnapControllerSnapUpdatedEvent
   | KeyringControllerLockEvent
-  | SnapRegistryControllerStateChangeEvent;
+  | SnapRegistryControllerRegistryUpdatedEvent;
 
 export type SnapControllerMessenger = Messenger<
   typeof controllerName,
@@ -974,19 +974,15 @@ export class SnapController extends BaseController<
       this.#handleLock.bind(this),
     );
 
-    this.messenger.subscribe(
-      'SnapRegistryController:stateChange',
-      () => {
-        this.#handleRegistryUpdate().catch((error) => {
-          logError(
-            `Error when processing Snaps registry update: ${getErrorMessage(
-              error,
-            )}`,
-          );
-        });
-      },
-      ({ database }) => database,
-    );
+    this.messenger.subscribe('SnapRegistryController:registryUpdated', () => {
+      this.#handleRegistryUpdate().catch((error) => {
+        logError(
+          `Error when processing Snaps registry update: ${getErrorMessage(
+            error,
+          )}`,
+        );
+      });
+    });
 
     this.#initializeStateMachine();
     this.#registerMessageHandlers();
@@ -1448,7 +1444,7 @@ export class SnapController extends BaseController<
   /**
    * Trigger an update of the registry.
    *
-   * As a side-effect of this, preinstalled Snaps may be updated and Snaps may be blocked/unblocked.
+   * As a side-effect, this will _always_ check if preinstalled Snaps can be updated and whether any Snaps need to be blocked/unblocked.
    */
   async updateRegistry(): Promise<void> {
     await this.#ensureCanUsePlatform();

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

@@ -73,8 +73,14 @@ export type SnapRegistryControllerStateChangeEvent = ControllerStateChangeEvent<
   SnapRegistryControllerState
 >;
 
+export type SnapRegistryControllerRegistryUpdatedEvent = {
+  type: `${typeof controllerName}:registryUpdated`;
+  payload: [databaseUpdated: boolean];
+};
+
 export type SnapRegistryControllerEvents =
-  SnapRegistryControllerStateChangeEvent;
+  | SnapRegistryControllerStateChangeEvent
+  | SnapRegistryControllerRegistryUpdatedEvent;
 
 export type SnapRegistryControllerMessenger = Messenger<
   typeof controllerName,
@@ -219,6 +225,7 @@ export class SnapRegistryController extends BaseController<
   async #update() {
     // No-op if we recently fetched the registry.
     if (this.#wasRecentlyFetched()) {
+      this.messenger.publish('SnapRegistryController:registryUpdated', false);
       return;
     }
 
@@ -236,6 +243,7 @@ export class SnapRegistryController extends BaseController<
           state.lastUpdated = Date.now();
           state.databaseUnavailable = false;
         });
+        this.messenger.publish('SnapRegistryController:registryUpdated', false);
         return;
       }
 
@@ -247,11 +255,14 @@ export class SnapRegistryController extends BaseController<
         state.databaseUnavailable = false;
         state.signature = signatureJson.signature;
       });
+
+      this.messenger.publish('SnapRegistryController:registryUpdated', true);
     } catch {
       // Ignore
       this.update((state) => {
         state.databaseUnavailable = true;
       });
+      this.messenger.publish('SnapRegistryController:registryUpdated', false);
     }
   }
 

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

@@ -6,6 +6,7 @@ export type {
   SnapRegistryControllerMessenger,
   SnapRegistryControllerState,
   SnapRegistryControllerStateChangeEvent,
+  SnapRegistryControllerRegistryUpdatedEvent,
 } from './SnapRegistryController';
 export { SnapRegistryController } from './SnapRegistryController';
 export type {

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

@@ -509,7 +509,7 @@ export const getSnapControllerMessenger = (
       'ExecutionService:outboundRequest',
       'ExecutionService:outboundResponse',
       'KeyringController:lock',
-      'SnapRegistryController:stateChange',
+      'SnapRegistryController:registryUpdated',
     ],
     messenger: snapControllerMessenger,
   });

packages/snaps-controllers/src/test-utils/registry.ts

@@ -57,5 +57,6 @@ export class MockSnapRegistryController {
       },
       [],
     );
+    this.#messenger.publish('SnapRegistryController:registryUpdated', true);
   });
 }