Skip to content

d26bf998 - fix(ledger): drop unreachable fallbacks on the mark-to-market counts - #4487

Merged
TaprootFreak merged 3 commits into
developfrom
fix/ledger-mark-unreachable-fallbacks
Jul 30, 2026
Merged

d26bf998 - fix(ledger): drop unreachable fallbacks on the mark-to-market counts#4487
TaprootFreak merged 3 commits into
developfrom
fix/ledger-mark-unreachable-fallbacks

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

ledger-mark-to-market.service.ts sat at 92.3% branch coverage with two branches that no test could ever reach. They were ?? 0 guards over SQL COUNT(*) columns, which cannot be null. Removing them is the correct fix — and the one the project's rule against silent fallbacks asks for. With them gone the file reaches full coverage and is pinned.

Why the branches were unreachable

accountBalance selects both counts as aggregates without a GROUP BY:

.addSelect('COUNT(*)', 'legCount')
.addSelect('COUNT(leg.amountBaseUnits)', 'valuedCount')

Such a query always returns exactly one row, and COUNT returns 0 on an empty set — never NULL. So +(raw.legCount ?? 0) could only ever evaluate its left side. Covering the right side would have required mocking a row the database cannot produce, which proves nothing.

Change

  • The raw-row type now says what the query actually returns: legCount: string and valuedCount: string instead of string | null.
  • +(raw.legCount ?? 0) === +(raw.valuedCount ?? 0) becomes +raw.legCount === +raw.valuedCount.
  • The other two fallbacks in the same method stay: +(raw?.native ?? 0) and +(raw?.chf ?? 0) sit over SUM(...), which really does return NULL on an empty leg set. Their types keep | null.
  • The file is added to PINNED_LOGIC (421 → 422).
  • docs/coverage-gate.md records the new state. Its paragraph on unreachable branches no longer cites this file as an open case, since both examples it names have now been closed by deletion rather than by a test.

No behaviour changes: for every row the query can produce, the old and new expressions evaluate identically.

Precedent in the same subdomain

This is not a new convention. ledger-reconciliation.service.ts already types the same two columns as non-nullable string and compares them directly, without a guard — see its getRawOne generic and the two comparisons +row.legCount === +row.valuedCount (lines 334 and 549). This PR brings ledger-mark-to-market.service.ts in line with a pattern that is already established next to it.

PR completeness

Migration: none. Environment/infrastructure: none. Service/DTO/interface: no contract changes — the narrowed type describes an internal query result, not an exposed shape. Frontend synchronization: none.

Test plan

npm run test:gate:cov on this branch: exit 0 with all 422 thresholds, 336 suites / 6,207 tests. ledger-mark-to-market.service.ts reports 100/100/100/100 with branches at 22/22 — the two unreachable ones are gone and every remaining branch is covered. tsc --noEmit and prettier --check both clean. No test needed changing: no existing stub sets baseUnits without the two counts, so none relied on the removed guards.

legCount and valuedCount come from COUNT(*) aggregates without GROUP BY,
so getRawOne always returns a row and neither column can be null. The
`?? 0` guards could therefore never fire, and no test could reach them.
Narrow the raw row type to match what the query actually returns and
drop both guards; the SUM-based native and chf fallbacks stay, since
those really can be null on an empty leg set.

With the two unreachable branches gone the file reaches full coverage,
so pin it in the ratchet.
…vice

422 files pinned, totals refreshed against the commit they were measured
on. The paragraph on unreachable branches no longer points at
ledger-mark-to-market as an open case - both of its examples have since
been closed by deleting the fallback.
The gate run behind them was made on the branch, not on plain develop.
@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Closed out after two review passes across both dimensions — conformance and project rules on one side, logic and correctness against the coverage data on the other.

  • Pass 1 confirmed the core claim independently: the query aggregates without a GROUP BY, so getRawOne always returns exactly one row and COUNT never yields NULL. Notably, the reviewing pass established this from the query itself rather than by analogy to ledger-reconciliation.service.ts — that file's comparable query is built differently (GROUP BY + getRawMany), so the analogy would not have carried, even though its type annotations match. One minor issue was raised: the doc said the numbers were measured on plain develop, when the gate run actually included this PR's change.
  • Pass 2 confirmed that correction and found nothing further.

The wording deliberately names the develop base rather than the branch commit, since the latter disappears on squash-merge.

Coverage ratchet is green here, which re-proves all 422 thresholds against this branch merged with current develop.

@TaprootFreak
TaprootFreak marked this pull request as ready for review July 30, 2026 09:30
@TaprootFreak
TaprootFreak merged commit 372b47f into develop Jul 30, 2026
12 checks passed
@TaprootFreak
TaprootFreak deleted the fix/ledger-mark-unreachable-fallbacks branch July 30, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant