Skip to content

Carry cost basis forward into gap-filled holdings#2476

Open
cleanjunc wants to merge 1 commit into
we-promise:mainfrom
cleanjunc:fix/gapfill-cost-basis
Open

Carry cost basis forward into gap-filled holdings#2476
cleanjunc wants to merge 1 commit into
we-promise:mainfrom
cleanjunc:fix/gapfill-cost-basis

Conversation

@cleanjunc

Copy link
Copy Markdown

Summary

Holding.gapfill carries the previous day's holding data forward for dates that have no real snapshot, but it left out cost_basis. Every synthesized day was stored with cost_basis = nil, which made Holding#avg_cost fall back to a per-holding trade query and report a different average cost, or no trend at all, on those days. This change carries cost_basis forward alongside the other values so the cost basis stays consistent across consecutive days for the same position.

Root cause

Holding::ForwardCalculator and Holding::ReverseCalculator both compute cost_basis for real, trade-driven dates and then call the shared Holding.gapfill to fill the dates in between. The carry-forward in gapfill copied qty, price, currency, and amount, but not cost_basis, so the value was dropped on weekends, holidays, and trade gaps.

Fix

Carry cost_basis forward from the previous real holding when building a gap-filled holding. This is correct because cost basis only changes on a buy trade, and gap-filled dates have none, so the previous real day's value applies unchanged. The fix lives in the shared gapfill, so both the forward and reverse calculators are covered in one place. cost_basis_source is intentionally not copied: calculator holdings never set it, and the materializer assigns the source when it persists the row.

Testing

Added a test in test/models/holding/forward_calculator_test.rb that creates a single trade so the days after the trade date have no price of their own and are gap-filled. It asserts that a gap-filled day keeps the same cost_basis as the real trade day. The test fails before the change (the gap-filled day reports nil) and passes after.

bin/rails test test/models/holding/forward_calculator_test.rb

Closes #2475

@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Gap-filled holdings lose cost basis, breaking average cost and trend on carried-forward days

1 participant