Confirmation redesign: add message section to personal sign page #22766

jpuri · 2024-02-01T11:08:39Z

Description

Adding message section to personal sign page

Related issues

Fixes: MetaMask/MetaMask-planning#1514

Manual testing steps

Enable confirmation re-design locally
Submit a personal signature request
Check origin section on personal sign page

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-02-01T11:08:52Z

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.

metamaskbot · 2024-02-01T13:32:38Z

Builds ready [3dbf2a7]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (833 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	94	139	114	13	6
		domContentLoaded	9	39	18	6	3
		load	754	1128	833	83	40
		domInteractive	9	39	18	6	3

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 1.21 KiB (0.02%)
common: 314 Bytes (0.01%)

codecov · 2024-02-01T13:32:44Z

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 68.59%. Comparing base (f561faa) to head (f8bab21).

❗ Current head f8bab21 differs from pull request most recent head fb52651. Consider uploading reports for the commit fb52651 to get more accurate results

Files	Patch %	Lines
ui/components/app/confirm/info/info.tsx	0.00%	2 Missing ⚠️
ui/helpers/utils/util.js	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #22766      +/-   ##
===========================================
- Coverage    68.70%   68.59%   -0.11%     
===========================================
  Files         1102     1102              
  Lines        43221    43165      -56     
  Branches     11558    11541      -17     
===========================================
- Hits         29691    29607      -84     
- Misses       13530    13558      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Olusegun Akintayo <akintayo.segun@gmail.com>

FrederikBolding · 2024-02-06T09:33:39Z

ui/helpers/utils/util.js

+    return hex;
+  }
+  try {
+    const stripped = stripHexPrefix(hex);


Can we use @metamask/utils instead of defining new functions?

I could not find an equivalent method in @matamask/utils.

I would recommend hexToBytes and bytesToString, though I'm not sure I fully understand the conditionals in this utility function.

@jpuri thoughts on the above?
Also for consistent behaviour we should replace the msgHexToText in the SignatureRequestOriginal component

@cryptotavares : I would avoid changing original confirmation components. As we work on more confirmations, we will come across many situations like this to keep code consistent, but it will be hard to change + test existing stuff also as we make new. Ans, also eventually we will g et rid of old components.

We have agreed that if it is a straightforward duplication then yes we should update current confirmations components with the shared code (instead of keeping the duplication)... it should simplify maintenance (for the time where we have to support old and new components). But the moment the logic is no longer a duplication and branches off somewhere, then we shouldn't update the old components (as we do not want to couple any new code paths that we are adding to old components, making it harder to get rid of them once the time comes).

FrederikBolding · 2024-02-06T09:34:24Z

ui/components/app/confirm/signature-message/signature-message.tsx

+  const t = useI18nContext();
+  const currentConfirmation = useSelector(currentConfirmationSelector);
+
+  const args = useMemo(() => {


Why is this architecture preferable than just using plain JSX? It seems like this would be harder to maintain without much added benefit. Unless I am missing something?

This gives us more flexibility as these components will be reused for different types of confirmations.
And I actually do not see any maintenance overhead in keeping this logic in / out of jsx.

Could you elaborate on how this system "gives us more flexibility"? I'm having some trouble following how this config maps to components, but it seems like this would be simpler to understand using more standard techniques.

@Gudahtt : we have multiple confirmation pages with different combination of this information displayed. I see this new info section more as tabular display of this data.

And using array to pass data to ConfirmInfo components gives us flexibility. But wherever we have fixed set of data like this example of message it will be better to use the jsx version.

Ref: #22763

IMHO, jsx is not the best place for conditional, plain JS or TS code is better at that. These components will take care of different confirmation type with differing data to be displayed, this nested conditional in JSX does not look like a good idea to me.

I am not against either of the approaches, I also agree its more of preference, but I do not see a downside of current approach either.

JSX is syntactic sugar and the conditionals will be eventually converted to JS code only.

JSX is syntactic sugar

Exactly, "syntactic sugar" making the code more readable (at least to my eyes, fwiw)

As someone not as heavily involved with this code as everyone else is in this thread, reading (conditional) JSX seems much easier to me than what is proposed in the 2nd example

I see a lot of people chiming in into this decision. The reality is that due to the nature of confirmations (all different possibilities and confirmations types, data that is displayed to the user, actions that the user can perform, etc) confirmations will always be complex. It is what it is. The question that we asked, was where can we move that complexity, so that it becomes easier to maintain, extend, and build on top of those confirmations.

There are infinite ways to solve this, but we have went through either having:

multiple confirmations pages (which is similar to what we have now a days). Easier to understand the structure of each confirmation screen, but complexity lies on routing and making the right data available to each page. It also encourages coupling components to each confirmation page.

single page with conditional jsx. Though for simple components this might seem a far more readable option, that might not be the case for confirmations. In this case complexity will be in the jsx and all the conditions that it will have. It also encourages coupling confirmation type logic to the jsx.

single page with props array mapping confirm info rows. In this case complexity goes into building/reading the props array. However it offers a more decoupled config per confirmation type, making it easier to maintain and extend. I would even argue if we can tweak the solution a bit and abstract the conditionals from the args and rely on a Map to get the args for each Confirmation type, could provide a more readable solution option 2 (especially if you take confirmations complexity into account).

We are trying to build a solution that will allow us to not only support current confirmations features but also eases up a lot the burden of maintenance while making it easier to continuously build on top. The decision to go with option 3 does not seem an incorrect one to me.

I might be overlooking something here, but how is (1) mutually exclusive from (2)? To me, a combination of the two feels ideal.

For context, every confirmation screen in the redesign is built up with the following sections / components with conditional sections italicized;

An overview of the confirmations DS / structure

Header (required)

Banner / alert (conditional)

Title & description (required)

Request body (required)

Request specific content / rows

3rd party insights in-line of request content (conditional)

3rd party (snaps) insight (conditional)

Footer w/ actions (required)

eriknson

So excited to see this! Here's design review / QA:

…extension into personal_sign_message

metamaskbot · 2024-03-04T08:52:33Z

Builds ready [f8bab21]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1404 ± 393 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	72	222	139	40	19
		domContentLoaded	8	77	33	22	10
		load	59	3026	1404	819	393
		domInteractive	8	77	33	22	10

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 1.21 KiB (0.02%)
common: 423 Bytes (0.01%)

cryptotavares · 2024-03-05T12:47:54Z

ui/helpers/utils/util.test.js

    });
  });
