feat: Integrate Snaps into the redesigned confirmations #26435
Conversation
FrederikBolding
force-pushed
the
fb/confirmation-redesign-insights
branch
from
9f0dd14 to
59f7357
Compare
FrederikBolding
force-pushed
the
fb/confirmation-redesign-insights
branch
from
0523104 to
02025bf
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26435 +/- ##
===========================================
+ Coverage 70.04% 70.05% +0.01%
===========================================
Files 1411 1411
Lines 49205 49210 +5
Branches 13754 13758 +4
===========================================
+ Hits 34463 34472 +9
+ Misses 14742 14738 -4 ☔ View full report in Codecov by Sentry. |
Builds ready [02025bf]
Page Load Metrics (99 ± 23 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
|
Builds ready [a331fa1]
Page Load Metrics (85 ± 10 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| @@ -38,7 +38,7 @@ const ExpandableIcon = ({ isExpanded }: { isExpanded: boolean }) => { | |||
| <Icon | |||
| name={isExpanded ? IconName.ArrowUp : IconName.ArrowDown} | |||
| size={IconSize.Sm} | |||
| color={IconColor.iconMuted} | |||
| color={IconColor.primaryDefault} | |||
There was a problem hiding this comment.
Minor, would it have value for audit to include a before screenshot with the old styling?
There was a problem hiding this comment.
Sure! This is roughly what the integration looked like before the design changes.
ui/pages/confirmations/components/confirm/snaps/snaps-section/snaps-section.tsx
Show resolved
Hide resolved
|
|
||
| export const SnapsSection = () => { | ||
| const currentConfirmation = useSelector(currentConfirmationSelector); | ||
| const { data } = useInsightSnaps(currentConfirmation?.id); |
There was a problem hiding this comment.
Just for my own understanding, am I right that the hook and underlying controller don't use the actual frontend data itself, but rather query the data direct from the controllers using just the ID?
This should be harmless if so given I believe both signatures and transactions uses UUIDs for IDs so clashes are almost impossible.
There was a problem hiding this comment.
Indeed! SnapInsightsController populates metamask.insights[ID] per transaction and signature that it detects via the events emitted by the TransactionController and SignatureController. It assumes the IDs doesn't clash, which should be safe given that they are UUIDs. For more details see: MetaMask/snaps#2555
| <SnapInsight | ||
| key={snapId} | ||
| snapId={snapId} | ||
| interfaceId={interfaceId} |
There was a problem hiding this comment.
Am I right that the hook and insight controller don't return any actual data or JSON to the UI, but just this interface ID that corresponds to a template in the state via the controller?
There was a problem hiding this comment.
Yes! All that is stored in the insights controller are these interfaceIds. They are effectively a pointer to an interface stored in state at metamask.interfaces, which is constructed using the Snaps UI JSX syntax. The SnapUIRenderer takes this interface ID and renders the interface that is available in state. For certain Snaps this interface may change over time, e.g. with a button press or some other trigger. The SnapUIRenderer handles re-rendering the interface when that happens!
metamaskbot
added
the
release-12.5.0
gauthierpetetin
added
release-12.4.0
Description
This PR integrates Snaps insights into the redesigned signature confirmations, showing them at the bottom using the new
Delineatorcomponent. Snaps are exposed to the new confirmations viaSnapsSectionandSnapInsight, these two components use the newly writtenuseInsightSnapshook.By request of @eriknson this PR makes some slight adjustments to the
Delineatorcomponent, tweaking the font colors, adding a disabled state etc.This PR does not integrate Snaps into the alert system, that'll be done in a follow-up PR at a later date.
Related issues
Closes MetaMask/snaps#2530
Manual testing steps
Screenshots/Recordings