ENS reverse resolution support #7177

whymarrh · 2019-09-17T19:35:33Z

This PR adds ENS Reverse Resolution support to the UI, showing ENS names in a few places where addresses are currently used (the confirm screen & tx activity log). This represents a step towards "level two" of ENS integration, as outlined in the ENS docs:^[1]

The second level of ENS integration involves displaying ENS names wherever your app currently displays addresses.

[...]

If a user entered an address, or the address was obtained from elsewhere, you may still be able to show an ENS name, by doing Reverse Resolution. This permits you to find the canonical name for an address and display that when possible. If no canonical name is provided, your application can fall back to displaying the address as it did previously.

By supporting reverse resolution, you make it easier for your users to identify accounts they interact with, associating them with a short human-readable name instead of a long opaque Ethereum address.

And, further, from Resolving Names § Reverse Resolution:^[2]

ENS does not enforce the accuracy of reverse records - for instance, anyone may claim that the name for their address is 'alice.eth'. To be certain that the claim is accurate, you must always perform a forward resolution for the returned name and check it matches the original address.

IDN homograph attack^[3]

As we're showing user-generated domain names in sensitive places in the UI, we need to prevent malicious users from tricking users by registering domains that use Unicode characters to look visually similar to other registered names (see also #1913).

This PR converts ENS domain names to Punycode via punycode.toASCII.

Tasks

Add tests for the ENS controller's reverse resolving functionality
Save punycode for ENS domains with Unicode characters (see also RFC 3492 and RFC 5891)
Update the confirm screen (screenshots below)
Update the transaction list details (screenshots below)

Confirm Screen Screenshots (`SenderToRecipient`)

Before, after, & tooltip

Transaction List Item Details (`TransactionListItemDetails`)

Before, after, & tooltip

Test addresses

I've registered peaksignal.eth for test purposes and have correctly configured it to enable reverse resolution.^[4]

> const ENS = require('ethjs-ens')
undefined
> const HttpProvider = require('ethjs-provider-http')
undefined
> const provider = new HttpProvider('https://ropsten.infura.io')
undefined
> const ens = new ENS({ provider, network: '3' })
undefined
> await ens.lookup('peaksignal.eth')
'0x8e5d75d60224ea0c33d0041e75de68b1c3cb6dd5'
undefined
> await ens.reverse('0x8e5d75d60224ea0c33d0041e75de68b1c3cb6dd5')
'peaksignal.eth'

whymarrh · 2019-10-28T16:15:23Z

This PR should be good to review now 😅

app/scripts/controllers/ens/index.js

danjm

QA'd and reviewed code. Tentatively approving in case the two comments I left don't require action. But if they require action, I'll approve again after they are addressed.

I think there are other place that we can show ENS names (on the send screen), but it may require design consideration. So we can handle those in another issue.

You may want to record a video of a demo of the feature implemented here for Bobby to review.

danjm · 2019-10-31T12:00:59Z

Recorded a video demo here: https://streamable.com/pevp4

app/scripts/controllers/ens/ens.js

app/scripts/controllers/ens/index.js

Gudahtt · 2019-10-31T13:47:05Z

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

