fix(holdings): use trade-date FX rate for cost basis price conversion

app/models/holding/forward_calculator.rb

@@ -87,12 +87,18 @@ def update_cost_basis_tracker(trade_entries)
         security_id = trade.security_id
         tracker = @cost_basis_tracker[security_id]
 
-        # Convert trade price to account currency if needed
+        # Convert trade price to account currency using the trade-date rate
         trade_price = Money.new(trade.price, trade.currency)
         begin
-          converted_price = trade_price.exchange_to(account.currency).amount
+          converted_price = trade_price.exchange_to(account.currency, date: trade_entry.date, custom_rate: trade.exchange_rate).amount
         rescue Money::ConversionError
-          converted_price = trade.price
+          Rails.logger.warn(
+            "Holding::ForwardCalculator: FX conversion failed for trade #{trade.id} " \
+            "(#{trade.currency}→#{account.currency}, date: #{trade_entry.date}, " \
+            "custom_rate: #{trade.exchange_rate.inspect}). " \
+            "Excluding trade from cost basis to avoid mixing currencies."
+          )
+          next
         end
 
         tracker[:total_cost] += converted_price * trade.qty

app/models/holding/reverse_calculator.rb

@@ -82,17 +82,28 @@ 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") } }
+      invalid_cost_basis = {}
 
       portfolio_cache.get_trades.sort_by(&:date).each do |trade_entry|
         trade = trade_entry.entryable
         next unless trade.qty > 0
 
         security_id = trade.security_id
+        next if invalid_cost_basis[security_id]
+
         trade_price = Money.new(trade.price, trade.currency)
         begin
-          converted_price = trade_price.exchange_to(account.currency).amount
+          converted_price = trade_price.exchange_to(account.currency, date: trade_entry.date, custom_rate: trade.exchange_rate).amount
         rescue Money::ConversionError
-          converted_price = trade.price
+          invalid_cost_basis[security_id] = true
+          @cost_basis_snapshots[security_id] << [ trade_entry.date, nil ]
+          Rails.logger.warn(
+            "Holding::ReverseCalculator: FX conversion failed for trade #{trade.id} " \
+            "(#{trade.currency}→#{account.currency}, date: #{trade_entry.date}, " \
+            "custom_rate: #{trade.exchange_rate.inspect}). " \
+            "Excluding trade from cost basis to avoid mixing currencies."
+          )
+          next
         end
 
         tracker[security_id][:total_cost] += converted_price * trade.qty

test/models/holding/forward_calculator_test.rb

@@ -130,6 +130,76 @@ class Holding::ForwardCalculatorTest < ActiveSupport::TestCase
     assert_holdings(expected, calculated)
   end
 
+  # Cost basis FX: trade-date rate must be used, not today's rate
+  test "cost_basis uses trade-date exchange rate for foreign-currency buy" do
+    travel_to Date.new(2025, 6, 1) do
+      eur_stock = Security.create!(ticker: "EURST", name: "EUR Stock")
+      buy_date  = Date.new(2025, 5, 27)
+
+      Security::Price.create!(security: eur_stock, date: buy_date, price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: Date.current, price: 100, currency: "EUR")
+
+      # EUR/USD was 1.10 on trade date; today it is 1.50 — cost basis must use 1.10
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: buy_date,   rate: 1.10)
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.50)
+
+      create_trade(eur_stock, qty: 10, date: buy_date, price: 100, currency: "EUR", account: @account)
+
+      calculated = Holding::ForwardCalculator.new(@account).calculate
+      today_holding = calculated.find { |h| h.date == Date.current && h.security == eur_stock }
+
+      # cost_basis per share = 100 EUR × 1.10 (trade-date rate) = 110 USD
+      assert_in_delta 110.0, today_holding.cost_basis.to_f, 0.01,
+        "cost_basis must use the trade-date FX rate (1.10), not today's rate (1.50)"
+    end
+  end
+
+  test "cost_basis is nil and trade is skipped when FX conversion fails" do
+    travel_to Date.new(2025, 6, 1) do
+      eur_stock = Security.create!(ticker: "EURST3", name: "EUR Stock 3")
+      buy_date  = Date.new(2025, 5, 27)
+
+      Security::Price.create!(security: eur_stock, date: buy_date, price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: Date.current, price: 100, currency: "EUR")
+
+      # No exchange rate for buy_date and no custom_rate — conversion will fail
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.50)
+
+      create_trade(eur_stock, qty: 10, date: buy_date, price: 100, currency: "EUR", account: @account)
+
+      calculated = nil
+      assert_nothing_raised { calculated = Holding::ForwardCalculator.new(@account).calculate }
+
+      today_holding = calculated.find { |h| h.date == Date.current && h.security == eur_stock }
+      assert_nil today_holding.cost_basis,
+        "cost_basis must be nil when FX conversion fails (trade excluded to avoid mixing currencies)"
+    end
+  end
+
+  test "cost_basis uses provider-supplied exchange_rate when present" do
+    travel_to Date.new(2025, 6, 1) do
+      eur_stock = Security.create!(ticker: "EURST2", name: "EUR Stock 2")
+      buy_date  = Date.new(2025, 5, 27)
+
+      Security::Price.create!(security: eur_stock, date: buy_date, price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: Date.current, price: 100, currency: "EUR")
+
+      # DB has a different rate — the provider-supplied rate (1.20) must win
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: buy_date,   rate: 1.10)
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.50)
+
+      create_trade(eur_stock, qty: 10, date: buy_date, price: 100, currency: "EUR",
+                   exchange_rate: 1.20, account: @account)
+
+      calculated = Holding::ForwardCalculator.new(@account).calculate
+      today_holding = calculated.find { |h| h.date == Date.current && h.security == eur_stock }
+
+      # cost_basis per share = 100 EUR × 1.20 (provider rate) = 120 USD
+      assert_in_delta 120.0, today_holding.cost_basis.to_f, 0.01,
+        "cost_basis must prefer the provider-supplied exchange_rate over the DB lookup"
+    end
+  end
+
   private
     def assert_holdings(expected, calculated)
       expected.each do |expected_entry|

