Add Petnames Name Component to Transaction Approval Screen by dbrans · Pull Request #22190 · MetaMask/metamask-extension

dbrans · 2023-12-06T21:15:00Z

Description

Currently

The extension uses the SenderToRecipient React component to display the from and to address when creating most types of transaction. This is displayed in the header of most transaction confirmations:

This internally uses a RecipientWithAddress component to render the target of the transaction.

This currently also supports names and nicknames but by using state from the AddressBookController, EnsController, and the names of the wallet accounts.

Goal of this PR

use the Name component in the RecipientWithAddress component to use petnames to display the transaction to address.
Continue to use the usePetnamesEnabled hook to only use the Name component if the petnames build feature is enabled.

Notes

Once petnames is in main, the build feature can be removed and the legacy code removed in a separate PR.

See the Name component storybook entry for usage details.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1625
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1626

Manual testing steps

In a flask build:

Click the Send button on the home screen
Fill out an address to send to.
On the confirmation screen you should see the recipient inside a name component (shaped like a pill) with either a warning or a bookmark icon, depending on whether or not there is a petname for this address-chain combination.

Screenshots/Recordings

Before

After

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 · 2023-12-06T21:15:13Z

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 · 2023-12-11T19:27:33Z

Builds ready [fec59c1]

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 (1271 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	89	126	101	10	5
		domContentLoaded	9	25	16	3	2
		load	1088	1468	1271	78	37
		domInteractive	9	25	16	3	2

Bundle size diffs

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

codecov · 2023-12-11T19:27:48Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c6a2af2) 68.15% compared to head (eeccb97) 68.15%.
Report is 21 commits behind head on develop.

Files	Patch %	Lines
...nder-to-recipient/sender-to-recipient.component.js	84.62%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #22190      +/-   ##
===========================================
- Coverage    68.15%   68.15%   -0.00%     
===========================================
  Files         1081     1081              
  Lines        42346    42348       +2     
  Branches     11316    11314       -2     
===========================================
+ Hits         28858    28859       +1     
- Misses       13488    13489       +1

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

…me-display

metamaskbot · 2024-01-04T21:13:55Z

