Skip to content

refactor(shared): remove dead MoneyAmount twins (USD/JMD/BTC) [ENG-518]#451

Merged
islandbitcoin merged 3 commits into
mainfrom
fix/eng-518-dedup-moneyamount
Jul 17, 2026
Merged

refactor(shared): remove dead MoneyAmount twins (USD/JMD/BTC) [ENG-518]#451
islandbitcoin merged 3 commits into
mainfrom
fix/eng-518-dedup-moneyamount

Conversation

@islandbitcoin

Copy link
Copy Markdown
Contributor

Why

Follow-up to the cashout #449#450 saga. src/domain/shared/MoneyAmount.ts carried duplicate USDAmount / JMDAmount / BtcAmount classes that were dead — the @domain/shared barrel serves those from ./money (export * from "./money") and takes only USDTAmount from this file. That shadow twin is exactly what let #449's JMD-rate fix land on the wrong copy while cashout stayed broken; #450 fixed the live ./money/JMDAmount.ts.

Change

Strip the file to its only live exports — the base MoneyAmount + USDTAmount — and delete the dead USDAmount/JMDAmount/BtcAmount. MoneyAmount.from()/fromJSON() (used only by tests, only with USDT; the sole prod reference is commented out) are trimmed to the USDT branch. A header comment documents the split so no currency-amount class gets re-added here.

Verification

  • tsc -p tsconfig.json --noEmit: clean project-wide — nothing referenced the deleted classes.
  • Money.spec.ts: 22/22 — including the barrel-resolved JMDAmount fractional-rate regression (fix(cashout): decimal-safe JMD rate on the ACTUAL JMDAmount used by cashout #450), the USDAmount → JMDAmount conversion, and the base MoneyAmount.from(Usdt) / fromJSON round-trip that exercise the trimmed factory.

Closes ENG-518.

🤖 Generated with Claude Code

The top-level src/domain/shared/MoneyAmount.ts defined USDAmount, JMDAmount,
and BtcAmount that were dead: the @domain/shared barrel resolves those from
./money (export * from "./money") and takes only USDTAmount from this file.
The duplicates shadowed the live ./money classes, which let the cashout JMD
rate fix land on the wrong copy (#449 patched here; #450 fixed the real
./money one).

Delete the three dead classes; scope this file and MoneyAmount.from() to the
base + USDTAmount, its only live exports. Comment added so no currency-amount
class is re-added here.

Verified: tsc --noEmit clean project-wide; Money.spec.ts 22/22 (it imports
JMDAmount from the barrel = the live ./money class, and exercises
MoneyAmount.from(Usdt)/fromJSON via the trimmed factory).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@linear

linear Bot commented Jul 15, 2026

Copy link
Copy Markdown

ENG-518

The previous header claimed this file was "the canonical home of the base
MoneyAmount". It is not: the @domain/shared barrel does `export * from "./money"`,
so ./money/MoneyAmount.ts is the base the app resolves as `MoneyAmount`; this file
only contributes `USDTAmount`. Since USDTAmount extends this file's (separate) base,
`x instanceof MoneyAmount` (the barrel's) does not match it — the comment now states
that and points at the OffersSerde `|| instanceof USDTAmount` guard, and flags the
two-base consolidation as the real follow-up. Also notes in Money.spec.ts why it
imports MoneyAmount from the file path rather than the barrel.

Comment-only; no behavioral change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@islandbitcoin

islandbitcoin commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Review — refactor(shared): remove dead MoneyAmount twins

Net: correct and safe. The deletion is verified — the removed USDAmount / JMDAmount / BtcAmount were never barrel-exported (the @domain/shared barrel does export * from "./money" and takes only { USDTAmount } from this file), nothing imports them from the file path, and tsc is clean project-wide. This is good, incident-preventing cleanup. Four findings — two already addressed in 0a7f2f1a6, two left as follow-ups.

A — [addressed in 0a7f2f1a6] The header comment was factually wrong. It claimed this file is "the canonical home of the base MoneyAmount." It isn't: the barrel exports ./money/MoneyAmount.ts as MoneyAmount, and this file's base is reached only via USDTAmount's inheritance plus one direct test import. A fix landing on the wrong MoneyAmount copy is exactly what this PR exists to prevent, so a comment asserting the wrong file is canonical is a landmine for the next debugger. Rewritten to describe the split accurately, including the instanceof consequence in B.

B — [open, tracked as ENG-523] The base-class duplication is still standing. There are two abstract class MoneyAmount in two files named MoneyAmount.ts, and they've diverged (./money's has abstract i18n() and a different asPaymentAmount() return type). Because USDTAmount extends this file's base and not the barrel's, x instanceof MoneyAmount (the barrel's) does not match a USDTAmount — see the live workaround at src/app/offers/storage/OffersSerde.ts:16 (value instanceof MoneyAmount || value instanceof USDTAmount). This PR removes the leaf twins but leaves the more dangerous base duplication. The real fix — USDTAmount implementing ./money's base and moving there, deleting this file, exporting everything from ./money, and dropping the || instanceof USDTAmount crutches — touches ~73 USDTAmount call sites and instanceof semantics, so it belongs in its own ticket, not this one. Tracked as ENG-523.

C — [open, author's call] from() / fromJSON() is now a one-type factory. After the trim it constructs only USDTAmount and returns UnsupportedCurrencyError for everything else, while ./money already provides toMoneyAmount / toMoneyAmountFromJSON for USD/JMD. It's not dead — it's the only generic USDT-JSON restore path (./money's factory deliberately doesn't handle USDT) — so keeping it is defensible, but a factory that makes exactly one thing is a smell. Worth deciding its fate alongside B.

D — [addressed in 0a7f2f1a6] The test reaches past the barrel. Money.spec.ts imports MoneyAmount from @domain/shared/MoneyAmount (the file, not the barrel) to hit this USDT-scoped base — the same deep coupling that made "which MoneyAmount?" ambiguous in the first place. Added a comment noting why it bypasses the barrel.

A and D are pushed in 0a7f2f1a6. B is tracked as ENG-523; C is your call.

…518]

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@islandbitcoin

Copy link
Copy Markdown
Contributor Author

Review dispositions — all four findings resolved

Thanks for the review. Closing out the loop so this is merge-ready:

  • A (header comment was wrong) — ✅ fixed in 0a7f2f1a6.
  • D (test reaches past the barrel) — ✅ commented in 0a7f2f1a6.
  • B (base-class duplication still standing) — deferred to its own ticket, as you suggested: ENG-523. It's out of scope for a "delete dead code" PR — the real fix moves USDTAmount onto ./money's base and drops the || instanceof USDTAmount crutches across ~73 call sites. The header comment now points there explicitly (8b14004b: follow-up: ENG-523) so the next debugger lands on the ticket, not a bare "(follow-up)".
  • C (from()/fromJSON() is a one-type factory)keeping it, folded into ENG-523. It's not dead: it's the generic USDT-JSON restore entry (./money's toMoneyAmount/toMoneyAmountFromJSON deliberately skip USDT) and is exercised by Money.spec.ts; its only prod reference is commented out (payment-input-validator.ts:33). Removing or reshaping it means touching ./money's factory too, which is exactly the base-consolidation work in B — so its fate is decided there, not in this cleanup PR.

This PR satisfies ENG-518's second acceptance path ("keep the top-level file solely for base MoneyAmount + USDTAmount with a comment explaining the split"). CI is green project-wide. Ready to merge.

@islandbitcoin
islandbitcoin merged commit 5af4416 into main Jul 17, 2026
15 checks passed
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.

2 participants