@@ -116,8 +142,8 @@ export default class SenderToRecipient extends PureComponent {
            <span>{ addressOnly ? `${t('to')}: ` : '' }</span>
            {
              addressOnly
-                ? checksummedRecipientAddress
-                : (recipientNickname || recipientName || this.context.t('newContract'))
+                ? (recipientNickname || recipientEns || checksummedRecipientAddress)


Interesting; what is addressOnly supposed to do anyway? Using a nickname seems contrary to asking for the address to be used, but maybe I'm misunderstanding.

addressOnly is the flag passed to the component when this is rendered in the tx details

Right, but semantically what does it mean? "address only" implies to me that nicknames shouldn't be used, and you've just updated this to show the nickname in that case. If nicknames are desired, maybe the prop should be renamed? Or perhaps I'm misunderstanding the meaning.

Or maybe the intended meaning was "no icons", in which case this is fine 🤔

whymarrh · 2019-10-31T20:44:36Z

I think this is ready for another round of 👀

Gudahtt · 2019-10-31T22:05:04Z

app/scripts/controllers/ens/index.js

+    const domain = await this._ens.reverse(address)
+    const registeredAddress = await this._ens.lookup(domain)
+    if (registeredAddress === ZERO_ADDRESS || registeredAddress === ZERO_X_ERROR_ADDRESS) {
+      return undefined


Nit: it would be nice to know what happened in these cases though - e.g. whether the address didn't resolve, or whether it did but it couldn't be verified, etc.

Maybe a log.debug statement would be appropriate in each of these cases? With something like the error messages you had before.

Gudahtt

Looks good!

* ENS Reverse Resolution support * Save punycode for ENS domains with Unicode characters * Update SenderToRecipient recipientEns tooltip * Use cached results when reverse-resolving ENS names * Display ENS names in tx activity log

whymarrh force-pushed the ens-support-reverse-resolution branch 4 times, most recently from 74055a9 to 20b351e Compare September 18, 2019 23:14

whymarrh force-pushed the ens-support-reverse-resolution branch 5 times, most recently from bb7a388 to 1b094a5 Compare October 28, 2019 13:37

whymarrh marked this pull request as ready for review October 28, 2019 16:13

whymarrh requested review from danjm and Gudahtt as code owners October 28, 2019 16:13

whymarrh force-pushed the ens-support-reverse-resolution branch 3 times, most recently from 09d32a0 to 8f5db5c Compare October 30, 2019 17:53

danjm reviewed Oct 31, 2019

View reviewed changes

app/scripts/controllers/ens/index.js Outdated Show resolved Hide resolved

danjm reviewed Oct 31, 2019

View reviewed changes

app/scripts/controllers/ens/index.js Outdated Show resolved Hide resolved

danjm previously approved these changes Oct 31, 2019

View reviewed changes

Gudahtt reviewed Oct 31, 2019

View reviewed changes

whymarrh added 7 commits October 31, 2019 18:04

ENS Reverse Resolution support

e654f19

Save punycode for ENS domains with Unicode characters

d48a381

Clear EnsController store on network change

4e86b71

Use cached results when reverse-resolving ENS names

9cc1d32

Update SenderToRecipient recipientEns tooltip

dfcc465

Display ENS names in tx activity log

cf548b3

Always subscribe to network changes for EnsController

14d2b89

whymarrh dismissed danjm’s stale review via 14d2b89 October 31, 2019 20:39

whymarrh force-pushed the ens-support-reverse-resolution branch from 8f5db5c to 14d2b89 Compare October 31, 2019 20:39

whymarrh added 2 commits October 31, 2019 18:12

fixup! ENS Reverse Resolution support

05b2fff

Cleanup EnsController return values

37ba4d8

Gudahtt reviewed Oct 31, 2019

View reviewed changes

Gudahtt approved these changes Oct 31, 2019

View reviewed changes

danjm approved these changes Nov 1, 2019

View reviewed changes

whymarrh merged commit eed4a9e into MetaMask:develop Nov 1, 2019

whymarrh deleted the ens-support-reverse-resolution branch November 1, 2019 17:54

This was referenced Nov 17, 2019

Show reverse-resolved ENS names when they are detected MetaMask/Design#89

Closed

Show ENS domain in confirmation popup #5294

Closed

bbondy mentioned this pull request Dec 16, 2019

Rebase Crypto Wallets to MetaMask upstream brave/brave-browser#7408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENS reverse resolution support #7177

ENS reverse resolution support #7177

whymarrh commented Sep 17, 2019 •

edited

Loading

whymarrh commented Oct 28, 2019

danjm left a comment

danjm commented Oct 31, 2019

Gudahtt Oct 31, 2019

whymarrh Oct 31, 2019

Gudahtt Oct 31, 2019

Gudahtt Oct 31, 2019

whymarrh commented Oct 31, 2019

Gudahtt Oct 31, 2019 •

edited

Loading

Gudahtt left a comment

ENS reverse resolution support #7177

ENS reverse resolution support #7177

Conversation

whymarrh commented Sep 17, 2019 • edited Loading

IDN homograph attack[3]

Tasks

Confirm Screen Screenshots (SenderToRecipient)

Transaction List Item Details (TransactionListItemDetails)

Test addresses

whymarrh commented Oct 28, 2019

danjm left a comment

Choose a reason for hiding this comment

danjm commented Oct 31, 2019

Gudahtt Oct 31, 2019

Choose a reason for hiding this comment

whymarrh Oct 31, 2019

Choose a reason for hiding this comment

Gudahtt Oct 31, 2019

Choose a reason for hiding this comment

Gudahtt Oct 31, 2019

Choose a reason for hiding this comment

whymarrh commented Oct 31, 2019

Gudahtt Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

whymarrh commented Sep 17, 2019 •

edited

Loading

IDN homograph attack^[3]

Confirm Screen Screenshots (`SenderToRecipient`)

Transaction List Item Details (`TransactionListItemDetails`)

Gudahtt Oct 31, 2019 •

edited

Loading