Skip to content

Conversation

@kclowes
Copy link
Collaborator

@kclowes kclowes commented May 24, 2019

What was wrong?

web3py was doing full IDNA processing, instead of UTS46.

Related to Issue #1358 and #1357

How was it fixed?

Added some emoji and other test cases that are valid according to UTS46 but not IDNA2008 to make sure they are nameprepped correctly.

Cute Animal Picture

images

@kclowes kclowes requested a review from carver May 24, 2019 22:37
@kclowes
Copy link
Collaborator Author

kclowes commented May 24, 2019

@carver From what I can tell, this stills works how it was intended, but I would appreciate your 👀to be sure. And, are there other test cases I should consider?

Edit: shoulda waited until all the tests passed before I pinged you, sorry. Let me know if you think there are any test cases outside of the currently failing specs I should consider.

@kclowes kclowes changed the title Don't do full idna processing Use UTS46 domain name processing, not full IDNA processing May 24, 2019
@carver
Copy link
Contributor

carver commented May 24, 2019

Nothing more to add, except there is a lot of room to improve the test suite of nameprep. I wouldn't assume that green means that everything is great. It's worth looking around to find a nice full set of fixtures to test against.

@kclowes kclowes changed the title Use UTS46 domain name processing, not full IDNA processing [WIP] Use UTS46 domain name processing, not full IDNA processing May 29, 2019
@kclowes kclowes force-pushed the 1358-no-idna-processing branch from a25ad92 to 90af64d Compare July 10, 2019 22:32
@kclowes kclowes changed the title [WIP] Use UTS46 domain name processing, not full IDNA processing Use UTS46 domain name processing, not full IDNA processing Jul 10, 2019
@kclowes kclowes requested a review from pipermerriam July 10, 2019 22:46
@kclowes kclowes merged commit 2eccbff into ethereum:master Jul 11, 2019
@kclowes kclowes deleted the 1358-no-idna-processing branch July 11, 2019 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants