From 44e8c65df4ee103c3424945511fc1cce4b94bc32 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Thu, 13 Aug 2026 16:32:31 -0700 Subject: [PATCH] remove dead code --- src/commands/contributions.zig | 152 ++++++++++++--------------------- 1 file changed, 53 insertions(+), 99 deletions(-) diff --git a/src/commands/contributions.zig b/src/commands/contributions.zig index 73d71bd..7a95dd3 100644 --- a/src/commands/contributions.zig +++ b/src/commands/contributions.zig @@ -177,9 +177,12 @@ //! `new_cash` Change (a single cash delta can be drained by //! multiple records, which `kind` can't represent). Instead the //! matcher appends a synthetic `transfer_in` Change for the -//! Transfers section and records the attributed amount in -//! `Report.cash_attributed_by_account`, which the per-account totals -//! and attribution summary subtract from cash-side contributions. +//! Transfers section and accumulates the attributed amount onto each +//! consumed Change's `transfer_attributed` field. `attributedValue()` +//! then reports the unattributed residual, which is what the +//! per-account totals, the contributions sections, and audit's +//! large-lot filter all consume - one mechanism, so no two views of +//! the same attribution can drift apart. //! //! ### Which records does the matcher consider? //! @@ -1036,28 +1039,6 @@ pub fn findUnmatchedLargeLots( return .{ .lots = lots, .arena = arena_state }; } -/// Pure filter: pick out new-side Changes with `value() >= threshold` -/// and dupe their string fields into `arena`. Split out so tests can -/// feed a synthetic `[]Change` without running the git/IO pipeline. -/// -/// Cash-destination transfers don't flip their original `new_cash` / -/// `cash_contribution` Change (a single cash delta can be drained by -/// multiple records, which the kind field can't represent). Instead -/// the matcher records the attributed amount in -/// `cash_attributed_by_account`. Subtract that here so a fully- -/// attributed cash lot doesn't re-surface as "unmatched large lot." -/// A partially-attributed cash lot surfaces only on the residual, -/// matching the user's mental model: "this much of the lot is -/// already documented, check the rest." -/// -/// Deliberately excludes `partial_transfer_in`. A partial lot already -/// has an explicit transfer record acknowledging the large movement; -/// the unmatched residual is typically small (pre-existing cash that -/// topped the lot off) and surfacing it again would nag on something -/// the user has already documented. If a residual is large enough to -/// care about independently, the user can review the lot's full value -/// via `zfin contributions` - this filter's job is to catch -/// *unrecorded* large movements, not to re-flag partial ones. /// Pure filter: pick out new-side Changes whose unattributed value /// (`attributedValue()`) is at or above `threshold`, and dupe their /// string fields into `arena`. Split out so tests can feed a @@ -1131,8 +1112,6 @@ fn summarizeAttribution(ctx: ReportContext) AttributionSummary { // + cash_contribution (opt-in cash_delta) // + partial_transfer_in residual // (`value()` - `transfer_attributed`) - // - cash-dest transfer totals - // (from `cash_attributed_by_account`) // - DRIP: new_drip_lot + drip_confirmed + rollup_delta // `rollup_delta` is the ambiguous "share increased on a drip::false // lot" case. Lumping it with DRIP here matches the report's own @@ -1152,10 +1131,10 @@ fn summarizeAttribution(ctx: ReportContext) AttributionSummary { .partial_transfer_in => new_contributions += c.attributedValue(), else => {}, }; - // Note: cash-dest transfer attribution is already removed by - // `attributedValue()` on the per-Change side (matchCashDestination - // accumulates into `transfer_attributed`). No second subtraction - // off `cash_attributed_by_account` needed here. + // Cash-dest transfer attribution is already removed by `attributedValue()` on + // the per-Change side: `matchCashDestination` accumulates into + // `transfer_attributed`, so a fully-attributed cash Change contributes zero + // here without any separate per-account subtraction. return .{ .new_contributions = new_contributions, .drip = drip }; } @@ -1329,11 +1308,6 @@ const Report = struct { changes: []Change, /// Per-account rollups for the summary section. account_totals: std.StringHashMap(AccountTotal), - /// Per-account cash amounts matched to transfer records. Subtracted - /// from cash-side totals in the per-account summary so transferred - /// cash doesn't double-count. Keys borrow from Change.account - /// strings (arena-owned, same lifetime as the Report). - cash_attributed_by_account: std.StringHashMap(f64), const AccountTotal = struct { new_money: f64 = 0, // stock+cd+cash new lots (drip::false) @@ -1873,18 +1847,10 @@ fn computeReport( // Transfer reclassification pass: rewrite destination/source // Change kinds for records the caller passed in (typically the // diff between before-side and after-side - // `transaction_log.srf`), and accumulate per-account cash - // attribution so transferred cash doesn't double-count in - // per-account totals. No-op when no records are supplied. See + // `transaction_log.srf`). No-op when no records are supplied. See // `matchTransfers` docstring for the matching algorithm. - var cash_attributed_by_account: std.StringHashMap(f64) = .init(allocator); if (opts.transfer_log) |records| { - try matchTransfers( - allocator, - &changes, - &cash_attributed_by_account, - records, - ); + try matchTransfers(allocator, &changes, records); } // Intra-account purchase netting: a decrease in an account's cash @@ -1941,18 +1907,13 @@ fn computeReport( } } - // Note: cash-dest transfer attribution is already removed by - // `attributedValue()` on each cash-side Change (the matcher - // accumulates into `transfer_attributed`). No second subtraction - // off `cash_attributed_by_account` needed here. The bucket is - // still populated for downstream consumers (e.g. callers that - // want a per-account view of attributed transfers) but isn't - // used in the totals math. + // Cash-dest transfer attribution is already removed by `attributedValue()` on + // each cash-side Change - the matcher accumulates into `transfer_attributed`, + // so the residual is what reaches these totals. Nothing further to subtract. return .{ .changes = try changes.toOwnedSlice(allocator), .account_totals = acct_totals, - .cash_attributed_by_account = cash_attributed_by_account, }; } @@ -2042,10 +2003,10 @@ fn diffTransferLogs( /// cash_contribution summed) can cover the record's amount /// (minus any prior cash-dest records on the same account). /// Success appends a synthetic `transfer_in` Change for -/// display AND accumulates into -/// `cash_attributed_by_account[to]`, which the caller -/// subtracts from cash-side per-account totals. Failure -/// (budget underflow) emits `unmatched_transfer`. +/// display AND accumulates onto the consumed cash Changes' +/// `transfer_attributed`, so `attributedValue()` reports only +/// the unattributed residual. Failure (budget underflow) emits +/// `unmatched_transfer`. /// /// - For the `from` side: try to find a matching negative /// `cash_delta` or `lot_removed` on the sending account and @@ -2065,15 +2026,9 @@ fn diffTransferLogs( /// `transfer::DATE`. This allows a user to back-date a record /// (e.g. add a `transfer::2026-05-20` entry on 2026-05-23) and /// have it pair against the working-copy diff that introduced it. -/// -/// Populates `cash_attributed_by_account` (caller-owned) with the -/// per-account total of amounts matched to cash-destination records; -/// these amounts are subtracted from the cash bucket in the -/// per-account totals pass so transferred cash doesn't double-count. fn matchTransfers( allocator: std.mem.Allocator, changes: *std.ArrayList(Change), - cash_attributed_by_account: *std.StringHashMap(f64), records: []const transaction_log.TransferRecord, ) !void { // Bookkeeping: track which Change indices have already been @@ -2132,7 +2087,7 @@ fn matchTransfers( try matchLotDestination(allocator, changes, &consumed_lot_idx, rec, dl); }, .cash => { - try matchCashDestination(allocator, changes, &cash_budget, cash_attributed_by_account, &transfer_funding, rec); + try matchCashDestination(allocator, changes, &cash_budget, &transfer_funding, rec); }, } @@ -2295,14 +2250,17 @@ fn appendUnmatchedWithOwnedNote( /// Verify the `to` account's cash budget has capacity for this /// record, draw from it, and either attach to an existing cash -/// Change or append a synthetic one. The per-account attribution -/// bucket (`cash_attributed_by_account`) is what actually drives -/// totals math - the Change-level reclassification is for display. +/// Change or append a synthetic one. +/// +/// Attribution is recorded per-Change on `transfer_attributed`, which +/// `attributedValue()` turns into the unattributed residual. That single +/// mechanism drives every consumer - the totals math, the contributions +/// sections, and audit's large-lot filter - so there is no separate +/// per-account bucket to keep in agreement with it. fn matchCashDestination( allocator: std.mem.Allocator, changes: *std.ArrayList(Change), cash_budget: *std.StringHashMap(f64), - cash_attributed_by_account: *std.StringHashMap(f64), transfer_funding: *std.StringHashMap(FundingShortfall), rec: transaction_log.TransferRecord, ) !void { @@ -2333,15 +2291,6 @@ fn matchCashDestination( // capacity. if (budget_entry) |p| p.* -= credited; - // Accumulate into per-account attribution bucket. The per- - // account totals pass subtracts this from cash-side totals so - // transferred cash doesn't double-count. - const gop = try cash_attributed_by_account.getOrPut(rec.to); - if (!gop.found_existing) gop.value_ptr.* = 0; - // Only the cash that was actually observed; the rest is attributed to the - // new lots instead, so adding the full amount here would double-count. - gop.value_ptr.* += credited; - // Distribute the record amount across the destination account's // cash-side Changes by accumulating into each Change's // `transfer_attributed`. We deliberately do NOT flip the @@ -2355,10 +2304,10 @@ fn matchCashDestination( // return the unattributed residual - fully-attributed Changes // drop out of the "New contributions" section, audit's // "Large new lots" filter, and any other consumer that asks - // "how much of this Change is real new money?". The summary - // pass continues to use `cash_attributed_by_account` for its - // per-account math - the two views agree because the same - // amount is subtracted on both sides. + // "how much of this Change is real new money?". Every consumer - + // per-account totals, the contributions sections, audit's large-lot + // filter - reads that same residual, so there is no second view to + // keep in agreement. var remaining = credited; for (changes.items) |*c| { if (remaining <= 0) break; @@ -4930,9 +4879,9 @@ test "matchTransfers: cash-to-cash happy path" { // The new_cash Change stays as new_cash in the display (we don't // flip cash-side Changes because a single cash_delta can be // drained by multiple records). A synthetic transfer_in Change - // is appended for Transfers-section display, and - // `cash_attributed_by_account` carries the $5k subtraction for - // attribution math. + // is appended for Transfers-section display, and the $5k lands on + // the cash Change's `transfer_attributed` so `attributedValue()` + // reports the residual. var n_new_cash: usize = 0; var n_transfer_in: usize = 0; for (report.changes) |c| switch (c.kind) { @@ -5829,17 +5778,21 @@ test "matchTransfers: attribution excludes transferred amount" { .transfer_log = tlog.transfers, }); - // Replicate summarizeAttribution's logic directly. + // Replicate summarizeAttribution's logic directly. Uses + // `attributedValue()` on the new-side kinds, exactly as the real + // summary does: the per-Change residual IS the subtraction, so there + // is nothing further to net off. An earlier version of this test + // summed raw `value()` and then subtracted a separate per-account + // bucket - the superseded mechanism - which meant it reproduced an + // old formula rather than exercising the shipped one. var new_contributions: f64 = 0; var drip: f64 = 0; for (report.changes) |c| switch (c.kind) { - .new_stock, .new_cash, .new_cd, .new_option, .cash_contribution => new_contributions += c.value(), + .new_stock, .new_cash, .new_cd, .new_option, .cash_contribution => new_contributions += c.attributedValue(), .new_drip_lot, .drip_confirmed, .rollup_delta => drip += c.value(), .partial_transfer_in => new_contributions += c.attributedValue(), else => {}, }; - var cait = report.cash_attributed_by_account.iterator(); - while (cait.next()) |entry| new_contributions -= entry.value_ptr.*; try std.testing.expectApproxEqAbs(@as(f64, 0.0), new_contributions, 0.01); try std.testing.expectApproxEqAbs(@as(f64, 0.0), drip, 0.01); @@ -5994,7 +5947,7 @@ test "collectUnmatchedLargeLots: cash-destination matched is silent" { // Sample Trust funded by a transfer record dated 2026-05-20 was // surfacing in audit's "Large new lots - confirm source" because // the cash matcher doesn't flip the original `new_cash` Change's - // kind (it draws from `cash_attributed_by_account` instead). + // kind (the attribution rides on `transfer_attributed` instead). // Without subtracting that attribution, the audit filter // re-flagged a lot that's already explained. var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); @@ -6021,7 +5974,7 @@ test "collectUnmatchedLargeLots: cash-destination matched is silent" { // Cash-dest matching does NOT flip the original new_cash Change // (a single delta can be drained by multiple records). The // matcher records the attributed amount in - // `cash_attributed_by_account` instead. + // `transfer_attributed` instead. var saw_new_cash = false; var saw_synthetic_transfer = false; for (report.changes) |c| switch (c.kind) { @@ -6031,10 +5984,16 @@ test "collectUnmatchedLargeLots: cash-destination matched is silent" { }; try std.testing.expect(saw_new_cash); try std.testing.expect(saw_synthetic_transfer); - const attributed = report.cash_attributed_by_account.get("Sample Trust") orelse 0; - try std.testing.expectEqual(@as(f64, 73158.33), attributed); + // The attribution lands on the consumed cash Change, so the residual - not a + // separate per-account bucket - is what every consumer sees. Assert it there, + // which is also the value the audit filter below acts on. + var residual: f64 = 0; + for (report.changes) |c| { + if (c.kind == .new_cash) residual += c.attributedValue(); + } + try std.testing.expectApproxEqAbs(@as(f64, 0.0), residual, 0.01); - // The audit filter must subtract the attribution and stay quiet. + // The audit filter must see a fully-attributed lot and stay quiet. const lots = try collectUnmatchedLargeLots(allocator, report.changes, null); try std.testing.expectEqual(@as(usize, 0), lots.len); } @@ -6554,13 +6513,10 @@ test "printReport: empty report says no changes" { var w: std.Io.Writer = .fixed(&buf); var account_totals = std.StringHashMap(Report.AccountTotal).init(testing.allocator); defer account_totals.deinit(); - var cash_attr = std.StringHashMap(f64).init(testing.allocator); - defer cash_attr.deinit(); var changes = [_]Change{}; const report = Report{ .changes = changes[0..], .account_totals = account_totals, - .cash_attributed_by_account = cash_attr, }; try printReport(&w, &report, "portfolio.srf", false); const out = w.buffered(); @@ -6599,12 +6555,10 @@ test "printReport: full report renders every section and sub-printer" { try account_totals.put("Roth", .{ .new_money = 3800, .drip_confirmed = 475, .rollup = 0, .cash_delta = 0 }); try account_totals.put("CD Acct", .{ .cash_delta = 10500 }); try account_totals.put("", .{}); // exercises "(no account)" label + all-zero summary cells - const cash_attr = std.StringHashMap(f64).init(arena); const report = Report{ .changes = changes[0..], .account_totals = account_totals, - .cash_attributed_by_account = cash_attr, }; var buf: [16384]u8 = undefined;