test/models/holding/reverse_calculator_test.rb

@@ -236,6 +236,134 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase
     assert_in_delta 100.0, cost_basis_for(calc, security, Date.current).to_f, 1e-6
   end
 
+  # Cost basis FX: trade-date rate must be used, not today's rate
+  test "cost_basis uses trade-date exchange rate for foreign-currency buy" do
+    travel_to Date.new(2025, 6, 1) do
+      eur_stock = Security.create!(ticker: "EURST", name: "EUR Stock")
+      buy_date  = Date.new(2025, 5, 27)
+
+      Security::Price.create!(security: eur_stock, date: buy_date, price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: Date.current, price: 100, currency: "EUR")
+
+      @account.holdings.create!(security: eur_stock, date: Date.current,
+                                 qty: 10, price: 100, amount: 1000, currency: "EUR")
+
+      # EUR/USD was 1.10 on trade date; today it is 1.50 — cost basis must use 1.10
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: buy_date,     rate: 1.10)
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.50)
+
+      create_trade(eur_stock, qty: 10, date: buy_date, price: 100, currency: "EUR", account: @account)
+
+      snapshot = OpenStruct.new(to_h: { eur_stock.id => 10 })
+      calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate
+      today_holding = calculated.find { |h| h.date == Date.current && h.security == eur_stock }
+
+      # cost_basis per share = 100 EUR × 1.10 (trade-date rate) = 110 USD
+      assert_in_delta 110.0, today_holding.cost_basis.to_f, 0.01,
+        "cost_basis must use the trade-date FX rate (1.10), not today's rate (1.50)"
+    end
+  end
+
+  test "cost_basis uses provider-supplied exchange_rate when present" do
+    travel_to Date.new(2025, 6, 1) do
+      eur_stock = Security.create!(ticker: "EURST2", name: "EUR Stock 2")
+      buy_date  = Date.new(2025, 5, 27)
+
+      Security::Price.create!(security: eur_stock, date: buy_date, price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: Date.current, price: 100, currency: "EUR")
+
+      @account.holdings.create!(security: eur_stock, date: Date.current,
+                                 qty: 10, price: 100, amount: 1000, currency: "EUR")
+
+      # DB has a different rate — the provider-supplied rate (1.20) must win
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: buy_date,     rate: 1.10)
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.50)
+
+      create_trade(eur_stock, qty: 10, date: buy_date, price: 100, currency: "EUR",
+                   exchange_rate: 1.20, account: @account)
+
+      snapshot = OpenStruct.new(to_h: { eur_stock.id => 10 })
+      calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate
+      today_holding = calculated.find { |h| h.date == Date.current && h.security == eur_stock }
+
+      # cost_basis per share = 100 EUR × 1.20 (provider rate) = 120 USD
+      assert_in_delta 120.0, today_holding.cost_basis.to_f, 0.01,
+        "cost_basis must prefer the provider-supplied exchange_rate over the DB lookup"
+    end
+  end
+
+  test "cost_basis is nil for all dates after a failed FX conversion even when a later buy succeeds" do
+    travel_to Date.new(2025, 6, 1) do
+      eur_stock = Security.create!(ticker: "EURST4", name: "EUR Stock 4")
+      first_buy  = Date.new(2025, 5, 20)
+      failed_buy = Date.new(2025, 5, 27)
+      later_buy  = Date.new(2025, 5, 30)
+
+      Security::Price.create!(security: eur_stock, date: first_buy,   price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: failed_buy,  price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: later_buy,   price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: Date.current, price: 100, currency: "EUR")
+
+      @account.holdings.create!(security: eur_stock, date: Date.current,
+                                 qty: 30, price: 100, amount: 3000, currency: "EUR")
+
+      # first_buy and later_buy have rates; failed_buy does not — it will raise ConversionError
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: first_buy,   rate: 1.10)
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: later_buy,   rate: 1.20)
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.50)
+
+      create_trade(eur_stock, qty: 10, date: first_buy,  price: 100, currency: "EUR", account: @account)
+      create_trade(eur_stock, qty: 10, date: failed_buy, price: 100, currency: "EUR", account: @account)
+      create_trade(eur_stock, qty: 10, date: later_buy,  price: 100, currency: "EUR", account: @account)
+
+      snapshot = OpenStruct.new(to_h: { eur_stock.id => 30 })
+      calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate
+
+      first_buy_holding = calculated.find { |h| h.date == first_buy && h.security == eur_stock }
+      assert_in_delta 110.0, first_buy_holding.cost_basis.to_f, 0.01,
+        "cost_basis should be valid (110 USD = 100 EUR × 1.10) before the FX failure on #{failed_buy}"
+
+      failed_date_holding = calculated.find { |h| h.date == failed_buy && h.security == eur_stock }
+      assert_nil failed_date_holding.cost_basis,
+        "cost_basis should be nil starting from the FX failure date"
+
+      later_date_holding = calculated.find { |h| h.date == later_buy && h.security == eur_stock }
+      assert_nil later_date_holding.cost_basis,
+        "cost_basis should remain nil even for dates with successful conversions after the failure"
+
+      today_holding = calculated.find { |h| h.date == Date.current && h.security == eur_stock }
+      assert_equal 30, today_holding.qty, "qty should still be calculated despite nil cost_basis"
+      assert_nil today_holding.cost_basis,
+        "cost_basis must be nil after FX failure even when a later buy converts successfully"
+    end
+  end
+
+  test "cost_basis is nil and trade is skipped when FX conversion fails" do
+    travel_to Date.new(2025, 6, 1) do
+      eur_stock = Security.create!(ticker: "EURST3", name: "EUR Stock 3")
+      buy_date  = Date.new(2025, 5, 27)
+
+      Security::Price.create!(security: eur_stock, date: buy_date, price: 100, currency: "EUR")
+      Security::Price.create!(security: eur_stock, date: Date.current, price: 100, currency: "EUR")
+
+      @account.holdings.create!(security: eur_stock, date: Date.current,
+                                 qty: 10, price: 100, amount: 1000, currency: "EUR")
+
+      # No exchange rate for buy_date and no custom_rate — conversion will fail
+      ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.50)
+
+      create_trade(eur_stock, qty: 10, date: buy_date, price: 100, currency: "EUR", account: @account)
+
+      snapshot = OpenStruct.new(to_h: { eur_stock.id => 10 })
+      calculated = nil
+      assert_nothing_raised { calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate }
+
+      today_holding = calculated.find { |h| h.date == Date.current && h.security == eur_stock }
+      assert_nil today_holding.cost_basis,
+        "cost_basis must be nil when FX conversion fails (trade excluded to avoid mixing currencies)"
+    end
+  end
+
   private
     def assert_holdings(expected, calculated)
       expected.each do |expected_entry|

test/support/entries_test_helper.rb

@@ -34,7 +34,7 @@ def create_valuation(attributes = {})
     Entry.create! entry_defaults.merge(entry_attributes)
   end
 
-  def create_trade(security, account:, qty:, date:, price: nil, currency: "USD")
+  def create_trade(security, account:, qty:, date:, price: nil, currency: "USD", exchange_rate: nil)
     trade_price = price || Security::Price.find_by!(security: security, date: date).price
 
     trade = Trade.new \
@@ -44,6 +44,8 @@ def create_trade(security, account:, qty:, date:, price: nil, currency: "USD")
       currency: currency,
       investment_activity_label: qty > 0 ? "Buy" : "Sell"
 
+    trade.exchange_rate = exchange_rate if exchange_rate
+
     account.entries.create! \
       name: "Trade",
       date: date,