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: add set tx fee endpoint #518

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

Conversation

tuxcanfly
Copy link
Member

Refs #512

@tuxcanfly tuxcanfly added the WIP label Jul 5, 2018
@bucko13 bucko13 requested a review from BluSyn July 5, 2018 15:27
@BluSyn
Copy link
Collaborator

BluSyn commented Jul 11, 2018

Great! Would suggest also adding a GET /fee endpoint, since we are now making /fee a top-level resource, it makes it more "RESTful" + probably useful to see the current fee rate.

@tuxcanfly
Copy link
Member Author

tuxcanfly commented Jul 16, 2018

Needs tests. Guess we could top this off with a admin command from bwallet cli as well. Thoughts @BluSyn @bucko13 ?

@codecov-io
Copy link

codecov-io commented Jul 16, 2018

Codecov Report

Merging #518 into master will decrease coverage by 0.02%.
The diff coverage is 13.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
- Coverage   52.88%   52.86%   -0.03%     
==========================================
  Files         104      104              
  Lines       27700    27715      +15     
  Branches     4748     4750       +2     
==========================================
+ Hits        14649    14651       +2     
- Misses      13051    13064      +13
Impacted Files Coverage Δ
lib/wallet/http.js 43.8% <13.33%> (-0.76%) ⬇️

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 f24a9b6...d06cb13. Read the comment docs.

@bucko13
Copy link
Contributor

bucko13 commented Jul 16, 2018

Yeah, I think that would make sense to keep bclient in sync with the server.

@nodech
Copy link
Member

nodech commented Jul 20, 2018

Bcoin HTTP API for fees does not use floats. E.g. Create TX -- we are using Satoshis at HTTP level, same for fees.
bclient should do conversion, but not HTTP.

Note: This won't persist.

@tonydspaniard
Copy link

This will force a fixed fee for a transaction?

@BluSyn
Copy link
Collaborator

BluSyn commented Jul 24, 2018

This will force a fixed fee for a transaction?

Yes, however as nodar said:

Note: This won't persist.

I think we should add clear documentation about this.

  1. By default the fee is 'floating' (uses fee estimation)
  2. Setting the fee by http/rpc will make it 'fixed' for all future txs created
  3. This setting will not persist between bcoin restarts

@tuxcanfly is it possible to 'reset' the behavior to 'floating' (fee estimation) after setting a fee using http/rpc? Doesn't seem obvious this is possible. Currently when I set it from rpc I always have to restart bcoin to return to using estimation. Perhaps allow null to return to default behavior.

Would also be cool to expose the rpc estimatefee <nblocks> over http, but that's a totally separate feature request.

@braydonf
Copy link
Contributor

A configuration option could be a way to persist such fee rates, albiet it will require a restart.

Also setting the rate when creating each transaction could be another, with less side effects, way to have configuration here, if it's not already.

@nodech
Copy link
Member

nodech commented Oct 12, 2018

Probably best way would be to persist it within wallet database, even though it might need migration, I believe it can be implemented without migration as well (Don't crash when field is not there). You will have per wallet fee rates.

You can overwrite fee rates when creating transaction, it's there already.

@tynes
Copy link
Member

tynes commented Feb 4, 2019

Any updates on this PR? Seems like a candidate for getting merged

@braydonf braydonf added wallet Wallet related and removed WIP labels Feb 6, 2019
@pinheadmz
Copy link
Member

Needs rebase on master. Test would be nice too. And since the client is now integrated into the repo, we could quickly add setfeerate and getfeerate to the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wallet Wallet related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants