Repository navigation
feat(cli): default a payment's currency to its wallet's, and units to base units - #55
thebrandonlucas wants to merge 1 commit into
Conversation
tee8z
left a comment
tee8z
left a comment
There was a problem hiding this comment.
Suggestions to implement Display on Currency so the notice can format it directly. Checked locally with cargo fmt --check, cargo clippy --all-targets -- -D warnings, and a_payment_without_currency_takes_its_wallets_line_of_credit_currency. Apply all three together: without the import change, the unused Currency import fails clippy.
I would keep unit.currency() and currency.base_unit() as named methods rather than From impls. BTC has three units, and choosing msats is a deliberate rule that AmountUnit::from(currency) would hide. currency() already has other callers, so From would add a second way to do the same thing.
| let name = match currency { | ||
| Currency::Btc => "btc", | ||
| Currency::Usd => "usd", | ||
| }; | ||
| global.terminal().notice(format!( | ||
| "Using the wallet's currency, {name}; pass --currency to choose another." | ||
| )); |
There was a problem hiding this comment.
We could implement the Display trait on Currency (suggested in src/payment.rs), then we wouldn't need this match statement at all and could just use currency in the format!:
| let name = match currency { | |
| Currency::Btc => "btc", | |
| Currency::Usd => "usd", | |
| }; | |
| global.terminal().notice(format!( | |
| "Using the wallet's currency, {name}; pass --currency to choose another." | |
| )); | |
| global.terminal().notice(format!( | |
| "Using the wallet's currency, {currency}; pass --currency to choose another." | |
| )); |
| auth, | ||
| cli::{BodySource, Command, FriendlyFlags, GlobalFlags}, | ||
| config::{Scope, Settings}, | ||
| payment::{Currency, PaymentDirection}, |
There was a problem hiding this comment.
With the match gone, Currency is unused here:
| payment::{Currency, PaymentDirection}, | |
| payment::PaymentDirection, |
| Self::Usd => AmountUnit::Cents, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Display for Currency, spelled as --currency takes it. The full std::fmt paths match impl std::fmt::Display for Error in src/error.rs.
| } | |
| } | |
| /// The `--currency` spelling, as in a notice that names the currency in use. | |
| impl std::fmt::Display for Currency { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.write_str(match self { | |
| Self::Btc => "btc", | |
| Self::Usd => "usd", | |
| }) | |
| } | |
| } |
tee8z
left a comment
tee8z
left a comment
There was a problem hiding this comment.
Second pass on this PR's complexity, with the CRAP report (scripts/crap-report.sh) run on this head. My Display suggestions are in the earlier review.
startup::execute: 20.15 (cyclomatic 20). The newwallet_currency::fillcall adds the 20th branch, and a score of 20 fails at any coverage. The fix moves the price block into its own function, but those lines are outside this diff, so the code is in the inline comment rather than a suggestion.- Untested failure paths: a suggestion adds tests for the unknown-currency error and the missing-wallet return.
pick::ask: 28.30. This comes from #22, lower in the stack. The suggestions on #22 fix it, and it clears here after #55 is rebased.
With every suggestion from both reviews applied:
cargo fmt --checkandcargo clippy --locked --all-targets -- -D warnings: clean.cargo test --locked: 100 unit and 73 integration tests pass.- CRAP report:
executeis 15.00, and the only remaining violation isaskfrom #22. - PR diff: 390 changed lines.
| let mut settings = Settings::open(config::directory(global.config_dir.clone())?)?; | ||
| let mut scope = settings.scope(global.scope_selection())?; | ||
| pick::fill(&command, &global, &settings, &mut scope, submission).await?; | ||
| wallet_currency::fill(&mut command, &global, &settings, &scope, submission).await?; |
There was a problem hiding this comment.
This line takes execute to cyclomatic 20, which fails the CRAP budget even at full coverage (execute scores 20.15; it was 19.16 on #22). Tests can't fix that, so the complexity has to come down.
The price and convert block above (lines 110-133) is its own command handler, with its own unreachable! arm. GitHub can't take a suggestion there because this PR doesn't touch those lines, so here is the change. In execute, replace the let terminal = global.terminal(); line and the price block with:
// Price commands need neither configuration nor credentials.
if let Command::Local(local @ (LocalCommand::Price(_) | LocalCommand::Convert(_))) = command {
let mut out = output(true)?;
let envelope = price_command(local, &global).await?;
return out.write(envelope, &[]);
}and add, before local_command:
/// Read the price service for `price` or `convert`.
async fn price_command(command: LocalCommand, global: &GlobalFlags) -> Result<Envelope> {
let service = PriceService::new(
global.price_url.as_deref().unwrap_or(PRICE_URL),
global.timeout,
)?;
let terminal = global.terminal();
const WAITING: &str = "Waiting for price service...";
match command {
LocalCommand::Price(flags) => Envelope::local(
terminal
.during(WAITING, service.report(flags.at.as_deref()))
.await?,
),
LocalCommand::Convert(flags) => Envelope::local(
terminal
.during(
WAITING,
service.convert(&flags.request(), flags.at.as_deref()),
)
.await?,
),
_ => unreachable!("only price commands are passed"),
}
}That takes execute to cyclomatic 15 (CRAP 15.00 at 100% coverage), with price_command at 5. Checked with fmt, clippy, and the full test suite.
| .assert() | ||
| .code(3); | ||
| assert!(stderr(&missing).contains("Or pass --currency to skip reading the wallet.")); | ||
| assert_eq!(server.received_requests().await.unwrap().len(), read); |
There was a problem hiding this comment.
Covers two new paths no test runs yet: the "Cannot tell this wallet's currency" error with its --currency hint (src/api.rs:570-572), and the early return when there is no wallet (src/wallet_currency.rs:40).
| assert_eq!(server.received_requests().await.unwrap().len(), read); | |
| assert_eq!(server.received_requests().await.unwrap().len(), read); | |
| // Without a wallet there is nothing to read; the payment reports the missing wallet. | |
| let unscoped: Vec<&str> = send | |
| .iter() | |
| .copied() | |
| .filter(|arg| !matches!(*arg, "--wallet" | WALLET)) | |
| .collect(); | |
| let error = cli(dir.path(), &server) | |
| .env_remove("VOLTAGE_EXECUTE") | |
| .args(&unscoped) | |
| .assert() | |
| .code(2); | |
| assert!(stderr(&error).contains("--wallet is required")); | |
| assert_eq!(server.received_requests().await.unwrap().len(), read); | |
| // A wallet without a line of credit has no currency to take, so the hint names the flag. | |
| respond_once( | |
| &server, | |
| route("GET", format!("/organizations/{ORG}/wallets/{RESOURCE}")), | |
| ok(json!({"id": RESOURCE})), | |
| ) | |
| .await; | |
| let mut other = send; | |
| other[7] = RESOURCE; | |
| let error = cli(dir.path(), &server) | |
| .env_remove("VOLTAGE_EXECUTE") | |
| .args(other) | |
| .assert() | |
| .code(2); | |
| assert!(stderr(&error).contains("Cannot tell this wallet's currency")); | |
| assert!(stderr(&error).contains("Pass --currency btc or --currency usd.")); |
… base units A friendly payment without --currency now uses its wallet's currency. A receive with --amount and --unit takes the unit's currency without any request. Otherwise the CLI reads the wallet for its line of credit, then the line of credit's summary, and uses that currency, which is the one that decides whether a payment needs a quote. A wallet has no currency field of its own, so this takes two reads. Stderr names the currency used. An explicit --currency is never replaced and needs no credential. The read happens in a description as in a real run, so both show the same body; it is the one request a described change makes, and passing --currency keeps the description offline. Without a credential, the error keeps its own hint and adds "Or pass --currency to skip reading the wallet." An amount without --unit is in the currency's base unit (msats or cents), and --max-fee without --fee-unit is in msats. The base unit is the smallest, so a misread amount errs toward too little: a smaller invoice, a smaller send, or a fee limit that fails the payment. Currency implements Display with the --currency spelling, so the notice formats it directly. execute hands price and convert to a new price_command, which keeps it under the CRAP budget with the added fill step. Tests cover a payment without a wallet and a wallet without a line of credit.
0659fdc to
d8b58c5
Compare
2e4a3e8 to
2db9d11
Compare
2 participants
Stack: master ← #20 ← #22 ← #24 ← #19 ← #26 ← #27 ← #28 ← #29 ← #30 ← this PR. #54 (removing
--dry-run) is already merged into #30's branch.Responds to tee8z's question on #20: can
--unitand--currencydefault to the wallet's currency and its base unit?Behavior
--currencyon a receive with--amountand--unit--currencyotherwiseGET /organizations/{org}/wallets/{id}forline_of_credit_id, thenGET /organizations/{org}/lines_of_credit/{id}/summaryforcurrency--unit--fee-unitbalances[].Using the wallet's currency, btc; pass --currency to choose another.--currencyis never replaced and needs no credential.--amount 1000meant as sats becomes 1 sat: a smaller invoice, a smaller send, or a fee limit low enough to fail the payment, never an overpayment. It also matches the API, which takes base units.Interaction with #54's describe-by-default
A described change (no
--execute, see #54) still sends nothing, but a payment without--currencynow reads its wallet first, as a real run would, so the description shows the body that will be sent. This is the one request a description makes.--currencykeeps the description fully offline. The safety model and payments guide say so. Without a credential, the error keeps its own hint and adds "Or pass --currency to skip reading the wallet."Validation
cargo fmt --check,cargo clippy --locked --all-targets -- -D warnings,cargo test --locked(100 unit, 73 integration), andpython3 scripts/check-coverage.pypass.New unit test: amounts without
--unituse msats or cents,--max-feewithout--fee-unituses msats, and an explicit unit still wins.New integration test: a send without
--currencymakes exactly the two reads and describes abtcbody; a receive with--unitand a send with--currencymake no reads; with no credential, the command fails with exit 3 and the appended hint.Every documented and help example still passes explicit units, so the offline example tests make no requests.
Staging (
testteam, mutinynet, browser login), from0659fdc:--currency/--unit(described)btc; amount1000 msats; exit 6--currency/--unit(described)usd; then the CLI's own "USD payment flows require an explicit quote_id" (exit 2) before anything is sent--unit sats, no--currency2000 msats--currency,--max-fee 300btc; amount5000 msats, fee300 msats--currency btc--currency/--unit,--wait ready --executeready, currencybtc,1000 msats, payment request presentLimits
line_of_credit_id. Reusing that saves one read; it is left out to keep this change small.quotes createstill requires its--unit.CRAP report
After tee8z's review suggestions (
DisplayforCurrency,price_commandextracted fromstartup::execute, tests for a missing wallet and a wallet without a line of credit):scripts/crap-report.shatd8b58c5finds no function at or above 20.startup::executewent from 20.15 to 15.00,price_commandis 5.03,wallet_currency::fill5.00, andapi::wallet_currency6.00. The highest score isapi::wait_for_paymentat 19.24, which this PR does not change.cargo test --locked: 100 unit and 74 integration tests pass.