fix(holdings): use trade-date FX rate for cost basis price conversion#2014
fix(holdings): use trade-date FX rate for cost basis price conversion#2014dale053 wants to merge 10 commits into
Conversation
superagent-security
Bot
added
the
contributor:verified
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBoth holding calculators now convert trade prices into account currency using the trade date and an optional provider-supplied ChangesTrade-date FX conversion for cost basis
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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/forward_calculator.rb`:
- Around line 90-96: The rescue for Money::ConversionError currently falls back
to trade.price and can mix currencies; change it to NOT silently convert: in the
rescue block log the conversion failure (include trade.id, trade.currency,
account.currency, trade_entry.date, trade.exchange_rate), enqueue an FX backfill
job (e.g. call your existing FxRateBackfillJob/FXBackfillWorker with
trade.currency, account.currency, trade_entry.date) and set converted_price =
nil (or return nil/skip this trade from the surrounding method) so the holding
cost is not updated; keep Money::ConversionError as the exception to catch and
ensure the method that uses converted_price correctly handles a nil value.
🪄 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: d29cd504-aa03-4ab4-883c-0ec91e42600a
📒 Files selected for processing (5)
app/models/holding/forward_calculator.rbapp/models/holding/reverse_calculator.rbtest/models/holding/forward_calculator_test.rbtest/models/holding/reverse_calculator_test.rbtest/support/entries_test_helper.rb
superagent-security
Bot
removed
the
contributor:verified
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
The core fix (passing date: trade_entry.date, custom_rate: trade.exchange_rate to exchange_to) is correct and well-motivated. Tests are comprehensive — trade-date DB rate, provider-supplied override, and failure path are all covered.
One inconsistency between the two calculators worth flagging:
ForwardCalculator (correct) — on Money::ConversionError, uses next to skip the trade entirely, resulting in nil cost basis. The warning log and the test "cost_basis is nil and trade is skipped when FX conversion fails" both document this intentional behavior.
ReverseCalculator (unchanged fallback) — on Money::ConversionError, still falls back to converted_price = trade.price, using the raw numeric price without currency conversion. For a 10-share EUR trade at €100, this silently produces a cost basis of $100 USD instead of nil, mixing currencies with no warning.
The PR description says "If FX conversion fails, the trade is excluded from cost-basis (resulting in nil)" — that describes ForwardCalculator only. ReverseCalculator still takes the old silent-fallback path, and there's no corresponding "cost_basis is nil and trade is skipped when FX conversion fails" test for the reverse path.
Suggest aligning ReverseCalculator's rescue branch to also use next (and optionally logging the same warning), so both calculators produce consistent output and the PR description accurately reflects the full change.
Generated by Claude Code
…ForwardCalculator
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 94-101: The rescue for Money::ConversionError currently does
"next", which only skips the failed trade and allows later snapshots to be built
for the same security; change the behavior so that once an FX conversion fails
for a security we stop computing any further snapshots for that security and
cause cost_basis_for to return nil for subsequent dates. Locate the rescue block
in Holding::ReverseCalculator (the one referencing trade, trade_entry and
trade.exchange_rate), and replace the "next" with logic that halts snapshot
construction for this security — e.g. set a flag or mark the security as invalid
for FX in the calculator's state (or break/return out of the snapshot-building
loop) so that cost_basis_for detects the flag and returns nil for later dates
instead of continuing to compute averages.
🪄 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: 3bf4a02d-423b-44bf-b9dd-b950fcd2e0ad
📒 Files selected for processing (2)
app/models/holding/reverse_calculator.rbtest/models/holding/reverse_calculator_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/models/holding/reverse_calculator_test.rb
…n failure in ReverseCalculator
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/holding/reverse_calculator_test.rb (1)
295-326: ⚡ Quick winConsider verifying intermediate holdings for more comprehensive test coverage.
The test correctly validates that
cost_basisisnilafter an FX conversion failure persists even when later trades succeed. However, it only asserts the final state (today's holding). To more thoroughly verify the "invalidate all dates after failure" behavior, consider adding assertions for:
- Holdings on
first_buydate (5/20) should have validcost_basis(≈110.0 = 100 EUR × 1.10)- Holdings on/after
failed_buydate (5/27, 5/30) should havenilcost_basis- The today holding still has correct
qtyandamountdespitenilcost_basisThis would provide stronger evidence that the invalidation happens precisely at the failure point and propagates forward correctly.
🧪 Proposed enhancement with intermediate assertions
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) 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"🤖 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 `@test/models/holding/reverse_calculator_test.rb` around lines 295 - 326, Add intermediate assertions to the test to verify holdings around the FX failure point: after creating prices, exchange rates, trades and computing calculated = Holding::ReverseCalculator.new(`@account`, portfolio_snapshot: snapshot).calculate, locate holdings by date/security (use snapshot and eur_stock) and assert that the holding on first_buy has a non-nil cost_basis ≈ 100 * 1.10, the holdings on failed_buy and later_buy (and Date.current) have nil cost_basis, and that today_holding (found as the element where h.date == Date.current && h.security == eur_stock) still has the expected qty and amount despite cost_basis being nil; reference methods/objects: Holding::ReverseCalculator, create_trade, ExchangeRate, Security::Price, today_holding and snapshot to find and assert the intermediate values.
🤖 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.
Nitpick comments:
In `@test/models/holding/reverse_calculator_test.rb`:
- Around line 295-326: Add intermediate assertions to the test to verify
holdings around the FX failure point: after creating prices, exchange rates,
trades and computing calculated = Holding::ReverseCalculator.new(`@account`,
portfolio_snapshot: snapshot).calculate, locate holdings by date/security (use
snapshot and eur_stock) and assert that the holding on first_buy has a non-nil
cost_basis ≈ 100 * 1.10, the holdings on failed_buy and later_buy (and
Date.current) have nil cost_basis, and that today_holding (found as the element
where h.date == Date.current && h.security == eur_stock) still has the expected
qty and amount despite cost_basis being nil; reference methods/objects:
Holding::ReverseCalculator, create_trade, ExchangeRate, Security::Price,
today_holding and snapshot to find and assert the intermediate values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0133b86-aae0-4aeb-a88c-c4e739c25cb9
📒 Files selected for processing (2)
app/models/holding/reverse_calculator.rbtest/models/holding/reverse_calculator_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/holding/reverse_calculator.rb
…tion in ReverseCalculator
|
Actionable comments posted: 0 |
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Can you update the Rails.log(...) statements to use the new Debug log at https://app.sure.am/settings/debug? Much cleaner and let's people see those without opening the server logs!
|
Hi, @jjmata |
|
Noticed the check runs on the current head commit ( Generated by Claude Code |
|
No commits since May 28 addressed @jjmata's June 1 request to route the Generated by Claude Code |
|
Automated review sweep: the fix and test coverage are solid and the first round of feedback was fully addressed same-day. Two things are still open: the 06-01 ask to route Generated by Claude Code |
4 participants
Summary
Fixes #2012
Cost basis for foreign-currency trades was being converted at today's exchange rate instead of the rate on the trade date, causing cost basis to drift as FX rates move over time.
ForwardCalculatorandReverseCalculatorboth calledtrade_price.exchange_to(account.currency)with no date context, soMoneyresolved the conversion using the most recent available rate.date: trade_entry.dateandcustom_rate: trade.exchange_ratetoMoney#exchange_to, pinning conversion to the trade date and preferring any provider-supplied rate (e.g. from IBKR/Plaid) over a DB lookup.Changes
app/models/holding/forward_calculator.rb— use trade-date rate for cost basis price conversionapp/models/holding/reverse_calculator.rb— same fixtest/models/holding/forward_calculator_test.rb— two new tests: trade-date DB rate and provider-supplied ratetest/models/holding/reverse_calculator_test.rb— same two tests for the reverse pathtest/support/entries_test_helper.rb— exposeexchange_rate:kwarg oncreate_tradehelperTest plan
bin/rails test test/models/holding/forward_calculator_test.rbbin/rails test test/models/holding/reverse_calculator_test.rbbin/rails test(full suite)bin/rubocop -f github -abin/brakeman --no-pagerSummary by CodeRabbit
Bug Fixes
Tests