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

jellyfish-wallet should provide non-hd WalletNode by default #453

Merged
merged 3 commits into from
Jul 9, 2021

Conversation

fuxingloh
Copy link
Contributor

What kind of PR is this?:

/kind refactor

What this PR does / why we need it:

To allow non-HdWallet use cases and implementations as not all wallet implementations are HD, we should not enforce HD Nodes.

@defichain-bot defichain-bot added the kind/refactor Non feature refactor change label Jul 9, 2021
@codeclimate
Copy link

codeclimate bot commented Jul 9, 2021

Code Climate has analyzed commit 512e53f and detected 0 issues on this pull request.

View more on Code Climate.

@github-actions
Copy link

github-actions bot commented Jul 9, 2021

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/jellyfish/dist/index.umd.js 21.3 KB (0%) 426 ms (0%) 169 ms (-0.54% 🔽) 595 ms

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #453 (512e53f) into main (00986ae) will increase coverage by 4.50%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #453      +/-   ##
==========================================
+ Coverage   92.98%   97.48%   +4.50%     
==========================================
  Files          10       98      +88     
  Lines         171     2904    +2733     
  Branches       19      288     +269     
==========================================
+ Hits          159     2831    +2672     
- Misses         12       73      +61     
Impacted Files Coverage Δ
...ackages/jellyfish-transaction-builder/src/index.ts 100.00% <ø> (ø)
...llyfish-transaction-builder/src/txn/txn_builder.ts 96.15% <ø> (ø)
...transaction-builder/src/txn/txn_builder_account.ts 100.00% <ø> (ø)
...ish-transaction-builder/src/txn/txn_builder_dex.ts 100.00% <ø> (ø)
...h-transaction-builder/src/txn/txn_builder_error.ts 100.00% <ø> (ø)
...ransaction-builder/src/txn/txn_builder_liq_pool.ts 100.00% <ø> (ø)
...transaction-builder/src/txn/txn_builder_oracles.ts 100.00% <ø> (ø)
...sh-transaction-builder/src/txn/txn_builder_utxo.ts 100.00% <ø> (ø)
...s/jellyfish-transaction-builder/src/txn/txn_fee.ts 100.00% <ø> (ø)
...kages/jellyfish-transaction-signature/src/index.ts 100.00% <ø> (ø)
... and 144 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 b39bb0f...512e53f. Read the comment docs.

@fuxingloh fuxingloh marked this pull request as ready for review July 9, 2021 07:09
@fuxingloh fuxingloh requested a review from monstrobishi July 9, 2021 07:10
@fuxingloh fuxingloh changed the title jellyfish-wallet should provde non-hd WalletNode by default jellyfish-wallet should provide non-hd WalletNode by default Jul 9, 2021
@monstrobishi
Copy link
Contributor

Since we are working on naming here, I'm not sure if I understand the term non-hd wallet node, as the term HD node is very specific to HD wallets?

@monstrobishi
Copy link
Contributor

monstrobishi commented Jul 9, 2021

https://arshbot.medium.com/hd-wallets-explained-from-high-level-to-nuts-and-bolts-9a41545f5b0

Quote:
Technically speaking, Hierarchical Deterministic Wallets are a tree structure where each node has an extended private and public key. Any node can have any number of children. Meaning a master key could regenerate 10 accounts in 10 different currencies each with a very high number of addresses. I chose 10 as an arbitrary number, the number could be as large as you’d like.

Imo it's either an HD Node or a key pair, a node without the tree is no longer a node

@monstrobishi
Copy link
Contributor

Perhaps call it WalletEllipticPair or ExtendedEllipticPair

@fuxingloh
Copy link
Contributor Author

Perhaps call it WalletEllipticPair or ExtendedEllipticPair

Make sense, let's call it WalletEllipticPair? can you make the change?

@monstrobishi
Copy link
Contributor

Sure

Copy link
Contributor

@ivan-zynesis ivan-zynesis left a comment

Choose a reason for hiding this comment

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

Looking at the implementation again. WalletHdNode do not really make sense, as how it can perform direct getScript() and getAddress()

multiple node derivation will be inside HdWalletNodeProvider<WalletNode>, maybe can just remove WalletHdNode entirely.

packages/jellyfish-wallet/src/wallet_account.ts Outdated Show resolved Hide resolved
packages/jellyfish-wallet/src/wallet_account.ts Outdated Show resolved Hide resolved
@fuxingloh
Copy link
Contributor Author

looks good, let's get this merged and we can create a release to sync whale

@fuxingloh fuxingloh merged commit dc897c7 into main Jul 9, 2021
@fuxingloh fuxingloh deleted the fuxingloh/wallet-should-be-non-hd branch July 9, 2021 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/workflow kind/refactor Non feature refactor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants