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

Add isEthereumAddress #1117

Merged
merged 3 commits into from
Feb 15, 2020

Conversation

hizkifw
Copy link
Contributor

@hizkifw hizkifw commented Oct 1, 2019

This PR adds basic validation for Ethereum addresses. Checksums are not validated to avoid dependencies.

$ npm run test

> validator@11.1.0 pretest /home/hizkifw/workspace/validator.js
> npm run lint && npm run build


> validator@11.1.0 lint /home/hizkifw/workspace/validator.js
> eslint src test


> validator@11.1.0 build /home/hizkifw/workspace/validator.js
> npm run build:browser && npm run build:node


> validator@11.1.0 build:browser /home/hizkifw/workspace/validator.js
> node --require @babel/register build-browser && npm run minify


> validator@11.1.0 minify /home/hizkifw/workspace/validator.js
> uglifyjs validator.js -o validator.min.js  --compress --mangle --comments /Copyright/


> validator@11.1.0 build:node /home/hizkifw/workspace/validator.js
> babel src -d .

Successfully compiled 81 files with Babel.

> validator@11.1.0 test /home/hizkifw/workspace/validator.js
> nyc mocha --require @babel/register --reporter dot



  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  162 passing (136ms)


=============================== Coverage summary ===============================
Statements   : 100% ( 1017/1017 )
Branches     : 100% ( 630/630 )
Functions    : 100% ( 96/96 )
Lines        : 100% ( 986/986 )
================================================================================

@ezkemboi
Copy link
Member

ezkemboi commented Oct 1, 2019

@hizkifw if I get some time, I will review this work.

Basic validation for Ethereum addresses
@@ -0,0 +1,8 @@
import assertString from './util/assertString';

const eth = /^(0x)[0-9a-f]{40}$/i;
Copy link
Member

Choose a reason for hiding this comment

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

I am just asking, don't we have any capitalize/uppercase for Ethereum Address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ethereum addresses can be either all-caps or all-lower, which is why I added the /i flag to mark the regex as case insensitive. Addresses with mixed casing (e.g. 0xdAC17F958D2ee523a2206206994597C13D831ec7) contain checksum information to make sure it's correct, but they require additional dependencies to compute and check.

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🎉

Just fix the merge conflicts and we should be ready to land.

@profnandaa profnandaa added ready-to-land For PRs that are reviewed and ready to be landed mc-to-land Just merge-conflict standing between the PR and landing. labels Feb 14, 2020
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

So many unrelated changes, please unstage.

es/index.js Outdated
@@ -23,6 +23,7 @@ import isFullWidth from './lib/isFullWidth';
import isHalfWidth from './lib/isHalfWidth';
import isVariableWidth from './lib/isVariableWidth';
import isMultibyte from './lib/isMultibyte';
import isSemVer from './lib/isSemVer';
Copy link
Member

Choose a reason for hiding this comment

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

remove the unrelated changes...

@profnandaa profnandaa merged commit 4f74f79 into validatorjs:master Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mc-to-land Just merge-conflict standing between the PR and landing. ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants