From 1f615bab4fa140a36a214e25a167035ee3dae5c4 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Wed, 5 Aug 2026 15:27:13 -0400 Subject: [PATCH] fix incorrect balance push notification --- apps/econ/src/econ.app.ts | 57 ++++++++++++---------- apps/econ/src/test/integration/api.test.ts | 48 ++++++++++++++++++ apps/econ/vitest.config.ts | 18 ++++++- 3 files changed, 97 insertions(+), 26 deletions(-) diff --git a/apps/econ/src/econ.app.ts b/apps/econ/src/econ.app.ts index 3645311..4bbf5ab 100644 --- a/apps/econ/src/econ.app.ts +++ b/apps/econ/src/econ.app.ts @@ -203,23 +203,30 @@ async function pushConsumableAdded( * Push a StorefrontBalanceUpdate to a player after their balance changes, mirroring the * reference's * `HubSendToPlayer(accountID, NotifFrame(StorefrontBalanceUpdate, {Balance, CurrencyType, BalanceType}))`. - * The client applies it to the shown balance so a purchase debit reflects immediately, - * without waiting for a `GET /balance` re-fetch. `Balance` is the resulting total in that - * currency (not the delta), `BalanceType` is -2 (account-wide, all platforms). Best-effort: - * a hub failure is logged and swallowed, since the balance change has already committed. + * The client applies it to the shown balance so a purchase reflects immediately, without + * waiting for a `GET /balance` re-fetch. + * + * `Balance` is the CHANGE — negative for a debit, positive for a payout — not the + * resulting total. The client ADDS what it receives to the balance it is already showing, + * so sending the total made a 10,000-token player who earned 250 read 20,250: their own + * balance plus the new total. That also makes this frame non-idempotent, so push exactly + * once per change and never re-send it as a "refresh". + * + * `BalanceType` is -2 (account-wide, all platforms). Best-effort: a hub failure is logged + * and swallowed, since the balance change has already committed. */ async function pushBalanceUpdate( c: Context, accountId: number, currencyType: number, - balance: number + change: number ): Promise { try { await c.env.RECFLARE_NOTIFICATIONS_HUB.getByName(HUB_INSTANCE).notifyPlayer( accountId, NotificationType.StorefrontBalanceUpdate, { - Balance: balance, + Balance: change, CurrencyType: currencyType, BalanceType: ALL_PLATFORMS, } @@ -1012,7 +1019,8 @@ const app = new Hono({ strict: false }) 'still matches, debits the buyer atomically, grants the item (into the inventory or', 'consumable table), and returns a gift box. A `Gift` block routes the item to another', 'player, but the caller always pays. `Balance` in the response is the CHANGE (negated', - 'price), not the new total. Pushes a StorefrontBalanceUpdate socket notification.', + 'price), not the new total. Pushes a StorefrontBalanceUpdate socket frame carrying the', + 'same change, which the client ADDS to the balance it is showing.', ].join(' '), security: AUTHED, requestBody: jsonBody(BuyItemRequest, 'The item, currency, price, and optional Gift'), @@ -1133,11 +1141,11 @@ const app = new Hono({ strict: false }) ) ) - // Push the buyer's new (reduced) balance over the socket so their client updates the - // shown total immediately — the buyer (`id`) is who was debited, in the currency they - // spent. Best-effort; the HTTP response still carries the change either way. - const newBalance = await getBalance(c.env.DB, id, currencyType as number, startingTokens) - await pushBalanceUpdate(c, id, currencyType as number, newBalance) + // Push the debit over the socket so the buyer's client updates the shown total + // immediately — the buyer (`id`) is who was charged, in the currency they spent. The + // frame carries the CHANGE, so a purchase is negative. Best-effort; the HTTP response + // carries the same change either way. + await pushBalanceUpdate(c, id, currencyType as number, -price.Price) // The response mirrors a captured real buyItem: `Balance` is the change applied (the // negated price), not the resulting balance (the client reads its new total from @@ -1206,7 +1214,9 @@ const app = new Hono({ strict: false }) 'its stored `Price`, debits the buyer and pays the creator that price in', 'RecCenterTokens (a free invention moves nothing), records ownership in', '`inventory_invention`, and returns the invention alongside the buyer’s resulting', - 'balance. Both players get a StorefrontBalanceUpdate push when tokens moved.', + 'balance. When tokens moved, both players get a StorefrontBalanceUpdate push carrying', + 'their CHANGE (the buyer’s negative, the creator’s positive), which the client adds to', + 'the balance it is showing — unlike this response body, which replaces it.', 'A GET because that is how the client sends it.', ].join(' '), security: AUTHED, @@ -1294,30 +1304,27 @@ const app = new Hono({ strict: false }) // creator who had never touched their balance would otherwise have the row created // here and lose their starting tokens forever. await ensureStartingBalances(c.env.DB, invention.CreatorPlayerId, startingTokens) - const creatorBalance = await creditCurrency( + await creditCurrency( c.env.DB, invention.CreatorPlayerId, CurrencyType.RecCenterTokens, price, startingTokens ) - // The creator is a different, probably-online player: push their new total so a - // sale lands on their shown balance without a re-fetch. Best-effort, as everywhere. - await pushBalanceUpdate( - c, - invention.CreatorPlayerId, - CurrencyType.RecCenterTokens, - creatorBalance - ) + // The creator is a different, probably-online player: push the payout so a sale + // lands on their shown balance without a re-fetch. Positive, because the frame + // carries the change. Best-effort, as everywhere. + await pushBalanceUpdate(c, invention.CreatorPlayerId, CurrencyType.RecCenterTokens, price) } // Unlike buyItem — whose `Balance` is the change applied — the reference server // answers this one with the RESULTING total (a first read seeds the buyer's starting - // grant, as everywhere else). + // grant, as everywhere else). The socket frame below is the other way round: the HTTP + // body REPLACES the shown balance, the push ADDS to it. const balance = await getBalance(c.env.DB, id, CurrencyType.RecCenterTokens, startingTokens) - // A free invention moved nothing, so there is no balance to push for it. + // A free invention moved nothing, so there is no change to push for it. if (price > 0) { - await pushBalanceUpdate(c, id, CurrencyType.RecCenterTokens, balance) + await pushBalanceUpdate(c, id, CurrencyType.RecCenterTokens, -price) } return c.json({ BalanceUpdateResponse: { diff --git a/apps/econ/src/test/integration/api.test.ts b/apps/econ/src/test/integration/api.test.ts index 2d6e78d..71fc0c0 100644 --- a/apps/econ/src/test/integration/api.test.ts +++ b/apps/econ/src/test/integration/api.test.ts @@ -701,6 +701,7 @@ describe('econ endpoints', () => { test('POST /api/storefronts/v2/buyItem debits, grants the item, and hands back a gift box', async () => { // Account 20: fresh, so its first balance touch grants the 10000 default. + await drainFrames() const res = await exports.default.fetch(`${ORIGIN}/api/storefronts/v2/buyItem`, { method: 'POST', headers: { ...(await bearer('20')), 'Content-Type': 'application/json' }, @@ -728,6 +729,16 @@ describe('econ endpoints', () => { expect(gift.AvatarItemDesc).not.toBe('') expect(gift.Id).toBeGreaterThan(0) + // The socket frame carries the same change the response does — the client adds it to + // the balance it is showing, so the resulting total here would double-count the 9550. + expect(await drainFrames()).toEqual([ + { + accountId: 20, + notificationType: STOREFRONT_BALANCE_UPDATE, + payload: { Balance: -450, CurrencyType: 2, BalanceType: -2 }, + }, + ]) + // The balance endpoint reflects the debit (this is the resulting total, 10000 - 450). const bal = await exports.default.fetch(`${ORIGIN}/api/storefronts/v4/balance/2`, { headers: await bearer('20'), @@ -992,6 +1003,26 @@ describe('econ endpoints', () => { expect(list.every((i) => i.FriendlyName !== 'Bowtie (White)')).toBe(true) }) + /** + * The StorefrontBalanceUpdate (and other) frames the worker has pushed since the last + * drain, read back off the stub hub in vitest.config.ts. Notification sends are + * best-effort — the worker logs and swallows a hub failure — so this is the only way a + * test sees what was actually pushed. + */ + const drainFrames = async (): Promise< + Array<{ accountId: number; notificationType: number; payload: Record }> + > => + ( + env.RECFLARE_NOTIFICATIONS_HUB.getByName('global') as unknown as { + drainFrames(): Promise< + Array<{ accountId: number; notificationType: number; payload: Record }> + > + } + ).drainFrames() + + /** `NotificationType.StorefrontBalanceUpdate` in the notify worker's enum. */ + const STOREFRONT_BALANCE_UPDATE = 61 + // buyInvention is a GET with query params — that is how the client sends it. const buyInvention = async (sub: string, inventionId: number, requestedPrice = 0) => exports.default.fetch( @@ -1042,6 +1073,7 @@ describe('econ endpoints', () => { test('GET /api/storefronts/v2/buyInvention pays the creator the buyer’s tokens', async () => { // Invention 9 costs 250 and was made by account 999. Buying it moves 250 tokens from // the buyer to that creator — no house cut, so the two sides are equal and opposite. + await drainFrames() const res = await buyInvention('51', 9, 250) expect(res.status).toBe(200) const body = (await res.json()) as { BalanceUpdateResponse: { Balance: number } } @@ -1056,6 +1088,22 @@ describe('econ endpoints', () => { await getBalance(env.DB, 999, CurrencyType.RecCenterTokens, DEFAULT_STARTING_TOKENS) ).toBe(DEFAULT_STARTING_TOKENS + 250) expect(await getOwnedInventionIds(env.DB, 51)).toEqual([9]) + + // Both sides get a socket frame carrying their CHANGE, not their new total: the client + // ADDS what it receives to the balance it is showing, so a total would have the creator + // reading their own balance plus the payout. Equal and opposite, like the ledger. + expect(await drainFrames()).toEqual([ + { + accountId: 999, + notificationType: STOREFRONT_BALANCE_UPDATE, + payload: { Balance: 250, CurrencyType: CurrencyType.RecCenterTokens, BalanceType: -2 }, + }, + { + accountId: 51, + notificationType: STOREFRONT_BALANCE_UPDATE, + payload: { Balance: -250, CurrencyType: CurrencyType.RecCenterTokens, BalanceType: -2 }, + }, + ]) }) test('GET /api/storefronts/v2/buyInvention rejects a stale price and an unaffordable one', async () => { diff --git a/apps/econ/vitest.config.ts b/apps/econ/vitest.config.ts index 71c1001..0cd3b70 100644 --- a/apps/econ/vitest.config.ts +++ b/apps/econ/vitest.config.ts @@ -14,6 +14,12 @@ export default defineConfig({ // isolated test, so provide a minimal stub exposing the same NotificationsHub // RPC surface — enough for the runtime to start and for notification sends to // no-op. + // + // The stub RECORDS what it was sent (`drainFrames`) rather than discarding it. + // Pushes are best-effort and swallow their own errors, so a frame carrying the + // wrong payload is otherwise invisible here — which is exactly how + // StorefrontBalanceUpdate shipped with the resulting total in a field the + // client adds to what it is already showing. workers: [ { name: 'notify', @@ -24,8 +30,18 @@ export default defineConfig({ script: ` import { DurableObject } from 'cloudflare:workers' export class NotificationsHub extends DurableObject { - async notifyPlayer() { return { delivered: 0, queued: true } } + frames = [] + async notifyPlayer(accountId, notificationType, payload) { + this.frames.push({ accountId, notificationType, payload }) + return { delivered: 0, queued: true } + } async broadcast() { return { delivered: 0 } } + /** Everything pushed since the last call, then forget it. */ + async drainFrames() { + const drained = this.frames + this.frames = [] + return drained + } } export default { fetch() { return new Response('ok') } } `,