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