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-http: create account: remove unused 'watch-only' #637

Closed
wants to merge 1 commit into from

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Nov 21, 2018

When a new account is created, it inherits its watch-only property from the wallet that contains it. By removing this check, user will get "unrecognized option" error if they try to create an account with watch-only. Currently the parameter is just ignored and could cause user confusion.

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.

@pinheadmz pinheadmz added the quick Can be fixed quickly, code change less than 10 lines label Jan 23, 2019
@braydonf braydonf added watch-only Watch only wallet wallet Wallet related labels Feb 6, 2019
@braydonf
Copy link
Contributor

braydonf commented Feb 7, 2019

So in testing, I didn't find a difference in behavior.

Using the example at #688 with the addition of:

const res = await client.request('PUT', '/wallet/:id/account/:account', {
  accountKey: xpub,
  watchOnly: false,
  type: 'pubkeyhash'
});

The response gave watchOnly: true for the account in that case.

@pinheadmz
Copy link
Member Author

pinheadmz commented Feb 7, 2019

I think this makes more sense in the context of this bclient PR: bcoin-org/bclient#15 and #639

Those PRs actually throw errors, this one just cleans up an unused line of code (exactly how you described -- the example creates a watchonly wallet, so all its accounts will be watchonly, regardless of the option you pass)

not sure what I meant in comment 1 about feedback to the user -crossing out!

@braydonf
Copy link
Contributor

braydonf commented Feb 8, 2019

The commit message still has the now crossed out line, fyi.

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.

Looks good.

@codecov-io
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #637 into master will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #637      +/-   ##
==========================================
+ Coverage   53.17%   53.33%   +0.15%     
==========================================
  Files         104      104              
  Lines       27719    27676      -43     
  Branches     4749     4738      -11     
==========================================
+ Hits        14740    14760      +20     
+ Misses      12979    12916      -63
Impacted Files Coverage Δ
lib/wallet/http.js 44.4% <ø> (ø) ⬆️
lib/workers/child.js 66.66% <0%> (-1.24%) ⬇️
lib/mempool/mempool.js 43.42% <0%> (-1.11%) ⬇️
lib/primitives/keyring.js 69.97% <0%> (-1%) ⬇️
lib/node/rpc.js 23.38% <0%> (-0.25%) ⬇️
lib/blockchain/chaindb.js 56.78% <0%> (-0.1%) ⬇️
lib/blockchain/chain.js 60.55% <0%> (-0.08%) ⬇️
lib/net/pool.js 15.87% <0%> (-0.05%) ⬇️
lib/wallet/walletdb.js 75.47% <0%> (-0.03%) ⬇️
lib/wallet/txdb.js 79.88% <0%> (-0.02%) ⬇️
... and 5 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 35a25e0...9ecb19f. Read the comment docs.

@pinheadmz
Copy link
Member Author

@braydonf removed the comment from the commit message, guess thats from github web interface

@braydonf
Copy link
Contributor

braydonf commented Feb 9, 2019

Merged at bb1ce25

@braydonf braydonf closed this Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quick Can be fixed quickly, code change less than 10 lines wallet Wallet related watch-only Watch only wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants