fix: Use `toUnicode` function to normalize ens domains in the UI #29231

OGPoyraz · 2024-12-16T12:37:40Z

Description

In ENSController(https://github.com/MetaMask/core/blob/main/packages/ens-controller/src/EnsController.ts#L375) right before saving the ens domain in the state we use toASCII. This function is typically used to convert a domain name from its Unicode representation to ASCII, specifically using the Punycode package encoding. This is necessary because the Domain Name System (DNS) operates with ASCII characters, and internationalized domain names (IDNs) need to be converted to a format that DNS can understand.

On the other side, in the client, we are not converting/normalizing this domain value. That is causing that unwanted ASCII coded domain in the UI when using smileys in the ENS domain.

Related issues

Fixes: #28610

Manual testing steps

See #28610 for repro steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

jpuri · 2024-12-16T13:26:05Z

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

@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import classnames from 'classnames';
 import copyToClipboard from 'copy-to-clipboard';
 import { NameType } from '@metamask/name-controller';
+import { toUnicode } from 'punycode';


punycode lib I think is deprecated, we should avoid using it

I don't think we should rely on any other package before removing the toASCII util from ENSController - these utils match each other since both using same encoding from punycode package

In that case, will be nice to mention that as a comment here.

Good call, added a comment.

jpuri · 2024-12-16T13:26:22Z

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

  const displayName =
    (recipientName ||
      recipientNickname ||
      recipientMetadataName ||
-      recipientEns ||
+      normalizedEnsRecipientName ||
      shortenAddress(checksummedRecipientAddress)) ??
    (!addressOnly && t('newContract'));



Will be useful to also test cover this change.

Since it's moved in to selector and we are not adding any test on them, I will skip and won't add it.

matthewwalsh0 · 2024-12-16T13:31:45Z

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

  const displayName =
    (recipientName ||
      recipientNickname ||
      recipientMetadataName ||
-      recipientEns ||
+      normalizedEnsRecipientName ||


Will we have this issue rendering ENS proposed names in petnames, and anywhere the ENS controller state is used?

Could we fix this deeper in the getEnsResolutionByAddress selector perhaps so we don't have to manually do this for each usage?

Make sense to do that casting on deeper, done.

…handling-of-emojis-ens-domains

metamaskbot · 2024-12-16T14:51:57Z

Builds ready [143deb9]

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, 6
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1589 ± 32 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1484	1691	1591	66	32
		domContentLoaded	1460	1678	1561	63	30
		load	1486	1690	1589	66	32
		domInteractive	23	92	37	20	9
		backgroundConnect	7	68	30	20	10
		firstReactRender	15	68	32	21	10
		getState	4	56	12	14	7
		initialActions	0	1	0	0	0
		loadScripts	1043	1293	1172	64	30
		setupStore	6	56	12	13	6
		uiStartup	1642	1911	1797	78	38

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

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

…handling-of-emojis-ens-domains

metamaskbot · 2024-12-20T08:04:20Z

Builds ready [d01b223]

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, 6
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1731 ± 66 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1530	2010	1731	135	65
		domContentLoaded	1518	1989	1705	132	63
		load	1532	2010	1731	137	66
		domInteractive	26	76	38	14	7
		backgroundConnect	9	59	27	17	8
		firstReactRender	16	103	56	33	16
		getState	5	84	20	25	12
		initialActions	0	0	0	0	0
		loadScripts	1122	1501	1281	103	49
		setupStore	7	75	21	24	11
		uiStartup	1752	2771	2143	300	144

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

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

Use toUnicode function to normalize ens addresses

06b89eb

OGPoyraz linked an issue Dec 16, 2024 that may be closed by this pull request

[Bug]: Send to ENS emojis domain - Wrong handling of emojis ENS domains #28610

Closed

metamaskbot added the team-confirmations Push issues to confirmations team label Dec 16, 2024

jpuri reviewed Dec 16, 2024

View reviewed changes

matthewwalsh0 requested changes Dec 16, 2024

View reviewed changes

OGPoyraz added 2 commits December 16, 2024 14:49

Add ens casting in the deeper level

bf16c20

Fix lint

c2bc8b5

OGPoyraz force-pushed the 28610-bug-send-to-ens-emojis-domain---wrong-handling-of-emojis-ens-domains branch from 19536a7 to c2bc8b5 Compare December 16, 2024 14:19

Merge branch 'main' into 28610-bug-send-to-ens-emojis-domain---wrong-…

143deb9

…handling-of-emojis-ens-domains

OGPoyraz enabled auto-merge December 18, 2024 07:33

OGPoyraz added 4 commits December 18, 2024 15:12

Add comment

825c14a

Merge branch 'main' into 28610-bug-send-to-ens-emojis-domain---wrong-…

232b845

…handling-of-emojis-ens-domains

Merge branch 'main' into 28610-bug-send-to-ens-emojis-domain---wrong-…

00d2693

…handling-of-emojis-ens-domains

Merge branch 'main' into 28610-bug-send-to-ens-emojis-domain---wrong-…

d01b223

…handling-of-emojis-ens-domains

jpuri approved these changes Dec 20, 2024

View reviewed changes

matthewwalsh0 approved these changes Dec 20, 2024

View reviewed changes

OGPoyraz added this pull request to the merge queue Dec 20, 2024

Merged via the queue into main with commit fd6e755 Dec 20, 2024
77 checks passed

OGPoyraz deleted the 28610-bug-send-to-ens-emojis-domain---wrong-handling-of-emojis-ens-domains branch December 20, 2024 10:10

github-actions bot locked and limited conversation to collaborators Dec 20, 2024

metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use `toUnicode` function to normalize ens domains in the UI #29231

fix: Use `toUnicode` function to normalize ens domains in the UI #29231

OGPoyraz commented Dec 16, 2024 •

edited

Loading

jpuri Dec 16, 2024

OGPoyraz Dec 16, 2024

jpuri Dec 16, 2024

OGPoyraz Dec 18, 2024

jpuri Dec 16, 2024

OGPoyraz Dec 18, 2024

matthewwalsh0 Dec 16, 2024

OGPoyraz Dec 16, 2024

metamaskbot commented Dec 16, 2024

metamaskbot commented Dec 20, 2024

fix: Use toUnicode function to normalize ens domains in the UI #29231

fix: Use toUnicode function to normalize ens domains in the UI #29231

Conversation

OGPoyraz commented Dec 16, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Dec 16, 2024

metamaskbot commented Dec 20, 2024

fix: Use `toUnicode` function to normalize ens domains in the UI #29231

fix: Use `toUnicode` function to normalize ens domains in the UI #29231

OGPoyraz commented Dec 16, 2024 •

edited

Loading