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

Wallet: require public key for watchonly #639

Closed
wants to merge 4 commits into from

Conversation

pinheadmz
Copy link
Member

Addresses #636 in which a user creates private keys they could never access due to a wallet being watch-only.

This PR goes along with #637 and bcoin-org/bclient#15, maybe we can decide if we need all three safeties in place or which ones we do or don't need.

In this pull request, we prevent a watch-only wallet from ever generating a master (private) key which could lead to a user seeing addresses they can not spend from.

I think watchOnly should always require an accountKey. The reverse isn't necessarily required, for example in bclient, creating a wallet with accountKey automatically sets watchOnly to true. In bcoin wallet._createAccount(), accountKey is just ignored if watchOnly is false, but at least that doesn't lead to unrecoverable keys!

@codecov-io
Copy link

codecov-io commented Nov 23, 2018

Codecov Report

Merging #639 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #639      +/-   ##
=========================================
+ Coverage   53.18%   53.2%   +0.02%     
=========================================
  Files         104     104              
  Lines       27724   27724              
  Branches     4752    4752              
=========================================
+ Hits        14745   14751       +6     
+ Misses      12979   12973       -6
Impacted Files Coverage Δ
lib/wallet/wallet.js 66.29% <100%> (+0.55%) ⬆️
lib/hd/hd.js 57.89% <0%> (+1.75%) ⬆️

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 7c64fd8...0dc0ad4. Read the comment docs.

Copy link
Member

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

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

OK. Needs a deprecation warning and API docs documenting this backward-incompatible change.

@pinheadmz
Copy link
Member Author

pinheadmz commented Jan 4, 2019

Added note in changelog, will update API docs after merge

@pinheadmz pinheadmz added tests Has tests for code change is just an addtional test logs / errors Better feedback to user or prevent user error labels Jan 23, 2019
@braydonf braydonf added watch-only Watch only wallet wallet Wallet related labels Feb 6, 2019
Copy link
Member

@nodech nodech left a comment

Choose a reason for hiding this comment

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

LGTM +1

braydonf added a commit that referenced this pull request Feb 11, 2019
Wallet: require public key for watchonly
@braydonf
Copy link
Contributor

Merged in 375965c

@braydonf braydonf closed this Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logs / errors Better feedback to user or prevent user error tests Has tests for code change is just an addtional test wallet Wallet related watch-only Watch only wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants