Introduce LightningAmount type and use across public APIs
#4408
Conversation
Add a new `LightningAmount` struct wrapping `u64` millisatoshis to
provide type-safe handling of Lightning Network amounts. This type
will be used to replace raw `u64` values in public APIs throughout
the codebase.
The type provides:
- Constructors: `from_msat()`, `from_sat()` (saturating on overflow)
- Accessors: `to_msat()`, `to_sat_rounded()`, `to_sat_ceil()`, `to_sat_floor()`
- Conversion: `From<bitcoin::Amount>`
- Arithmetic: `Add`, `Sub`, `AddAssign`, `SubAssign`, `Sum`
- Display: formats as "{value} msat"
- A `ZERO` constant for convenience
Co-Authored-By: HAL 9000
|
👋 Hi! This PR is now in draft status. |
|
Pinged a few people for conceptual review. We still might need to figure out some details, especially on the |
Replace raw `u64` values with the type-safe `LightningAmount` wrapper in all public method APIs dealing with millisatoshi amounts. This includes renaming methods like `as_msat()` to `as_amount()`, and `amount_milli_satoshis()` to `amount()`, ensuring callers use `LightningAmount::from_msat()` and `.to_msat()` for conversions. Internal logic, serialized struct fields, and message types remain unchanged. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
tnull
force-pushed
the
2026-02-amount-type
branch
from
56fbdb2 to
477f9ae
Compare
|
|
||
| /// Constructs a new [`LightningAmount`] from the given number of satoshis. | ||
| /// | ||
| /// Saturates to [`u64::MAX`] on overflow. |
There was a problem hiding this comment.
It this preferable to panic? I wonder if there could be signed/unsigned conversion bugs on the input, then leading to a silent overflow here.
| /// can be denominated in millisatoshis (1/1000 of a satoshi), allowing for finer-grained payment | ||
| /// amounts. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] | ||
| pub struct LightningAmount { |
There was a problem hiding this comment.
Why not call it MilliSatoshi, when that is what it is? Or Msat. If this type is eventually used everywhere, that's quite a few chars saved.
There was a problem hiding this comment.
Also wondering if a type alias would be sufficient. No compiler checking, but it does help I think. Avoids the custom arithmetic ops etc. An amount is so widely passed around, maybe it is too much to make it a new type.
There was a problem hiding this comment.
Why not call it
MilliSatoshi, when that is what it is? OrMsat. If this type is eventually used everywhere, that's quite a few chars saved.
Because we have the bitcoin::Amount counterpart.
No compiler checking
It's mostly the compiler checking and providing default conversion methods between denominations that we're after.
There was a problem hiding this comment.
bitcoin::Amount isn't quite the same. With LightningAmount the use case is pulled into it. bitcoin::MsatAmount would be more consistent.
There was a problem hiding this comment.
bitcoin::Amountisn't quite the same. WithLightningAmountthe use case is pulled into it.bitcoin::MsatAmountwould be more consistent.
Not sure I'm following? FWIW, bitcoin::Amount used to have a MilliSatoshi denomination which was highly confusing (rust-bitcoin/rust-bitcoin#2820), and rust-bitcoin decidedly doesn't to support anything msat in their crate. So we need a dedicated type here (also spoke to other projects that would be interested in reusing it, FWIW).
There was a problem hiding this comment.
Oh sorry, the comment was only about what to name it AmountMsat instead of LightningAmount. Independent of where it lives.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Bleh, can we do this more iteratively? Just introduce the types and use them in a few places that don't touch every test, then do separate PRs that change more code iteratively?
I don't expect the overwhelming majority of these changes to influence anything in-flight. I'd rather not make this yet another years-long endeavour. For example, we added fn-level FWIW, just doing the API, and doing |
Oh come on, that's an inconsistent priority, at best (doubly for things that aren't super actively being modified). Modifying a bunch of interfaces and resulting tests all in a single commit is really not great for review, let alone conflicts. There's ongoing high-priority splicing work that likely conflicts with some of the |
Well, if you insist this needs to be separate PRs, then we better hold up on this after we ship 0.3 to ensure that all PRs (hopefully including I do however also think a similar approach would be preferable to devs, i.e., one rebase to upgrade all APIs is preferable to do it over and over again as we land 5-6 PRs over time. |
|
FWIW, |
5 participants
Fixes #2076.
The confusion around sats/msat keeps coming up over and over again, most-recently surfaced again over at lightningdevkit/ldk-server#123 for example. We therefore for ages planned to introduce a dedicated
LightningAmounttype that avoids that confusion by using strong typing. Now, with the power ofGrayskulClaude, we can easily make this refactoring.To this end, we introduce a
lightning_types::amount::LightningAmountand use it for all occurences of_msat/_msatsin the public API.As follow-ups I also want to finish migrating
_sat/_sats/_satoshisarguments tobitcoin::Amountand fee rates in public APIs to usebitcoin::Amount(cf. #2410). I basically have the changes ready. Reviewers, please let me know if you'd prefer to do it all in one PR rather in ~2-3 PRs.Note that I'd be in favor of eventually using these strong types everywhere in the codebase instead of the
u64s everywhere, as this would considerably reduce the chances of messing up conversions. However, we we choose the less invasive option first: introduce the type, and change the API layer.