Fix holding cost basis to relieve sells under weighted-average method

app/models/holding/cost_basis_tracker.rb

@@ -0,0 +1,65 @@
+# Tracks weighted-average cost basis for a single security as trades are
+# applied in chronological order.
+#
+# Buys add to the running cost and quantity. Sells relieve quantity at the
+# current average cost (leaving the per-share average unchanged), and once the
+# position is fully closed the tracker resets so a later repurchase starts from
+# a clean basis. Without this relief, cost basis would be averaged over every
+# buy the account ever made for the security — corrupting the figure after a
+# position is sold off and bought again.
+#
+# Prices are expected in the account's currency (callers convert before
+# applying), so the tracker itself is currency-agnostic.
+class Holding::CostBasisTracker
+  def initialize
+    @total_cost = BigDecimal("0")
+    @total_qty = BigDecimal("0")
+  end
+
+  # Applies a trade by its signed quantity: positive is a buy, negative a sell.
+  def apply(price, qty)
+    return if qty.nil?
+
+    qty = qty.to_d
+    return if qty.zero?
+
+    qty.positive? ? buy(price, qty) : sell(qty)
+  end
+
+  def buy(price, qty)
+    price = price&.to_d
+    qty = qty&.to_d
+    return if price.nil? || qty.nil? || !qty.positive?
+
+    @total_cost += price * qty
+    @total_qty += qty
+  end
+
+  def sell(qty)
+    qty = qty&.to_d
+    return if qty.nil? || qty.zero? || @total_qty.zero?
+
+    # Relieve at the current average cost so the per-share average is unchanged.
+    # Guard against over-selling more than is currently held.
+    relieved_qty = [ qty.abs, @total_qty ].min
+    @total_cost -= average_cost * relieved_qty
+    @total_qty -= relieved_qty
+
+    # Coercing to BigDecimal keeps arithmetic exact, so a full liquidation lands
+    # on exactly zero and the reset fires (Float math could leave a tiny remainder).
+    reset if @total_qty <= 0
+  end
+
+  # Current weighted-average cost per share, or nil when nothing is held.
+  def average_cost
+    return nil if @total_qty.zero?
+
+    @total_cost / @total_qty
+  end
+
+  private
+    def reset
+      @total_cost = BigDecimal("0")
+      @total_qty = BigDecimal("0")
+    end
+end

app/models/holding/forward_calculator.rb

@@ -1,11 +1,14 @@
 class Holding::ForwardCalculator
+  include Holding::TradeCalculatorHelpers
+
   attr_reader :account
 
   def initialize(account, security_ids: nil)
     @account = account
     @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") } }
+    # Track weighted-average cost basis per security, relieving sells so the
+    # figure stays correct after a position is fully sold and repurchased.
+    @cost_basis_trackers = Hash.new { |h, k| h[k] = Holding::CostBasisTracker.new }
   end
 
   def calculate
@@ -77,34 +80,18 @@ def build_holdings(portfolio, date, price_source: nil)
       end.compact
     end
 
-    # Updates cost basis tracker with buy trades (qty > 0)
-    # Uses weighted average cost method
+    # Applies each trade to its security's weighted-average cost basis tracker.
+    # Buys raise the basis; sells relieve quantity at the running average and a
+    # full liquidation resets it, so a later repurchase starts from a clean basis.
     def update_cost_basis_tracker(trade_entries)
       trade_entries.each do |trade_entry|
         trade = trade_entry.entryable
-        next unless trade.qty > 0 # Only track buys
-
-        security_id = trade.security_id
-        tracker = @cost_basis_tracker[security_id]
-
-        # Convert trade price to account currency if needed
-        trade_price = Money.new(trade.price, trade.currency)
-        begin
-          converted_price = trade_price.exchange_to(account.currency).amount
-        rescue Money::ConversionError
-          converted_price = trade.price
-        end
-
-        tracker[:total_cost] += converted_price * trade.qty
-        tracker[:total_qty] += trade.qty
+        @cost_basis_trackers[trade.security_id].apply(converted_trade_price(trade), trade.qty)
       end
     end
 
-    # Returns the current cost basis for a security, or nil if no buys recorded
+    # Returns the current cost basis for a security, or nil if nothing is held.
     def cost_basis_for(security_id, currency)
-      tracker = @cost_basis_tracker[security_id]
-      return nil if tracker[:total_qty].zero?
-
-      tracker[:total_cost] / tracker[:total_qty]
+      @cost_basis_trackers[security_id].average_cost
     end
 end

app/models/holding/reverse_calculator.rb

@@ -1,4 +1,6 @@
 class Holding::ReverseCalculator
+  include Holding::TradeCalculatorHelpers
+
   attr_reader :account, :portfolio_snapshot
 
   def initialize(account, portfolio_snapshot:, security_ids: nil)
@@ -81,27 +83,20 @@ def build_holdings(portfolio, date, price_source: nil)
 
     def precompute_cost_basis
       @cost_basis_snapshots = Hash.new { |h, k| h[k] = [] }
-      tracker = Hash.new { |h, k| h[k] = { total_cost: BigDecimal("0"), total_qty: BigDecimal("0") } }
+      trackers = Hash.new { |h, k| h[k] = Holding::CostBasisTracker.new }
 
       portfolio_cache.get_trades.sort_by(&:date).each do |trade_entry|
         trade = trade_entry.entryable
-        next unless trade.qty > 0
+        tracker = trackers[trade.security_id]
 
