镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

refactor(cli): resolve credentials through one auth::Resolver - #59

Closed
tee8z wants to merge 9 commits into
feat/cli-wallet-currency-defaultfrom
refactor/cli-auth-context
Closed

tee8z wants to merge 9 commits into
feat/cli-wallet-currency-defaultfrom
refactor/cli-auth-context

Conversation

@tee8z

@tee8z tee8z commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Stack: master ← #24 ← #19 ← #26 ← #27 ← #28 ← #29 ← #30 ← #55 ← this PR. It follows up on the review of #22.

@thebrandonlucas, this refactor touches the credential code from #22 and #55. Please review the pattern.

Summary

The credential functions in auth took the same &Settings, &GlobalFlags, and &SubmissionState on every call. Each caller in pick, api, wallet_currency, and startup passed all three through. This PR adds auth::Resolver, which holds those three values for one command. The functions become its methods.

There is no behavior change. No test changed.

Signatures

Function Before After
auth::resolve (settings, scope, flags, submission) resolver.resolve(scope)
auth::resolve_organization (settings, scope, flags, submission) resolver.resolve_organization(scope)
auth::exchange (settings, scope, flags, credential) resolver.exchange(scope, credential)
auth::discover (settings, scope, flags, what, submission) resolver.discover(scope, what)
auth::list (settings, scope, flags, what, token) resolver.list(scope, what, token)
api::Authorization::resolve 5 parameters 3 parameters
api::execute 7 parameters 6 parameters
pick::fill 5 parameters 4 parameters
pick::ask 6 parameters, fills a &mut Vec<String> 4 parameters, returns Vec<String>
wallet_currency::fill 5 parameters 4 parameters

The pattern

A struct holds the dependencies that stay the same for one command. Values that change during the command stay as method arguments.

#[derive(Clone, Copy)]
pub struct Resolver<'a> {
    pub settings: &'a Settings,
    pub flags: &'a GlobalFlags,
    pub submission: &'a SubmissionState,
}

startup::execute builds one Resolver after it reads the settings and scope. startup::local_command builds one for each discover call, because the other local commands need &mut Settings.

flowchart LR
    X["startup::execute"] -->|builds once| R["auth::Resolver"]
    R --> P["pick::fill"]
    R --> W["wallet_currency::fill"]
    R --> E["api::execute"]
    E --> A["Authorization::resolve"]
    L["startup::local_command"] -->|builds per call| D["Resolver::discover"]
Loading

The repository already uses this pattern. Steps<'a> in api.rs holds "what every step of one command's run shares." Api::new reads timeout, api_url, and the terminal from GlobalFlags one time.

Why the scope is not in the struct

pick::ask changes scope.org between resolve and exchange. If Resolver held a &Scope, pick could not get &mut Scope while the resolver exists. Each method takes the scope as an argument.

Why not one Context struct for everything

A single struct with all shared values is a service locator. docs/quality.md forbids this: "Pass dependencies through constructors. Do not use mutable global state or a service locator."

A context struct also causes these problems:

  • It hides each function's real dependencies. Now, exchange does not use SubmissionState, so it cannot change the saved login. A context struct hides that fact.
  • It conflicts with borrows. login, logout, import_key, and profiles need &mut Settings. pick needs &mut Scope. A struct with both forces &mut ctx everywhere, or a split back into parts.

What stays outside Resolver

Item Reason
auth::select voltage context shares it and never refreshes a login, so it needs no SubmissionState.
auth::login, logout, import_key, status These are commands. login, logout, and import_key change Settings.
Mutation::execute It uses settings and submission for the request journal, not for credentials. api::execute passes resolver.settings and resolver.submission explicitly, so Resolver does not become a general context.

pick::ask returns what it picked

Before, fill passed &mut Vec<String> to ask and read it after the call. fill ignored the vector when ask failed. Now ask returns Result<Vec<String>>. Esc, an empty list, or an API key returns the picks so far. An error returns no picks. The "Next time, pass …" hint shows in the same cases as before.

ask also resolves the credential itself now. It does this first, so a missing credential is still reported before a missing scope.

Review exceptions

  • Crate-visible item: auth::Resolver. Its fields are public because it is a record with no invariant (docs/quality.md: "Expose fields directly on records"). Callers build it with a struct literal, so it has no constructor.
  • No new dependencies, traits, getters, unsafe blocks, or lint exceptions.

Review tip

Most of the auth.rs diff moves five functions into impl Resolver<'_> and indents them. To see only the changed lines, hide whitespace on GitHub, or run:

git diff --color-moved=zebra --color-moved-ws=allow-indentation-change origin/feat/cli-wallet-currency-default...origin/refactor/cli-auth-context

Checks

On f260426, rebased onto #55 (0e6ff88). The rebase conflicted in auth.rs; the resolution keeps #24's expired_login and shell_word, and Resolver::exchange calls require_login, so a revoked session still gets the logout-then-login hint at the token exchange.

  • cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata: pass.
  • cargo test --locked: 107 unit and 91 integration tests pass, including a_revoked_login_names_the_command_that_renews_it.
  • python3 scripts/check-coverage.py: pass.
  • CRAP (scripts/crap-report.sh): no function at or above 20. The highest is api::wait_for_payment at 19.24, which this PR does not change. pick::ask and Resolver::exchange are 14.00 at 100% coverage.
  • Diff against feat(cli): default a payment's currency to its wallet's, and units to base units #55: 5 files, 224 insertions, 234 deletions.

Not run: VOLTAGE_BLESS=1 cargo test, because no global flag changed. The staging and live tutorials need credentials.

