feat: Add domain binding SIWE redesign alert row #25281
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25281 +/- ##
===========================================
+ Coverage 69.69% 69.70% +0.01%
===========================================
Files 1350 1351 +1
Lines 47865 47881 +16
Branches 13199 13203 +4
===========================================
+ Hits 33355 33371 +16
Misses 14510 14510 ☔ View full report in Codecov by Sentry. |
jpuri
reviewed
Jun 20, 2024
…mation useBlockaidAlert
Builds ready [780f5b7]
Page Load Metrics (47 ± 6 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
matthewwalsh0
previously approved these changes
Jun 25, 2024
Builds ready [e817fec]
Page Load Metrics (43 ± 3 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [e817fec]
Page Load Metrics (43 ± 3 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
hello again and apologies, may I get a re-review please? @jpuri @matthewwalsh0 merge conflict undid approvals. I took this as an opportunity to update the filepaths from useDomainMismatchAlert → useDomainMismatchAlerts to match pattern of similar hooks |
matthewwalsh0
approved these changes
Jun 26, 2024
Builds ready [52502c2]
Page Load Metrics (136 ± 171 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
jpuri
reviewed
Jun 27, 2024
| @@ -44,7 +44,7 @@ const SIWESignInfo: React.FC = () => { | |||
| return ( | |||
| <> | |||
| <ConfirmInfoRow label={t('message')}> | |||
| <ConfirmInfoRowText text={statement} /> | |||
| <ConfirmInfoRowText text={statement || ''} /> | |||
There was a problem hiding this comment.
Suggested change
| <ConfirmInfoRowText text={statement || ''} /> | |
| <ConfirmInfoRowText text={statement ?? ''} /> |
There was a problem hiding this comment.
oh nice! I can update this in a subsequent alert PR to not need re-reviews
jpuri
reviewed
Jun 27, 2024
|
|
||
| const isSIWE = isSIWESignatureRequest(currentConfirmation); | ||
| const isInvalidSIWEDomain = | ||
| isSIWE && !isValidSIWEOrigin(msgParams as WrappedSIWERequest); |
There was a problem hiding this comment.
It will be useful to include above 3 lines also inside useMemo
There was a problem hiding this comment.
I had this originally and then @matthewwalsh0 brought up a good point to reduce the # of observed dependencies
metamaskbot
added
the
release-12.1.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, the existing logic displays the same friction modal for multiple alerts with only one danger alert. There will be a separate PR to update the Alert System to follow the designs mentioned in the Issue ticket
Related issues
Fixes: #24683
Manual testing steps
Screenshots/Recordings
Before
After
(There is another PR that is hiding the "i" icon beside the Alert for SIWE)
Pre-merge author checklist
Pre-merge reviewer checklist