Builds ready [ab1f76c]

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 (1379 ± 126 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	91	437	181	89	43
		domContentLoaded	10	180	52	63	30
		load	950	1940	1379	263	126
		domInteractive	10	180	52	63	30

Bundle size diffs

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

OGPoyraz · 2024-01-08T11:23:11Z

test/e2e/flask/petnames-helpers.js

+  if (name) {
+    const input = await driver.findElement('.form-combo-field input');
+    await input.fill(name);
+    await input.press(driver.Key.ENTER);


For sure not related with this PR, but do you happen to know the intention of pressing "Enter" here?
I was wondering maybe this is not needed since we click "Save" afterwards.

Good observation. I removed it and the tests continue to pass.

It turns out, pressing enter before saving is needed for firefox to get the dropdown to go away. I re-added it along with a comment to explain why.

Thanks for digging out Derek!

matthewwalsh0 · 2024-01-09T20:50:19Z

ui/components/ui/sender-to-recipient/sender-to-recipient.component.js

 };

 export function RecipientWithAddress({
+  recipientAddress,


Do we need the non-normalized address? Does the Name component and hook not normalize any input?

Moved to #22515

matthewwalsh0 · 2024-01-09T20:53:53Z

ui/components/ui/sender-to-recipient/sender-to-recipient.component.js

+      recipientEns ||
+      shortenAddress(checksummedRecipientAddress);
+
+    if (addressOnly && !displayName) {


Looking at the original logic, is the newContract fallback used only when addressOnly is false?

Good catch! Done in #22515

matthewwalsh0 · 2024-01-09T20:55:38Z

ui/components/ui/sender-to-recipient/sender-to-recipient.component.js

    );
  }

+  let displayName;


Minor, but could we instead assign to a const using a ternary to avoid the uninitialised let?

Done in #22515

matthewwalsh0 · 2024-01-09T20:59:00Z

test/e2e/flask/petnames-transactions.spec.js

+  await driver.delay(3000);
+}
+
+describe('Petnames', function () {


Should we use alternate names here to distinguish the two sets of tests, maybe Petnames - Signatures and Petnames - Transactions?

Done in #22515

matthewwalsh0 · 2024-01-09T21:05:40Z

test/e2e/flask/petnames-transactions.spec.js

+        await createTransactionRequest(driver);
+        await switchToNotificationWindow(driver, 3);
+        await expectName(driver, '0x0c54F...7AaFb', false);
+        await saveName(driver, '0x0c54F...7AaFb', 'test.lens', undefined);


In the signature tests, there were multiple addresses per confirmation, so for some we used a proposed name, and for one we saved a custom name.

Here we are exclusively using a proposed name so there is an argument that we could have a separate test for saving a transaction with a custom name, though it's low risk since we know it's fundamentally the same underlying component.

Equally we know that most transaction types ultimately use the sender-to-recipient component we've updated but for the sake of coverage, we could also add additional tests for different transaction types such as send transactions from within the wallet, or an approve transaction which uses some alternate components.

Moved to #22515

snapshot, initial pass

ae07d8e

dbrans force-pushed the dbrans/petnames-name-display branch 2 times, most recently from 1f3852f to ae07d8e Compare December 11, 2023 18:43

dbrans added 2 commits December 11, 2023 13:50

snapshot

2d71a85

snapshot

fec59c1

Merge remote-tracking branch 'origin/develop' into dbrans/petnames-na…

499686b

…me-display

dbrans added the team-confirmations-system-deprecated DEPRECATED: please use "team-confirmations" instead label Dec 12, 2023

Merge remote-tracking branch 'origin/develop' into dbrans/petnames-na…

ab1f76c

…me-display

dbrans added 5 commits January 5, 2024 15:03

Merge branch 'develop' into dbrans/petnames-name-display

1e1a411

fixed e2e test broken by change.

b39545a

renamed petnames.spec.js -> petnames-signatures.spec.js

1e46df5

Added e2e test for petnames in transactions

c6f787b

revert test/e2e/tests/send-eth.spec.js

1e9b595

dbrans marked this pull request as ready for review January 8, 2024 03:58

dbrans requested a review from a team as a code owner January 8, 2024 03:58

dbrans changed the title ~~Use name component to display transaction destination~~ Add Petnames Name Component to Transaction Approval Screen Jan 8, 2024

OGPoyraz reviewed Jan 8, 2024

View reviewed changes

OGPoyraz previously approved these changes Jan 8, 2024

View reviewed changes

lint fix

c966e18

dbrans dismissed OGPoyraz’s stale review via c966e18 January 8, 2024 12:59

dbrans added 2 commits January 8, 2024 08:01

Merge branch 'develop' into dbrans/petnames-name-display

72ba9bd

remove ENTER press when saving petname

81637c9

OGPoyraz previously approved these changes Jan 8, 2024

View reviewed changes

Added ENTER press back for firefox

c4cb219

dbrans dismissed OGPoyraz’s stale review via c4cb219 January 8, 2024 18:29

Merge branch 'develop' into dbrans/petnames-name-display

eeccb97

OGPoyraz approved these changes Jan 9, 2024

View reviewed changes

vinistevam approved these changes Jan 9, 2024

View reviewed changes

dbrans merged commit de85de5 into develop Jan 9, 2024

dbrans deleted the dbrans/petnames-name-display branch January 9, 2024 20:03

github-actions bot locked and limited conversation to collaborators Jan 9, 2024

metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Jan 9, 2024

matthewwalsh0 reviewed Jan 9, 2024

View reviewed changes

Conversation

dbrans commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Currently

Goal of this PR

Notes

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Dec 6, 2023

Uh oh!

metamaskbot commented Dec 11, 2023

Uh oh!

codecov bot commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Jan 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!

Choose a reason for hiding this comment

Uh oh!

matthewwalsh0 Jan 9, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dbrans commented Dec 6, 2023 •

edited

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading

matthewwalsh0 Jan 9, 2024 •

edited

Loading