Feat: Add background events #2941
Conversation
| }; | ||
|
|
||
| const subscriptionMap = new WeakMap(); |
There was a problem hiding this comment.
This is needed to keep a valid reference to the subscriptions, otherwise tests start breaking with subscription not found errors.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2941 +/- ##
==========================================
+ Coverage 94.49% 94.52% +0.03%
==========================================
Files 487 490 +3
Lines 10451 10583 +132
Branches 1604 1613 +9
==========================================
+ Hits 9876 10004 +128
- Misses 575 579 +4 ☔ View full report in Codecov by Sentry. |
| }; | ||
|
|
||
| const subscriptionMap = new WeakMap(); |
There was a problem hiding this comment.
I'm really not a fan of this, there has to be a better way to handle teardown of controllers 🫠
I'll note again that the teardown might not be important regardless due to the client life-cycle. E.g. controllers aren't destroyed unless the extension is being shut down entirely anyway.
There was a problem hiding this comment.
Yeah I know, I mostly added it in because it seemed like we were already testing destroy.
| * @param snap - Basic Snap information. | ||
| */ | ||
| private _handleSnapDisabledEvent(snap: TruncatedSnap) { | ||
| this.unregister(snap.id, true); |
There was a problem hiding this comment.
I'm not following the logic here, unregister is called already for Snaps that are disabled or uninstalled. Why do we need a separate handler and this boolean flag?
There was a problem hiding this comment.
Because we don't want to remove background event state when disabling, a snap can become enabled again and have unexpired events.
packages/snaps-rpc-methods/src/permitted/scheduleBackgroundEvent.ts
Outdated
Show resolved
Hide resolved
packages/snaps-rpc-methods/src/permitted/scheduleBackground.test.ts
Outdated
Show resolved
Hide resolved
Opened an issue for it! :) |
IMO it would be great to add it to this PR for ease of testing when we merge this! |
Added in this PR. |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
This PR adds a new feature: background events, per SIP-28. A summary of the changes made:
CronjobControllerwas updated to now include logic for handling and storing background events.snap_scheduleBackgroundEventRPC method was added to schedule an event.snap_cancelBackgroundEventRPC method was added to cancel an event.snap_getBackgroundEventsRPC method was added to get a snap's background events (not outlined in the SIP, but will be added as an addendum soon).