feat: add alerts for redesigned contract interaction confirmation #25174
Conversation
Fix multiple alert modal from inline alert. Add dummy estimated fee row.
Move transaction alerts to separate function.
Builds ready [04229ba]
Page Load Metrics (51 ± 4 ms)
|
Builds ready [500e1ba]
Page Load Metrics (47 ± 4 ms)
|
| export const selectUnapprovedMessage = createDeepEqualSelector( | ||
| internalSelectUnapprovedMessage, | ||
| (message) => message, | ||
| ); |
There was a problem hiding this comment.
Confirmation specific selectors can be placed here: https://github.com/MetaMask/metamask-extension/tree/develop/ui/pages/confirmations/selectors
There was a problem hiding this comment.
These new selectors aren't really specific to confirmations, since they are simply querying the signature controller state and using createSelector and createDeepEqualSelector to minimise re-renders. All the existing message selectors were legacy and mixed with transactions.
On a broader level, I worry that creating separate confirmation specific selectors might restrict their re-usability since all the existing selectors are defined at the top level in ui/selectors, so arguably any truly confirmation specific ones could also sit in ui/selectors/confirmation.ts?
| isSIWE, | ||
| transactionMetadata, | ||
| signatureMessage, | ||
| ]); |
There was a problem hiding this comment.
Here we are doing all of logic like iterating over confirmations outside useMemo.
There was a problem hiding this comment.
That's correct, the performance cost of any logic here is negligible so the purpose of the useMemo is to only return a fresh object reference when the underlying data has changed, to prevent all the dependent components re-rendering unnecessarily.
Any iteration over the controller state is encapsulated within the selectors, which is a cleaner separation of concerns, plus we can benefit from deep equal selectors that also only return new object references when the specifc controller state we care about has actually changed.
| ); | ||
| }; | ||
|
|
||
| export default ConfirmAlerts; |
|
Sign alerts look good: signAlerts.movNew Sign Permit is not detected as 'deceptive' - I'm not sure if it is expected: signPermit.mov |
Builds ready [e2138dc]
Page Load Metrics (44 ± 3 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-12.1.0
Description
Migrate the existing transaction alerts for contract interaction transactions to the new alert system.
In addition:
useCurrentConfirmationhook to simplify the logic and use deep equal selectors to automatically update the confirmation if the underlying data changes.Related issues
Fixes: #23953
Manual testing steps
E2E tests per alert will be created in a subsequent ticket, so testing each alert now is not needed.
Instead we could just verify that the existing signature confirmations are unaffected and no transaction alerts are shown.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist