multi: Preview refresh fees and gate refresh on consent - #987
Conversation
In this commit, we add a RefreshFeeEstimate message to RefreshVTXOsResponse so a dry_run refresh can carry an advisory, itemized operator-fee preview for the selected VTXOs. Today the refresh path never surfaces the fee anywhere: the binding amount is set by the server-issued JoinRoundQuote at seal time and auto-accepted against the client's MaxOperatorFeeSat cap, so the user only learns the charge afterwards via the fee history. The per-outpoint rows carry the amount and remaining lifetime the daemon resolved for each VTXO (removing the manual amount / remaining-blocks input the standalone fees estimate command requires) plus the operator's liquidity / on-chain share / margin breakdown. Rows are deliberately NOT waiver-adjusted so their components always sum to their total; the selection-level free_refresh_eligible flag and zeroed total express the free-late-refresh waiver instead, mirroring the operator's all-or-nothing seal-time rule from lumos#675. estimate_error keeps dry_run usable as a validity probe when the operator or chain height is unavailable: the preview still returns and the caller is told the numbers are absent rather than zero. The daemon-side population of the new field lands in the next commit. Part of #986.
levmi
added
cli
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fee-consent gate for the ark vtxos refresh command and its corresponding MCP tool, requiring explicit confirmation or the --yes flag on non-interactive stdin to proceed with a fee-charging refresh. It implements a dry-run preview mechanism on the daemon side that calculates advisory fee estimates, handles operator quote deduplication, and computes free-refresh waivers. A review comment identifies a potential integer overflow vulnerability when accumulating the total fee estimate in estimateRefreshFees and suggests a safe addition check to prevent it.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| total += quote.TotalFeeSat | ||
| if total > int64(btcutil.MaxSatoshi) { | ||
| r.server.log.WarnS(ctx, "Refresh fee estimate: "+ | ||
| "selection total overflows", | ||
| fmt.Errorf("total=%d", total)) | ||
|
|
||
| return degradeRefreshEstimate( | ||
| est, "operator fee estimate invalid", | ||
| ) | ||
| } |
There was a problem hiding this comment.
In estimateRefreshFees, total is accumulated by adding quote.TotalFeeSat at each iteration. Since total is an int64, if the sum of fees exceeds MaxInt64, it will overflow and wrap around to a negative value. This would bypass the check total > int64(btcutil.MaxSatoshi) and result in a negative or incorrect total fee estimate being returned to the client.\n\nTo prevent integer overflow, perform a safe addition check before adding quote.TotalFeeSat to total.
\t\tif total > int64(btcutil.MaxSatoshi)-quote.TotalFeeSat {\n\t\t\tr.server.log.WarnS(ctx, \"Refresh fee estimate: \"+\n\t\t\t\t\"selection total overflows\",\n\t\t\t\tfmt.Errorf(\"total=%d\", total+quote.TotalFeeSat))\n\n\t\t\treturn degradeRefreshEstimate(\n\t\t\t\test, \"operator fee estimate invalid\",\n\t\t\t)\n\t\t}\n\t\ttotal += quote.TotalFeeSatIn this commit, we populate the new RefreshVTXOsResponse fee estimate on the dry-run path. The daemon resolves every selected VTXO to its full descriptor, computes each remaining lifetime from the chain tip, and fetches the operator's itemized quote through the existing EstimateFee proxy, deduped on (amount, remaining blocks). The remaining-blocks figure is clamped to 1 for expiring VTXOs because the operator treats zero as "price the full sweep-delay lifetime". The explicit-outpoint path now looks targets up in the VTXO store instead of only parsing strings: an unknown or non-live outpoint surfaces as InvalidArgument instead of echoing back as a plausible preview, mirroring the LiveState filter the --all path already applies — the real refresh can never execute either, and dry_run is the validity probe the CLI consent prompt trusts. Explicit selections are also deduped (order-preserving) so a repeated outpoint can neither double-count in the estimate nor register a doomed duplicate forfeit pair with the wallet. The free-late-refresh waiver (lumos#675) is applied locally: the operator's EstimateFee prices every refresh as paid, so quoting a free late refresh through it alone would over-quote. The daemon already caches the advertised window in its operator terms and knows a refresh selection is the pure one-for-one renewal shape the waiver requires, so a selection fully inside the window previews with an explicit zero total while the rows keep the ordinary paid quote to show what the waiver saves. The estimate is strictly best-effort and degrades all-or-nothing: every quote is fetched and vetted (no negative or beyond-money- supply values, no overflowing selection total) before any component is written to a row, an unreachable operator or chain backend sets estimate_error while the preview and the locally computed waiver verdict survive, and the total uses explicit proto presence — it is only set when meaningful, so a degraded estimate can never be misread as a free refresh. The dry-run branch also moves ahead of the wallet-ready gate, matching the LeaveVTXOs ordering rule that pure-argument validation and previews must not depend on wallet state (the H-5 fix's comment already claimed this parity). A side effect is that an empty selection=all dry run now honestly reports status "preview" instead of "queued". The real refresh path still gates on wallet readiness before queuing. Part of #986.
In this commit, we render the dry-run fee estimate on the refresh command. The itemized numbers stay in the JSON body on stdout; stderr gains a short human-readable summary so an operator reading the terminal sees the headline cost — or the free-refresh-window verdict, or the degraded-mode warning — without stdout consumers having to strip prose. Every wording branch repeats that the value is advisory and the binding fee is set at seal time, and the degraded branch explicitly says a fee still applies so a missing estimate is never read as a free refresh. VTXOs below the operator's minimum viable amount add a count-level warning, mirroring the fees estimate command's below-dust handling. A non-empty dry-run preview that carries no estimate at all can only come from a daemon predating the feature, so the CLI warns about that skew explicitly instead of silently dropping the preview the flag help promises. The fees estimate help now points refresh users at `ark vtxos refresh --dry_run`, which resolves each selected VTXO's amount and remaining lifetime automatically instead of requiring both by hand. Part of #986.
In this commit, we stop dispatching a real refresh without consent. Before this change the number a dry run can now surface was not actionable: by the time a user read anything, the refresh was queued, auto-joined, and priced by the auto-accepted seal-time quote, with no cancel surface anywhere in between. The refresh command now mirrors the leave --all consent pattern: --yes skips the gate for scripted use, non-interactive stdin refuses to prompt and directs the caller to --yes or --dry_run (so agents are never blocked, per the issue's acceptance criteria), and only an interactive TTY prompts. The interactive prompt shows the advisory estimate first — fetched through the same RPC in dry-run form — so the operator consents to a number rather than a mystery fee, and the prompt goes to stderr: stdout stays reserved for the JSON body, so a piped invocation can never swallow the question and read as a hung command. A preview rejected as InvalidArgument aborts outright (the real dispatch would reject the same request shape); any other preview failure degrades to prompting with an explicit "still charged the seal-time fee" warning, so a broken estimate path never makes refreshes unconfirmable and a missing estimate is never read as free. An empty selection (refresh --all with no live VTXOs) skips the prompt entirely — there is no fee to consent to, and warning about a charge for a no-op would be false. The schema registry entry gains the yes parameter and now names the fee in its description, so schema consumers see the same contract as the flag surface. getDaemonClient becomes a package-level indirection (mirroring stdinIsTTY) so wiring tests can drive the full command path against an in-process bufconn daemon and pin that the gate runs before any dispatch: a refused invocation must reach the daemon zero times. Part of #986.
In this commit, we move the leave --all confirmation prompt from stdout to stderr, matching the refresh confirmation gate and the package's stream-split invariant: stdout is reserved for the JSON body, diagnostics go to stderr. Before this change, piping the command (e.g. into jq) on an interactive terminal swallowed the prompt into the pipe — the command read as hung, and a user who typed y blind fed prose into the JSON consumer. Part of #986.
In this commit, we extend the refresh fee-consent contract to the MCP surface. The MCP tool calls the daemon directly rather than executing the cobra command, so the CLI gate never ran there: an agent could queue a fee-incurring refresh with no warning, while the schema registry — documented as the shared source of truth for CLI commands and MCP tools — promised that a real refresh requires yes. The tool description also never mentioned the fee. The ark.vtxos.refresh tool now takes a yes acknowledgement: a real (non-dry-run) call without it returns an immediate, actionable error naming both the dry_run:true preview and the yes:true acknowledgement path — nothing can block on MCP, matching the CLI's non-interactive refusal — and the description now names the seal-time fee and the preview-first flow. The dry-run preview carries the same itemized fee estimate as every other surface. Part of #986.
In this commit, we document the refresh-fee visibility surface. The CLI guide's refresh section now names the operator fee and the seal-time quote that binds it, describes the dry-run estimate (per-outpoint amounts and lifetimes resolved by the daemon, the free-refresh-window verdict, the degraded estimate_error mode), and covers the interactive confirmation plus the non-interactive --yes requirement, with examples updated to match. The waveclicommands agent docs gain the refresh consent-gate invariant — CLI prompt, non-interactive refusal, the MCP tool's yes argument, and the absent-not-zero wire semantics of a degraded total — alongside the existing leave --all and recovery escalate prompt-refusal postures. The waved agent docs gain the dry-run estimation invariant (pre-wallet-gate ordering, store-resolved live-only outpoints, quote dedupe and validation, locally computed waiver, degrade-only failure mode). The repo's own agent-facing refresh examples in the waved skill and CONTEXT.md pick up --dry_run/--yes so an agent following them is not stopped cold by the new non-interactive refusal. Both members of each touched CLAUDE/AGENTS pair receive the same edit; the waved pair's pre-existing divergence on unrelated OOR text is left to the doc-gardening workflow. Part of #986.
64279c1 to
021094c
Compare
|
@claude review this PR |
|
Claude finished @levmi's task in 2m 51s —— View job Review: Preview refresh fees and gate refresh on consentI read the daemon estimate path ( A few notes below, one of which corrects the Gemini finding. 1. The Gemini overflow finding is a false positiveGemini flagged the var total int64
for _, row := range est.Outpoints {
...
total += quote.TotalFeeSat // each quote ≤ MaxSatoshi (refreshQuoteSane)
if total > int64(btcutil.MaxSatoshi) { // returns/degrades the moment it exceeds
return degradeRefreshEstimate(est, "operator fee estimate invalid")
}
}The loop maintains the invariant 2.
|
In this commit, we extend the refresh quote sanity check to enforce the reconciliation the proto documents: OutpointFeeEstimate promises its components always sum to the row's total_fee_sat, and the operator's fee schedule defines the total as exactly liquidity + on-chain share + margin, but refreshQuoteSane only vetted each field for negative or beyond-money-supply values. A broken (or hostile) operator could return an itemization that contradicts its own total and the daemon would echo the inconsistency into the advisory rows. Each component is now individually bounded by MaxSatoshi before the wrap-free sum is compared against total_fee_sat, so a mismatched quote degrades the estimate all-or-nothing exactly like a negative one. Raised in the automated review of #987. Part of #986.
In this commit, we extract the refresh selection parsing into a shared buildRefreshVTXOsRequest builder, mirroring the buildLeaveVTXOsRequest pattern, and add the mutual-exclusion guard the leave surface already has: passing both --outpoint and --all previously let --all win silently, refreshing (and charging for) every live VTXO while the named outpoints were dropped. The MCP tool now goes through the same builder, closing the same silent-drop gap on that surface and turning an empty selection into an immediate client-side error instead of a daemon round trip. Raised in the automated review of #987. Part of #986.
|
Addressed the review findings in two follow-up commits:
Per #1, the accumulation overflow flagged by Gemini is not reachable (the loop invariant keeps |
In this commit, we extract the refresh selection parsing into a shared buildRefreshVTXOsRequest builder, mirroring the buildLeaveVTXOsRequest pattern, and add the mutual-exclusion guard the leave surface already has: passing both --outpoint and --all previously let --all win silently, refreshing (and charging for) every live VTXO while the named outpoints were dropped. The MCP tool now goes through the same builder, closing the same silent-drop gap on that surface and turning an empty selection into an immediate client-side error instead of a daemon round trip. Raised in the automated review of #987. Part of #986.
b2c7902 to
5551d2f
Compare
Roasbeef
added
the
backport-v0.1.x-branch
|
Successfully created backport PR for |
…anch [v0.1.x-branch] Backport #987: multi: Preview refresh fees and gate refresh on consent
2 participants
In this PR, we fix #986, where
ark vtxos refreshqueued a fee-incurring refresh with no estimate, warning, or consent anywhere in the flow — the binding seal-time quote is auto-accepted againstmaxoperatorfeesat, so the user only learned the charge afterwards via the fee history.A dry-run refresh now returns an itemized advisory fee estimate for the selected VTXOs. The daemon resolves each target's amount and remaining lifetime from its own store and chain tip (no more manual
--amount/--remaining-blocksinput), fetches the operator's liquidity / on-chain share / margin breakdown through the existingEstimateFeeproxy (deduped and validated per quote), and applies the free-late-refresh waiver locally — the operator'sEstimateFeeprices every refresh as paid, so a selection fully inside the advertised window previews with an explicit zero total while the per-outpoint rows keep the ordinary paid quote to show what the waiver saves (all-or-nothing, mirroring the seal-timeisFreeRefreshrule from lightninglabs/lumos#675). Estimate failures degrade all-or-nothing to anestimate_errorfield rather than failing the preview, and the total uses explicit proto presence so a degraded estimate is absent on the wire, never a zero an agent could misread as free.A real refresh is now gated on consent on every surface. The CLI mirrors the
leave --allpattern: an interactive TTY shows the estimate and prompts (on stderr, preserving the stdout-JSON stream split — the pre-existing leave prompt moves to stderr in its own commit for symmetry),--yesskips the gate for scripted use, and non-interactive stdin refuses with an actionableINVALID_ARGSerror instead of blocking on a prompt an agent cannot answer. The MCP tool — which calls the daemon directly and never ran the CLI gate — enforces the same contract through a newyesargument with an immediate, actionable error, and its description now names the fee and the preview-first flow, matching what the schema registry promises.The dry-run path also becomes an honest validity probe: it moves ahead of the wallet-ready gate for parity with the
LeaveVTXOsH-5 ordering (whose comment already claimed this symmetry), unknown and non-live explicit outpoints now failInvalidArgumentinstead of echoing back as a plausible preview, repeated outpoints are collapsed so they can neither double-count in the estimate nor register doomed duplicate forfeit pairs, and an empty--allpreview honestly reports"preview". The real refresh path is unchanged beyond the shared selection canonicalization, and a new wallet-actor test pins its--allexpansion.Downstream surfaces are unaffected:
swapdk-serverreads onlyqueued_outpoints+statusfrom refresh responses, and the SDK passes the proto through verbatim. Version skew is handled in both directions — a new CLI against a pre-feature daemon warns that no estimate came back, and old clients simply ignore the new fields. A follow-up issue against lumos for a waiver-aware / outpoint-keyed server-side estimate will be filed once this lands.See each commit message for a detailed description of the individual changes.
Validation
make unit pkg=./waved(also withtags="test_sqlite")make unit pkg=./cmd/wavecli/waveclicommandsmake unit pkg=./waverpcgo test -race ./waved/ -run TestRefresh -count=1andgo test -race ./cmd/wavecli/waveclicommands/ -count=1make fmt-changed-check && make tidy-module-check && make sample-conf-checkmake lint-changed-local(0 issues)make commitmsg-lint range="origin/main..HEAD"make rpc(regenerated output committed; tree clean on re-run)--help,schema ark.vtxos.refresh, non-TTY refusal exits 2 with theINVALID_ARGSenvelope before any RPC reaches the daemon,--yesproceeds straight to dispatchfix #986