fix(portfolio): derive holding currency from assets, stop USD fallback poisoning #85

Merged

gertjan merged 1 commit from fix/holding-currency-from-assets into main

2026-07-14 18:11:59 +00:00

gertjan commented

2026-07-14 17:51:55 +00:00

Owner

Root cause

Every stored holding has currency="USD", including EUR/CHF-listed assets.

HoldingRepository.replace_for_portfolio (the sole holdings write path when
portfolio_behavior.transactions_authoritative=true, which is true in
prod) defaulted a symbol's currency to Currency.USD the first time it saw
that symbol, since its only source of currency was a pre-existing holding of
the same symbol:

currency = str(metadata.get("currency", Currency.USD.value))

Transactions carry no currency field at all (TransactionCreate,
ParsedTransaction from the DEGIRO parser), so on first import the USD
default was baked in — and every subsequent resync (create/update/delete of
any transaction, or a re-import) carried that same wrong value forward via
metadata_by_symbol, self-perpetuating the bug forever. create_holding
had a parallel issue: it overrides body.asset_type from the resolved asset
but never touched body.currency.

Blast radius

Display-only. Currency is never used in any position-sizing, P&L, or
analytics math in this codebase — it's a label on the holding response.
Existing bad data doesn't need a migration: the read-time fix below covers
it immediately, and the write-time fix self-heals it in Mongo on the next
resync (which happens on every transaction create/update/delete/import for
that portfolio).

Fix

(a) Read-time derivation — list_holdings, get_holdings_valuation,
and get_holding in src/api/routes/portfolios.py already look up assets by
symbol to build a full_name map; the same lookup now also overrides each
holding's served currency from assets.currency when known, falling back
to the stored value when the asset can't be resolved. This fixes what users
see for already-poisoned holdings without touching the database.

(b) Write-path fix (self-healing storage) — HoldingRepository.replace_for_portfolio
now accepts a currency_by_symbol map and applies precedence
assets.currency > prior stored currency > Currency.USD.value (previously
just prior stored currency > USD, with no asset lookup at all).
TransactionRepository.sync_holdings_from_transactions takes an optional
asset_repo and builds that map from the projected positions before
calling replace_for_portfolio. Every route that triggers a resync
(create_transaction, update_transaction/put_transaction,
delete_transaction, import_transactions) now depends on AssetRepo and
threads it through — import_transactions already had the dependency for
ISIN resolution, the others gained it. create_holding also now overrides
body.currency from the resolved asset next to the existing asset_type
override.

No migration needed: every future resync — which happens automatically on
essentially every write to a portfolio's transaction ledger — rewrites the
holding with the correct currency. Combined with the read-time override,
users never see a wrong currency, and storage self-heals within the normal
write path.

Tests

tests/unit/test_repositories_portfolios.py (new): replace_for_portfolio
currency precedence — first-time symbol + known asset -> asset currency;
first-time symbol + unknown asset -> USD fallback; existing bad-USD
holding + known asset -> self-heals to asset currency; existing holding +
unknown asset -> prior stored currency preserved (regression guard).
tests/unit/test_routes_portfolios.py: extended
test_create_holding_auto_resolves_asset_type to also assert the currency
override (submits USD, asset is EUR, persisted body and response are EUR);
added test_list_holdings_currency_overridden_from_asset,
test_get_holding_currency_overridden_from_asset, and
test_get_holding_keeps_stored_currency_when_asset_unknown; updated the
transaction-route tests (create/patch/put/delete) to override
get_asset_repository now that those routes depend on it.

ruff check src/, mypy src/, and pytest -m "not integration" are all
green (1069 passed).

Co-Authored-By: Claude Fable 5 noreply@anthropic.com

