Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
seaona opened this issue Nov 21, 2024 · 0 comments · Fixed by #29231
Closed

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

seaona opened this issue Nov 21, 2024 · 0 comments · Fixed by #29231
Assignees
@OGPoyraz
Labels
regression-prod-12.6.2 Regression bug that was found in production in release 12.6.2 release-12.11.0 Issue or pull request that will be included in release 12.11.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug

Comments

@seaona
Copy link
Contributor

seaona commented Nov 21, 2024 •
edited
Loading

Describe the bug

Whenever we try to perform a send tx to an address with an ENS domain which uses emojis, like 😉️😉️😉️.eth with Nicknames disabled, we can see in 2 places how this is not correctly handled:

  • in the case of old transactions screens, we can see this value in the recipient xn-n28haa.eth for the last confirmation page
  • in both redesigned and non redesigned, after performing the transaction, we can see the recipient xn-n28haa.eth in the tx receipt

Expected behavior

I think for the old transaction screens, that could be left unfixed, since this will be fixed in the new redesigned screens.
For the transactions details, once that tx has been send, the recipient should be fixed.

Screenshots/Recordings

In the Transaction receipt, I can see the wrong "parsing" of that ENS domain:

Screenshot from 2024-11-21 13-25-35

In the non redesign transactions, I can also see the wrong parsing in the last Confirmation screen:

send-eth-emoji-ens.mp4

Steps to reproduce

  1. Disable allow nicknames from experimental settings
  2. Disable Improved Transaction Requests from experimental settings
  3. Start a send to this account `0x02d8B596130Fc2d8263d32F4f08a92d1761f2e86
  4. See how in the last confirmation page is displayed xn-n28haa.eth
  5. Enable Improved Transaction REquests
  6. Perform a complete send flow
  7. Wait until tx is confirmed
  8. Check tx receipt info --> see how recipient is xn-n28haa.eth

Error messages or log output

No response

Detection stage

In production (default)

Version

12.6.2

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Nov 21, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Nov 21, 2024
@seaona seaona added the Sev2-normal Normal severity; minor loss of service or inconvenience. label Nov 21, 2024
@metamaskbot metamaskbot added the regression-prod-12.6.2 Regression bug that was found in production in release 12.6.2 label Nov 21, 2024
@seaona seaona added the team-confirmations Push issues to confirmations team label Nov 21, 2024
@OGPoyraz OGPoyraz self-assigned this Dec 16, 2024
@OGPoyraz OGPoyraz mentioned this issue Dec 16, 2024
Merged
7 tasks
@OGPoyraz OGPoyraz linked a pull request Dec 16, 2024 that will close this issue
fix: Use toUnicode function to normalize ens domains in the UI #29231
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this issue Dec 20, 2024
@OGPoyraz
fix: Use toUnicode function to normalize ens domains in the UI (#29231
fd6e755 
)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->
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.


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29231?quickstart=1)

## **Related issues**

Fixes: #28610

## **Manual testing steps**

See #28610 for
repro steps

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

![Screenshot 2024-12-16 at 13 37
04](https://github.com/user-attachments/assets/62163ce0-007a-404b-8f2e-7a49eaa7b927)


### **After**

![Screenshot 2024-12-16 at 13 36
32](https://github.com/user-attachments/assets/cd287cb0-aa81-49da-aafb-1e753d8544e7)


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
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.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Dec 20, 2024
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@OGPoyraz OGPoyraz

Labels
regression-prod-12.6.2 Regression bug that was found in production in release 12.6.2 release-12.11.0 Issue or pull request that will be included in release 12.11.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug
Projects
Bugs by severity
Archived in project
Bugs by team
Status: Fixed
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Use toUnicode function to normalize ens domains in the UI MetaMask/metamask-extension
3 participants
@OGPoyraz @metamaskbot @seaona