-
-
Notifications
You must be signed in to change notification settings - Fork 902
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
feat(bitcoinAddress): Multiple bitcoin address types and testnet #2922
Conversation
✅ Deploy Preview for fakerjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2922 +/- ##
========================================
Coverage 99.96% 99.96%
========================================
Files 2986 2987 +1
Lines 215926 216037 +111
Branches 599 598 -1
========================================
+ Hits 215841 215952 +111
Misses 85 85
|
Co-authored-by: ST-DDT <[email protected]>
Co-authored-by: ST-DDT <[email protected]>
Co-authored-by: ST-DDT <[email protected]>
Co-authored-by: ST-DDT <[email protected]>
Co-authored-by: ST-DDT <[email protected]>
Co-authored-by: ST-DDT <[email protected]>
Co-authored-by: ST-DDT <[email protected]>
Co-authored-by: ST-DDT <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far! ❤️
Co-authored-by: DivisionByZero <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
Thanks for submitting suggestions folks, makes addressing comments a lot easier ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last questions and suggestions from my side.
Co-authored-by: DivisionByZero <[email protected]>
Co-authored-by: DivisionByZero <[email protected]>
Co-authored-by: DivisionByZero <[email protected]>
Co-authored-by: DivisionByZero <[email protected]>
Co-authored-by: DivisionByZero <[email protected]>
I'm somewhat confused by the lint error. IMO the current code looks correct to me. Maybe the two blocks need to be swapped? Like this one: Lines 39 to 40 in e6a2a9f
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review. It has been a pleasure to work with you.
The
faker.finance.bitcoinAddress()
only generateslegacy
andsegwit
addresses at random choice, and only formainnet
. In some cases the user might need to generate different types of addresses and use different networks. All of them are slightly different in terms of prefix, characters allowed and length.The different types of bitcoin addresses and their prefixes are explained in the following articles:
This PR introduces an options parameter that will allow the user to specify different types of address and also different networks. Examples:
Note: The current
faker.finance.bitcoinAddress
function generates bothlegacy
andsegwit
addresses, while in the current implementation with the type parameter, the default behavior is to generatelegacy
addresses only. Happy make some changes here to preserve the original behavior