-
Notifications
You must be signed in to change notification settings - Fork 560
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
feat: Add SnapInsightsController
#2555
Conversation
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
@metamaskbot update-pr |
875565c
to
5b90101
Compare
packages/snaps-controllers/src/insights/SnapInsightsController.ts
Outdated
Show resolved
Hide resolved
packages/snaps-controllers/src/insights/SnapInsightsController.ts
Outdated
Show resolved
Hide resolved
3327cf8
to
f6dba70
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2555 +/- ##
==========================================
+ Coverage 93.95% 93.99% +0.03%
==========================================
Files 456 459 +3
Lines 9568 9647 +79
Branches 1465 1480 +15
==========================================
+ Hits 8990 9068 +78
- Misses 578 579 +1 ☔ View full report in Codecov by Sentry. |
packages/snaps-controllers/src/insights/SnapInsightsController.ts
Outdated
Show resolved
Hide resolved
import type { Hex, Json } from '@metamask/utils'; | ||
|
||
// Partial types that should overlap with types from controllers. | ||
export type TransactionMeta = { |
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.
It's not ideal that we have to copy this. Can we add a dev dependency and type tests to make sure the types are in sync at least?
}) { | ||
this.update((state) => { | ||
state.insights[id][snapId].loading = false; | ||
state.insights[id][snapId].interfaceId = response?.id as string; |
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.
Why do we need a type cast here?
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.
The response type is typed as Json
, but we know the return type for an insight Snap will contain id
as a string or undefined.
SnapInsightsController
This PR adds
SnapInsightsController
, which is meant to handle fetching a persistent response from transaction/signature insight Snaps. This controller will temporarily store the insight response, letting the client UI consume it wherever necessary.The controller listens to events from
TransactionController
andSignatureController
to determine when new signatures or transactions have entered the state. The events immediately trigger requests to any available Snaps. The controller also manages cleaning up its own state as well as the interface state when a confirmation finishes.Most of the controller logic should mirror the implementation currently found in the extension. With the exception that this controller now populates the state as Snaps respond instead of waiting for all the Snaps to load before triggering a state update.
Closes #2560