Skip to content

Conversation

@ezkemboi
Copy link
Member

@ezkemboi ezkemboi commented Dec 8, 2020

fixes #1386

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #1548 (2975bfa) into master (302d295) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1548   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           99        99           
  Lines         1773      1776    +3     
=========================================
+ Hits          1773      1776    +3     
Impacted Files Coverage Δ
src/lib/isBtcAddress.js 100.00% <100.00%> (ø)
src/lib/isFQDN.js 100.00% <0.00%> (ø)
src/lib/isMobilePhone.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 302d295...2975bfa. Read the comment docs.

// supports Bech32 addresses
const btc = /^(bc1|[13])[a-zA-HJ-NP-Z0-9]{25,39}$/;
const bech32 = /^(bc1)[a-z0-9]{25,39}$/;
const base58 = /^(1|3)[A-HJ-NP-Za-km-z1-9]{25,39}$/;
Copy link
Member

Choose a reason for hiding this comment

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

We have isBase58 validator, how about re-using it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I only have an issue with the reuse since:

  • base58 will only be needed on A-HJ-NP-Za-km-z1-9 hence we will need to add (1|3) in front of it and check that base58 has 25-39 chars.
    Based on the above, I realized it will be hard to read and maintain the code

Copy link
Member

Choose a reason for hiding this comment

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

I see, makes sense. I'm good then.

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

@profnandaa profnandaa merged commit 787df19 into validatorjs:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IsBtcAddress not working with testnet addresses #639

2 participants