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

guess P2SH / P2PKH / P2WSH / P2WPKH type #189

Merged
merged 38 commits into from
May 17, 2021

Conversation

ivan-zynesis
Copy link
Contributor

@ivan-zynesis ivan-zynesis commented May 6, 2021

What kind of PR is this?:

/kind feature

What this PR does / why we need it:

an Address library to check (user) input string is a valid address type and guess the type

Which issue(s) does this PR fixes?:

Fixes #

Additional comments?:

@ivan-zynesis ivan-zynesis requested a review from fuxingloh as a code owner May 6, 2021 05:41
@fuxingloh fuxingloh marked this pull request as draft May 6, 2021 05:43
@github-actions
Copy link

github-actions bot commented May 6, 2021

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/jellyfish/dist/jellyfish.umd.js 18.93 KB (0%) 379 ms (0%) 171 ms (-2.34% 🔽) 549 ms

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #189 (06ef04b) into main (7f906b1) will increase coverage by 0.00%.
The diff coverage is 96.25%.

❗ Current head 06ef04b differs from pull request most recent head b82f137. Consider uploading reports for the commit b82f137 to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##             main     #189    +/-   ##
========================================
  Coverage   96.21%   96.22%            
========================================
  Files          66       74     +8     
  Lines        1718     1958   +240     
  Branches      235      264    +29     
========================================
+ Hits         1653     1884   +231     
- Misses         65       74     +9     
Impacted Files Coverage Δ
packages/jellyfish-address/src/address.ts 84.00% <84.00%> (ø)
packages/jellyfish-address/src/p2sh.ts 90.90% <90.90%> (ø)
packages/jellyfish-address/src/base58_address.ts 93.93% <93.93%> (ø)
packages/jellyfish-address/src/index.ts 97.82% <97.82%> (ø)
packages/jellyfish-address/src/bech32_address.ts 100.00% <100.00%> (ø)
packages/jellyfish-address/src/p2pkh.ts 100.00% <100.00%> (ø)
packages/jellyfish-address/src/p2wpkh.ts 100.00% <100.00%> (ø)
packages/jellyfish-address/src/p2wsh.ts 100.00% <100.00%> (ø)
... and 6 more

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 7f906b1...b82f137. Read the comment docs.

@netlify
Copy link

netlify bot commented May 10, 2021

Deploy preview for jellyfish-defi ready!

Built with commit 9e78608

https://deploy-preview-189--jellyfish-defi.netlify.app

ivan added 3 commits May 11, 2021 09:33
…wn, added unit test for all 4 types of address (so far), base58 and bech32 address is extensible for other network use
@ivan-zynesis ivan-zynesis marked this pull request as ready for review May 11, 2021 07:40
@fuxingloh fuxingloh marked this pull request as draft May 12, 2021 05:13
@ivan-zynesis ivan-zynesis requested a review from fuxingloh May 12, 2021 05:39
packages/jellyfish-network/src/index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/jellyfish-address/src/index.ts Outdated Show resolved Hide resolved
@ivan-zynesis ivan-zynesis marked this pull request as ready for review May 12, 2021 06:25
@ivan-zynesis ivan-zynesis requested a review from fuxingloh May 17, 2021 01:40
@fuxingloh
Copy link
Contributor

rebase/merge main branch, I will check in abit

Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

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

https://github.com/DeFiCh/jellyfish/blob/main/CONTRIBUTING.md

I am going to refactor myself instead based on the contributing guidelines.

packages/jellyfish-address/src/index.ts Outdated Show resolved Hide resolved
fuxingloh and others added 5 commits May 17, 2021 22:53
* updated order of jellyfish-address

* rename file to lower snakecase

* rename test class

* fix and rename

* reorder packages in introduction.md
…ield

simply h160 to bs58 address construction, must specify address type (p2pkh or p2sh)
* updated order of jellyfish-address

* rename file to lower snakecase

* rename test class

* fix and rename

* reorder packages in introduction.md
Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

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

@@           Coverage Diff            @@
##             main     #189    +/-   ##
========================================
  Coverage   96.21%   96.22%            
========================================
  Files          66       74     +8     
  Lines        1718     1958   +240     
  Branches      235      264    +29     
========================================
+ Hits         1653     1884   +231     
- Misses         65       74     +9     

Mixed feelings about this PR. I would expect around <15 code logical branches for this ticket but there are +29 which is 10% more than the current combined mono repo code base.

#237 has only +7 for comparison and it has a much more complex implementation design. Clearly, there is much to optimize, the more code there is, the more there is to maintain. Less is more.

I would take it as it is now as it functionally works, but I think I will revisit and refactor this implementation in the future. I will create a ticket to refactor this at a future date.

@fuxingloh fuxingloh merged commit 1bfeab9 into main May 17, 2021
@fuxingloh fuxingloh deleted the feature/wallet-address-validator branch May 17, 2021 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants