feat: add further `bitcoin::Amount` usage on public APIs by oleonardolima · Pull Request #1426 · bitcoindevkit/bdk

oleonardolima · 2024-05-05T21:21:29Z

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 · 2024-05-06T19:45:56Z

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

oleonardolima · 2024-05-07T19:23:00Z

@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.

crates/bdk/src/wallet/tx_builder.rs

ValuedMammal · 2024-05-08T14:45:46Z

crates/bdk/src/wallet/tx_builder.rs

+    /// 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.


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

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

}

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.

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.

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

crates/chain/src/tx_graph.rs

crates/bdk/src/wallet/mod.rs

ValuedMammal · 2024-05-08T15:53:44Z

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 · 2024-05-17T19:50:31Z

[...]

--

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.

crates/wallet/src/wallet/error.rs

oleonardolima · 2024-05-23T02:00:30Z

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.

crates/chain/src/tx_graph.rs

oleonardolima · 2024-06-03T14:13:05Z

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.

crates/electrum/tests/test_electrum.rs

…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()`

storopoli

ACK a03949a

20341a3 fix: typo on `SignedAmount` instead of `Amount` (Leonardo Lima) Pull request description:  ### Description It fixes the typo on the `expect()` message introduced on #1426, noticed at #1426 (comment)  ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: storopoli: ACK 20341a3 notmandatory: ACK 20341a3 Tree-SHA512: 62deb7308b2a6891eeb4598ebdf3c5ee1541fc52fd454bca2816b819bd7f6ab25c18e10c4166c80c4e553bcb3ce2207323376d260484a956837ac857854cbd6b

292ec3c refactor(wallet): use `Amount` everywhere (valued mammal) Pull request description: This is a followup to #1426 that refactors wallet internals to use `bitcoin::Amount` (nearly) everywhere. I chose not to change any public types in `coin_selection.rs` that still use `u64` as that might require more discussion. partially addresses #1432 fixes #1434 ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: oleonardolima: ACK 292ec3c notmandatory: ACK 292ec3c Tree-SHA512: e84c543e796e151803321ad238023bd5f446448b4430dd6c62929180d159ee1ef867e98f69a4ef3b152c3146b8e30bbf01ce7952ac00b726847a224dca7e3be4

oleonardolima mentioned this pull request May 5, 2024

feat: update keychain::Balance to use bitcoin::Amount #1411

Merged

8 tasks

notmandatory assigned oleonardolima May 5, 2024

notmandatory added api A breaking API change module-blockchain module-wallet labels May 5, 2024

notmandatory added this to the 1.0.0-alpha milestone May 5, 2024

oleonardolima force-pushed the feat/add-further-amount-usage branch 3 times, most recently from fb4a337 to b15d01d Compare May 6, 2024 19:35

oleonardolima marked this pull request as ready for review May 6, 2024 19:45

oleonardolima force-pushed the feat/add-further-amount-usage branch from b15d01d to 1f9728b Compare May 7, 2024 19:22

ValuedMammal reviewed May 8, 2024

View reviewed changes

oleonardolima force-pushed the feat/add-further-amount-usage branch 2 times, most recently from 8f0b685 to 80deac6 Compare May 15, 2024 02:15

ValuedMammal mentioned this pull request May 15, 2024

Use bitcoin::Amount everywhere #1432

Closed

oleonardolima changed the title ~~feat: add further bitcoin::Amount usage~~ feat: add further bitcoin::Amount usage on public APIs May 22, 2024

oleonardolima force-pushed the feat/add-further-amount-usage branch from 80deac6 to 6c572d7 Compare May 22, 2024 18:54

ValuedMammal reviewed May 23, 2024

View reviewed changes

crates/wallet/src/wallet/error.rs Show resolved Hide resolved

oleonardolima force-pushed the feat/add-further-amount-usage branch from 6c572d7 to 5f2d6cb Compare May 23, 2024 01:59

notmandatory approved these changes Jun 1, 2024

View reviewed changes

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the feat/add-further-amount-usage branch 2 times, most recently from 3ae450f to b98ed0f Compare June 3, 2024 14:11

storopoli suggested changes Jun 3, 2024

View reviewed changes

crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the feat/add-further-amount-usage branch from b98ed0f to a03949a Compare June 3, 2024 16:19

storopoli approved these changes Jun 3, 2024

View reviewed changes

notmandatory merged commit 26586fa into bitcoindevkit:master Jun 4, 2024

oleonardolima deleted the feat/add-further-amount-usage branch June 4, 2024 23:14

oleonardolima mentioned this pull request Jun 5, 2024

fix: typo on SignedAmount instead of Amount #1458

Merged

3 tasks

notmandatory mentioned this pull request Jun 14, 2024

Release 1.0.0-alpha.13 #1471

Closed

31 tasks

ValuedMammal mentioned this pull request Sep 9, 2024

refactor(wallet): use Amount everywhere #1595

Merged

3 tasks

	#[derive(Debug, Clone, Copy)]
	pub(crate) enum FeePolicy {
	FeeRate(FeeRate),
	FeeAmount(u64),
	}

Conversation

oleonardolima commented May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

oleonardolima commented May 6, 2024

Uh oh!

oleonardolima commented May 7, 2024

Uh oh!

Uh oh!

ValuedMammal May 8, 2024

Choose a reason for hiding this comment

Uh oh!

storopoli May 8, 2024

Choose a reason for hiding this comment

Uh oh!

oleonardolima May 13, 2024

Choose a reason for hiding this comment

Uh oh!

ValuedMammal May 20, 2024

Choose a reason for hiding this comment

Uh oh!

oleonardolima May 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oleonardolima commented May 17, 2024

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleonardolima commented May 23, 2024

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleonardolima commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oleonardolima commented May 5, 2024 •

edited

Loading

ValuedMammal commented May 8, 2024 •

edited

Loading

oleonardolima commented Jun 3, 2024 •

edited

Loading