+
+  describe('hexToText', () => {


Can we add a test to cover the hexToText catch block?

I just added this test case

cryptotavares · 2024-03-05T16:52:07Z

ui/pages/confirmations/confirm/confirm.tsx

        <Title />
        <Info />
-        <Box>CONFIRMATION PAGE BODY TO COME HERE</Box>
+        <SignatureMessage />


Should this be abstracted within the Info component? I would expect that the different info blocks would be coupled to each confirmation info component. For example, PersonalSignInfo component should return the actual structure that we are expecting:

<> <Box ... > <ConfirmInfoRow> ... </ConfirmInfoRow> ... </Box> <SignatureMessage> </>

And the Info component could just be confirmation map to the right infoComponent and return it:

return ( <InfoComponent /> )

What do you think about it?

Yes I also have been thinking about this, I actually I wanted to discuss the confirmation designs with Saya to have more clarity on this. In one of the following PRs I will make this change.

segun

looks good except one comment

segun · 2024-03-05T20:55:41Z

ui/components/app/confirm/info/row/text.tsx

+      style={{
+        columnGap: '8px',
+      }}


Suggested change

style={{

columnGap: '8px',

}}

gap={2}

Good point, I made this update

…extension into personal_sign_message

metamaskbot · 2024-03-06T06:27:26Z

Builds ready [fb52651]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1179 ± 335 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	193	131	33	16
		domContentLoaded	9	79	33	22	11
		load	62	1941	1179	697	335
		domInteractive	9	79	33	22	11

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 1.16 KiB (0.02%)
common: 423 Bytes (0.01%)

cryptotavares

Looks good ✅

metamaskbot · 2024-03-06T15:08:47Z

Builds ready [80daf05]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1454 ± 240 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	168	123	31	15
		domContentLoaded	9	85	30	23	11
		load	58	2288	1454	500	240
		domInteractive	9	85	30	23	11

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 723 Bytes (0.01%)
common: 423 Bytes (0.01%)

jpuri added 9 commits January 31, 2024 16:43

Confirmation redesign: adding title to personal sign component

df1340d

Confirmation redesign: adding title to personal sign component

818e82b

fix build

99473e3

fix build

4ce07f1

fix build

cff8515

Confirmation redesign: ass origin details to personal sign page

182534e

Confirmation redesign: ass origin details to personal sign page

1a62dea

Sorting imports

64db05b

Confirmation redesign: add message section to personal sign page

9b725e7

jpuri added team-confirmations-secure-ux-PR PRs from the confirmations team needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) confirmation-redesign labels Feb 1, 2024

jpuri requested a review from a team as a code owner February 1, 2024 11:08

Update

3dbf2a7

segun previously approved these changes Feb 1, 2024

View reviewed changes

Update ui/components/app/confirm/sender-info/sender-info.test.tsx

4c77a47

Co-authored-by: Olusegun Akintayo <akintayo.segun@gmail.com>

FrederikBolding reviewed Feb 6, 2024

View reviewed changes

eriknson requested changes Feb 6, 2024

View reviewed changes

jpuri added 4 commits February 6, 2024 20:15

Update

a9ac08c

Add unit test coverage

b6bf320

Merge

fdd0e1f

Update

ce1e7e5

jpuri marked this pull request as draft February 9, 2024 12:21

jpuri added 4 commits February 9, 2024 18:11

Update

a2cdd02

Update

d5dcc89

Update

5a8c373

Update

4da4138

jpuri added 6 commits March 4, 2024 13:08

Updates

e09ee50

Merge branch 'personal_sign_message' of github.com:MetaMask/metamask-…

cf5560d

…extension into personal_sign_message

Updates

56f4dc5

Merge branch 'develop' into personal_sign_message

2037ac0

Update

73dde7f

Merge branch 'personal_sign_message' of github.com:MetaMask/metamask-…

f8bab21

…extension into personal_sign_message

segun previously approved these changes Mar 5, 2024

View reviewed changes

cryptotavares reviewed Mar 5, 2024

View reviewed changes

blackdevelopa previously approved these changes Mar 5, 2024

View reviewed changes

segun reviewed Mar 5, 2024

View reviewed changes

Update

918892c

jpuri dismissed stale reviews from blackdevelopa and segun via 918892c March 6, 2024 05:37

jpuri added 4 commits March 6, 2024 11:08

Merge branch 'develop' into personal_sign_message

cab506c

Update

035b2ad

Merge branch 'personal_sign_message' of github.com:MetaMask/metamask-…

4f750b7

…extension into personal_sign_message

Update

fb52651

jpuri added 2 commits March 6, 2024 20:07

Update

5153c9f

Update

80daf05

cryptotavares approved these changes Mar 6, 2024

View reviewed changes

blackdevelopa approved these changes Mar 6, 2024

View reviewed changes

jpuri merged commit 75a91b0 into develop Mar 6, 2024

jpuri deleted the personal_sign_message branch March 6, 2024 15:04

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 6, 2024

github-actions bot locked and limited conversation to collaborators Mar 6, 2024

metamaskbot added the release-11.13.0 Issue or pull request that will be included in release 11.13.0 label Mar 6, 2024

Confirmation redesign: add message section to personal sign page #22766

Confirmation redesign: add message section to personal sign page #22766

Uh oh!

Conversation

jpuri commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Feb 1, 2024

Uh oh!

metamaskbot commented Feb 1, 2024

Uh oh!

codecov bot commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrederikBolding Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpuri Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriknson Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriknson left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

segun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 6, 2024

jpuri commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

FrederikBolding Feb 14, 2024 •

edited

Loading

jpuri Feb 19, 2024 •

edited

Loading

eriknson Feb 21, 2024 •

edited

Loading