Files
sure/app/models/trade.rb
T
buzzromainandClaude Opus 5 07524d0d84 fix(holdings): a transfer must not set a cost basis (#3154)
* fix(holdings): a transfer must not set a cost basis

calculate_avg_cost sums every trade with a positive quantity, so an asset moved
in from elsewhere is counted as bought on the day it arrived. A coin acquired at
30k and transferred in at 60k reports a cost of 60k and no gain at all — a
number that looks authoritative and is wrong.

Nothing here can know what a transferred asset cost: the purchase happened
somewhere this app never saw. Leaving the cost unknown is what the method
already does when it has nothing to work from, and for the same stated reason
the fallback to market price was removed from it: "Previously this fell back to
current market price, which was misleading."

Two things it would be easy to get wrong, and both are covered:

- **One transfer makes the whole position unknown**, not just its own row.
  Averaging the purchases alone and applying that to every unit is the same
  fabrication in a quieter form: buy one at 30k, receive one, and the position
  reports 30k a unit for two units that did not cost that.
- **Unlabelled purchases are preserved.** `!=` is NULL for a row with no label,
  so a naive exclusion would drop the ordinary trades that carry none — which
  is most of them. Hence IS DISTINCT FROM.

Balances and value are unaffected: they come from holdings, which providers
import from the position itself rather than from trade history.

This reaches every integration that labels a movement as a transfer. Questrade
journals already did; the self-custody wallets do as of #3153.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(holdings): stop a stored figure outranking the transfer guard

Review on #3154, and the reviewers were right that the first pass only
covered half the path.

`Holding#avg_cost` returns a stored `cost_basis` before it ever calls
`calculate_avg_cost`, so the transfer guard was protecting only holdings
that had nothing stored. Worse, the stored value was itself wrong: both
calculators counted every positive-quantity trade toward the running
average, transfers included, and the materializer persisted that as a
`calculated` basis. A coin bought elsewhere at 30k and moved in at 60k
reported no gain at all, and said so with a figure that looks derived.

Fixed in the write path rather than the read one. Adding an `exists?` per
holding to `avg_cost` would have reintroduced exactly the N+1 the stored
value exists to avoid; clearing the stored value instead lets the read
path fall through to the guard that was already there.

Both calculators now exclude transfers from the average and mark the
security's basis unknown — the forward one for good, the reverse one from
the transfer's date onward, since the purchases before it still stand on
their own. `cost_basis_unknown` is carried separately from a nil
`cost_basis` because the materializer treats them differently: nil means
"nothing computed, leave what is there", unknown means "this cannot be
known, clear what is there".

A `manual` or `provider` basis survives. That is somebody asserting what
the position cost them, which is precisely the thing the app cannot derive
for a transfer.

The migration clears figures already stored. Positions heal on the next
materialization anyway, but a manual or disconnected account may not
materialize again for a long time, and the wrong number is not visibly
wrong.

The regression test materializes first and relabels after, because that is
the case that matters: a position already carrying a figure worked out
before anyone knew the movement was a transfer.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

* fix(holdings): clear a transferred basis that recorded no source

Follow-up on the same review. `load_existing_holdings_map` loaded holdings
that were locked, sourced, or provider-owned — so a row carrying a
`cost_basis` with no `cost_basis_source` was invisible to it. The clearing
then saw no existing holding and left the figure standing, which meant the
rows least able to justify the number they hold were the ones that kept it.

The migration takes `[7.2]` to match `schema.rb` and the other 398
migrations, rather than the `[8.0]` I had written.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

* fix(holdings): renumber the migration off a colliding version

`20260825120000` is already taken by `add_consumed_amount_to_goals` on the
goals stack. Two migrations sharing a version is not a merge conflict —
`schema_migrations` is keyed by it, so whichever landed second would be
recorded as already run and skipped in silence. For a data migration that
means transferred positions quietly keeping the cost basis this branch
exists to clear.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

* docs(holdings): say why the basis guard keys on one label, not the set

#3192 landed Trade::INTERNAL_MOVEMENT_LABELS in this file, next to the
TRANSFER_LABEL this branch adds. Both rest on ownership being preserved, so
two constants sitting together invite the question of why the basis guard does
not simply use the broader one.

It could, and that would be a behaviour change: the sweep labels would start
clearing a cost basis too. Nothing has shown a sweep landing on a security, and
widening a guard that erases figures on the strength of a guess is the wrong
direction, so it stays narrow and now says so.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-08-28 07:53:33 +02:00

155 lines
5.3 KiB
Ruby

class Trade < ApplicationRecord
include Entryable, Monetizable
monetize :price
monetize :fee
belongs_to :security
belongs_to :category, optional: true
# Use the same activity labels as Transaction
ACTIVITY_LABELS = Transaction::ACTIVITY_LABELS.dup.freeze
# The labels that mean the asset went somewhere else you own rather than
# being bought or sold.
#
# Deliberately NOT `Transaction::INTERNAL_MOVEMENT_LABELS`, which also holds
# "Exchange". On cash that means a currency exchange and is internal; on a
# security the label covers "currency **or security** exchanges"
# (docs/onboarding/guide.md), and a security-for-security exchange can
# dispose of an appreciated asset.
#
# The two errors are not symmetrical. Listing a movement that was not a sale
# is visible and correctable; erasing a realized gain is neither — it simply
# is not there. So only labels that unambiguously preserve ownership are
# excluded, and an ambiguous one is left where the user can see it.
INTERNAL_MOVEMENT_LABELS = %w[Transfer Sweep\ In Sweep\ Out].freeze
# Moving an asset between places you own is not an acquisition, so it must not
# set a cost basis. Named here because Holding reads it.
#
# A single label rather than INTERNAL_MOVEMENT_LABELS above, though both rest
# on ownership being preserved: this one is the label the onchain processor
# writes, and the only one seen setting a basis it should not. Widening the
# basis guard to the sweep labels would change which holdings lose their
# basis, and nothing has shown a sweep landing on a security — so it stays
# narrow until something does.
TRANSFER_LABEL = "Transfer".freeze
validates :qty, presence: true
validates :price, :currency, presence: true
validates :investment_activity_label, inclusion: { in: ACTIVITY_LABELS }, allow_nil: true
def exchange_rate
extra&.dig("exchange_rate")
end
def exchange_rate=(value)
if value.blank?
self.extra = (extra || {}).merge("exchange_rate" => nil, "exchange_rate_invalid" => false)
else
begin
normalized_value = Float(value)
raise ArgumentError unless normalized_value.finite?
self.extra = (extra || {}).merge("exchange_rate" => normalized_value, "exchange_rate_invalid" => false)
rescue ArgumentError, TypeError
self.extra = (extra || {}).merge("exchange_rate" => value, "exchange_rate_invalid" => true)
end
end
end
validate :exchange_rate_must_be_valid
# Trade types for categorization
def buy?
qty.positive?
end
def sell?
qty.negative?
end
# A negative quantity that left for another account you own. It looks exactly
# like a sale — same sign, same shape — and only the label tells them apart.
def internal_movement?
INTERNAL_MOVEMENT_LABELS.include?(investment_activity_label)
end
class << self
def build_name(type, qty, ticker)
prefix = type == "buy" ? "Buy" : "Sell"
"#{prefix} #{qty.to_d.abs} shares of #{ticker}"
end
end
def unrealized_gain_loss
return nil unless qty.positive?
current_price = security.current_price
return nil if current_price.nil?
current_value = current_price * qty.abs
cost_basis = price_money * qty.abs
Trend.new(current: current_value, previous: cost_basis)
end
# Calculates realized gain/loss for sell trades based on avg_cost at time of sale
# Returns nil for buy trades or when cost basis cannot be determined
def realized_gain_loss
return @realized_gain_loss if defined?(@realized_gain_loss)
@realized_gain_loss = calculate_realized_gain_loss
end
# Trades are always excluded from expense budgets
# They represent portfolio management, not living expenses
def excluded_from_budget?
true
end
private
def exchange_rate_must_be_valid
if extra&.dig("exchange_rate_invalid")
errors.add(:exchange_rate, "must be a number")
elsif exchange_rate.present?
numeric_rate = Float(exchange_rate) rescue nil
if numeric_rate.nil? || !numeric_rate.finite? || numeric_rate <= 0
errors.add(:exchange_rate, "must be greater than 0")
end
end
end
def calculate_realized_gain_loss
return nil unless sell?
# Moving an asset to another account you own realises nothing. Without
# this the cost basis is compared against the day's price and the
# difference is booked as a gain the user never made.
return nil if internal_movement?
# Use preloaded holdings if available (set by reports controller to avoid N+1)
# Treat defined-but-empty preload as authoritative to prevent DB fallback
holding = if defined?(@preloaded_holdings)
# Use select + max_by for deterministic selection regardless of array order
(@preloaded_holdings || [])
.select { |h| h.security_id == security_id && h.date <= entry.date }
.max_by(&:date)
else
# Fall back to database query only when not preloaded
entry.account.holdings
.where(security_id: security_id)
.where("date <= ?", entry.date)
.order(date: :desc)
.first
end
return nil unless holding&.avg_cost
cost_basis = holding.avg_cost * qty.abs
sale_proceeds = price_money * qty.abs
Trend.new(current: sale_proceeds, previous: cost_basis)
end
end