Uh oh!
There was an error while loading. Please reload this page.
feat: Add max negative caps and enforce ledger bounds - #618
Conversation
WalkthroughThis pull request introduces Changes
Sequence DiagramsequenceDiagram
actor User as Administrator
participant UI as currency-detail.tsx
participant API as CurrencyController
participant Service as CurrencyService
participant Ledger as LedgerService
participant DB as Database
User->>UI: Set max negative balance value
UI->>API: PATCH /api/currencies/:id/max-negative {value}
API->>Service: updateMaxNegativeBalance(id, value, admin)
Note over Service: Validate:<br/>- Currency exists<br/>- Admin permission<br/>- allowNegative=true<br/>- Value is null or ≤0
Service->>DB: Update currency.maxNegativeBalance
DB-->>Service: Updated Currency
Service-->>API: Currency entity
API-->>UI: {id, maxNegativeBalance, ...}
UI->>UI: Invalidate cache<br/>(currency, accountDetails)
Note over User,Ledger: Later: User attempts transfer
User->>API: POST /transfer {amount, ...}
API->>Ledger: transfer(currencyId, amount, ...)
Note over Ledger: Compute balance change<br/>if (maxNegativeBalance set<br/>AND balance+amount < limit)<br/>throw error
alt Balance within limit
Ledger->>DB: addLedgerEntry(...)
DB-->>Ledger: Ledger entry
Ledger-->>API: Success
else Balance exceeds limit
Ledger-->>API: Insufficient funds error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/eCurrency-api/src/services/LedgerService.ts (1)
96-124: Wrap the transfer method in a database transaction to prevent concurrent balance validation violations.There is a time-of-check to time-of-use (TOCTOU) gap between fetching
currentBalance(line 96) and persisting ledger entries (lines 112-139). Concurrent transfers from the same account could both pass validation before either commits, potentially violating themaxNegativeBalanceconstraint.Use
AppDataSource.transaction()with proper isolation level (similar to the pattern in GroupService.ts), or implement row-level locking withFOR UPDATEto ensure balance validation and ledger creation are atomic.
🧹 Nitpick comments (4)
platforms/eCurrency-api/src/database/entities/Currency.ts (1)
39-40: TypeORM decimal columns return strings at runtime.TypeORM's
decimaltype returns values as strings, not JavaScriptnumbers. The TypeScript typenumber | nullis misleading and may cause subtle bugs when performing arithmetic or comparisons without explicit conversion.Consider adjusting the type or using a transformer:
- @Column("decimal", { precision: 18, scale: 2, nullable: true })- maxNegativeBalance!: number | null;+ @Column("decimal", { precision: 18, scale: 2, nullable: true, transformer: {+ to: (value: number | null) => value,+ from: (value: string | null) => value === null ? null : parseFloat(value)+ }})+ maxNegativeBalance!: number | null;Alternatively, update the type to
string | nulland convert explicitly at usage sites.platforms/eCurrency-api/src/controllers/CurrencyController.ts (1)
162-166: Consider adding semantic validation formaxNegativeBalance.The current validation only checks that the value is a valid number (or null). Given the semantic meaning (a floor for how negative a balance can go), consider validating that non-null values are
<= 0:// Allow null to clear; otherwise must be a number const parsedValue = value === null || value === undefined ? null : Number(value); if (parsedValue !== null && Number.isNaN(parsedValue)) { return res.status(400).json({ error: "Invalid value for maxNegativeBalance" }); } + if (parsedValue !== null && parsedValue > 0) {+ return res.status(400).json({ error: "maxNegativeBalance must be zero or negative" });+ }platforms/eCurrency-api/src/services/CurrencyService.ts (1)
20-47: AligncreateCurrencymaxNegativeBalance invariants with update path
createCurrencynow accepts and persistsmaxNegativeBalancebut does not enforce the same constraints you added inupdateMaxNegativeBalance(must only be set whenallowNegativeis true, must benullor<= 0, notNaN). That means callers can create a currency in a state that the update path would later reject (e.g.allowNegative = falsewith a non‑null cap, or a positive cap).To keep invariants consistent and avoid surprising states, consider normalizing and validating here as well, e.g.:
// Generate eName (UUID with @ prefix) const ename = `@${uuidv4()}`; + const normalizedMaxNegativeBalance =+ allowNegative && maxNegativeBalance !== null ? maxNegativeBalance : null;++ if (normalizedMaxNegativeBalance !== null) {+ if (Number.isNaN(normalizedMaxNegativeBalance)) {+ throw new Error("Invalid max negative balance value");+ }+ if (normalizedMaxNegativeBalance > 0) {+ throw new Error("Max negative balance must be zero or negative");+ }+ }+ const currency = this.currencyRepository.create({ name, description, ename, groupId, createdBy, allowNegative, - maxNegativeBalance,+ maxNegativeBalance: normalizedMaxNegativeBalance, });platforms/eCurrency/client/src/pages/currency-detail.tsx (1)
292-301: Optional: Format maxNegativeBalance consistently with other monetary valuesHere you display
maxNegativeBalancewith a baretoLocaleString(), while balance and total supply are formatted with two decimal places. For consistency and readability (especially since the DB column is decimal with scale), consider aligning the formatting:- {currency.allowNegative- ? (currency.maxNegativeBalance !== null && currency.maxNegativeBalance !== undefined- ? Number(currency.maxNegativeBalance).toLocaleString()- : "No cap")- : "Not applicable"}+ {currency.allowNegative+ ? (currency.maxNegativeBalance !== null && currency.maxNegativeBalance !== undefined+ ? Number(currency.maxNegativeBalance).toLocaleString(undefined, {+ minimumFractionDigits: 2,+ maximumFractionDigits: 2,+ })+ : "No cap")+ : "Not applicable"}Not required for correctness, but it keeps the UI consistent with the way balances and total supply are shown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
platforms/eCurrency-api/src/controllers/CurrencyController.ts(7 hunks)platforms/eCurrency-api/src/database/entities/Currency.ts(1 hunks)platforms/eCurrency-api/src/database/migrations/1765784749012-migration.ts(1 hunks)platforms/eCurrency-api/src/index.ts(1 hunks)platforms/eCurrency-api/src/services/CurrencyService.ts(3 hunks)platforms/eCurrency-api/src/services/LedgerService.ts(3 hunks)platforms/eCurrency/client/src/pages/currency-detail.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
platforms/eCurrency-api/src/database/migrations/1765784749012-migration.ts (1)
platforms/eCurrency-api/src/database/migrations/1765039149572-migration.ts (1)
Migration1765039149572(3-14)
platforms/eCurrency-api/src/index.ts (1)
platforms/eCurrency-api/src/middleware/auth.ts (1)
authGuard(36-41)
platforms/eCurrency/client/src/pages/currency-detail.tsx (1)
platforms/eCurrency/client/src/lib/apiClient.ts (1)
apiClient(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
platforms/eCurrency-api/src/database/migrations/1765784749012-migration.ts (1)
1-14: LGTM!The migration correctly adds and removes the
maxNegativeBalancecolumn with the appropriate type (numeric(18,2)), matching the entity definition and following the established migration pattern in the codebase.platforms/eCurrency-api/src/services/LedgerService.ts (2)
43-49: LGTM!The
existingBalanceparameter is a sensible optimization that avoids a redundant database query when the caller has already fetched the balance for validation purposes.
103-108: Consider validatingmaxNegativeBalanceis non-positive.The current logic assumes
maxNegativeBalancerepresents a floor (e.g.,-100). If a positive value is accidentally set, the conditionnewBalance < maxNegativeBalancewould never trigger for typical debits leading to negative balances. Consider adding validation in the controller/service to ensuremaxNegativeBalance <= 0when set.platforms/eCurrency-api/src/index.ts (1)
115-115: LGTM!The new PATCH endpoint follows the existing route patterns, uses appropriate HTTP semantics for a partial update, and is correctly protected with
authGuard.platforms/eCurrency-api/src/controllers/CurrencyController.ts (2)
18-18: LGTM!The
maxNegativeBalancefield is correctly extracted from the request body, passed to the service, and included in the response. The implementation follows the existing patterns for other currency fields.Also applies to: 29-29, 40-40
174-185: LGTM!The response structure is consistent with other currency endpoints, including all relevant fields.
platforms/eCurrency-api/src/services/CurrencyService.ts (1)
87-117: Max negative balance update flow and validation look solidThe new
updateMaxNegativeBalancemethod correctly:
- Verifies the currency exists and the requester is a group admin.
- Disallows setting a cap when
allowNegativeis false.- Ensures the value is either
nullor a non‑NaN number that is<= 0.This keeps the persisted state consistent with the business rules around negative balances.
platforms/eCurrency/client/src/pages/currency-detail.tsx (1)
1-1: State and effect wiring for max negative balance looks consistentThe added React Query client usage,
MAX_NEGATIVE_SLIDERconstant, andmaxNegative*state plus theuseEffectthat derives the absolute input fromcurrency.maxNegativeBalanceform a coherent flow and ensure the UI is initialized from the latest server value when the currency data loads or is refetched. No functional issues spotted here.Also applies to: 18-18, 25-28, 56-64
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Description of change
Issue Number
closes#553
closes#554
closes#555
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.