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
feat: add further bitcoin::Amount
usage on public APIs
#1426
base: master
Are you sure you want to change the base?
feat: add further bitcoin::Amount
usage on public APIs
#1426
Conversation
fb4a337
to
b15d01d
Compare
@ValuedMammal Please let me know what are your thoughts on these changes. |
b15d01d
to
1f9728b
Compare
I did remove the updates where it's |
crates/bdk/src/wallet/tx_builder.rs
Outdated
/// The fee_absolute method refers to the absolute transaction fee in [`Amount`]. | ||
/// If anyone sets both the `fee_absolute` method and the `fee_rate` method, | ||
/// the `FeePolicy` enum will be set by whichever method was called last, | ||
/// as the [`FeeRate`] and `FeeAmount` are mutually exclusive. |
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.
Thanks for fixing this. Now that I'm looking at it, maybe referring to the "FeePolicy enum" is a bit too much detail, but we can state it more generally.
/// Set an absolute fee [`Amount`].
///
/// The `fee_absolute` method refers to the absolute transaction fee. If anyone sets both
/// this method and the [`fee_rate`] method, the fee policy for building the transaction
/// will be set by whichever method was called last, as the feerate and fee amount are
/// mutually exclusive.
///
/// Note that this is really a minimum absolute fee -- it's possible to
/// overshoot it slightly since adding a change output to drain the remaining
/// excess might not be viable.
///
/// [`fee_rate`]: Self::fee_rate
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.
I don't agree. I always think that these links are useful.
They are "hover" only and avoids you having to go through the codebase, both in an IDE/LSP and also in the Docs.
I would agree with you if only the thing we are referring is not public, which is not the case here
bdk/crates/bdk/src/wallet/tx_builder.rs
Lines 166 to 170 in 66abc73
#[derive(Debug, Clone, Copy)] | |
pub(crate) enum FeePolicy { | |
FeeRate(FeeRate), | |
FeeAmount(u64), | |
} |
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.
Thanks for fixing this. Now that I'm looking at it, maybe referring to the "FeePolicy enum" is a bit too much detail, but we can state it more generally.
I agree with Jose's point, and that's what I had in mind and also why I updated it.
It gives the ergonomic for the developer to know exactly which type is being used, and to navigate easily on the code, at least I use it a lot with rust-analyzer
(even navigating into the dependencies and so on).
[...]
I would agree with you if only the thing we are referring is not public, which is not the case here
Exactly, I updated it only for the ones that are explicitly public, the compiler does not even allow you to reference non-public APIs.
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.
I based this #1426 (comment) suggestion on the observation that FeePolicy
won't be visible to someone outside the crate reading the docs for example on docs.rs, which is why I think it doesn't belong here.
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.
Good point, but still for us and other BDK developers I think it helps to navigate the codebase through IDE tools 🤔.
Here's how I would approach this PR with fewer lines touched ValuedMammal/bdk@0c72958 However I think it's worth having a discussion about where we can continue to get benefits from the Amount type so I opened an issue #1432 -- edit: @oleonardolima Just checking, is the plan to go ahead and complete #1432 in this PR? |
8f0b685
to
80deac6
Compare
I think I can continue the work on non-public code (everywhere) on another PR, and keep this one only for the public APIs that remained from #1411. wdyt @ValuedMammal ? Also, I was waiting if some new discussion would occur on #1432 before working on it :) |
bitcoin::Amount
usagebitcoin::Amount
usage on public APIs
80deac6
to
6c572d7
Compare
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.
Thanks for working on this. There's a small typo in the changelog notice - FeePolicy::fee_absolute
should say TxBuilder::fee_absolute
.
…nd others - update to use `bitcoin::Amount` on `CreateTxError::FeeTooLow` variant. - update to use `bitcoin::Amount` on `Wallet::calculate_fee()`. - update to use `bitcoin::Amount` on `FeePolicy::fee_absolute()`. - update to use `bitcoin::SignedAmount` on `CalculateFeeError::NegativeFee` variant. - update to use `bitcoin::Amount` on `TxGraph::calculate_fee()`. - update to use `bitcoin::Amount` on `PsbUtils::fee_amount()`
6c572d7
to
5f2d6cb
Compare
Oh, thanks! Sorry for the oversight, fixed it. |
builds on top of #1411, further improves #823
Description
It further updates and adds the usage of
bitcoin::Amount
instead ofu64
.Notes to the reviewers
Open for comments and discussions.
Changelog notice
CreateTxError::FeeTooLow
to usebitcoin::Amount
.Wallet::calculate_fee()
. to usebitcoin::Amount
TxBuilder::fee_absolute()
. to usebitcoin::Amount
.CalculateFeeError::NegativeFee
to usebitcoin::SignedAmount
.TxGraph::calculate_fee()
. to usebitcoin::Amount
.PsbUtils::fee_amount()
to usebitcoin::Amount
.Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features: