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

Use bitcoin::Amount everywhere #1432

Closed
ValuedMammal opened this issue May 8, 2024 · 4 comments
Closed

Use bitcoin::Amount everywhere #1432

ValuedMammal opened this issue May 8, 2024 · 4 comments
Assignees
Labels
discussion There's still a discussion ongoing
Milestone

Comments

@ValuedMammal
Copy link
Contributor

It was decided initially to only include bitcoin::Amount at the API boundary. No doubt this makes for a better UX. Would it be worth replacing all satoshi amounts represented internally as u64 with the Amount type?

One concern would be: why introduce an abstraction over the denomination when sats are already the standard used throughout the library, but it's possible this fear is overblown.

Alternatives:

  • Use a type alias such as pub type Satoshis = u64; However this serves no real purpose at the type level other than to enhance readability.
  • Use a custom new type and provide conversions to/from Amount. The problem with this is re-inventing the wheel when the Amount type already exists.

So with respect to the internals we should either use Amount everywhere or (almost) nowhere, which seems to be similarly expressed here #823 (comment)

@ValuedMammal ValuedMammal added the discussion There's still a discussion ongoing label May 8, 2024
@tnull
Copy link
Contributor

tnull commented May 13, 2024

I still like the idea of using Amount everywhere as it clearly communicates the type. Furthermore, we'll eventually look to introduce similar type in LDK, as the convertion between msat and sat is an error source that keeps on giving.

@oleonardolima
Copy link
Contributor

It was decided initially to only include bitcoin::Amount at the API boundary. No doubt this makes for a better UX. Would it be worth replacing all satoshi amounts represented internally as u64 with the Amount type?

I might be biased, but I think that using bitcoin::Amount everywhere we use previously used u64 as sats makes it clear and easier for the user to understand what type is being expected/used.

It internally adopts sats as the standard way to represent Bitcoin amounts (because it just wraps: Amount(u64) and SignedAmount(i64), while still having all the support for other operations and conversions between denominations.

Alternatives:

  • Use a type alias such as pub type Satoshis = u64; However this serves no real purpose at the type level other than to enhance readability.
  • Use a custom new type and provide conversions to/from Amount. The problem with this is re-inventing the wheel when the Amount type already exists.

I understand that on both scenarios we are reinventing the wheel, and even at a loss of all the conversions between denominations and error treatment already implemented on bitcoin::Amount

@ValuedMammal
Copy link
Contributor Author

ValuedMammal commented May 15, 2024

Using Amount everywhere means we'll have to remember to change any error messages returned by bdk to no longer include the word "sats" #1426 (comment)

edit: I think we can use display_dynamic which will dynamically set the denomination to display based on the value of the amount.

@notmandatory notmandatory added this to BDK Jun 23, 2024
@notmandatory notmandatory moved this to Discussion in BDK Jun 23, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Jun 23, 2024
@notmandatory notmandatory moved this from Discussion to Needs Review in BDK Sep 9, 2024
notmandatory added a commit that referenced this issue Sep 9, 2024
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
@notmandatory
Copy link
Member

Fixed by #1595. If we want to do the same in other places (like coin selection) please open new PR.

@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion There's still a discussion ongoing
Projects
Archived in project
Development

No branches or pull requests

4 participants