diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 01cf76f79..c75756709 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -80,7 +80,7 @@ Nest owns everything. Every domain is a DI module under `src/nest//` (`c - **Every outbound `fetch`** gets a timeout, a response-size cap and boundary validation: no `as`-casting provider responses, no silent `catch → null`. User-influenced URLs go through the SSRF guard. Never fall back to another user's API key. ESLint requires an init with a `signal` on `fetch`, `safeFetch`, `safeFetchFollow` and `safeFetchAdminConfigured` alike, or a literal `timeoutMs` in the options object of `safeFetch`/`safeFetchFollow`; an init spelled `undefined`, `null` or `void 0` counts as none (`safeFetchLlm` takes `LLM_TIMEOUT_MS` inside). Behind that, the guarded wrappers (`utils/ssrfGuard.ts`) cap every body at `DEFAULT_MAX_RESPONSE_BYTES` (64 MB) and, for a call without a signal, wait `DEFAULT_RESPONSE_TIMEOUT_MS` (30 s) for the headers. Pass a tighter `maxBytes` when you buffer a small answer, `timeoutMs` for a deadline over the whole exchange, and `maxBytes: null` only where the body is streamed on or counted by the caller (`pipeAsset`, the doc-sync transfers, the Ollama pull). - **Fail closed.** Unrecognized flag values mean OFF; sandbox/permission opt-outs refuse in production; never pick a less-safe path based on the *absence* of a file. GET handlers must not write. - **Don't fork canonical paths.** Auth verification is `verifyJwtAndLoadUser` (with the `password_version` gate and the session gate); reuse it. A session token is minted only by `SessionsService.issue` (`nest/sessions/`, reached through `AuthService.generateToken`), which writes the `user_sessions` row its `jti` names; a token signed anywhere else carries no session and cannot be listed or revoked. An action that should sign a user out everywhere calls `SessionsService.revokeAll` inside the same transaction as the write that warrants it. -- **Money**: integer cents; largest-remainder splits (`BudgetService.splitEqualShares` is the reference); FX frozen at entry; explicit flags, never sentinel values; custom splits must reconcile to zero before persisting. +- **Money**: integer cents; largest-remainder splits (`splitEqualShares` in `@trek/shared` (`shared/src/money/`) is the reference, with `toMinor`/`sumMinor` for every sum and comparison); FX frozen at entry; explicit flags, never sentinel values; custom splits must reconcile to zero before persisting. ## SQL dialect diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index 8a1c5b298..d791a3d47 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -8,7 +8,7 @@ "src/nest/atlas/atlas-geo.ts": 1005, "src/nest/atlas/atlas.service.ts": 1182, "src/nest/auth/auth.service.ts": 1143, - "src/nest/budget/budget.service.ts": 1702, + "src/nest/budget/budget.service.ts": 1668, "src/nest/collections/collections.service.ts": 1483, "src/nest/doc-sync/doc-sync.service.ts": 1060, "src/nest/doc-sync/providers/synology-drive.client.ts": 1160, diff --git a/server/src/nest/budget/budget.mcp.ts b/server/src/nest/budget/budget.mcp.ts index f353d01b4..f82f38134 100644 --- a/server/src/nest/budget/budget.mcp.ts +++ b/server/src/nest/budget/budget.mcp.ts @@ -6,7 +6,7 @@ import { } from '../../nest-mcp'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; import { z } from 'zod'; -import { idSchema } from '@trek/shared'; +import { idSchema, sumMinor, toMinor } from '@trek/shared'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import { ADDON_IDS } from '../../addons'; import { noAccess, permissionDenied } from '../../mcp/tools/_shared'; @@ -48,14 +48,6 @@ function parseId(value: string | string[]): number | null { return Number.isInteger(n) && n > 0 ? n : null; } -/** Money is compared in whole cents, never in floats (the budget money rule). */ -function toCents(amount: number): number { - return Math.round(amount * 100); -} - -function sumCents(amounts: number[]): number { - return amounts.reduce((sum, a) => sum + toCents(a), 0); -} function formatCents(cents: number): string { return (cents / 100).toFixed(2); @@ -131,9 +123,9 @@ export class BudgetMcp { // Negative payers count with their sign (#2176) — the write path stores them. if (payers !== undefined) { const roster = await this.tripMembers.rosterUserIds(tripId); - return sumCents(payers.filter(p => p.amount !== 0 && roster.has(p.user_id)).map(p => p.amount)); + return sumMinor(payers.filter(p => p.amount !== 0 && roster.has(p.user_id)).map(p => p.amount)); } - if (total_price !== undefined) return toCents(total_price); + if (total_price !== undefined) return toMinor(total_price); return fallbackCents; } @@ -169,7 +161,7 @@ export class BudgetMcp { return `payers contains user IDs that are not on this trip: ${payerStrangers.join(', ')}. Resolve them with list_trip_members.`; } } - const splitCents = sumCents(members.map(m => m.amount)); + const splitCents = sumMinor(members.map(m => m.amount)); if (splitCents !== totalCents) { return `The split does not add up: the member amounts total ${formatCents(splitCents)} but the expense total is ${formatCents(totalCents)}. Adjust the amounts so they sum to the expense total.`; } @@ -216,7 +208,7 @@ export class BudgetMcp { if (members !== undefined && member_ids !== undefined) return errorResult('Pass either members (uneven split) or member_ids (equal split), not both.'); if (place_id != null && !(await this.placeOnTrip(tripId, place_id))) return errorResult('place_id does not belong to this trip.'); if (members !== undefined) { - const refusal = await this.splitRefusal(tripId, members, await this.settledTotalCents(tripId, payers, total_price, toCents(total_price)), payers); + const refusal = await this.splitRefusal(tripId, members, await this.settledTotalCents(tripId, payers, total_price, toMinor(total_price)), payers); if (refusal) return errorResult(refusal); } // The split participants are the members of an uneven split; the equal-split @@ -296,7 +288,7 @@ export class BudgetMcp { // the stored figure stands in when the call does not restate one. const existing = await this.budget.getBudgetItem(itemId, tripId); if (!existing) return errorResult('Budget item not found.'); - const refusal = await this.splitRefusal(tripId, members, await this.settledTotalCents(tripId, payers, total_price, toCents(existing.total_price)), payers); + const refusal = await this.splitRefusal(tripId, members, await this.settledTotalCents(tripId, payers, total_price, toMinor(existing.total_price)), payers); if (refusal) return errorResult(refusal); } // Freeze-then-write composite: a currency change re-freezes the rate at entry diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index c561279cf..47e54235e 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -1,6 +1,7 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; import type { BudgetFallbackFx, BudgetParticipantFinal, BudgetUnconverted, TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; +import { splitEqualShares, sumMinor, toMinor } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import { avatarUrl } from '../common/avatarUrl'; @@ -97,7 +98,7 @@ export function splitLegacyTicketNote( * (toTripCents), applied to the one arithmetic that had been left in euros. */ function sumMoney(amounts: number[]): number { - return amounts.reduce((a, v) => a + Math.round(v * 100), 0) / 100; + return sumMinor(amounts) / 100; } /** @@ -1043,10 +1044,10 @@ export class BudgetService { // A foreign row nothing can convert counts for nobody, as in the settlement. const convert = tripConverter(item.currency, item.exchange_rate, tripCurrency, rates); if (!convert) continue; - const toTripCents = (amount: number) => Math.round(convert(amount) * 100); + const toTripCents = (amount: number) => toMinor(convert(amount)); // A member with an amount of their own owes exactly that; the rest split the // total evenly, as the query this replaces did. - const equal = this.splitEqualShares(toTripCents(item.total_price || 0), own, item.id); + const equal = splitEqualShares(toTripCents(item.total_price || 0), own, item.id); for (const m of own) { const share = m.amount !== null && m.amount !== undefined ? toTripCents(m.amount) : (equal[m.user_id] || 0); let p = people.get(m.user_id); @@ -1089,7 +1090,7 @@ export class BudgetService { unconverted.push(r.id); continue; } - const cents = Math.round(convert(r.total_price || 0) * 100); + const cents = toMinor(convert(r.total_price || 0)); total += cents; const cat = r.category || ''; byCategory[cat] = (byCategory[cat] || 0) + cents; @@ -1112,41 +1113,6 @@ export class BudgetService { return unbooked ? this.exchangeRates.getRates(trip) : null; } - /** - * Largest-remainder split of an expense across its participants. Takes and - * returns **whole cents**, so the shares add back up to the input exactly — - * the settlement ledger is netted in integer cents (#1382). - * - * The remainder cent rotates with the item id rather than always landing on the - * first member, so across several expenses the rounding evens out instead of - * always favouring the same person. - * - * Floor-based (`totalCents - baseCents * n`, never `%`), so a negative total — - * a refund split across its beneficiaries (#2176) — still yields a remainder - * in [0, n) and shares that sum back to the total exactly. The client mirror - * (CostsPanel.helpers.splitEqualShares) must stay share-for-share identical; - * the parity fixture in budget.service.calc.test.ts pins both sides. - */ - private splitEqualShares(totalCents: number, members: { user_id: number }[], itemId: number): Record { - const n = members.length; - if (n === 0) return {}; - - const baseCents = Math.floor(totalCents / n); - const remainder = totalCents - baseCents * n; - - const shares: Record = {}; - const sortedMembers = [...members].sort((a, b) => a.user_id - b.user_id); - const startIndex = itemId % n; - - for (let i = 0; i < n; i++) { - const member = sortedMembers[i]; - const hasExtraCent = ((i - startIndex + n) % n) < remainder; - shares[member.user_id] = baseCents + (hasExtraCent ? 1 : 0); - } - - return shares; - } - /** * Who owes whom (`balances` + the simplified `flows`), the recorded transfers * (`settlements`), and what the trip ends up costing each participant @@ -1267,7 +1233,7 @@ export class BudgetService { unconvertedCurrencies.add((item.currency || '').toUpperCase()); continue; } - const toTripCents = (amount: number): number => Math.round(convert(amount) * 100); + const toTripCents = (amount: number): number => toMinor(convert(amount)); const members = allMembers.filter(m => m.budget_item_id === item.id); const payers = allPayers.filter(p => p.budget_item_id === item.id); if (members.length === 0) continue; // planning-only entry → doesn't affect balances @@ -1314,7 +1280,7 @@ export class BudgetService { // here since #2225. An item whose payers net to exactly zero still does, and // divides zero, which is what it is worth. const hasCustomSplit = members.some(m => m.amount !== null && m.amount !== undefined); - const equalShares = !hasCustomSplit ? this.splitEqualShares(creditCents, members, item.id) : {}; + const equalShares = !hasCustomSplit ? splitEqualShares(creditCents, members, item.id) : {}; for (const m of members) { const memberShare = hasCustomSplit && m.amount !== null && m.amount !== undefined ? toTripCents(m.amount) diff --git a/server/tests/unit/nest/budget.service.calc.test.ts b/server/tests/unit/nest/budget.service.calc.test.ts index 052537acd..c3633141a 100644 --- a/server/tests/unit/nest/budget.service.calc.test.ts +++ b/server/tests/unit/nest/budget.service.calc.test.ts @@ -538,23 +538,6 @@ describe('calculateSettlement — negative amounts (#2176)', () => { }); }); -// ── Client/server share parity (#2176) ─────────────────────────────────────── -// -// splitEqualShares exists twice: here (netting the settlement in cents) and on -// the client (previewing the split in euros — CostsPanel.helpers.ts). This -// fixture is duplicated verbatim in -// client/src/components/Budget/CostsPanel.helpers.test.ts; if either -// implementation drifts — sign handling included — its copy of the table fails. -const SHARE_PARITY_FIXTURE: { totalCents: number; users: number[]; itemId: number; expected: Record }[] = [ - { totalCents: 10000, users: [1, 2, 3], itemId: 0, expected: { 1: 3334, 2: 3333, 3: 3333 } }, - { totalCents: 10000, users: [1, 2, 3], itemId: 1, expected: { 1: 3333, 2: 3334, 3: 3333 } }, - { totalCents: -10000, users: [1, 2, 3], itemId: 0, expected: { 1: -3333, 2: -3333, 3: -3334 } }, - { totalCents: -10000, users: [1, 2, 3], itemId: 1, expected: { 1: -3334, 2: -3333, 3: -3333 } }, - { totalCents: -101, users: [1, 2], itemId: 0, expected: { 1: -50, 2: -51 } }, - { totalCents: -101, users: [1, 2], itemId: 1, expected: { 1: -51, 2: -50 } }, - { totalCents: -1, users: [1, 2, 3], itemId: 0, expected: { 1: 0, 2: 0, 3: -1 } }, -]; - // ── Final budget per participant ────────────────────────────────────────── describe('calculateSettlement — finalBudgets', () => { @@ -781,26 +764,6 @@ describe('calculateSettlement — finalBudgets', () => { }); }); -describe('splitEqualShares — client parity (#2176)', () => { - // Private on purpose (only the settlement calls it); the parity pin reaches - // through so the fixture exercises the real implementation, not a re-model. - // `budget` isn't built until `beforeAll` runs, so this resolves it lazily at - // call time rather than binding it during collection (still undefined then). - const split = (totalCents: number, members: { user_id: number }[], itemId: number): Record => - (budget as unknown as { - splitEqualShares(totalCents: number, members: { user_id: number }[], itemId: number): Record; - }).splitEqualShares(totalCents, members, itemId); - - it.each(SHARE_PARITY_FIXTURE)( - 'splits $totalCents cents across $users.length members (item $itemId) exactly like the client', - ({ totalCents, users, itemId, expected }) => { - const shares = split(totalCents, users.map(user_id => ({ user_id })), itemId); - expect(shares).toEqual(expected); - expect((Object.values(shares) as number[]).reduce((a, b) => a + b, 0)).toBe(totalCents); - }, - ); -}); - // ── #1382: balances and flows have to tell the same story ──────────────────── // The ledger is netted in whole cents of the trip currency, so a balance is only // ever a whole number of cents and every one of them can be settled. What used to diff --git a/shared/CLAUDE.md b/shared/CLAUDE.md index 56e32ab89..9b537df4a 100644 --- a/shared/CLAUDE.md +++ b/shared/CLAUDE.md @@ -60,3 +60,7 @@ The parity rule governs **existing** routes; it is not a license to mint new deb ## Sanitization (`src/sanitize/sanitize.ts`) `isomorphic-dompurify` with a minimal inline-only allow-list (a rich-text variant already exists there for when rich text ships). `sanitizeInlineHtml` backs the client's HTML translation path; `escapeHtml` is used wherever the client builds markup from user strings (map popups, account screens). This is meant to be the one home for escaping. No lint rule enforces that yet and a handful of client files still carry a local `escapeHtml` — that is debt to fold in, not a pattern to copy. + +## Money (`src/money/money.ts`) + +`toMinor`, `sumMinor`, `splitEqualShares` and `currencyDecimals` are the budget arithmetic both sides must agree on to the cent: amounts are netted in whole hundredths, equal splits hand the leftover hundredths out by rotation from `itemId % n`, and the shares always sum back to the total. The server's settlement uses these directly; `money.spec.ts` holds the share table that used to be duplicated between the server and client tests. The client still carries its own `splitEqualShares` (in euros, `CostsPanel.helpers.ts`) and `currencyDecimals` (`utils/formatters.ts`); folding them onto these is client work, not a second implementation to copy. diff --git a/shared/src/index.ts b/shared/src/index.ts index ec7e56225..b1ffe584b 100644 --- a/shared/src/index.ts +++ b/shared/src/index.ts @@ -108,3 +108,6 @@ export * from './roadtrip/google-import.schema'; export * from './roadtrip/charging.schema'; export * from './vacay/school-holiday-catalog.schema'; + +// Money arithmetic in whole hundredths (sums, equal splits, currency precision). +export * from './money/money'; diff --git a/shared/src/money/money.spec.ts b/shared/src/money/money.spec.ts new file mode 100644 index 000000000..e72d12599 --- /dev/null +++ b/shared/src/money/money.spec.ts @@ -0,0 +1,72 @@ +import { currencyDecimals, splitEqualShares, sumMinor, toMinor } from './money'; + +import { describe, expect, it } from 'vitest'; + +describe('currencyDecimals', () => { + it.each([ + ['EUR', 2], + ['usd', 2], + ['JPY', 0], + ['huf', 0], + ['XPF', 0], + ['KWD', 3], + ['tnd', 3], + ['ZZZ', 2], + ])('%s has %i decimals', (code, decimals) => { + expect(currencyDecimals(code)).toBe(decimals); + }); +}); + +describe('toMinor / sumMinor', () => { + it('rounds a float amount to whole hundredths', () => { + expect(toMinor(81.61)).toBe(8161); + expect(toMinor(0.1 + 0.2)).toBe(30); + expect(toMinor(-30)).toBe(-3000); + }); + + it('sums each amount rounded, so the two halves of 163.21 add back exactly (#1964)', () => { + expect(81.61 + 81.6).not.toBe(163.21); + expect(sumMinor([81.61, 81.6])).toBe(16321); + expect(sumMinor([])).toBe(0); + }); +}); + +// The share table the server's settlement and the client's preview were both +// pinned to (#2176). It is the contract of the shared function now. +const SHARE_PARITY_FIXTURE: { + totalCents: number; + users: number[]; + itemId: number; + expected: Record; +}[] = [ + { totalCents: 10000, users: [1, 2, 3], itemId: 0, expected: { 1: 3334, 2: 3333, 3: 3333 } }, + { totalCents: 10000, users: [1, 2, 3], itemId: 1, expected: { 1: 3333, 2: 3334, 3: 3333 } }, + { totalCents: -10000, users: [1, 2, 3], itemId: 0, expected: { 1: -3333, 2: -3333, 3: -3334 } }, + { totalCents: -10000, users: [1, 2, 3], itemId: 1, expected: { 1: -3334, 2: -3333, 3: -3333 } }, + { totalCents: -101, users: [1, 2], itemId: 0, expected: { 1: -50, 2: -51 } }, + { totalCents: -101, users: [1, 2], itemId: 1, expected: { 1: -51, 2: -50 } }, + { totalCents: -1, users: [1, 2, 3], itemId: 0, expected: { 1: 0, 2: 0, 3: -1 } }, +]; + +describe('splitEqualShares', () => { + it.each(SHARE_PARITY_FIXTURE)( + 'splits $totalCents across $users.length members (item $itemId)', + ({ totalCents, users, itemId, expected }) => { + const shares = splitEqualShares( + totalCents, + users.map((user_id) => ({ user_id })), + itemId, + ); + expect(shares).toEqual(expected); + expect(Object.values(shares).reduce((a, b) => a + b, 0)).toBe(totalCents); + }, + ); + + it('orders members by user id whatever order they arrive in', () => { + expect(splitEqualShares(100, [{ user_id: 9 }, { user_id: 2 }, { user_id: 5 }], 0)).toEqual({ 2: 34, 5: 33, 9: 33 }); + }); + + it('answers an empty map for nobody', () => { + expect(splitEqualShares(500, [], 3)).toEqual({}); + }); +}); diff --git a/shared/src/money/money.ts b/shared/src/money/money.ts new file mode 100644 index 000000000..9f4e652d4 --- /dev/null +++ b/shared/src/money/money.ts @@ -0,0 +1,83 @@ +/** + * Money arithmetic both sides need to agree on to the cent. + * + * The ledger is netted in whole hundredths of the currency ("minor units" here + * always means hundredths, whatever the currency's display precision: a yen + * amount is held as yen × 100 like every other one). Floats are only the + * storage format; every sum, split and comparison happens on these integers. + */ + +/** + * Currencies whose display precision is not two decimals. HUF is kept + * zero-decimal by app convention even though ISO lists two. + */ +const ZERO_DECIMAL_CURRENCIES: ReadonlySet = new Set([ + 'JPY', + 'KRW', + 'VND', + 'CLP', + 'ISK', + 'HUF', + 'BIF', + 'DJF', + 'GNF', + 'KMF', + 'PYG', + 'RWF', + 'UGX', + 'VUV', + 'XAF', + 'XOF', + 'XPF', +]); +const THREE_DECIMAL_CURRENCIES: ReadonlySet = new Set(['BHD', 'IQD', 'JOD', 'KWD', 'LYD', 'OMR', 'TND']); + +/** How many decimals a currency is shown with (0, 2 or 3). Case-insensitive; unknown codes get 2. */ +export function currencyDecimals(currency: string): number { + const cur = currency.toUpperCase(); + if (ZERO_DECIMAL_CURRENCIES.has(cur)) return 0; + if (THREE_DECIMAL_CURRENCIES.has(cur)) return 3; + return 2; +} + +/** An amount in whole hundredths, rounded half away from zero the way `Math.round` does for positives. */ +export function toMinor(amount: number): number { + return Math.round(amount * 100); +} + +/** The sum of several amounts in whole hundredths: each one is rounded first, so float noise never adds up. */ +export function sumMinor(amounts: readonly number[]): number { + return amounts.reduce((sum, amount) => sum + toMinor(amount), 0); +} + +/** + * Split `totalMinor` equally across `members`, in whole hundredths, keyed by + * user id. + * + * Largest remainder by rotation: the leftover hundredths go to consecutive + * members starting at `itemId % n` (members ordered by user id), so across + * several expenses the rounding evens out instead of always favouring the same + * person. The remainder is `total - base * n` with a floored base, never `%`, + * so a negative total (a refund, #2176) still yields a remainder in [0, n) and + * the shares always sum back to the total exactly. + */ +export function splitEqualShares( + totalMinor: number, + members: readonly { user_id: number }[], + itemId: number, +): Record { + const n = members.length; + if (n === 0) return {}; + + const base = Math.floor(totalMinor / n); + const remainder = totalMinor - base * n; + const sorted = [...members].sort((a, b) => a.user_id - b.user_id); + const startIndex = itemId % n; + + const shares: Record = {}; + sorted.forEach((member, i) => { + const hasExtra = (i - startIndex + n) % n < remainder; + shares[member.user_id] = base + (hasExtra ? 1 : 0); + }); + return shares; +}