Repository navigation
feat(cli): lead wallets get with balance and activity on a terminal - #108
Conversation
tee8z
left a comment
There was a problem hiding this comment.
Requesting changes on 35e46c0 (base #107 799b6db). The headline appears only with table output to a terminal, with no --output-file, so JSON, NDJSON, piped and file output send no extra requests and print nothing new. The wallet name is scrubbed, the price service gets no credential, and a failed extra read only leaves its line out.
P2: the activity line misses most payment types and can say no payments were made. activity (src/balance.rs:142) sums data.amount_msats. In api/openapi.json, that field is deprecated ("Use amount instead. This field will be removed"). On-chain payments carry amount_sats instead, and other types carry only amount. Their completed payments count as 0, so a wallet whose recent completed payments are all on-chain shows no completed payments in 14 days, which is false. Once amount_msats is removed, every wallet shows that. Read data.amount (an Amount) and sum it per currency. The test fixtures use only amount_msats, which is why the suite misses this.
P2: the USD estimate can attach to the wrong balance. priceable_msats is the first BTC balance anywhere in the list (:46), but the ≈ is added to the balance at index == 0 (:78). With balances [usd $12.34, btc 150,000 sats] on mainnet, the line reads Balance $12.34 ≈ $90.00. Attach the estimate to the BTC balance it was computed from.
P2: the payments read is built by hand. The query at src/api.rs:924 sends only wallet_id and limit:
- It sends no
statuses, so failed and expired payments use up the 100 slots. - It sends no sort order, so "newest first" rests on an unstated default.
- It sends no
pagination=cursor, which every other list request gets frominput, so the server runs the total-count query that the contract warns is expensive.
When the 100 newest payments cover only a few days, the rest of the 14 days shows as empty, and the caption understates the volume without saying so.
P2 (AGENTS.md: test every new error path): nothing tests the price read failing (an HTTP error, or a price that UsdPerBtc::parse rejects). Nothing asserts that redirected or --json output makes no price or payments request either. An .expect(0) route on a redirected run would catch that.
Nonblocking:
- The price and payments reads each get the full
--timeout(60 seconds by default), and they run one after the other before anything is printed. An unreachable price service delayswallets getby a minute. Bounding them by the command's deadline, or a short limit, would help. - The
Balancelabel showsavailable, nottotal, without saying so. ≈ignores the ASCII glyph setting.readable_amountinherits #101's mixed units, so stacked lines can read150,000 satsabove1,500,001 msats completed.- The description names a staging organization. A placeholder would be better.
I have not run the gate or the CRAP report on this head yet. CI is green on it.
35e46c0 to
a329faf
Compare
On a terminal, voltage wallets get first shows the wallet's name in Voltage orange, each available balance in bold (with a USD estimate for a BTC balance on mainnet, from the price service), and a sparkline of its completed payment volume over the last 14 days from its 100 newest payments. The two extra reads are best effort: a failure leaves its line out. The wallet's fields follow unchanged, and redirected or JSON output makes no extra request.
a329faf to
73d4b3f
Compare
|
Review-body items, in 73d4b3f:
|
tee8z
left a comment
There was a problem hiding this comment.
Approved 73d4b3f against feat/cli-look-08-context (#107 d5bd5fa). My findings are fixed:
- Activity sums
data.amountin the wallet's currency throughbase_units, so on-chain and other payment types count. - The USD estimate stays on the BTC balance it priced, and a test covers a USD balance that comes first.
- The payments read goes through
Request::scopedwithstatuses[]=completed,sort_order=DESCandpagination=cursor, and payment checks are excluded. - New tests cover a failed or unreadable price, which leaves the estimate out, and a redirected
wallets get, which makes no extra request (.expect(0)). - Each extra read is now bounded by a 5-second
READ_LIMIT, and the balance line is labelledAvailable.
Validation on this head, in nix develop: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (180 unit, 137 integration) and python3 scripts/check-coverage.py passed. CRAP: 779 measured functions, none at or above 20, highest wait_for_payment at 17.05. CI is green.
Stack position: 9 of 13 in the look-and-feel stack. Base:
feat/cli-look-08-context(#107).Problem
voltage wallets getanswers "how much is in this wallet?", but the balance was a JSON blob in the middle of 20 fields, in msats, and nothing showed recent activity.Behavior
On a terminal (stdout is a TTY, table output), the wallet's fields are led by:
name, in Voltage orange.availableamount in the friendly units from #101; negative balances (a line of credit) keep their sign.--price-url). Test-network coins have no price, so mutinynet and signet show none.data.amountin the wallet's currency, from its 100 newest completed payments (statuses[]=completed,sort_order=DESC, by cursor). Each bar is scaled to the busiest day; a day with no volume is the floor bar. ASCII bars without a UTF-8 locale.The extra reads (the BTC price on mainnet, and
payments list --wallet ID --limit 100) are best effort: one that fails leaves its line out and the command still succeeds. They happen only on a terminal with table output. Redirected,--output table | cat, and--jsonmake no extra request and print exactly what they did before.Design notes
balance::Headlineis pure:from_wallet,price,activity(page, now),sparkline, andlines. All formatting reusesoutput::readable_amount(the feat(cli): show friendly amounts and relative times, and color tables #101 amount renderer, now exported) andepoch_seconds.Call::readcallslead_with_balanceforOperationId::GetWalletwhenOutput::on_terminal(). That writes the headline throughOutput::preface, the same color-aware stdout as the table, then the usualwrite_pages.Callnow carries&GlobalFlagsfor the price URL and timeout.Line::styled_brandpaints scrubbed text, the wallet's name, in the brand color.UsdPerBtc::msats_to_cents, the same exact, half-up conversion asvoltage convert.Try it on staging
Setup: Try a pull request on staging (added by #97). Read-only. Find a wallet ID with
voltage wallets list --account me.Expect: the wallet's name in orange,
Balancewith its available amount (staging wallets are on mutinynet, so there's no≈ $), and anActivitysparkline with a caption. The usual fields follow after a blank line.Expect: ASCII bars (
_for empty days). Then the plain fields only, with no headline and no extra requests. Then JSON, unchanged.Validation
cargo fmt --check,cargo clippy --locked --all-targets -- -D warnings,cargo machete --with-metadata,cargo test --locked(164 unit and 128 integration tests), andpython3 scripts/check-coverage.pyall pass.-$90.00at $60,000), a USD balance, and an empty wallet.≈ $90.00from a mocked price service, and the activity read sent withwallet_idandlimit=100.≈. A 500 from the activity read leaves theActivityline out, and the command still succeeds.LANG, removesLC_ALLandLC_CTYPE, and passes underLC_ALL=C. That is how the CRAP run first caught it leaking the runner's locale.a staging wallet).src/cli.rsendpoint, untouched).lead_with_balanceis 5.0,Call::read11.0, andbalance.rsat most 6.0.Docs
docs/guides/wallets.mdandCHANGELOG.md.