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! |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
ldk-server-cli/src/main.rs
Outdated
| #[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
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.
benthecarman
force-pushed
the
cli-positional
branch
from
c15b8ce to
423b3d3
Compare
4 participants
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>.