## Root cause Every stored holding has `currency="USD"`, including EUR/CHF-listed assets. `HoldingRepository.replace_for_portfolio` (the sole holdings write path when `portfolio_behavior.transactions_authoritative=true`, which is `true` in prod) defaulted a symbol's currency to `Currency.USD` the first time it saw that symbol, since its only source of currency was a pre-existing holding of the same symbol: ```python currency = str(metadata.get("currency", Currency.USD.value)) ``` Transactions carry no currency field at all (`TransactionCreate`, `ParsedTransaction` from the DEGIRO parser), so on first import the USD default was baked in — and every subsequent resync (create/update/delete of any transaction, or a re-import) carried that same wrong value forward via `metadata_by_symbol`, self-perpetuating the bug forever. `create_holding` had a parallel issue: it overrides `body.asset_type` from the resolved asset but never touched `body.currency`. ## Blast radius Display-only. Currency is never used in any position-sizing, P&L, or analytics math in this codebase — it's a label on the holding response. Existing bad data doesn't need a migration: the read-time fix below covers it immediately, and the write-time fix self-heals it in Mongo on the next resync (which happens on every transaction create/update/delete/import for that portfolio). ## Fix **(a) Read-time derivation** — `list_holdings`, `get_holdings_valuation`, and `get_holding` in `src/api/routes/portfolios.py` already look up assets by symbol to build a `full_name` map; the same lookup now also overrides each holding's served `currency` from `assets.currency` when known, falling back to the stored value when the asset can't be resolved. This fixes what users see for *already-poisoned* holdings without touching the database. **(b) Write-path fix (self-healing storage)** — `HoldingRepository.replace_for_portfolio` now accepts a `currency_by_symbol` map and applies precedence `assets.currency > prior stored currency > Currency.USD.value` (previously just `prior stored currency > USD`, with no asset lookup at all). `TransactionRepository.sync_holdings_from_transactions` takes an optional `asset_repo` and builds that map from the projected positions before calling `replace_for_portfolio`. Every route that triggers a resync (`create_transaction`, `update_transaction`/`put_transaction`, `delete_transaction`, `import_transactions`) now depends on `AssetRepo` and threads it through — `import_transactions` already had the dependency for ISIN resolution, the others gained it. `create_holding` also now overrides `body.currency` from the resolved asset next to the existing `asset_type` override. No migration needed: every future resync — which happens automatically on essentially every write to a portfolio's transaction ledger — rewrites the holding with the correct currency. Combined with the read-time override, users never see a wrong currency, and storage self-heals within the normal write path. ## Tests - `tests/unit/test_repositories_portfolios.py` (new): `replace_for_portfolio` currency precedence — first-time symbol + known asset -> asset currency; first-time symbol + unknown asset -> USD fallback; existing bad-USD holding + known asset -> self-heals to asset currency; existing holding + unknown asset -> prior stored currency preserved (regression guard). - `tests/unit/test_routes_portfolios.py`: extended `test_create_holding_auto_resolves_asset_type` to also assert the currency override (submits USD, asset is EUR, persisted body and response are EUR); added `test_list_holdings_currency_overridden_from_asset`, `test_get_holding_currency_overridden_from_asset`, and `test_get_holding_keeps_stored_currency_when_asset_unknown`; updated the transaction-route tests (create/patch/put/delete) to override `get_asset_repository` now that those routes depend on it. `ruff check src/`, `mypy src/`, and `pytest -m "not integration"` are all green (1069 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

gertjan added 1 commit

2026-07-14 17:51:55 +00:00

fix(portfolio): derive holding currency from assets, stop USD fallback poisoning

Deploy / check (pull_request) Successful in 3m15s

Details

Deploy / deploy (pull_request) Has been skipped

Details

4d02d97dac

replace_for_portfolio defaulted every first-seen symbol's currency to USD and
then carried that value forward on every resync, so any EUR/CHF-listed asset
synced via transactions (e.g. DEGIRO import) stayed mislabeled USD forever.

Fix precedence in replace_for_portfolio to assets.currency > prior stored
currency > USD fallback, threading a symbol->currency map (built from
AssetRepository) through sync_holdings_from_transactions from every route
that triggers a resync. Also override HoldingCreate.currency from the
resolved asset in create_holding, and override the served currency from the
assets collection at read time in list_holdings, get_holdings_valuation, and
get_holding so already-poisoned stored holdings display correctly before
their next resync self-heals them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

gertjan force-pushed fix/holding-currency-from-assets from 4d02d97dac

Deploy / check (pull_request) Successful in 3m15s

Details

Deploy / deploy (pull_request) Has been skipped

Details

to 8c6b51e8e2

Deploy / check (pull_request) Successful in 3m18s

Details

Deploy / deploy (pull_request) Has been skipped

Details

2026-07-14 18:06:26 +00:00

Compare