ENS - Stop inferring .eth TLD #1205

njgheorghita · 2019-01-14T11:26:13Z

What was wrong?

From Planned v5 changes

Stop inferring .eth as a suffix on ENS names.

Related to Issue #722

How was it fixed?

Updated label_to_name() util to raise an InvalidTLD exception if label was missing a TLD or used an unsupported TLD
Removed the option to guess_TLD in ens.address()

Cute Animal Picture

pipermerriam

could we update the use of RECOGNIZED_TLDS to be overridable?

Something like

# constants.py
DEFAULT_RECOGNIZED_TLDS = [...]

# ens/utils.py
def get_recognized_tlds():
    if 'ENS_RECOGNIZED_TLDS' in os.environ:
        return set(os.environ['ENS_RECOGNIZED_TLDS'].split(':'))
    else:
        return DEFAULT_RECOGNIZED_TLDS

Note that the code to pull the data from os.environ should be beefed up to have some better error handling and normalization so that it's a bit more forgiving about how they are formatted.

pipermerriam · 2019-01-14T15:22:32Z

ens/utils.py

    label = normalize_name(label)
    pieces = label.split('.')
    if pieces[-1] not in recognized_tlds:
-        pieces.append(default_tld)
+        raise InvalidTLD(
+            f"Label does not have a recognized TLD. TLD must match one of: {recognized_tlds}."


It'd be good to include both the label and the pieces[-1] in this message.

njgheorghita · 2019-01-14T17:06:24Z

ens/utils.py

+        override_tlds = os.environ['ENS_RECOGNIZED_TLDS'].split(':')
+        return set(DEFAULT_RECOGNIZED_TLDS + override_tlds)
+    else:
+        return DEFAULT_RECOGNIZED_TLDS


@pipermerriam How about this? I wasn't sure how much checking of the env variable to do. (eg. even if it's an empty string, everything should still work fine - and the user should be able to tell where they went wrong via the error msg). I went for also including the default TLDs even if the user sets the env variable, not sure if this should be avoided or not?

Two things

Maybe validate that all of the names are valid. I.E. I think it would be invalid to provide thing.something as a TLD since it has a . in it.

Can ENS actually work without any defined TLDs? If so, throwing an error if the specified TLD list is empty.

@pipermerriam Just saw this on the ens docs

Can I register a TLD of my own in the ENS?
No, TLDs are restricted to only .eth (on mainnet), or .eth and .test (on Ropsten), plus any special purpose TLDs such as those required to permit reverse lookups. There are no immediate plans to invite proposals for additional TLDs. In large part this is to reduce the risk of a namespace collision with the IANA DNS namespace.

Do we still want to allow users to set their own tlds?

Do we still want to allow users to set their own tlds?

For private networks I guess yes. Also for ENS team to test new TLDs. (I know Nick has used the library at least once, and requested changes) Also, that doc looks out of date. There is at least .xyz and I think another available now.

pipermerriam · 2019-01-14T17:24:22Z

ens/utils.py

+def get_recognized_tlds():
+    if 'ENS_RECOGNIZED_TLDS' in os.environ:
+        override_tlds = os.environ['ENS_RECOGNIZED_TLDS'].split(':')
+        return set(DEFAULT_RECOGNIZED_TLDS + override_tlds)


Hrm, I'm not sure these should be combined. If the user supplies their own it would be up to them to ensure they included all desired TLDs

carver · 2019-01-14T19:47:16Z

Actually... it's not clear to me why we need any hard-coding of TLDs. They are resolved like any other name (looked up as a subdomain of the root), so they will just work like any other name that can't be found.

The only reason that we kept that list before was so that we could do something like "laptop.jasoncarver" getting resolved to "laptop.jasoncarver.eth", since jasoncarver isn't in the list of TLDs. That's of limited use, and I'm happy to see that go away.

In fact, I think one of the key benefits of removing the "guessing" of the TLDs is that we get to remove any logic that treats the TLDs as special, simplifying several places in the code.

