Skip to content
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

Balance should use bitcoin::Amount #823

Closed
Tracked by #39 ...
tnull opened this issue Dec 15, 2022 · 6 comments · Fixed by #1411
Closed
Tracked by #39 ...

Balance should use bitcoin::Amount #823

tnull opened this issue Dec 15, 2022 · 6 comments · Fixed by #1411
Labels
api A breaking API change enhancement New feature or request module-wallet
Milestone

Comments

@tnull
Copy link
Contributor

tnull commented Dec 15, 2022

Describe the bug
Currently, the Balance type returned by get_balance() only uses non-descript u64s, while not even stating a denomination anywhere in the documentation (I assume it's sats?). To avoid any confusion and make this much less error prone BDK should reuse the Amount type provided by rust-bitcoin.

Also, the documentation could generally be improved, e.g., clarifying what exactly it means that some funds are immature/trusted_pending/untrusted_pending/confirmed.

To Reproduce
Call Wallet::get_balance().

Expected behavior
Receive a meaningful return value.

Additional context
This is in particular confusing since similar u64 values are used in the Lightning space very often to denominate millisatoshis. It's best practice to append _msat to the names of such variables, which however still doesn't make use of the type system to avoid any conversion errors (we can't, as Amount doesn't allow to express millisatoshis). As BDK doesn't have to deal with millisatoshis, it really should make use of Amount here.

@tnull tnull added the bug Something isn't working label Dec 15, 2022
@LLFourn
Copy link
Contributor

LLFourn commented Dec 16, 2022

If we're going to return it in balance we should probably use it everywhere. The main downside with this is that rust-bitcoin itself doesn't use amounts everywhere e.g. TxOut's value field so there'll still be a some converting to do.

@tnull
Copy link
Contributor Author

tnull commented Dec 16, 2022

I mean I'd def. be +1 for more making use of the type system everywhere possible to avoid any conversion errors, but maybe Balance is a good place to start out? Or would BDK Core be able to adopt and introduce Amount everywhere?

@notmandatory
Copy link
Member

Discussed in team chat, this would be a good change for bdk_core 1.0 release since it's a big API break.

@notmandatory
Copy link
Member

See also: #797 (comment)

@notmandatory notmandatory added enhancement New feature or request module-wallet api A breaking API change and removed bug Something isn't working labels Mar 17, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Mar 17, 2024
@notmandatory
Copy link
Member

I re-tagged this as an enhancement since balances work, just needs a better API.

@oleonardolima
Copy link
Contributor

dev call: I'll give it a try on this one

notmandatory added a commit that referenced this issue Jun 4, 2024
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

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

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

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  ### Notes to the reviewers

  Open for comments and discussions.

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### 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`.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change enhancement New feature or request module-wallet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants