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.

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

Resolves confusing characters & warnings #3389

Merged
merged 1 commit into from
May 15, 2022

Conversation

danbr
Copy link
Contributor

@danbr danbr commented May 11, 2022

Description

This PR updates colonyDapp to use @colony/unicode-confusables-noascii. As the name suggests, the new package does not include ascii characters (numbers are still considered) in its ruleset.
Therefore, 'm' & 'l' will not be considered worthy of a warning.

Screenshot 2022-05-10 at 19 36 27

Screenshot 2022-05-10 at 19 36 33

A more detailed explanation, and the hidden part of this PR:

The package unicode-confusable has a script that at build time, takes the official unicode definitions (https://unicode.org/Public/security/10.0.0/confusables.txt) and extracts to confusables.json which is then distributed within the NPM and used as the source of its decisions of confusability.

Steps followed:

  • Forked unicode-confusable (currently to my github - need to move to colony)
  • Updated the script to ignore ascii characters from being included in confusables.json.
  • All other routines are untouched and remain the same as original package.
  • Published NPM '@colony/unicode-confusables-noascii'
  • Updated colonyDapp to use '@colony/unicode-confusables-noascii'

This approach should be more inline with Metamask and other users of confusable, and hopefully requires less maintenance in the future. Part of which can be 'piggybacked' from unicode-confusable

Resolves #3268

@danbr danbr added the bug label May 11, 2022
@danbr danbr requested a review from a team May 11, 2022 12:24
@danbr danbr self-assigned this May 11, 2022
Copy link
Contributor

@chinins chinins left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a comment

Choose a reason for hiding this comment

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

From what I could see, it's working great! Surely all the Marks and Ians in the World would appreciate this update

@danbr danbr force-pushed the fix/3268-confusables-no-ascii branch from 0bb6816 to 77390ea Compare May 15, 2022 18:00
@danbr danbr merged commit 55f5cd0 into feature/verified-recipients May 15, 2022
@danbr danbr deleted the fix/3268-confusables-no-ascii branch May 15, 2022 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants