From 07524d0d84c3afdd6325e15dc2c96e4260ff8a32 Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Fri, 28 Aug 2026 07:53:33 +0200 Subject: [PATCH] fix(holdings): a transfer must not set a cost basis (#3154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(holdings): a transfer must not set a cost basis calculate_avg_cost sums every trade with a positive quantity, so an asset moved in from elsewhere is counted as bought on the day it arrived. A coin acquired at 30k and transferred in at 60k reports a cost of 60k and no gain at all — a number that looks authoritative and is wrong. Nothing here can know what a transferred asset cost: the purchase happened somewhere this app never saw. Leaving the cost unknown is what the method already does when it has nothing to work from, and for the same stated reason the fallback to market price was removed from it: "Previously this fell back to current market price, which was misleading." Two things it would be easy to get wrong, and both are covered: - **One transfer makes the whole position unknown**, not just its own row. Averaging the purchases alone and applying that to every unit is the same fabrication in a quieter form: buy one at 30k, receive one, and the position reports 30k a unit for two units that did not cost that. - **Unlabelled purchases are preserved.** `!=` is NULL for a row with no label, so a naive exclusion would drop the ordinary trades that carry none — which is most of them. Hence IS DISTINCT FROM. Balances and value are unaffected: they come from holdings, which providers import from the position itself rather than from trade history. This reaches every integration that labels a movement as a transfer. Questrade journals already did; the self-custody wallets do as of #3153. Co-Authored-By: Claude Opus 5 * fix(holdings): stop a stored figure outranking the transfer guard Review on #3154, and the reviewers were right that the first pass only covered half the path. `Holding#avg_cost` returns a stored `cost_basis` before it ever calls `calculate_avg_cost`, so the transfer guard was protecting only holdings that had nothing stored. Worse, the stored value was itself wrong: both calculators counted every positive-quantity trade toward the running average, transfers included, and the materializer persisted that as a `calculated` basis. A coin bought elsewhere at 30k and moved in at 60k reported no gain at all, and said so with a figure that looks derived. Fixed in the write path rather than the read one. Adding an `exists?` per holding to `avg_cost` would have reintroduced exactly the N+1 the stored value exists to avoid; clearing the stored value instead lets the read path fall through to the guard that was already there. Both calculators now exclude transfers from the average and mark the security's basis unknown — the forward one for good, the reverse one from the transfer's date onward, since the purchases before it still stand on their own. `cost_basis_unknown` is carried separately from a nil `cost_basis` because the materializer treats them differently: nil means "nothing computed, leave what is there", unknown means "this cannot be known, clear what is there". A `manual` or `provider` basis survives. That is somebody asserting what the position cost them, which is precisely the thing the app cannot derive for a transfer. The migration clears figures already stored. Positions heal on the next materialization anyway, but a manual or disconnected account may not materialize again for a long time, and the wrong number is not visibly wrong. The regression test materializes first and relabels after, because that is the case that matters: a position already carrying a figure worked out before anyone knew the movement was a transfer. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(holdings): clear a transferred basis that recorded no source Follow-up on the same review. `load_existing_holdings_map` loaded holdings that were locked, sourced, or provider-owned — so a row carrying a `cost_basis` with no `cost_basis_source` was invisible to it. The clearing then saw no existing holding and left the figure standing, which meant the rows least able to justify the number they hold were the ones that kept it. The migration takes `[7.2]` to match `schema.rb` and the other 398 migrations, rather than the `[8.0]` I had written. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(holdings): renumber the migration off a colliding version `20260825120000` is already taken by `add_consumed_amount_to_goals` on the goals stack. Two migrations sharing a version is not a merge conflict — `schema_migrations` is keyed by it, so whichever landed second would be recorded as already run and skipped in silence. For a data migration that means transferred positions quietly keeping the cost basis this branch exists to clear. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * docs(holdings): say why the basis guard keys on one label, not the set #3192 landed Trade::INTERNAL_MOVEMENT_LABELS in this file, next to the TRANSFER_LABEL this branch adds. Both rest on ownership being preserved, so two constants sitting together invite the question of why the basis guard does not simply use the broader one. It could, and that would be a behaviour change: the sweep labels would start clearing a cost basis too. Nothing has shown a sweep landing on a security, and widening a guard that erases figures on the strength of a guess is the wrong direction, so it stays narrow and now says so. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 --- app/models/holding.rb | 17 +++++ app/models/holding/forward_calculator.rb | 17 ++++- app/models/holding/gapfillable.rb | 3 +- app/models/holding/holding_data.rb | 6 +- app/models/holding/materializer.rb | 26 ++++++++ app/models/holding/reverse_calculator.rb | 20 +++++- app/models/trade.rb | 11 ++++ ...0_clear_transferred_position_cost_bases.rb | 40 +++++++++++ db/schema.rb | 2 +- test/models/holding/materializer_test.rb | 66 +++++++++++++++++++ test/models/holding_test.rb | 61 +++++++++++++++++ 11 files changed, 264 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20260826090000_clear_transferred_position_cost_bases.rb diff --git a/app/models/holding.rb b/app/models/holding.rb index ac8c37c26..57b5debb5 100644 --- a/app/models/holding.rb +++ b/app/models/holding.rb @@ -290,6 +290,23 @@ class Holding < ApplicationRecord .where(security_id: security.id) .where("trades.qty > 0 AND entries.date <= ?", date) + # A transfer is not a purchase: coins moved in from elsewhere were + # acquired at a price nothing here knows, so treating the day they + # arrived as their cost states a number that looks authoritative and is + # wrong — a coin bought at 30k and moved in at 60k would report no gain. + # + # One transfer makes the whole position unknown, rather than just its own + # row. Averaging the purchases alone and applying that to every unit is + # the same fabrication in a quieter form: buy one at 30k, receive one, and + # the position would report 30k a unit for two units it did not cost that. + return nil if trades.where(investment_activity_label: Trade::TRANSFER_LABEL).exists? + + # IS DISTINCT FROM, because `!=` is NULL for an unlabelled row and would + # drop the ordinary purchases that carry no label at all. + trades = trades.where( + "trades.investment_activity_label IS DISTINCT FROM ?", Trade::TRANSFER_LABEL + ) + total_cost, total_qty = trades.pick( Arel.sql("SUM(trades.price * trades.qty * COALESCE(exchange_rates.rate, 1))"), Arel.sql("SUM(trades.qty)") diff --git a/app/models/holding/forward_calculator.rb b/app/models/holding/forward_calculator.rb index 4b16ea54e..fdc8e05f7 100644 --- a/app/models/holding/forward_calculator.rb +++ b/app/models/holding/forward_calculator.rb @@ -6,6 +6,10 @@ class Holding::ForwardCalculator @security_ids = security_ids # Track cost basis per security: { security_id => { total_cost: BigDecimal, total_qty: BigDecimal } } @cost_basis_tracker = Hash.new { |h, k| h[k] = { total_cost: BigDecimal("0"), total_qty: BigDecimal("0") } } + # Securities whose position has taken in a transfer. A coin moved in was + # acquired at a price nothing here knows, and averaging the purchases alone + # would apply their price to units that never cost it. + @transferred_security_ids = Set.new end def calculate @@ -72,7 +76,8 @@ class Holding::ForwardCalculator price: price.price, currency: price.currency, amount: qty * price.price, - cost_basis: cost_basis_for(security_id, price.currency) + cost_basis: cost_basis_for(security_id, price.currency), + cost_basis_unknown: @transferred_security_ids.include?(security_id) ) end.compact end @@ -85,6 +90,14 @@ class Holding::ForwardCalculator next unless trade.qty > 0 # Only track buys security_id = trade.security_id + + # A transfer is not a purchase: it contributes no cost and it makes the + # whole position unknowable, not just its own units. + if trade.investment_activity_label == Trade::TRANSFER_LABEL + @transferred_security_ids << security_id + next + end + tracker = @cost_basis_tracker[security_id] # Convert trade price to account currency if needed @@ -102,6 +115,8 @@ class Holding::ForwardCalculator # Returns the current cost basis for a security, or nil if no buys recorded def cost_basis_for(security_id, currency) + return nil if @transferred_security_ids.include?(security_id) + tracker = @cost_basis_tracker[security_id] return nil if tracker[:total_qty].zero? diff --git a/app/models/holding/gapfillable.rb b/app/models/holding/gapfillable.rb index 232a9552e..f841bbc93 100644 --- a/app/models/holding/gapfillable.rb +++ b/app/models/holding/gapfillable.rb @@ -28,7 +28,8 @@ module Holding::Gapfillable price: previous_holding.price, currency: previous_holding.currency, amount: previous_holding.amount, - cost_basis: previous_holding.cost_basis + cost_basis: previous_holding.cost_basis, + cost_basis_unknown: previous_holding.cost_basis_unknown ) end end diff --git a/app/models/holding/holding_data.rb b/app/models/holding/holding_data.rb index be466b75a..849ffaf23 100644 --- a/app/models/holding/holding_data.rb +++ b/app/models/holding/holding_data.rb @@ -1,5 +1,9 @@ +# `cost_basis_unknown` is not the same as a nil `cost_basis`. Nil means "this +# calculation produced nothing", and the materializer is right to leave an +# earlier figure standing. Unknown means "this position contains a transfer, so +# no cost basis can be known here" — a stale calculated figure has to go. Holding::HoldingData = Struct.new( :account_id, :security_id, :date, - :qty, :price, :currency, :amount, :cost_basis, + :qty, :price, :currency, :amount, :cost_basis, :cost_basis_unknown, keyword_init: true ) diff --git a/app/models/holding/materializer.rb b/app/models/holding/materializer.rb index fefbfe0d5..599a79979 100644 --- a/app/models/holding/materializer.rb +++ b/app/models/holding/materializer.rb @@ -103,6 +103,19 @@ class Holding::Materializer # For locked holdings, preserve ALL cost_basis fields holdings_buffer_to_upsert_without_cost << base_attrs flush.call(holdings_buffer_to_upsert_without_cost) if holdings_buffer_to_upsert_without_cost.size >= PERSIST_BATCH_SIZE + elsif holding.cost_basis_unknown && clearable_calculated_basis?(existing) + # The position took in a transfer, so it has no cost basis this app + # can know. A nil from the calculator alone would leave the previous + # calculated figure standing — which is the stale number reporting a + # transferred coin as if it had been bought on arrival. Cleared, so + # the read path falls through and answers "unknown" rather than + # confidently wrong. A manual or provider figure is somebody's + # assertion about what the position cost, and stays. + holdings_buffer_to_upsert_with_cost << base_attrs.merge( + "cost_basis" => nil, + "cost_basis_source" => nil + ) + flush.call(holdings_buffer_to_upsert_with_cost) if holdings_buffer_to_upsert_with_cost.size >= PERSIST_BATCH_SIZE elsif reconciled[:should_update] && reconciled[:cost_basis].present? # Update with new cost_basis and source holdings_buffer_to_upsert_with_cost << base_attrs.merge( @@ -143,15 +156,28 @@ class Holding::Materializer flush.call(holdings_buffer_to_upsert_without_cost) unless holdings_buffer_to_upsert_without_cost.empty? end + # Nothing to clear, or a figure somebody asserted rather than one this app + # worked out. + def clearable_calculated_basis?(existing) + return false if existing.nil? || existing.cost_basis.blank? + + existing.cost_basis_source.nil? || existing.cost_basis_source == "calculated" + end + def load_existing_holdings_map # Load holdings that might affect reconciliation: # - Locked holdings (must preserve their cost_basis) # - Holdings with a source (need to check priority) # - Provider-sourced holdings (must not be overwritten) + # - Anything carrying a cost_basis at all, source or not. A row with a + # figure and no source was invisible here, so the transfer clearing + # below saw `existing` as nil and left the stale basis standing — + # exactly the rows least able to justify the number they hold. account.holdings .where(cost_basis_locked: true) .or(account.holdings.where.not(cost_basis_source: nil)) .or(account.holdings.where.not(account_provider_id: nil)) + .or(account.holdings.where.not(cost_basis: nil)) .index_by { |h| holding_key(h) } end diff --git a/app/models/holding/reverse_calculator.rb b/app/models/holding/reverse_calculator.rb index 1c163fa0a..9b0bd76d0 100644 --- a/app/models/holding/reverse_calculator.rb +++ b/app/models/holding/reverse_calculator.rb @@ -74,13 +74,18 @@ class Holding::ReverseCalculator price: price.price, currency: price.currency, amount: qty * price.price, - cost_basis: cost_basis_for(security_id, date) + cost_basis: cost_basis_for(security_id, date), + cost_basis_unknown: transferred_by?(security_id, date) ) end.compact end def precompute_cost_basis @cost_basis_snapshots = Hash.new { |h, k| h[k] = [] } + # First date a transfer landed on each security. From that day on the + # position contains units acquired at a price nothing here knows, so it + # has no cost basis — before it, the purchases still stand on their own. + @first_transfer_dates = {} tracker = Hash.new { |h, k| h[k] = { total_cost: BigDecimal("0"), total_qty: BigDecimal("0") } } portfolio_cache.get_trades.sort_by(&:date).each do |trade_entry| @@ -88,6 +93,12 @@ class Holding::ReverseCalculator next unless trade.qty > 0 security_id = trade.security_id + + if trade.investment_activity_label == Trade::TRANSFER_LABEL + @first_transfer_dates[security_id] ||= trade_entry.date + next + end + trade_price = Money.new(trade.price, trade.currency) begin converted_price = trade_price.exchange_to(account.currency).amount @@ -105,7 +116,14 @@ class Holding::ReverseCalculator end end + def transferred_by?(security_id, date) + first = @first_transfer_dates[security_id] + first.present? && first <= date + end + def cost_basis_for(security_id, date) + return nil if transferred_by?(security_id, date) + snapshots = @cost_basis_snapshots[security_id] return nil if snapshots.empty? diff --git a/app/models/trade.rb b/app/models/trade.rb index a07575b6a..8a1f06a5a 100644 --- a/app/models/trade.rb +++ b/app/models/trade.rb @@ -25,6 +25,17 @@ class Trade < ApplicationRecord # excluded, and an ambiguous one is left where the user can see it. INTERNAL_MOVEMENT_LABELS = %w[Transfer Sweep\ In Sweep\ Out].freeze + # Moving an asset between places you own is not an acquisition, so it must not + # set a cost basis. Named here because Holding reads it. + # + # A single label rather than INTERNAL_MOVEMENT_LABELS above, though both rest + # on ownership being preserved: this one is the label the onchain processor + # writes, and the only one seen setting a basis it should not. Widening the + # basis guard to the sweep labels would change which holdings lose their + # basis, and nothing has shown a sweep landing on a security — so it stays + # narrow until something does. + TRANSFER_LABEL = "Transfer".freeze + validates :qty, presence: true validates :price, :currency, presence: true validates :investment_activity_label, inclusion: { in: ACTIVITY_LABELS }, allow_nil: true diff --git a/db/migrate/20260826090000_clear_transferred_position_cost_bases.rb b/db/migrate/20260826090000_clear_transferred_position_cost_bases.rb new file mode 100644 index 000000000..c27e29b75 --- /dev/null +++ b/db/migrate/20260826090000_clear_transferred_position_cost_bases.rb @@ -0,0 +1,40 @@ +# A transferred position has no cost basis this app can know: the units were +# acquired somewhere else, at a price nothing here recorded. The calculators +# were counting the transfer's own price as a purchase, so those positions have +# a stored figure that reports a coin bought at 30k and moved in at 60k as +# having made no gain at all. +# +# `Holding#avg_cost` reads that stored value before it reaches the transfer +# guard, so the figure stands until the account materializes again — which for +# a manual or disconnected account may be never. +# +# Only figures this app worked out are cleared. A `manual` or `provider` basis +# is somebody asserting what the position cost, which is exactly the thing the +# app cannot derive for a transfer, and stays. +class ClearTransferredPositionCostBases < ActiveRecord::Migration[7.2] + def up + execute <<~SQL + UPDATE holdings + SET cost_basis = NULL, cost_basis_source = NULL + WHERE cost_basis IS NOT NULL + AND cost_basis_locked = false + AND (cost_basis_source IS NULL OR cost_basis_source = 'calculated') + AND EXISTS ( + SELECT 1 + FROM trades + JOIN entries ON entries.entryable_id = trades.id + AND entries.entryable_type = 'Trade' + WHERE trades.security_id = holdings.security_id + AND entries.account_id = holdings.account_id + AND trades.investment_activity_label = 'Transfer' + AND entries.date <= holdings.date + ) + SQL + end + + # The cleared figures were wrong, and the correct value is "unknown". Putting + # them back would mean recomputing the same fabrication. + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index 815e32f72..a66cb6050 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_25_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_08_26_090000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" diff --git a/test/models/holding/materializer_test.rb b/test/models/holding/materializer_test.rb index e975de3cd..d6c34eb20 100644 --- a/test/models/holding/materializer_test.rb +++ b/test/models/holding/materializer_test.rb @@ -19,6 +19,66 @@ class Holding::MaterializerTest < ActiveSupport::TestCase end end + # A position that takes in a transfer has no cost basis this app can know. + # A nil from the calculator alone left the previous calculated figure + # standing — the stale number reporting a transferred coin as if it had been + # bought on the day it arrived. `avg_cost` reads that stored value before it + # ever reaches the transfer guard, so the guard was only protecting holdings + # that had nothing stored at all. + # + # Materialised first and relabelled after, because that is the case that + # matters: a position already carrying a figure worked out before anyone + # knew the movement was a transfer. + test "a transfer clears a cost basis this app had worked out" do + create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current) + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + assert_equal 200, latest_holding.cost_basis.to_d, "nothing was stored to clear" + + @account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) } + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + + assert_nil latest_holding.cost_basis, "the stale calculated figure survived the transfer" + assert_nil latest_holding.cost_basis_source + assert_nil latest_holding.avg_cost + end + + # A row carrying a figure with no `cost_basis_source` was invisible to + # `load_existing_holdings_map`, so the clearing above saw no existing holding + # and left the stale basis standing — the rows least able to justify the + # number they hold being the ones that kept it. + test "a transfer clears a stored basis that never recorded where it came from" do + create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current) + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + + @account.holdings.where(security: @aapl).update_all(cost_basis: 200, cost_basis_source: nil) + @account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) } + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + + assert_nil latest_holding.cost_basis, "a source-less figure outlived the transfer" + end + # Somebody asserted what this position cost them, which is exactly what the + # app cannot work out for a transfer. Theirs to keep. + test "a transfer leaves a provider cost basis alone" do + create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current) + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + + @account.holdings.where(security: @aapl).update_all(cost_basis: 150, cost_basis_source: "provider") + @account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) } + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + + assert_equal 150, latest_holding.cost_basis.to_d + end + + # An ordinary purchase is untouched: the exclusion keys off the label, and + # most rows carry none at all. + test "an ordinary purchase still gets its calculated basis" do + create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current) + + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + + assert_equal 200, latest_holding.cost_basis.to_d + end + test "purges stale holdings for unlinked accounts" do # Since the account has no entries, there should be no holdings Holding.create!(account: @account, security: @aapl, qty: 1, price: 100, amount: 100, currency: "USD", date: Date.current) @@ -400,4 +460,10 @@ class Holding::MaterializerTest < ActiveSupport::TestCase today_holdings.pluck(:security_id, :currency).sort ) end + + private + + def latest_holding + @account.holdings.where(security: @aapl).order(:date).last + end end diff --git a/test/models/holding_test.rb b/test/models/holding_test.rb index 32eb1b072..353bf0ffd 100644 --- a/test/models/holding_test.rb +++ b/test/models/holding_test.rb @@ -454,4 +454,65 @@ class HoldingTest < ActiveSupport::TestCase amount: qty * price, currency: "USD" end + + # A coin bought elsewhere at one price and moved in at another was never + # bought here, so counting the day it arrived as its cost reports a gain of + # zero on a position that may have doubled. + test "a transfer does not set the cost basis" do + holding = holdings(:one) + holding.account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) } + + assert_nil holding.avg_cost, + "a transferred position has no cost basis this app can know" + end + + # `!=` is NULL for an unlabelled row, so a naive exclusion drops the ordinary + # purchases that carry no label — which is most of them. + test "an unlabelled purchase still sets it" do + holding = holdings(:one) + holding.account.trades.each { |t| t.update!(investment_activity_label: nil) } + + assert_not_nil holding.avg_cost + end + + # Averaging the purchases alone and applying that to every unit is the same + # fabrication in a quieter form. + test "a position mixing a purchase and a transfer has no knowable cost" do + holding = holdings(:one) + holding.account.trades.each { |t| t.update!(investment_activity_label: "Buy") } + + holding.account.entries.create!( + date: holding.date - 1, + name: "Received 1 unit", + amount: -100, + currency: holding.currency, + entryable: Trade.new( + security: holding.security, + qty: 1, + price: 100, + currency: holding.currency, + investment_activity_label: Trade::TRANSFER_LABEL + ) + ) + + assert_nil holding.avg_cost + end + + # A figure the user typed is theirs, not ours to discard: they are saying + # what the position cost them, which is exactly what the app cannot work + # out on its own for a transfer. + test "a cost basis the user set survives a transfer" do + holding = holdings(:one) + holding.account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) } + holding.update_columns(cost_basis: 100, cost_basis_source: "manual") + + assert_equal 100, holding.reload.avg_cost.amount.to_d + end + + test "a purchase still sets it" do + holding = holdings(:one) + holding.account.trades.each { |t| t.update!(investment_activity_label: "Buy") } + + assert_not_nil holding.avg_cost + end end