Fix holding cost basis to relieve sells under weighted-average method#2087
Fix holding cost basis to relieve sells under weighted-average method#2087philluiz2323 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces ChangesCost Basis Tracking Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/holding/reverse_calculator.rb`:
- Around line 86-97: The current loop in reverse_calculator.rb uses
portfolio_cache.get_trades.sort_by(&:date) which leaves same-day trade ordering
ambiguous and can snapshot the wrong end-of-day basis; change the sort so it
deterministically orders trades within a date using an intra-day tiebreaker from
the trade_entry or trade (e.g., execution timestamp, sequence number, or id)
before applying tracker.apply and capturing
`@cost_basis_snapshots`[trade.security_id]; update the sort expression in the
block that iterates over portfolio_cache.get_trades to sort by [date,
tie_breaker] (using trade_entry.timestamp / trade.executed_at /
trade_entry.sequence / trade.id as available) so same-day sell/all + repurchase
sequences are processed in a stable, deterministic order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf3ebf55-2757-422b-b723-fb3f4802017c
📒 Files selected for processing (5)
app/models/holding/cost_basis_tracker.rbapp/models/holding/forward_calculator.rbapp/models/holding/reverse_calculator.rbtest/models/holding/cost_basis_tracker_test.rbtest/models/holding/forward_calculator_test.rb
| 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 ] |
There was a problem hiding this comment.
Make same-day trade ordering explicit before snapshotting.
Now that sells mutate the running tracker, replay order matters. sort_by(&:date) leaves same-day buy/sell sequences ambiguous, so a sell-all + repurchase on one date can still snapshot the wrong end-of-day basis unless this loop uses a deterministic intra-day tiebreaker from trade_entry/trade (execution timestamp, sequence, id, etc.).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/holding/reverse_calculator.rb` around lines 86 - 97, The current
loop in reverse_calculator.rb uses portfolio_cache.get_trades.sort_by(&:date)
which leaves same-day trade ordering ambiguous and can snapshot the wrong
end-of-day basis; change the sort so it deterministically orders trades within a
date using an intra-day tiebreaker from the trade_entry or trade (e.g.,
execution timestamp, sequence number, or id) before applying tracker.apply and
capturing `@cost_basis_snapshots`[trade.security_id]; update the sort expression
in the block that iterates over portfolio_cache.get_trades to sort by [date,
tie_breaker] (using trade_entry.timestamp / trade.executed_at /
trade_entry.sequence / trade.id as available) so same-day sell/all + repurchase
sequences are processed in a stable, deterministic order.
|
Clean abstraction — Two notes:
Generated by Claude Code |
…inputs to BigDecimal
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/holding/reverse_calculator.rb (1)
96-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord liquidation resets in the snapshot history.
Skipping the append on Line 97 when
average_costbecomesnilmeanscost_basis_forfalls back to the last pre-liquidation value for later dates. Becausebuild_holdingsstill emits zero-qty rows, sold-out days inherit a stale basis instead ofnil. Writing a reset snapshot here, or filtering zero-qty holdings, would keep the reverse path aligned with the forward tracker behavior.Based on learnings "store cost_basis on Holding as a per-share value (not total position cost)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/holding/reverse_calculator.rb` around lines 96 - 99, When tracker.average_cost becomes nil on liquidation, you must record a reset snapshot instead of skipping it so cost_basis_for doesn't reuse stale values; in reverse_calculator.rb update the block that currently does "average_cost = tracker.average_cost / next if average_cost.nil?" to always push a snapshot for trade.security_id using trade_entry.date with nil (or an explicit reset marker) when average_cost is nil, ensuring `@cost_basis_snapshots`[trade.security_id] << [trade_entry.date, average_cost_or_nil] is written in both cases; alternatively, if you prefer filtering, adjust build_holdings to skip zero-qty Holding rows so sold-out days aren’t given a carried-forward basis (reference symbols: tracker.average_cost, `@cost_basis_snapshots`, trade_entry.date, cost_basis_for, build_holdings, Holding).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/models/holding/reverse_calculator.rb`:
- Around line 96-99: When tracker.average_cost becomes nil on liquidation, you
must record a reset snapshot instead of skipping it so cost_basis_for doesn't
reuse stale values; in reverse_calculator.rb update the block that currently
does "average_cost = tracker.average_cost / next if average_cost.nil?" to always
push a snapshot for trade.security_id using trade_entry.date with nil (or an
explicit reset marker) when average_cost is nil, ensuring
`@cost_basis_snapshots`[trade.security_id] << [trade_entry.date,
average_cost_or_nil] is written in both cases; alternatively, if you prefer
filtering, adjust build_holdings to skip zero-qty Holding rows so sold-out days
aren’t given a carried-forward basis (reference symbols: tracker.average_cost,
`@cost_basis_snapshots`, trade_entry.date, cost_basis_for, build_holdings,
Holding).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00446fc5-5d8b-4a80-9bfe-c12cdf42c2a1
📒 Files selected for processing (5)
app/models/holding/cost_basis_tracker.rbapp/models/holding/forward_calculator.rbapp/models/holding/reverse_calculator.rbapp/models/holding/trade_calculator_helpers.rbtest/models/holding/cost_basis_tracker_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- test/models/holding/cost_basis_tracker_test.rb
- app/models/holding/cost_basis_tracker.rb
Thanks @jjmata — both addressed in eb6a3a2: Extracted converted_trade_price into a shared Holding::TradeCalculatorHelpers module, now included by both calculators. |
2 participants
Fixes #2086.
Summary
The holding cost-basis calculators track a weighted average but only count buy trades — sells never relieve the running tracker (
next unless trade.qty > 0). Cost basis is therefore averaged over every buy the account ever made for a security, including shares already sold. After a position is fully sold and later repurchased, the old (realized) lots stay in the denominator and corrupt the cost basis of the currently-held lot, which feedsHolding#avg_cost(holdings table, holding page, APIavg_cost, and unrealized gain/loss).Example: buy 10 @ $100, sell all 10 @ $300, re-buy 10 @ $300 -> reported cost basis $200/share instead of the correct $300, overstating unrealized gain by $1,000.
Changes
Holding::CostBasisTracker(PORO) encapsulating the weighted-average method: buys raise the basis, sells relieve quantity at the running average (leaving the per-share average unchanged), and a full liquidation resets the basis so a later repurchase starts clean. Guards against over-selling.Holding::ForwardCalculator#update_cost_basis_trackernow applies every trade (buys and sells) through the tracker.Holding::ReverseCalculator#precompute_cost_basisuses the tracker and snapshots cost basis only while a position is held.ForwardCalculatorregression test covering buy -> sell-all -> re-buy.Why this is safe (no regression on existing holdings)
Relieving a sell at the running average leaves the per-share average unchanged for a simple partial trim, so positions that were never fully closed are unaffected. Only the sold-off-and-repurchased case changes — to the correct value. The existing
forward portfolio calculationtest (which asserts qty/price/amount) is unaffected.Scope note: the
Holding#calculate_avg_costSQL fallback (used only for holdings without a pre-computed cost basis) shares the same buys-only limitation, but is intentionally left out of this PR to keep it orthogonal to the separate FX-rate concern tracked in #2012/#2014.Testing
Summary by CodeRabbit
New Features
Tests