fix(snaptrade): import non-primary-currency cash as cash holdings#1979
fix(snaptrade): import non-primary-currency cash as cash holdings#1979dripsmvcp wants to merge 4 commits into
Conversation
Fixes we-promise#1809. SnaptradeAccount#upsert_balances! picked a single cash entry (account currency -> USD -> first) and stored only that in cash_balance; every other currency's cash was discarded. A moomoo Canada account with CAD $500 + USD $1000 imported only the CAD. Persist the full balances snapshot (new raw_balances_payload column) and surface each non-primary-currency cash entry as a synthetic per-currency cash holding (Security.cash_for(account, currency:)), mirroring the existing cash-security pattern. The primary currency stays in cash_balance. HoldingsProcessor now also runs for cash-only balances, and the Processor invokes it when there are holdings OR non-primary cash. Cash holdings use a stable external_id so repeated syncs update rather than duplicate.
superagent-security
Bot
added
the
contributor:verified
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPersists full SnapTrade balance snapshots, selects a primary cash entry for ChangesMulti-currency cash holdings on SnapTrade
Sequence DiagramsequenceDiagram
participant Processor
participant HoldingsProcessor
participant Account
participant Security
participant ImportAdapter
Processor->>HoldingsProcessor: run when raw_holdings_payload.present? OR non_primary_cash_entries.any?
HoldingsProcessor->>Account: non_primary_cash_entries
Account-->>HoldingsProcessor: [{currency, amount}...]
loop for each non-primary entry
HoldingsProcessor->>Security: Security.cash_for(account, currency: entry.currency)
Security-->>HoldingsProcessor: cash_security
HoldingsProcessor->>ImportAdapter: import_holding(quantity=amount, amount=amount, price=1, external_id)
ImportAdapter-->>HoldingsProcessor: import result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0eba9cc34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Only update cash_balance, preserve current_balance (total account value) | ||
| update!(cash_balance: cash_value) | ||
| # Only update cash_balance, preserve current_balance (total account value) | ||
| update!(cash_balance: cash_value, raw_balances_payload: data) |
There was a problem hiding this comment.
upsert_balances! now stores the full SnapTrade balances snapshot in raw_balances_payload, but this attribute is not added to the model’s encrypted fields (only raw_payload, raw_transactions_payload, raw_holdings_payload, and raw_activities_payload are encrypted). In environments where ActiveRecord encryption is enabled, this writes account cash-by-currency data in plaintext at rest, which is a regression in handling provider raw payload sensitivity and should be fixed by encrypting this column too.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/snaptrade_account/holdings_processor.rb (1)
53-53: 💤 Low valueAvoid logging monetary amounts at info level.
Per project learnings, financial event code should not log sensitive monetary amounts at info level. Consider lowering to debug or removing the amount from the log message.
Suggested change
- Rails.logger.info "SnaptradeAccount::HoldingsProcessor - Importing #{entry[:currency]} cash holding: #{amount}" + Rails.logger.info "SnaptradeAccount::HoldingsProcessor - Importing #{entry[:currency]} cash holding"Based on learnings: "In financial event code (e.g., settlement/maturity flows in app/models/**/*.rb), do not log sensitive monetary amounts (such as gross, tax, net) at info level."
🤖 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/snaptrade_account/holdings_processor.rb` at line 53, The log in SnaptradeAccount::HoldingsProcessor that currently prints monetary amounts (the Rails.logger.info call "SnaptradeAccount::HoldingsProcessor - Importing #{entry[:currency]} cash holding: #{amount}") should not output sensitive amounts at info level; change this call to either Rails.logger.debug and keep the amount only at debug level, or remove the #{amount} from the message and keep an info-level log that only includes non-sensitive context (e.g., currency and action). Locate the log statement in the HoldingsProcessor and update it so amounts are never emitted at info level.
🤖 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 `@app/models/snaptrade_account/holdings_processor.rb`:
- Line 53: The log in SnaptradeAccount::HoldingsProcessor that currently prints
monetary amounts (the Rails.logger.info call
"SnaptradeAccount::HoldingsProcessor - Importing #{entry[:currency]} cash
holding: #{amount}") should not output sensitive amounts at info level; change
this call to either Rails.logger.debug and keep the amount only at debug level,
or remove the #{amount} from the message and keep an info-level log that only
includes non-sensitive context (e.g., currency and action). Locate the log
statement in the HoldingsProcessor and update it so amounts are never emitted at
info level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7375c7c5-b6fc-4e59-81d6-8cd3a2fba454
📒 Files selected for processing (7)
app/models/security.rbapp/models/snaptrade_account.rbapp/models/snaptrade_account/holdings_processor.rbapp/models/snaptrade_account/processor.rbdb/migrate/20260525121841_add_raw_balances_payload_to_snaptrade_accounts.rbdb/schema.rbtest/models/snaptrade_account_processor_test.rb
|
Nice PR! The approach of using synthetic cash holdings for non-primary currencies is architecturally consistent with existing patterns in the codebase — A few things worth double-checking:
Overall the implementation looks solid and well-tested (45 passing tests is great coverage). The critical thing to be confident about is point #1 — the USD fallback exclusion logic is the main place double-counting could sneak in. To reply, just mention @dosu. Share context across your team and agents. Try Dosu.
|
…m log Addresses PR we-promise#1979 review: Codex P1 (encrypt the newly persisted balances snapshot at rest, matching the other raw provider payloads) and CodeRabbit nitpick (do not log monetary amounts at info level).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/snaptrade_account.rb (1)
155-172: 💤 Low valueConsider extracting primary cash entry selection to avoid logic duplication.
The primary cash selection logic (account currency → USD → first) at lines 160-162 is duplicated from
upsert_balances!(lines 139-141). Extracting to a private helper would ensure both stay in sync.♻️ Proposed refactor
+ private + + def primary_cash_entry(entries) + entries.find { |b| b.dig(:currency, :code) == currency } || + entries.find { |b| b.dig(:currency, :code) == "USD" } || + entries.first + endThen use
primary_cash_entry(data)in bothupsert_balances!andnon_primary_cash_entries.🤖 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/snaptrade_account.rb` around lines 155 - 172, The primary-cash-selection logic is duplicated between non_primary_cash_entries and upsert_balances!; extract that logic into a private helper (e.g., primary_cash_entry or primary_cash_code) that accepts the balances payload (or parsed entries) and returns the primary entry or its currency code using the account currency → "USD" → first fallback; then replace the inline selection in non_primary_cash_entries and upsert_balances! to call this helper so both use the same source of truth.
🤖 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 `@app/models/snaptrade_account.rb`:
- Around line 155-172: The primary-cash-selection logic is duplicated between
non_primary_cash_entries and upsert_balances!; extract that logic into a private
helper (e.g., primary_cash_entry or primary_cash_code) that accepts the balances
payload (or parsed entries) and returns the primary entry or its currency code
using the account currency → "USD" → first fallback; then replace the inline
selection in non_primary_cash_entries and upsert_balances! to call this helper
so both use the same source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74305bd6-5af4-4c0f-8c58-e5eaf5670494
📒 Files selected for processing (2)
app/models/snaptrade_account.rbapp/models/snaptrade_account/holdings_processor.rb
|
The design is sound — persisting the full balance snapshot and deriving synthetic holdings from non-primary entries is the right approach. One item worth verifying before merge: Potential key-type mismatch in @snaptrade_account.upsert_balances!([
{ "currency" => { "code" => "USD" }, "cash" => "1500.00" },
{ "currency" => { "code" => "EUR" }, "cash" => "800.00" }
])But the existing implementation uses symbol-key access: Other observations:
Generated by Claude Code |
PR we-promise#1979 review: extract the shared account-currency->USD->first cash selection into a private helper (CodeRabbit DRY nitpick); reorder the upsert_balances! test so the primary currency is not first, proving dig(:currency,:code) resolves it on string-keyed payloads rather than the entries.first fallback (jjmata).
|
Thanks @jjmata. Confirmed the key-type path: |
2 participants
Summary
Fixes #1809. SnapTrade brokerage accounts holding cash in multiple currencies only imported the primary currency's cash — every other currency was silently discarded (e.g. a moomoo Canada account with CAD $500 + USD $1000 imported only the CAD). Non-primary cash is now surfaced as per-currency synthetic cash holdings.
Root cause
SnaptradeAccount#upsert_balances!selected a single cash entry (account currency → USD → first) intocash_balanceand dropped the rest.Fix
raw_balances_payloadcolumn persists the full balances snapshot (was thrown away).upsert_balances!keeps the primary currency incash_balanceand stores all entries;non_primary_cash_entriesexcludes the actual primary (including the USD fallback) so nothing double-counts.Security.cash_for(account, currency:)gains an optional currency → distinct per-currency synthetic cash security ("Cash (USD)"); existing no-arg callers unchanged.HoldingsProcessoremits a synthetic cash holding per non-primary currency with a stableexternal_id(idempotent across re-syncs), and runs even when there are no security holdings.Processorinvokes the holdings processor when there are holdings or non-primary cash.Test plan
cash_forper-currency variants.snaptrade_account_processor_test,snaptrade_account_test,security_test,activities_processor_test); rubocop + brakeman clean.Before / after
A CAD account with CAD $500 + USD $1000 cash:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores