feat: update `keychain::Balance` to use `bitcoin::Amount` by oleonardolima · Pull Request #1411 · bitcoindevkit/bdk

oleonardolima · 2024-04-24T21:15:43Z

fixes #823

Description

It's being used on Balance, and throughout the code, an u64 represents the amount, which relies on the user to infer its sats, not millisats, or any other representation.

It updates the usage of u64 on Balance, and other APIs:

TxParams::add_recipient
KeyChainTxOutIndex::sent_and_received, KeyChainTxOutIndex::net_value
SpkTxOutIndex::sent_and_received, SpkTxOutIndex::net_value

Notes to the reviewers

It updates some of the APIs to expect the bitcoin::Amount, but it does not update internal usage of u64, such as TxParams still expects and uses u64, please see the PR comments for related discussion.

Changelog notice

Changed the keychain::Balance struct fields to use Amount instead of u64.
Changed the add_recipient method on TxBuilder implementation to expect bitcoin::Amount.
Changed the sent_and_received, and net_value methods on KeyChainTxOutIndex to expect bitcoin::Amount.
Changed the sent_and_received, and net_value methods on SpkTxOutIndex to expect 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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

crates/bdk/src/wallet/tx_builder.rs

crates/chain/src/spk_txout_index.rs

oleonardolima · 2024-04-29T14:55:21Z

I think it's ready for a new round of reviews

I'll move the previous conversations to resolved after it, and if it's everything looks good on this new review round)

storopoli · 2024-04-29T15:21:57Z

I think it's ready for a new round of reviews

Can you move it from Draft PR to a full PR?

oleonardolima · 2024-04-29T16:04:14Z

I think it's ready for a new round of reviews

Can you move it from Draft PR to a full PR?

Yes, forgot to do it

oleonardolima · 2024-04-29T16:34:15Z

~~Oh, I forgot to run the tests on docs, working on it 😅~~
Done!

storopoli

Some suggestions.

Most important, why we are using SignedAmount in some places?

crates/bdk/src/wallet/coin_selection.rs

crates/bdk/src/wallet/mod.rs

crates/bdk/src/wallet/tx_builder.rs

crates/chain/src/keychain/txout_index.rs

crates/chain/src/spk_txout_index.rs

crates/chain/src/tx_graph.rs

crates/chain/tests/test_spk_txout_index.rs

oleonardolima · 2024-04-30T17:33:01Z

Some suggestions.

Most important, why we are using SignedAmount in some places?

It's because net_value used i64 previously, and to keep the standard (and makes sense in my opinion given the fn name) I used the SignedAmount, same for the other comments, they are all net_value references.

(replying on the overall comment too)

storopoli

ACK 362a67a

Great work!

crates/chain/src/spk_txout_index.rs

example-crates/example_cli/src/lib.rs

crates/chain/src/spk_txout_index.rs

ValuedMammal

Sorry my reviews have been intermittent. I think this is good.

The commit history could be cleaned up slightly:

In df92624 we want to refer to TxBuilder::add_recipient (not CoinSelectionAlgorithm)
Some changes in later commits (like fixing imports and wallet examples) could be squashed into earlier ones, but this not a blocker in my opinion.

Some other thoughts: I agree there is potentially more to do like updating calculate_fee and Psbt::fee_amount to use Amount. I'm still unsure how we should handle amounts internally. Honestly I don't mind leaving it as u64. If we decide that all amounts should be strongly typed, then Amount seems like a logical choice, my only concern is we should avoid the possibility of mixing up denominations.

crates/bdk/src/wallet/mod.rs

crates/bdk/tests/wallet.rs

- update all fields `immature`, ` trusted_pending`, `unstrusted_pending` and `confirmed` to use the `bitcoin::Amount` instead of `u64` - update all `impl Balance` methods to use `bitcoin::Amount` - update all tests that relies on `keychain::Balance`

oleonardolima · 2024-05-05T01:02:07Z

Sorry my reviews have been intermittent. I think this is good.

The commit history could be cleaned up slightly:

In df92624 we want to refer to TxBuilder::add_recipient (not CoinSelectionAlgorithm)

Some changes in later commits (like fixing imports and wallet examples) could be squashed into earlier ones, but this not a blocker in my opinion.

Some other thoughts: I agree there is potentially more to do like updating calculate_fee and Psbt::fee_amount to use Amount. I'm still unsure how we should handle amounts internally. Honestly I don't mind leaving it as u64. If we decide that all amounts should be strongly typed, then Amount seems like a logical choice, my only concern is we should avoid the possibility of mixing up denominations.

@ValuedMammal No worries, thanks for the comments and suggestions! I tried to address them, please let me know if something remains or if new needed changes appear.

ValuedMammal

ACK 2e6aeb7

crates/bdk/src/wallet/mod.rs

ValuedMammal · 2024-05-05T04:46:04Z

Actually, changing calculate_fee and Psbt::fee_amount should be fairly easy to add to this PR but I'll let you decide if you want to tackle it here or later. Besides that, I'm not sure if there's still more places in the public API that would benefit from using Amount.

