Valuation consistency, lots improvements, and positions JSON removal#820
Closed
triantos wants to merge 37 commits intoafadil:mainfrom
Closed
Valuation consistency, lots improvements, and positions JSON removal#820triantos wants to merge 37 commits intoafadil:mainfrom
triantos wants to merge 37 commits intoafadil:mainfrom
Conversation
Introduces the `lots` table — the persistent, relational form of tax lots. Each row represents one acquisition with its own cost basis, open date, and disposal tracking. Initially empty; the holdings calculator will begin shadow-writing lot rows alongside existing JSON snapshots once the lot repository is wired in. Changes: - Migration: CREATE TABLE lots with indexes (account_id/asset_id, asset/open, account/open, open_activity_id) - schema.rs: add lots table definition and joinables to accounts/assets - crates/core/src/lots/: new LotRecord, DisposalMethod, HoldingPeriod types No behavioral changes. The existing JSON snapshot path is untouched.
After each holdings recalculation the snapshot service writes a row to the `lots` table for every open lot in every position, in parallel with the existing JSON snapshot path. The JSON snapshots remain authoritative; the lot rows exist so both representations can be compared. Any quantity mismatch between the two is logged at ERROR severity. The lot repository is opt-in via SnapshotService::with_lot_repository(), so existing callers are unaffected until they wire it in. open_activity_id is left NULL for now — transferred sub-lots use composite IDs that don't correspond to rows in the activities table.
Both apps (Tauri desktop and server) now pass a LotsRepository to SnapshotService, so lot rows are written on every holdings recalculation. For existing users, the lots table starts empty. On desktop startup, backfill_lots_if_needed() checks the row count and triggers a full holdings recalculation if the table is empty, ensuring lot rows are populated without any manual action. Adds count_open_lots() to LotRepositoryTrait to support the check.
…rite Unit tests (core::lots): - extract_lot_records: AAPL (3 lots), LQD bond ETF (2 lots), AAPL Jun 2026 \$200 call option (1 lot), mixed portfolio totalling all three - check_lot_quantity_consistency: passing case and mismatch detection Integration tests (storage-sqlite::lots): - replace_lots_for_account: inserts, replaces, and clears correctly - replace_only_affects_target_account: other accounts' rows are untouched Snapshot service test: - MockLotRepository records calls; verifies that after a full recalculation with AAPL (3 buys → 100 shares), LQD (2 buys → 150 shares), and one options position, the lot repository receives the correct quantities
…lots table Security positions now sourced from lots JOIN assets instead of snapshot JSON. contract_multiplier from asset.contract_multiplier(), currency from asset.quote_ccy. Cash balances still come from snapshot. Verified on 18-account test DB: 35 securities, quantities, cost bases, and market values all match pre-switch baseline exactly.
Replace replace_lots_for_account (wipe+reinsert) with sync_lots_for_account (upsert open, mark closed) so lot rows are never deleted. Fully consumed lots are stamped with is_closed=1, close_date, and close_activity_id sourced from the disposing activity's date and ID. - FifoReductionResult gains fully_consumed_lot_ids to surface which lots drained to zero during a reduction - HoldingsCalculator records LotClosure entries for all three reduction sites: handle_sell, handle_transfer_out, handle_adjustment (OPTION_EXPIRY) - snapshot_service clears the disposed_lots cache before each run and passes closures to sync_lots_for_account alongside the open lots - storage-sqlite implements sync_lots_for_account: row-by-row upsert (SQLite limitation) + UPDATE for each closure - New test: sync_lots_records_closures_for_full_sell verifies a fully sold lot appears in closures with the correct date/activity, not in open lots
Adds two new LotRepositoryTrait methods: - get_lots_as_of_date(account_ids, date): lots active on a given date - get_all_lots_for_account(account_id): all lots (open + closed) for memory-side date filtering Switches NetWorthService.get_net_worth to read security positions from the lots table instead of snapshot.positions. Cash balances still come from snapshots (extra state, will be removed once cash is tracked independently of snapshots).
… lots ValuationService.calculate_valuation_history now fetches all lots for the account once via get_all_lots_for_account and filters per date in memory, replacing iteration over snapshot.positions for securities. Currency, is_alternative, and contract_multiplier are still read from the snapshot's position entries (extra state carried until ValuationService gains direct asset-repo access). Cash positions are unaffected.
Changes the holdings_from_snapshot signature from (&snapshot, base_currency) to (account_id, date, base_currency). Security positions now come from get_lots_as_of_date; cash balances still fetched from the snapshot for that date (extra state carried until cash is tracked independently). Also includes individual lot detail in the returned holdings (display_lots), which was absent in the snapshot-based version.
Mirrors the Tauri backfill_lots_if_needed logic: on startup, count open lots and if zero, run a full holdings recalculation to populate the table. Runs non-blocking in a tokio::spawn so it doesn't delay server readiness. Adds lots_repository to AppState so the scheduler can check the count.
Two callsites called per-account lot queries with account_id="TOTAL",
which has no rows in the lots table, causing:
- build_live_holdings_from_snapshot: returned 0 securities, leaving
only the aggregate cash balance (-$14.8M) as live holdings
- calculate_valuation_history: investment_market_value=0, making the
TOTAL daily valuation equal to the (negative) cash balance
Add get_all_open_lots() and get_all_lots() to LotRepositoryTrait and
use them in holdings_service and valuation_service respectively when
account_id == "TOTAL", so both paths aggregate lots across all accounts.
When save_manual_snapshot is called for the latest snapshot on an account, synthesize one LotRecord per position (using snapshot date, quantity, average cost, and total cost basis from the position) and call replace_lots_for_account. This makes HOLDINGS-mode accounts visible in lot-backed holdings and valuation queries.
Add `original_quantity` field to the snapshot Lot struct, set at creation time in add_lot, add_lot_values, and add_transferred_lots. Never modified by reduce_lots_fifo or apply_split, so it preserves the as-acquired quantity through all subsequent mutations. extract_lot_records now writes original_quantity (the acquisition amount) and remaining_quantity (the current amount) as distinct values to the lots table. Old snapshots that predate this field deserialize original_quantity as zero via serde(default); extract_lot_records falls back to remaining_quantity in that case. This is the prerequisite for historical as-of queries that replay activities forward from each lot's original_quantity anchor.
The startup lot backfill calls recalculate_holdings_snapshots(None, Full) which only processes TRANSACTIONS-mode accounts. HOLDINGS-mode accounts were permanently skipped because the lots table became non-empty after the first backfill, suppressing future runs. Add backfill_lots_for_holdings_accounts() to SnapshotServiceTrait. It iterates HOLDINGS-mode accounts and synthesizes lots from each account's latest snapshot via the existing refresh_lots_from_latest_snapshot method. Both server and Tauri schedulers now call this after the activity-replay backfill, ensuring all account types have lot rows on first launch.
… delete sync_lots_for_account now deletes lots for the account that weren't produced by the current recalculation. Orphans arise when activities are deleted (FK SET NULL) and rebuilds create new lots with new IDs. Also adds migration to change open_activity_id FK from ON DELETE SET NULL to ON DELETE CASCADE, so activity deletion immediately removes lots. The migration cleans up existing orphans during the table rebuild.
When a sell activity is deleted and lots are recalculated, the lot is re-emitted as open. The ON CONFLICT DO UPDATE clause was only updating remaining_quantity and total_cost_basis, leaving is_closed=1 and close_date set from the prior state. The lot remained invisible to open-lot queries. Add is_closed, close_date, and close_activity_id to the upsert SET clause so that reopened lots are correctly marked as open.
The lots table stores current state (remaining_quantity is mutated in place by sells). Historical as-of queries were returning current quantities for past dates — buy 10, sell 4, query before the sell returned 6 instead of 10. Add replay_lots_to_date() in lots/mod.rs: resets each lot to original_quantity, then replays Sell/TransferOut/Adjustment/Split activities in chronological FIFO order up to the requested date. 7 unit tests cover partial sell, full sell, FIFO across lots, splits, split+sell, and cross-account isolation. Inject activity_repository into HoldingsService and NetWorthService. Both get_lots_as_of_date call sites now fetch activities and replay. Also fixes the SQL bug in the storage method: .assume_not_null() on nullable close_date was dropping open lots (NULL > 'x' is NULL in SQL). Replaced with is_not_null().and(close_date.gt(date)).
The is_latest check in save_manual_snapshot used get_latest_snapshot_before_date(account_id, snapshot_date + 1) which returned the just-saved snapshot itself, making the condition always true. Historical HOLDINGS snapshots would overwrite lots from the latest snapshot. Replace with a check for any snapshots after the saved date. If none exist, this is the latest and lots should be written.
…ntly coercing to zero Multiple lot readers used unwrap_or_default() or filter_map with .ok() to parse Decimal fields from TEXT columns. Malformed values silently produced zero quantities or dropped lots from display with no signal. Replace with unwrap_or_else + log::error! at all lot parse sites in holdings_service, net_worth_service, and valuation_service. The filter_map in lot_records_to_display_lots now uses inspect_err to log before dropping.
The method counts all rows in the lots table (open and closed), not just open lots. The name was misleading after is_closed was introduced. Callers only use it to check "is the table empty" for the startup backfill guard.
The handler was rewritten to read security positions from the lots table, which silently returned an empty Vec when no data exists at the requested date. Restore the original behavior of erroring on missing data, returning 404 Not Found via ApiError::NotFound.
build_assets_section silently dropped categories aggregating to ≤ 0. Replace with build_balance_sheet_sections that routes negative non-liability categories to the liabilities section with positive magnitude. Adds 7 unit tests for the new function.
…ents tab Add alternative_market_value to DailyAccountValuation, computed from the already-calculated performance_eligible_value in the valuation calculator. The Investments tab account cards now subtract this value so they show only tradable securities + cash, matching the header filter behavior and industry convention (Empower, Kubera, etc.). Migration adds nullable column with default '0' to daily_account_valuation.
…luation table Portfolio-level daily valuations now live in a dedicated table instead of as fake account rows in daily_account_valuation. Portfolio values are derived by summing per-account valuations (option B), not independently recalculated from lots. Migration creates the table, backfills from existing TOTAL rows, and deletes the old rows. All reads/writes transparently route via the valuation service. TOTAL holdings snapshots are kept for now (quote sync planning depends on them).
PortfolioUpdateTrigger now displays a visible "As of [date]" badge next to the balance when calculatedAt is more than 24 hours old. Previously the date was only visible on hover. Applies to both the dashboard and account detail pages.
… status New LotView display type converts directly from LotRecord, separate from the calculation Lot struct. Holding gains a lotDetails field populated with all lots (open + closed) on the asset detail page. Frontend shows open/closed badges, original vs remaining quantity, close dates, and groups by account with collapsible sections when an asset is held across multiple accounts. 4 unit tests for LotView.
Allows linking alternative assets (liabilities, property, etc.) to the account they belong to. NULL for unlinked assets (house, gold in safe) and for investment assets (linked via activities/lots instead).
Backend: account_id plumbed through CreateAlternativeAssetRequest, UpdateAssetDetailsRequest, repository trait, and storage layer. Frontend: account selector added to both the quick-add modal (create) and the asset details sheet (edit). "None" option clears the link.
…T_ID constant Scattered string comparisons against "TOTAL" across 7 files replaced with the existing constant from crate::constants. Reduces divergence risk if the ID is ever renamed.
…d of snapshot positions ValuationService read currency, is_alternative, and contract_multiplier from snapshot.positions as a fallback when building lot-derived positions. This was the last snapshot-positions dependency in the valuation pipeline. Inject asset_repository into ValuationService and pre-fetch asset metadata for all lot-referenced assets. The valuation calculator now runs on data sourced entirely from the lots table + assets table, with no JSON snapshot position reads.
The holdings_snapshots.positions column stored serialized JSON blobs of
security positions with nested lots. This data now lives relationally
in the lots and assets tables.
Stop writing positions to the DB column and stop reading them back.
The in-memory positions field remains on AccountStateSnapshot for the
HoldingsCalculator pipeline (working state during activity replay).
Replace all read-side consumers:
- Valuation history: gather FX pairs and asset metadata from the assets
table instead of snapshot positions. Move lot+asset fetch earlier in
calculate_valuation_history so quote and FX prefetch use correct data.
- Quote sync (5 call sites): query lot_repository.get_open_position_quantities()
for the asset_id->quantity map needed by quote sync planning.
- Tauri get_snapshot_by_date: delegate to holdings_service.holdings_from_snapshot()
matching the server handler, removing 130 lines of duplicated code.
- Broker reconciliation: build prior positions from lots table, refactor
compute_holdings_diff to compare position maps directly, replace
is_content_equal with diff.is_unchanged() + cash equality check.
- Snapshot listing: derive position_count from open lots per account.
Migration clears existing positions JSON: UPDATE holdings_snapshots SET positions = '{}';
Fully-consumed lots (buy + full sell in one recalc pass) were never written to the DB because LotClosure only carried the lot ID, and sync_lots_for_account did a bare UPDATE that silently affected 0 rows. Enriched LotClosure with full lot data (account_id, asset_id, open_date, original_quantity, cost_per_unit, total_cost_basis, fee_allocated) and added fully_consumed_lots to FifoReductionResult to capture lot state before VecDeque removal. Changed the closure path in sync_lots_for_account to INSERT ON CONFLICT UPDATE so new closed lots are created even if they were never previously written. Verified on real data: 778 closed lots created (was 0), zero assets with missing lot rows (was 228), all 630 open lots unchanged.
Removed the redundant `lots: Option<VecDeque<Lot>>` field from the Holding API struct and the lot_records_to_display_lots() converter that populated it. The lot_details field (Option<Vec<LotView>>) — which reads from the same lots table but includes open/closed status, account ID, and original quantity — is now the sole lot display path. apply_fx_to_holding now adjusts lot_details instead of lots. Frontend: removed LegacyLotsView fallback and lots prop from AssetLotsTable.
The positions column was already dead — reads returned empty HashMap,
writes stored '{}'. This migration drops it entirely and removes all
references from the Diesel model, raw SQL queries, and test fixtures.
The in-memory `positions` field on AccountStateSnapshot remains — it's
the working state during the holdings calculation pipeline, never
persisted.
Bash script that creates 3 test accounts (USD TRANSACTIONS, HOLDINGS-mode, EUR TRANSACTIONS) with multi-lot FIFO sells, options, alternative assets, and HOLDINGS-mode manual snapshots. Captures 37 API endpoint responses before and after a code change and diffs the results, with categorized expected differences (FX drift, new fields, position count, quantity fix). Used to verify that lots-related refactors don't introduce regressions across live holdings, snapshots, valuation history, historical as-of queries, net worth, performance, and alternative holdings. Usage: ./scripts/test-lots-migration.sh setup # create test data ./scripts/test-lots-migration.sh baseline # capture (old code) ./scripts/test-lots-migration.sh verify # capture + diff (new code) ./scripts/test-lots-migration.sh cleanup # remove test data
Contributor
Author
|
Closing temporarily — found a regression in the positions JSON column drop that breaks incremental recalc starting state. The empty positions on DB-loaded snapshots cause subsequent incremental recalcs to start from an empty state, which produces empty results and (combined with the wipe-on-empty bug in sync_lots_for_account) wipes lots for affected accounts. Will investigate and resubmit. |
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.
Summary
Builds on the materialized lots table from #792 to fix valuation
consistency issues, improve the lot viewer, eliminate the dead positions
JSON column, and clean up loose ends discovered during testing.
Depends on #792 and should be merged after it. Until #792 merges,
the diff here will include both PRs' changes.
Changes (14 commits on top of #792)
Valuation consistency
the sum of account card totals
daily_portfolio_valuationtableLots improvements
dropped because
sync_lots_for_accountdid a bare UPDATE that affected0 rows when the lot had never been INSERTed)
lots: Option<VecDeque<Lot>>field from theHoldingAPI struct and use
lot_details(LotView) exclusivelySchema and account linking
account_idFK to assets tablePositions JSON removal
snapshot.positionspositionsJSON column (quote sync, brokerreconciliation, Tauri
get_snapshot_by_date, valuation history)positionscolumn fromholdings_snapshotsentirely (migration)"TOTAL"string literals withPORTFOLIO_TOTAL_ACCOUNT_IDTest infrastructure
scripts/test-lots-migration.shfor end-to-end API verificationVerification
Tested against real production data on a 24-account portfolio with mixed
TRANSACTIONS and HOLDINGS-mode accounts including bonds, T-bills, options,
and multi-currency positions.
Consumed lots fix — recalculated full portfolio:
End-to-end API test (
scripts/test-lots-migration.sh):historical holdings at 5 dates spanning the activity range, net worth,
performance, and alternative holdings
manual snapshot, multi-lot FIFO with partial sells, cross-currency
positions, alternative assets, and TOTAL aggregation
Migration —
positionscolumn drop verified on real database viaPRAGMA table_info.All 892 unit + integration tests pass. Clippy clean.