refactor(core): promote Money value object to BuildingBlocks and adopt in Billing top-ups by marcelo-maciel · Pull Request #1316 · fullstackhero/dotnet-starter-kit

marcelo-maciel · 2026-06-26T17:48:45Z

What

Promote the Money value object from the Catalog module to BuildingBlocks/Core
(FSH.Framework.Core.Domain) as a shared domain primitive, enrich it, and adopt
it where a single self-contained monetary value fits cleanly.

Why

Money lived only in Catalog, while Billing modeled monetary amounts as a raw
decimal plus a parallel Currency string, with no invariant binding the two.
That is the same concept modeled two ways in one codebase.

Changes

Money moves to FSH.Framework.Core.Domain (dependency-free, passes the
BuildingBlocks independence/immutability/layering arch tests).
Enriched with same-currency-guarded Add/Subtract/Multiply (+ operators)
and away-from-zero Round. Money now allows signed amounts (ledger semantics);
non-negative rules stay at the use-site validators.
Adopted as a persisted owned type only where it is a clean single-value fit:
- Catalog.Product.Price (namespace move only, schema unchanged).
- Billing.TopupRequest.Amount (folds the parallel Currency column into Money
  via OwnsOne, preserving the existing Amount/Currency columns).
Multi-amount, single-currency aggregates (BillingPlan, Invoice, Wallet)
intentionally keep a single Currency field to avoid denormalizing currency
across owned columns.

Migration

The TopupRequest migration is a no-op: OwnsOne preserves the columns, so only
the EF model snapshot changes.

Testing

Build clean (-warnaserror). Unit: Framework/Architecture/Catalog/Billing green.
Integration (Testcontainers/Postgres): 730 passed, exercising the top-up Money
round-trip on a real database. No public contract/DTO change, so no frontend impact.

Follow-up (separate PR, if this is accepted)

Propagate the shared Money into Billing's internal arithmetic without a schema
change: same-currency-guarded subtotal summation in Invoice, balance/credit/debit
operations in Wallet, and line-item rounding. This PR keeps the surface minimal;
the follow-up improves correctness on the multi-amount aggregates.

…ch it Money lived only in Catalog while Billing modeled monetary amounts as a raw decimal plus a parallel currency string, with no invariant binding the two. Promote Money to BuildingBlocks/Core (FSH.Framework.Core.Domain) as a shared domain primitive and enrich it with same-currency-guarded arithmetic (Add/Subtract/Multiply), away-from-zero rounding, and matching operators. Money now allows signed amounts for ledger semantics; non-negative rules stay at the use-site validators (product price, top-up amount). Adopt the shared Money where it is a clean single-value fit: - Catalog.Product.Price: namespace move only, schema unchanged. - Billing.TopupRequest.Amount: folds the parallel Currency column into Money via OwnsOne, preserving the existing "Amount" and "Currency" columns. Multi-amount, single-currency aggregates (BillingPlan, Invoice, Wallet) keep a single Currency field, to avoid denormalizing currency across owned columns. The TopupRequest migration is a no-op: OwnsOne preserves the columns, so only the EF model snapshot changes.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 94127a5544

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-06-26T17:57:30Z

+namespace FSH.Framework.Core.Domain;
+
+public sealed record Money


Get approval before changing BuildingBlocks

This adds a shared domain primitive under src/BuildingBlocks, but AGENTS.md golden rule 4 says “Do NOT modify src/BuildingBlocks without explicit approval” and .agents/rules/buildingblocks-protection.md says to “Confirm explicit approval.” I don't see that approval in this reviewed change/context; because Core is consumed by every module, this should not land until the approval is recorded or the type stays module-local.

Useful? React with 👍 / 👎.

Acknowledged. This BuildingBlocks change is deliberate, not incidental: it promotes Money to a shared domain primitive, stays dependency-free, and passes the BuildingBlocks independence/immutability/layering arch tests. Whether the golden-rule-4 approval bar is satisfied is a maintainer call, so flagging it here for explicit sign-off rather than self-approving.

