Skip to content

Introduce and use an amount type throughout public interface #2076

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

Open
tnull opened this issue Mar 5, 2023 · 1 comment
Open

Introduce and use an amount type throughout public interface #2076

tnull opened this issue Mar 5, 2023 · 1 comment

Comments

@tnull
Copy link
Contributor

tnull commented Mar 5, 2023

When converting between different denominations, errors happen easily and actually quite frequently (in particular the notorious off-by-factor-1000 error when converting between sat and msat). While in our current interface all arguments should have the appropriate suffix (_sat or _msat), they still may be easily misused in practice, e.g., as we require giving an amount_sat for channel operations but most other places we require amounts to be denominated in millisatoshi.

I therefore propose to mitigate this issue by introducing a LightningAmount type akin to/reusing bitcoin::Amount and use it consistently throughout our public API. Over time, we then may even want to transition to use this type internally wherever possible to ensure the correctness of conversions.

While being quite a refactoring of our API, I think this might be a worthwhile quality-of-life improvement that pays dividend over time. Looking for opinions, general thoughts, and concept ACKs here.

@TheBlueMatt
Copy link
Collaborator

Makes sense to me. I don't think we should overhaul the API in one go, but rather add the type and then use it in new code and things being touched already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants