Skip to content
This repository was archived by the owner on Jul 27, 2022. It is now read-only.

Problem: (CRO-518) HD and Basic wallet code is not separated out#532

Merged
bors[bot] merged 1 commit intocrypto-com:masterfrom
devashishdxt:cleanup
Oct 30, 2019
Merged

Problem: (CRO-518) HD and Basic wallet code is not separated out#532
bors[bot] merged 1 commit intocrypto-com:masterfrom
devashishdxt:cleanup

Conversation

@devashishdxt
Copy link
Copy Markdown
Contributor

@devashishdxt devashishdxt commented Oct 29, 2019

Solution: Separated HD wallet code in HdKeyService

Problem: (CRO-518) HD and Basic wallet code is not separated out
fdba9eb 
Solution: Separated HD wallet code in `HdKeyService`
leejw51crypto
leejw51crypto approved these changes Oct 29, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

good job

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 29, 2019 •
edited
Loading

Codecov Report

Merging #532 into master will decrease coverage by 0.21%.
The diff coverage is 73.07%.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage   67.37%   67.16%   -0.22%     
==========================================
  Files         120      122       +2     
  Lines       14077    14070       -7     
==========================================
- Hits         9485     9450      -35     
- Misses       4592     4620      +28
Impacted Files Coverage Δ
client-core/src/lib.rs 100% <ø> (ø) ⬆️
client-core/src/hd_wallet/key_chain.rs 86.45% <ø> (ø)
client-core/src/types/wallet_type.rs 0% <ø> (ø)
...lient-core/src/hd_wallet/extended_key/key_index.rs 70.96% <ø> (ø)
client-core/src/hd_wallet/extended_key.rs 91.97% <ø> (ø)
client-core/src/hd_wallet/key_chain/chain_path.rs 72.85% <ø> (ø)
client-cli/src/command/wallet_command.rs 0% <0%> (ø) ⬆️
client-core/src/hd_wallet/error.rs 0% <0%> (ø)
client-core/src/types/address_type.rs 0% <0%> (ø)
dev-utils/src/commands/init_command.rs 0% <0%> (ø) ⬆️
... and 17 more

calvinaco
calvinaco approved these changes Oct 29, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@calvinaco calvinaco left a comment

Choose a reason for hiding this comment

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

LG2M

tomtau
tomtau suggested changes Oct 29, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

I don't have enough context for this problem, but for the HD wallet code common for client-cli/rpc that can be exposed in a library, would it make sense to move it to client-common?

client-cli/src/command/wallet_command.rs
use client_common::{Error, ErrorKind, Result};
use client_core::WalletClient;
use quest::{ask, success};
use bip39::{Language, Mnemonic};
Copy link
Copy Markdown
Contributor

@tomtau tomtau Oct 29, 2019

Choose a reason for hiding this comment

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

should this functionality be here, as it'll be needed in client-rpc + if the client-* is exposed as a library?

Copy link
Copy Markdown
Contributor Author

@devashishdxt devashishdxt Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don’t think HD wallet code can be moved to client-common because it works with other parts in client-core. One possible change is to return SecUtf8 instead of Mnemonic when creating a new wallet so that we can remove bip39 dependency from client-cli and client-rpc.

@leejw51crypto Any comments?

Copy link
Copy Markdown
Contributor

@leejw51 leejw51 Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

changing to SecUtf8 makes sense

@tomtau
Copy link
Copy Markdown
Contributor

tomtau commented Oct 30, 2019

bors r+

bors bot added a commit that referenced this pull request Oct 30, 2019
@bors
Merge #532
f521849 
532: Problem: (CRO-518) HD and Basic wallet code is not separated out r=tomtau a=devashishdxt

Solution: Separated HD wallet code in `HdKeyService`

Co-authored-by: Devashish Dixit <devashish@crypto.com>
@leejw51
Copy link
Copy Markdown
Contributor

leejw51 commented Oct 30, 2019

bors is pending

@bors
Copy link
Copy Markdown
Contributor

bors bot commented Oct 30, 2019

Build succeeded

@bors bors bot merged commit fdba9eb into crypto-com:master Oct 30, 2019
@devashishdxt devashishdxt deleted the cleanup branch October 30, 2019 02:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lezzokafka lezzokafka Awaiting requested review from lezzokafka

@linfeng-crypto linfeng-crypto Awaiting requested review from linfeng-crypto

4 more reviewers

@leejw51 leejw51 leejw51 left review comments

@tomtau tomtau tomtau requested changes

@calvinaco calvinaco calvinaco approved these changes

@leejw51crypto leejw51crypto leejw51crypto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@devashishdxt @tomtau @leejw51 @calvinaco @leejw51crypto