thebrandonlucas and others added 9 commits October 20, 2026 07:25
Errors for a missing --org, --env, --wallet, or webhook ID now name the
command that lists those IDs and every way to supply one. An unknown or
ambiguous credential or profile lists the saved names. An expired or
revoked login (OAuth invalid_grant) says so and gives the exact voltage
logout --local and voltage login commands that replace it. organizations
list and environments list with an API key say which key was used and how
to select a browser login.

API rejections carry a next step. For an account credential, HTTP 401
points to voltage auth status and HTTP 403 to voltage context. For a
checkout session or stream token, both point to the token in --token-file,
VOLTAGE_CHECKOUT_TOKEN, or VOLTAGE_STREAM_TOKEN. quote_required points to
voltage quotes create and notes that a wallet on a BTC line of credit needs
no quote. Hints can now be built at runtime and are scrubbed like
messages, since they can name saved credentials.

A 4xx rejection's message leads with the API's own reason, such as "Quote
is required for payment", instead of only the HTTP status. A 5xx message
gives only the status, and its text stays in the details. The presented
credentials are redacted from the message as well as the details, so an
API reason that echoes a key cannot print it. Human output shows the rest
of the API error as indented field/value lines rather than a JSON line;
--json still reports the complete response. The payments guide shows the
quote-then-receive flow a USD wallet needs.
… per line

Each item of a string list now gets its own line in human output, aligned
with the value column, so a dry run's notes no longer run together. An empty
list shows as -.

voltage context reports auth_url beside api_url: for a saved browser login,
the URL it was saved with (sourced like the credential), else the
normalized --auth-url flag, else the built-in default. A --auth-url that
differs from a saved login's URL is the credential problem
auth_url_mismatch, which context reports and commands reject (exit 2) with
a hint to omit the flag or sign in at the new URL.

Payment waits and device approval show how long they have waited, such as
"Waiting for the payment to be completed... 1m 05s", so a status line on a
slow wait does not look stuck. Progress keeps its #[must_use].
New guides cover wallets (create, inspect, ledger, policies, delete) and
webhooks (create with a saved secret, events, test, pause, rotation,
deliveries). The docs index links both.

An integration test now runs every change the docs show with --execute as an
offline --dry-run --json and requires exit 0 and outcome dry_run, with no
request reaching the mock API. It caught that webhooks test needs a --data
body. The docs example extractor moves to tests/support/examples.rs so the
parse test and the dry-run test share it.
Every command that changes something now has examples in --help, each
under a # comment that says what it does, as do login, auth import-key,
profiles create, context, convert, payments get --wait, and checkout
events watch.

-h stays short: usage and options. --help adds the notes (request body,
friendly flags, scope, feature requirement, examples) and a Learn more link
to the command's guide in the docs of the running release. The root help
lists common commands and links the documentation.

Tests require an example for every change command and every alias, a
Learn more link on every command that names an existing docs page, and no
notes under -h.
docs/reference/global-flags.md is rendered from GlobalFlags by a test:
plain cargo test fails when it is stale, and VOLTAGE_BLESS=1 cargo test
rewrites it. New hand-written pages cover every environment variable with
the flag that beats it, and the configuration directory, credential stores,
and request journal.

Parity tests require every VOLTAGE_* variable read in src/ to appear in the
environment reference, and every exit code in the exit code reference; the
exit code list is checked by an exhaustive match, so a new ErrorKind fails
to compile until it is listed.
…, and a PR template

docs/guides/troubleshooting.md is keyed by exit code and by each
credential_problem name, with the fix for each; a test requires an entry for
every exit code and every CredentialProblem, which exhaustive matches keep
complete. docs/getting-started.md walks from install to a settled mutinynet
payment and a scripted receive.

CHANGELOG.md starts with the changes since 0.1.0. A Changelog workflow
fails a PR that changes src/cli.rs or api/commands.json without
CHANGELOG.md, unless it has the no-changelog label. AGENTS.md states the
rule and that the check is a heuristic. The pull request template asks for
the changelog entry, the verification gate, the review checks, the CRAP
result, and any check not run with its reason.

The docs example checks now also read CHANGELOG.md and the tutorial, so
their commands parse and their changes pass as offline dry runs.
A change without --execute already printed the same description as
--dry-run, offline and without credentials, so the flag only added a
second way to preview. The remaining differences did not need a flag:
VOLTAGE_EXECUTE=0 overrides an exported VOLTAGE_EXECUTE=1 for one command,
--no-input and --json keep the pickers from asking, and exit code 6 is
how a script recognizes a description. Reads are safe to run, so
describing them offline had little use. One rule remains: reads run, and
changes are described (exit 6) unless --execute asks to send them.

-n and --dry-run are gone (they were never released). The description
and its outcome: dry_run envelope are unchanged. The docs and help
examples show a preview as the same command without --execute; the
example test accepts a change without --execute only when the same
command is also documented with --execute, so a forgotten --execute
still fails.

A new integration test walks the command lists that --help prints,
collects each command's Examples section, and runs every example that
sends a change without --execute. Each must exit 6 with outcome dry_run,
create no file, and send no request. Help examples were only parsed
before, so their amounts, units, and scope were never validated.

The safety model leads with a table of reads, changes, and --execute. A
test that previewed checkout reads is replaced by one that sends a real
session read and checks its Origin header.
… 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.
The credential functions in `auth` took the same settings, flags, and
session state on every call. `auth::Resolver` holds those three for the
whole command; the scope stays a per-call argument because a picker can
fill it in between calls. `pick::ask` returns the flags it picked instead
of filling a caller's vector. No behavior changes.
@tee8z

tee8z commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Closing #60 contains this commit and continues the same pattern across auth, config, api, and startup, for discussion as one PR.

@tee8z tee8z closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Skip the Changelog check: the change has no user-visible effect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants