refactoring jellyfish-address #302
Conversation
|
Code Climate has analyzed commit c624624 and detected 6 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
✔️ Deploy Preview for jellyfish-defi ready! 🔨 Explore the source changes: 779d119 🔍 Inspect the deploy log: https://app.netlify.com/sites/jellyfish-defi/deploys/60af5395a945c100086dfc97 😎 Browse the preview: https://deploy-preview-302--jellyfish-defi.netlify.app |
size-limit report 📦
|
Codecov Report
@@ Coverage Diff @@
## main #302 +/- ##
==========================================
+ Coverage 96.92% 97.31% +0.39%
==========================================
Files 89 89
Lines 2404 2425 +21
Branches 308 324 +16
==========================================
+ Hits 2330 2360 +30
+ Misses 74 65 -9
Continue to review full report at Codecov.
|
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| let valid: boolean | ||
| let prefix: string | ||
| let data: string = '' | ||
| try { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
There was a problem hiding this comment.
multiple "similar blocks" found
p2wsh similar to p2wpkh (bech32), however, refactoring them to parent class with require abstract methods like getDataLength() / getExpectedPrefixes()
| /** | ||
| * @param net to be validated against the decoded one from the raw address | ||
| * @param address raw human readable address (utf-8) | ||
| * @returns DefiAddress or a child class | ||
| */ |
There was a problem hiding this comment.
gonna update the jsdoc
| */ | ||
| static to (net: Network | NetworkName, h160: string, witnessVersion = 0x00): P2WPKH { | ||
| static to (net: Network | NetworkName, h160: string | Buffer, witnessVersion = 0x00): P2WPKH { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| */ | ||
| static to (net: Network | NetworkName, hex: string, witnessVersion = 0x00): P2WSH { | ||
| static to (net: Network | NetworkName, data: string | Buffer, witnessVersion = 0x00): P2WSH { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| * @throws when h160 input string is not 64 characters long (32 bytes) | ||
| * @returns {P2SH} | ||
| */ | ||
| static to (net: NetworkName | Network, h160: string | Buffer): P2SH { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| * @throws when h160 input string is not 40 characters long (20 bytes) | ||
| * @returns {P2WSH} | ||
| */ | ||
| static to (net: NetworkName | Network, h160: string | Buffer): P2PKH { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| try { | ||
| const decoded = Bs58.toHash160(utf8String) | ||
| buffer = decoded.buffer | ||
| network = [MainNet, TestNet, RegTest].find(net => net.scriptHashPrefix === decoded.prefix) | ||
| valid = network !== undefined | ||
| } catch { | ||
| // non b58 string, invalid address | ||
| } |
There was a problem hiding this comment.
try.. catch without handling??
There was a problem hiding this comment.
Intended.
const addressStr = takeUserInput()
const address = DeFiAddress.from(addressStr)
It try instantiate multiple types of address and return if any valid. Return one with valid flag false even none is possible. (eg typo or extra space character pasted)
|
this is still too complicated in my opinion, let keep it open for now |
* Bump @defichain/jellyfish dependencies * fixed deps resolution Co-authored-by: Fuxing Loh <[email protected]>
What kind of PR is this?:
/kind feature
What this PR does / why we need it:
better code coverage
less logical branches
Which issue(s) does this PR fixes?:
Fixes #
Additional comments?: