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

fix: add base58 and fix bech32 for is btc address #1548

Merged
merged 1 commit into from
Dec 9, 2020

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.

@@ -1,9 +1,14 @@
import assertString from './util/assertString';

// 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