-
Notifications
You must be signed in to change notification settings - Fork 644
refactor!: Pass executor parameters by object #3803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9c304c9 to
75ef6cf
Compare
75ef6cf to
5d0b9fc
Compare
This PR refactors `BaseSnapExecutor` to follow our more recent patterns, notably using hash private for most properties and functions, but also changing a couple of naming patterns and improving types. This makes `BaseSnapExecutor` way more compliant with our linter. Split off #3803 to make this easier to review. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches core snap execution/termination and teardown tracking; while mostly a refactor, it changes how termination promises and teardown state are managed and could affect edge-case lifecycle behavior. > > **Overview** > Refactors `BaseSnapExecutor` to use **hash-private** fields/methods throughout (removing prior lint suppressions) and tightens command dispatch typing by calling `#methods[method as keyof Methods]` instead of `any`. > > Updates execution/termination plumbing: snap evaluation cancellation now uses `createDeferredPromise` and a shared `#teardownRef` object is passed into `withTeardown` instead of `this`, changing the termination error string to `The Snap "..." has been terminated during execution.` (and updating the corresponding browser test expectation). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 0082a8c. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
108c3e2 to
40406eb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3803 +/- ##
==========================================
+ Coverage 98.39% 98.46% +0.07%
==========================================
Files 430 429 -1
Lines 12457 12395 -62
Branches 1932 1922 -10
==========================================
- Hits 12257 12205 -52
+ Misses 200 190 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Refactor
BaseSnapExecutorto expect parameters to always be passed by object. We were already passing the parameters by object in production, but then internally converting them to an array. This made typing harder and generally isn't necessary. We can now more easily validate the parameters and correctly narrow the types, reducing the casting required. The PR also refactors the handling of incoming command requests a bit to become more readable. Theoretically this should also make the executor a bit faster at processing commands.This PR is breaking as now only passing by object is allowed.
Note
Medium Risk
Medium risk because it changes the internal JSON-RPC command contract for
executeSnap/snapRpc(array params no longer accepted) and refactors command dispatch/validation in the snap execution environment, which could break any remaining callers still using positional params.Overview
Breaking change: executor command RPCs now require named object params (no positional arrays).
executeSnaptakes{ snapId, sourceCode, endowments }andsnapRpctakes{ snapId, handler, origin, request };snapRpcalso drops the oldtargetfield.BaseSnapExecutorcommand handling is refactored to a switch-based dispatcher with centralized param validation (assertCommandParams), removing the old param-sorting/command-implementation helpers. Tests and test utilities across browser/iframe/node execution environments are updated accordingly, and coverage expectations are adjusted.Written by Cursor Bugbot for commit edee29c. This will update automatically on new commits. Configure here.