-
Notifications
You must be signed in to change notification settings - Fork 61
New stuff on the FeeRate
type
#859
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
base: master
Are you sure you want to change the base?
New stuff on the FeeRate
type
#859
Conversation
033a48b
to
e3e6981
Compare
e3e6981
to
e152d68
Compare
|
||
// The whole code above should be replaceable by the following line: | ||
// self.0.fee_vb(vb).map(Arc::new(Amount::from)) | ||
// But in practice you get uniffi compilation errors on it. Not sure what is going on with it, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how weird, good comment for the future when hopefully it resolves itself
/// This is an integer type representing fee rate in sat/kwu. It provides protection against mixing | ||
/// up the types as well as basic formatting features. | ||
#[derive(Clone, Debug, uniffi::Object)] | ||
#[uniffi::export(Display)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
Thanks for adding all the code comments for docs |
// | ||
// This is equivalent to Self::checked_mul_by_weight(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
///
instead of //
for these two lines
|
||
impl Display for FeeRate { | ||
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
write!(f, "{} sat/kwu", self.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think of using "{:#}" because I think it forwards both the number and the unit decision to the FeeRate type from bitcoin-units (instead of hardcoding sat/kwu)? For now I think we get the same output with both, but maybe in the future if something weird changes it'll keep things aligned with the upstream.
So the idea is replacing write!(f, "{} sat/kwu", self.0)
with write!(f, "{:#}", self.0)
e152d68
to
7ea0162
Compare
This is adding 3 things to the
FeeRate
type.The first and 3rd ones are simple and totally cool. The display trait need a bit of discussion maybe.
Note that the new methods are cool/useful because if you have the weight or vbytes for a transaction, you can give this to the
FeeRate
type and get anAmount
that the transaction will pay in fee. I need to play with some of the advanced parts of the API a bit more, but I think you could basically predict the number of sats a transaction will pay before signing, which is pretty useful.The display trait
This one is me sort of freehanding what I think is the better visual display of the feerate, and I understand it's a bit of a subjective thing. Happy to hear what other people think. The rust-bitcoin FeeRate type defines actually 2 "display" versions (this is a fairly obscure Rust feature but I do think it's pretty cool). The standard display is just the number (sat/kwu), which is... kind of weird if you're not used to it. For example 1 sat/vbyte is just
250
if you use the standard display (println!("Fee rate: {}", feerate)
). If you use the alternative display, however (println!("Fee rate: {:#}", feerate)
), you get "1.00 sat/vb". This is a better and nicer string, but the "X.00" is hardcoded, and internally you just get theFeeRate::to_sat_per_vb_ceil
method. This means you have big plateaus of all the same sat/vb rate, even if at a more granual level you're sort of in between (1.09, 1.75, etc.). The decimals are always X.00. Instead, what I think makes the type interesting is its use of thekwu
, which affords you more granularity. The downside of this is that on the normal print/display, you're not reminded of that and just get a weird and big number. So in this PR... I decided to merge the two Rust implementation and ideas, and add the rate type to the string while keeping the sat/kwu number. This is the result:The new methods
This one has a tiny hiccup, more in style than anything, but I want to point it out. There appears to be a bug (as far as me and my friend Claude can tell it's at the uniffi level? that prevents us from returning an
Option<Amount>
on those two methods if I use the standard way to handle this. In theory, these should be very straightforward lines:In practice, if you try to do this you'll get compilation errors at the uniffi level. I've tried a whole lot of stuff, and I even put Claude on it for a while, and it just kept running in circles and finding the same problems as me. Some names are clashing in the imports and uniffi thinks returning an
Amount
is the same as aBdkAmount
, but of course it's not. In the end I can make it work by separating the one liner into a few lines with full type declaration.Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features: