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

Turn on ENSIP-15 by default #3024

Merged
merged 5 commits into from
Jul 10, 2023
Merged

Turn on ENSIP-15 by default #3024

merged 5 commits into from
Jul 10, 2023

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jul 5, 2023 •
edited
Loading

What was wrong?

  • ENSIP-15 is the new standard for normalizing and validating ENS names. If we don't turn this on by default we will be allowing invalid names to be created via web3.py. Though this is technically an upstream breaking change by ENS, it is in the best interest of our users to not be allowed to create invalid names in ENS. According to the standard, 99% of existing names are still valid under this new standard.

closes #3010

How was it fixed?

  • Turn on ENSIP-15 normalization by default, effectively reverting the ENSIP-15 flag changes before any release was made.
  • Remove / update newsfragments related to ENSIP-15 flag.
  • Keep tests that ensure that appropriate ENS utility methods and ENS / AsyncENS class methods call normalize_name_ensip15 as this is important to test for.
  • Update documentation to alert users of this possibly breaking change.

Todo:

  • Add entry to the release notes
  • Update and add appropriate documentation and notices
  • Update newsfragments

Cute Animal Picture

Screenshot 2023-07-10 at 14 12 14

@fselmo
Don't throw when using normalize_name w/ ENSIP-15 if name is empty
a1045aa 
- This causes methods that utilize normalize_name() internally to throw with a very obscure message that empty names aren't allowed. e.g. when calling ns.name(ens_address), if the reverse resolver isn't properly set up it will throw ``InvalidName`` with a message saying names can't be empty. This would create a lot of confusion.
- ``normalize_name_ensip15()`` method still raises ``InvalidName`` on empty name and this seems more appropriate when directly calling the method.
@fselmo fselmo force-pushed the ensip15-better-user-experience branch from c8f0761 to a1045aa Compare July 7, 2023 21:28
@fselmo
Revert ENSIP-15 flag and use it by default for web3.py
8ddd7ef 
- Remove the ENSIP-15 flags since we will be turning this on as default in ``v6``.
- Keep tests that assert the ``normalize_name_ensip15()`` method is called under the hood for utility methods, explicitly ensuring that we are normalizing names passed to those methods according to standard.
@fselmo fselmo changed the title [bugfix] ENSIP-15 better UX Turn on ENSIP-15 by default Jul 10, 2023
@fselmo fselmo force-pushed the ensip15-better-user-experience branch from 408a765 to dfbdaa7 Compare July 10, 2023 20:03
@fselmo fselmo force-pushed the ensip15-better-user-experience branch from dfbdaa7 to 85e272d Compare July 10, 2023 20:09
@fselmo fselmo force-pushed the ensip15-better-user-experience branch from 85e272d to d9766b7 Compare July 10, 2023 20:23
@fselmo fselmo marked this pull request as ready for review July 10, 2023 20:24
@fselmo fselmo requested review from reedsa, kclowes and pacrob July 10, 2023 20:27
pacrob
pacrob reviewed Jul 10, 2023
View reviewed changes
docs/ens_overview.rst Outdated Show resolved Hide resolved
pacrob
pacrob reviewed Jul 10, 2023
View reviewed changes
docs/index.rst Outdated Show resolved Hide resolved
pacrob
pacrob approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do with nits as you will, lgtm!

fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 10, 2023
@fselmo
Address comments on PR ethereum#3024
ee93d15
fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 10, 2023
@fselmo
Address comments on PR ethereum#3024
4212ffd
@fselmo fselmo force-pushed the ensip15-better-user-experience branch from ee93d15 to 4212ffd Compare July 10, 2023 21:11
reedsa
reedsa reviewed Jul 10, 2023
View reviewed changes
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking, thought of something we could add to add to the docs.

docs/ens_overview.rst

web3.py ``v6.1.0`` introduced ENS name normalization standard
`ENSIP-15 <https://docs.ens.domains/ens-improvement-proposals/ensip-15-normalization-standard>`_.
This update to ENS name validation and normalization won't affect ~99%
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a statement about the prior "art" of normalization in the ENS module. Not sure if it would help to call out specific cases that prompted the breaking change but that might be useful too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a statement about the prior "art" of normalization in the ENS module.

Any suggestion here? I'm not sure I understand.

Not sure if it would help to call out specific cases that prompted the breaking change but that might be useful too.

Not sure here either. I think the biggest driver was that there wasn't a properly defined standard to begin with, or at least clear lines drawn when it comes to complex names. I lean towards deferring to the proper ENS documentation for answering the why questions but if you think of a clarifying addition I'm happy to pop it in here 👀 .

Copy link
Contributor

@reedsa reedsa Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this doesn't make sense, feel free to leave it out. I was not following everything that was implemented prior to this release. Suggestion might be:

Prior to this version, ENS normalization used a ruleset that allowed invalid names.

What you have is basically the inverse, so I think people will get it with or without the history lesson.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, heard. Yeah I think that can be addressed in the proper ENS docs as long as we link out to it here. There's a motivation section in the ENSIP-15 documentation that I think addresses this pretty well.

kclowes
kclowes approved these changes Jul 10, 2023
View reviewed changes
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a small nit around the version this was introduced in in the docs.

docs/ens_overview.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@fselmo fselmo force-pushed the ensip15-better-user-experience branch from 4212ffd to 48cbe73 Compare July 10, 2023 21:53
@fselmo fselmo merged commit c552a96 into ethereum:main Jul 10, 2023
@fselmo fselmo deleted the ensip15-better-user-experience branch July 10, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kclowes kclowes kclowes approved these changes

@pacrob pacrob pacrob approved these changes

@reedsa reedsa reedsa approved these changes

Assignees
No one assigned
Labels
Breaking Change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use ENSIP-15 by default for ENS
4 participants
@fselmo @reedsa @pacrob @kclowes