feat: add further bitcoin::Amount usage on public APIs

[oleonardolima]

builds on top of #1411, further improves #823

Description

It further updates and adds the usage of bitcoin::Amount instead of u64.

Notes to the reviewers

Open for comments and discussions.

Changelog notice

• Updated CreateTxError::FeeTooLow to use bitcoin::Amount.
• Updated Wallet::calculate_fee(). to use bitcoin::Amount
• Updated TxBuilder::fee_absolute(). to use bitcoin::Amount.
• Updated CalculateFeeError::NegativeFee to use bitcoin::SignedAmount.
• Updated TxGraph::calculate_fee(). to use bitcoin::Amount.
• Updated PsbUtils::fee_amount() to use bitcoin::Amount.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

[oleonardolima]

@ValuedMammal Please let me know what are your thoughts on these changes.

[oleonardolima]

@ValuedMammal Please let me know what are your thoughts on these changes.

I did remove the updates where it's pub(crate) APIs, and kept only the ones for explicitly pub APIs.

[ValuedMammal]

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 

[storopoli]

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),
	}

[oleonardolima]

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.

[ValuedMammal]

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.

[oleonardolima]

Good point, but still for us and other BDK developers I think it helps to navigate the codebase through IDE tools 🤔.

[ValuedMammal]

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?

[oleonardolima]

[...]

--

edit: @oleonardolima Just checking, is the plan to go ahead and complete #1432 in this PR?

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 :)

[ValuedMammal]

Thanks for working on this. There's a small typo in the changelog notice - FeePolicy::fee_absolute should say TxBuilder::fee_absolute.

[oleonardolima]

Thanks for working on this. There's a small typo in the changelog notice - FeePolicy::fee_absolute should say TxBuilder::fee_absolute.

Oh, thanks! Sorry for the oversight, fixed it.

[notmandatory]

ACK 5f2d6cb

I made one small suggestion and I think you need to do a rebase. Otherwise looks ready to merge to me.

[oleonardolima]

ACK 5f2d6cb

I made one small suggestion and I think you need to do a rebase. Otherwise looks ready to merge to me.

EDIT: I applied the suggestion and rebased it on top of master at a03949a, should be ready to go now.

[storopoli]

ACK a03949a