- update `wallet.rs` fns: `sent_and_received` fn - update `keychain` `txout_index` fn: `sent_and_received and `net_value`

oleonardolima · 2024-05-05T21:19:49Z

Actually, changing calculate_fee and Psbt::fee_amount should be fairly easy to add to this PR but I'll let you decide if you want to tackle it here or later. Besides that, I'm not sure if there's still more places in the public API that would benefit from using Amount.

I updated calculate_fee and fee_amount on ac762f4, but ended up touching other parts as well.

I would rather move this commit ac762f4 to another PR, and keep only the previous ones here, wdyt ?

oleonardolima · 2024-05-05T21:23:24Z

Actually, changing calculate_fee and Psbt::fee_amount should be fairly easy to add to this PR but I'll let you decide if you want to tackle it here or later. Besides that, I'm not sure if there's still more places in the public API that would benefit from using Amount.

I updated calculate_fee and fee_amount on ac762f4, but ended up touching other parts as well.

I would rather move this commit ac762f4 to another PR, and keep only the previous ones here, wdyt ?

I moved it to the other PR #1426, we can start discussing the changes on that one after this one is merged.

evanlinjin

ACK 22aa534

a03949a feat: use `Amount` on `calculate_fee`, `fee_absolute`, `fee_amount` and others (Leonardo Lima) Pull request description: 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: * [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 #### New Features: * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: storopoli: ACK a03949a Tree-SHA512: abfbf7d48ec004eb448ed0a928e7e64b5f82a2ab51ec278fede4ddbff4eaba16469a7403f78dfeba1aba693b0132a528bc7c4ef072983cbbcc2592993230e40f

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from 0e6d7e5 to ae54551 Compare April 25, 2024 00:08

oleonardolima changed the title ~~wip(feat): update wallet::Balance to use bitcoin::Amount~~ wip(feat): update keychain::Balance to use bitcoin::Amount Apr 25, 2024

oleonardolima commented Apr 25, 2024

View reviewed changes

crates/bdk/src/wallet/tx_builder.rs Outdated Show resolved Hide resolved

oleonardolima commented Apr 25, 2024

View reviewed changes

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

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from 736495f to fe5e313 Compare April 26, 2024 14:22

notmandatory added module-wallet api A breaking API change labels Apr 28, 2024

notmandatory assigned oleonardolima Apr 28, 2024

notmandatory added this to the 1.0.0-alpha milestone Apr 28, 2024

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch 4 times, most recently from 5aa1990 to 82b2ae5 Compare April 29, 2024 14:42

oleonardolima changed the title ~~wip(feat): update keychain::Balance to use bitcoin::Amount~~ feat: update keychain::Balance to use bitcoin::Amount Apr 29, 2024

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from 82b2ae5 to 06d3d96 Compare April 29, 2024 16:03

oleonardolima marked this pull request as ready for review April 29, 2024 16:03

oleonardolima requested review from ValuedMammal, notmandatory and storopoli April 29, 2024 16:04

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch 2 times, most recently from 423aed4 to df4cdfe Compare April 29, 2024 20:42

storopoli suggested changes Apr 30, 2024

View reviewed changes

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from df4cdfe to 362a67a Compare April 30, 2024 17:29

oleonardolima requested a review from storopoli April 30, 2024 17:31

storopoli approved these changes Apr 30, 2024

View reviewed changes

ValuedMammal reviewed Apr 30, 2024

View reviewed changes

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

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from 362a67a to c0b0e14 Compare April 30, 2024 20:40

oleonardolima requested a review from ValuedMammal April 30, 2024 20:42

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from c0b0e14 to bd41a0c Compare May 1, 2024 23:00

ValuedMammal reviewed May 2, 2024

View reviewed changes

example-crates/example_cli/src/lib.rs Show resolved Hide resolved

ValuedMammal reviewed May 2, 2024

View reviewed changes

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

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from bd41a0c to df92624 Compare May 2, 2024 16:59

oleonardolima requested a review from ValuedMammal May 2, 2024 17:00

ValuedMammal reviewed May 3, 2024

View reviewed changes

crates/bdk/src/wallet/mod.rs Show resolved Hide resolved

crates/bdk/tests/wallet.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch 2 times, most recently from 86bbd79 to 234ab88 Compare May 5, 2024 00:54

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from 234ab88 to 2e6aeb7 Compare May 5, 2024 01:00

oleonardolima requested a review from ValuedMammal May 5, 2024 01:01

ValuedMammal approved these changes May 5, 2024

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

oleonardolima added 2 commits May 5, 2024 12:07

feat: use Amount on spk_txout_index and related

d5c0e72

- update `wallet.rs` fns: `sent_and_received` fn - update `keychain` `txout_index` fn: `sent_and_received and `net_value`

feat: use Amount on TxBuilder::add_recipient

22aa534

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from 2e6aeb7 to 22aa534 Compare May 5, 2024 15:07

oleonardolima mentioned this pull request May 5, 2024

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

Merged

5 tasks

oleonardolima force-pushed the feat/use-rust-bitcoin-balance-instead branch from ac762f4 to 22aa534 Compare May 5, 2024 21:22

evanlinjin approved these changes May 6, 2024

View reviewed changes

evanlinjin merged commit dcd2d47 into bitcoindevkit:master May 6, 2024

oleonardolima deleted the feat/use-rust-bitcoin-balance-instead branch May 6, 2024 13:48

notmandatory mentioned this pull request May 10, 2024

Release 1.0.0-alpha.11 #1435

Closed

28 tasks

reez mentioned this pull request May 11, 2024

Bump rust bdk to alpha 11 bitcoindevkit/bdk-ffi#533

Merged

8 tasks

Conversation

oleonardolima commented Apr 24, 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:

Bugfixes:

Uh oh!

Uh oh!

Uh oh!

oleonardolima commented Apr 29, 2024

Uh oh!

storopoli commented Apr 29, 2024

Uh oh!

oleonardolima commented Apr 29, 2024

Uh oh!

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

Uh oh!

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oleonardolima commented May 5, 2024

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ValuedMammal commented May 5, 2024

Uh oh!

oleonardolima commented May 5, 2024

Uh oh!

oleonardolima commented May 5, 2024

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

oleonardolima commented Apr 24, 2024 •

edited

Loading

oleonardolima commented Apr 29, 2024 •

edited

Loading

oleonardolima commented Apr 30, 2024 •

edited

Loading