Valuation consistency, lots improvements, and positions JSON removal#1
Closed
triantos wants to merge 14 commits intodata-model-refactorfrom
Closed
Valuation consistency, lots improvements, and positions JSON removal#1triantos wants to merge 14 commits intodata-model-refactorfrom
triantos wants to merge 14 commits intodata-model-refactorfrom
Conversation
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
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 PR afadil#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 PR afadil#792 and should be merged after it.
Changes (14 commits)
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.