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

Automatically set walletdb.spv according to client #621

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

OrfeasLitos
Copy link
Contributor

This PR obviates the need for manually setting the 'spv' property of walletdb when the 'client' property is provided. The property is set to the same value as the client node. The original, manual way of setting the spv property is maintained for backwards compatibility, for walletdbs without a client and for manually overriding the client mode.

@codecov-io
Copy link

codecov-io commented Oct 17, 2018

Codecov Report

Merging #621 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
+ Coverage   53.32%   53.53%   +0.21%     
==========================================
  Files         104      109       +5     
  Lines       27666    27955     +289     
  Branches     4737     4748      +11     
==========================================
+ Hits        14752    14965     +213     
- Misses      12914    12990      +76
Impacted Files Coverage Δ
lib/wallet/walletdb.js 75.78% <100%> (+0.3%) ⬆️
lib/node/spvnode.js 48.11% <0%> (ø)
lib/wallet/index.js 100% <0%> (ø)
lib/wallet/node.js 18.42% <0%> (ø)
lib/bcoin.js 97.26% <0%> (ø)
lib/wallet/client.js 13.2% <0%> (ø)
lib/blockchain/chain.js 60.83% <0%> (+0.27%) ⬆️
lib/net/hostlist.js 31.04% <0%> (+0.27%) ⬆️
lib/node/http.js 39.64% <0%> (+0.5%) ⬆️
... and 3 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 125e818...ce4bf1d. Read the comment docs.

@nodech
Copy link
Member

nodech commented Oct 17, 2018

This will crash on most common use case: WalletClient where wallet node runs separately, because WalletClient wont have node.

@OrfeasLitos
Copy link
Contributor Author

It now checks whether the node exists before touching its spv filter.

Obviate the need for manually setting the 'spv' property
of walletdb when the 'client' property is provided
@pinheadmz pinheadmz added enhancement Improving a current feature tests Has tests for code change is just an addtional test labels Jan 23, 2019
@braydonf braydonf added the wallet Wallet related label Feb 6, 2019
@pinheadmz
Copy link
Member

#532 Might be a better solution to this issue - it covers all three wallet/node configuration (plugin, separate process and testing suite), and in all cases chain.spv (bool) is accessible. We could add a line to #532 that sets walletDB.spv based on that client info, if no options.spv is set...

@OrfeasLitos
Copy link
Contributor Author

Sounds good. Should I redo this PR against your PR so that we keep the regression test as well?

@pinheadmz
Copy link
Member

Hm yeah, I have one more update I need to do on that PR, which will include a spv-test.js hopefully we can get that merged, then follow up with your feature, and merge your test into that one. Will probably have that ready in a few days

@OrfeasLitos
Copy link
Contributor Author

Sounds good. Also take a look at #417, I've written an spvnode-test.js. It may be of help for your test.

@pinheadmz
Copy link
Member

@OrfeasLitos Ok #749 is a cleaner PR that just exposes prune and spv properties to the wallet, whether it's running as a plugin or standalone. Only problem is the info is unavailable until walletDB.open() gets the details from chain (it has to make an API call to the node server if running in standalone mode) but I think you could insert your check in open() as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving a current feature tests Has tests for code change is just an addtional test wallet Wallet related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants