Add absolute fee rate convenience functions to FeeRate
#1947
Conversation
Make the docs use the same form as everywhere else, which also mimics stdlib docs on integer types.
tcharding
changed the title
FeeRateFeeRate
bitcoin/src/blockdata/fee_rate.rs
Outdated
| /// `None` if overflow occurred. | ||
| /// | ||
| /// This is equivalent to `Self::checked_mul_by_weight()`. | ||
| pub fn absolute_fee_wu(self, weight: Weight) -> Option<Amount> { |
There was a problem hiding this comment.
why not just use checked_mul_by_weight()?
There was a problem hiding this comment.
Also, why absolute fee instead of fee_wu? if the name checked_mul_by_weight is confusing, maybe just rename that function instead of adding this wrapper function?
There was a problem hiding this comment.
Personally I like the absolute_fee_wu name, I've always found bdk's fee_wu to be quite confusing. I don't have any strong opinion on renaming vs adding a wrapper.
There was a problem hiding this comment.
What does the word absolute mean in the context of fee? Is there such a thing as a non-absolute fee?
There was a problem hiding this comment.
I dug around math and english definitions of "absolute" and, contrary to my gut feeling, it does not add any additional meaning although it was also [mis]used in this Bitocin Core PR.
I've removed the absolute_ prefix.
bitcoin/src/blockdata/fee_rate.rs
Outdated
| /// | ||
| /// This is equivalent to converting `vb` to `weight` using `Weight::from_vb` and then calling | ||
| /// `Self::checked_mul_by_weight(weight)`. | ||
| pub fn absolute_fee_vb(self, vb: u64) -> Option<Amount> { |
There was a problem hiding this comment.
an additional test would be nice to go along with this.
bitcoin/src/blockdata/fee_rate.rs
Outdated
| /// `None` if overflow occurred. | ||
| /// | ||
| /// This is equivalent to `Self::checked_mul_by_weight()`. | ||
| pub fn absolute_fee_wu(self, weight: Weight) -> Option<Amount> { |
There was a problem hiding this comment.
Personally I like the absolute_fee_wu name, I've always found bdk's fee_wu to be quite confusing. I don't have any strong opinion on renaming vs adding a wrapper.
tcharding
force-pushed
the
07-20-absolute-fee
branch
from
89c8119
to
39f28b9
Compare
tcharding
force-pushed
the
07-20-absolute-fee
branch
from
39f28b9
to
5a469be
Compare
|
Changes in force push:
|
Calculating the absolute fee from a fee rate can currently be achieved by creating a `Weight` object and using `FeeRate::checked_mul_by_weight`. This is kind of hard to discover, we can add two public convenience functions that make discovery of the functionality easier. Add two functions for calculating the absolute fee by multiplying by weight units (`Weight`) and virtual bytes (by first converting to weight units).
tcharding
force-pushed
the
07-20-absolute-fee
branch
from
5a469be
to
9eff2f2
Compare
| @@ -199,4 +225,20 @@ mod tests { | |||
| let fee_rate = FeeRate(10).checked_div(0); | |||
| assert!(fee_rate.is_none()); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn fee_convenience_functions_agree() { | |||
There was a problem hiding this comment.
this test could be boilled down to like two lines:
fn fee_convenience_functions_agree() {
let rate = FeeRate::from_sat_per_vb(1).expect("1 sat/byte is valid");
assert_eq!(rate.fee_vb(1), rate.fee_wu(Weight::from_wu(4)));
}
after all, no need to test all of the transaction serialization stuff again here.
|
Removed the @ mention from the PR description because it's likely to cause more noise than intended. |
|
Oh I thought it only spammed if in the commit not in the PR? |
|
@tcharding the merge script copies the PR message into the commit. |
|
Oh of course. Thanks. |
FeeRateFrom the commit log of patch 2:
This seems like an obvious thing so I'm inclined to think that Kixunil left it out for a reason. (Mentioning you here Kix so even if this merges you'll see it in notifications later on.)