accounting: fill client fee ledger gaps - #768
Conversation
![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 comprehensive wallet sweep accounting, updates the round idempotency index, and adds a standalone admin reporting command for the client-side accounting ledger. Feedback on these changes highlights three key areas for improvement: first, the operator fee calculation in round/transitions.go should include cooperative leave outputs (intents.Leaves) to prevent fee inflation; second, the ListClientAccountBalances query in fee_accounting.sql should be optimized using correlated subqueries instead of an OR join to avoid full table scans; and third, the accounting tool should verify the existence of the SQLite database file before opening it to prevent silent file creation.
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.
| if len(intents.VTXOs) > 0 { | ||
| for i := range intents.VTXOs { | ||
| amt := int64(intents.VTXOs[i].Amount) | ||
| if amt > 0 { | ||
| outputsSat += amt | ||
| } | ||
| } | ||
| } else { | ||
| for _, v := range ownedVTXOs { | ||
| if v != nil { | ||
| outputsSat += int64(v.Amount) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The operator fee calculation currently ignores cooperative leave outputs (intents.Leaves). Under the Ark round model, the client's operator fee is the difference between their contributed inputs and all requested outputs (including cooperative leaves). Ignoring intents.Leaves will cause the calculated operator fee to be incorrectly inflated by the entire cooperative leave amount. Summing intents.Leaves and adding them to outputsSat resolves this issue.
if len(intents.VTXOs) > 0 {
for i := range intents.VTXOs {
amt := int64(intents.VTXOs[i].Amount)
if amt > 0 {
outputsSat += amt
}
}
} else {
for _, v := range ownedVTXOs {
if v != nil {
outputsSat += int64(v.Amount)
}
}
}
for i := range intents.Leaves {
amt := intents.LeaveAmount(i)
if amt > 0 {
outputsSat += amt
}
}| -- name: ListClientAccountBalances :many | ||
| SELECT a.account_id, | ||
| a.account_name, | ||
| a.account_type, | ||
| CAST(COALESCE(SUM( | ||
| CASE | ||
| WHEN le.debit_account = a.account_id THEN le.amount_sat | ||
| WHEN le.credit_account = a.account_id THEN -le.amount_sat | ||
| ELSE CAST(0 AS BIGINT) | ||
| END | ||
| ), 0) AS BIGINT) AS balance_sat | ||
| FROM accounts AS a | ||
| LEFT JOIN ledger_entries AS le | ||
| ON le.debit_account = a.account_id | ||
| OR le.credit_account = a.account_id | ||
| GROUP BY a.account_id, a.account_name, a.account_type | ||
| ORDER BY a.account_id; |
There was a problem hiding this comment.
Using an OR condition in a LEFT JOIN (e.g., le.debit_account = a.account_id OR le.credit_account = a.account_id) prevents SQLite from efficiently utilizing indexes on debit_account and credit_account, resulting in full table scans. Since ledger_entries can grow very large in production, this query will become a performance bottleneck. Rewriting the query to use two correlated subqueries allows SQLite to perform highly efficient index lookups.
-- name: ListClientAccountBalances :many
SELECT a.account_id,
a.account_name,
a.account_type,
CAST(
COALESCE((SELECT SUM(amount_sat) FROM ledger_entries WHERE debit_account = a.account_id), 0) -
COALESCE((SELECT SUM(amount_sat) FROM ledger_entries WHERE credit_account = a.account_id), 0)
AS BIGINT) AS balance_sat
FROM accounts AS a
ORDER BY a.account_id;| if *sqlitePath == "" { | ||
| return fmt.Errorf("--sqlite is required") | ||
| } |
There was a problem hiding this comment.
By default, opening a non-existent file path with SQLite will silently create an empty database file. If a user makes a typo in the --sqlite flag, this tool will create an empty file and then fail with a confusing database error. Checking if the file exists using os.Stat before opening it prevents this silent file creation and provides a much clearer error message.
| if *sqlitePath == "" { | |
| return fmt.Errorf("--sqlite is required") | |
| } | |
| if *sqlitePath == "" { | |
| return fmt.Errorf("--sqlite is required") | |
| } | |
| if _, err := os.Stat(*sqlitePath); os.IsNotExist(err) { | |
| return fmt.Errorf("sqlite database file does not exist: %s", *sqlitePath) | |
| } |
|
@claude review this |
|
Claude finished @Roasbeef's task in 5m 28s —— View job Review: accounting — fill client fee ledger gapsI read the full PR diff (the 11 commits in Assessment of the existing Gemini review1. Operator fee should add The leaves loop is still present in for i := range intents.Leaves {
amt := intents.LeaveAmount(i)
if amt > 0 {
outputsSat += amt
}
}Gemini's diff was reading the first hunk only; the leaves subtraction sits just past it and is unchanged from 2. The point is technically correct: 3.
My own findingsStrengths
Minor suggestions (non-blocking)
VerdictSolid, mergeable work. None of the three Gemini comments require action (two are already implemented; one would regress the fee math). My suggestions are all optional hardening. The double-entry model, idempotency design, and atomicity guarantees are sound, and the documentation (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c60ad8196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(intents.VTXOs) > 0 { | ||
| for i := range intents.VTXOs { | ||
| amt := int64(intents.VTXOs[i].Amount) | ||
| if amt > 0 { | ||
| outputsSat += amt |
There was a problem hiding this comment.
For normal boarding/refresh intents, intents.VTXOs[i].Amount is the pre-fee target amount the wallet registered, while the server-stamped residual is what ends up in the confirmed VTXO and is available via ownedVTXOs (and only leave quotes are copied back onto Intents). Because this branch ignores ownedVTXOs whenever any VTXO request exists, a 100k boarding or refresh that confirms with a 99.5k VTXO computes outputs as 100k and emits no FeePaidMsg, so fees_paid and vtxo_balance are wrong for the common fee-paying paths. Please sum actual/quoted local VTXO amounts and only add the non-owned request amounts needed for recipient outflows.
Useful? React with 👍 / 👎.
In this commit, we close the fee-ledger gaps around round outflows and wallet sweep accounting. Round settlement now emits explicit outflow rows for directed-send recipients and leaves, and it records boarding fees the same way refresh fees are recorded. Boarding sweep confirmation now moves inputs through wallet_clearing, records chain cost including the P2A anchor, and settles external sweep destinations with a wallet sweep transfer event.
In this commit, we wire the unroll registry and child actor to the ledger sink and emit ExitCostMsg once the final sweep confirms. The event uses the proof target output as the gross exited value and the persisted sweep transaction outputs to derive miner cost. This keeps VTXO's expiry handoff from emitting zero-fee poison messages while still recording confirmed unilateral exits.
In this commit, we add an internal accounting report tool backed by the local ledger database. The command reads sqlc-backed report queries directly, can render text or JSON, and has an optional CoinGecko price source for fiat balance checks without taking a Faraday dependency.
In this commit, we update the package maps and fee-ledger guide for the new accounting flows. The docs now point unilateral exit costs at unroll, describe wallet_clearing and sweep transfers, and keep the accounting command discoverable from the internal tools package.
In this commit, we make the accounting report command refuse to create or mutate the target SQLite database. The command now checks that the path exists, opens SQLite with mode=ro, enables query_only on a single connection, and has regression tests for missing paths and rejected writes.
Round outflow ledger rows use idempotency keys because directed-send recipient outputs and cooperative leave outputs do not have local VTXO outpoints. The old keys only included the outflow kind and index. Since the DB dedupes idempotency keys globally, later rounds could silently skip transfers_out rows. Prefix those keys with the round ID and add a regression test covering foreign VTXO and leave outflows. Refresh the stale accounting comments while here.
Update stale test and sweep comments to match wallet clearing. External sweep destinations now settle with WalletSweepTransferMsg. Boarding fees are covered by the fee handler tests.
On-chain sweep fee rows do not carry a round or session id. Require their sweep txid idempotency key before inserting, so malformed messages cannot bypass every ledger dedup index. Refresh the directed-send fee test to cover the current split between recipient outflow value and operator fee value.
The admin accounting tool's CoinGecko price source piped the HTTP response body straight into json.NewDecoder().Decode with no size bound, so a spoofed, MITM'd, or misbehaving endpoint could stream an unbounded body and exhaust memory within the request timeout window. Wrap the body in an io.LimitReader capped at 1 MiB (the simple-price payload is a few hundred bytes) before decoding, and reject NaN/Inf prices alongside the existing non-positive check so a poisoned value cannot flow into the satoshi-to-fiat conversion.
Boarding-sweep confirmation emitted its accounting as a fan-out of
independent Tells: one FeePaidMsg{FeeTypeOnchainSweep}, one UTXOSpentMsg
per input, and a UTXOCreatedMsg or WalletSweepTransferMsg for the
destination. Each Tell landed in the ledger actor separately, so a
partial failure (one rejected or dropped leg) could commit some legs and
strand the rest, leaving the wallet_clearing account drifted non-zero
with no path back to zero.
The ledger is a durable actor, so the fix is not an outbox but a single
Tell. Add BoardingSweepConfirmedMsg carrying the sweep txid, chain cost
(miner fee + P2A anchor), the per-input list, and the destination
(external flag + amount). handleBoardingSweepConfirmed expands it into
every clearing leg inside ONE Commit, so wallet_clearing either nets to
zero or nothing is written. The per-leg idempotency keys match the old
single-message keys, so an in-flight message straddling an upgrade still
dedups.
This also closes two related gaps: the emitter now skips a record whose
tx is absent (it could not produce a balanced set, and the old
FeeAmount fallback dropped the anchor), and the audit-only non-positive
guard in handleUTXOSpent is scoped to the boarding-sweep-input branch so
a zero-amount audit spend can no longer poison the durable mailbox.
WalletSweepTransferMsg is removed (folded in). The FeeTypeOnchainSweep
and boarding-sweep classification handlers are retained for direct
callers and unit tests.
The confirmed unilateral exit cost is emitted to the ledger from notifyRegistryIfTerminal, just before the registry is told the child is terminal. That handoff stops the child, and completed records are not restored on boot, so a failed exit-cost Tell had no later retry: the emission was best-effort and the leg was lost. The ledger sink is a durable mailbox, so a Tell that returns nil is durably accepted and processed at-least-once. The only loss is a Tell that errors (e.g. mailbox backpressure). Make emitExitCostIfCompleted report whether the terminal handoff may proceed: a transient Tell failure defers it, so the child stays alive and subscribed and the next height tick retries; the ledger handler dedups by target outpoint, so a redelivery is a no-op. A deterministic build failure (a should-never- happen inconsistency on a completed actor) logs at error and proceeds so the handoff is not wedged forever. VTXO retirement has its own startup-reconciliation backstop, so deferring it is safe. Also document that the recorded exit cost is the final sweep transaction's miner fee only, not the cumulative cost of broadcasting the intermediate tree transactions, which remains a deferred item.
While this work was in review, 000019_oor_session_registry merged to main. CI rebases each PR onto main before testing, so the rebased tree carried two migrations numbered 000019 (oor_session_registry and accounting_wallet_sweeps); golang-migrate rejects the duplicate version with "duplicate migration file", which aborted schema setup and failed nearly every DB-backed test across the unit and system suites. Renumber the accounting migration to 000020 so it lands after 000019_oor_session_registry, and bump LatestMigrationVersion to 20. The migration SQL is unchanged, so make sqlc regenerates an identical schema. Verified the migration tests pass both in isolation and with 000019_oor_session_registry present (the rebased layout).
Update the wallet -> ledger emission note to reflect the single BoardingSweepConfirmedMsg that replaced the FeePaidMsg + UTXOSpentMsg + UTXOCreatedMsg/WalletSweepTransferMsg fan-out.
The accounting report command hardcoded a read-only SQLite open, so it could not run against a daemon backed by Postgres. Open the database through the shared db package instead (db.NewStoreFromConfig), selecting the backend exactly as the daemon does, and read inside a read-only transaction (db.ReadTxOption) with SkipMigrations so the report never mutates the schema. This mirrors the server-side accounting command. Add a -backend selector plus per-backend connection flags, and emit the report as CSV in addition to text and JSON. The sqlite backend still refuses to create a fresh database on a missing path. Note: the read-only transaction is enforced by Postgres, but the modernc sqlite driver does not reject writes in a read-only transaction, so on sqlite the read-only guarantee rests on SkipMigrations plus the report issuing only SELECTs.
Document how to run the accounting report command against a SQLite or Postgres daemon, the available flags, the text/JSON/CSV output formats, optional CoinGecko fiat conversion, and the read-only behavior (including the modernc sqlite caveat). Register the page under Operations in the docs index and cross-link it from the command package docs.
22381f0 to
2cb10c2
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 5m 52s —— View job Re-review: the new Codex P1 is correct — this is a real fee-accounting bugI focused this pass on the one item that postdates my earlier review: Codex's P1 on The bug
if len(intents.VTXOs) > 0 {
for i := range intents.VTXOs {
amt := int64(intents.VTXOs[i].Amount) // intent target — pre-fee / zero
if amt > 0 { outputsSat += amt }
}
} else {
for _, v := range ownedVTXOs { // confirmed leaf — post-fee
if v != nil { outputsSat += int64(v.Amount) }
}
}The What // The on-chain tx output is the source of truth for the VTXO amount —
// under #270 the server stamps the seal-time residual onto the
// VTXODescriptor before building the tree, so the leaf's non-anchor
// output carries the quoted value rather than the intent target (which
// is zero for change outputs). Reading req.Amount here would persist
// stale data.
leafAmount, err := leafNonAnchorAmount(leaf)So the fee path consumes precisely the value the VTXO-build path discards as "stale data." Confirmed by the seal handshake: at Impact (both directions, both wrong)
Either way Why the unit tests didn't catch it
Suggested fixUse confirmed The rest of the diff (unchanged from my prior assessment)
VerdictOne blocking change: fix the operator-fee output source in
|
1 participant
In this PR, we close the client fee-ledger gaps that would make production accounting hard to reconcile. Round settlement now records boarding fees, refresh and recipient outflows, and wallet sweep clearing legs; unroll now emits the confirmed unilateral exit cost after the final sweep lands.
We also add an internal DB-backed accounting report command under
internal/cmd/tools/accounting. It reads sqlc report projections directly from the daemon SQLite DB, can output text or JSON, and can ask CoinGecko for an optional BTC/fiat balance view without a Faraday dependency.Checks
make sqlcmake fmt-changedgo test ./unroll ./ledger ./vtxo ./darepodgo test ./...make lint-changed-localmake doc-check(exits 0; reports existing mirror divergences inbaselib/actor,db/actordelivery,p-models/durableactor, andp-models/durableactor/bridge)make commitmsg-lint range="origin/main..HEAD"