bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
838 stars 300 forks source link

Use `bitcoin::Amount` everywhere #1432

Closed ValuedMammal closed 3 days ago

ValuedMammal commented 4 months ago

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:

So with respect to the internals we should either use Amount everywhere or (almost) nowhere, which seems to be similarly expressed here https://github.com/bitcoindevkit/bdk/issues/823#issuecomment-1354092490

tnull commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Using Amount everywhere means we'll have to remember to change any error messages returned by bdk to no longer include the word "sats" https://github.com/bitcoindevkit/bdk/pull/1426#discussion_r1594222444

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

notmandatory commented 3 days ago

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