ledger: make the zero-sum check sound, and stop rendering money through a double - #312
Merged
Conversation
…e zero-sum check (#304 §A1) The per-currency zero-sum invariant was unsound in both directions, and two tests now pin it. `TransactionLeg::amount` is a `morph::math::Rational` used as a scaled integer -- the numerator counts the currency's minor units and `decimalPlaces` names the scale they are counted at, which is how `parseAmount`, `LedgerQmlBridge::storeTransaction` and every QML money label already read it. `Rational` reads the same triple differently: its value is `numerator/denominator` and `decimalPlaces` is a display tag that, by its own spec, "never changes a stored value" and is ignored entirely by comparison. Nothing restated a leg before summing it, and `Rational::operator+` adds numerators and propagates `std::max` of the two precisions, so: - $4.50 (`{450, dp 2}`) against -$45.00 (`{-450, dp 1}`) netted to a numerator of zero and was **accepted** -- a journal booking four dollars fifty against forty-five dollars, posted as balanced; - $4.50 written `{45, dp 1}` against -$4.50 written `{-450, dp 2}` netted to -405 and was **rejected**, though it balances. Both were reproduced against the real `StoreTransaction` path before this change: the first as "no exception was thrown where one was expected", the second as "zero-sum violation in USD: legs did not sum to zero". The fix is `ledger::restateMinorUnits` (new `ledger/core/money.hpp`), applied in `execute(StoreTransaction)` before the partitioning loop and in `storeJournalImpl` before its transaction opens, so undo and CSV import get it too. Restating is exact or nothing: widening goes through `morph::math::checkedMul` so an overflow is reported rather than saturated, and narrowing is refused outright when it would drop a non-zero digit. An amount with more precision than its currency has ($4.505 in USD), or a non-integral minor-unit count off the wire (`{"num":9,"den":2}`), is rejected with `ValidationError`. The model never rounds money. The restated amount is also what gets stored, which fixes a second, longer-lived symptom: `buildLedgerState` seeds each balance at the currency's precision and accumulates with the same `std::max` propagation, so one leg written at dp 4 in a USD account rendered that account's balance at a hundredth of its value for as long as the row existed. `SetBudgetLimit` restates a limit the same way -- a limit is compared against a sum of legs, and two amounts only compare as money on one scale -- and `GetBudgetReport` now seeds its running total at the report currency's own precision instead of a hardcoded 2, so a JPY budget stops reporting yen as if they had cents. Direction, since two were on the table. Moving leg amounts to `morph::units::Quantity` is what the design spec claimed the rung did, and it is not implementable: `Quantity` takes its unit as a compile-time non-type template parameter and a leg's currency is the account's runtime data, so `Money<C>` cannot type a leg without putting a false USD tag on every EUR, JPY and KRW one -- a conclusion the spec's own §2 had already reached two paragraphs after asserting the opposite. So the encoding stays, and is now written down and enforced rather than assumed. `Money<C>` is used where the currency *is* known at the point of use: `ledger::formatMoney` switches on it to render through `morph::units::toDecimalString`. `tests/test_ledger_rational_fuzz.cpp` had enshrined the false accept as correct behaviour. Its first case was titled "zero-sum check never false-positives across differing decimalPlaces in one currency" and claimed its two operands were "constructed to sum to true zero once both are reduced to a common scale" -- they are not reduced to a common scale, they are -$50.00 and +$0.50, and the case never reaches the model at all. It now says what it actually measures, and points at the model tests for the invariant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§A1) Three QML views computed their own money text from the exact triple the bridges publish -- `(numerator / denominator) / Math.pow(10, places)` in `LedgerView`, `BudgetView` and `ReportView`. QML has only IEEE doubles, so those three lines undid `Rational`'s exactness at the very end of a path built to preserve it, and drifted for balances past 2^53 while the payload beneath them stayed exact. `examples/ledger/README.md` forbids exactly this, and `ledger_qml_bridge.cpp`'s own doc comment had promised a `balanceText` since the bridge was written; `grep balanceText` matched that comment and nothing else. The bridges now emit it: `balanceText` on each account, `limitText`/`spentText` on a budget report, `amountText` on each report line, all rendered by `ledger::formatMoney`, which recovers the decimal value from the minor-unit count, wraps it in `Money<C>` for the currency -- the one place in this rung where the currency is a compile-time fact, and so the one place `morph::units` can type money at all -- and lets `morph::units::toDecimalString` produce the digits by exact integer long division. The exact triple is still published alongside, for a view that wants the parts. `toDecimalString` renders shortest-form, so $4.50 comes back as "4.5" and a zero balance as "0". `Quantity` has no fixed-fraction-width mode to ask for "4.50" instead; the rung renders shortest-form and records the gap as a finding in its README rather than growing a second, hand-rolled formatter next to the framework's. `BudgetQmlBridge::setBudgetLimit` also takes its scale from the currency the caller named instead of a hardcoded 2, so a JPY limit is counted in whole yen rather than being restated a hundredfold smaller by the model. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng it Done (#250) `RunReportJob` checked its own job row and threw `NotFound` for a job that does not exist, but never checked the ledger the job names. A job whose ledger had since been deleted therefore aggregated an empty account set, produced `[]`, and settled `Done` -- so a caller could not tell "no such ledger" from "a ledger with no activity", and the App's failure arm was unreachable that way. Every sibling action already has the guard: `OpenAccount`, `StoreTransaction`, `ImportLedgerChunk`, `SubmitReport` and `storeJournalImpl` all refuse a ledger they cannot find. The guard is raised *inside* the aggregation's own `try`, which is the part worth explaining. Throwing out of the method instead would leave the row `Pending`, and `App::runPendingReportsOnce` re-sweeps every `Pending` row on every pass -- the same doomed job would be re-dispatched forever, which is precisely the failure the existing characterisation test was written to guard against ("a row left Pending is what a poller spins on forever"). Raising it where the existing catch-all can see it settles the row `Failed`: terminal, and distinguishable from the empty-but-real report. The characterisation test moves with the behaviour it characterises, in this same commit, and now asserts `Failed` with no body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ReportJobPoller`'s dispatch-error callback re-implemented `morph::ladder::gui::errorText` and called `std::rethrow_exception(err)` with no null check. A default-constructed `std::exception_ptr` is a legal thing for a backend to hand an error callback, and rethrowing one is undefined behaviour -- in a `Completion` error callback, which is exactly where an escaping failure takes the process down rather than reaching the user. `examples/common/gui/error_text.hpp` documents that case and `error_text.cpp` handles it, and this rung's four presenters (`report_presenter`, `ledger_presenter`, `budget_presenter`, `rule_presenter`) already route through it. The copy is deleted. One behavioural difference, deliberate: the fallback text for a non-`std` exception becomes the helper's `"unknown error"` rather than this poller's own `"report status poll failed"`. Nothing pinned the old string, and one wording across the rung is worth more than a per-site one. The regression test drives a null `exception_ptr` through the error path, which previously would have been undefined behaviour rather than a message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
TransactionLeg::amountis amorph::math::Rationalused as a scaled-integer container —parseAmountputs minor units in the numerator and treatsdecimalPlacesas a scale hint. ButRational's contract says the opposite (rational.hpp:14: the precision tag "never changes a stored value";:34: comparison "ignoresdecimalPlacesentirely").The zero-sum check summed with plain
operator+and testedsum.numerator != 0, so scale never participated:"4.50"and"-45.0"sum to numerator 0 and were accepted, booking $4.50 against $45.00."4.5"/"-4.50"pair was refused.Both are pinned as tests driven through the real
StoreTransactionpath, written before the fix.The fix
ledger::restateMinorUnits(newledger/core/money.hpp) puts every leg on its account currency's scale before the zero-sum check and before any row is written. Exact-or-nothing: widening viacheckedMul(reports overflow rather than saturating), narrowing refused when it would drop a non-zero digit.$4.505in USD is aValidationError, never rounded.Why not
QuantityThe design spec asserted leg amounts should be
morph::units::Quantity. They cannot be:Quantity<auto U, uint32_t>takes its unit as a compile-time non-type template parameter, while a leg's currency is the account's runtime data —Money<Currency::USD>would stamp a false USD tag on every EUR/JPY leg. The spec reached this same conclusion two paragraphs later in §2, contradicting itself. The spec is corrected here rather than the code.Money<C>is used where the currency is known at the point of use:formatMoneyrenders throughmorph::units::toDecimalString, which is how the three hand-rolled IEEE-double divisions leave the QML.Also in this branch
RunReportJobon a vanished ledger settledDonewith"[]"; it now checks the ledger row like its five siblings do. The deliberate characterization test pinning the old behaviour is updated in the same commit.ReportJobPollerre-implementederrorTextwithout the null-exception_ptrguard (UB). Replaced with the shared helper the rung already uses in four other presenters.tests/test_ledger_rational_fuzz.cpp's first case claimed to prove the zero-sum check never false-positives, but never touches the model, and its operands are −$50.00 and +$0.50 under the rung's encoding. Rewritten to state what it actually measures.Verification
ladder_ledger_testsladder_common_testsmorph_testsClean under
-Weverything -Werror; each commit checked out and tested in isolation for bisectability. Not run: the full 1360-testctestset (needs bank/forms/TLS built), and no sanitizer configuration.Not fixed here
ImportLedgerChunkbypasses the zero-sum check entirely and can post across two currencies — filed as #306. A distinct defect on a distinct path; this branch does not close it.Part of #304 (§A1, §A5). Closes #250.
🤖 Generated with Claude Code