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

Open
ValuedMammal opened this issue May 8, 2024 · 3 comments
Open

Use bitcoin::Amount everywhere #1432

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

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.

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
None yet
Development

No branches or pull requests

3 participants