From 1a420960af5b3a777fade65da1793207d53debd2 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 26 May 2025 09:57:03 +0200 Subject: [PATCH 01/16] Refactor cronjob controller to reduce duplication --- packages/snaps-controllers/package.json | 1 + .../src/cronjob/CronjobController.ts | 679 ++++++++---------- .../src/cronjob/utils.test.ts | 52 ++ .../snaps-controllers/src/cronjob/utils.ts | 69 ++ .../src/permitted/scheduleBackgroundEvent.ts | 44 +- .../types/methods/get-background-events.ts | 11 +- packages/snaps-utils/src/cronjob.ts | 8 +- packages/snaps-utils/src/time.ts | 1 + yarn.lock | 1 + 9 files changed, 462 insertions(+), 404 deletions(-) create mode 100644 packages/snaps-controllers/src/cronjob/utils.test.ts create mode 100644 packages/snaps-controllers/src/cronjob/utils.ts diff --git a/packages/snaps-controllers/package.json b/packages/snaps-controllers/package.json index 862a7a1092..c27bcbc367 100644 --- a/packages/snaps-controllers/package.json +++ b/packages/snaps-controllers/package.json @@ -98,6 +98,7 @@ "@xstate/fsm": "^2.0.0", "async-mutex": "^0.5.0", "concat-stream": "^2.0.0", + "cron-parser": "^4.5.0", "fast-deep-equal": "^3.1.3", "get-npm-tarball-url": "^2.0.3", "immer": "^9.0.6", diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index d5990be9eb..31811efb62 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -10,22 +10,14 @@ import { SnapEndowments, } from '@metamask/snaps-rpc-methods'; import type { BackgroundEvent, SnapId } from '@metamask/snaps-sdk'; -import type { - TruncatedSnap, - CronjobSpecification, -} from '@metamask/snaps-utils'; -import { - HandlerType, - parseCronExpression, - logError, - logWarning, - toCensoredISO8601String, -} from '@metamask/snaps-utils'; -import { assert, Duration, hasProperty, inMilliseconds } from '@metamask/utils'; +import type { TruncatedSnap } from '@metamask/snaps-utils'; +import { HandlerType, logError } from '@metamask/snaps-utils'; +import { assert, Duration, inMilliseconds } from '@metamask/utils'; import { castDraft } from 'immer'; import { DateTime } from 'luxon'; import { nanoid } from 'nanoid'; +import { getCurrentDate, getExecutionDate } from './utils'; import type { GetAllSnaps, HandleSnapRequest, @@ -35,7 +27,7 @@ import type { SnapUninstalled, SnapUpdated, } from '..'; -import { getRunnableSnaps } from '..'; +import { METAMASK_ORIGIN } from '../snaps/constants'; import { Timer } from '../snaps/Timer'; export type CronjobControllerGetStateAction = ControllerGetStateAction< @@ -47,19 +39,19 @@ export type CronjobControllerStateChangeEvent = ControllerStateChangeEvent< CronjobControllerState >; -export type ScheduleBackgroundEvent = { - type: `${typeof controllerName}:scheduleBackgroundEvent`; - handler: CronjobController['scheduleBackgroundEvent']; +export type Schedule = { + type: `${typeof controllerName}:schedule`; + handler: CronjobController['schedule']; }; -export type CancelBackgroundEvent = { - type: `${typeof controllerName}:cancelBackgroundEvent`; - handler: CronjobController['cancelBackgroundEvent']; +export type Cancel = { + type: `${typeof controllerName}:cancel`; + handler: CronjobController['cancel']; }; -export type GetBackgroundEvents = { - type: `${typeof controllerName}:getBackgroundEvents`; - handler: CronjobController['getBackgroundEvents']; +export type Get = { + type: `${typeof controllerName}:get`; + handler: CronjobController['get']; }; export type CronjobControllerActions = @@ -67,9 +59,9 @@ export type CronjobControllerActions = | HandleSnapRequest | GetPermissions | CronjobControllerGetStateAction - | ScheduleBackgroundEvent - | CancelBackgroundEvent - | GetBackgroundEvents; + | Schedule + | Cancel + | Get; export type CronjobControllerEvents = | SnapInstalled @@ -91,529 +83,420 @@ export const DAILY_TIMEOUT = inMilliseconds(24, Duration.Hour); export type CronjobControllerArgs = { messenger: CronjobControllerMessenger; + /** * Persisted state that will be used for rehydration. */ state?: CronjobControllerState; }; -export type Cronjob = { - timer?: Timer; - id: string; - snapId: SnapId; -} & CronjobSpecification; +/** + * Represents a background event that is scheduled to be executed by the + * cronjob controller. + */ +export type InternalBackgroundEvent = BackgroundEvent & { + /** + * Whether the event is recurring. + */ + recurring: boolean; -export type StoredJobInformation = { - lastRun: number; + /** + * The cron expression or ISO 8601 duration string that defines the event's + * schedule. + */ + schedule: string; +}; + +/** + * A schedulable background event, which is a subset of the + * {@link InternalBackgroundEvent} type, containing only the fields required to + * schedule an event. Other fields will be populated by the cronjob controller + * automatically. + */ +export type SchedulableBackgroundEvent = Omit< + InternalBackgroundEvent, + 'scheduledAt' | 'date' | 'id' +> & { + /** + * The optional ID of the event. If not provided, a new ID will be + * generated. + */ + id?: string; }; export type CronjobControllerState = { - jobs: Record; - events: Record; + /** + * Background events and cronjobs that are scheduled to be executed. + */ + events: Record; }; const controllerName = 'CronjobController'; /** - * Use this controller to register and schedule periodically executed jobs - * using RPC method hooks. + * The cronjob controller is responsible for managing cronjobs and background + * events for Snaps. It allows Snaps to schedule events that will be executed + * at a later time. */ export class CronjobController extends BaseController< typeof controllerName, CronjobControllerState, CronjobControllerMessenger > { - #dailyTimer!: Timer; - readonly #timers: Map; - // Mapping from jobId to snapId - readonly #snapIds: Map; + readonly #dailyTimer: Timer = new Timer(DAILY_TIMEOUT); constructor({ messenger, state }: CronjobControllerArgs) { super({ messenger, metadata: { - jobs: { persist: true, anonymous: false }, events: { persist: true, anonymous: false }, }, name: controllerName, state: { - jobs: {}, events: {}, ...state, }, }); + this.#timers = new Map(); - this.#snapIds = new Map(); - - this._handleSnapRegisterEvent = this._handleSnapRegisterEvent.bind(this); - this._handleSnapUnregisterEvent = - this._handleSnapUnregisterEvent.bind(this); - this._handleEventSnapUpdated = this._handleEventSnapUpdated.bind(this); - this._handleSnapDisabledEvent = this._handleSnapDisabledEvent.bind(this); - this._handleSnapEnabledEvent = this._handleSnapEnabledEvent.bind(this); - // Subscribe to Snap events - /* eslint-disable @typescript-eslint/unbound-method */ this.messagingSystem.subscribe( 'SnapController:snapInstalled', - this._handleSnapRegisterEvent, + this.#handleSnapInstalledEvent.bind(this), ); this.messagingSystem.subscribe( 'SnapController:snapUninstalled', - this._handleSnapUnregisterEvent, + this.#handleSnapUninstalledEvent.bind(this), ); this.messagingSystem.subscribe( 'SnapController:snapEnabled', - this._handleSnapEnabledEvent, + this.#handleSnapEnabledEvent.bind(this), ); this.messagingSystem.subscribe( 'SnapController:snapDisabled', - this._handleSnapDisabledEvent, + this.#handleSnapDisabledEvent.bind(this), ); this.messagingSystem.subscribe( 'SnapController:snapUpdated', - this._handleEventSnapUpdated, + this.#handleSnapUpdatedEvent.bind(this), ); - /* eslint-enable @typescript-eslint/unbound-method */ this.messagingSystem.registerActionHandler( - `${controllerName}:scheduleBackgroundEvent`, - (...args) => this.scheduleBackgroundEvent(...args), + `${controllerName}:schedule`, + (...args) => this.schedule(...args), ); this.messagingSystem.registerActionHandler( - `${controllerName}:cancelBackgroundEvent`, - (...args) => this.cancelBackgroundEvent(...args), + `${controllerName}:cancel`, + (...args) => this.cancel(...args), ); this.messagingSystem.registerActionHandler( - `${controllerName}:getBackgroundEvents`, - (...args) => this.getBackgroundEvents(...args), + `${controllerName}:get`, + (...args) => this.get(...args), ); - this.dailyCheckIn(); - - this.#rescheduleBackgroundEvents(Object.values(this.state.events)); + this.#clear(); + this.#reschedule(); + this.#start(); } /** - * Retrieve all cronjob specifications for all runnable snaps. + * Schedule an event. * - * @returns Array of Cronjob specifications. + * @param event - The event to schedule. + * @returns The ID of the scheduled event. */ - #getAllJobs(): Cronjob[] { - const snaps = this.messagingSystem.call('SnapController:getAll'); - const filteredSnaps = getRunnableSnaps(snaps); + schedule(event: SchedulableBackgroundEvent) { + const id = event.id ?? nanoid(); + const internalEvent: InternalBackgroundEvent = { + ...event, + id, + date: getExecutionDate(event.schedule), + scheduledAt: getCurrentDate(), + }; + + this.update((state) => { + state.events[internalEvent.id] = castDraft(internalEvent); + }); - const jobs = filteredSnaps.map((snap) => this.#getSnapJobs(snap.id)); - return jobs.flat().filter((job) => job !== undefined) as Cronjob[]; + this.#schedule(internalEvent); + return id; } /** - * Retrieve all Cronjob specifications for a Snap. + * Cancel an event. * - * @param snapId - ID of a Snap. - * @returns Array of Cronjob specifications. + * @param origin - The origin making the cancel call. + * @param id - The id of the event to cancel. + * @throws If the event does not exist. */ - #getSnapJobs(snapId: SnapId): Cronjob[] | undefined { - const permissions = this.messagingSystem.call( - 'PermissionController:getPermissions', - snapId, + cancel(origin: string, id: string) { + assert( + this.state.events[id], + `A background event with the id of "${id}" does not exist.`, ); - const permission = permissions?.[SnapEndowments.Cronjob]; - const definitions = getCronjobCaveatJobs(permission); + assert( + this.state.events[id].snapId === origin, + 'Only the origin that scheduled this event can cancel it.', + ); - return definitions?.map((definition, idx) => { - return { ...definition, id: `${snapId}-${idx}`, snapId }; - }); + this.#cancel(id); } /** - * Register cron jobs for a given snap by getting specification from a permission caveats. - * Once registered, each job will be scheduled. + * Get a list of a Snap's background events. * - * @param snapId - ID of a snap. + * @param snapId - The id of the Snap to fetch background events for. + * @returns An array of background events. */ - register(snapId: SnapId) { - const jobs = this.#getSnapJobs(snapId); - jobs?.forEach((job) => this.#schedule(job)); + get(snapId: SnapId): InternalBackgroundEvent[] { + return Object.values(this.state.events).filter( + (snapEvent) => snapEvent.snapId === snapId, // && !snapEvent.recurring, + ); } /** - * Schedule a new job. - * This will interpret the cron expression and tell the timer to execute the job - * at the next suitable point in time. - * Job last run state will be initialized afterwards. + * Unregister all cronjobs and background events for a given Snap. * - * Note: Schedule will be skipped if the job's execution time is too far in the future and - * will be revisited on a daily check. - * - * @param job - Cronjob specification. + * @param snapId - ID of a snap. + * @param _skipEvents - Whether the unregistration process should skip + * scheduled background events. */ - #schedule(job: Cronjob) { - if (this.#timers.has(job.id)) { - return; + unregister(snapId: SnapId, _skipEvents = false) { + for (const [id, event] of Object.entries(this.state.events)) { + if (event.snapId === snapId) { + this.#cancel(id); + } } + } - const parsed = parseCronExpression(job.expression); - const next = parsed.next(); - const now = new Date(); - const ms = next.getTime() - now.getTime(); + /** + * Run controller teardown process and unsubscribe from Snap events. + */ + destroy() { + super.destroy(); - // Don't schedule this job yet as it is too far in the future - if (ms > DAILY_TIMEOUT) { - return; - } + /* eslint-disable @typescript-eslint/unbound-method */ + this.messagingSystem.unsubscribe( + 'SnapController:snapInstalled', + this.#handleSnapInstalledEvent, + ); - const timer = new Timer(ms); - timer.start(() => { - this.#executeCronjob(job).catch((error) => { - // TODO: Decide how to handle errors. - logError(error); - }); + this.messagingSystem.unsubscribe( + 'SnapController:snapUninstalled', + this.#handleSnapUninstalledEvent, + ); - this.#timers.delete(job.id); - this.#schedule(job); - }); + this.messagingSystem.unsubscribe( + 'SnapController:snapEnabled', + this.#handleSnapEnabledEvent, + ); - if (!this.state.jobs[job.id]?.lastRun) { - this.#updateJobLastRunState(job.id, 0); // 0 for init, never ran actually - } + this.messagingSystem.unsubscribe( + 'SnapController:snapDisabled', + this.#handleSnapDisabledEvent, + ); - this.#timers.set(job.id, timer); - this.#snapIds.set(job.id, job.snapId); - } + this.messagingSystem.unsubscribe( + 'SnapController:snapUpdated', + this.#handleSnapUpdatedEvent, + ); + /* eslint-enable @typescript-eslint/unbound-method */ - /** - * Execute job. - * - * @param job - Cronjob specification. - */ - async #executeCronjob(job: Cronjob) { - this.#updateJobLastRunState(job.id, Date.now()); - await this.messagingSystem.call('SnapController:handleRequest', { - snapId: job.snapId, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: job.request, - }); + // Cancel all timers and clear the map. + this.#timers.forEach((timer) => timer.cancel()); + this.#timers.clear(); } /** - * Schedule a background event. - * - * @param backgroundEventWithoutId - Background event. - * @returns An id representing the background event. + * Start the daily timer that will reschedule events every 24 hours. */ - scheduleBackgroundEvent( - backgroundEventWithoutId: Omit, - ) { - // Remove millisecond precision and convert to UTC. - const scheduledAt = DateTime.fromJSDate(new Date()) - .toUTC() - .startOf('second') - .toISO({ - suppressMilliseconds: true, - }); - - assert(scheduledAt); - - const event = { - ...backgroundEventWithoutId, - id: nanoid(), - scheduledAt, - }; - - this.#setUpBackgroundEvent(event); - - return event.id; + #start() { + this.#dailyTimer.start(() => this.#reschedule()); } /** - * Cancel a background event. + * Get the next execution date for a given event and start a timer for it. * - * @param origin - The origin making the cancel call. - * @param id - The id of the background event to cancel. - * @throws If the event does not exist. + * @param event - The event to schedule. */ - cancelBackgroundEvent(origin: string, id: string) { - assert( - this.state.events[id], - `A background event with the id of "${id}" does not exist.`, - ); - - assert( - this.state.events[id].snapId === origin, - 'Only the origin that scheduled this event can cancel it.', - ); - - const timer = this.#timers.get(id); - timer?.cancel(); - this.#timers.delete(id); - this.#snapIds.delete(id); + #schedule(event: InternalBackgroundEvent) { + const date = getExecutionDate(event.schedule); this.update((state) => { - delete state.events[id]; + state.events[event.id].date = date; + }); + + this.#startTimer({ + ...event, + date, }); } /** - * A helper function to handle setup of the background event. + * Set up and start a timer for the given event. * - * @param event - A background event. + * @param event - The event to schedule. + * @throws If the event is scheduled in the past. */ - #setUpBackgroundEvent(event: BackgroundEvent) { - const date = new Date(event.date); - const now = new Date(); - const ms = date.getTime() - now.getTime(); - + #startTimer(event: InternalBackgroundEvent) { + const ms = DateTime.fromISO(event.date).toMillis() - Date.now(); if (ms <= 0) { throw new Error('Cannot schedule an event in the past.'); } - // The event may already be in state when we get here. - if (!hasProperty(this.state.events, event.id)) { - this.update((state) => { - state.events[event.id] = castDraft(event); - }); + // We don't schedule this job yet as it is too far in the future. + if (ms > DAILY_TIMEOUT) { + return; } const timer = new Timer(ms); timer.start(() => { - this.messagingSystem - .call('SnapController:handleRequest', { - snapId: event.snapId, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: event.request, - }) - .catch((error) => { - logError(error); - }); - - this.#timers.delete(event.id); - this.#snapIds.delete(event.id); - this.update((state) => { - delete state.events[event.id]; - }); + this.#execute(event); }); this.#timers.set(event.id, timer); - this.#snapIds.set(event.id, event.snapId); } /** - * Get a list of a Snap's background events. + * Execute a background event. This method is called when the event's timer + * expires. * - * @param snapId - The id of the Snap to fetch background events for. - * @returns An array of background events. - */ - getBackgroundEvents(snapId: SnapId): BackgroundEvent[] { - return Object.values(this.state.events) - .filter((snapEvent) => snapEvent.snapId === snapId) - .map((event) => ({ - ...event, - // Truncate dates to remove milliseconds. - date: toCensoredISO8601String(event.date), - })); - } - - /** - * Unregister all jobs and background events related to the given snapId. + * If the event is not recurring, it will be removed from the state after + * execution. If it is recurring, it will be rescheduled. * - * @param snapId - ID of a snap. - * @param skipEvents - Whether the unregistration process should skip scheduled background events. + * @param event - The event to execute. */ - unregister(snapId: SnapId, skipEvents = false) { - const jobs = [...this.#snapIds.entries()].filter( - ([_, jobSnapId]) => jobSnapId === snapId, - ); + #execute(event: InternalBackgroundEvent) { + this.messagingSystem + .call('SnapController:handleRequest', { + snapId: event.snapId, + origin: METAMASK_ORIGIN, + handler: HandlerType.OnCronjob, + request: event.request, + }) + .catch((error) => { + logError(error); + }); + + this.#timers.delete(event.id); - if (jobs.length) { - const eventIds: string[] = []; - jobs.forEach(([id]) => { - const timer = this.#timers.get(id); - if (timer) { - timer.cancel(); - this.#timers.delete(id); - this.#snapIds.delete(id); - if (!skipEvents && this.state.events[id]) { - eventIds.push(id); - } - } + // Non-recurring events are removed from the state after execution, and + // recurring events are rescheduled. + if (!event.recurring) { + this.update((state) => { + delete state.events[event.id]; }); - if (eventIds.length > 0) { - this.update((state) => { - eventIds.forEach((id) => { - delete state.events[id]; - }); - }); - } + return; } + + this.#schedule(event); } /** - * Update time of a last run for the Cronjob specified by ID. + * Cancel a background event by its ID. Unlike {@link cancel}, this method + * does not check the origin of the event, so it can be used internally. * - * @param jobId - ID of a cron job. - * @param lastRun - Unix timestamp when the job was last ran. + * @param id - The ID of the background event to cancel. */ - #updateJobLastRunState(jobId: string, lastRun: number) { + #cancel(id: string) { + const timer = this.#timers.get(id); + timer?.cancel(); + this.#timers.delete(id); + this.update((state) => { - state.jobs[jobId] = { - lastRun, - }; + delete state.events[id]; }); } /** - * Runs every 24 hours to check if new jobs need to be scheduled. + * Retrieve all cronjob specifications for a Snap. * - * This is necessary for longer running jobs that execute with more than 24 hours between them. + * @param snapId - ID of a Snap. + * @returns Array of cronjob specifications. */ - dailyCheckIn() { - const jobs = this.#getAllJobs(); - - for (const job of jobs) { - const parsed = parseCronExpression(job.expression); - const lastRun = this.state.jobs[job.id]?.lastRun; - // If a job was supposed to run while we were shut down but wasn't we run it now - if ( - lastRun !== undefined && - parsed.hasPrev() && - parsed.prev().getTime() > lastRun - ) { - this.#executeCronjob(job).catch((error) => { - logError(error); - }); - } + #getSnapCronjobs(snapId: SnapId): SchedulableBackgroundEvent[] { + const permissions = this.messagingSystem.call( + 'PermissionController:getPermissions', + snapId, + ); - // Try scheduling, will fail if an existing scheduled job is found - this.#schedule(job); + const permission = permissions?.[SnapEndowments.Cronjob]; + const definitions = getCronjobCaveatJobs(permission); + + if (!definitions) { + return []; } - this.#dailyTimer = new Timer(DAILY_TIMEOUT); - this.#dailyTimer.start(() => { - this.dailyCheckIn(); + return definitions.map((definition, idx) => { + return { + snapId, + id: `cronjob-${snapId}-${idx}`, + request: definition.request, + schedule: definition.expression, + recurring: true, + }; }); } /** - * Reschedule background events. + * Register cronjobs for a given Snap by getting specification from the + * permission caveats. Once registered, each job will be scheduled. * - * @param backgroundEvents - A list of background events to reschdule. - */ - #rescheduleBackgroundEvents(backgroundEvents: BackgroundEvent[]) { - for (const snapEvent of backgroundEvents) { - const { date } = snapEvent; - const now = new Date(); - const then = new Date(date); - if (then.getTime() < now.getTime()) { - // Remove expired events from state - this.update((state) => { - delete state.events[snapEvent.id]; - }); - - logWarning( - `Background event with id "${snapEvent.id}" not scheduled as its date has expired.`, - ); - } else { - this.#setUpBackgroundEvent(snapEvent); - } - } - } - - /** - * Run controller teardown process and unsubscribe from Snap events. + * @param snapId - The snap ID to register jobs for. */ - destroy() { - super.destroy(); - - /* eslint-disable @typescript-eslint/unbound-method */ - this.messagingSystem.unsubscribe( - 'SnapController:snapInstalled', - this._handleSnapRegisterEvent, - ); - - this.messagingSystem.unsubscribe( - 'SnapController:snapUninstalled', - this._handleSnapUnregisterEvent, - ); - - this.messagingSystem.unsubscribe( - 'SnapController:snapEnabled', - this._handleSnapEnabledEvent, - ); - - this.messagingSystem.unsubscribe( - 'SnapController:snapDisabled', - this._handleSnapDisabledEvent, - ); - - this.messagingSystem.unsubscribe( - 'SnapController:snapUpdated', - this._handleEventSnapUpdated, - ); - /* eslint-enable @typescript-eslint/unbound-method */ - - this.#snapIds.forEach((snapId) => this.unregister(snapId)); + #register(snapId: SnapId) { + const jobs = this.#getSnapCronjobs(snapId); + jobs?.forEach((job) => this.schedule(job)); } /** - * Handle events that should cause cronjobs to be registered. + * Handle events that should cause cron jobs to be registered. * * @param snap - Basic Snap information. */ - // TODO: Either fix this lint violation or explain why it's necessary to - // ignore. - // eslint-disable-next-line no-restricted-syntax - private _handleSnapRegisterEvent(snap: TruncatedSnap) { - this.register(snap.id); + #handleSnapInstalledEvent(snap: TruncatedSnap) { + this.#register(snap.id); } /** - * Handle events that could cause cronjobs to be registered - * and for background events to be rescheduled. + * Handle the Snap enabled event. This checks if the Snap has any cronjobs or + * background events that need to be rescheduled. * * @param snap - Basic Snap information. */ - // TODO: Either fix this lint violation or explain why it's necessary to - // ignore. - // eslint-disable-next-line no-restricted-syntax - private _handleSnapEnabledEvent(snap: TruncatedSnap) { - const events = this.getBackgroundEvents(snap.id); - this.#rescheduleBackgroundEvents(events); - this.register(snap.id); + #handleSnapEnabledEvent(snap: TruncatedSnap) { + const events = this.get(snap.id); + this.#reschedule(events); + this.#register(snap.id); } /** - * Handle events that should cause cronjobs and background events to be unregistered. + * Handle events that should cause cronjobs and background events to be + * unregistered. * * @param snap - Basic Snap information. */ - // TODO: Either fix this lint violation or explain why it's necessary to - // ignore. - // eslint-disable-next-line no-restricted-syntax - private _handleSnapUnregisterEvent(snap: TruncatedSnap) { + #handleSnapUninstalledEvent(snap: TruncatedSnap) { this.unregister(snap.id); } /** - * Handle events that should cause cronjobs and background events to be unregistered. + * Handle events that should cause cronjobs and background events to be + * unregistered. * * @param snap - Basic Snap information. */ - // TODO: Either fix this lint violation or explain why it's necessary to - // ignore. - // eslint-disable-next-line no-restricted-syntax - private _handleSnapDisabledEvent(snap: TruncatedSnap) { + #handleSnapDisabledEvent(snap: TruncatedSnap) { + // TODO: Check why `skipEvents` is set to `true` here. this.unregister(snap.id, true); } @@ -622,11 +505,65 @@ export class CronjobController extends BaseController< * * @param snap - Basic Snap information. */ - // TODO: Either fix this lint violation or explain why it's necessary to - // ignore. - // eslint-disable-next-line no-restricted-syntax - private _handleEventSnapUpdated(snap: TruncatedSnap) { + #handleSnapUpdatedEvent(snap: TruncatedSnap) { this.unregister(snap.id); - this.register(snap.id); + this.#register(snap.id); + } + + /** + * Reschedule events that are yet to be executed. This should be called on + * controller initialization and once every 24 hours to ensure that + * background events are scheduled correctly. + * + * @param events - An array of events to reschedule. Defaults to all events in + * the controller state. + */ + #reschedule(events = Object.values(this.state.events)) { + const now = DateTime.fromJSDate(new Date()) + .toUTC() + .startOf('second') + .toSeconds(); + + for (const event of events) { + if (this.#timers.has(event.id)) { + // If the timer for this event already exists, we don't need to + // reschedule it. + continue; + } + + const eventDate = DateTime.fromISO(event.date) + .toUTC() + .startOf('second') + .toSeconds(); + + // If the event is recurring and the date is in the past, execute it + // immediately. + if (event.recurring && eventDate <= now) { + this.#execute(event); + } + + this.#schedule(event); + } + } + + /** + * Clear non-recurring events that are past their scheduled time. + */ + #clear() { + const now = DateTime.fromJSDate(new Date()) + .toUTC() + .startOf('second') + .toSeconds(); + + for (const event of Object.values(this.state.events)) { + const eventDate = DateTime.fromISO(event.date) + .toUTC() + .startOf('second') + .toSeconds(); + + if (!event.recurring && eventDate < now) { + this.#cancel(event.id); + } + } } } diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts new file mode 100644 index 0000000000..646d5ab027 --- /dev/null +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -0,0 +1,52 @@ +import { getCurrentDate, getExecutionDate } from './utils'; + +jest.useFakeTimers(); +jest.setSystemTime(1747994147000); + +describe('getCurrentDate', () => { + it('returns the current date in ISO 8601 format without milliseconds', () => { + expect(getCurrentDate()).toBe('2025-05-23T09:55:47Z'); + }); +}); + +describe('getExecutionDate', () => { + it('parses an ISO 8601 date', () => { + expect(getExecutionDate('2025-05-23T09:55:47Z')).toBe( + '2025-05-23T09:55:47Z', + ); + expect(getExecutionDate('2025-05-23T09:55:47+00:00')).toBe( + '2025-05-23T09:55:47Z', + ); + expect(getExecutionDate('2025-05-23T09:55:47+01:00')).toBe( + '2025-05-23T08:55:47Z', + ); + }); + + it('parses an ISO 8601 duration', () => { + expect(getExecutionDate('P1Y')).toBe('2026-05-23T09:55:47.000Z'); + expect(getExecutionDate('PT1S')).toBe('2025-05-23T09:55:48.000Z'); + expect(getExecutionDate('PT0S')).toBe('2025-05-23T09:55:48.000Z'); + }); + + it('parses a cron expression', () => { + expect(getExecutionDate('0 0 * * *')).toBe('2025-05-24T00:00:00.000Z'); + expect(getExecutionDate('0 0 1 * *')).toBe('2025-06-01T00:00:00.000Z'); + expect(getExecutionDate('0 0 1 1 *')).toBe('2026-01-01T00:00:00.000Z'); + expect(getExecutionDate('0 0 1 1 mon')).toBe('2026-01-01T00:00:00.000Z'); + }); + + it('throws an error for invalid input', () => { + expect(() => getExecutionDate('invalid')).toThrow( + 'Unable to parse "invalid" as ISO 8601 date, ISO 8601 duration, or cron expression.', + ); + expect(() => getExecutionDate('2025-05-23T09:55:47Z+01:00')).toThrow( + 'Unable to parse "2025-05-23T09:55:47Z+01:00" as ISO 8601 date, ISO 8601 duration, or cron expression.', + ); + expect(() => getExecutionDate('P1Y2M3D4H')).toThrow( + 'Unable to parse "P1Y2M3D4H" as ISO 8601 date, ISO 8601 duration, or cron expression.', + ); + expect(() => getExecutionDate('100 * * * * *')).toThrow( + 'Unable to parse "100 * * * * *" as ISO 8601 date, ISO 8601 duration, or cron expression.', + ); + }); +}); diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts new file mode 100644 index 0000000000..49b0f0b4a0 --- /dev/null +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -0,0 +1,69 @@ +import { assert } from '@metamask/utils'; +import { parseExpression } from 'cron-parser'; +import { DateTime, Duration } from 'luxon'; + +/** + * Get a duration with a minimum of 1 second. This function assumes the provided + * duration is valid. + * + * @param duration - The duration to validate. + * @returns The validated duration. + */ +function getDuration(duration: Duration): Duration { + if (duration.as('seconds') < 1) { + return Duration.fromObject({ seconds: 1 }); + } + + return duration; +} + +/** + * Get the current date in ISO 8601 format with millisecond precision removed. + * + * @returns The current date in ISO 8601 format. + */ +export function getCurrentDate() { + const date = DateTime.fromJSDate(new Date()).toUTC().startOf('second'); + assert(date.isValid); + + return date.toISO({ + suppressMilliseconds: true, + }); +} + +/** + * Get the next execution date from a schedule, which should be either: + * + * - An ISO 8601 date string, or + * - An ISO 8601 duration string, or + * - A cron expression. + * + * @param schedule - The schedule of the event. + * @returns The parsed ISO 8601 date at which the event should be executed. + */ +export function getExecutionDate(schedule: string) { + try { + const date = DateTime.fromISO(schedule); + if (date.isValid) { + return date.toUTC().startOf('second').toISO({ + suppressMilliseconds: true, + }); + } + + const duration = Duration.fromISO(schedule); + if (duration.isValid) { + const validatedDuration = getDuration(duration); + return DateTime.now().toUTC().plus(validatedDuration).toISO(); + } + + const parsed = parseExpression(schedule, { utc: true }); + const next = parsed.next(); + const nextDate = DateTime.fromJSDate(next.toDate()); + assert(nextDate.isValid); + return nextDate.toUTC().toISO(); + } catch { + throw new Error( + `Unable to parse "${schedule}" as ISO 8601 date, ISO 8601 duration, or cron expression.`, + ); + } +} diff --git a/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts b/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts index 248e94e94a..7699f6cadf 100644 --- a/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts +++ b/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts @@ -12,15 +12,9 @@ import { CronjobRpcRequestStruct, ISO8601DateStruct, ISO8601DurationStruct, - toCensoredISO8601String, } from '@metamask/snaps-utils'; import { StructError, create, object } from '@metamask/superstruct'; -import { - assert, - hasProperty, - type PendingJsonRpcResponse, -} from '@metamask/utils'; -import { DateTime, Duration } from 'luxon'; +import { hasProperty, type PendingJsonRpcResponse } from '@metamask/utils'; import { SnapEndowments } from '../endowments'; import type { MethodHooksObject } from '../utils'; @@ -33,7 +27,8 @@ const hookNames: MethodHooksObject = { }; type ScheduleBackgroundEventHookParams = { - date: string; + recurring: boolean; + schedule: string; request: CronjobRpcRequest; }; @@ -78,24 +73,18 @@ export type ScheduleBackgroundEventParameters = InferMatching< >; /** - * Generates a `DateTime` object based on if a duration or date is provided. + * Get the schedule for a background event based on the provided parameters. * - * @param params - The validated params from the `snap_scheduleBackgroundEvent` call. - * @returns A `DateTime` object. + * @param params - The parameters for the background event. + * @returns The schedule parameters for the background event. */ -function getStartDate(params: ScheduleBackgroundEventParams) { - if ('duration' in params) { - const parsed = Duration.fromISO(params.duration); - - // Disallow durations less than 1 second. - const duration = - parsed.as('seconds') >= 1 ? parsed : Duration.fromObject({ seconds: 1 }); - - return DateTime.fromJSDate(new Date()).toUTC().plus(duration).toISO(); +function getSchedule(params: ScheduleBackgroundEventParameters): string { + if (hasProperty(params, 'date')) { + // TODO: Check why `params.date` is not a string. + return params.date as string; } - // Make sure any millisecond precision is removed. - return toCensoredISO8601String(params.date); + return params.duration; } /** @@ -129,14 +118,15 @@ async function getScheduleBackgroundEventImplementation( try { const validatedParams = getValidatedParams(params); - const { request } = validatedParams; + const schedule = getSchedule(validatedParams); - const date = getStartDate(validatedParams); - - assert(date); + const id = scheduleBackgroundEvent({ + recurring: false, + schedule, + request, + }); - const id = scheduleBackgroundEvent({ date, request }); res.result = id; } catch (error) { return end(error); diff --git a/packages/snaps-sdk/src/types/methods/get-background-events.ts b/packages/snaps-sdk/src/types/methods/get-background-events.ts index b2f322c038..77014ba819 100644 --- a/packages/snaps-sdk/src/types/methods/get-background-events.ts +++ b/packages/snaps-sdk/src/types/methods/get-background-events.ts @@ -3,15 +3,18 @@ import type { Json } from '@metamask/utils'; import type { SnapId } from '../snap'; /** - * Background event type + * Background event type. * - * Note: The date generated when scheduling an event with a duration will be represented in UTC. + * Note: The date generated when scheduling an event with a duration will be + * represented in UTC. * * @property id - The unique id representing the event. - * @property scheduledAt - The ISO 8601 time stamp of when the event was scheduled. + * @property scheduledAt - The ISO 8601 time stamp of when the event was + * scheduled. * @property snapId - The id of the snap that scheduled the event. * @property date - The ISO 8601 date of when the event is scheduled for. - * @property request - The request that is supplied to the `onCronjob` handler when the event is fired. + * @property request - The request that is supplied to the `onCronjob` handler + * when the event is fired. */ export type BackgroundEvent = { id: string; diff --git a/packages/snaps-utils/src/cronjob.ts b/packages/snaps-utils/src/cronjob.ts index 694ce762c0..ef31d9ebc4 100644 --- a/packages/snaps-utils/src/cronjob.ts +++ b/packages/snaps-utils/src/cronjob.ts @@ -1,3 +1,4 @@ +import { union } from '@metamask/snaps-sdk'; import type { Infer } from '@metamask/superstruct'; import { array, @@ -14,6 +15,8 @@ import { } from '@metamask/utils'; import { parseExpression } from 'cron-parser'; +import { ISO8601DurationStruct } from './time'; + export const CronjobRpcRequestStruct = object({ jsonrpc: optional(JsonRpcVersionStruct), id: optional(JsonRpcIdStruct), @@ -25,7 +28,7 @@ export type CronjobRpcRequest = Infer; export const CronExpressionStruct = refine( string(), - 'CronExpression', + 'cronjob expression', (value) => { try { parseExpression(value); @@ -50,9 +53,10 @@ export function parseCronExpression(expression: string | object) { } export const CronjobSpecificationStruct = object({ - expression: CronExpressionStruct, + expression: union([CronExpressionStruct, ISO8601DurationStruct]), request: CronjobRpcRequestStruct, }); + export type CronjobSpecification = Infer; /** diff --git a/packages/snaps-utils/src/time.ts b/packages/snaps-utils/src/time.ts index 8f5916890e..bafa6d282f 100644 --- a/packages/snaps-utils/src/time.ts +++ b/packages/snaps-utils/src/time.ts @@ -12,6 +12,7 @@ export const ISO8601DurationStruct = refine( if (!parsedDuration.isValid) { return 'Not a valid ISO 8601 duration'; } + return true; }, ); diff --git a/yarn.lock b/yarn.lock index 31db6e99d2..fd866d1c0c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4244,6 +4244,7 @@ __metadata: "@xstate/fsm": "npm:^2.0.0" async-mutex: "npm:^0.5.0" concat-stream: "npm:^2.0.0" + cron-parser: "npm:^4.5.0" deepmerge: "npm:^4.2.2" depcheck: "npm:^1.4.7" eslint: "npm:^9.11.0" From fa6a2517cf518af038a74f43cf5b6745cf2ee460 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 26 May 2025 11:05:02 +0200 Subject: [PATCH 02/16] Disable millisecond precision for durations and cron expressions --- .../snaps-controllers/src/cronjob/utils.test.ts | 14 +++++++------- packages/snaps-controllers/src/cronjob/utils.ts | 9 +++++++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts index 646d5ab027..2adb0a5d81 100644 --- a/packages/snaps-controllers/src/cronjob/utils.test.ts +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -23,16 +23,16 @@ describe('getExecutionDate', () => { }); it('parses an ISO 8601 duration', () => { - expect(getExecutionDate('P1Y')).toBe('2026-05-23T09:55:47.000Z'); - expect(getExecutionDate('PT1S')).toBe('2025-05-23T09:55:48.000Z'); - expect(getExecutionDate('PT0S')).toBe('2025-05-23T09:55:48.000Z'); + expect(getExecutionDate('P1Y')).toBe('2026-05-23T09:55:47Z'); + expect(getExecutionDate('PT1S')).toBe('2025-05-23T09:55:48Z'); + expect(getExecutionDate('PT0S')).toBe('2025-05-23T09:55:48Z'); }); it('parses a cron expression', () => { - expect(getExecutionDate('0 0 * * *')).toBe('2025-05-24T00:00:00.000Z'); - expect(getExecutionDate('0 0 1 * *')).toBe('2025-06-01T00:00:00.000Z'); - expect(getExecutionDate('0 0 1 1 *')).toBe('2026-01-01T00:00:00.000Z'); - expect(getExecutionDate('0 0 1 1 mon')).toBe('2026-01-01T00:00:00.000Z'); + expect(getExecutionDate('0 0 * * *')).toBe('2025-05-24T00:00:00Z'); + expect(getExecutionDate('0 0 1 * *')).toBe('2025-06-01T00:00:00Z'); + expect(getExecutionDate('0 0 1 1 *')).toBe('2026-01-01T00:00:00Z'); + expect(getExecutionDate('0 0 1 1 mon')).toBe('2026-01-01T00:00:00Z'); }); it('throws an error for invalid input', () => { diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts index 49b0f0b4a0..66d0c1c158 100644 --- a/packages/snaps-controllers/src/cronjob/utils.ts +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -53,14 +53,19 @@ export function getExecutionDate(schedule: string) { const duration = Duration.fromISO(schedule); if (duration.isValid) { const validatedDuration = getDuration(duration); - return DateTime.now().toUTC().plus(validatedDuration).toISO(); + return DateTime.now().toUTC().plus(validatedDuration).toISO({ + suppressMilliseconds: true, + }); } const parsed = parseExpression(schedule, { utc: true }); const next = parsed.next(); const nextDate = DateTime.fromJSDate(next.toDate()); assert(nextDate.isValid); - return nextDate.toUTC().toISO(); + + return nextDate.toUTC().toISO({ + suppressMilliseconds: true, + }); } catch { throw new Error( `Unable to parse "${schedule}" as ISO 8601 date, ISO 8601 duration, or cron expression.`, From 34f31fb6f29ec1189ccb7b1f7bba19257302efd2 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 26 May 2025 13:28:02 +0200 Subject: [PATCH 03/16] Use `duration` field for cronjobs --- .../src/cronjob/CronjobController.ts | 8 +- .../src/cronjob/utils.test.ts | 26 +++- .../snaps-controllers/src/cronjob/utils.ts | 24 +++- packages/snaps-utils/coverage.json | 8 +- packages/snaps-utils/src/cronjob.test.ts | 132 ++++++++++++------ packages/snaps-utils/src/cronjob.ts | 32 +++-- 6 files changed, 164 insertions(+), 66 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 31811efb62..c614c7f714 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -17,7 +17,11 @@ import { castDraft } from 'immer'; import { DateTime } from 'luxon'; import { nanoid } from 'nanoid'; -import { getCurrentDate, getExecutionDate } from './utils'; +import { + getCronjobSpecificationSchedule, + getCurrentDate, + getExecutionDate, +} from './utils'; import type { GetAllSnaps, HandleSnapRequest, @@ -441,7 +445,7 @@ export class CronjobController extends BaseController< snapId, id: `cronjob-${snapId}-${idx}`, request: definition.request, - schedule: definition.expression, + schedule: getCronjobSpecificationSchedule(definition), recurring: true, }; }); diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts index 2adb0a5d81..c6374167a7 100644 --- a/packages/snaps-controllers/src/cronjob/utils.test.ts +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -1,8 +1,32 @@ -import { getCurrentDate, getExecutionDate } from './utils'; +import { + getCronjobSpecificationSchedule, + getCurrentDate, + getExecutionDate, +} from './utils'; jest.useFakeTimers(); jest.setSystemTime(1747994147000); +describe('getCronjobSpecificationSchedule', () => { + it('returns the duration if specified', () => { + const specification = { + duration: 'PT1H', + request: { method: 'foo' }, + }; + + expect(getCronjobSpecificationSchedule(specification)).toBe('PT1H'); + }); + + it('returns the expression if no duration is specified', () => { + const specification = { + expression: '0 0 * * *', + request: { method: 'foo' }, + }; + + expect(getCronjobSpecificationSchedule(specification)).toBe('0 0 * * *'); + }); +}); + describe('getCurrentDate', () => { it('returns the current date in ISO 8601 format without milliseconds', () => { expect(getCurrentDate()).toBe('2025-05-23T09:55:47Z'); diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts index 66d0c1c158..2194a33910 100644 --- a/packages/snaps-controllers/src/cronjob/utils.ts +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -1,7 +1,29 @@ -import { assert } from '@metamask/utils'; +import type { CronjobSpecification } from '@metamask/snaps-utils'; +import { assert, hasProperty } from '@metamask/utils'; import { parseExpression } from 'cron-parser'; import { DateTime, Duration } from 'luxon'; +/** + * Get the schedule from a cronjob specification. + * + * This function assumes the cronjob specification is valid and contains + * either a `duration` or an `expression` property. + * + * @param specification - The cronjob specification to extract the schedule + * from. + * @returns The schedule of the cronjob, which can be either an ISO 8601 + * duration or a cron expression. + */ +export function getCronjobSpecificationSchedule( + specification: CronjobSpecification, +) { + if (hasProperty(specification, 'duration')) { + return specification.duration as string; + } + + return specification.expression; +} + /** * Get a duration with a minimum of 1 second. This function assumes the provided * duration is valid. diff --git a/packages/snaps-utils/coverage.json b/packages/snaps-utils/coverage.json index 4d476822af..976413843d 100644 --- a/packages/snaps-utils/coverage.json +++ b/packages/snaps-utils/coverage.json @@ -1,6 +1,6 @@ { - "branches": 99.76, - "functions": 99, - "lines": 98.68, - "statements": 97.2 + "branches": 99.77, + "functions": 99.02, + "lines": 98.67, + "statements": 97.33 } diff --git a/packages/snaps-utils/src/cronjob.test.ts b/packages/snaps-utils/src/cronjob.test.ts index c7ba2d0605..26e19e94e8 100644 --- a/packages/snaps-utils/src/cronjob.test.ts +++ b/packages/snaps-utils/src/cronjob.test.ts @@ -1,69 +1,113 @@ +import { create } from '@metamask/superstruct'; + import { + CronjobSpecificationStruct, isCronjobSpecification, isCronjobSpecificationArray, - parseCronExpression, } from './cronjob'; -describe('Cronjob Utilities', () => { - describe('isCronjobSpecification', () => { - it('returns true for a valid cronjob specification', () => { - const cronjobSpecification = { - expression: '* * * * *', +describe('CronjobSpecificationStruct', () => { + it.each([ + ['100 * * * * *', 'Expected an object, but received: "100 * * * * *"'], + ['PT10S', 'Expected an object, but received: "PT10S"'], + [ + { + expression: '100 * * * * *', request: { method: 'exampleMethodOne', params: ['p1'], }, - }; - expect(isCronjobSpecification(cronjobSpecification)).toBe(true); - }); - - it('returns false for an invalid cronjob specification', () => { - const cronjobSpecification = { - expression: '* * * * * * * * * * *', + }, + 'At path: expression -- Expected a cronjob expression, but received: "100 * * * * *"', + ], + [ + { + duration: '10S', + request: { + method: 'exampleMethodOne', + params: ['p1'], + }, + }, + 'At path: duration -- Not a valid ISO 8601 duration', + ], + [ + { request: { method: 'exampleMethodOne', params: ['p1'], }, - }; - expect(isCronjobSpecification(cronjobSpecification)).toBe(false); - }); + }, + 'At path: expression -- Expected a string, but received: undefined', + ], + ])('return a readable error for %p', (value, error) => { + expect(() => create(value, CronjobSpecificationStruct)).toThrow(error); }); +}); - describe('isCronjobSpecificationArray', () => { - it('returns true for a valid cronjob specification array', () => { - const cronjobSpecificationArray = [ - { - expression: '* * * * *', - request: { - method: 'exampleMethodOne', - params: ['p1'], - }, - }, - ]; - expect(isCronjobSpecificationArray(cronjobSpecificationArray)).toBe(true); - }); +describe('isCronjobSpecification', () => { + it('returns true for a valid cronjob specification with an expression', () => { + const cronjobSpecification = { + expression: '* * * * *', + request: { + method: 'exampleMethodOne', + params: ['p1'], + }, + }; + expect(isCronjobSpecification(cronjobSpecification)).toBe(true); + }); - it('returns false for an invalid cronjob specification array', () => { - const cronjobSpecificationArray = { + it('returns true for a valid cronjob specification with a duration', () => { + const cronjobSpecification = { + duration: 'PT10S', + request: { + method: 'exampleMethodOne', + params: ['p1'], + }, + }; + expect(isCronjobSpecification(cronjobSpecification)).toBe(true); + }); + + it('returns false for an invalid cronjob specification', () => { + const cronjobSpecification = { + expression: '* * * * * * * * * * *', + request: { + method: 'exampleMethodOne', + params: ['p1'], + }, + }; + expect(isCronjobSpecification(cronjobSpecification)).toBe(false); + }); +}); + +describe('isCronjobSpecificationArray', () => { + it('returns true for a valid cronjob specification array', () => { + const cronjobSpecificationArray = [ + { expression: '* * * * *', request: { method: 'exampleMethodOne', params: ['p1'], }, - }; - expect(isCronjobSpecificationArray(cronjobSpecificationArray)).toBe( - false, - ); - }); + }, + { + duration: 'PT10S', + request: { + method: 'exampleMethodOne', + params: ['p1'], + }, + }, + ]; + expect(isCronjobSpecificationArray(cronjobSpecificationArray)).toBe(true); }); - describe('parseCronExpression', () => { - it('successfully parses cronjob expression that is provided as a string', () => { - const cronjobExpression = '* * * * *'; - - const parsedExpression = parseCronExpression(cronjobExpression); - expect(parsedExpression.next()).toBeDefined(); - expect(typeof parsedExpression.next().getTime()).toBe('number'); - }); + it('returns false for an invalid cronjob specification array', () => { + const cronjobSpecificationArray = { + expression: '* * * * *', + request: { + method: 'exampleMethodOne', + params: ['p1'], + }, + }; + expect(isCronjobSpecificationArray(cronjobSpecificationArray)).toBe(false); }); }); diff --git a/packages/snaps-utils/src/cronjob.ts b/packages/snaps-utils/src/cronjob.ts index ef31d9ebc4..35df3bbb35 100644 --- a/packages/snaps-utils/src/cronjob.ts +++ b/packages/snaps-utils/src/cronjob.ts @@ -1,4 +1,4 @@ -import { union } from '@metamask/snaps-sdk'; +import { selectiveUnion } from '@metamask/snaps-sdk'; import type { Infer } from '@metamask/superstruct'; import { array, @@ -9,6 +9,8 @@ import { string, } from '@metamask/superstruct'; import { + hasProperty, + isObject, JsonRpcIdStruct, JsonRpcParamsStruct, JsonRpcVersionStruct, @@ -34,29 +36,31 @@ export const CronExpressionStruct = refine( parseExpression(value); return true; } catch { - return false; + return `Expected a cronjob expression, but received: "${value}"`; } }, ); export type CronExpression = Infer; -/** - * Parses a cron expression. - * - * @param expression - Expression to parse. - * @returns A CronExpression class instance. - */ -export function parseCronExpression(expression: string | object) { - const ensureStringExpression = create(expression, CronExpressionStruct); - return parseExpression(ensureStringExpression); -} +const CronjobExpressionSpecificationStruct = object({ + expression: CronExpressionStruct, + request: CronjobRpcRequestStruct, +}); -export const CronjobSpecificationStruct = object({ - expression: union([CronExpressionStruct, ISO8601DurationStruct]), +const CronjobDurationSpecificationStruct = object({ + duration: ISO8601DurationStruct, request: CronjobRpcRequestStruct, }); +export const CronjobSpecificationStruct = selectiveUnion((value) => { + if (isObject(value) && hasProperty(value, 'duration')) { + return CronjobDurationSpecificationStruct; + } + + return CronjobExpressionSpecificationStruct; +}); + export type CronjobSpecification = Infer; /** From 57603c5141837136c126e905830a0fb6ada17cad Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 26 May 2025 13:47:49 +0200 Subject: [PATCH 04/16] Fix manifest permission types --- packages/snaps-sdk/src/types/permissions.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/snaps-sdk/src/types/permissions.ts b/packages/snaps-sdk/src/types/permissions.ts index 2bf1b60113..1427b6bce3 100644 --- a/packages/snaps-sdk/src/types/permissions.ts +++ b/packages/snaps-sdk/src/types/permissions.ts @@ -3,11 +3,20 @@ import type { CaipChainId, JsonRpcRequest } from '@metamask/utils'; export type EmptyObject = Record; -export type Cronjob = { - expression: string; +type CronjobRequest = { request: Omit; }; +type CronjobWithExpression = CronjobRequest & { + expression: string; +}; + +type CronjobWithDuration = CronjobRequest & { + duration: string; +}; + +export type Cronjob = CronjobWithExpression | CronjobWithDuration; + export type NameLookupMatchers = | { tlds: string[]; From 3b53314e00e7aabaf12c4fd83e03cb54abc14977 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 26 May 2025 13:56:40 +0200 Subject: [PATCH 05/16] Fix `destroy` logic to properly unsubscribe --- .../src/cronjob/CronjobController.ts | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index c614c7f714..a6324285cf 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -168,27 +168,27 @@ export class CronjobController extends BaseController< this.messagingSystem.subscribe( 'SnapController:snapInstalled', - this.#handleSnapInstalledEvent.bind(this), + this.#handleSnapInstalledEvent, ); this.messagingSystem.subscribe( 'SnapController:snapUninstalled', - this.#handleSnapUninstalledEvent.bind(this), + this.#handleSnapUninstalledEvent, ); this.messagingSystem.subscribe( 'SnapController:snapEnabled', - this.#handleSnapEnabledEvent.bind(this), + this.#handleSnapEnabledEvent, ); this.messagingSystem.subscribe( 'SnapController:snapDisabled', - this.#handleSnapDisabledEvent.bind(this), + this.#handleSnapDisabledEvent, ); this.messagingSystem.subscribe( 'SnapController:snapUpdated', - this.#handleSnapUpdatedEvent.bind(this), + this.#handleSnapUpdatedEvent, ); this.messagingSystem.registerActionHandler( @@ -263,7 +263,7 @@ export class CronjobController extends BaseController< */ get(snapId: SnapId): InternalBackgroundEvent[] { return Object.values(this.state.events).filter( - (snapEvent) => snapEvent.snapId === snapId, // && !snapEvent.recurring, + (snapEvent) => snapEvent.snapId === snapId && !snapEvent.recurring, ); } @@ -288,7 +288,6 @@ export class CronjobController extends BaseController< destroy() { super.destroy(); - /* eslint-disable @typescript-eslint/unbound-method */ this.messagingSystem.unsubscribe( 'SnapController:snapInstalled', this.#handleSnapInstalledEvent, @@ -313,7 +312,6 @@ export class CronjobController extends BaseController< 'SnapController:snapUpdated', this.#handleSnapUpdatedEvent, ); - /* eslint-enable @typescript-eslint/unbound-method */ // Cancel all timers and clear the map. this.#timers.forEach((timer) => timer.cancel()); @@ -467,9 +465,9 @@ export class CronjobController extends BaseController< * * @param snap - Basic Snap information. */ - #handleSnapInstalledEvent(snap: TruncatedSnap) { + readonly #handleSnapInstalledEvent = (snap: TruncatedSnap) => { this.#register(snap.id); - } + }; /** * Handle the Snap enabled event. This checks if the Snap has any cronjobs or @@ -477,11 +475,11 @@ export class CronjobController extends BaseController< * * @param snap - Basic Snap information. */ - #handleSnapEnabledEvent(snap: TruncatedSnap) { + readonly #handleSnapEnabledEvent = (snap: TruncatedSnap) => { const events = this.get(snap.id); this.#reschedule(events); this.#register(snap.id); - } + }; /** * Handle events that should cause cronjobs and background events to be @@ -489,9 +487,9 @@ export class CronjobController extends BaseController< * * @param snap - Basic Snap information. */ - #handleSnapUninstalledEvent(snap: TruncatedSnap) { + readonly #handleSnapUninstalledEvent = (snap: TruncatedSnap) => { this.unregister(snap.id); - } + }; /** * Handle events that should cause cronjobs and background events to be @@ -499,20 +497,20 @@ export class CronjobController extends BaseController< * * @param snap - Basic Snap information. */ - #handleSnapDisabledEvent(snap: TruncatedSnap) { + readonly #handleSnapDisabledEvent = (snap: TruncatedSnap) => { // TODO: Check why `skipEvents` is set to `true` here. this.unregister(snap.id, true); - } + }; /** * Handle cron jobs on 'snapUpdated' event. * * @param snap - Basic Snap information. */ - #handleSnapUpdatedEvent(snap: TruncatedSnap) { + readonly #handleSnapUpdatedEvent = (snap: TruncatedSnap) => { this.unregister(snap.id); this.#register(snap.id); - } + }; /** * Reschedule events that are yet to be executed. This should be called on From ceb222ee98eb1026c86771ba692e1c514360be3d Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 26 May 2025 15:06:24 +0200 Subject: [PATCH 06/16] Address some feedback --- .../src/cronjob/CronjobController.ts | 98 ++++++++++++------- .../src/cronjob/utils.test.ts | 26 ++--- .../snaps-controllers/src/cronjob/utils.ts | 24 +---- packages/snaps-rpc-methods/jest.config.js | 4 +- .../permitted/scheduleBackgroundEvent.test.ts | 56 +---------- .../src/permitted/scheduleBackgroundEvent.ts | 2 - 6 files changed, 78 insertions(+), 132 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index a6324285cf..b0fab95cf3 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -11,17 +11,17 @@ import { } from '@metamask/snaps-rpc-methods'; import type { BackgroundEvent, SnapId } from '@metamask/snaps-sdk'; import type { TruncatedSnap } from '@metamask/snaps-utils'; -import { HandlerType, logError } from '@metamask/snaps-utils'; +import { + toCensoredISO8601String, + HandlerType, + logError, +} from '@metamask/snaps-utils'; import { assert, Duration, inMilliseconds } from '@metamask/utils'; import { castDraft } from 'immer'; import { DateTime } from 'luxon'; import { nanoid } from 'nanoid'; -import { - getCronjobSpecificationSchedule, - getCurrentDate, - getExecutionDate, -} from './utils'; +import { getCronjobSpecificationSchedule, getExecutionDate } from './utils'; import type { GetAllSnaps, HandleSnapRequest, @@ -212,26 +212,16 @@ export class CronjobController extends BaseController< } /** - * Schedule an event. + * Schedule a non-recurring background event. * * @param event - The event to schedule. * @returns The ID of the scheduled event. */ - schedule(event: SchedulableBackgroundEvent) { - const id = event.id ?? nanoid(); - const internalEvent: InternalBackgroundEvent = { + schedule(event: Omit) { + return this.#add({ ...event, - id, - date: getExecutionDate(event.schedule), - scheduledAt: getCurrentDate(), - }; - - this.update((state) => { - state.events[internalEvent.id] = castDraft(internalEvent); + recurring: false, }); - - this.#schedule(internalEvent); - return id; } /** @@ -262,9 +252,26 @@ export class CronjobController extends BaseController< * @returns An array of background events. */ get(snapId: SnapId): InternalBackgroundEvent[] { - return Object.values(this.state.events).filter( - (snapEvent) => snapEvent.snapId === snapId && !snapEvent.recurring, - ); + return Object.values(this.state.events) + .filter( + (snapEvent) => snapEvent.snapId === snapId && !snapEvent.recurring, + ) + .map((event) => ({ + ...event, + date: toCensoredISO8601String(event.date), + scheduledAt: toCensoredISO8601String(event.scheduledAt), + })); + } + + /** + * Register cronjobs for a given Snap by getting specification from the + * permission caveats. Once registered, each job will be scheduled. + * + * @param snapId - The snap ID to register jobs for. + */ + register(snapId: SnapId) { + const jobs = this.#getSnapCronjobs(snapId); + jobs?.forEach((job) => this.#add(job)); } /** @@ -325,6 +332,30 @@ export class CronjobController extends BaseController< this.#dailyTimer.start(() => this.#reschedule()); } + /** + * Add a cronjob or background event to the controller state and schedule it + * for execution. + * + * @param event - The event to schedule. + * @returns The ID of the scheduled event. + */ + #add(event: SchedulableBackgroundEvent) { + const id = event.id ?? nanoid(); + const internalEvent: InternalBackgroundEvent = { + ...event, + id, + date: getExecutionDate(event.schedule), + scheduledAt: new Date().toISOString(), + }; + + this.update((state) => { + state.events[internalEvent.id] = castDraft(internalEvent); + }); + + this.#schedule(internalEvent); + return id; + } + /** * Get the next execution date for a given event and start a timer for it. * @@ -349,7 +380,9 @@ export class CronjobController extends BaseController< * @throws If the event is scheduled in the past. */ #startTimer(event: InternalBackgroundEvent) { - const ms = DateTime.fromISO(event.date).toMillis() - Date.now(); + const ms = + DateTime.fromISO(event.date, { setZone: true }).toMillis() - Date.now(); + if (ms <= 0) { throw new Error('Cannot schedule an event in the past.'); } @@ -449,24 +482,13 @@ export class CronjobController extends BaseController< }); } - /** - * Register cronjobs for a given Snap by getting specification from the - * permission caveats. Once registered, each job will be scheduled. - * - * @param snapId - The snap ID to register jobs for. - */ - #register(snapId: SnapId) { - const jobs = this.#getSnapCronjobs(snapId); - jobs?.forEach((job) => this.schedule(job)); - } - /** * Handle events that should cause cron jobs to be registered. * * @param snap - Basic Snap information. */ readonly #handleSnapInstalledEvent = (snap: TruncatedSnap) => { - this.#register(snap.id); + this.register(snap.id); }; /** @@ -478,7 +500,7 @@ export class CronjobController extends BaseController< readonly #handleSnapEnabledEvent = (snap: TruncatedSnap) => { const events = this.get(snap.id); this.#reschedule(events); - this.#register(snap.id); + this.register(snap.id); }; /** @@ -509,7 +531,7 @@ export class CronjobController extends BaseController< */ readonly #handleSnapUpdatedEvent = (snap: TruncatedSnap) => { this.unregister(snap.id); - this.#register(snap.id); + this.register(snap.id); }; /** diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts index c6374167a7..2792619409 100644 --- a/packages/snaps-controllers/src/cronjob/utils.test.ts +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -1,8 +1,4 @@ -import { - getCronjobSpecificationSchedule, - getCurrentDate, - getExecutionDate, -} from './utils'; +import { getCronjobSpecificationSchedule, getExecutionDate } from './utils'; jest.useFakeTimers(); jest.setSystemTime(1747994147000); @@ -27,12 +23,6 @@ describe('getCronjobSpecificationSchedule', () => { }); }); -describe('getCurrentDate', () => { - it('returns the current date in ISO 8601 format without milliseconds', () => { - expect(getCurrentDate()).toBe('2025-05-23T09:55:47Z'); - }); -}); - describe('getExecutionDate', () => { it('parses an ISO 8601 date', () => { expect(getExecutionDate('2025-05-23T09:55:47Z')).toBe( @@ -47,16 +37,16 @@ describe('getExecutionDate', () => { }); it('parses an ISO 8601 duration', () => { - expect(getExecutionDate('P1Y')).toBe('2026-05-23T09:55:47Z'); - expect(getExecutionDate('PT1S')).toBe('2025-05-23T09:55:48Z'); - expect(getExecutionDate('PT0S')).toBe('2025-05-23T09:55:48Z'); + expect(getExecutionDate('P1Y')).toBe('2026-05-23T09:55:47.000Z'); + expect(getExecutionDate('PT1S')).toBe('2025-05-23T09:55:48.000Z'); + expect(getExecutionDate('PT0S')).toBe('2025-05-23T09:55:48.000Z'); }); it('parses a cron expression', () => { - expect(getExecutionDate('0 0 * * *')).toBe('2025-05-24T00:00:00Z'); - expect(getExecutionDate('0 0 1 * *')).toBe('2025-06-01T00:00:00Z'); - expect(getExecutionDate('0 0 1 1 *')).toBe('2026-01-01T00:00:00Z'); - expect(getExecutionDate('0 0 1 1 mon')).toBe('2026-01-01T00:00:00Z'); + expect(getExecutionDate('0 0 * * *')).toBe('2025-05-24T00:00:00.000Z'); + expect(getExecutionDate('0 0 1 * *')).toBe('2025-06-01T00:00:00.000Z'); + expect(getExecutionDate('0 0 1 1 *')).toBe('2026-01-01T00:00:00.000Z'); + expect(getExecutionDate('0 0 1 1 mon')).toBe('2026-01-01T00:00:00.000Z'); }); it('throws an error for invalid input', () => { diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts index 2194a33910..b39c88bfeb 100644 --- a/packages/snaps-controllers/src/cronjob/utils.ts +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -39,20 +39,6 @@ function getDuration(duration: Duration): Duration { return duration; } -/** - * Get the current date in ISO 8601 format with millisecond precision removed. - * - * @returns The current date in ISO 8601 format. - */ -export function getCurrentDate() { - const date = DateTime.fromJSDate(new Date()).toUTC().startOf('second'); - assert(date.isValid); - - return date.toISO({ - suppressMilliseconds: true, - }); -} - /** * Get the next execution date from a schedule, which should be either: * @@ -67,6 +53,7 @@ export function getExecutionDate(schedule: string) { try { const date = DateTime.fromISO(schedule); if (date.isValid) { + // We round to the nearest second to avoid milliseconds in the output. return date.toUTC().startOf('second').toISO({ suppressMilliseconds: true, }); @@ -74,10 +61,9 @@ export function getExecutionDate(schedule: string) { const duration = Duration.fromISO(schedule); if (duration.isValid) { + // This ensures the duration is at least 1 second. const validatedDuration = getDuration(duration); - return DateTime.now().toUTC().plus(validatedDuration).toISO({ - suppressMilliseconds: true, - }); + return DateTime.now().toUTC().plus(validatedDuration).toISO(); } const parsed = parseExpression(schedule, { utc: true }); @@ -85,9 +71,7 @@ export function getExecutionDate(schedule: string) { const nextDate = DateTime.fromJSDate(next.toDate()); assert(nextDate.isValid); - return nextDate.toUTC().toISO({ - suppressMilliseconds: true, - }); + return nextDate.toUTC().toISO(); } catch { throw new Error( `Unable to parse "${schedule}" as ISO 8601 date, ISO 8601 duration, or cron expression.`, diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js index 399f895c89..8986c36f9d 100644 --- a/packages/snaps-rpc-methods/jest.config.js +++ b/packages/snaps-rpc-methods/jest.config.js @@ -11,9 +11,9 @@ module.exports = deepmerge(baseConfig, { coverageThreshold: { global: { branches: 95, - functions: 98.64, + functions: 98.65, lines: 98.78, - statements: 98.46, + statements: 98.47, }, }, }); diff --git a/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.test.ts b/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.test.ts index c81eaaccf5..2a95b90d96 100644 --- a/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.test.ts +++ b/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.test.ts @@ -83,7 +83,7 @@ describe('snap_scheduleBackgroundEvent', () => { expect(response).toStrictEqual({ jsonrpc: '2.0', id: 1, result: 'foo' }); }); - it('schedules a background event with second precision', async () => { + it('schedules a background event', async () => { const { implementation } = scheduleBackgroundEventHandler; const scheduleBackgroundEvent = jest.fn(); @@ -123,7 +123,7 @@ describe('snap_scheduleBackgroundEvent', () => { }); expect(scheduleBackgroundEvent).toHaveBeenCalledWith({ - date: '2022-01-01T01:00:35+02:00', + schedule: '2022-01-01T01:00:35.786+02:00', request: { method: 'handleExport', params: ['p1'], @@ -131,7 +131,7 @@ describe('snap_scheduleBackgroundEvent', () => { }); }); - it('schedules a background event using duration', async () => { + it('schedules a background event using a duration', async () => { const { implementation } = scheduleBackgroundEventHandler; const scheduleBackgroundEvent = jest.fn(); @@ -171,55 +171,7 @@ describe('snap_scheduleBackgroundEvent', () => { }); expect(scheduleBackgroundEvent).toHaveBeenCalledWith({ - date: '2025-05-21T13:25:21.500Z', - request: { - method: 'handleExport', - params: ['p1'], - }, - }); - }); - - it('schedules a background event using a minimum duration of 1 second', async () => { - const { implementation } = scheduleBackgroundEventHandler; - - const scheduleBackgroundEvent = jest.fn(); - const hasPermission = jest.fn().mockImplementation(() => true); - - const hooks = { - scheduleBackgroundEvent, - hasPermission, - }; - - const engine = new JsonRpcEngine(); - - engine.push(createOriginMiddleware(MOCK_SNAP_ID)); - engine.push((request, response, next, end) => { - const result = implementation( - request as JsonRpcRequest, - response as PendingJsonRpcResponse, - next, - end, - hooks, - ); - - result?.catch(end); - }); - - await engine.handle({ - jsonrpc: '2.0', - id: 1, - method: 'snap_scheduleBackgroundEvent', - params: { - duration: 'PT0.5S', - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - }); - - expect(scheduleBackgroundEvent).toHaveBeenCalledWith({ - date: '2025-05-21T13:25:21.500Z', + schedule: 'PT1S', request: { method: 'handleExport', params: ['p1'], diff --git a/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts b/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts index 7699f6cadf..498d93dc42 100644 --- a/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts +++ b/packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts @@ -27,7 +27,6 @@ const hookNames: MethodHooksObject = { }; type ScheduleBackgroundEventHookParams = { - recurring: boolean; schedule: string; request: CronjobRpcRequest; }; @@ -122,7 +121,6 @@ async function getScheduleBackgroundEventImplementation( const schedule = getSchedule(validatedParams); const id = scheduleBackgroundEvent({ - recurring: false, schedule, request, }); From 747ee98df10a8c8b5ea4f4701b5d594d2e58bdf8 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Mon, 26 May 2025 15:50:08 +0200 Subject: [PATCH 07/16] Remove `skipEvents` behaviour --- .../snaps-controllers/src/cronjob/CronjobController.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index b0fab95cf3..31c1e44557 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -278,10 +278,8 @@ export class CronjobController extends BaseController< * Unregister all cronjobs and background events for a given Snap. * * @param snapId - ID of a snap. - * @param _skipEvents - Whether the unregistration process should skip - * scheduled background events. */ - unregister(snapId: SnapId, _skipEvents = false) { + unregister(snapId: SnapId) { for (const [id, event] of Object.entries(this.state.events)) { if (event.snapId === snapId) { this.#cancel(id); @@ -520,8 +518,7 @@ export class CronjobController extends BaseController< * @param snap - Basic Snap information. */ readonly #handleSnapDisabledEvent = (snap: TruncatedSnap) => { - // TODO: Check why `skipEvents` is set to `true` here. - this.unregister(snap.id, true); + this.unregister(snap.id); }; /** From 2002806c8394c7aa9f492b4d95231e07eea2c75b Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 13:10:48 +0200 Subject: [PATCH 08/16] Update tests for `CronjobController` --- packages/snaps-controllers/coverage.json | 8 +- .../src/cronjob/CronjobController.test.ts | 993 ++++++++---------- .../src/cronjob/CronjobController.ts | 18 +- .../src/cronjob/utils.test.ts | 18 +- .../snaps-controllers/src/cronjob/utils.ts | 31 +- .../src/test-utils/cronjob.ts | 19 +- 6 files changed, 489 insertions(+), 598 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index ccd5052972..c8030f6cdc 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { - "branches": 95.26, - "functions": 98.42, - "lines": 98.79, - "statements": 98.62 + "branches": 95.17, + "functions": 98.65, + "lines": 98.83, + "statements": 98.66 } diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts index e094bab90b..e51371411d 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts @@ -6,13 +6,14 @@ import type { SemVerVersion } from '@metamask/utils'; import { Duration, inMilliseconds } from '@metamask/utils'; import { CronjobController } from './CronjobController'; +import { METAMASK_ORIGIN } from '../snaps/constants'; import { getRestrictedCronjobControllerMessenger, getRootCronjobControllerMessenger, } from '../test-utils'; import { getCronjobPermission } from '../test-utils/cronjob'; -const MOCK_VERSION = '1.0' as SemVerVersion; +const MOCK_VERSION = '1.0.0' as SemVerVersion; describe('CronjobController', () => { const originalProcessNextTick = process.nextTick; @@ -25,11 +26,22 @@ describe('CronjobController', () => { jest.useRealTimers(); }); - it('registers a cronjob', () => { + it('registers a cronjob with an expression', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => { + return { + [SnapEndowments.Cronjob]: getCronjobPermission({ + expression: '* * * * *', + }), + }; + }, + ); + const cronjobController = new CronjobController({ messenger: controllerMessenger, }); @@ -44,14 +56,60 @@ describe('CronjobController', () => { jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); expect(rootMessenger.call).toHaveBeenNthCalledWith( - 4, + 2, 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, origin: 'metamask', handler: HandlerType.OnCronjob, request: { - method: 'exampleMethodOne', + method: 'exampleMethod', + params: ['p1'], + }, + }, + ); + + cronjobController.destroy(); + }); + + it('registers a cronjob with a duration', () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => { + return { + [SnapEndowments.Cronjob]: getCronjobPermission({ + duration: 'PT1M', + }), + }; + }, + ); + + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + }); + + cronjobController.register(MOCK_SNAP_ID); + + expect(rootMessenger.call).toHaveBeenCalledWith( + 'PermissionController:getPermissions', + MOCK_SNAP_ID, + ); + + jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); + + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 2, + 'SnapController:handleRequest', + { + snapId: MOCK_SNAP_ID, + origin: METAMASK_ORIGIN, + handler: HandlerType.OnCronjob, + request: { + method: 'exampleMethod', params: ['p1'], }, }, @@ -74,12 +132,11 @@ describe('CronjobController', () => { jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); - expect(rootMessenger.call).not.toHaveBeenNthCalledWith( - 4, + expect(rootMessenger.call).not.toHaveBeenCalledWith( 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, - origin: 'metamask', + origin: METAMASK_ORIGIN, handler: HandlerType.OnCronjob, request: { method: 'exampleMethodOne', @@ -91,7 +148,7 @@ describe('CronjobController', () => { cronjobController.destroy(); }); - it('executes cronjobs that were missed during daily check in', () => { + it('immediately executes cronjobs that are past the scheduled execution date', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); @@ -107,26 +164,33 @@ describe('CronjobController', () => { messenger: controllerMessenger, }); - // Update state manually for test - // @ts-expect-error Accessing private property + // @ts-expect-error: `update` is protected. cronjobController.update(() => { return { - jobs: { - [`${MOCK_SNAP_ID}-0`]: { lastRun: 0 }, + events: { + [`cronjob-${MOCK_SNAP_ID}-0`]: { + id: `cronjob-${MOCK_SNAP_ID}-0`, + snapId: MOCK_SNAP_ID, + date: new Date('2022-01-01T00:00Z').toISOString(), + scheduledAt: new Date('2022-01-01T00:00Z').toISOString(), + schedule: 'PT25H', + recurring: true, + request: { + method: 'exampleMethod', + params: ['p1'], + }, + }, }, - events: {}, }; }); - cronjobController.dailyCheckIn(); - jest.advanceTimersByTime(inMilliseconds(24, Duration.Hour)); expect(rootMessenger.call).toHaveBeenCalledWith( 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, - origin: 'metamask', + origin: METAMASK_ORIGIN, handler: HandlerType.OnCronjob, request: { method: 'exampleMethod', @@ -138,24 +202,12 @@ describe('CronjobController', () => { cronjobController.destroy(); }); - it('executes cronjobs that were missed during daily check in but doesnt repeat every init', async () => { + it('immediately executes cronjobs that are past the scheduled execution date and reschedules the cronjob', async () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); - rootMessenger.registerActionHandler( - 'PermissionController:getPermissions', - () => { - return { - [SnapEndowments.Cronjob]: getCronjobPermission({ - expression: '30 * * * *', - }), - }; - }, - ); - - const handleRequest = jest.fn(); - + const handleRequest = jest.fn().mockResolvedValue(undefined); rootMessenger.registerActionHandler( 'SnapController:handleRequest', handleRequest, @@ -164,20 +216,29 @@ describe('CronjobController', () => { const cronjobController = new CronjobController({ messenger: controllerMessenger, state: { - jobs: { - [`${MOCK_SNAP_ID}-0`]: { lastRun: 0 }, + events: { + [`cronjob-${MOCK_SNAP_ID}-0`]: { + id: `cronjob-${MOCK_SNAP_ID}-0`, + snapId: MOCK_SNAP_ID, + date: new Date('2022-01-01T00:00Z').toISOString(), + scheduledAt: new Date('2022-01-01T00:00Z').toISOString(), + schedule: 'PT25H', + recurring: true, + request: { + method: 'exampleMethod', + params: ['p1'], + }, + }, }, - events: {}, }, }); await new Promise((resolve) => originalProcessNextTick(resolve)); - expect(rootMessenger.call).toHaveBeenCalledWith( 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, - origin: 'metamask', + origin: METAMASK_ORIGIN, handler: HandlerType.OnCronjob, request: { method: 'exampleMethod', @@ -186,49 +247,16 @@ describe('CronjobController', () => { }, ); - const cronjobController2 = new CronjobController({ + const secondCronjobController = new CronjobController({ messenger: controllerMessenger, state: cronjobController.state, }); await new Promise((resolve) => originalProcessNextTick(resolve)); - expect(handleRequest).toHaveBeenCalledTimes(1); cronjobController.destroy(); - cronjobController2.destroy(); - }); - - it('catches errors during daily check in', () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); - - rootMessenger.registerActionHandler( - 'PermissionController:getPermissions', - () => { - return { [SnapEndowments.Cronjob]: getCronjobPermission() }; - }, - ); - - const handleRequest = jest.fn().mockRejectedValue('Snap failed to boot.'); - - rootMessenger.registerActionHandler( - 'SnapController:handleRequest', - handleRequest, - ); - - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - }); - - cronjobController.dailyCheckIn(); - - jest.advanceTimersByTime(inMilliseconds(24, Duration.Hour)); - - expect(handleRequest).toHaveBeenCalledTimes(2); - - cronjobController.destroy(); + secondCronjobController.destroy(); }); it('does not schedule cronjob that is too far in the future', () => { @@ -252,19 +280,19 @@ describe('CronjobController', () => { }); cronjobController.register(MOCK_SNAP_ID); + jest.runOnlyPendingTimers(); + expect(rootMessenger.call).toHaveBeenCalledTimes(1); expect(rootMessenger.call).toHaveBeenCalledWith( 'PermissionController:getPermissions', MOCK_SNAP_ID, ); - jest.runOnlyPendingTimers(); - expect(rootMessenger.call).not.toHaveBeenCalledWith( 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, - origin: 'metamask', + origin: METAMASK_ORIGIN, handler: HandlerType.OnCronjob, request: { method: 'exampleMethod', @@ -276,110 +304,68 @@ describe('CronjobController', () => { cronjobController.destroy(); }); - it('schedules a background event', () => { + it('does not schedule events for a Snap without a permission caveat', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - }); - - const backgroundEvent = { - snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', - request: { - method: 'handleEvent', - params: ['p1'], - }, - }; - - const id = cronjobController.scheduleBackgroundEvent(backgroundEvent); - - expect(cronjobController.state.events).toStrictEqual({ - [id]: { id, scheduledAt: expect.any(String), ...backgroundEvent }, - }); - - jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); - - expect(rootMessenger.call).toHaveBeenCalledWith( - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'handleEvent', - params: ['p1'], - }, + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => { + return { + [SnapEndowments.Cronjob]: { + date: 1664187844588, + id: 'izn0WGUO8cvq_jqvLQuQP', + invoker: MOCK_ORIGIN, + parentCapability: SnapEndowments.Cronjob, + caveats: null, + }, + }; }, ); - expect(cronjobController.state.events).toStrictEqual({}); - - cronjobController.destroy(); - }); - - it('fails to schedule a background event if the date is in the past', () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); - const cronjobController = new CronjobController({ messenger: controllerMessenger, }); - const backgroundEvent = { - snapId: MOCK_SNAP_ID, - date: '2021-01-01T01:00Z', - request: { - method: 'handleEvent', - params: ['p1'], - }, - }; - - expect(() => - cronjobController.scheduleBackgroundEvent(backgroundEvent), - ).toThrow('Cannot schedule an event in the past.'); - + cronjobController.register(MOCK_SNAP_ID); expect(cronjobController.state.events).toStrictEqual({}); cronjobController.destroy(); }); - it('cancels a background event', () => { + it('reschedules any un-expired events that are in state upon initialization', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); const cronjobController = new CronjobController({ messenger: controllerMessenger, - }); - - const backgroundEvent = { - snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', - request: { - method: 'handleEvent', - params: ['p1'], + state: { + events: { + foo: { + id: 'foo', + recurring: false, + date: '2022-01-01T01:00Z', + schedule: '2022-01-01T01:00Z', + scheduledAt: new Date().toISOString(), + snapId: MOCK_SNAP_ID, + request: { + method: 'handleEvent', + params: ['p1'], + }, + }, + }, }, - }; - - const id = cronjobController.scheduleBackgroundEvent(backgroundEvent); - - expect(cronjobController.state.events).toStrictEqual({ - [id]: { id, scheduledAt: expect.any(String), ...backgroundEvent }, }); - cronjobController.cancelBackgroundEvent(MOCK_SNAP_ID, id); - jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); - expect(rootMessenger.call).not.toHaveBeenCalledWith( + expect(rootMessenger.call).toHaveBeenCalledWith( 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, - origin: 'metamask', + origin: METAMASK_ORIGIN, handler: HandlerType.OnCronjob, request: { method: 'handleEvent', @@ -393,7 +379,7 @@ describe('CronjobController', () => { cronjobController.destroy(); }); - it('fails to cancel a background event if the caller is not the scheduler', () => { + it('handles the `snapInstalled` event', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); @@ -402,166 +388,67 @@ describe('CronjobController', () => { messenger: controllerMessenger, }); - const backgroundEvent = { - snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', - request: { - method: 'handleEvent', - params: ['p1'], - }, + const snapInfo: TruncatedSnap = { + blocked: false, + enabled: true, + id: MOCK_SNAP_ID, + initialPermissions: {}, + version: MOCK_VERSION, }; - const id = cronjobController.scheduleBackgroundEvent(backgroundEvent); - - expect(cronjobController.state.events).toStrictEqual({ - [id]: { id, scheduledAt: expect.any(String), ...backgroundEvent }, - }); - - expect(() => cronjobController.cancelBackgroundEvent('foo', id)).toThrow( - 'Only the origin that scheduled this event can cancel it.', + rootMessenger.publish( + 'SnapController:snapInstalled', + snapInfo, + MOCK_ORIGIN, + false, ); - cronjobController.destroy(); - }); - - it("returns a list of a Snap's background events", () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); - - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - }); - - const backgroundEvent = { - snapId: MOCK_SNAP_ID, - date: '2025-05-21T13:25:21.500Z', - request: { - method: 'handleEvent', - params: ['p1'], - }, - }; - - const id = cronjobController.scheduleBackgroundEvent(backgroundEvent); + jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); - const events = cronjobController.getBackgroundEvents(MOCK_SNAP_ID); - expect(events).toStrictEqual([ + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 2, + 'SnapController:handleRequest', { - id, snapId: MOCK_SNAP_ID, - date: '2025-05-21T13:25:21Z', + origin: METAMASK_ORIGIN, + handler: HandlerType.OnCronjob, request: { - method: 'handleEvent', + method: 'exampleMethodOne', params: ['p1'], }, - scheduledAt: expect.any(String), }, - ]); + ); cronjobController.destroy(); }); - it('reschedules any un-expired events that are in state upon initialization', () => { + it('handles the `snapEnabled` event', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => { + return { + [SnapEndowments.Cronjob]: getCronjobPermission({ + expression: '0 0 * * *', + }), + }; + }, + ); + const cronjobController = new CronjobController({ messenger: controllerMessenger, state: { - jobs: {}, events: { foo: { id: 'foo', + recurring: false, + date: '2022-01-01T01:00Z', + schedule: '2022-01-01T01:00Z', scheduledAt: new Date().toISOString(), snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', - request: { - method: 'handleEvent', - params: ['p1'], - }, - }, - }, - }, - }); - - jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); - - expect(rootMessenger.call).toHaveBeenCalledWith( - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'handleEvent', - params: ['p1'], - }, - }, - ); - - expect(cronjobController.state.events).toStrictEqual({}); - - cronjobController.destroy(); - }); - - it('handles SnapInstalled event', () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); - - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - }); - - const snapInfo: TruncatedSnap = { - blocked: false, - enabled: true, - id: MOCK_SNAP_ID, - initialPermissions: {}, - version: MOCK_VERSION, - }; - - rootMessenger.publish( - 'SnapController:snapInstalled', - snapInfo, - MOCK_ORIGIN, - ); - - jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 4, - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'exampleMethodOne', - params: ['p1'], - }, - }, - ); - - cronjobController.destroy(); - }); - - it('handles SnapEnabled event', () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); - - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - state: { - jobs: {}, - events: { - foo: { - id: 'foo', - scheduledAt: new Date().toISOString(), - snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', request: { method: 'handleEvent', params: ['p1'], @@ -569,9 +456,11 @@ describe('CronjobController', () => { }, bar: { id: 'bar', + recurring: false, + date: '2021-01-01T01:00Z', + schedule: '2021-01-01T01:00Z', scheduledAt: new Date().toISOString(), snapId: MOCK_SNAP_ID, - date: '2021-01-01T01:00Z', request: { method: 'handleEvent', params: ['p1'], @@ -592,11 +481,25 @@ describe('CronjobController', () => { rootMessenger.publish('SnapController:snapEnabled', snapInfo); expect(cronjobController.state.events).toStrictEqual({ + [`cronjob-${MOCK_SNAP_ID}-0`]: { + id: `cronjob-${MOCK_SNAP_ID}-0`, + recurring: true, + date: '2022-01-02T00:00:00.000Z', + schedule: '0 0 * * *', + scheduledAt: expect.any(String), + snapId: MOCK_SNAP_ID, + request: { + method: 'exampleMethod', + params: ['p1'], + }, + }, foo: { id: 'foo', + recurring: false, + date: '2022-01-01T01:00:00Z', + schedule: '2022-01-01T01:00Z', scheduledAt: new Date().toISOString(), snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', request: { method: 'handleEvent', params: ['p1'], @@ -606,28 +509,30 @@ describe('CronjobController', () => { jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + expect(rootMessenger.call).toHaveBeenCalledTimes(3); expect(rootMessenger.call).toHaveBeenNthCalledWith( - 4, + 2, 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, origin: 'metamask', handler: HandlerType.OnCronjob, request: { - method: 'exampleMethodOne', + method: 'handleEvent', params: ['p1'], }, }, ); - expect(rootMessenger.call).toHaveBeenCalledWith( + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 3, 'SnapController:handleRequest', { snapId: MOCK_SNAP_ID, - origin: 'metamask', + origin: METAMASK_ORIGIN, handler: HandlerType.OnCronjob, request: { - method: 'handleEvent', + method: 'exampleMethod', params: ['p1'], }, }, @@ -636,7 +541,7 @@ describe('CronjobController', () => { cronjobController.destroy(); }); - it('handles SnapUninstalled event', () => { + it('handles the `snapUninstalled` event', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); @@ -653,59 +558,22 @@ describe('CronjobController', () => { version: MOCK_VERSION, }; - cronjobController.scheduleBackgroundEvent({ + cronjobController.schedule({ snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', + schedule: '2022-01-01T01:00Z', request: { method: 'handleEvent', params: ['p1'], }, }); - rootMessenger.publish( - 'SnapController:snapInstalled', - snapInfo, - MOCK_ORIGIN, - ); - rootMessenger.publish('SnapController:snapUninstalled', snapInfo); - - jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); - - expect(rootMessenger.call).not.toHaveBeenCalledWith( - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'exampleMethodOne', - params: ['p1'], - }, - }, - ); - - jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); - - expect(rootMessenger.call).not.toHaveBeenCalledWith( - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'handleEvent', - params: ['p1'], - }, - }, - ); - expect(cronjobController.state.events).toStrictEqual({}); cronjobController.destroy(); }); - it('handles SnapDisabled event', () => { + it('handles the `snapDisabled` event', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); @@ -722,70 +590,22 @@ describe('CronjobController', () => { version: MOCK_VERSION, }; - const id = cronjobController.scheduleBackgroundEvent({ + cronjobController.schedule({ snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', + schedule: '2022-01-01T01:00Z', request: { method: 'handleEvent', params: ['p1'], }, }); - rootMessenger.publish( - 'SnapController:snapInstalled', - snapInfo, - MOCK_ORIGIN, - ); - rootMessenger.publish('SnapController:snapDisabled', snapInfo); - - jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); - - expect(rootMessenger.call).not.toHaveBeenCalledWith( - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'exampleMethodOne', - params: ['p1'], - }, - }, - ); - - jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); - - expect(rootMessenger.call).not.toHaveBeenCalledWith( - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'handleEvent', - params: ['p1'], - }, - }, - ); - - expect(cronjobController.state.events).toStrictEqual({ - [id]: { - id, - scheduledAt: expect.any(String), - snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', - request: { - method: 'handleEvent', - params: ['p1'], - }, - }, - }); + expect(cronjobController.state.events).toStrictEqual({}); cronjobController.destroy(); }); - it('handles SnapUpdated event', () => { + it('handles the `snapUpdated` event', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); @@ -793,13 +613,26 @@ describe('CronjobController', () => { const cronjobController = new CronjobController({ messenger: controllerMessenger, state: { - jobs: {}, events: { + [`cronjob-${MOCK_SNAP_ID}-0`]: { + id: `cronjob-${MOCK_SNAP_ID}-0`, + recurring: true, + date: new Date('2022-01-01T00:00Z').toISOString(), + schedule: 'PT25H', + scheduledAt: new Date('2022-01-01T00:00Z').toISOString(), + snapId: MOCK_SNAP_ID, + request: { + method: 'exampleMethod', + params: ['p1'], + }, + }, foo: { id: 'foo', + recurring: false, + date: '2022-01-01T01:00Z', + schedule: '2022-01-01T01:00Z', scheduledAt: new Date().toISOString(), snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', request: { method: 'handleEvent', params: ['p1'], @@ -817,55 +650,33 @@ describe('CronjobController', () => { version: MOCK_VERSION, }; - rootMessenger.publish( - 'SnapController:snapInstalled', - snapInfo, - MOCK_ORIGIN, - ); - rootMessenger.publish( 'SnapController:snapUpdated', snapInfo, snapInfo.version, MOCK_ORIGIN, + false, ); - expect(cronjobController.state.events).toStrictEqual({}); - - jest.advanceTimersByTime(inMilliseconds(15, Duration.Minute)); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 5, - 'SnapController:handleRequest', - { + expect(cronjobController.state.events).toStrictEqual({ + [`cronjob-${MOCK_SNAP_ID}-0`]: { + id: `cronjob-${MOCK_SNAP_ID}-0`, + recurring: true, + date: '2022-01-01T00:01:00.000Z', + schedule: '* * * * *', + scheduledAt: expect.any(String), snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, request: { method: 'exampleMethodOne', params: ['p1'], }, }, - ); - - expect(rootMessenger.call).not.toHaveBeenCalledWith( - 5, - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'handleEvent', - params: ['p1'], - }, - }, - ); + }); cronjobController.destroy(); }); - it('removes all jobs and schedules after controller destroy is called', () => { + it('removes all events when the controller is destroyed', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); @@ -876,194 +687,252 @@ describe('CronjobController', () => { cronjobController.register(MOCK_SNAP_ID); + cronjobController.destroy(); + + jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + + expect(rootMessenger.call).toHaveBeenCalledTimes(1); expect(rootMessenger.call).toHaveBeenCalledWith( 'PermissionController:getPermissions', MOCK_SNAP_ID, ); + }); - cronjobController.destroy(); + it('logs errors caught during cronjob execution', async () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); - jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => { + return { + [SnapEndowments.Cronjob]: getCronjobPermission({ + expression: '* * * * *', + }), + }; + }, + ); - expect(rootMessenger.call).not.toHaveBeenCalledWith( + jest.spyOn(console, 'error').mockImplementation(); + + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + }); + + const error = new Error('Test error.'); + rootMessenger.registerActionHandler( 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'exampleMethodOne', - params: ['p1'], - }, + async () => { + throw error; }, ); + + cronjobController.register(MOCK_SNAP_ID); + + jest.advanceTimersByTime(inMilliseconds(1, Duration.Minute)); + await new Promise((resolve) => originalProcessNextTick(resolve)); + + expect(console.error).toHaveBeenCalledWith( + `An error occurred while executing an event for Snap "${MOCK_SNAP_ID}":`, + error, + ); + + cronjobController.destroy(); }); - describe('CronjobController actions', () => { - describe('CronjobController:scheduleBackgroundEvent', () => { - it('schedules a background event', () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); + describe('CronjobController:schedule', () => { + it('schedules a background event', () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - }); + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + }); - cronjobController.register(MOCK_SNAP_ID); + const event = { + snapId: MOCK_SNAP_ID, + schedule: '2022-01-01T01:00Z', + request: { + method: 'handleEvent', + params: ['p1'], + }, + }; - const id = rootMessenger.call( - 'CronjobController:scheduleBackgroundEvent', - { - snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - ); + const id = rootMessenger.call('CronjobController:schedule', event); + expect(cronjobController.state.events).toStrictEqual({ + [id]: { + id, + recurring: false, + date: '2022-01-01T01:00:00Z', + scheduledAt: expect.any(String), + ...event, + }, + }); - expect(cronjobController.state.events).toStrictEqual({ - [id]: { - id, - snapId: MOCK_SNAP_ID, - scheduledAt: expect.any(String), - date: '2022-01-01T01:00Z', - request: { - method: 'handleExport', - params: ['p1'], - }, + jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + + expect(rootMessenger.call).toHaveBeenCalledWith( + 'SnapController:handleRequest', + { + snapId: MOCK_SNAP_ID, + origin: METAMASK_ORIGIN, + handler: HandlerType.OnCronjob, + request: { + method: 'handleEvent', + params: ['p1'], }, - }); + }, + ); - jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + expect(cronjobController.state.events).toStrictEqual({}); - expect(rootMessenger.call).toHaveBeenCalledWith( - 'SnapController:handleRequest', - { - snapId: MOCK_SNAP_ID, - origin: 'metamask', - handler: HandlerType.OnCronjob, - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - ); + cronjobController.destroy(); + }); - expect(cronjobController.state.events).toStrictEqual({}); + it('throws when scheduling a background event if the date is in the past', () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); - cronjobController.destroy(); + const cronjobController = new CronjobController({ + messenger: controllerMessenger, }); - }); - describe('CronjobController:cancelBackgroundEvent', () => { - it('cancels a background event', () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); + const event = { + snapId: MOCK_SNAP_ID, + schedule: '2021-01-01T01:00Z', + request: { + method: 'handleEvent', + params: ['p1'], + }, + }; - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - }); + expect(() => + rootMessenger.call('CronjobController:schedule', event), + ).toThrow('Cannot schedule an event in the past.'); - cronjobController.register(MOCK_SNAP_ID); + expect(cronjobController.state.events).toStrictEqual({}); - const id = rootMessenger.call( - 'CronjobController:scheduleBackgroundEvent', - { - snapId: MOCK_SNAP_ID, - date: '2022-01-01T01:00Z', - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - ); + cronjobController.destroy(); + }); + }); - expect(cronjobController.state.events).toStrictEqual({ - [id]: { - id, - snapId: MOCK_SNAP_ID, - scheduledAt: expect.any(String), - date: '2022-01-01T01:00Z', - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - }); + describe('CronjobController:cancel', () => { + it('cancels a background event', () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); - rootMessenger.call( - 'CronjobController:cancelBackgroundEvent', - MOCK_SNAP_ID, - id, - ); + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + }); - expect(cronjobController.state.events).toStrictEqual({}); + const event = { + snapId: MOCK_SNAP_ID, + schedule: '2022-01-01T01:00Z', + request: { + method: 'handleEvent', + params: ['p1'], + }, + }; - cronjobController.destroy(); + const id = rootMessenger.call('CronjobController:schedule', event); + expect(cronjobController.state.events).toStrictEqual({ + [id]: { + id, + recurring: false, + date: '2022-01-01T01:00:00Z', + scheduledAt: expect.any(String), + ...event, + }, }); + + rootMessenger.call('CronjobController:cancel', MOCK_SNAP_ID, id); + expect(cronjobController.state.events).toStrictEqual({}); + + jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + expect(rootMessenger.call).toHaveBeenCalledTimes(2); + + cronjobController.destroy(); }); - describe('CronjobController:getBackgroundEvents', () => { - it("gets a list of a Snap's background events", () => { - const rootMessenger = getRootCronjobControllerMessenger(); - const controllerMessenger = - getRestrictedCronjobControllerMessenger(rootMessenger); + it('throws when cancelling an event scheduled by another origin', () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); - const cronjobController = new CronjobController({ - messenger: controllerMessenger, - }); + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + }); - cronjobController.register(MOCK_SNAP_ID); + const event = { + snapId: MOCK_SNAP_ID, + schedule: '2022-01-01T01:00Z', + request: { + method: 'handleEvent', + params: ['p1'], + }, + }; - const id = rootMessenger.call( - 'CronjobController:scheduleBackgroundEvent', - { - snapId: MOCK_SNAP_ID, - date: '2025-05-21T13:25:21.500Z', - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - ); + const id = rootMessenger.call('CronjobController:schedule', event); + expect(cronjobController.state.events).toStrictEqual({ + [id]: { + id, + recurring: false, + date: '2022-01-01T01:00:00Z', + scheduledAt: expect.any(String), + ...event, + }, + }); - expect(cronjobController.state.events).toStrictEqual({ - [id]: { - id, - snapId: MOCK_SNAP_ID, - scheduledAt: expect.any(String), - date: '2025-05-21T13:25:21.500Z', - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - }); + expect(() => + rootMessenger.call('CronjobController:cancel', 'foo', id), + ).toThrow('Only the origin that scheduled this event can cancel it.'); - const events = rootMessenger.call( - 'CronjobController:getBackgroundEvents', - MOCK_SNAP_ID, - ); + cronjobController.destroy(); + }); + }); - expect(events).toStrictEqual([ - { - id, - snapId: MOCK_SNAP_ID, - scheduledAt: expect.any(String), - date: '2025-05-21T13:25:21Z', - request: { - method: 'handleExport', - params: ['p1'], - }, - }, - ]); + describe('CronjobController:get', () => { + it("returns a list of a Snap's background events", () => { + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); - cronjobController.destroy(); + const cronjobController = new CronjobController({ + messenger: controllerMessenger, }); + + const event = { + snapId: MOCK_SNAP_ID, + schedule: '2025-05-21T13:25:21.500Z', + request: { + method: 'handleEvent', + params: ['p1'], + }, + }; + + const id = cronjobController.schedule(event); + + const events = rootMessenger.call('CronjobController:get', MOCK_SNAP_ID); + expect(events).toStrictEqual([ + { + id, + snapId: MOCK_SNAP_ID, + date: '2025-05-21T13:25:21Z', + recurring: false, + request: { + method: 'handleEvent', + params: ['p1'], + }, + schedule: '2025-05-21T13:25:21.500Z', + scheduledAt: expect.any(String), + }, + ]); + + cronjobController.destroy(); }); }); }); diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 31c1e44557..c610d087f8 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -206,9 +206,9 @@ export class CronjobController extends BaseController< (...args) => this.get(...args), ); + this.#start(); this.#clear(); this.#reschedule(); - this.#start(); } /** @@ -321,6 +321,10 @@ export class CronjobController extends BaseController< // Cancel all timers and clear the map. this.#timers.forEach((timer) => timer.cancel()); this.#timers.clear(); + + if (this.#dailyTimer.status === 'running') { + this.#dailyTimer.cancel(); + } } /** @@ -381,9 +385,10 @@ export class CronjobController extends BaseController< const ms = DateTime.fromISO(event.date, { setZone: true }).toMillis() - Date.now(); - if (ms <= 0) { - throw new Error('Cannot schedule an event in the past.'); - } + // This should be validated by the `getExecutionDate` function, but we + // add an extra check here to ensure that we don't schedule events in the + // past. + assert(ms > 0, 'Cannot schedule an event in the past.'); // We don't schedule this job yet as it is too far in the future. if (ms > DAILY_TIMEOUT) { @@ -416,7 +421,10 @@ export class CronjobController extends BaseController< request: event.request, }) .catch((error) => { - logError(error); + logError( + `An error occurred while executing an event for Snap "${event.snapId}":`, + error, + ); }); this.#timers.delete(event.id); diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts index 2792619409..ca344bee63 100644 --- a/packages/snaps-controllers/src/cronjob/utils.test.ts +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -25,14 +25,14 @@ describe('getCronjobSpecificationSchedule', () => { describe('getExecutionDate', () => { it('parses an ISO 8601 date', () => { - expect(getExecutionDate('2025-05-23T09:55:47Z')).toBe( - '2025-05-23T09:55:47Z', + expect(getExecutionDate('2025-05-24T09:55:47Z')).toBe( + '2025-05-24T09:55:47Z', ); - expect(getExecutionDate('2025-05-23T09:55:47+00:00')).toBe( - '2025-05-23T09:55:47Z', + expect(getExecutionDate('2025-05-24T09:55:47+00:00')).toBe( + '2025-05-24T09:55:47Z', ); - expect(getExecutionDate('2025-05-23T09:55:47+01:00')).toBe( - '2025-05-23T08:55:47Z', + expect(getExecutionDate('2025-05-24T09:55:47+01:00')).toBe( + '2025-05-24T08:55:47Z', ); }); @@ -63,4 +63,10 @@ describe('getExecutionDate', () => { 'Unable to parse "100 * * * * *" as ISO 8601 date, ISO 8601 duration, or cron expression.', ); }); + + it('throws an error for dates in the past', () => { + expect(() => getExecutionDate('2020-01-01T00:00:00Z')).toThrow( + 'Cannot schedule an event in the past.', + ); + }); }); diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts index b39c88bfeb..c6c654de97 100644 --- a/packages/snaps-controllers/src/cronjob/utils.ts +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -50,22 +50,27 @@ function getDuration(duration: Duration): Duration { * @returns The parsed ISO 8601 date at which the event should be executed. */ export function getExecutionDate(schedule: string) { - try { - const date = DateTime.fromISO(schedule); - if (date.isValid) { - // We round to the nearest second to avoid milliseconds in the output. - return date.toUTC().startOf('second').toISO({ - suppressMilliseconds: true, - }); + const date = DateTime.fromISO(schedule); + if (date.isValid) { + const now = Date.now(); + if (date.toMillis() < now) { + throw new Error('Cannot schedule an event in the past.'); } - const duration = Duration.fromISO(schedule); - if (duration.isValid) { - // This ensures the duration is at least 1 second. - const validatedDuration = getDuration(duration); - return DateTime.now().toUTC().plus(validatedDuration).toISO(); - } + // We round to the nearest second to avoid milliseconds in the output. + return date.toUTC().startOf('second').toISO({ + suppressMilliseconds: true, + }); + } + const duration = Duration.fromISO(schedule); + if (duration.isValid) { + // This ensures the duration is at least 1 second. + const validatedDuration = getDuration(duration); + return DateTime.now().toUTC().plus(validatedDuration).toISO(); + } + + try { const parsed = parseExpression(schedule, { utc: true }); const next = parsed.next(); const nextDate = DateTime.fromJSDate(next.toDate()); diff --git a/packages/snaps-controllers/src/test-utils/cronjob.ts b/packages/snaps-controllers/src/test-utils/cronjob.ts index 15c61c8dc9..5f1d7406c8 100644 --- a/packages/snaps-controllers/src/test-utils/cronjob.ts +++ b/packages/snaps-controllers/src/test-utils/cronjob.ts @@ -26,20 +26,23 @@ export const MOCK_CRONJOB_PERMISSION: PermissionConstraint = { parentCapability: SnapEndowments.Cronjob, }; -type GetCronjobPermissionArgs = { - expression?: string; -}; +type GetCronjobPermissionArgs = + | { + expression?: string; + } + | { + duration?: string; + }; /** * Get a cronjob permission object as {@link PermissionConstraint}. * * @param args - The arguments to use when creating the permission. - * @param args.expression - The cron expression to use for the permission. * @returns The permission object. */ -export function getCronjobPermission({ - expression = '59 6 * * *', -}: GetCronjobPermissionArgs = {}): PermissionConstraint { +export function getCronjobPermission( + args: GetCronjobPermissionArgs = {}, +): PermissionConstraint { return { caveats: [ { @@ -47,7 +50,7 @@ export function getCronjobPermission({ value: { jobs: [ { - expression, + ...args, request: { method: 'exampleMethod', params: ['p1'], From 4150914065ed008051291542bf1c7d6debb9c1bb Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 13:22:42 +0200 Subject: [PATCH 09/16] Revert changes to cronjob example --- packages/snaps-rpc-methods/package.json | 4 +--- yarn.lock | 2 -- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/snaps-rpc-methods/package.json b/packages/snaps-rpc-methods/package.json index 61dc6002e2..f9d02960e9 100644 --- a/packages/snaps-rpc-methods/package.json +++ b/packages/snaps-rpc-methods/package.json @@ -62,8 +62,7 @@ "@metamask/snaps-utils": "workspace:^", "@metamask/superstruct": "^3.2.1", "@metamask/utils": "^11.4.0", - "@noble/hashes": "^1.7.1", - "luxon": "^3.5.0" + "@noble/hashes": "^1.7.1" }, "devDependencies": { "@lavamoat/allow-scripts": "^3.3.3", @@ -72,7 +71,6 @@ "@swc/core": "1.11.31", "@swc/jest": "^0.2.38", "@ts-bridge/cli": "^0.6.1", - "@types/luxon": "^3", "@types/node": "18.14.2", "deepmerge": "^4.2.2", "depcheck": "^1.4.7", diff --git a/yarn.lock b/yarn.lock index fd866d1c0c..59e813dfde 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4430,7 +4430,6 @@ __metadata: "@swc/core": "npm:1.11.31" "@swc/jest": "npm:^0.2.38" "@ts-bridge/cli": "npm:^0.6.1" - "@types/luxon": "npm:^3" "@types/node": "npm:18.14.2" deepmerge: "npm:^4.2.2" depcheck: "npm:^1.4.7" @@ -4438,7 +4437,6 @@ __metadata: jest: "npm:^29.0.2" jest-it-up: "npm:^2.0.0" jest-silent-reporter: "npm:^0.6.0" - luxon: "npm:^3.5.0" prettier: "npm:^3.3.3" typescript: "npm:~5.3.3" languageName: unknown From 9c88d033ea694d088b0a7db7f03fa8236edadabb Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 13:49:44 +0200 Subject: [PATCH 10/16] Remove default argument for `GetCronjobPermissionArgs` function --- packages/snaps-controllers/src/test-utils/cronjob.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/test-utils/cronjob.ts b/packages/snaps-controllers/src/test-utils/cronjob.ts index 5f1d7406c8..d0cc626559 100644 --- a/packages/snaps-controllers/src/test-utils/cronjob.ts +++ b/packages/snaps-controllers/src/test-utils/cronjob.ts @@ -41,7 +41,7 @@ type GetCronjobPermissionArgs = * @returns The permission object. */ export function getCronjobPermission( - args: GetCronjobPermissionArgs = {}, + args: GetCronjobPermissionArgs, ): PermissionConstraint { return { caveats: [ From fd1bd6a596b22a3c915518f6f406b0047b1e6acd Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 13:55:41 +0200 Subject: [PATCH 11/16] Update mock date for `getExecutionDate` tests --- packages/snaps-controllers/src/cronjob/utils.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts index ca344bee63..fd9176fdb0 100644 --- a/packages/snaps-controllers/src/cronjob/utils.test.ts +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -1,7 +1,7 @@ import { getCronjobSpecificationSchedule, getExecutionDate } from './utils'; jest.useFakeTimers(); -jest.setSystemTime(1747994147000); +jest.setSystemTime(1747994147500); describe('getCronjobSpecificationSchedule', () => { it('returns the duration if specified', () => { @@ -37,9 +37,9 @@ describe('getExecutionDate', () => { }); it('parses an ISO 8601 duration', () => { - expect(getExecutionDate('P1Y')).toBe('2026-05-23T09:55:47.000Z'); - expect(getExecutionDate('PT1S')).toBe('2025-05-23T09:55:48.000Z'); - expect(getExecutionDate('PT0S')).toBe('2025-05-23T09:55:48.000Z'); + expect(getExecutionDate('P1Y')).toBe('2026-05-23T09:55:47.500Z'); + expect(getExecutionDate('PT1S')).toBe('2025-05-23T09:55:48.500Z'); + expect(getExecutionDate('PT0S')).toBe('2025-05-23T09:55:48.500Z'); }); it('parses a cron expression', () => { From 3c6b9d3a143e62a588fe60bea6e34ca6ebc45814 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 13:57:39 +0200 Subject: [PATCH 12/16] Remove unused action type --- .../snaps-controllers/src/cronjob/CronjobController.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index c610d087f8..0c54bc65b5 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -23,7 +23,6 @@ import { nanoid } from 'nanoid'; import { getCronjobSpecificationSchedule, getExecutionDate } from './utils'; import type { - GetAllSnaps, HandleSnapRequest, SnapDisabled, SnapEnabled, @@ -59,21 +58,20 @@ export type Get = { }; export type CronjobControllerActions = - | GetAllSnaps + | CronjobControllerGetStateAction | HandleSnapRequest | GetPermissions - | CronjobControllerGetStateAction | Schedule | Cancel | Get; export type CronjobControllerEvents = + | CronjobControllerStateChangeEvent | SnapInstalled | SnapUninstalled | SnapUpdated | SnapEnabled - | SnapDisabled - | CronjobControllerStateChangeEvent; + | SnapDisabled; export type CronjobControllerMessenger = RestrictedMessenger< typeof controllerName, From 05a38bcc3d2c7535407f22cedbef4419945a2eca Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 14:48:43 +0200 Subject: [PATCH 13/16] Improve validation --- .../snaps-controllers/src/cronjob/CronjobController.ts | 5 ----- packages/snaps-controllers/src/cronjob/utils.test.ts | 4 ++++ packages/snaps-controllers/src/cronjob/utils.ts | 10 ++++++---- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 0c54bc65b5..6330686fde 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -383,11 +383,6 @@ export class CronjobController extends BaseController< const ms = DateTime.fromISO(event.date, { setZone: true }).toMillis() - Date.now(); - // This should be validated by the `getExecutionDate` function, but we - // add an extra check here to ensure that we don't schedule events in the - // past. - assert(ms > 0, 'Cannot schedule an event in the past.'); - // We don't schedule this job yet as it is too far in the future. if (ms > DAILY_TIMEOUT) { return; diff --git a/packages/snaps-controllers/src/cronjob/utils.test.ts b/packages/snaps-controllers/src/cronjob/utils.test.ts index fd9176fdb0..ad7e149d96 100644 --- a/packages/snaps-controllers/src/cronjob/utils.test.ts +++ b/packages/snaps-controllers/src/cronjob/utils.test.ts @@ -68,5 +68,9 @@ describe('getExecutionDate', () => { expect(() => getExecutionDate('2020-01-01T00:00:00Z')).toThrow( 'Cannot schedule an event in the past.', ); + + expect(() => + getExecutionDate(new Date(Date.now() + 100).toISOString()), + ).toThrow('Cannot schedule an event in the past.'); }); }); diff --git a/packages/snaps-controllers/src/cronjob/utils.ts b/packages/snaps-controllers/src/cronjob/utils.ts index c6c654de97..1d61fa7f9c 100644 --- a/packages/snaps-controllers/src/cronjob/utils.ts +++ b/packages/snaps-controllers/src/cronjob/utils.ts @@ -50,15 +50,17 @@ function getDuration(duration: Duration): Duration { * @returns The parsed ISO 8601 date at which the event should be executed. */ export function getExecutionDate(schedule: string) { - const date = DateTime.fromISO(schedule); + const date = DateTime.fromISO(schedule, { setZone: true }); if (date.isValid) { const now = Date.now(); - if (date.toMillis() < now) { + + // We round to the nearest second to avoid milliseconds in the output. + const roundedDate = date.toUTC().startOf('second'); + if (roundedDate.toMillis() < now) { throw new Error('Cannot schedule an event in the past.'); } - // We round to the nearest second to avoid milliseconds in the output. - return date.toUTC().startOf('second').toISO({ + return roundedDate.toISO({ suppressMilliseconds: true, }); } From f3a26aef7f9f47b9b349aa9b4aa95a76434974ca Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 15:22:09 +0200 Subject: [PATCH 14/16] Restart daily timer after execution --- .../src/cronjob/CronjobController.test.ts | 58 ++++++++++++++++--- .../src/cronjob/CronjobController.ts | 8 ++- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts index e51371411d..5fa23d879c 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts @@ -153,13 +153,6 @@ describe('CronjobController', () => { const controllerMessenger = getRestrictedCronjobControllerMessenger(rootMessenger); - rootMessenger.registerActionHandler( - 'PermissionController:getPermissions', - () => { - return { [SnapEndowments.Cronjob]: getCronjobPermission() }; - }, - ); - const cronjobController = new CronjobController({ messenger: controllerMessenger, }); @@ -304,6 +297,57 @@ describe('CronjobController', () => { cronjobController.destroy(); }); + it('schedules jobs that were not scheduled due to the daily timeout', () => { + const expression = '0 0 4 * *'; // At 12:00am on the 4th of every month. + + const rootMessenger = getRootCronjobControllerMessenger(); + const controllerMessenger = + getRestrictedCronjobControllerMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => { + return { + [SnapEndowments.Cronjob]: getCronjobPermission({ expression }), + }; + }, + ); + + const cronjobController = new CronjobController({ + messenger: controllerMessenger, + }); + + cronjobController.register(MOCK_SNAP_ID); + + jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + expect(rootMessenger.call).toHaveBeenCalledTimes(1); + expect(rootMessenger.call).toHaveBeenCalledWith( + 'PermissionController:getPermissions', + MOCK_SNAP_ID, + ); + + jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + expect(rootMessenger.call).toHaveBeenCalledTimes(1); + + jest.advanceTimersByTime(inMilliseconds(1, Duration.Day)); + expect(rootMessenger.call).toHaveBeenCalledTimes(2); + expect(rootMessenger.call).toHaveBeenNthCalledWith( + 2, + 'SnapController:handleRequest', + { + snapId: MOCK_SNAP_ID, + origin: METAMASK_ORIGIN, + handler: HandlerType.OnCronjob, + request: { + method: 'exampleMethod', + params: ['p1'], + }, + }, + ); + + cronjobController.destroy(); + }); + it('does not schedule events for a Snap without a permission caveat', () => { const rootMessenger = getRootCronjobControllerMessenger(); const controllerMessenger = diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 6330686fde..059107e1be 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -147,7 +147,7 @@ export class CronjobController extends BaseController< > { readonly #timers: Map; - readonly #dailyTimer: Timer = new Timer(DAILY_TIMEOUT); + #dailyTimer: Timer = new Timer(DAILY_TIMEOUT); constructor({ messenger, state }: CronjobControllerArgs) { super({ @@ -329,7 +329,11 @@ export class CronjobController extends BaseController< * Start the daily timer that will reschedule events every 24 hours. */ #start() { - this.#dailyTimer.start(() => this.#reschedule()); + this.#dailyTimer = new Timer(DAILY_TIMEOUT); + this.#dailyTimer.start(() => { + this.#reschedule(); + this.#start(); + }); } /** From c16a65606390eecc99eff4db570b90f41176f269 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 27 May 2025 21:34:15 +0200 Subject: [PATCH 15/16] Don't remove millisecond precision when rescheduling events --- .../src/cronjob/CronjobController.ts | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 059107e1be..6c4b1a71c1 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -545,10 +545,7 @@ export class CronjobController extends BaseController< * the controller state. */ #reschedule(events = Object.values(this.state.events)) { - const now = DateTime.fromJSDate(new Date()) - .toUTC() - .startOf('second') - .toSeconds(); + const now = Date.now(); for (const event of events) { if (this.#timers.has(event.id)) { @@ -557,10 +554,11 @@ export class CronjobController extends BaseController< continue; } - const eventDate = DateTime.fromISO(event.date) + const eventDate = DateTime.fromISO(event.date, { + setZone: true, + }) .toUTC() - .startOf('second') - .toSeconds(); + .toMillis(); // If the event is recurring and the date is in the past, execute it // immediately. @@ -576,16 +574,14 @@ export class CronjobController extends BaseController< * Clear non-recurring events that are past their scheduled time. */ #clear() { - const now = DateTime.fromJSDate(new Date()) - .toUTC() - .startOf('second') - .toSeconds(); + const now = Date.now(); for (const event of Object.values(this.state.events)) { - const eventDate = DateTime.fromISO(event.date) + const eventDate = DateTime.fromISO(event.date, { + setZone: true, + }) .toUTC() - .startOf('second') - .toSeconds(); + .toMillis(); if (!event.recurring && eventDate < now) { this.#cancel(event.id); From dfb3b7847ebdde81ca99ba7a487dd57dba24f07b Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Fri, 6 Jun 2025 16:21:54 +0200 Subject: [PATCH 16/16] Update coverage --- packages/snaps-controllers/coverage.json | 2 +- packages/snaps-rpc-methods/jest.config.js | 6 +++--- packages/snaps-utils/coverage.json | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index c8030f6cdc..66bbedb6b1 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,5 +1,5 @@ { - "branches": 95.17, + "branches": 95.29, "functions": 98.65, "lines": 98.83, "statements": 98.66 diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js index 8986c36f9d..82fa12ceb7 100644 --- a/packages/snaps-rpc-methods/jest.config.js +++ b/packages/snaps-rpc-methods/jest.config.js @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 95, - functions: 98.65, + branches: 94.96, + functions: 98.64, lines: 98.78, - statements: 98.47, + statements: 98.45, }, }, }); diff --git a/packages/snaps-utils/coverage.json b/packages/snaps-utils/coverage.json index 976413843d..128a3f13dd 100644 --- a/packages/snaps-utils/coverage.json +++ b/packages/snaps-utils/coverage.json @@ -1,6 +1,6 @@ { - "branches": 99.77, - "functions": 99.02, - "lines": 98.67, - "statements": 97.33 + "branches": 99.76, + "functions": 99, + "lines": 98.68, + "statements": 97.27 }