Skip to content

fix: preserve FIFO cost basis through depot transfers#59

Open
vfpdev wants to merge 5 commits into
ma4nn:masterfrom
vfpdev:fix/depot-transfer-fifo
Open

fix: preserve FIFO cost basis through depot transfers#59
vfpdev wants to merge 5 commits into
ma4nn:masterfrom
vfpdev:fix/depot-transfer-fifo

Conversation

@vfpdev

@vfpdev vfpdev commented May 24, 2026

Copy link
Copy Markdown

Problem

When securities are transferred between brokers using Portfolio Performance's Wertpapierübertrag feature, the FIFO cost basis was silently lost. Affected positions showed up with empty lots and no tax calculation.

Root cause was two separate issues:

  1. _get_remaining_lots_after_fifo_matching only processed SELL and DELIVERY_OUTBOUND as outgoing transactions. TRANSFER_OUT was ignored, leaving the original BUY lots stranded in the source account.

  2. prepare_share_sell_df filtered transactions by account_id, so only the destination account's transactions were passed to enrich_fifo_lots. The original BUY lots in the source account were never visible.

Fix

  • Process TRANSFER_OUTTRANSFER_IN pairs in the FIFO matching loop by moving lots from the source account to the destination account, preserving the original purchasePrice and fees.
  • Change prepare_share_sell_df to pass all transactions for a security across all accounts (filter_by_security instead of filter_by_account_and_security), deduplicated per security via seen_security_ids.

Tested with

A portfolio where Hugo Boss, PayPal, EasyJet, and Parrot were purchased at one broker and transferred to another via Wertpapierübertrag. Before the fix, simulate share-sell showed empty results for all four. After the fix, the original acquisition costs are correctly carried over.

Valeriano Ferreras Paz and others added 5 commits May 24, 2026 15:15
… TRANSFER_IN)

When securities are transferred between brokers using Portfolio Performance's
Wertpapierübertrag feature, PP records a TRANSFER_OUT from the source
depot and a TRANSFER_IN to the destination depot. The FIFO cost basis was
silently lost in this case because:

1. _get_remaining_lots_after_fifo_matching only processed SELL and
   DELIVERY_OUTBOUND as outgoing transactions — TRANSFER_OUT was ignored,
   leaving BUY lots stranded in the source account.
2. prepare_share_sell_df filtered transactions by account_id, so only the
   destination account's transactions were visible to enrich_fifo_lots.
   The original BUY lots in the source account were never seen.

Fix: process TRANSFER_OUT → TRANSFER_IN pairs in the FIFO matching loop by
moving lots from the source account to the destination account while
preserving the original acquisition cost (purchasePrice and fees). This
matches the German tax treatment under § 43a EStG, where cost basis carries
over unchanged on transfers between an investor's own accounts.

Also change prepare_share_sell_df to pass all transactions for a security
across all accounts (filter_by_security instead of filter_by_account_and_security),
deduplicated per security via seen_security_ids, so that the FIFO algorithm
can see the full transfer chain.
Exposes securities account transactions via:
- CLI: `pp-terminal view transactions [SECURITY] [--account-id] [--from] [--to] [--type]`
- MCP: `query_transactions(security, account_id, from_date, to_date, transaction_type)`

The core `prepare_transactions_df()` function applies filters for
security (by ISIN or UUID), account, date range, and transaction type,
then enriches each row with the resolved security name.

Covered by 14 unit tests and one integration test with a golden CSV.
Extract _build_transfer_in_lookup() and _consume_lots_fifo() from
_get_remaining_lots_after_fifo_matching() to bring cognitive complexity
below the allowed threshold of 15.
…arning

Extract shared ISIN/UUID resolution logic into resolve_security_id()
in portfolio.py. CLI uses it via InputError→BadParameter conversion,
MCP uses the existing _resolve_security() which also handles WKN.
Fixes duplicate-code warning from static analysis.

Also fix mypy type annotations in cost_basis.py (Any instead of object
for lot dicts, tuple[object, ...] for transfer lookup keys).
@sonarqubecloud

Copy link
Copy Markdown

@ma4nn

ma4nn commented May 25, 2026

Copy link
Copy Markdown
Owner

@vfpdev thanks for your contribution, very much appreciated 🙏

The ppxml2db dependency was updated to a (no longer existent) version, I reverted that change so the Github Action runners succeed.

Some issues I discovered during review, could you have a look at it please?

Perhaps it could make sense to split the TRANSFER fix and the view_transactions command/MCP into separate PRs, so we can release them independently.

Blocking

  1. FIFO ordering broken after transfer — cost_basis.py:129: remaining_lots.extend(transferred_lots) appends without re-sorting by date, so a transferred older lot ends up behind newer lots in the destination account. A subsequent SELL picks the wrong lot.
  2. simulate_share_sell ignores --account-id in result — simulate_share_sell.py:94: switching to all-account transactions is correct for FIFO matching, but never filters the output back to the requested account. test_share_sell_csv_output passes only because the kommer fixture has the queried security in only one securities account — a fixture with the same security in multiple accounts would surface the leak. Filter enrich_fifo_lots output by accountId == account_id before concat, and add a multi-account test case to the kommer fixture (or a new fixture).
  3. TRANSFER_IN lookup collapses same-day duplicates — cost_basis.py:44: dict key (date, securityId, round(shares, 4)) overwrites on identical pairs, dropping a leg's cost basis. Store a list, or key on the PP cross-entry UUID.
  4. No test covers the headline fix. Please add unit tests for (a) simple transfer preserves cost basis, (b) FIFO ordering after transfer (the 1. regression), (c) unmatched TRANSFER_OUT warns.
  5. Existing FIFO integration tests still use the old code path. tests/domain/test_fifo_sell_integration.py builds transactions via filter_by_account_and_security — so the production refactor in prepare_share_sell_df is untested by the integration suite. Please migrate these to exercise the new cross-account path.

Worth a look

  • view transactions --to excludes intra-day events (<= 00:00:00); also no from <= to validation.
  • CLI's resolve_security_id doesn't support WKN; MCP's _resolve_security does. Consolidate, since CLI/MCP parity was a stated goal.
  • CLI --type accepts a list; MCP transaction_type accepts a single string. Same parity gap — MCP should accept a list.
  • CLI uses strict %Y-%m-%d; MCP uses datetime.fromisoformat (accepts time components). Align input strictness.
  • Internal share-matching precision in cost_basis.py:44, :123 is hard-coded to 4 digits, decoupled from PP's _SCALE = 1e8 (pp_portfolio_builder.py:37). Round to 8 digits, ideally via a named constant tied to _SCALE.

Nice work on splitting _consume_lots_fifo / _build_transfer_in_lookup and on the warning log for unmatched TRANSFER_OUT.

Happy to discuss any of the above!

(Hand-reviewed, Claude linted my brain.)

@ma4nn ma4nn added bug Something isn't working enhancement New feature or request labels May 25, 2026
@ma4nn

ma4nn commented Jul 15, 2026

Copy link
Copy Markdown
Owner

As suggested I created the follow-up PR #73 for the depot transfer issue only ✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants