Use `bitcoin::Amount` everywhere
Closed this issue · 4 comments
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 theAmount
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)
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.
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 theAmount
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
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.
Fixed by #1595. If we want to do the same in other places (like coin selection) please open new PR.