carver

As I read through, I'm even more convinced that the win of dropping the guess_tld option is that we stop treating TLDs as special, and we get to delete code. 🎉

carver · 2019-01-14T19:49:50Z

tests/ens/test_setup_name.py

@@ -127,7 +130,7 @@ def test_setup_name_unowned_exception(ens):

 def test_setup_name_unauthorized(ens, TEST_ADDRESS):
    with pytest.raises(UnauthorizedError):
-        ens.setup_name('root-owned-tld', TEST_ADDRESS)
+        ens.setup_name('root-owned-tld.eth', TEST_ADDRESS)


This test was originally attempting to set up a TLD, which would require owning the root. (Hence the name root-owned-tld)

Since the address sending the transaction doesn't own the root, it's supposed to fail.

carver · 2019-01-14T19:51:11Z

tests/ens/test_setup_name.py

+        'lots.of.subdomains.tester',
+    ),
+)
+def test_cannot_setup_name_with_missing_or_invalid_tld(ens, name):


So I think this whole test can go away if we stop treating TLDs special.

carver · 2019-01-14T19:51:44Z

tests/ens/test_setup_address.py

+        'lots.of.subdomains.tester',
+    ),
+)
+def test_set_address_raises_exception_with_invalid_or_missing_tld(ens, name, TEST_ADDRESS):


I think this test goes away, too.

carver · 2019-01-14T19:54:07Z

ens/utils.py

    return '.'.join(pieces)


 def dot_eth_name(label):
-    return label_to_name(label, 'eth', RECOGNIZED_TLDS)
+    recognized_tlds = get_recognized_tlds()


I don't think dot_eth_name should exist anymore. Looks like it can be replaced by normalize_name everywhere. Then this and label_to_name can be dropped entirely.

ens/main.py

carver · 2019-01-14T19:55:40Z

ens/exceptions.py

@@ -77,3 +77,10 @@ class UnderfundedBid(ValueError):
    as your intent to bid.
    '''
    pass
+
+
+class InvalidTLD(ValueError):


Since we stop treating TLDs special, we can drop this exception.

carver · 2019-01-14T19:56:23Z

ens/constants.py

@@ -9,6 +9,6 @@

 MIN_ETH_LABEL_LENGTH = 7

-RECOGNIZED_TLDS = ['eth', 'reverse', 'test', 'luxe', 'xyz']
+DEFAULT_RECOGNIZED_TLDS = ['eth', 'reverse', 'test', 'luxe', 'xyz']


can be dropped

carver · 2019-01-14T19:56:33Z

docs/ens_overview.rst

-
-    assert ns.address('jasoncarver') == eth_address
+    # ens.py only support names using one of these recognized TLDs
+    # ['eth', 'reverse', 'test', 'luxe', 'xyz']


can be dropped

njgheorghita · 2019-01-15T09:16:16Z

Thanks @carver & @pipermerriam for the review, made the changes. Ping for a final 👀

carver

Looks good, yay for delete-heavy PRs!

Just the one missing assertion caught my attention. Some naming suggestions, if you like.

carver · 2019-01-16T21:15:09Z

ens/utils.py

@@ -200,7 +185,7 @@ def dot_eth_namehash(name):
    :rtype: bytes
    :raises InvalidName: if ``name`` has invalid syntax
    '''
-    expanded_name = dot_eth_name(name)
+    expanded_name = normalize_name(name)


