Uh oh!
There was an error while loading. Please reload this page.
Make CLI commands use positional arguments - #123
Conversation
Made it so the primary arguments in our cli commands no longer use named flags but instead use positional arguments. For example, bolt11-send can now be used as `bolt11-send <invoice>` instead of `bolt11-send --invoice <invoice>`.
I've assigned @jkczyz as a reviewer! |
ldk-reviews-bot
commented
Feb 9, 2026
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
| #[arg(help = "The address to send coins to")] | ||
| address: String, | ||
| #[arg( | ||
| long, | ||
| help = "The amount in satoshis to send. Will respect any on-chain reserve needed for anchor channels" | ||
| )] | ||
| amount_sats: Option<u64>, |
There was a problem hiding this comment.
Small concern with using positional arguments where the named argument includes a unit (e.g., sats and msats for amounts). At least when reading code, having u64 for sat and msat parameters can be a source of confusion (e.g., channel open in sats with push msats).
@tnull Any preference?
There was a problem hiding this comment.
hmm yeah, this one will be in sats but the lightning ones are in msats.
We could make the lightning ones in sats as that's what people probably expect more and add a msats option for people who need it.
There was a problem hiding this comment.
Small concern with using positional arguments where the named argument includes a unit (e.g., sats and msats for amounts). At least when reading code, having
u64for sat and msat parameters can be a source of confusion (e.g., channel open in sats with push msats).@tnull Any preference?
How about we force the user to supply a denomination whenever we take an amount in the CLI? I.e., parse it to a special amount type that errors if the string doesn't have a sat/sats/msat/msats suffix? We could then also error when a user tries to give an msat value to a sat-denominated field? Seems this might be the safest way? Honestly, I previously ran into this myself on CLN but also ldk-sample, and always hated that some of the amounts where sats and others in msat.
There was a problem hiding this comment.
FWIW, this also brought me to finally do lightningdevkit/rust-lightning#4408 to address lightningdevkit/rust-lightning#2076
There was a problem hiding this comment.
I like this, added. Curious on thoughts on making no suffix default to sats? Current implementation errors on no suffix but imo having a default would make it feel more ergonomic
There was a problem hiding this comment.
Curious on thoughts on making no suffix default to sats?
In a post-BIP177 world, we want to be as explicit as possible I think 😆
No, but really, IMO having it slightly less ergonomic is worth the potential footgun.
c15b8ce to
423b3d3Compare| help = "The amount in satoshis to send. Will respect any on-chain reserve needed for anchor channels" | ||
| )] | ||
| amount_sats: Option<u64>, | ||
| #[arg(help = "The amount to send, e.g. 50sat or 50000msat")] |
There was a problem hiding this comment.
Hmmm.. should anything on-chain should note that it's not possible to send / receive / splice fractional sats?
| /// - `<number>sat` or `<number>sats` — interpreted as satoshis | ||
| /// - `<number>msat` or `<number>msats` — interpreted as millisatoshis |
There was a problem hiding this comment.
May want to allow a space between the amount and the denomination. This is what bitcoin::Amount does for parsing. So we'd probably want to match that for LightningAmount, which I assume will replace this?
There was a problem hiding this comment.
the only way for this to work is if the param is done in a string, ie ldk-server-cli onchain-send bc1p... "900 sats" otherwise clap will treat it as another argument. Can add support but I doubt it'll be used much
Uh oh!
There was an error while loading. Please reload this page.
Replace raw u64 amount fields with an Amount type that requires a denomination suffix (e.g. 50sat, 50000msat). This removes ambiguity about whether positional arguments are in sats or msats.
423b3d3 to
c964787Compare| /// | ||
| /// Bare numbers without a suffix are rejected. | ||
| #[derive(Debug, Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash)] | ||
| pub struct Amount { |
There was a problem hiding this comment.
Probably shouldn't be pub especially if it will be replaced by LightningAmount.
There was a problem hiding this comment.
eh, ldk-server-cli shouldn't be used as a lib so not the end of the world
Uh oh!
There was an error while loading. Please reload this page.
Make CLI commands use positional arguments
Made it so the primary arguments in our cli commands no longer use named flags but instead use positional arguments. For example, bolt11-send can now be used as
bolt11-send <invoice>instead ofbolt11-send --invoice <invoice>.