feat: add supported for typedSignData V1 by jpuri · Pull Request #23539 · MetaMask/metamask-extension

jpuri · 2024-03-18T04:52:03Z

Description

Re-design implementation for typed sign V1 signature requests.

Related issues

Fixes: #23481

Manual testing steps

Enable re-designs locally
Go to test dapp
Submit typed sign V1 message signature

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-03-18T04:52:16Z

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.

cryptotavares · 2024-03-22T11:42:10Z

ui/pages/confirmations/components/confirm/info/personal-sign/personal-sign.tsx

+  const t = useI18nContext();
+  const currentConfirmation = useSelector(currentConfirmationSelector);
+
+  if (!currentConfirmation?.msgParams) {


is it possible for currentConfirmation to be undefined?

TS complains if I do not use ?

cryptotavares · 2024-03-22T11:46:24Z

ui/pages/confirmations/components/confirm/info/personal-sign/personal-sign.test.tsx


-describe('personalSign', () => {
-  it('renders origin for personal sign request', () => {
+describe('PersonalSignInfo', () => {


In case the currentConfirmation can be undefined, should we add a test for that?

Not really... but since hook can possible return undefined, TS requires me to handle it

ui/pages/confirmations/components/confirm/info/personal-sign/personal-sign.test.tsx

cryptotavares · 2024-03-22T12:12:11Z

ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx

No need for optional chainig.. we have arleady validated in the previous if clause

Also would love if we could avoid type casting

Yep optional chaining can be avoided here but typecasting not

ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx

cryptotavares · 2024-03-22T12:20:00Z

ui/pages/confirmations/components/confirm/info/info.tsx

Should this be abstracted in the ConfirmationInfoConponentMap?
We could isolate Confirmation component selection logic within it (in my opinion making it more isolate and readable). Something like:

const getTypedSignInfo = (currentConfirmation) => { const { version } = currentConfirmation?.msgParams ?? {}; if (version === 'V1') { return TypedSignV1Info; } return TypedSignInfo; } const ConfirmationInfoConponentMap = { [TransactionType.personalSign]: () => PersonalSignInfo, [TransactionType.signTypedData]: getTypedSignInfo, }; ... const InfoComponent = ConfirmationInfoConponentMap[currentConfirmation?.type as ConfirmationType](currentConfirmation);

What do you think?

(Also just noticed, ConfirmationInfoConponentMap has a typo.. should be ConfirmationInfoComponentMap)

We can do it either way, since the logic will stay in 1 file to me either looks ok

PR is updated

ui/pages/confirmations/components/confirm/info/typed-sign-v1/typed-sign-v1.test.tsx

…n into typed_sign_v1

codecov · 2024-03-26T09:01:55Z

Codecov Report

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

Project coverage is 69.30%. Comparing base (8b12ade) to head (04efefe).

Files	Patch %	Lines
...ges/confirmations/components/confirm/info/info.tsx	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #23539      +/-   ##
===========================================
+ Coverage    69.28%   69.30%   +0.02%     
===========================================
  Files         1158     1160       +2     
  Lines        44069    44083      +14     
  Branches     11758    11760       +2     
===========================================
+ Hits         30531    30548      +17     
+ Misses       13538    13535       -3

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

metamaskbot · 2024-03-26T09:05:21Z

Builds ready [04efefe]

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, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (451 ± 336 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	62	161	117	27	13
		domContentLoaded	13	84	37	23	11
		load	50	2021	451	700	336
		domInteractive	13	84	37	23	11

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

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 Mar 18, 2024

jpuri requested a review from a team as a code owner March 18, 2024 04:52

jpuri changed the title ~~Add supported for typedSignData V1~~ feat: add supported for typedSignData V1 Mar 18, 2024

jpuri force-pushed the typed_sign_mmi branch from 565a352 to 700402b Compare March 21, 2024 14:55

cryptotavares reviewed Mar 22, 2024

View reviewed changes

jpuri force-pushed the typed_sign_mmi branch from 020cc24 to 952f825 Compare March 22, 2024 11:48

cryptotavares reviewed Mar 22, 2024

View reviewed changes

ui/pages/confirmations/components/confirm/info/personal-sign/personal-sign.test.tsx Outdated Show resolved Hide resolved

jpuri force-pushed the typed_sign_v1 branch from 2e34dae to 00576be Compare March 22, 2024 11:54

cryptotavares reviewed Mar 22, 2024

View reviewed changes

ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx Outdated Show resolved Hide resolved

cryptotavares reviewed Mar 22, 2024

View reviewed changes

ui/pages/confirmations/components/confirm/info/typed-sign-v1/typed-sign-v1.test.tsx Outdated Show resolved Hide resolved

Base automatically changed from typed_sign_mmi to develop March 22, 2024 15:36

Add supported for typedSignData V1

d3844bd

jpuri force-pushed the typed_sign_v1 branch from 00576be to d3844bd Compare March 22, 2024 16:17

update

ba16b74

jpuri requested a review from cryptotavares March 22, 2024 16:27

jpuri added 2 commits March 22, 2024 22:22

Update

4dc55de

Merge branch 'develop' into typed_sign_v1

0b70b95

jpuri mentioned this pull request Mar 25, 2024

[Bug]: Confirmations Re-design - Sign Typed Data signature not using Re-design components #23688

Closed

jpuri added 2 commits March 26, 2024 14:09

merge

269738a

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

04efefe

…n into typed_sign_v1

pedronfigueiredo approved these changes Mar 26, 2024

View reviewed changes

blackdevelopa approved these changes Mar 26, 2024

View reviewed changes

jpuri merged commit 23c2d27 into develop Mar 26, 2024

jpuri deleted the typed_sign_v1 branch March 26, 2024 14:32

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

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

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

Conversation

jpuri commented Mar 18, 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 Mar 18, 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!

jpuri Mar 22, 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!

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!

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!

Uh oh!

codecov bot commented Mar 26, 2024

Codecov Report

Uh oh!

metamaskbot commented Mar 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jpuri commented Mar 18, 2024 •

edited

Loading

jpuri Mar 22, 2024 •

edited

Loading