The name of dot_eth_namehash should also probably change (since it doesn't append dot_eth anymore). Maybe raw_name_to_hash?

The other similar method, name_to_hash, could be changed to normal_name_to_hash to help distinguish.

carver · 2019-01-16T21:17:46Z

tests/ens/test_setup_address.py

    namehash = Web3.toBytes(hexstr=namehash_hex)
    normal_name = ens.nameprep(full_name)
-    if ens.nameprep(name) == normal_name:
-        assert is_same_address(ens.address(name, guess_tld=False), TEST_ADDRESS)


I think just this assertion should stay in, omitting guess_tld of course.

carver · 2019-01-17T14:56:33Z

tests/ens/test_setup_address.py


    ens.setup_address(name, TEST_ADDRESS)
    assert is_same_address(ens.address(name), TEST_ADDRESS)

-    # check that .eth is only appended if guess_tld is True
    namehash = Web3.toBytes(hexstr=namehash_hex)
    normal_name = ens.nameprep(full_name)
    if ens.nameprep(name) == normal_name:


Can either drop this if test, or have an else: assert False, "all names must be full names" below.

Stop inferring .eth tld

922dfea

njgheorghita force-pushed the ens-stop-inferring-eth-tld branch from 2d8da1c to ec37813 Compare January 14, 2019 11:29

Remove guess_tld optional arg

fce2e20

njgheorghita force-pushed the ens-stop-inferring-eth-tld branch from ec37813 to fce2e20 Compare January 14, 2019 11:32

njgheorghita requested review from carver, kclowes and pipermerriam January 14, 2019 11:37

pipermerriam reviewed Jan 14, 2019

View reviewed changes

Enable additional tlds via environment variables for ens

11d06e2

njgheorghita force-pushed the ens-stop-inferring-eth-tld branch from 4750485 to 11d06e2 Compare January 14, 2019 17:02

njgheorghita commented Jan 14, 2019

View reviewed changes

pipermerriam reviewed Jan 14, 2019

View reviewed changes

carver reviewed Jan 14, 2019

View reviewed changes

njgheorghita force-pushed the ens-stop-inferring-eth-tld branch 2 times, most recently from e15dac3 to 9659998 Compare January 15, 2019 08:51

Remove auto-infer of ENS TLDs

1ab9472

njgheorghita force-pushed the ens-stop-inferring-eth-tld branch from 9659998 to 1ab9472 Compare January 15, 2019 08:54

pipermerriam approved these changes Jan 16, 2019

View reviewed changes

carver approved these changes Jan 16, 2019

View reviewed changes

Change dot_eth_namehash to raw_name_to_hash

903f646

njgheorghita merged commit 4f42885 into ethereum:master Jan 17, 2019

njgheorghita deleted the ens-stop-inferring-eth-tld branch January 17, 2019 14:11

njgheorghita mentioned this pull request Jan 17, 2019

Remove .DS_Store files #1215

Merged

carver reviewed Jan 17, 2019

View reviewed changes

njgheorghita mentioned this pull request Jan 17, 2019

Planned v5 changes #722

Closed

22 tasks

njgheorghita mentioned this pull request Apr 5, 2019

Remove all remaining guess_tld kwargs #1315

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENS - Stop inferring .eth TLD #1205

ENS - Stop inferring .eth TLD #1205

njgheorghita commented Jan 14, 2019

pipermerriam left a comment

pipermerriam Jan 14, 2019

njgheorghita Jan 14, 2019

pipermerriam Jan 14, 2019

njgheorghita Jan 14, 2019

carver Jan 14, 2019 •

edited

Loading

pipermerriam Jan 14, 2019

carver commented Jan 14, 2019

carver left a comment

carver Jan 14, 2019

carver Jan 14, 2019

carver Jan 14, 2019

carver Jan 14, 2019

carver Jan 14, 2019

carver Jan 14, 2019

carver Jan 14, 2019

njgheorghita commented Jan 15, 2019

carver left a comment

carver Jan 16, 2019

carver Jan 16, 2019

carver Jan 17, 2019

ENS - Stop inferring .eth TLD #1205

ENS - Stop inferring .eth TLD #1205

Conversation

njgheorghita commented Jan 14, 2019

What was wrong?

How was it fixed?

Cute Animal Picture

pipermerriam left a 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

carver Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver commented Jan 14, 2019

carver left a 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

Choose a reason for hiding this comment

njgheorghita commented Jan 15, 2019

carver left a 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

carver Jan 14, 2019 •

edited

Loading