-        security_id = trade.security_id
-        trade_price = Money.new(trade.price, trade.currency)
-        begin
-          converted_price = trade_price.exchange_to(account.currency).amount
-        rescue Money::ConversionError
-          converted_price = trade.price
-        end
+        # Buys raise the basis; sells relieve quantity at the running average so
+        # the figure stays correct after a position is fully sold and repurchased.
+        tracker.apply(converted_trade_price(trade), trade.qty)
 
-        tracker[security_id][:total_cost] += converted_price * trade.qty
-        tracker[security_id][:total_qty] += trade.qty
+        average_cost = tracker.average_cost
+        next if average_cost.nil?
 
-        @cost_basis_snapshots[security_id] << [
-          trade_entry.date,
-          tracker[security_id][:total_cost] / tracker[security_id][:total_qty]
-        ]
+        @cost_basis_snapshots[trade.security_id] << [ trade_entry.date, average_cost ]
       end
     end
 

app/models/holding/trade_calculator_helpers.rb

@@ -0,0 +1,12 @@
+# Shared helpers for holding calculators (ForwardCalculator / ReverseCalculator).
+# Expects the including class to expose an `account` reader.
+module Holding::TradeCalculatorHelpers
+  private
+    # Converts a trade's price into the account's currency, falling back to the
+    # raw price when no exchange rate is available.
+    def converted_trade_price(trade)
+      Money.new(trade.price, trade.currency).exchange_to(account.currency).amount
+    rescue Money::ConversionError
+      trade.price
+    end
+end

test/models/holding/cost_basis_tracker_test.rb

@@ -0,0 +1,66 @@
+require "test_helper"
+
+class Holding::CostBasisTrackerTest < ActiveSupport::TestCase
+  setup do
+    @tracker = Holding::CostBasisTracker.new
+  end
+
+  test "average cost is nil with no trades" do
+    assert_nil @tracker.average_cost
+  end
+
+  test "weighted average across multiple buys" do
+    @tracker.apply(BigDecimal("100"), BigDecimal("10"))
+    @tracker.apply(BigDecimal("200"), BigDecimal("10"))
+
+    assert_equal BigDecimal("150"), @tracker.average_cost
+  end
+
+  test "a partial sell leaves the per-share average unchanged" do
+    @tracker.apply(BigDecimal("100"), BigDecimal("10"))
+    @tracker.apply(BigDecimal("200"), BigDecimal("10")) # avg 150
+
+    # Sell price is irrelevant to cost basis; shares are relieved at the average.
+    @tracker.apply(BigDecimal("999"), BigDecimal("-5"))
+
+    assert_equal BigDecimal("150"), @tracker.average_cost
+  end
+
+  test "cost basis resets after a full liquidation and repurchase" do
+    @tracker.apply(BigDecimal("100"), BigDecimal("10"))
+    @tracker.apply(BigDecimal("300"), BigDecimal("-10")) # fully sold
+
+    assert_nil @tracker.average_cost
+
+    @tracker.apply(BigDecimal("300"), BigDecimal("10")) # repurchased
+
+    # Only the repurchased lot is held — not (100 + 300) / 2 = 200.
+    assert_equal BigDecimal("300"), @tracker.average_cost
+  end
+
+  test "over-selling cannot drive quantity or basis negative" do
+    @tracker.apply(BigDecimal("100"), BigDecimal("5"))
+    @tracker.apply(BigDecimal("100"), BigDecimal("-10")) # sell more than held
+
+    assert_nil @tracker.average_cost
+  end
+
+  test "selling with no position is a no-op" do
+    @tracker.apply(BigDecimal("100"), BigDecimal("-5"))
+
+    assert_nil @tracker.average_cost
+  end
+
+  test "coerces float quantities so a full liquidation still resets" do
+    # Float inputs must not accumulate rounding error that leaves a tiny
+    # residual quantity and bypasses the reset-on-full-liquidation.
+    @tracker.apply(100.0, 10.0)
+    @tracker.apply(300.0, -10.0)
+
+    assert_nil @tracker.average_cost
+
+    @tracker.apply(300.0, 10.0)
+
+    assert_equal BigDecimal("300"), @tracker.average_cost
+  end
+end

test/models/holding/forward_calculator_test.rb

@@ -130,6 +130,23 @@ class Holding::ForwardCalculatorTest < ActiveSupport::TestCase
     assert_holdings(expected, calculated)
   end
 
+  # Regression: selling a position to zero must relieve the cost-basis tracker so
+  # that a later repurchase is not averaged against the already-sold lots.
+  test "cost basis resets after a position is fully sold and repurchased" do
+    load_prices
+
+    create_trade(@voo, qty: 10, date: 4.days.ago.to_date, price: 460, account: @account)
+    create_trade(@voo, qty: -10, date: 3.days.ago.to_date, price: 480, account: @account) # fully sold
+    create_trade(@voo, qty: 10, date: 1.day.ago.to_date, price: 490, account: @account)    # repurchased
+
+    calculated = Holding::ForwardCalculator.new(@account).calculate
+    current = calculated.find { |h| h.security_id == @voo.id && h.date == Date.current }
+
+    # Only the repurchased lot is held, so cost basis is $490 — not the
+    # buggy (460 + 490) / 2 = $475 averaged over the sold-off shares.
+    assert_equal BigDecimal("490"), current.cost_basis
+  end
+
   private
     def assert_holdings(expected, calculated)
       expected.each do |expected_entry|