From f2d8bed852e7e712654118b430cddef6e26079f1 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 21 Jan 2025 10:27:12 -0500 Subject: [PATCH 1/8] refactor cronjob controller --- packages/snaps-controllers/coverage.json | 8 +- .../src/cronjob/CronjobController.test.ts | 2 +- .../src/cronjob/CronjobController.ts | 250 ++++++++++-------- 3 files changed, 149 insertions(+), 111 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 357e17f097..8bbda9cafe 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { - "branches": 93.06, - "functions": 96.54, - "lines": 98.02, - "statements": 97.74 + "branches": 93.11, + "functions": 97.14, + "lines": 98.13, + "statements": 97.84 } diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts index 4d65257d6a..4dfef5328d 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.test.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.test.ts @@ -308,7 +308,7 @@ describe('CronjobController', () => { expect(() => cronjobController.scheduleBackgroundEvent(backgroundEvent), - ).toThrow('Cannot schedule an event in the past.'); + ).toThrow('Cannot schedule execution in the past.'); expect(cronjobController.state.events).toStrictEqual({}); diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index f93995c9c2..91cc6888c4 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -78,6 +78,11 @@ export type CronjobControllerEvents = | SnapDisabled | CronjobControllerStateChangeEvent; +type BackgroundEventActions = + | `${typeof controllerName}:scheduleBackgroundEvent` + | `${typeof controllerName}:cancelBackgroundEvent` + | `${typeof controllerName}:getBackgroundEvents`; + export type CronjobControllerMessenger = RestrictedControllerMessenger< typeof controllerName, CronjobControllerActions, @@ -155,55 +160,128 @@ export class CronjobController extends BaseController< 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.#initializeEventListeners(); - this.messagingSystem.subscribe( - 'SnapController:snapInstalled', - this._handleSnapRegisterEvent, - ); + // Register action handlers + this.#initializeActionHandlers(); - this.messagingSystem.subscribe( - 'SnapController:snapUninstalled', - this._handleSnapUnregisterEvent, - ); + this.dailyCheckIn().catch((error) => { + logError(error); + }); - this.messagingSystem.subscribe( - 'SnapController:snapEnabled', - this._handleSnapEnabledEvent, - ); + this.#rescheduleBackgroundEvents(Object.values(this.state.events)); + } - this.messagingSystem.subscribe( - 'SnapController:snapDisabled', - this._handleSnapDisabledEvent, - ); + /** + * Initialize event listeners. + */ + #initializeEventListeners() { + /* eslint-disable @typescript-eslint/unbound-method */ + const events = { + 'SnapController:snapInstalled': this._handleSnapRegisterEvent, + 'SnapController:snapUninstalled': this._handleSnapUnregisterEvent, + 'SnapController:snapEnabled': this._handleSnapEnabledEvent, + 'SnapController:snapDisabled': this._handleSnapDisabledEvent, + 'SnapController:snapUpdated': this._handleEventSnapUpdated, + }; + /* eslint-disable @typescript-eslint/unbound-method */ - this.messagingSystem.subscribe( - 'SnapController:snapUpdated', - this._handleEventSnapUpdated, - ); - /* eslint-enable @typescript-eslint/unbound-method */ + Object.entries(events).forEach(([event, handler]) => { + this.messagingSystem.subscribe( + event as CronjobControllerEvents['type'], + handler, + ); + }); + } - this.messagingSystem.registerActionHandler( - `${controllerName}:scheduleBackgroundEvent`, - (...args) => this.scheduleBackgroundEvent(...args), - ); + /** + * Initialize action handlers. + */ + #initializeActionHandlers(): void { + const handlers: Record any> = { + [`${controllerName}:scheduleBackgroundEvent`]: ( + event: Omit, + ) => this.scheduleBackgroundEvent(event), + [`${controllerName}:cancelBackgroundEvent`]: ( + origin: string, + id: string, + ) => this.cancelBackgroundEvent(origin, id), + [`${controllerName}:getBackgroundEvents`]: (snapId: SnapId) => + this.getBackgroundEvents(snapId), + }; - this.messagingSystem.registerActionHandler( - `${controllerName}:cancelBackgroundEvent`, - (...args) => this.cancelBackgroundEvent(...args), - ); + Object.entries(handlers).forEach(([action, handler]) => { + this.messagingSystem.registerActionHandler( + action as BackgroundEventActions, + handler, + ); + }); + } - this.messagingSystem.registerActionHandler( - `${controllerName}:getBackgroundEvents`, - (...args) => this.getBackgroundEvents(...args), - ); + /** + * Execute a request. + * + * @param snapId - ID of a Snap. + * @param request - Request to be executed. + */ + async #executeRequest(snapId: SnapId, request: unknown) { + await this.messagingSystem + .call('SnapController:handleRequest', { + snapId, + origin: '', + handler: HandlerType.OnCronjob, + request: request as Record, + }) + .catch((error) => { + logError(error); + }); + } - this.dailyCheckIn().catch((error) => { - logError(error); + /** + * Setup a timer. + * + * @param id - ID of a timer. + * @param ms - Time in milliseconds. + * @param snapId - ID of a Snap. + * @param onComplete - Callback function to be executed when the timer completes. + * @param validateTime - Whether the time should be validated. + */ + #setupTimer( + id: string, + ms: number, + snapId: SnapId, + onComplete: () => void, + validateTime = true, + ) { + if (validateTime && ms <= 0) { + throw new Error('Cannot schedule execution in the past.'); + } + + const timer = new Timer(ms); + timer.start(() => { + onComplete(); + this.#timers.delete(id); + this.#snapIds.delete(id); }); - this.#rescheduleBackgroundEvents(Object.values(this.state.events)); + this.#timers.set(id, timer); + this.#snapIds.set(id, snapId); + } + + /** + * Cleanup a timer. + * + * @param id - ID of a timer. + */ + #cleanupTimer(id: string) { + const timer = this.#timers.get(id); + if (timer) { + timer.cancel(); + this.#timers.delete(id); + this.#snapIds.delete(id); + } } /** @@ -277,23 +355,20 @@ export class CronjobController extends BaseController< return; } - const timer = new Timer(ms); - timer.start(() => { - this.#executeCronjob(job).catch((error) => { - // TODO: Decide how to handle errors. - logError(error); - }); - - this.#timers.delete(job.id); - this.#schedule(job); - }); + this.#setupTimer( + job.id, + ms, + job.snapId, + () => { + this.#executeCronjob(job).catch(logError); + this.#schedule(job); + }, + false, + ); if (!this.state.jobs[job.id]?.lastRun) { - this.#updateJobLastRunState(job.id, 0); // 0 for init, never ran actually + this.#updateJobLastRunState(job.id, 0); } - - this.#timers.set(job.id, timer); - this.#snapIds.set(job.id, job.snapId); } /** @@ -303,12 +378,7 @@ export class CronjobController extends BaseController< */ async #executeCronjob(job: Cronjob) { this.#updateJobLastRunState(job.id, Date.now()); - await this.#messenger.call('SnapController:handleRequest', { - snapId: job.snapId, - origin: '', - handler: HandlerType.OnCronjob, - request: job.request, - }); + await this.#executeRequest(job.snapId, job.request); } /** @@ -362,10 +432,7 @@ export class CronjobController extends BaseController< '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); + this.#cleanupTimer(id); this.update((state) => { delete state.events[id]; }); @@ -381,32 +448,12 @@ export class CronjobController extends BaseController< const now = new Date(); const ms = date.getTime() - now.getTime(); - if (ms <= 0) { - throw new Error('Cannot schedule an event in the past.'); - } - - const timer = new Timer(ms); - timer.start(() => { - this.#messenger - .call('SnapController:handleRequest', { - snapId: event.snapId, - origin: '', - handler: HandlerType.OnCronjob, - request: event.request, - }) - .catch((error) => { - logError(error); - }); - - this.#timers.delete(event.id); - this.#snapIds.delete(event.id); + this.#setupTimer(event.id, ms, event.snapId, () => { + this.#executeRequest(event.snapId, event.request).catch(logError); this.update((state) => { delete state.events[event.id]; }); }); - - this.#timers.set(event.id, timer); - this.#snapIds.set(event.id, event.snapId); } /** @@ -535,32 +582,23 @@ export class CronjobController extends BaseController< 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, - ); + const events = { + 'SnapController:snapInstalled': this._handleSnapRegisterEvent, + 'SnapController:snapUninstalled': this._handleSnapUnregisterEvent, + 'SnapController:snapEnabled': this._handleSnapEnabledEvent, + 'SnapController:snapDisabled': this._handleSnapDisabledEvent, + 'SnapController:snapUpdated': this._handleEventSnapUpdated, + }; - this.messagingSystem.unsubscribe( - 'SnapController:snapUpdated', - this._handleEventSnapUpdated, - ); /* eslint-enable @typescript-eslint/unbound-method */ + Object.entries(events).forEach(([event, handler]) => { + this.messagingSystem.unsubscribe( + event as CronjobControllerEvents['type'], + handler, + ); + }); + this.#snapIds.forEach((snapId) => this.unregister(snapId)); } From 75020fbc5765ad549385562606b184a3104eb6c8 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 21 Jan 2025 10:33:19 -0500 Subject: [PATCH 2/8] undo comment deletion --- packages/snaps-controllers/src/cronjob/CronjobController.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 91cc6888c4..97803f20bc 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -360,6 +360,7 @@ export class CronjobController extends BaseController< ms, job.snapId, () => { + // TODO: Decide how to handle errors. this.#executeCronjob(job).catch(logError); this.#schedule(job); }, @@ -367,7 +368,7 @@ export class CronjobController extends BaseController< ); if (!this.state.jobs[job.id]?.lastRun) { - this.#updateJobLastRunState(job.id, 0); + this.#updateJobLastRunState(job.id, 0); // 0 for init, never ran actually } } From 33dfc92ee0ad9f3c6f72027354e5c193ed2d8532 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 21 Jan 2025 10:55:22 -0500 Subject: [PATCH 3/8] fix coverage --- packages/snaps-controllers/coverage.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 8bbda9cafe..d8d1a9a219 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,5 +1,5 @@ { - "branches": 93.11, + "branches": 93.01, "functions": 97.14, "lines": 98.13, "statements": 97.84 From 546f791b6657276f227f62967447ff795006b221 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Tue, 21 Jan 2025 13:00:21 -0500 Subject: [PATCH 4/8] fix coverage again --- packages/snaps-controllers/coverage.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index d8d1a9a219..75e854c1aa 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { "branches": 93.01, - "functions": 97.14, - "lines": 98.13, - "statements": 97.84 + "functions": 97.15, + "lines": 98.15, + "statements": 97.87 } From 9452debb906d3738aa7a7451b677487d8c7061c1 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 23 Jan 2025 08:05:17 -0500 Subject: [PATCH 5/8] apply code review --- .../src/cronjob/CronjobController.ts | 75 ++++++++++--------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index 028b74792d..e85a171768 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -78,11 +78,6 @@ export type CronjobControllerEvents = | SnapDisabled | CronjobControllerStateChangeEvent; -type BackgroundEventActions = - | `${typeof controllerName}:scheduleBackgroundEvent` - | `${typeof controllerName}:cancelBackgroundEvent` - | `${typeof controllerName}:getBackgroundEvents`; - export type CronjobControllerMessenger = RestrictedControllerMessenger< typeof controllerName, CronjobControllerActions, @@ -176,45 +171,51 @@ export class CronjobController extends BaseController< */ #initializeEventListeners() { /* eslint-disable @typescript-eslint/unbound-method */ - const events = { - 'SnapController:snapInstalled': this._handleSnapRegisterEvent, - 'SnapController:snapUninstalled': this._handleSnapUnregisterEvent, - 'SnapController:snapEnabled': this._handleSnapEnabledEvent, - 'SnapController:snapDisabled': this._handleSnapDisabledEvent, - 'SnapController:snapUpdated': this._handleEventSnapUpdated, - }; - /* eslint-disable @typescript-eslint/unbound-method */ + this.messagingSystem.subscribe( + 'SnapController:snapInstalled', + this._handleSnapRegisterEvent, + ); - Object.entries(events).forEach(([event, handler]) => { - this.messagingSystem.subscribe( - event as CronjobControllerEvents['type'], - handler, - ); - }); + this.messagingSystem.subscribe( + 'SnapController:snapUninstalled', + this._handleSnapUnregisterEvent, + ); + + this.messagingSystem.subscribe( + 'SnapController:snapEnabled', + this._handleSnapEnabledEvent, + ); + + this.messagingSystem.subscribe( + 'SnapController:snapDisabled', + this._handleSnapDisabledEvent, + ); + + this.messagingSystem.subscribe( + 'SnapController:snapUpdated', + this._handleEventSnapUpdated, + ); + /* eslint-disable @typescript-eslint/unbound-method */ } /** * Initialize action handlers. */ - #initializeActionHandlers(): void { - const handlers: Record any> = { - [`${controllerName}:scheduleBackgroundEvent`]: ( - event: Omit, - ) => this.scheduleBackgroundEvent(event), - [`${controllerName}:cancelBackgroundEvent`]: ( - origin: string, - id: string, - ) => this.cancelBackgroundEvent(origin, id), - [`${controllerName}:getBackgroundEvents`]: (snapId: SnapId) => - this.getBackgroundEvents(snapId), - }; + #initializeActionHandlers() { + this.messagingSystem.registerActionHandler( + `${controllerName}:scheduleBackgroundEvent`, + (...args) => this.scheduleBackgroundEvent(...args), + ); - Object.entries(handlers).forEach(([action, handler]) => { - this.messagingSystem.registerActionHandler( - action as BackgroundEventActions, - handler, - ); - }); + this.messagingSystem.registerActionHandler( + `${controllerName}:cancelBackgroundEvent`, + (...args) => this.cancelBackgroundEvent(...args), + ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:getBackgroundEvents`, + (...args) => this.getBackgroundEvents(...args), + ); } /** From 2d380a24d69e29c3b5c57cc50bc3e7c5ce4f9409 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 23 Jan 2025 08:17:01 -0500 Subject: [PATCH 6/8] update coverage --- packages/snaps-controllers/coverage.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index fffcd4a239..4ab1093e89 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { - "branches": 93.06, - "functions": 96.59, - "lines": 98.08, - "statements": 97.8 + "branches": 93.1, + "functions": 97.16, + "lines": 98.18, + "statements": 97.9 } From 131a290981b8bf6c4b857b57ad8144e03ef7a7c9 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 23 Jan 2025 08:23:35 -0500 Subject: [PATCH 7/8] revert change to destroy method --- .../src/cronjob/CronjobController.ts | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/packages/snaps-controllers/src/cronjob/CronjobController.ts b/packages/snaps-controllers/src/cronjob/CronjobController.ts index e85a171768..15b963f463 100644 --- a/packages/snaps-controllers/src/cronjob/CronjobController.ts +++ b/packages/snaps-controllers/src/cronjob/CronjobController.ts @@ -581,22 +581,31 @@ export class CronjobController extends BaseController< super.destroy(); /* eslint-disable @typescript-eslint/unbound-method */ - const events = { - 'SnapController:snapInstalled': this._handleSnapRegisterEvent, - 'SnapController:snapUninstalled': this._handleSnapUnregisterEvent, - 'SnapController:snapEnabled': this._handleSnapEnabledEvent, - 'SnapController:snapDisabled': this._handleSnapDisabledEvent, - 'SnapController:snapUpdated': this._handleEventSnapUpdated, - }; + this.messagingSystem.unsubscribe( + 'SnapController:snapInstalled', + this._handleSnapRegisterEvent, + ); - /* eslint-enable @typescript-eslint/unbound-method */ + this.messagingSystem.unsubscribe( + 'SnapController:snapUninstalled', + this._handleSnapUnregisterEvent, + ); - Object.entries(events).forEach(([event, handler]) => { - this.messagingSystem.unsubscribe( - event as CronjobControllerEvents['type'], - handler, - ); - }); + 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)); } From f7f4cb51c0bf26d899c6c147d43642f9f70f946a Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 23 Jan 2025 09:23:23 -0500 Subject: [PATCH 8/8] update coverage --- packages/snaps-controllers/coverage.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 4ab1093e89..3f5a02686e 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { "branches": 93.1, - "functions": 97.16, + "functions": 97.15, "lines": 98.18, "statements": 97.9 }