chatgpt-codex-connector · 2026-06-26T17:57:30Z

-        if (amount < 0)
-        {
-            throw new ArgumentOutOfRangeException(nameof(amount), "Amount cannot be negative.");


Preserve Catalog price non-negativity

Removing this guard makes the shared Money accept negative amounts, but Product.Create/ChangePrice still accept a Money directly and do not check Price.Amount; any catalog import, seed, test helper, or internal caller that bypasses the two command validators can now persist a negative product price. Move the non-negative price invariant into the Catalog aggregate before deleting the Catalog-specific Money guard.

Useful? React with 👍 / 👎.

Resolved in 7864674. The promoted Money now allows signed amounts (ledger semantics), so the non-negative price invariant moved onto the Catalog aggregate: Product.Create and ChangePrice throw ArgumentOutOfRangeException when price.Amount < 0, so any caller that bypasses the command validators (seed, import, test helpers) can no longer persist a negative price. Added unit tests for both paths.

chatgpt-codex-connector · 2026-06-26T17:57:30Z

@@ -1,4 +1,5 @@
 using System.Net;
+using FSH.Framework.Core.Domain;


Keep the Catalog snapshot in sync

This switches Catalog code to the new FSH.Framework.Core.Domain.Money, but the current Catalog model snapshot still has OwnsOne("FSH.Modules.Catalog.Domain.Money", "Price") at src/Host/FSH.Starter.Migrations.PostgreSQL/Catalog/CatalogDbContextModelSnapshot.cs:263. Because EF diffs future migrations from that snapshot, the next unrelated Catalog migration will inherit this CLR-type rename/snapshot drift instead of this refactor; include a no-op Catalog migration or snapshot update here.

Useful? React with 👍 / 👎.

Resolved in 7864674. Added a no-op Catalog migration (CatalogMoneyValueObject) whose model snapshot now references FSH.Framework.Core.Domain.Money instead of the removed FSH.Modules.Catalog.Domain.Money. OwnsOne preserves the PriceAmount/Currency columns, so Up/Down are empty (with an explanatory comment) and there is no schema change. Mirrors the Billing TopupRequest no-op already in this PR.

…snapshot The shared Money now allows signed amounts (ledger semantics), so the non-negative price invariant must live on the Catalog aggregate rather than the value object. Guard Product.Create and ChangePrice so any caller that bypasses the command validators (seed, import, test helpers) cannot persist a negative price. Add a no-op Catalog migration so the model snapshot references the promoted FSH.Framework.Core.Domain.Money owned type instead of the removed FSH.Modules.Catalog.Domain.Money; otherwise the next unrelated Catalog migration would inherit this CLR-type rename. Mirrors the Billing TopupRequest no-op already in this PR. Addresses Codex review feedback on fullstackhero#1316.

chatgpt-codex-connector · 2026-06-26T19:57:18Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

…snapshot The shared Money now allows signed amounts (ledger semantics), so the non-negative price invariant must live on the Catalog aggregate rather than the value object. Guard Product.Create and ChangePrice so any caller that bypasses the command validators (seed, import, test helpers) cannot persist a negative price. Add a no-op Catalog migration so the model snapshot references the promoted FSH.Framework.Core.Domain.Money owned type instead of the removed FSH.Modules.Catalog.Domain.Money; otherwise the next unrelated Catalog migration would inherit this CLR-type rename. Mirrors the Billing TopupRequest no-op already in this PR. Addresses Codex review feedback on fullstackhero#1316.

chatgpt-codex-connector · 2026-06-26T19:58:27Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

…snapshot The shared Money now allows signed amounts (ledger semantics), so the non-negative price invariant must live on the Catalog aggregate rather than the value object. Guard Product.Create and ChangePrice so any caller that bypasses the command validators (seed, import, test helpers) cannot persist a negative price. Add a no-op Catalog migration so the model snapshot references the promoted FSH.Framework.Core.Domain.Money owned type instead of the removed FSH.Modules.Catalog.Domain.Money; otherwise the next unrelated Catalog migration would inherit this CLR-type rename. Mirrors the Billing TopupRequest no-op already in this PR. Addresses Codex review feedback on fullstackhero#1316.

chatgpt-codex-connector · 2026-06-26T20:00:14Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

iammukeshm

Verdict: direction approved — one change requested before merge.

Promoting Money to FSH.Framework.Core.Domain and folding Billing's parallel decimal+Currency into a single value object is the right call. Execution is clean: migrations are honest no-ops with columns preserved (Amount/Currency, PriceAmount/PriceCurrency), the contract TopupRequestDto is unchanged so there's no frontend/API impact, the non-negative invariant is preserved at the use-sites (top-up validator GreaterThan(0m), Product aggregate guards with new tests), and the only TopupRequest.Amount/.Currency consumers (WalletMappings, BillingService) are both updated. Well covered by tests.

Requested change — trim the speculative API from this PR. Add/Subtract/Multiply/Round and the +/-/* operators are added to Money but used nowhere in production code — only in tests. Their first real consumer is exactly the "Follow-up" section of this PR's description. Since Money now lives in the protected BuildingBlocks (highest blast radius), please keep the surface minimal: land just the value type + currency normalization + adoption on TopupRequest.Amount and Product.Price here, and reintroduce the arithmetic together with the follow-up that actually subtotals/credits/debits with it. That keeps each PR's shared-lib surface justified by a real caller.

Non-blocking note. This leaves Billing modeling money two ways (Money on TopupRequest; raw decimal+Currency on Invoice/Wallet/WalletTransaction/InvoiceLineItem/BillingPlan). Acceptable as an incremental step given the stated follow-up, but worth landing that follow-up promptly so the half-state is short-lived.

When the arithmetic is removed (and its Framework.Tests cases moved to the follow-up), this is good to merge.

Money was promoted to BuildingBlocks/Core with same-currency-guarded Add/Subtract/Multiply, away-from-zero Round, and +/-/* operators, but no production code consumes them yet; their first real caller is the Billing subtotal/credit/debit follow-up. Since Money now lives in the protected BuildingBlocks (highest blast radius), keep the shared surface minimal: drop the arithmetic and its operators, leaving the value type, currency normalization, and Zero factory adopted by Product.Price and TopupRequest.Amount. The arithmetic and its Framework.Tests cases will be reintroduced together with the follow-up that actually performs Money arithmetic in Invoice/Wallet. Per review feedback on PR fullstackhero#1316.

chatgpt-codex-connector · 2026-06-29T05:22:14Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

marcelo-maciel · 2026-06-29T05:24:21Z

Done — trimmed the arithmetic (Add/Subtract/Multiply/Round + the +/-/* operators) and moved its Framework.Tests cases out, to be reintroduced together with the Billing follow-up that actually subtotals/credits/debits. The PR now lands just the value type + currency normalization + adoption on Product.Price and TopupRequest.Amount. Build clean (-warnaserror), unit suites green. Thanks for the review.

chatgpt-codex-connector Bot reviewed Jun 26, 2026

View reviewed changes

marcelo-maciel force-pushed the refactor/shared-money-valueobject branch from be6bea6 to b859450 Compare June 26, 2026 19:58

marcelo-maciel force-pushed the refactor/shared-money-valueobject branch from b859450 to 7864674 Compare June 26, 2026 20:00

iammukeshm requested changes Jun 27, 2026

View reviewed changes

		namespace FSH.Framework.Core.Domain;

		public sealed record Money

		@@ -1,4 +1,5 @@
		using System.Net;
		using FSH.Framework.Core.Domain;

Uh oh!

Conversation

marcelo-maciel commented Jun 26, 2026

What

Why

Changes

Migration

Testing

Follow-up (separate PR, if this is accepted)

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

marcelo-maciel Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

marcelo-maciel Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

marcelo-maciel Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot commented Jun 26, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 26, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 26, 2026

Uh oh!

iammukeshm left a comment

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot commented Jun 29, 2026

Uh oh!

marcelo-maciel commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marcelo-maciel commented Jun 29, 2026 •

edited

Loading