mirror of
https://github.com/we-promise/sure.git
synced 2026-09-06 15:14:19 +00:00
buzzromainandClaude Opus 5
07524d0d84
* 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>