Update documentation for fee_rate and fee_absolute methods in tx_builder.rs #969
Conversation
nondiremanuel
added
documentation
jonmarrs
changed the title
crates/bdk/src/wallet/tx_builder.rs
Outdated
| /// If anyone sets both the fee_absolute method and the fee_rate method, | ||
| /// the fee_absolute value will take precedence over the fee_rate. |
There was a problem hiding this comment.
I don't think this is correct, the code looks like it will be which ever is set last.
There was a problem hiding this comment.
I think you're right. It looks like FeePolicy in an enum in which only one of either FeeRate or FeeAmount can be set at any time, as they are mutually exclusive.
I've updated the documentation for the fee_absolute() method to clarify this.
crates/bdk/src/wallet/tx_builder.rs
Outdated
| @@ -184,12 +184,16 @@ impl<'a, D, Cs: Clone, Ctx> Clone for TxBuilder<'a, D, Cs, Ctx> { | |||
| // methods supported by both contexts, for any CoinSelectionAlgorithm | |||
| impl<'a, D, Cs: CoinSelectionAlgorithm, Ctx: TxBuilderContext> TxBuilder<'a, D, Cs, Ctx> { | |||
| /// Set a custom fee rate | |||
| /// The fee_rate method refers to the fee rate in satoshis/vbyte (sats/vbyte). | |||
There was a problem hiding this comment.
This takes in a FeeRate which can represent sats/vbyte and sats/kw
There was a problem hiding this comment.
You're right. I see that sats/kwu is a possibility for FeeRate now after looking at the FeeRate type defined in types.rs.
I've updated the documentation for the fee_rate() method to reflect this. Sorry for the confusion, I am a new contributor and am still getting familiar with the code.
jonmarrs
changed the title
crates/bdk/src/wallet/tx_builder.rs
Outdated
| @@ -184,12 +184,18 @@ impl<'a, D, Cs: Clone, Ctx> Clone for TxBuilder<'a, D, Cs, Ctx> { | |||
| // methods supported by both contexts, for any CoinSelectionAlgorithm | |||
| impl<'a, D, Cs: CoinSelectionAlgorithm, Ctx: TxBuilderContext> TxBuilder<'a, D, Cs, Ctx> { | |||
| /// Set a custom fee rate | |||
| /// The fee_rate method sets the fee rate using a FeeRate type in either satoshis/vbyte (sats/vbyte) | |||
There was a problem hiding this comment.
I needed to use this method and came across your PR. I think it is useful to comment a little more how to set fee rates since it is always a source of confusion even for people familiar with Bitcoin, let alone newcomers.
That being said I'd be a little more verbose here, and write something like this:
/// Set the mining fee paid by the transaction as a rate on its size.
/// This means that the total fee paid is equal to this rate * size of the transaction in virtual Bytes (vB) or Weigth Unit (wu).
/// This rate is internally expressed in satoshis-per-virtual bytes (sats/vB), but can also be set by:
/// * sats/kvB (1000 sats/kvB == 1 sats/vB) using FeeRate::from_sat_per_kvb
/// * btc/kvB (0.00001000 btc/kvB == 1 sats/vB) using FeeRate::from_btc_per_kvb
/// * sats/kwu (250 sats/kwu == 1 sats/vB) using FeeRate::from_sat_per_kwu
/// Default is 1 sat/vB (see min_relay_fee)
But maybe that's too verbose now so just a suggestion
There was a problem hiding this comment.
I think these extra details are a good idea. I've updated the PR to include your suggestion.
|
Hey, can you please rebase on top of master? Thanks! |
|
Hi @danielabrozzoni, I've rebased on top of master. Thank you! |
|
Hey, can you please squash and sign your commits? |
jonmarrs
force-pushed
the
patch-2
branch
3 times, most recently
from
4f70b88 to
37cda69
Compare
|
Alright. I've rebased, squashed, and signed my commits. Sorry for the delay. |
|
No problems for the delay! Thanks for the squashing/rebasing, it looks good now, but the commit is still not signed. If you need help: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
Signed-off-by: Jon Marrs <[email protected]>
|
Sorry about that. The commit should be signed now. |
|
Assigned to milestone alpha.2 since it was merged in the last days |
Description
This pr helps solve this issue: #856
I added documentation to the fee_rate() method to describe the units as either satoshis/vbyte (sats/vbyte) or satoshis/kwu (sats/kwu), depending on the FeeRate type.
I also added documentation to the fee_absolute() method to clarify that the fee is determined by whichever method (fee_rate or fee_absolute) was called last, as the FeePolicy is an enum, and FeeRate/FeeAmount are mutually exclusive.
Notes to the reviewers
I thought it would be helpful to provide documentation to alleviate confusion over the fee_rate method and the fee_absolute method.
Checklists
All Submissions:
Bugfixes: