mirror of
https://github.com/we-promise/sure.git
synced 2026-05-28 15:04:57 +00:00
fix(holdings): carry provider cost_basis forward to calculated rows past snapshot date (#1818)
* fix(holdings): carry provider cost_basis forward to calculated rows Providers like IBKR Flex emit holdings on report_date and only include trades within the query window. The reverse calculator + gapfill therefore produces rows past report_date with nil cost_basis, even though the provider supplied a basis on the snapshot. That nil basis silently blanks `Trend`, the Reports "Total Return" card, the Top Holdings return column, and Gains by Tax Treatment, because every one of them gates on `holding.avg_cost`. When a calculated row would otherwise have no usable cost_basis, backfill it with the most recent provider-supplied cost_basis for the same (security, currency) on or before the holding date. Existing calculated/manual values are preserved (they outrank a provider carry-forward), and existing provider carry-forwards are refreshed when a newer snapshot supersedes them. * - Fix currency mismatch: provider snapshots were keyed by (security_id, currency) but calculated rows use account currency while IBKR provider rows use the security's native currency (e.g., USD vs EUR). Now keyed by security_id only; carry_forward_provider_cost_basis converts via Money#exchange_to at the snapshot date (same convention as ReverseCalculator for trade prices), with a ConversionError fallback. - Trim long inline comment to three lines - Fix safe-nav inconsistency: existing.cost_basis.positive? -> existing&.cost_basis&.positive? - Add test: refreshes stale carry-forward when a newer provider snapshot arrives - Add test: carry-forward is a no-op for forward-strategy accounts with no provider holdings * fix(holdings): prevent overwriting zero-valued manual cost basis Ensure that manual cost basis entries with a value of zero (e.g., for free shares) are not overwritten by provider carry-forward values during materialization. Additionally, updated the logic to allow zero-valued manual or calculated cost bases to be preserved, and added tests to verify currency conversion and error handling during cost basis carry-forward. * refactor(holdings): allow zero-valued cost basis in provider snapshots Remove the filter that restricted provider cost basis snapshots to values greater than zero. This ensures that manual cost basis entries with a value of zero (e.g., for free shares) are correctly captured and available for carry-forward logic. * perf(holdings): optimize provider cost basis snapshot lookup Filter provider cost basis snapshots by the security IDs present in the current holdings set to reduce the amount of data loaded into memory. * refactor(holdings): move PortfolioCache FX fix to dedicated branch Remove date-accurate exchange rate fix from this branch — it has been split into fix/portfolio-cache-historical-fx-rate to keep concerns separate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * revert(portfolio_cache): restore date-accurate FX in get_price36676784removed date: date from exchange_to intending to move it to fix/portfolio-cache-historical-fx-rate, but that branch was a duplicate ofdb1051d2which was already in main. The revert therefore regressed portfolio_cache.rb below main's state. Restore the historical exchange rate lookup so this branch no longer removes a fix already present in main. * fix(portfolio_cache): restore date-accurate FX and its test36676784removed date: date from exchange_to and deleted the historical FX test, intending to carry them in fix/portfolio-cache-historical-fx-rate. That branch was a duplicate ofdb1051d2already in main, so the removal regressed portfolio_cache.rb below main's state. Restore both. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
CrossDrain
@@ -88,8 +88,27 @@ class Holding::Materializer
|
||||
"cost_basis_source" => reconciled[:cost_basis_source]
|
||||
)
|
||||
else
|
||||
# No cost_basis to set, or existing is better - don't touch cost_basis fields
|
||||
holdings_to_upsert_without_cost << base_attrs
|
||||
# No new calculated value — fall back to the most recent provider
|
||||
# cost_basis for this security on or before the holding date.
|
||||
# Calculated/manual values outrank a provider carry-forward.
|
||||
existing_source = existing&.cost_basis_source
|
||||
preserve_existing = existing&.cost_basis.present? && %w[calculated manual].include?(existing_source)
|
||||
|
||||
if preserve_existing
|
||||
holdings_to_upsert_without_cost << base_attrs
|
||||
else
|
||||
carried = carry_forward_provider_cost_basis(holding)
|
||||
|
||||
if carried && (existing&.cost_basis != carried || existing_source != "provider")
|
||||
holdings_to_upsert_with_cost << base_attrs.merge(
|
||||
"cost_basis" => carried,
|
||||
"cost_basis_source" => "provider"
|
||||
)
|
||||
else
|
||||
# No cost_basis to set, or existing is better - don't touch cost_basis fields
|
||||
holdings_to_upsert_without_cost << base_attrs
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -165,6 +184,50 @@ class Holding::Materializer
|
||||
[ holding.account_id || account.id, holding.security_id, holding.date, holding.currency ]
|
||||
end
|
||||
|
||||
# Returns the most recent provider-supplied cost_basis for the given holding's
|
||||
# security on or before its date, converted to the holding's currency.
|
||||
# Used to backfill calculated rows past the provider's last snapshot so
|
||||
# reports keep showing trend data.
|
||||
#
|
||||
# Provider and calculated rows can be denominated in different currencies
|
||||
# (e.g., IBKR reports USD holdings while the reverse calculator converts to
|
||||
# the account's base currency). When they differ, the cost_basis is converted
|
||||
# at the snapshot date — the same convention ReverseCalculator uses for trade
|
||||
# prices — so the result is consistent with trade-derived cost_basis values.
|
||||
def carry_forward_provider_cost_basis(holding)
|
||||
snapshots = provider_cost_basis_snapshots[holding.security_id]
|
||||
return nil if snapshots.blank?
|
||||
|
||||
result = nil
|
||||
snapshots.each do |snap_date, cost_basis, snap_currency|
|
||||
break if snap_date > holding.date
|
||||
result = [ cost_basis, snap_currency, snap_date ]
|
||||
end
|
||||
return nil unless result
|
||||
|
||||
cost_basis, snap_currency, snap_date = result
|
||||
return cost_basis if snap_currency == holding.currency
|
||||
|
||||
Money.new(cost_basis, snap_currency).exchange_to(holding.currency, date: snap_date).amount
|
||||
rescue Money::ConversionError
|
||||
nil
|
||||
end
|
||||
|
||||
def provider_cost_basis_snapshots
|
||||
@provider_cost_basis_snapshots ||= begin
|
||||
ids = @holdings.map(&:security_id).uniq
|
||||
account.holdings
|
||||
.where.not(account_provider_id: nil)
|
||||
.where.not(cost_basis: nil)
|
||||
.where(security_id: ids)
|
||||
.order(:date) # ascending required: carry_forward_provider_cost_basis scans and breaks on snap_date > holding.date
|
||||
.pluck(:security_id, :currency, :date, :cost_basis)
|
||||
.each_with_object(Hash.new { |h, k| h[k] = [] }) do |(security_id, currency, date, cost_basis), memo|
|
||||
memo[security_id] << [ date, cost_basis, currency ]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def purge_stale_holdings
|
||||
portfolio_security_ids = account.trades.distinct.pluck(:security_id)
|
||||
|
||||
|
||||
@@ -155,6 +155,203 @@ class Holding::MaterializerTest < ActiveSupport::TestCase
|
||||
assert_equal [ account_provider.id ], today_holdings.pluck(:account_provider_id)
|
||||
end
|
||||
|
||||
test "carries forward provider cost_basis to calculated rows past the provider snapshot date" do
|
||||
coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key")
|
||||
coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD")
|
||||
account_provider = AccountProvider.create!(account: @account, provider: coinstats_account)
|
||||
|
||||
# Provider snapshot two days ago with known cost basis, but no trades.
|
||||
# This mirrors IBKR Flex where the export ends on Friday but today is Sunday.
|
||||
Holding.create!(
|
||||
account: @account,
|
||||
security: @aapl,
|
||||
qty: 10,
|
||||
price: 200,
|
||||
amount: 2000,
|
||||
currency: "USD",
|
||||
date: 2.days.ago.to_date,
|
||||
account_provider: account_provider,
|
||||
cost_basis: BigDecimal("125.50"),
|
||||
cost_basis_source: "provider"
|
||||
)
|
||||
|
||||
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
|
||||
|
||||
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
|
||||
assert_nil today_holding.account_provider_id,
|
||||
"Today's row is calculated, not a provider snapshot"
|
||||
assert_equal BigDecimal("125.50"), today_holding.cost_basis,
|
||||
"Today's calculated row should inherit the provider's cost_basis so trend/return calcs work"
|
||||
assert_equal "provider", today_holding.cost_basis_source
|
||||
end
|
||||
|
||||
test "does not overwrite an existing calculated cost_basis with provider carry-forward" do
|
||||
coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key")
|
||||
coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD")
|
||||
account_provider = AccountProvider.create!(account: @account, provider: coinstats_account)
|
||||
|
||||
Holding.create!(
|
||||
account: @account,
|
||||
security: @aapl,
|
||||
qty: 10,
|
||||
price: 200,
|
||||
amount: 2000,
|
||||
currency: "USD",
|
||||
date: 2.days.ago.to_date,
|
||||
account_provider: account_provider,
|
||||
cost_basis: BigDecimal("125.50"),
|
||||
cost_basis_source: "provider"
|
||||
)
|
||||
|
||||
# Pre-existing calculated row for today (e.g., from a prior trade-derived run)
|
||||
Holding.create!(
|
||||
account: @account,
|
||||
security: @aapl,
|
||||
qty: 10,
|
||||
price: 200,
|
||||
amount: 2000,
|
||||
currency: "USD",
|
||||
date: Date.current,
|
||||
cost_basis: BigDecimal("180.00"),
|
||||
cost_basis_source: "calculated"
|
||||
)
|
||||
|
||||
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
|
||||
|
||||
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
|
||||
assert_equal BigDecimal("180.00"), today_holding.cost_basis,
|
||||
"Existing calculated cost_basis must beat provider carry-forward"
|
||||
assert_equal "calculated", today_holding.cost_basis_source
|
||||
end
|
||||
|
||||
test "refreshes stale provider carry-forward when a newer provider snapshot arrives" do
|
||||
coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key")
|
||||
coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD")
|
||||
account_provider = AccountProvider.create!(account: @account, provider: coinstats_account)
|
||||
|
||||
# With no entries, start_date = yesterday, so materializer only descends to
|
||||
# yesterday. Use an older date so the second snapshot doesn't land on a date
|
||||
# the materializer already owns.
|
||||
Holding.create!(
|
||||
account: @account, security: @aapl, qty: 10, price: 200, amount: 2000,
|
||||
currency: "USD", date: 5.days.ago.to_date,
|
||||
account_provider: account_provider,
|
||||
cost_basis: BigDecimal("100.00"), cost_basis_source: "provider"
|
||||
)
|
||||
|
||||
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
|
||||
|
||||
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
|
||||
assert_equal BigDecimal("100.00"), today_holding.cost_basis
|
||||
|
||||
# Provider publishes a newer snapshot with an updated cost_basis on a date
|
||||
# that falls outside the materializer's window (older than start_date).
|
||||
Holding.create!(
|
||||
account: @account, security: @aapl, qty: 10, price: 210, amount: 2100,
|
||||
currency: "USD", date: 3.days.ago.to_date,
|
||||
account_provider: account_provider,
|
||||
cost_basis: BigDecimal("150.00"), cost_basis_source: "provider"
|
||||
)
|
||||
|
||||
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
|
||||
|
||||
today_holding.reload
|
||||
assert_equal BigDecimal("150.00"), today_holding.cost_basis,
|
||||
"Carry-forward should update to the newer provider snapshot value"
|
||||
assert_equal "provider", today_holding.cost_basis_source
|
||||
end
|
||||
|
||||
test "carry-forward is a no-op for forward-strategy accounts without provider holdings" do
|
||||
create_trade(@aapl, account: @account, qty: 5, price: 200, date: Date.current)
|
||||
|
||||
assert_nothing_raised do
|
||||
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
|
||||
end
|
||||
|
||||
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
|
||||
assert_equal "calculated", today_holding.cost_basis_source
|
||||
assert_equal BigDecimal("200.00"), today_holding.cost_basis,
|
||||
"Forward strategy with no provider rows should compute cost_basis from trades normally"
|
||||
end
|
||||
|
||||
test "does not overwrite a zero-valued manual cost_basis with provider carry-forward" do
|
||||
coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key")
|
||||
coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD")
|
||||
account_provider = AccountProvider.create!(account: @account, provider: coinstats_account)
|
||||
|
||||
Holding.create!(
|
||||
account: @account, security: @aapl,
|
||||
qty: 10, price: 200, amount: 2000, currency: "USD",
|
||||
date: 2.days.ago.to_date,
|
||||
account_provider: account_provider,
|
||||
cost_basis: BigDecimal("125.50"), cost_basis_source: "provider"
|
||||
)
|
||||
|
||||
# Free shares: legitimate zero-cost basis recorded manually
|
||||
Holding.create!(
|
||||
account: @account, security: @aapl,
|
||||
qty: 10, price: 200, amount: 2000, currency: "USD",
|
||||
date: Date.current,
|
||||
cost_basis: BigDecimal("0"), cost_basis_source: "manual"
|
||||
)
|
||||
|
||||
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
|
||||
|
||||
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
|
||||
assert_equal BigDecimal("0"), today_holding.cost_basis,
|
||||
"Zero-valued manual cost_basis (e.g., free shares) must not be overwritten by provider carry-forward"
|
||||
assert_equal "manual", today_holding.cost_basis_source
|
||||
end
|
||||
|
||||
test "carry-forward converts provider cost_basis currency when provider and calculated currencies differ" do
|
||||
snap_date = 2.days.ago.to_date
|
||||
ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: snap_date, rate: 1.2)
|
||||
|
||||
coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key")
|
||||
coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "EUR")
|
||||
account_provider = AccountProvider.create!(account: @account, provider: coinstats_account)
|
||||
|
||||
Holding.create!(
|
||||
account: @account, security: @aapl,
|
||||
qty: 10, price: 200, amount: 2000, currency: "EUR",
|
||||
date: snap_date,
|
||||
account_provider: account_provider,
|
||||
cost_basis: BigDecimal("100.00"), cost_basis_source: "provider"
|
||||
)
|
||||
|
||||
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
|
||||
|
||||
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
|
||||
assert_in_delta BigDecimal("120.00"), today_holding.cost_basis, BigDecimal("0.01"),
|
||||
"Provider cost_basis in EUR should be converted to USD at the snapshot-date exchange rate"
|
||||
assert_equal "provider", today_holding.cost_basis_source
|
||||
end
|
||||
|
||||
test "carry-forward skips provider cost_basis when FX conversion raises Money::ConversionError" do
|
||||
snap_date = 2.days.ago.to_date
|
||||
# No ExchangeRate created — EUR→USD conversion will raise Money::ConversionError
|
||||
|
||||
coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key")
|
||||
coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "EUR")
|
||||
account_provider = AccountProvider.create!(account: @account, provider: coinstats_account)
|
||||
|
||||
Holding.create!(
|
||||
account: @account, security: @aapl,
|
||||
qty: 10, price: 200, amount: 2000, currency: "EUR",
|
||||
date: snap_date,
|
||||
account_provider: account_provider,
|
||||
cost_basis: BigDecimal("100.00"), cost_basis_source: "provider"
|
||||
)
|
||||
|
||||
assert_nothing_raised do
|
||||
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
|
||||
end
|
||||
|
||||
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
|
||||
assert_nil today_holding.cost_basis,
|
||||
"Carry-forward should be skipped gracefully when currency conversion fails"
|
||||
end
|
||||
|
||||
test "preserves same-day non-provider holdings for securities absent from the provider snapshot" do
|
||||
ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.2)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user