From c0b156fd44f8241d5e77e60e4c05fd49abd43142 Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 13:37:21 -0400 Subject: [PATCH 01/36] fix(backend): decode the stored battle snapshot through one codec --- backend/src/features/battle/ledger/index.ts | 7 + .../features/battle/ledger/snapshot.codec.ts | 102 +++++++ .../features/battle/worker/compute.worker.ts | 66 +---- .../src/features/battle/worker/sign.worker.ts | 60 +---- .../features/battle/worker/verify.worker.ts | 29 +- .../battle/ledger/accept.service.test.ts | 40 ++- .../battle/worker/compute.worker.test.ts | 8 +- .../battle/worker/sign.worker.test.ts | 239 ++++++++++++----- .../battle/worker/verify.worker.test.ts | 8 +- docs/plan-battle-inventory-hardening.md | 248 ++++++++++++++++++ 10 files changed, 616 insertions(+), 191 deletions(-) create mode 100644 backend/src/features/battle/ledger/snapshot.codec.ts create mode 100644 docs/plan-battle-inventory-hardening.md diff --git a/backend/src/features/battle/ledger/index.ts b/backend/src/features/battle/ledger/index.ts index 5dc27798..abc0a79d 100644 --- a/backend/src/features/battle/ledger/index.ts +++ b/backend/src/features/battle/ledger/index.ts @@ -115,3 +115,10 @@ export { type TransitionResult, } from './transitions'; export { buildPetSnapshot } from './snapshot.builder'; +export { + decodeStoredPet, + decodeStoredSnapshot, + type StoredBattleSnapshot, + type StoredEquipEntry, + type StoredPetSnapshot, +} from './snapshot.codec'; diff --git a/backend/src/features/battle/ledger/snapshot.codec.ts b/backend/src/features/battle/ledger/snapshot.codec.ts new file mode 100644 index 00000000..2e9720cd --- /dev/null +++ b/backend/src/features/battle/ledger/snapshot.codec.ts @@ -0,0 +1,102 @@ +import type { BattleSnapshot, EquipEntry, PetSnapshot } from '@cryptopets/protocol'; + +/** + * Reads a frozen snapshot back out of the ledger row it was stored in. + * + * The snapshot is persisted with `JSON.stringify`, which has no bigint, so `petId`, `dna`, + * `lastOpponentId`, `sourceVersion` and an equipped item's `itemType` all come back as + * decimal strings. The protocol types require real bigints, so nothing may hash, validate + * or simulate a stored snapshot without going through here first. + * + * One decoder, used by every worker that reads the column. There were three, written + * separately, and that is precisely how one of them came to be missing `schemaVersion` and + * `equipment` after roadmap §4 added them: the signing worker rebuilt every snapshot at + * layout version 1, its hash stopped matching the one acceptance committed, and the seed + * check inside `assertBattleReceipt` refused every receipt. Adding a field to `PetSnapshot` + * must be a change in one place, or the next field lands the same way. + * + * `schemaVersion` is carried verbatim and never defaulted. A row written before that field + * existed genuinely is a version 1 snapshot with a receipt already signed over it, so + * leaving it absent is what lets `assertBattleSnapshot` read it as 1; substituting this + * build's current version would re-encode it under a layout it was never hashed under. + */ + +/** One equipped item as stored: JSON, so the item type arrives as a decimal string. */ +export interface StoredEquipEntry { + slot: number; + itemType: string | bigint; + hp: number; + atk: number; + def: number; + int: number; + mdef: number; +} + +/** One pet as stored, with every bigint field widened to accept its decimal-string form. */ +export interface StoredPetSnapshot { + petId: string | bigint; + owner: string; + dna: string | bigint; + rarity: number; + level: number; + skill: number; + xp: number; + lastOpponentId: string | bigint; + streak: number; + readyAt: number; + sourceVersion: string | bigint; + equipment?: StoredEquipEntry[]; +} + +/** A battle snapshot as stored in `battle_ledger.snapshot`. */ +export interface StoredBattleSnapshot { + domain: BattleSnapshot['domain']; + attacker: StoredPetSnapshot; + defender: StoredPetSnapshot; + takenAt: number; + schemaVersion?: number; +} + +/** Decodes one stored pet. */ +export function decodeStoredPet(pet: StoredPetSnapshot): PetSnapshot { + return { + petId: BigInt(pet.petId), + owner: pet.owner, + dna: BigInt(pet.dna), + rarity: pet.rarity, + level: pet.level, + skill: pet.skill, + xp: pet.xp, + lastOpponentId: BigInt(pet.lastOpponentId), + streak: pet.streak, + readyAt: pet.readyAt, + sourceVersion: BigInt(pet.sourceVersion), + // Omitted rather than empty when the pet wore nothing, matching what + // `assertPetSnapshot` normalizes to and what `snapshot.builder` wrote. + ...(pet.equipment && pet.equipment.length > 0 && { equipment: pet.equipment.map(decodeStoredEquipEntry) }), + }; +} + +/** Decodes a whole stored snapshot, ready to hash, validate or simulate. */ +export function decodeStoredSnapshot(stored: unknown): BattleSnapshot { + const snapshot = stored as StoredBattleSnapshot; + return { + domain: snapshot.domain, + attacker: decodeStoredPet(snapshot.attacker), + defender: decodeStoredPet(snapshot.defender), + takenAt: snapshot.takenAt, + ...(snapshot.schemaVersion !== undefined && { schemaVersion: snapshot.schemaVersion }), + }; +} + +function decodeStoredEquipEntry(entry: StoredEquipEntry): EquipEntry { + return { + slot: entry.slot, + itemType: BigInt(entry.itemType), + hp: entry.hp, + atk: entry.atk, + def: entry.def, + int: entry.int, + mdef: entry.mdef, + }; +} diff --git a/backend/src/features/battle/worker/compute.worker.ts b/backend/src/features/battle/worker/compute.worker.ts index d65cbe06..6c97f5c4 100644 --- a/backend/src/features/battle/worker/compute.worker.ts +++ b/backend/src/features/battle/worker/compute.worker.ts @@ -1,5 +1,4 @@ import { - type BattleSnapshot, computeProgression, bonusFromEquipment, hashCombatLog, @@ -11,7 +10,13 @@ import { BattleState } from '@generated/prisma/enums'; import type { Prisma } from '@generated/prisma/client'; import { prisma } from '@config/prisma'; -import { applyTransition, type ClaimedMessage, completeOutbox, OUTBOX_TOPICS } from '@features/battle/ledger'; +import { + applyTransition, + type ClaimedMessage, + completeOutbox, + decodeStoredSnapshot, + OUTBOX_TOPICS, +} from '@features/battle/ledger'; import { notifyBattleRoomIfPresent } from '@ws/battleRoomSocket'; /** @@ -49,9 +54,8 @@ export async function processComputeMessage(message: ClaimedMessage, nowSeconds: } const ruleset = loadRulesetBundle(JSON.stringify(rulesetRow.bundle), battle.rulesetHash as Hex); - const snapshot = battle.snapshot as unknown as BattleSnapshot; - const attacker = deserializePet(snapshot.attacker); - const defender = deserializePet(snapshot.defender); + const snapshot = decodeStoredSnapshot(battle.snapshot); + const { attacker, defender } = snapshot; // Equipment totals come from the frozen snapshot, not from the catalog: the fight has // to use the modifiers that were written down at acceptance, so unequipping since then @@ -71,11 +75,7 @@ export async function processComputeMessage(message: ClaimedMessage, nowSeconds: bonusFromEquipment(defender.equipment), ); - const progression = computeProgression( - { ...snapshot, attacker, defender }, - outcome.result.firstWins, - { maxLevel: ruleset.maxLevel }, - ); + const progression = computeProgression(snapshot, outcome.result.firstWins, { maxLevel: ruleset.maxLevel }); const combatLogHash = hashCombatLog(outcome); const patch: Prisma.BattleLedgerUncheckedUpdateInput = { @@ -98,52 +98,6 @@ export async function processComputeMessage(message: ClaimedMessage, nowSeconds: await completeOutbox(message.id, new Date(nowSeconds * 1000)); } -/** As stored: JSON, so the item type arrives as a decimal string. */ -export type SnapshotEquipment = { - slot: number; - itemType: string | bigint; - hp: number; - atk: number; - def: number; - int: number; - mdef: number; -}[]; - -/** The snapshot is stored as JSON, where bigint fields round-trip as decimal strings. */ -function deserializePet(pet: { - petId: string | bigint; - owner: string; - dna: string | bigint; - rarity: number; - level: number; - skill: number; - xp: number; - lastOpponentId: string | bigint; - streak: number; - readyAt: number; - sourceVersion: string | bigint; - equipment?: SnapshotEquipment; -}) { - return { - petId: BigInt(pet.petId), - owner: pet.owner, - dna: BigInt(pet.dna), - rarity: pet.rarity, - level: pet.level, - skill: pet.skill, - xp: pet.xp, - lastOpponentId: BigInt(pet.lastOpponentId), - streak: pet.streak, - readyAt: pet.readyAt, - sourceVersion: BigInt(pet.sourceVersion), - // Widened back to bigint: JSON storage round-trips the item type as a decimal - // string, and the protocol's validator wants the number it was written as. - ...(pet.equipment && { - equipment: pet.equipment.map((entry) => ({ ...entry, itemType: BigInt(entry.itemType) })), - }), - }; -} - function serializeBigints(value: T): Prisma.InputJsonValue { return JSON.parse(JSON.stringify(value, (_key, v) => (typeof v === 'bigint' ? v.toString() : v))); } diff --git a/backend/src/features/battle/worker/sign.worker.ts b/backend/src/features/battle/worker/sign.worker.ts index ea0adf9c..067cb3e9 100644 --- a/backend/src/features/battle/worker/sign.worker.ts +++ b/backend/src/features/battle/worker/sign.worker.ts @@ -11,7 +11,13 @@ import type { Prisma } from '@generated/prisma/client'; import { env } from '@config/env'; import { prisma } from '@config/prisma'; -import { applyTransition, type ClaimedMessage, completeOutbox, OUTBOX_TOPICS } from '@features/battle/ledger'; +import { + applyTransition, + type ClaimedMessage, + completeOutbox, + decodeStoredSnapshot, + OUTBOX_TOPICS, +} from '@features/battle/ledger'; import { activeSigningKey, type EngineAttestation, sign, SignerRefusedError } from '@features/battle/signer'; import { recordBattleDrops } from '@features/inventory'; import { recordBattleFromReceipt } from '@repositories/history.repository'; @@ -57,21 +63,12 @@ export async function processSignMessage(message: ClaimedMessage, nowSeconds: nu throw new Error(`battle ${battle.battleId} is verified but is missing a field sign needs`); } - // Stored as JSON, where bigint fields (petId, dna, lastOpponentId, sourceVersion) - // round-trip as decimal strings — the protocol types require real bigints, so - // this must be deserialized before anything here hashes or validates it. - const storedSnapshot = battle.snapshot as unknown as { - domain: BattleSnapshot['domain']; - attacker: StoredPet; - defender: StoredPet; - takenAt: number; - }; - const snapshot: BattleSnapshot = { - domain: storedSnapshot.domain, - attacker: deserializePet(storedSnapshot.attacker), - defender: deserializePet(storedSnapshot.defender), - takenAt: storedSnapshot.takenAt, - }; + // Decoded through the shared codec, which is what carries `schemaVersion` and the + // equipment list back out of storage. Rebuilding the snapshot field by field here is + // what previously dropped both: the receipt then encoded at layout version 1, its + // snapshot hash stopped matching the one acceptance committed, and the seed check + // inside `assertBattleReceipt` refused the receipt for every battle, geared or not. + const snapshot: BattleSnapshot = decodeStoredSnapshot(battle.snapshot); // Same deserialization need: PetProgression.petId/lastOpponentId are bigint in // the protocol type but decimal strings in storage. const storedProgression = battle.progression as unknown as { @@ -358,37 +355,6 @@ function serializeBigints(value: T): Prisma.InputJsonValue { return JSON.parse(JSON.stringify(value, (_key, v) => (typeof v === 'bigint' ? v.toString() : v))); } -interface StoredPet { - petId: string | bigint; - owner: string; - dna: string | bigint; - rarity: number; - level: number; - skill: number; - xp: number; - lastOpponentId: string | bigint; - streak: number; - readyAt: number; - sourceVersion: string | bigint; -} - -/** Reverses `serializeBigints` for one pet's snapshot fields. */ -function deserializePet(pet: StoredPet): BattleSnapshot['attacker'] { - return { - petId: BigInt(pet.petId), - owner: pet.owner, - dna: BigInt(pet.dna), - rarity: pet.rarity, - level: pet.level, - skill: pet.skill, - xp: pet.xp, - lastOpponentId: BigInt(pet.lastOpponentId), - streak: pet.streak, - readyAt: pet.readyAt, - sourceVersion: BigInt(pet.sourceVersion), - }; -} - interface StoredProgression { petId: string | bigint; won: boolean; diff --git a/backend/src/features/battle/worker/verify.worker.ts b/backend/src/features/battle/worker/verify.worker.ts index e733126a..329ce517 100644 --- a/backend/src/features/battle/worker/verify.worker.ts +++ b/backend/src/features/battle/worker/verify.worker.ts @@ -1,10 +1,10 @@ import { - type BattleSnapshot, bonusFromEquipment, type Hex, hashCombatLog, loadRulesetBundle, type PetProgression, + type PetSnapshot, type ProgressionDelta, type SimOutcome, } from '@cryptopets/protocol'; @@ -12,10 +12,15 @@ import { BattleState } from '@generated/prisma/enums'; import type { Prisma } from '@generated/prisma/client'; import { prisma } from '@config/prisma'; -import { applyTransition, type ClaimedMessage, completeOutbox, OUTBOX_TOPICS } from '@features/battle/ledger'; +import { + applyTransition, + type ClaimedMessage, + completeOutbox, + decodeStoredSnapshot, + OUTBOX_TOPICS, +} from '@features/battle/ledger'; import { callVerifyBattle, type VerifyBattleWire, type VerifyPetProgressionWire } from '@grpc-client/verifyBattle'; import { notifyBattleRoomIfPresent } from '@ws/battleRoomSocket'; -import { type SnapshotEquipment } from './compute.worker'; /** * Handles `verify` messages: `computed` -> `verified` (§F). @@ -57,9 +62,7 @@ export async function processVerifyMessage(message: ClaimedMessage, nowSeconds: } const ruleset = loadRulesetBundle(JSON.stringify(rulesetRow.bundle), battle.rulesetHash as Hex); - const snapshot = battle.snapshot as unknown as BattleSnapshot; - const attacker = snapshot.attacker as unknown as Record; - const defender = snapshot.defender as unknown as Record; + const { attacker, defender } = decodeStoredSnapshot(battle.snapshot); const outcome = await callVerifyBattle({ attacker: toWirePet(attacker), @@ -109,21 +112,21 @@ export async function processVerifyMessage(message: ClaimedMessage, nowSeconds: await completeOutbox(message.id, new Date(nowSeconds * 1000)); } -function toWirePet(pet: Record) { +function toWirePet(pet: PetSnapshot) { // The resolved equipment total, so the independent recomputation runs on the same // inputs the canonical engine used (roadmap §4). Sending the frozen modifiers rather // than item ids is what lets the verifier hold no item catalog at all: what §F checks // is that the fight follows from the numbers the receipt publishes. - const bonus = bonusFromEquipment(pet.equipment as SnapshotEquipment | undefined); + const bonus = bonusFromEquipment(pet.equipment); return { petId: String(pet.petId), dna: String(pet.dna), - rarity: Number(pet.rarity), - level: Number(pet.level), - skill: Number(pet.skill), - xp: Number(pet.xp), + rarity: pet.rarity, + level: pet.level, + skill: pet.skill, + xp: pet.xp, lastOpponentId: String(pet.lastOpponentId), - streak: Number(pet.streak), + streak: pet.streak, bonusHp: bonus.hp, bonusAtk: bonus.atk, bonusDef: bonus.def, diff --git a/backend/tests/features/battle/ledger/accept.service.test.ts b/backend/tests/features/battle/ledger/accept.service.test.ts index a22dd324..225f9519 100644 --- a/backend/tests/features/battle/ledger/accept.service.test.ts +++ b/backend/tests/features/battle/ledger/accept.service.test.ts @@ -1,6 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { QUICKNET, roundTime } from '@cryptopets/protocol'; +import { hashBattleSnapshot, QUICKNET, roundTime } from '@cryptopets/protocol'; vi.mock('@config/env', () => ({ env: { battle: { deploymentId: 'base-sepolia-live', chainIds: ['eip155:84532'] } }, @@ -56,7 +56,7 @@ vi.mock('../../../../src/features/battle/ledger/transitions', () => ({ })); import { prisma } from '@config/prisma'; -import { acceptBattle } from '@features/battle/ledger'; +import { acceptBattle, decodeStoredSnapshot } from '@features/battle/ledger'; import { chooseCommitmentRound, roundPublishTime } from '@features/battle/randomness'; import { activeSigningKey, sign, SignerRefusedError } from '@features/battle/signer'; import { consumeDailyBudget, findCoveringAuthorization } from '../../../../src/features/battle/ledger/consent.service'; @@ -312,6 +312,42 @@ describe('opening the ledger', () => { }); }); +describe('the stored snapshot survives a storage round trip', () => { + /** + * The property every worker downstream depends on: what acceptance persisted, read back + * through `decodeStoredSnapshot`, still hashes to the `snapshotHash` acceptance + * committed. The seed is derived from that hash and `assertBattleReceipt` re-derives it + * from the receipt's own snapshot, so a decoder that loses any field stops every battle + * at signing. + * + * Written as a property rather than as an assertion about `schemaVersion` and + * `equipment` specifically, because those are only the two fields that have been lost + * so far. Any field added to `PetSnapshot` is covered here on the day it is added. + */ + async function storedLedger() { + await acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW }); + return vi.mocked(openBattle).mock.calls[0]![0].ledger as unknown as { + snapshot: unknown; + snapshotHash: string; + }; + } + + it('rehashes to the committed snapshotHash', async () => { + const ledger = await storedLedger(); + expect(hashBattleSnapshot(decodeStoredSnapshot(ledger.snapshot))).toBe(ledger.snapshotHash); + }); + + it('rehashes to the committed snapshotHash with equipment', async () => { + vi.mocked(buildPetSnapshot).mockImplementation((async (_chainId: string, petId: string) => + petId === '1' + ? { ...ATTACKER, equipment: [{ slot: 0, itemType: 3n, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }] } + : DEFENDER) as never); + + const ledger = await storedLedger(); + expect(hashBattleSnapshot(decodeStoredSnapshot(ledger.snapshot))).toBe(ledger.snapshotHash); + }); +}); + describe('signer failure unwinds the accepted row', () => { it('moves the ledger to rejected and reports signer-unavailable', async () => { vi.mocked(sign).mockRejectedValue(new SignerRefusedError('signer-not-configured', 'no key')); diff --git a/backend/tests/features/battle/worker/compute.worker.test.ts b/backend/tests/features/battle/worker/compute.worker.test.ts index 87e22d56..4417e43c 100644 --- a/backend/tests/features/battle/worker/compute.worker.test.ts +++ b/backend/tests/features/battle/worker/compute.worker.test.ts @@ -9,10 +9,16 @@ vi.mock('@config/prisma', () => ({ }, })); -vi.mock('@features/battle/ledger', () => ({ +// The snapshot codec is pure and stays real. Stubbing it would let these tests pass +// against a decoder production does not use, which is exactly how the signing worker's +// schemaVersion bug survived a green suite. +vi.mock('@features/battle/ledger', async () => ({ applyTransition: vi.fn(), completeOutbox: vi.fn(), OUTBOX_TOPICS: { verify: 'verify' }, + ...(await vi.importActual( + '@features/battle/ledger/snapshot.codec', + )), })); vi.mock('@ws/battleRoomSocket', () => ({ diff --git a/backend/tests/features/battle/worker/sign.worker.test.ts b/backend/tests/features/battle/worker/sign.worker.test.ts index c45d7e4d..515f538d 100644 --- a/backend/tests/features/battle/worker/sign.worker.test.ts +++ b/backend/tests/features/battle/worker/sign.worker.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { + bonusFromEquipment, computeProgression, deriveBattleSeed, hashBattleReceipt, @@ -10,6 +11,7 @@ import { QUICKNET, roundTime, simulate, + SNAPSHOT_SCHEMA_VERSION, SOURCE_DEFAULT_RULESET, } from '@cryptopets/protocol'; @@ -31,10 +33,16 @@ vi.mock('@config/prisma', () => ({ }, })); -vi.mock('@features/battle/ledger', () => ({ +// The snapshot codec is pure and stays real. Stubbing it would let these tests pass +// against a decoder production does not use, which is exactly how the schemaVersion bug +// this file now covers survived a green suite. +vi.mock('@features/battle/ledger', async () => ({ applyTransition: vi.fn(), completeOutbox: vi.fn(), OUTBOX_TOPICS: { publish: 'publish' }, + ...(await vi.importActual( + '@features/battle/ledger/snapshot.codec', + )), })); vi.mock('@features/battle/signer', async () => { @@ -92,79 +100,117 @@ const DEFENDER = { lastOpponentId: '1', streak: 2, }; -const SNAPSHOT = { domain: DOMAIN, attacker: ATTACKER, defender: DEFENDER, takenAt: NOW - 10 }; - -// The real hash of the snapshot as production code will deserialize and hash it -// (real bigints, not the decimal strings JSON storage carries) — the seed check -// inside assertBattleReceipt recomputes this independently, so the fixture has to -// agree with it or every "happy path" case fails on the seed check alone. -const snapshotHash = hashBattleSnapshot({ - domain: DOMAIN as never, - attacker: { ...ATTACKER, petId: 1n, dna: BigInt(ATTACKER.dna), lastOpponentId: 0n, sourceVersion: 1000n } as never, - defender: { ...DEFENDER, petId: 2n, dna: BigInt(DEFENDER.dna), lastOpponentId: 1n, sourceVersion: 1000n } as never, - takenAt: SNAPSHOT.takenAt, -}); +/** One equipped item, in the decimal-string form JSON storage carries. */ +type StoredGear = { slot: number; itemType: string; hp: number; atk: number; def: number; int: number; mdef: number }; const beaconRandomness = '0xfe290beca10872ef2fb164d2aa4442de4566183ec51c56ff3cd603d930e54fdd'; -const seed = deriveBattleSeed({ - domain: DOMAIN as never, - drandRandomness: beaconRandomness, - battleId: 'btl_1', - snapshotHash, - rulesetHash: RULESET_HASH, -}); -const outcome = simulate( - BigInt(ATTACKER.dna), - ATTACKER.rarity, - ATTACKER.level, - ATTACKER.skill, - BigInt(DEFENDER.dna), - DEFENDER.rarity, - DEFENDER.level, - DEFENDER.skill, - seed.value, - SOURCE_DEFAULT_RULESET.skillConfig, -); -const combatLogHash = hashCombatLog(outcome); -const progression = computeProgression( - { + +/** + * A verified battle row, exactly as acceptance and the compute worker would have left it. + * + * Everything downstream is derived rather than pinned: the snapshot hash feeds the seed, + * the seed feeds the fight, and the fight feeds the progression, so a fixture that + * disagrees with production anywhere in that chain fails the seed check inside + * `assertBattleReceipt` rather than passing quietly. + * + * `schemaVersion` is declared, because acceptance declares it. Leaving it off made every + * fixture here a version 1 snapshot on both sides of the comparison, which is what let the + * signing worker hash real battles at a layout acceptance never used and still pass. + * + * The decoded form is spelled out rather than obtained from `decodeStoredSnapshot`, also + * deliberately: this is the value the codec is checked against, so deriving it from the + * codec would let a decoder that drops a field agree with itself. + */ +function buildFixture(gear?: { attacker?: StoredGear[]; defender?: StoredGear[] }) { + const attackerStored = { ...ATTACKER, ...(gear?.attacker && { equipment: gear.attacker }) }; + const defenderStored = { ...DEFENDER, ...(gear?.defender && { equipment: gear.defender }) }; + const stored = { + domain: DOMAIN, + attacker: attackerStored, + defender: defenderStored, + takenAt: NOW - 10, + schemaVersion: SNAPSHOT_SCHEMA_VERSION, + }; + + const decodeGear = (equipment?: StoredGear[]) => + equipment?.map((entry) => ({ ...entry, itemType: BigInt(entry.itemType) })); + const decoded = { domain: DOMAIN as never, - attacker: { ...ATTACKER, petId: 1n, dna: BigInt(ATTACKER.dna), lastOpponentId: 0n, sourceVersion: 1000n } as never, - defender: { ...DEFENDER, petId: 2n, dna: BigInt(DEFENDER.dna), lastOpponentId: 1n, sourceVersion: 1000n } as never, - takenAt: SNAPSHOT.takenAt, - }, - outcome.result.firstWins, -); -const serializedProgression = JSON.parse( - JSON.stringify(progression, (_k, v) => (typeof v === 'bigint' ? v.toString() : v)), -); - -const BATTLE = { - battleId: 'btl_1', - chainId: 'eip155:84532', - deploymentId: 'base-sepolia-live', - state: 'verified', - intentHash: `0x${'aa'.repeat(32)}`, - authorizationHash: `0x${'bb'.repeat(32)}`, - attackerPetId: '1', - defenderPetId: '2', - snapshot: SNAPSHOT, - seed: seed.hex, - rulesetHash: RULESET_HASH, - rulesetVersion: SOURCE_DEFAULT_RULESET.version, - drandChainHash: QUICKNET.chainHash, - drandRound: BigInt(1000), - beaconSignature: - '0xb44679b9a59af2ec876b1a6b1ad52ea9b1615fc3982b19576350f93447cb1125e342b73a8dd2bacbe47e4b6b63ed5e39', - beaconRandomness, - attackerWon: outcome.result.firstWins, - rounds: outcome.result.rounds, - winnerHpRemaining: outcome.result.winnerHpRemaining, - combatLogHash, - progression: serializedProgression, - verificationDetail: { mismatches: [] }, - roomId: 'room_1', -}; + attacker: { + ...attackerStored, + petId: BigInt(ATTACKER.petId), + dna: BigInt(ATTACKER.dna), + lastOpponentId: BigInt(ATTACKER.lastOpponentId), + sourceVersion: BigInt(ATTACKER.sourceVersion), + ...(gear?.attacker && { equipment: decodeGear(gear.attacker) }), + } as never, + defender: { + ...defenderStored, + petId: BigInt(DEFENDER.petId), + dna: BigInt(DEFENDER.dna), + lastOpponentId: BigInt(DEFENDER.lastOpponentId), + sourceVersion: BigInt(DEFENDER.sourceVersion), + ...(gear?.defender && { equipment: decodeGear(gear.defender) }), + } as never, + takenAt: stored.takenAt, + schemaVersion: SNAPSHOT_SCHEMA_VERSION, + }; + + const snapshotHash = hashBattleSnapshot(decoded); + const seed = deriveBattleSeed({ + domain: DOMAIN as never, + drandRandomness: beaconRandomness, + battleId: 'btl_1', + snapshotHash, + rulesetHash: RULESET_HASH, + }); + const outcome = simulate( + BigInt(ATTACKER.dna), + ATTACKER.rarity, + ATTACKER.level, + ATTACKER.skill, + BigInt(DEFENDER.dna), + DEFENDER.rarity, + DEFENDER.level, + DEFENDER.skill, + seed.value, + SOURCE_DEFAULT_RULESET.skillConfig, + bonusFromEquipment(decodeGear(gear?.attacker)), + bonusFromEquipment(decodeGear(gear?.defender)), + ); + const progression = computeProgression(decoded, outcome.result.firstWins); + + return { + battleId: 'btl_1', + chainId: 'eip155:84532', + deploymentId: 'base-sepolia-live', + state: 'verified', + intentHash: `0x${'aa'.repeat(32)}`, + authorizationHash: `0x${'bb'.repeat(32)}`, + attackerPetId: '1', + defenderPetId: '2', + snapshot: stored, + seed: seed.hex, + rulesetHash: RULESET_HASH, + rulesetVersion: SOURCE_DEFAULT_RULESET.version, + drandChainHash: QUICKNET.chainHash, + drandRound: BigInt(1000), + beaconSignature: + '0xb44679b9a59af2ec876b1a6b1ad52ea9b1615fc3982b19576350f93447cb1125e342b73a8dd2bacbe47e4b6b63ed5e39', + beaconRandomness, + attackerWon: outcome.result.firstWins, + rounds: outcome.result.rounds, + winnerHpRemaining: outcome.result.winnerHpRemaining, + combatLogHash: hashCombatLog(outcome), + progression: JSON.parse(JSON.stringify(progression, (_k, v) => (typeof v === 'bigint' ? v.toString() : v))), + verificationDetail: { mismatches: [] }, + roomId: 'room_1', + }; +} + +const BATTLE = buildFixture(); +/** Who won, which several assertions branch on. Read off the fixture rather than recomputed. */ +const outcome = { result: { firstWins: BATTLE.attackerWon } }; const MESSAGE = { id: 'msg_1', battleId: 'btl_1', topic: 'sign', payload: {}, attempts: 1 }; @@ -335,6 +381,57 @@ describe('the happy path', () => { }); }); +describe('equipment survives into the receipt (roadmap §4)', () => { + // A steel sword and reinforced plate from the shipped catalog, on the attacker only, so + // an assertion about the defender's absent list is meaningful rather than symmetric. + const GEAR = [ + { slot: 0, itemType: '3', hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }, + { slot: 1, itemType: '12', hp: 45, atk: 0, def: 16, int: 0, mdef: 6 }, + ]; + const GEARED = buildFixture({ attacker: GEAR }); + + beforeEach(() => { + vi.mocked(prisma.battleLedger.findUnique).mockResolvedValue(GEARED as never); + }); + + it('signs a geared battle, whose seed only derives from a version 2 snapshot', async () => { + // The whole failure mode in one assertion: `hashBattleReceipt` re-derives the seed + // from the snapshot the receipt carries, so a worker that dropped the gear or the + // layout version would throw here rather than sign. + await processSignMessage(MESSAGE, NOW); + expect(sign).toHaveBeenCalledTimes(1); + }); + + it('carries the resolved modifiers and the item type into the persisted receipt', async () => { + const tx = fakeTx(); + vi.mocked(applyTransition).mockImplementationOnce((async (req: { onApplied?: (tx: unknown) => Promise }) => { + if (req.onApplied) await req.onApplied(tx); + return { applied: true, state: 'signed' }; + }) as never); + + await processSignMessage(MESSAGE, NOW); + + const { payload } = tx.battleReceipt.create.mock.calls[0]![0].data; + // Item type as a decimal string, since the payload is stored as JSON. The modifiers + // ride along with it: they are what a replay uses, and the type is what lets a + // third party check them against the published catalog. + expect(payload.snapshot.attacker.equipment).toEqual([ + { slot: 0, itemType: '3', hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }, + { slot: 1, itemType: '12', hp: 45, atk: 0, def: 16, int: 0, mdef: 6 }, + ]); + // Absent, not empty: an ungeared pet encodes a zero-length list either way, and + // omitting it keeps the stored row identical to what it was before gear existed. + expect(payload.snapshot.defender.equipment).toBeUndefined(); + expect(payload.snapshot.schemaVersion).toBe(SNAPSHOT_SCHEMA_VERSION); + }); + + it('fights the geared battle differently from the ungeared one', async () => { + // Guards the fixture itself. If this gear made no difference to the outcome, the + // two tests above would pass against an engine that ignored equipment entirely. + expect(GEARED.seed).not.toBe(BATTLE.seed); + }); +}); + describe('chain-position retry', () => { it('retries with a fresh chain head when another battle under this key wins the position first', async () => { vi.mocked(prisma.battleReceipt.findFirst) diff --git a/backend/tests/features/battle/worker/verify.worker.test.ts b/backend/tests/features/battle/worker/verify.worker.test.ts index 2877c1ff..8c4381c5 100644 --- a/backend/tests/features/battle/worker/verify.worker.test.ts +++ b/backend/tests/features/battle/worker/verify.worker.test.ts @@ -19,10 +19,16 @@ vi.mock('@config/prisma', () => ({ }, })); -vi.mock('@features/battle/ledger', () => ({ +// The snapshot codec is pure and stays real. Stubbing it would let these tests pass +// against a decoder production does not use, which is exactly how the signing worker's +// schemaVersion bug survived a green suite. +vi.mock('@features/battle/ledger', async () => ({ applyTransition: vi.fn(), completeOutbox: vi.fn(), OUTBOX_TOPICS: { sign: 'sign' }, + ...(await vi.importActual( + '@features/battle/ledger/snapshot.codec', + )), })); vi.mock('@grpc-client/verifyBattle', () => ({ diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md new file mode 100644 index 00000000..437d21e3 --- /dev/null +++ b/docs/plan-battle-inventory-hardening.md @@ -0,0 +1,248 @@ +# Plan: harden the battle + inventory seam before it goes live + +Review of the shipped roadmap §4 work (`docs/plan-inventory-items.md`, all four phases marked +complete) against the backend-authoritative battle path (`docs/battle-protocol.md`). This file +is the execution order for what that review found. Each step ends at a command that passes. + +Branch: `fix/battle-inventory-seam`. + +## Verdict + +The feature is well built. Ownership boundaries are stated and held (indexer writes the +projections, the seeder writes the catalog, the player signs the equip), the two live combat +ports move together, the golden vectors cover the modifier ordering at a one-point margin, and +the doc comments record reasoning rather than restating code. Every suite is green: + +| Suite | Result | +|---|---| +| `pnpm --filter backend test` | 914 passed / 89 files | +| `pnpm --filter @cryptopets/protocol test` | 595 passed / 34 files | +| `pnpm --filter @cryptopets/verifier test` | 86 passed / 13 files | +| `pnpm --filter frontend test` | 370 passed / 48 files | +| `pnpm --filter @shared/core test` | 567 passed / 80 files | +| `go test ./internal/{combat,evm,store}` | ok | + +The defects are concentrated at one seam: the point where a stored snapshot is read back out +of the ledger. Phase 4 gave the snapshot a schema version and an equipment list, and one of the +three readers was never updated. That reader is the signing worker, so nothing settles. + +Severity ordering below is by consequence, not by size of fix. + +--- + +## B1 (blocker): the sign worker rebuilds every snapshot at schema version 1 + +**No battle can produce a receipt on any deployment where this code runs.** Not only geared +battles. Every battle. + +`accept.service.ts:167-174` writes the snapshot with `schemaVersion: SNAPSHOT_SCHEMA_VERSION` +(currently 2) and stores `snapshotHash` computed at that version. `beacon.worker.ts:65` derives +the battle seed from that stored hash. + +`sign.worker.ts:63-74` reads the row back through a `storedSnapshot` type that declares only +`domain`, `attacker`, `defender`, `takenAt`, and a `deserializePet` (line 376) whose `StoredPet` +interface stops at `sourceVersion`. Both `schemaVersion` and `equipment` are dropped. The +reconstructed object therefore encodes at version 1, because `assertBattleSnapshot` reads an +absent version as 1 by design. + +`writeHeader` writes the version as a `u16` inside the hashed bytes, so a v1 encoding and a v2 +encoding of the same ungeared pet differ. Confirmed by running the two hashes side by side on +an identical ungeared pair: + +``` +accept (schemaVersion 2): 0xf7ccca0ba2c0971b7f3b3a18b9bc200aa5616c84751efa3256ab93524ed7a054 +sign (version dropped): 0xac2c61bb061506d0a88b00b3c31203daba05e0a82d48239c8fd99cc54b60a159 +``` + +`hashBattleReceipt` calls `assertBattleReceipt`, which re-derives the seed from +`hashBattleSnapshot(receipt.snapshot)` (`protocol/src/receipt/types.ts:152-161`) and throws when +it disagrees with `receipt.seed`. That throw is not a `SignerRefusedError`, so it escapes +`processSignMessage` into the dispatcher's backoff. Battles pile up in `verified` and +dead-letter. + +For a geared battle the same bug has a second effect: had it not thrown first, the receipt +would publish a snapshot with the gear removed, and `checks/combatReplay.ts` would replay an +ungeared fight against a geared result. + +Why the suite is green: `backend/tests/features/battle/worker/sign.worker.test.ts:95` defines +`SNAPSHOT` with no `schemaVersion` and computes its fixture `snapshotHash` from the same +version-less object (line 101). The fixture is a v1 snapshot on both sides, so it agrees with +itself. Production writes v2 on one side only. + +### Fix + +- [x] **B1.1 One deserializer, not three.** Extract the stored-snapshot reader into + `backend/src/features/battle/ledger/snapshot.codec.ts` next to `snapshot.builder.ts`, + round-tripping `schemaVersion` and `equipment` alongside the bigint fields. Delete + `compute.worker.ts:113-145`'s copy and `sign.worker.ts:361-390`'s copy, and have + `verify.worker.ts:60-62` use it instead of casting to `Record`. Three + readers of one stored shape is what let one of them fall behind, and a fix that patches + only the third leaves the same trap set. + Verify: `pnpm --filter backend test`. +- [x] **B1.2 Make the fixture representative.** Set `schemaVersion: SNAPSHOT_SCHEMA_VERSION` on + the sign-worker test's stored snapshot and derive its `snapshotHash` from the same object, + so the test fails without B1.1. Add a second case with a geared attacker asserting the + persisted `payload.snapshot.attacker.equipment` survives into the receipt. + Verify: `pnpm --filter backend exec vitest run tests/features/battle/worker/sign.worker.test.ts`. +- [x] **B1.3 Close the class of bug, not the instance.** Add a ledger-level test that runs + accept, then re-reads the stored row and asserts `hashBattleSnapshot(decoded) === + row.snapshotHash`. That assertion holds for any future field added to the snapshot, + which the two tests above do not. + Verify: `pnpm --filter backend test`. + +Nothing outside `backend/src/features/battle` changes. The protocol encoder, both combat ports +and the vectors are correct as they stand. + +--- + +## C1: an unreadable catalog effect silently re-prices the ruleset + +`catalog.ts:158-162` states the rule: "once an effect feeds combat, an unreadable one has to be +a hard error, because silently dropping it would change a fight rather than a label." Phase 4 +made effects feed combat and the code did not follow. + +`toItemView` (`inventory.service.ts:229-247`) calls `asItemEffect`, which returns `null` on any +shape it does not recognise, logs a warning, and continues. Two consumers then read the result +as authoritative: + +- `ruleset.builder.ts:40` skips the item, so it leaves `itemCatalog`. That moves `rulesetHash`, + which invalidates every outstanding `DefenseAuthorization`. +- `snapshot.builder.ts:91` skips it too, so a pet wearing that item fights ungeared, and its + receipt says it was ungeared. + +Both happen from one malformed JSON column, with a `console.warn` as the only signal. + +- [ ] **C1.1** Split the read. Keep `asItemEffect`'s leniency on the display path (a bag with one + unnamed tile beats a bag that will not open) and make the combat path strict: a + `stat_bonus` row that fails to parse throws from `servedRuleset()` and from + `resolveEquipment`. A deployment that cannot state its own rules should refuse to accept + battles rather than quietly fight under different ones. + Verify: `pnpm --filter backend test`. + +## C2: an equipped item missing from the catalog fights as nothing + +Same shape, different cause. `resolveEquipment` (`snapshot.builder.ts:86-109`) drops any equipped +item with no `stat_bonus`, including one with no catalog row at all. `getPetEquipment` warns and +drops it first. + +The receipt then says ungeared while `ItemCore.equipmentOf(petId)` at the recorded +`sourceVersion` says otherwise. That is exactly the cross-check §4 added `itemType` to the +snapshot to enable, reporting a discrepancy an outsider cannot distinguish from operator +misbehaviour. + +- [ ] **C2.1** Reject the acceptance instead. An uncatalogued equipped item means the seeder is + behind the contract, which is an operational fault; failing the accept with a named reason + surfaces it in seconds, where a silent ungeared fight surfaces as an unexplained verifier + failure weeks later. Reuse the existing reject path in `accept.service.ts`. + Verify: `pnpm --filter backend test`. + +## C3: the TypeScript bonus sum is unclamped where Go range-checks + +`protocol/src/combat/equipment.ts:56-66`'s `sumBonuses` totals in plain JS numbers with no +ceiling; only `applyBonus` clamps, and it clamps after adding to the attributes. Go's +`SumBonuses` saturates at each step. The two agree on the final attribute value, because both +ceilings are 65535 and attributes are non-negative, so this is not a live divergence. + +It does change one thing. `verify.worker.ts:117` sends the unclamped total over gRPC, and +`grpcsrv/verify.go:111-125` rejects any bonus field above 65535 rather than truncating. A total +past the ceiling becomes an RPC error, which `processVerifyMessage` correctly treats as "could +not check" rather than "disagreed", so the battle retries and dead-letters. + +Unreachable with shipped content: `MAX_STAT_BONUS` is 500, three slots cap a pet at 1500, and +the shipped catalog's largest single bonus is 45 HP. This is a guardrail, not a bug. + +- [ ] **C3.1** Clamp in `sumBonuses` to match the Go port, and add the case to + `contracts/test-vectors/equipment.json` so the two stay pinned. Both live ports change in + the same commit, per `AGENTS.md`. + Verify: `pnpm --filter @cryptopets/protocol test && go test ./internal/combat` from + `services/indexer-go`. + +--- + +## D1 (decision, not a fix): consent bounds level, gear is unbounded + +`DefenseAuthorization` covers pet, attacker level band, ruleset hash, validity window and daily +cap. Phase 4 made equipment a combat input without adding it to that list. A defender who +authorizes a level 10 to 14 attacker gets whatever that attacker equips afterwards, and the +snapshot is taken at accept, after consent. + +Sized honestly: the shipped catalog tops out near +22 ATK against attributes in the low +hundreds, so today this is a tuning matter rather than an exploit. But `MAX_STAT_BONUS` permits +500 a stat, and the level band is the only power bound the defender was given. + +Options, in the order I would take them: + +1. **Bound it in the ruleset.** Add a per-fight modifier cap to `Ruleset`, so the band the + defender consents to implies a power ceiling. Costs a ruleset schema bump and a re-consent + event, which item D3 below already requires once. +2. **Put a gear digest in the authorization.** Strictly correct and much worse to use: the + defender re-consents every time an attacker changes a sword. +3. **Accept it and write it down.** Defensible while the catalog stays modest. Needs a stated + ceiling in `catalog.ts` that a content edit cannot quietly raise. + +- [ ] **D1.1** Pick one. This is a game-design call, not an engineering one, and per CLAUDE.md + it does not get decided in a loop. + +## D2: drops are outside the signed payload + +Recorded in `drops.ts:14-19` as a known v1 limit and correct as written: derived from the +battle's own drand seed, written in the receipt's transaction, recomputable by anyone holding +the receipt. What an outsider cannot do is *prove* a discrepancy from the receipt alone. + +Listed here so it is a tracked decision rather than a comment. It needs a receipt schema +version, so it belongs with any other bump rather than on its own. + +## D3: shipping Phase 4 is a re-consent event + +Already recorded in `plan-inventory-items.md`. `ENGINE_VERSION` 1 to 2 plus the ruleset's item +catalog moves `rulesetHash` for every battle, so every outstanding `DefenseAuthorization` is +invalidated and every defender re-consents once. Intended behaviour, user-visible, ships +deliberately. If D1 lands as option 1, fold it into the same rollout and pay this once. + +--- + +## Code quality + +Small, none of them urgent. + +- [ ] **Q1 Two caches, one reset each.** `inventory.service.ts:205` caches the catalog for the + process's life and `ruleset.builder.ts:30` caches a ruleset derived from it. Their reset + seams are separate (`resetItemCatalog`, `resetServedRuleset`), so clearing one leaves the + other holding data built from what was just dropped. Have `resetItemCatalog` clear both. +- [ ] **Q2 Orphaned doc comment.** `env.ts:141-146` documents `adminWallets` directly above the + comment for `dropsEnabled`; the field itself is at line 156. Move the comment to its field. +- [x] **Q3 `verify.worker.ts:60-62` casts to `Record`** to read a shape the + codec from B1.1 will type properly. Folded into B1.1 rather than done twice. + +## Operational, unblocked by code + +Carried over from `plan-inventory-items.md`'s "still outstanding", still outstanding. All three +are operator calls. + +- [ ] **O1 Apply the migration.** `20260807160000_add_inventory` has never run. RLS is correctly + present on all four new tables (`migration.sql:78-81`). `pnpm --filter backend prisma:migrate`, + which is `migrate deploy`, never `dev`. +- [ ] **O2 Run the seeder,** then `scripts/verify-inventory-setup.ts` to confirm the on-chain slot + registrations and `item_definition` agree. The chain half was registered directly during the + Base Sepolia deploy because the table did not exist; the seeder is idempotent and will find + every slot already correct. +- [ ] **O3 End-to-end, once.** The check at the end of `plan-inventory-items.md`. Neither web + screen has been opened against real data and the `ItemCore` write client is stubbed in every + test, so the first real exercise of grant, claim, equip, fight, verify is still ahead. Do it + after B1, or it will fail at signing regardless of anything inventory does. + +--- + +## Order + +B1 first and alone: nothing settles until it lands, so every other check runs against a stalled +pipeline. Then C1 and C2 together (one theme, adjacent code). C3 with its vector case. D1 needs +an answer before D3 is scheduled, since they should ship as one re-consent. O1 to O3 last, +because they are the only steps that touch production. + +## Do not touch + +- `contracts/test-vectors/{battle,xp,equipment}.json`. Nothing here is a vector failure. +- Solana's frozen ports (`game/battle_sim.rs`, `game/xp.rs`). +- The snapshot and ruleset encoders. Both handle their two versions correctly; B1 is a caller + that stopped telling them which version it held. From 17e1f92b5d66d96b892f96ccbb52dda794ce395b Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 14:03:10 -0400 Subject: [PATCH 02/36] fix(backend): refuse a battle the item catalog cannot price --- backend/src/config/env.ts | 12 +- .../battle/ledger/accept.controller.ts | 11 +- .../features/battle/ledger/accept.service.ts | 52 +++++- .../features/battle/ledger/ruleset.builder.ts | 33 +++- .../battle/ledger/snapshot.builder.ts | 35 ++-- backend/src/features/inventory/index.ts | 5 + .../features/inventory/inventory.service.ts | 164 ++++++++++++++++-- .../battle/ledger/accept.service.test.ts | 43 +++++ .../battle/ledger/ruleset.builder.test.ts | 5 +- .../battle/ledger/snapshot.builder.test.ts | 43 ++--- .../inventory/inventory.service.test.ts | 116 ++++++++++++- docs/plan-battle-inventory-hardening.md | 19 +- protocol/src/combat/equipment.ts | 29 +++- .../tests/combat/equipmentVectors.test.ts | 31 ++++ .../internal/combat/equipment_golden_test.go | 27 +++ 15 files changed, 534 insertions(+), 91 deletions(-) diff --git a/backend/src/config/env.ts b/backend/src/config/env.ts index a13aed8d..cdadcc16 100644 --- a/backend/src/config/env.ts +++ b/backend/src/config/env.ts @@ -138,12 +138,6 @@ export const env = { : undefined) as `0x${string}` | undefined, chainId: process.env.ITEM_CORE_CHAIN_ID ? Number(process.env.ITEM_CORE_CHAIN_ID) : undefined, address: process.env.ITEM_CORE_ADDRESS?.trim() as `0x${string}` | undefined, - /** - * Wallets allowed to grant items, comma-separated. Empty by default, so the admin - * route is closed until someone is named rather than open until someone is - * excluded. Normalized here so a checksummed address in the env still matches the - * lowercased one the JWT carries. - */ /** * Whether a settled battle pays item drops. * @@ -153,6 +147,12 @@ export const env = { * added for something else. */ dropsEnabled: process.env.ITEM_DROPS_ENABLED?.trim().toLowerCase() === 'true', + /** + * Wallets allowed to grant items, comma-separated. Empty by default, so the admin + * route is closed until someone is named rather than open until someone is + * excluded. Normalized here so a checksummed address in the env still matches the + * lowercased one the JWT carries. + */ adminWallets: new Set( (process.env.ITEM_ADMIN_WALLETS ?? '') .split(',') diff --git a/backend/src/features/battle/ledger/accept.controller.ts b/backend/src/features/battle/ledger/accept.controller.ts index 99f4106f..aa5948dd 100644 --- a/backend/src/features/battle/ledger/accept.controller.ts +++ b/backend/src/features/battle/ledger/accept.controller.ts @@ -6,9 +6,13 @@ import { acceptBattle, type AcceptRejection } from './accept.service'; /** * 409 for "someone already acted on this", 404/403/422 for the client's own fault, and 503 for - * the two dependencies this flow cannot proceed without (drand, the signer). A 503 is the - * honest answer for those: retrying shortly is the correct client behaviour, and nothing about - * the request itself was wrong. + * the dependencies this flow cannot proceed without (drand, the signer, a catalog that can + * price the gear in play). A 503 is the honest answer for those: retrying shortly is the + * correct client behaviour, and nothing about the request itself was wrong. + * + * A stale catalog is the odd one of the three, since retrying will not help until someone runs + * the seeder. It is still a 503 rather than a 500: the deployment is misconfigured, not broken, + * and a client that backs off and retries is behaving correctly either way. */ const STATUS_BY_REASON: Record = { 'intent-not-found': 404, @@ -30,6 +34,7 @@ const STATUS_BY_REASON: Record = { 'pet-locked': 409, 'drand-unavailable': 503, 'signer-unavailable': 503, + 'item-catalog-stale': 503, }; interface AcceptBody { diff --git a/backend/src/features/battle/ledger/accept.service.ts b/backend/src/features/battle/ledger/accept.service.ts index dd59c323..3dee9163 100644 --- a/backend/src/features/battle/ledger/accept.service.ts +++ b/backend/src/features/battle/ledger/accept.service.ts @@ -16,6 +16,7 @@ import type { Prisma } from '@generated/prisma/client'; import { BattleState } from '@generated/prisma/enums'; import { prisma } from '@config/prisma'; +import { ItemCatalogError } from '@features/inventory'; import { notifyBattleRoomIfPresent } from '@ws/battleRoomSocket'; import { activeSigningKey, sign, SignerRefusedError } from '../signer'; @@ -76,7 +77,17 @@ export type AcceptRejection = | ConsentFailure | 'pet-locked' | 'drand-unavailable' - | 'signer-unavailable'; + | 'signer-unavailable' + /** + * The item catalog cannot price something this battle needs priced: a pet wears an + * item with no catalog row, or an equipment row's modifier will not parse (roadmap §4). + * + * Its own reason rather than a 500, because it is an operational fault with an obvious + * remedy (run the seeder) and no fault of the player's. Refusing is the conservative + * end: the alternative is a fight under rules this deployment cannot state, recorded in + * a signed receipt that contradicts chain state. + */ + | 'item-catalog-stale'; export interface AcceptedBattle { battleId: string; @@ -105,10 +116,16 @@ export async function acceptBattle(request: AcceptBattleRequest): Promise>; + let defender: Awaited>; + try { + [attacker, defender] = await Promise.all([ + buildPetSnapshot(chainId, intent.attackerPetId), + buildPetSnapshot(chainId, intent.defenderPetId), + ]); + } catch (error) { + return catalogRejection(error); + } if (!attacker) { return reject('attacker-pet-missing', `pet ${intent.attackerPetId} is not in the roster`); } @@ -126,7 +143,12 @@ export async function acceptBattle(request: AcceptBattleRequest): Promise>; + try { + ruleset = await servedRuleset(); + } catch (error) { + return catalogRejection(error); + } const rulesetHash = hashRuleset(ruleset); await ensureRulesetPublished(rulesetHash); @@ -381,3 +403,21 @@ function serializeBigints(value: T): Prisma.InputJsonValue { function reject(reason: AcceptRejection, detail: string): AcceptBattleResult { return { ok: false, reason, detail }; } + +/** + * Turns a stale item catalog into a named rejection, and rethrows anything else. + * + * Both catalog-dependent reads on this path (what the pets are wearing, and the ruleset + * the fight is priced under) run before the first write, so refusing here strands nothing: + * no ledger row, no consumed intent, no spent daily budget. + * + * Rethrows rather than swallowing, because "the catalog is behind the contract" is a + * recoverable operational state with a clear remedy, while any other failure here is a bug + * and should keep reaching the error handler as one. + */ +function catalogRejection(error: unknown): AcceptBattleResult { + if (error instanceof ItemCatalogError) { + return reject('item-catalog-stale', error.message); + } + throw error; +} diff --git a/backend/src/features/battle/ledger/ruleset.builder.ts b/backend/src/features/battle/ledger/ruleset.builder.ts index aac12d26..b692c3c1 100644 --- a/backend/src/features/battle/ledger/ruleset.builder.ts +++ b/backend/src/features/battle/ledger/ruleset.builder.ts @@ -1,6 +1,6 @@ import { type ItemModifier, type Ruleset, SOURCE_DEFAULT_RULESET } from '@cryptopets/protocol'; -import { getCatalog } from '@features/inventory'; +import { getCombatCatalog, itemCatalogGeneration } from '@features/inventory'; /** * Builds the ruleset this deployment fights under (roadmap §4). @@ -26,15 +26,26 @@ import { getCatalog } from '@features/inventory'; * path to answer the same question. A catalog edit therefore needs a restart to take * effect, which is the right shape for something that invalidates outstanding consent: * it should be a deliberate rollout, not a row edit that quietly re-prices live battles. + * + * Stamped with the catalog generation it was built from, so dropping the catalog drops + * this too. Two independent process-life caches over the same rows was a trap: whichever + * one a caller knew to reset, the other kept answering from data that no longer existed. */ -let cached: Ruleset | null = null; +let cached: { ruleset: Ruleset; generation: number } | null = null; export async function servedRuleset(): Promise { - if (cached) { - return cached; + // Read before the await, not after: a reset landing mid-build then stamps this result + // with the older generation, so the next call rebuilds. The other order would stamp a + // half-stale ruleset as current. + const generation = itemCatalogGeneration(); + if (cached && cached.generation === generation) { + return cached.ruleset; } - const catalog = await getCatalog(); + // The strict read: an equipment row whose modifier will not parse throws here rather + // than dropping out of the list. Dropping it would move `rulesetHash` and invalidate + // every outstanding defence authorization on the strength of one bad column. + const catalog = await getCombatCatalog(); const itemCatalog: ItemModifier[] = []; for (const item of catalog) { if (item.effect?.kind !== 'stat_bonus' || item.slot === null) { @@ -56,11 +67,17 @@ export async function servedRuleset(): Promise { // type surfaces rather than being tidied away. itemCatalog.sort((a, b) => (a.itemType < b.itemType ? -1 : a.itemType > b.itemType ? 1 : 0)); - cached = { ...SOURCE_DEFAULT_RULESET, itemCatalog }; - return cached; + cached = { ruleset: { ...SOURCE_DEFAULT_RULESET, itemCatalog }, generation }; + return cached.ruleset; } -/** Test seam: drops the memoized ruleset so a changed catalog is picked up. */ +/** + * Test seam: drops the memoized ruleset directly. + * + * Rarely the one to reach for now. `resetItemCatalog()` invalidates this as well, which is + * what a caller changing catalog rows actually wants; this is for a test that stubs the + * catalog module itself and so never bumps a generation. + */ export function resetServedRuleset(): void { cached = null; } diff --git a/backend/src/features/battle/ledger/snapshot.builder.ts b/backend/src/features/battle/ledger/snapshot.builder.ts index e7c0826e..450f741d 100644 --- a/backend/src/features/battle/ledger/snapshot.builder.ts +++ b/backend/src/features/battle/ledger/snapshot.builder.ts @@ -1,7 +1,7 @@ import { chainFamily, type ChainId, type EquipEntry, type PetSnapshot } from '@cryptopets/protocol'; import { prisma } from '@config/prisma'; -import { getPetEquipment } from '@features/inventory'; +import { getPetEquipmentForCombat } from '@features/inventory'; import { servedDeploymentId } from './domain'; @@ -79,28 +79,23 @@ export async function buildPetSnapshot(chainId: ChainId, petId: string): Promise * so what is frozen is what the chain said at a version the snapshot records. An outsider * can therefore check the gear as well as the numbers. * - * An equipped item with no catalog effect contributes nothing and is left out entirely. - * Including it with zeroes would put an entry in the receipt claiming an item was worn and - * did nothing, which reads as a bug rather than as a fact. + * `getPetEquipmentForCombat` throws rather than skipping an item this process cannot + * price, so there is no filtering left to do here. Skipping was the tempting version and + * the wrong one: it produced a receipt claiming the pet fought bare while chain state at + * `sourceVersion` said it was wearing something. */ async function resolveEquipment(family: string, petId: string): Promise { - const equipped = await getPetEquipment(family, petId); - const entries: EquipEntry[] = []; + const equipped = await getPetEquipmentForCombat(family, petId); - for (const { slot, item } of equipped) { - if (item.effect?.kind !== 'stat_bonus') { - continue; - } - entries.push({ - slot, - itemType: BigInt(item.itemType), - hp: item.effect.hp, - atk: item.effect.atk, - def: item.effect.def, - int: item.effect.int, - mdef: item.effect.mdef, - }); - } + const entries = equipped.map(({ slot, itemType, bonus }) => ({ + slot, + itemType: BigInt(itemType), + hp: bonus.hp, + atk: bonus.atk, + def: bonus.def, + int: bonus.int, + mdef: bonus.mdef, + })); // Ascending by slot, which the protocol requires: the order is part of the snapshot // digest, and `assertPetSnapshot` refuses to sort silently so an upstream bug that diff --git a/backend/src/features/inventory/index.ts b/backend/src/features/inventory/index.ts index e5122aba..d496f4d1 100644 --- a/backend/src/features/inventory/index.ts +++ b/backend/src/features/inventory/index.ts @@ -4,11 +4,16 @@ */ export { getCatalog, + getCombatCatalog, getInventory, getPendingItems, getPetEquipment, + getPetEquipmentForCombat, getPetEquipmentForPets, + ItemCatalogError, + itemCatalogGeneration, resetItemCatalog, + type CombatEquippedItem, type EquippedItem, type InventoryEntry, type PendingItem, diff --git a/backend/src/features/inventory/inventory.service.ts b/backend/src/features/inventory/inventory.service.ts index 6334d892..a0eb34fb 100644 --- a/backend/src/features/inventory/inventory.service.ts +++ b/backend/src/features/inventory/inventory.service.ts @@ -9,7 +9,7 @@ import { type ItemDefinitionRow, } from '@repositories/inventory.repository'; -import { asItemEffect, type ItemEffect } from './catalog'; +import { asItemEffect, type ItemEffect, type StatBonus } from './catalog'; /** * Inventory reads (roadmap §4). @@ -97,8 +97,14 @@ export async function getPendingItems(chain: string, owner: string): Promise { - return [...(await catalogByType()).values()]; + return [...(await loadCatalog()).byType.values()]; } /** @@ -189,6 +195,36 @@ export async function getPetEquipmentForPets( return [...byPet].map(([petId, equipped]) => ({ petId, equipped })); } +/** + * What one equipped item contributes to a fight, already narrowed to the modifier. + * + * Distinct from `EquippedItem` because a combat caller has no use for a name or a + * description and every use for a bonus it does not have to re-narrow. The narrowing is + * the point: reaching this type at all means the item is catalogued equipment with a + * readable modifier, so `snapshot.builder` has nothing left to check. + */ +export interface CombatEquippedItem { + slot: number; + itemType: string; + key: string; + bonus: StatBonus; +} + +/** + * Raised when the catalog cannot answer a question combat needs answered. + * + * Its own type so acceptance can turn it into a named rejection rather than a 500. Every + * case it covers is an operational fault: the seeder is behind the contract, or a row was + * edited into a shape the reader does not recognise. Both mean this deployment cannot + * state the rules it is about to fight under. + */ +export class ItemCatalogError extends Error { + constructor(detail: string) { + super(detail); + this.name = 'ItemCatalogError'; + } +} + /** * The catalog, read once per process. * @@ -201,46 +237,142 @@ export async function getPetEquipmentForPets( * `servedRuleset()` already caches catalog-derived data and documents that a catalog edit * needs a restart. With one half frozen and the other live, a mid-process seeder run produced * a ruleset that did not price an item the bag was already showing. + * + * `unreadable` is kept beside the views because `ItemView.effect` is null for two very + * different rows: a collectible that legitimately does nothing, and an equipment row whose + * modifier would not parse. A display path may treat those alike; a combat path must not, + * and the null alone cannot tell them apart. */ -let cached: Map | null = null; +interface CachedCatalog { + byType: Map; + /** Types whose stored `effect` column was present but unreadable. */ + unreadable: Set; +} -async function catalogByType(): Promise> { +let cached: CachedCatalog | null = null; + +async function loadCatalog(): Promise { if (!cached) { - cached = new Map((await findAllDefinitions()).map((row) => [row.itemType, toItemView(row)])); + const byType = new Map(); + const unreadable = new Set(); + for (const row of await findAllDefinitions()) { + const view = toItemView(row); + byType.set(row.itemType, view); + if (row.effect !== null && view.effect === null) { + unreadable.add(row.itemType); + // Loud because the only writer is the seeder, so this means the stored + // shape and the code that reads it have diverged. + console.warn(`[inventory] item ${row.itemType} (${row.key}) has an unreadable effect payload`); + } + } + cached = { byType, unreadable }; } return cached; } -/** Drops the cache, for the seeder and for tests. Mirrors `resetServedRuleset`. */ +/** + * How many times the catalog has been dropped. + * + * Read by anything holding its own cache of catalog-derived data, so dropping the catalog + * invalidates that too. `servedRuleset` is the one such holder, and it cannot simply be + * called from `resetItemCatalog`: `ruleset.builder` imports this module, so the call would + * close a cycle. A number it can compare against costs nothing and points the dependency + * the way it already runs. + */ +let generation = 0; + +export function itemCatalogGeneration(): number { + return generation; +} + +/** Drops the cache, for the seeder and for tests. Also invalidates anything derived from it. */ export function resetItemCatalog(): void { cached = null; + generation += 1; +} + +/** + * The catalog as the ruleset must read it. + * + * Strict where `getCatalog` is lenient, and the split is the rule `catalog.ts` states for + * itself: an unreadable effect costs an item its label on a read path, but once effects + * feed combat, dropping one silently changes a fight rather than a tooltip. An equipment + * row whose modifier will not parse simply vanishes from `itemCatalog`, which moves + * `rulesetHash` and invalidates every outstanding defence authorization, from one bad + * column and a console warning. + */ +export async function getCombatCatalog(): Promise { + const catalog = await loadCatalog(); + for (const view of catalog.byType.values()) { + if (view.category !== 'equipment') { + continue; + } + if (catalog.unreadable.has(view.itemType) || view.effect?.kind !== 'stat_bonus') { + throw new ItemCatalogError( + `item ${view.itemType} (${view.key}) is equipment with no readable stat_bonus; this deployment cannot state its own ruleset`, + ); + } + } + return [...catalog.byType.values()]; +} + +/** + * What a pet has equipped, resolved for combat. + * + * Refuses the two states `getPetEquipment` hides. An item with no catalog row is the + * seeder running behind the contract; an item whose modifier will not parse is a corrupt + * row. Either way the pet is wearing something on chain that this process cannot price, + * and the lenient read would have it fight as though the slot were empty. + * + * That is worse than it sounds, because it is not merely a weaker pet. The receipt would + * publish an ungeared snapshot while `ItemCore.equipmentOf(petId)` at the recorded + * `sourceVersion` says otherwise, and that discrepancy is indistinguishable from the + * operator having quietly removed the gear. §4 put `itemType` in the snapshot precisely so + * an outsider could make that comparison; failing here keeps the answer honest. + */ +export async function getPetEquipmentForCombat(chain: string, petId: string): Promise { + const slots = await findEquipment(chain, petId); + if (slots.length === 0) { + return []; + } + + const catalog = await loadCatalog(); + return slots.map(({ slot, itemType }) => { + const item = catalog.byType.get(itemType); + if (!item) { + throw new ItemCatalogError( + `pet ${petId} has uncatalogued item type ${itemType} equipped in slot ${slot}; the item catalog is behind the contract`, + ); + } + if (catalog.unreadable.has(itemType) || item.effect?.kind !== 'stat_bonus') { + throw new ItemCatalogError( + `pet ${petId} has item ${itemType} (${item.key}) equipped in slot ${slot}, which carries no readable stat_bonus`, + ); + } + return { slot, itemType, key: item.key, bonus: item.effect }; + }); } async function definitionsByType(itemTypes: string[]): Promise> { - const catalog = await catalogByType(); + const catalog = await loadCatalog(); const wanted = new Map(); for (const itemType of new Set(itemTypes)) { - const definition = catalog.get(itemType); + const definition = catalog.byType.get(itemType); if (definition) wanted.set(itemType, definition); } return wanted; } function toItemView(row: ItemDefinitionRow): ItemView { - const effect = asItemEffect(row.effect); - if (row.effect !== null && effect === null) { - // Readable but unrecognised: the item still renders, without whatever it does. - // Loud because the only writer is the seeder, so this means the stored shape and - // the code that reads it have diverged. - console.warn(`[inventory] item ${row.itemType} (${row.key}) has an unreadable effect payload`); - } return { itemType: row.itemType, key: row.key, category: row.category, slot: row.slot, rarity: row.rarity, - effect, + // Readable but unrecognised leaves the item rendering without whatever it does. + // `loadCatalog` records which rows those were, since this null cannot say. + effect: asItemEffect(row.effect), name: row.name, description: row.description, }; diff --git a/backend/tests/features/battle/ledger/accept.service.test.ts b/backend/tests/features/battle/ledger/accept.service.test.ts index 225f9519..f5d01c6c 100644 --- a/backend/tests/features/battle/ledger/accept.service.test.ts +++ b/backend/tests/features/battle/ledger/accept.service.test.ts @@ -60,6 +60,8 @@ import { acceptBattle, decodeStoredSnapshot } from '@features/battle/ledger'; import { chooseCommitmentRound, roundPublishTime } from '@features/battle/randomness'; import { activeSigningKey, sign, SignerRefusedError } from '@features/battle/signer'; import { consumeDailyBudget, findCoveringAuthorization } from '../../../../src/features/battle/ledger/consent.service'; +import { ItemCatalogError } from '@features/inventory'; +import { servedRuleset } from '../../../../src/features/battle/ledger/ruleset.builder'; import { buildPetSnapshot } from '../../../../src/features/battle/ledger/snapshot.builder'; import { applyTransition, openBattle } from '../../../../src/features/battle/ledger/transitions'; @@ -312,6 +314,47 @@ describe('opening the ledger', () => { }); }); +describe('a catalog that cannot price the battle', () => { + /** + * Refused, not fought (roadmap §4). Both reads that consult the catalog run before the + * first write, so this rejects with nothing stranded: no ledger row, no consumed + * intent, no spent daily budget. + */ + it('rejects when a pet wears something the catalog cannot price', async () => { + vi.mocked(buildPetSnapshot).mockRejectedValue( + new ItemCatalogError('pet 1 has uncatalogued item type 999 equipped in slot 0'), + ); + + expect(await acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW })).toMatchObject({ + ok: false, + reason: 'item-catalog-stale', + }); + expect(openBattle).not.toHaveBeenCalled(); + expect(sign).not.toHaveBeenCalled(); + }); + + it('rejects when the ruleset itself cannot be built', async () => { + vi.mocked(servedRuleset).mockRejectedValueOnce( + new ItemCatalogError('item 2 (bent_fang) is equipment with no readable stat_bonus'), + ); + + expect(await acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW })).toMatchObject({ + ok: false, + reason: 'item-catalog-stale', + }); + expect(openBattle).not.toHaveBeenCalled(); + }); + + it('lets any other failure through as a real error', async () => { + // A stale catalog is a recoverable operational state with an obvious remedy. A bug + // is not, and collapsing the two would turn every defect on this path into a + // routine 503 nobody investigates. + vi.mocked(servedRuleset).mockRejectedValueOnce(new Error('connection reset')); + + await expect(acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW })).rejects.toThrow('connection reset'); + }); +}); + describe('the stored snapshot survives a storage round trip', () => { /** * The property every worker downstream depends on: what acceptance persisted, read back diff --git a/backend/tests/features/battle/ledger/ruleset.builder.test.ts b/backend/tests/features/battle/ledger/ruleset.builder.test.ts index 9f806af4..60692617 100644 --- a/backend/tests/features/battle/ledger/ruleset.builder.test.ts +++ b/backend/tests/features/battle/ledger/ruleset.builder.test.ts @@ -1,7 +1,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const catalog = vi.fn(); -vi.mock('@features/inventory', () => ({ getCatalog: () => catalog() })); +// The catalog module is stubbed wholesale here, so no generation is ever bumped and +// `resetServedRuleset` is the seam these cases use. A fixed generation keeps the memo +// behaving as it does in production between seeder runs. +vi.mock('@features/inventory', () => ({ getCombatCatalog: () => catalog(), itemCatalogGeneration: () => 0 })); import { hashRuleset, SOURCE_DEFAULT_RULESET } from '@cryptopets/protocol'; diff --git a/backend/tests/features/battle/ledger/snapshot.builder.test.ts b/backend/tests/features/battle/ledger/snapshot.builder.test.ts index 2f2055c3..b78d5859 100644 --- a/backend/tests/features/battle/ledger/snapshot.builder.test.ts +++ b/backend/tests/features/battle/ledger/snapshot.builder.test.ts @@ -7,7 +7,10 @@ vi.mock('@config/env', () => ({ // Equipment resolution has its own coverage; stubbed to ungeared so these stay about // merging the roster with progression. vi.mock('@features/inventory', () => ({ - getPetEquipment: vi.fn(async () => []), + getPetEquipmentForCombat: vi.fn(async () => []), + // Real, because the builder is expected to let it through untouched and a stub class + // would make `rejects.toThrow(ItemCatalogError)` pass against any error at all. + ItemCatalogError: class ItemCatalogError extends Error {}, })); vi.mock('@config/prisma', () => ({ @@ -19,7 +22,7 @@ vi.mock('@config/prisma', () => ({ import { prisma } from '@config/prisma'; import { buildPetSnapshot } from '@features/battle/ledger'; -import { getPetEquipment } from '@features/inventory'; +import { getPetEquipmentForCombat, ItemCatalogError } from '@features/inventory'; const ROSTER_ROW = { chain: 'evm', @@ -239,26 +242,26 @@ describe('freezing equipment (roadmap §4)', () => { } as never); }); + // Already narrowed to the modifier by `getPetEquipmentForCombat`, which is also where + // an uncatalogued or unreadable item is refused outright. That refusal is covered in + // `inventory.service.test.ts`; by the time the builder sees a row it is priceable. const BLADE = { slot: 0, - item: { - itemType: '1', key: 'iron_fang', category: 'equipment', slot: 0, rarity: 1, - effect: { kind: 'stat_bonus' as const, hp: 0, atk: 4, def: 0, int: 0, mdef: 0 }, - name: 'Iron Fang', description: '', - }, + itemType: '1', + key: 'iron_fang', + bonus: { kind: 'stat_bonus' as const, hp: 0, atk: 4, def: 0, int: 0, mdef: 0 }, }; const PLATE = { slot: 1, - item: { - ...BLADE.item, itemType: '11', key: 'scale_mail', slot: 1, - effect: { kind: 'stat_bonus' as const, hp: 30, atk: 0, def: 10, int: 0, mdef: 0 }, - }, + itemType: '11', + key: 'scale_mail', + bonus: { kind: 'stat_bonus' as const, hp: 30, atk: 0, def: 10, int: 0, mdef: 0 }, }; // Resolved, not referenced: unequipping after acceptance must not change a committed // fight, exactly as a level-up between acceptance and settlement must not. it('freezes the resolved modifiers alongside the item type', async () => { - vi.mocked(getPetEquipment).mockResolvedValue([BLADE] as never); + vi.mocked(getPetEquipmentForCombat).mockResolvedValue([BLADE] as never); const snapshot = await buildPetSnapshot('eip155:84532', '1'); @@ -270,26 +273,26 @@ describe('freezing equipment (roadmap §4)', () => { // Slot order is part of the snapshot digest, and assertPetSnapshot refuses to sort // silently, so the builder has to hand it over already ordered. it('orders slots ascending whatever order the rows arrive in', async () => { - vi.mocked(getPetEquipment).mockResolvedValue([PLATE, BLADE] as never); + vi.mocked(getPetEquipmentForCombat).mockResolvedValue([PLATE, BLADE] as never); const snapshot = await buildPetSnapshot('eip155:84532', '1'); expect(snapshot!.equipment?.map((e) => e.slot)).toEqual([0, 1]); }); - // An entry claiming an item was worn and did nothing reads as a bug rather than a fact. - it('leaves out an equipped item with no combat effect', async () => { - vi.mocked(getPetEquipment).mockResolvedValue([ - { slot: 0, item: { ...BLADE.item, effect: null } }, - ] as never); + // Propagated, not caught: acceptance turns this into an `item-catalog-stale` rejection, + // and a snapshot builder that swallowed it would hand back a pet fighting bare while + // chain state says otherwise. + it('lets a catalog failure reach the caller', async () => { + vi.mocked(getPetEquipmentForCombat).mockRejectedValue(new ItemCatalogError('uncatalogued item type 99')); - expect((await buildPetSnapshot('eip155:84532', '1'))!.equipment).toBeUndefined(); + await expect(buildPetSnapshot('eip155:84532', '1')).rejects.toThrow(ItemCatalogError); }); // Omitted rather than empty, so an ungeared snapshot's stored JSON is identical to what // it was before equipment existed. it('omits the field entirely for an ungeared pet', async () => { - vi.mocked(getPetEquipment).mockResolvedValue([] as never); + vi.mocked(getPetEquipmentForCombat).mockResolvedValue([] as never); expect((await buildPetSnapshot('eip155:84532', '1'))!.equipment).toBeUndefined(); }); diff --git a/backend/tests/features/inventory/inventory.service.test.ts b/backend/tests/features/inventory/inventory.service.test.ts index a5e56bc9..7f67b198 100644 --- a/backend/tests/features/inventory/inventory.service.test.ts +++ b/backend/tests/features/inventory/inventory.service.test.ts @@ -14,7 +14,17 @@ vi.mock('@repositories/inventory.repository', () => ({ findUnclaimedEntitlements: (chain: string, owner: string) => repo.findUnclaimedEntitlements(chain, owner), })); -import { getInventory, getPendingItems, getPetEquipment, resetItemCatalog } from '@features/inventory'; +import { + getCatalog, + getCombatCatalog, + getInventory, + getPendingItems, + getPetEquipment, + getPetEquipmentForCombat, + ItemCatalogError, + itemCatalogGeneration, + resetItemCatalog, +} from '@features/inventory'; const POTION = { itemType: '100', @@ -134,6 +144,110 @@ describe('getPetEquipment', () => { }); }); +/** + * The strict counterparts (roadmap §4). + * + * `getPetEquipment` and `getCatalog` hide a row they cannot read, which is right for a bag + * and wrong for a fight: dropping an item silently changes a battle rather than a label, + * and the resulting receipt claims a pet fought bare while `ItemCore.equipmentOf` at the + * recorded `sourceVersion` says it was wearing something. + * + * An unreadable effect and an absent one are the same `null` on `ItemView`, so each case + * below is checked against a lenient read as well, to show the two paths genuinely differ + * rather than the fixture simply being malformed everywhere. + */ +describe('the combat reads refuse what the display reads hide', () => { + /** Equipment whose stored effect will not parse: `atk` is a string, not an integer. */ + const CORRUPT_BLADE = { ...BLADE, itemType: '2', key: 'bent_fang', effect: { kind: 'stat_bonus', hp: 0, atk: '4', def: 0, int: 0, mdef: 0 } }; + + describe('getCombatCatalog', () => { + it('returns the catalog when every equipment row is readable', async () => { + repo.findAllDefinitions.mockResolvedValue([BLADE, POTION]); + + expect((await getCombatCatalog()).map((item) => item.key)).toEqual(['iron_fang', 'xp_potion_i']); + }); + + it('refuses an equipment row whose modifier will not parse', async () => { + repo.findAllDefinitions.mockResolvedValue([BLADE, CORRUPT_BLADE]); + + await expect(getCombatCatalog()).rejects.toThrow(ItemCatalogError); + // The lenient read still serves it, effect dropped. That difference is the + // point: a bad row costs a tooltip on the bag screen and costs a battle here. + expect((await getCatalog()).find((item) => item.key === 'bent_fang')?.effect).toBeNull(); + }); + + it('ignores an unreadable effect on something that cannot reach a fight', async () => { + // A consumable's effect is applied by `useItem`, never by the engine, so it has + // no business invalidating the ruleset every battle is priced under. + repo.findAllDefinitions.mockResolvedValue([BLADE, { ...POTION, effect: { kind: 'grant_xp', amount: 'fifty' } }]); + + await expect(getCombatCatalog()).resolves.toHaveLength(2); + }); + }); + + describe('getPetEquipmentForCombat', () => { + it('narrows a readable item to its modifier', async () => { + repo.findEquipment.mockResolvedValue([{ slot: 0, itemType: '1' }]); + repo.findAllDefinitions.mockResolvedValue([BLADE]); + + expect(await getPetEquipmentForCombat('evm', '7')).toEqual([ + { slot: 0, itemType: '1', key: 'iron_fang', bonus: { kind: 'stat_bonus', hp: 0, atk: 4, def: 0, int: 0, mdef: 0 } }, + ]); + }); + + it('refuses an equipped item with no catalog row', async () => { + // The seeder running behind the contract. Refusing surfaces it in seconds; the + // lenient read hides it behind a console warning and an ungeared fight. + repo.findEquipment.mockResolvedValue([{ slot: 0, itemType: '999' }]); + repo.findAllDefinitions.mockResolvedValue([BLADE]); + + await expect(getPetEquipmentForCombat('evm', '7')).rejects.toThrow(/uncatalogued item type 999/); + expect(await getPetEquipment('evm', '7')).toEqual([]); + }); + + it('refuses an equipped item whose modifier will not parse', async () => { + repo.findEquipment.mockResolvedValue([{ slot: 0, itemType: '2' }]); + repo.findAllDefinitions.mockResolvedValue([CORRUPT_BLADE]); + + await expect(getPetEquipmentForCombat('evm', '7')).rejects.toThrow(/no readable stat_bonus/); + }); + + it('costs nothing for a pet with no gear', async () => { + repo.findEquipment.mockResolvedValue([]); + + expect(await getPetEquipmentForCombat('evm', '7')).toEqual([]); + expect(repo.findAllDefinitions).not.toHaveBeenCalled(); + }); + }); +}); + +describe('resetItemCatalog', () => { + // The contract `servedRuleset` memoizes against. It cannot call this module's reset + // directly (ruleset.builder imports this one, so the call would close a cycle), so it + // compares generations instead, and a reset that did not bump one would leave a ruleset + // built from rows that no longer exist. + it('bumps the generation so catalog-derived caches rebuild', async () => { + repo.findAllDefinitions.mockResolvedValue([BLADE]); + await getCatalog(); + + const before = itemCatalogGeneration(); + resetItemCatalog(); + + expect(itemCatalogGeneration()).not.toBe(before); + }); + + it('re-reads the definitions after a reset', async () => { + repo.findAllDefinitions.mockResolvedValue([BLADE]); + await getCatalog(); + await getCatalog(); + expect(repo.findAllDefinitions).toHaveBeenCalledTimes(1); + + resetItemCatalog(); + await getCatalog(); + expect(repo.findAllDefinitions).toHaveBeenCalledTimes(2); + }); +}); + describe('getPendingItems', () => { const row = { id: 'e1', diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index 437d21e3..db703e34 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -112,7 +112,7 @@ as authoritative: Both happen from one malformed JSON column, with a `console.warn` as the only signal. -- [ ] **C1.1** Split the read. Keep `asItemEffect`'s leniency on the display path (a bag with one +- [x] **C1.1** Split the read. Keep `asItemEffect`'s leniency on the display path (a bag with one unnamed tile beats a bag that will not open) and make the combat path strict: a `stat_bonus` row that fails to parse throws from `servedRuleset()` and from `resolveEquipment`. A deployment that cannot state its own rules should refuse to accept @@ -130,7 +130,7 @@ The receipt then says ungeared while `ItemCore.equipmentOf(petId)` at the record snapshot to enable, reporting a discrepancy an outsider cannot distinguish from operator misbehaviour. -- [ ] **C2.1** Reject the acceptance instead. An uncatalogued equipped item means the seeder is +- [x] **C2.1** Reject the acceptance instead. An uncatalogued equipped item means the seeder is behind the contract, which is an operational fault; failing the accept with a named reason surfaces it in seconds, where a silent ungeared fight surfaces as an unexplained verifier failure weeks later. Reuse the existing reject path in `accept.service.ts`. @@ -151,12 +151,21 @@ not check" rather than "disagreed", so the battle retries and dead-letters. Unreachable with shipped content: `MAX_STAT_BONUS` is 500, three slots cap a pet at 1500, and the shipped catalog's largest single bonus is 45 HP. This is a guardrail, not a bug. -- [ ] **C3.1** Clamp in `sumBonuses` to match the Go port, and add the case to +- [x] **C3.1** Clamp in `sumBonuses` to match the Go port, and add the case to `contracts/test-vectors/equipment.json` so the two stay pinned. Both live ports change in the same commit, per `AGENTS.md`. Verify: `pnpm --filter @cryptopets/protocol test && go test ./internal/combat` from `services/indexer-go`. + **Done without the vector case, deliberately.** The vector format hands `simulate` one + already-summed `bonus1`/`bonus2` per pet, so no case in `equipment.json` reaches + `sumBonuses` at all; pinning it there would have meant extending the vector schema to + carry item lists. Both ports already pin this function with a unit test beside their + golden tests (order-independence), so the clamp went there too: + `equipmentVectors.test.ts`'s `saturates at 65535` and `equipment_golden_test.go`'s + `TestSumBonusesSaturates` assert the same thing on both sides. `equipment.json` is + untouched. + --- ## D1 (decision, not a fix): consent bounds level, gear is unbounded @@ -205,11 +214,11 @@ deliberately. If D1 lands as option 1, fold it into the same rollout and pay thi Small, none of them urgent. -- [ ] **Q1 Two caches, one reset each.** `inventory.service.ts:205` caches the catalog for the +- [x] **Q1 Two caches, one reset each.** `inventory.service.ts:205` caches the catalog for the process's life and `ruleset.builder.ts:30` caches a ruleset derived from it. Their reset seams are separate (`resetItemCatalog`, `resetServedRuleset`), so clearing one leaves the other holding data built from what was just dropped. Have `resetItemCatalog` clear both. -- [ ] **Q2 Orphaned doc comment.** `env.ts:141-146` documents `adminWallets` directly above the +- [x] **Q2 Orphaned doc comment.** `env.ts:141-146` documents `adminWallets` directly above the comment for `dropsEnabled`; the field itself is at line 156. Move the comment to its field. - [x] **Q3 `verify.worker.ts:60-62` casts to `Record`** to read a shape the codec from B1.1 will type properly. Folded into B1.1 rather than done twice. diff --git a/protocol/src/combat/equipment.ts b/protocol/src/combat/equipment.ts index 13b55365..95e7c115 100644 --- a/protocol/src/combat/equipment.ts +++ b/protocol/src/combat/equipment.ts @@ -24,6 +24,7 @@ export interface AttrBonus { export const NO_BONUS: AttrBonus = { hp: 0, atk: 0, def: 0, int: 0, mdef: 0 }; const U16_MAX = 65535n; +const U16_MAX_NUMBER = 65535; /** * Adds a bonus to extracted attributes, in place. @@ -52,15 +53,27 @@ export function applyBonus(attrs: Attrs, bonus: AttrBonus): void { * * Order-independent by construction: addition commutes, so the caller does not have to * sort, unlike the snapshot encoding where order is part of the digest. + * + * Saturating at each step, matching the Go port's `SumBonuses` exactly. The final + * attribute is the same either way, since `applyBonus` clamps at the same ceiling and + * attributes are non-negative, so this is not what keeps the two engines agreeing. + * + * What it keeps working is the wire between them. §F sends this total to indexer-go as a + * `uint32`, and `bonusFromProto` range-checks it rather than truncating: a total past + * 65535 came back as an RPC error, which `verify.worker` correctly reads as "could not + * check" rather than "disagreed", so the battle retried until it dead-lettered. Clamping + * here means a value that cannot change the outcome cannot stall the pipeline either. + * Unreachable with shipped content (`MAX_STAT_BONUS` is 500 across three slots), which is + * why it is a guardrail rather than a fix. */ export function sumBonuses(items: readonly AttrBonus[]): AttrBonus { const total: AttrBonus = { ...NO_BONUS }; for (const item of items) { - total.hp += item.hp; - total.atk += item.atk; - total.def += item.def; - total.int += item.int; - total.mdef += item.mdef; + total.hp = addClamped(total.hp, item.hp); + total.atk = addClamped(total.atk, item.atk); + total.def = addClamped(total.def, item.def); + total.int = addClamped(total.int, item.int); + total.mdef = addClamped(total.mdef, item.mdef); } return total; } @@ -88,3 +101,9 @@ export function bonusFromEquipment(items: readonly AttrBonus[] | undefined): Att function clampU16(value: bigint): bigint { return value > U16_MAX ? U16_MAX : value; } + +/** Mirrors the Go port's `addClamped`: saturate at the ceiling rather than run past it. */ +function addClamped(a: number, b: number): number { + const sum = a + b; + return sum > U16_MAX_NUMBER ? U16_MAX_NUMBER : sum; +} diff --git a/protocol/tests/combat/equipmentVectors.test.ts b/protocol/tests/combat/equipmentVectors.test.ts index 58c99f61..6a0447fe 100644 --- a/protocol/tests/combat/equipmentVectors.test.ts +++ b/protocol/tests/combat/equipmentVectors.test.ts @@ -115,6 +115,37 @@ describe('properties the vectors exist to pin', () => { ]; expect(sumBonuses(parts)).toEqual(sumBonuses([...parts].reverse())); }); + + /** + * Saturates rather than running past the ceiling, matching the Go port step for step + * (`TestSumBonusesSaturates`). + * + * Not a vector case, and the reason is worth stating: the vectors hand `simulate` one + * already-summed bonus per pet, so nothing in that file reaches this function. The Go + * counterpart is a unit test beside its own order-independence test, for the same + * reason, and these two are the parity check for the summation itself. + * + * The final attribute would be identical without the clamp, since `applyBonus` clamps + * at the same ceiling. What the clamp protects is the §F wire: an unclamped total was + * range-rejected by `bonusFromProto`, which surfaced as "verification unavailable" and + * retried the battle to a dead letter. + */ + it('saturates at 65535 rather than running past it', () => { + const huge: AttrBonus = { hp: 40000, atk: 40000, def: 40000, int: 40000, mdef: 40000 }; + expect(sumBonuses([huge, huge])).toEqual({ hp: 65535, atk: 65535, def: 65535, int: 65535, mdef: 65535 }); + }); + + it('saturates identically however many items it takes to get there', () => { + // Order independence has to survive the clamp: saturating early must not make the + // total depend on which item pushed it over. + const parts: AttrBonus[] = [ + { hp: 60000, atk: 0, def: 0, int: 0, mdef: 0 }, + { hp: 10000, atk: 0, def: 0, int: 0, mdef: 0 }, + { hp: 1, atk: 0, def: 0, int: 0, mdef: 0 }, + ]; + expect(sumBonuses(parts)).toEqual(sumBonuses([...parts].reverse())); + expect(sumBonuses(parts).hp).toBe(65535); + }); }); describe('bonusFromEquipment', () => { diff --git a/services/indexer-go/internal/combat/equipment_golden_test.go b/services/indexer-go/internal/combat/equipment_golden_test.go index b6aeb54f..69dfb73b 100644 --- a/services/indexer-go/internal/combat/equipment_golden_test.go +++ b/services/indexer-go/internal/combat/equipment_golden_test.go @@ -230,3 +230,30 @@ func TestSumBonusesIsOrderIndependent(t *testing.T) { t.Errorf("sum depends on order: %+v vs %+v", SumBonuses(parts), SumBonuses(reversed)) } } + +// TestSumBonusesSaturates pins the ceiling, and is the Go half of a parity pair: the +// TypeScript port asserts the identical thing in equipmentVectors.test.ts. +// +// Not a vector case, deliberately. equipment.json hands Simulate one already-summed bonus +// per pet, so no case in that file reaches this function; a unit test on each side is what +// holds the two summations together. +// +// This port has always saturated. The TypeScript one summed unclamped, which produced a +// total that bonusFromProto range-rejects at the gRPC boundary, turning a battle nothing +// could have changed the outcome of into a retry loop and a dead letter. +func TestSumBonusesSaturates(t *testing.T) { + huge := AttrBonus{HP: 40000, ATK: 40000, DEF: 40000, INT: 40000, MDEF: 40000} + want := AttrBonus{HP: 65535, ATK: 65535, DEF: 65535, INT: 65535, MDEF: 65535} + + if got := SumBonuses([]AttrBonus{huge, huge}); got != want { + t.Errorf("sum did not saturate: got %+v, want %+v", got, want) + } + + // Order independence has to survive the clamp: saturating early must not make the + // total depend on which item pushed it over. + parts := []AttrBonus{{HP: 60000}, {HP: 10000}, {HP: 1}} + reversed := []AttrBonus{parts[2], parts[1], parts[0]} + if SumBonuses(parts) != SumBonuses(reversed) || SumBonuses(parts).HP != 65535 { + t.Errorf("saturated sum depends on order: %+v vs %+v", SumBonuses(parts), SumBonuses(reversed)) + } +} From d44c879f648af28ba4e5c036f4a84e23d63c60c4 Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 14:37:01 -0400 Subject: [PATCH 03/36] feat(protocol): refuse gear the ruleset does not price, at acceptance --- backend/scripts/verify-inventory-setup.ts | 14 +++ .../battle/ledger/accept.controller.ts | 1 + .../features/battle/ledger/accept.service.ts | 33 +++++- .../battle/ledger/accept.service.test.ts | 77 +++++++++++++- docs/plan-battle-inventory-hardening.md | 100 +++++++++++++----- protocol/src/ruleset/equipmentCheck.ts | 69 ++++++++++++ protocol/src/ruleset/index.ts | 1 + protocol/tests/ruleset/equipmentCheck.test.ts | 92 ++++++++++++++++ verifier/src/checks/equipment.ts | 55 +++------- 9 files changed, 375 insertions(+), 67 deletions(-) create mode 100644 protocol/src/ruleset/equipmentCheck.ts create mode 100644 protocol/tests/ruleset/equipmentCheck.test.ts diff --git a/backend/scripts/verify-inventory-setup.ts b/backend/scripts/verify-inventory-setup.ts index d4a84739..f7909c36 100644 --- a/backend/scripts/verify-inventory-setup.ts +++ b/backend/scripts/verify-inventory-setup.ts @@ -20,6 +20,7 @@ import 'dotenv/config'; import { prisma } from '../src/config/prisma'; import { assertCatalog, SLOT } from '../src/features/inventory/catalog'; import { ITEM_CATALOG } from '../src/features/inventory/catalog.data'; +import { getCombatCatalog } from '../src/features/inventory/inventory.service'; const ITEM_CORE_ABI = [ { @@ -120,6 +121,19 @@ async function checkCatalog(): Promise { drifted.length === 0, drifted.length === 0 ? 'no drift' : `drifted: ${drifted.map((i) => i.key).join(', ')}`, ); + + // The stored rows have to be readable as *rules*, not merely present. Since roadmap §4 + // an equipment row whose modifier will not parse is refused rather than skipped, so one + // bad `effect` column stops every battle this deployment accepts with + // `item-catalog-stale`. The seeder cannot produce that state (`assertCatalog` rejects it + // at authoring), which is exactly why it is worth checking here: it means someone edited + // the table by hand, and that is invisible to every other check above. + try { + const priced = await getCombatCatalog(); + record('catalog can price a fight', true, `${priced.length} definitions readable`); + } catch (error) { + record('catalog can price a fight', false, `${(error as Error).message} — every accept will 503`); + } } async function checkChain(): Promise { diff --git a/backend/src/features/battle/ledger/accept.controller.ts b/backend/src/features/battle/ledger/accept.controller.ts index aa5948dd..e7520c0d 100644 --- a/backend/src/features/battle/ledger/accept.controller.ts +++ b/backend/src/features/battle/ledger/accept.controller.ts @@ -35,6 +35,7 @@ const STATUS_BY_REASON: Record = { 'drand-unavailable': 503, 'signer-unavailable': 503, 'item-catalog-stale': 503, + 'equipment-catalog-mismatch': 503, }; interface AcceptBody { diff --git a/backend/src/features/battle/ledger/accept.service.ts b/backend/src/features/battle/ledger/accept.service.ts index 3dee9163..6e3f6146 100644 --- a/backend/src/features/battle/ledger/accept.service.ts +++ b/backend/src/features/battle/ledger/accept.service.ts @@ -4,6 +4,7 @@ import { type BattleCommitment, type BattleSnapshot, type ChainId, + findEquipmentMismatches, hashBattleSnapshot, hashRuleset, isBattleReady, @@ -87,7 +88,16 @@ export type AcceptRejection = * end: the alternative is a fight under rules this deployment cannot state, recorded in * a signed receipt that contradicts chain state. */ - | 'item-catalog-stale'; + | 'item-catalog-stale' + /** + * The frozen gear disagrees with what the ruleset this battle names prices it at + * (roadmap §4, threat T13). Reachable when the catalog changes between resolving the + * snapshot and building the ruleset, and otherwise a bug. + * + * Refused rather than fought, because the verifier makes the same comparison on the + * finished receipt: accepting would produce a battle guaranteed to fail verification. + */ + | 'equipment-catalog-mismatch'; export interface AcceptedBattle { battleId: string; @@ -149,6 +159,27 @@ export async function acceptBattle(request: AcceptBattleRequest): Promise 0) { + return reject('equipment-catalog-mismatch', mismatches.join('; ')); + } + const rulesetHash = hashRuleset(ruleset); await ensureRulesetPublished(rulesetHash); diff --git a/backend/tests/features/battle/ledger/accept.service.test.ts b/backend/tests/features/battle/ledger/accept.service.test.ts index f5d01c6c..cbc224e9 100644 --- a/backend/tests/features/battle/ledger/accept.service.test.ts +++ b/backend/tests/features/battle/ledger/accept.service.test.ts @@ -1,6 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { hashBattleSnapshot, QUICKNET, roundTime } from '@cryptopets/protocol'; +import { hashBattleSnapshot, QUICKNET, roundTime, SOURCE_DEFAULT_RULESET } from '@cryptopets/protocol'; vi.mock('@config/env', () => ({ env: { battle: { deploymentId: 'base-sepolia-live', chainIds: ['eip155:84532'] } }, @@ -355,6 +355,74 @@ describe('a catalog that cannot price the battle', () => { }); }); +describe('gear the ruleset does not price', () => { + /** + * The same comparison the verifier runs on the finished receipt, made at acceptance so + * a battle guaranteed to fail verification is never accepted (roadmap §4, threat T13). + * + * The reachable cause is narrow: `buildPetSnapshot` resolves the modifiers and + * `servedRuleset` publishes them, and those are two reads of the item catalog at + * different points, so a seeder run landing between them prices the fight from one + * catalog and the rules from another. + */ + const WORN = { slot: 0, itemType: 3n, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }; + + function wearing(entry: typeof WORN) { + vi.mocked(buildPetSnapshot).mockImplementation((async (_chainId: string, petId: string) => + petId === '1' ? { ...ATTACKER, equipment: [entry] } : DEFENDER) as never); + } + + function pricing(item: { itemType: bigint; slot: number; hp: number; atk: number; def: number; int: number; mdef: number }) { + vi.mocked(servedRuleset).mockResolvedValueOnce({ + ...SOURCE_DEFAULT_RULESET, + itemCatalog: [item], + } as never); + } + + it('accepts when the worn modifiers are what the catalog declares', async () => { + wearing(WORN); + pricing({ itemType: 3n, slot: 0, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }); + + expect(await acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW })).toMatchObject({ ok: true }); + }); + + it('rejects an inflated modifier', async () => { + // The attack the check exists for: a fight given +50 ATK from a 22-ATK sword + // replays perfectly, because the inflated number is the thing being replayed. + wearing({ ...WORN, atk: 50 }); + pricing({ itemType: 3n, slot: 0, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }); + + expect(await acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW })).toMatchObject({ + ok: false, + reason: 'equipment-catalog-mismatch', + detail: expect.stringContaining('atk applied 50, catalog declares 22'), + }); + expect(openBattle).not.toHaveBeenCalled(); + }); + + it('rejects an item the ruleset never priced', async () => { + wearing(WORN); + pricing({ itemType: 999n, slot: 0, hp: 0, atk: 1, def: 0, int: 0, mdef: 0 }); + + expect(await acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW })).toMatchObject({ + ok: false, + reason: 'equipment-catalog-mismatch', + }); + }); + + it('refuses before consuming the defender daily budget', async () => { + // Ordering matters as much as the refusal: a rejected battle must not spend a use + // of someone's cap, and this check sits ahead of every write for that reason. + wearing({ ...WORN, atk: 50 }); + pricing({ itemType: 3n, slot: 0, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }); + + await acceptBattle({ intentHash: INTENT.intentHash, nowSeconds: NOW }); + + expect(consumeDailyBudget).not.toHaveBeenCalled(); + expect(sign).not.toHaveBeenCalled(); + }); +}); + describe('the stored snapshot survives a storage round trip', () => { /** * The property every worker downstream depends on: what acceptance persisted, read back @@ -385,6 +453,13 @@ describe('the stored snapshot survives a storage round trip', () => { petId === '1' ? { ...ATTACKER, equipment: [{ slot: 0, itemType: 3n, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }] } : DEFENDER) as never); + // The ruleset has to price the sword, or acceptance now refuses the battle before + // it ever reaches `openBattle` — which is the catalog cross-check above doing its + // job, not a problem with this case. + vi.mocked(servedRuleset).mockResolvedValueOnce({ + ...SOURCE_DEFAULT_RULESET, + itemCatalog: [{ itemType: 3n, slot: 0, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }], + } as never); const ledger = await storedLedger(); expect(hashBattleSnapshot(decodeStoredSnapshot(ledger.snapshot))).toBe(ledger.snapshotHash); diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index db703e34..472502ab 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -168,29 +168,53 @@ the shipped catalog's largest single bonus is 45 HP. This is a guardrail, not a --- -## D1 (decision, not a fix): consent bounds level, gear is unbounded - -`DefenseAuthorization` covers pet, attacker level band, ruleset hash, validity window and daily -cap. Phase 4 made equipment a combat input without adding it to that list. A defender who -authorizes a level 10 to 14 attacker gets whatever that attacker equips afterwards, and the -snapshot is taken at accept, after consent. - -Sized honestly: the shipped catalog tops out near +22 ATK against attributes in the low -hundreds, so today this is a tuning matter rather than an exploit. But `MAX_STAT_BONUS` permits -500 a stat, and the level band is the only power bound the defender was given. - -Options, in the order I would take them: - -1. **Bound it in the ruleset.** Add a per-fight modifier cap to `Ruleset`, so the band the - defender consents to implies a power ceiling. Costs a ruleset schema bump and a re-consent - event, which item D3 below already requires once. -2. **Put a gear digest in the authorization.** Strictly correct and much worse to use: the - defender re-consents every time an attacker changes a sword. -3. **Accept it and write it down.** Defensible while the catalog stays modest. Needs a stated - ceiling in `catalog.ts` that a content edit cannot quietly raise. - -- [ ] **D1.1** Pick one. This is a game-design call, not an engineering one, and per CLAUDE.md - it does not get decided in a loop. +## D1: consent already bounds gear. The gap was smaller than stated + +**The framing above this line was wrong, and the correction is the useful part.** Written out +because it was believed long enough to nearly justify a permanent ruleset schema version. + +The original claim was that `DefenseAuthorization` bounds the attacker's level but not their +gear, leaving a defender exposed to whatever the attacker equips after consenting. Two things +already in the code say otherwise: + +- **`itemCatalog` is inside `rulesetHash`** (ruleset schema v2), and consent is bound to that + hash. So a defender has consented to the exact set of items and their exact effects, + including the strongest loadout that set can express. Shipping a stronger sword moves the + hash and re-consents everyone. That is the mechanism §4 designed, working. +- **`verifier/src/checks/equipment.ts` already enforces it**, comparing every resolved + modifier in the snapshot against what the ruleset declares, per item and per slot. + +So gear is bounded, the bound is signed, and the modifiers are checked against it. What +actually remained was narrower: + +1. The ceiling is *derivable* (compute best-in-slot across the catalog) rather than legible as + a single number a defender could read. +2. The catalog comparison happened only at verification, so a disagreeing snapshot became a + failed receipt rather than a refused battle. + +A `Ruleset.maxEquipmentBonus` field would have bought mostly (1), at the price of a permanent +entry in `SUPPORTED_VERSIONS` and a second re-consent event. Not proportionate. + +- [x] **D1.1 Make the comparison at acceptance, with no schema change.** `findEquipmentMismatches` + moved into `@cryptopets/protocol` (`ruleset/equipmentCheck.ts`) and now has two callers: + the verifier, reporting on a finished receipt, and `accept.service.ts`, refusing a battle + that would be guaranteed to fail that report. One implementation, because two would drift + into a battle that accepts and then fails to verify, with the comparison itself the last + thing anyone would suspect. New rejection: `equipment-catalog-mismatch` (503). + + This is not merely redundant with the verifier. `buildPetSnapshot` resolves the modifiers + and `servedRuleset` publishes them, and those are two reads of the item catalog at + different points in one accept, so a seeder run landing between them prices the fight + from one catalog and the rules from another. Narrow, unreachable by an attacker, and + invisible to every other check. + + Verify: `pnpm --filter @cryptopets/protocol test && pnpm --filter @cryptopets/verifier test + && pnpm --filter backend test`. + +Left open deliberately: `MAX_STAT_BONUS` is still 500 a stat against attributes in the low +hundreds, where the largest shipped bonus is 45. Lowering it is a balance call, and raising it +later widens the ceiling every outstanding authorization implies. Worth a line in `catalog.ts` +saying so. ## D2: drops are outside the signed payload @@ -223,11 +247,31 @@ Small, none of them urgent. - [x] **Q3 `verify.worker.ts:60-62` casts to `Record`** to read a shape the codec from B1.1 will type properly. Folded into B1.1 rather than done twice. +### Flagged, not fixed: `backend/scripts/` is not typechecked + +`backend/tsconfig.json` includes `src/**/*` only, so nothing typechecks the operator scripts, +and three of them do not compile today. `grant-defense-authorization.ts` has a `ChainId` cast +and a readonly-vs-mutable `TypedDataField[]` mismatch; `seed-item-catalog.ts` cannot assign a +nullable `effect` to Prisma's `InputJsonValue`. All pre-existing and unrelated to this branch +(none of these files, nor anything they import, is in its diff). They still *run*, since `tsx` +strips types rather than checking them. + +Left alone deliberately, per CLAUDE.md's surgical-changes rule, but worth its own branch: these +are exactly the files an operator runs against production, and they are the only TypeScript in +the repo with no compiler watching them. + ## Operational, unblocked by code Carried over from `plan-inventory-items.md`'s "still outstanding", still outstanding. All three are operator calls. +Prepared ahead of them: the migration SQL was reviewed (RLS on all four tables, no `FORCE`, +matching the posture every other table has), and `verify-inventory-setup.ts` gained a +`catalog can price a fight` check. That one exists because C1 turned an unreadable equipment +row into a hard refusal, so a bad `effect` column now stops every accept with +`item-catalog-stale`. The seeder cannot produce that state, which is why nothing else in the +preflight would have caught it. + - [ ] **O1 Apply the migration.** `20260807160000_add_inventory` has never run. RLS is correctly present on all four new tables (`migration.sql:78-81`). `pnpm --filter backend prisma:migrate`, which is `migrate deploy`, never `dev`. @@ -245,9 +289,13 @@ are operator calls. ## Order B1 first and alone: nothing settles until it lands, so every other check runs against a stalled -pipeline. Then C1 and C2 together (one theme, adjacent code). C3 with its vector case. D1 needs -an answer before D3 is scheduled, since they should ship as one re-consent. O1 to O3 last, -because they are the only steps that touch production. +pipeline. Then C1 and C2 together (one theme, adjacent code), then C3. D1 turned out to need no +schema change, so it no longer has to be sequenced against D3's re-consent; D3 is still a +one-time cost that Phase 4 forces on its own. O1 to O3 last, because they are the only steps +that touch production. + +Everything above D2 is done. What remains is D2 (a tracked v1 limit, not a defect), D3 (ship +the re-consent deliberately), and the three operator steps. ## Do not touch diff --git a/protocol/src/ruleset/equipmentCheck.ts b/protocol/src/ruleset/equipmentCheck.ts new file mode 100644 index 00000000..19ef392c --- /dev/null +++ b/protocol/src/ruleset/equipmentCheck.ts @@ -0,0 +1,69 @@ +import type { EquipEntry } from '../snapshot/types'; + +import type { Ruleset } from './types'; + +/** + * Holds frozen equipment to what the ruleset's catalog declares (roadmap §4, threat T13). + * + * A combat replay proves a fight followed from the numbers in the snapshot. It cannot + * prove those numbers were the right ones: a snapshot granting +50 ATK from a 4-ATK dagger + * replays perfectly, because the inflated bonus is the very thing being replayed against. + * Self-consistent is not the same as honest, and this is what closes the difference. + * + * Two fields exist for no other purpose. The snapshot records each item's `itemType` + * beside its resolved bonus, and the ruleset publishes what every combat-affecting item + * does, so the declared effect and the applied one can be compared by anyone, from the + * receipt and its bundle alone. + * + * Lives here rather than in the verifier because it now has two callers with very + * different jobs: the verifier reporting on a receipt after the fact, and the backend + * refusing a battle before it starts. Two implementations of one comparison would diverge, + * and the symptom would be a battle that accepts and then fails to verify, with the + * comparison itself the last thing anyone suspects. + * + * What it does not prove is that the pet *owned* the item. That is a claim about chain + * state at `sourceVersion`, which this package deliberately cannot read; a caller wanting + * it checks `ItemCore.equipmentOf` at the recorded version itself. + */ +export interface EquipmentBearer { + /** Label used in the mismatch text, e.g. `attacker`. */ + role: string; + /** + * Spelled `| undefined` as well as optional, because this package builds under + * `exactOptionalPropertyTypes`: both callers read the field straight off a + * `PetSnapshot`, where an ungeared pet has it present and undefined rather than absent. + */ + equipment?: readonly EquipEntry[] | undefined; +} + +/** Every disagreement between what was worn and what the ruleset prices, as prose. */ +export function findEquipmentMismatches(bearers: readonly EquipmentBearer[], ruleset: Ruleset): string[] { + const declared = new Map((ruleset.itemCatalog ?? []).map((item) => [item.itemType, item])); + const mismatches: string[] = []; + + for (const { role, equipment } of bearers) { + for (const entry of equipment ?? []) { + const item = declared.get(entry.itemType); + if (!item) { + // An item the ruleset never priced. The fight used a modifier from + // nowhere, which is unauditable rather than merely unusual. + mismatches.push(`${role} slot ${entry.slot}: item ${entry.itemType} is not in the ruleset's catalog`); + continue; + } + if (item.slot !== entry.slot) { + mismatches.push( + `${role} item ${entry.itemType}: worn in slot ${entry.slot}, catalog says slot ${item.slot}`, + ); + } + for (const field of ['hp', 'atk', 'def', 'int', 'mdef'] as const) { + if (entry[field] !== item[field]) { + mismatches.push( + `${role} item ${entry.itemType}: ${field} applied ${entry[field]}, catalog declares ${item[field]}`, + ); + } + } + } + } + + return mismatches; +} diff --git a/protocol/src/ruleset/index.ts b/protocol/src/ruleset/index.ts index 6e75449c..04eb7549 100644 --- a/protocol/src/ruleset/index.ts +++ b/protocol/src/ruleset/index.ts @@ -1,4 +1,5 @@ export { loadRulesetBundle, parseRulesetBundle, publishRuleset, serializeRuleset } from './bundle'; +export { type EquipmentBearer, findEquipmentMismatches } from './equipmentCheck'; export { assertRulesetHash, encodeRuleset, hashRuleset } from './hash'; export { type ItemModifier, diff --git a/protocol/tests/ruleset/equipmentCheck.test.ts b/protocol/tests/ruleset/equipmentCheck.test.ts new file mode 100644 index 00000000..2f5bfc59 --- /dev/null +++ b/protocol/tests/ruleset/equipmentCheck.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it } from 'vitest'; + +import { findEquipmentMismatches } from '../../src/ruleset/equipmentCheck'; +import { SOURCE_DEFAULT_RULESET, type Ruleset } from '../../src/ruleset/types'; + +/** + * The comparison that holds a snapshot's frozen modifiers to the ruleset that priced them + * (roadmap §4, threat T13). + * + * Tested here rather than only through its callers because it now has two, and they use it + * for opposite purposes: the verifier reports on a finished receipt, the backend refuses a + * battle before it starts. Both have to reach the same verdict on the same inputs, so the + * verdict belongs in one place with its own tests. + */ + +const SWORD = { itemType: 3n, slot: 0, hp: 0, atk: 22, def: 0, int: 0, mdef: 0 }; +const PLATE = { itemType: 12n, slot: 1, hp: 45, atk: 0, def: 16, int: 0, mdef: 6 }; + +const ruleset: Ruleset = { ...SOURCE_DEFAULT_RULESET, itemCatalog: [SWORD, PLATE] }; + +/** One worn item, in the shape a snapshot carries it. */ +const worn = (item: typeof SWORD) => ({ slot: item.slot, itemType: item.itemType, hp: item.hp, atk: item.atk, def: item.def, int: item.int, mdef: item.mdef }); + +describe('findEquipmentMismatches', () => { + it('accepts gear priced exactly as the catalog declares', () => { + expect( + findEquipmentMismatches([{ role: 'attacker', equipment: [worn(SWORD), worn(PLATE)] }], ruleset), + ).toEqual([]); + }); + + it('accepts an ungeared pet, however the absence is spelled', () => { + expect(findEquipmentMismatches([{ role: 'attacker' }], ruleset)).toEqual([]); + expect(findEquipmentMismatches([{ role: 'attacker', equipment: undefined }], ruleset)).toEqual([]); + expect(findEquipmentMismatches([{ role: 'attacker', equipment: [] }], ruleset)).toEqual([]); + }); + + // The attack this exists for. An inflated bonus replays perfectly, because it is the + // very number being replayed against, so replay alone can never catch it. + it('catches an inflated modifier and names both numbers', () => { + const [mismatch] = findEquipmentMismatches( + [{ role: 'attacker', equipment: [{ ...worn(SWORD), atk: 50 }] }], + ruleset, + ); + + expect(mismatch).toBe('attacker item 3: atk applied 50, catalog declares 22'); + }); + + it('catches an item the ruleset never priced', () => { + const [mismatch] = findEquipmentMismatches( + [{ role: 'defender', equipment: [{ ...worn(SWORD), itemType: 999n }] }], + ruleset, + ); + + expect(mismatch).toBe("defender slot 0: item 999 is not in the ruleset's catalog"); + }); + + it('catches an item worn in a slot the catalog does not put it in', () => { + const [mismatch] = findEquipmentMismatches( + [{ role: 'attacker', equipment: [{ ...worn(SWORD), slot: 2 }] }], + ruleset, + ); + + expect(mismatch).toBe('attacker item 3: worn in slot 2, catalog says slot 0'); + }); + + it('reports every mismatch rather than stopping at the first', () => { + // A caller deciding whether to refuse a battle wants the whole disagreement in one + // message, not to rediscover it one field at a time. + const mismatches = findEquipmentMismatches( + [ + { role: 'attacker', equipment: [{ ...worn(SWORD), atk: 50, hp: 7 }] }, + { role: 'defender', equipment: [{ ...worn(PLATE), def: 99 }] }, + ], + ruleset, + ); + + expect(mismatches).toHaveLength(3); + expect(mismatches.filter((m) => m.startsWith('attacker'))).toHaveLength(2); + expect(mismatches.filter((m) => m.startsWith('defender'))).toHaveLength(1); + }); + + // A ruleset with no catalog is every version 1 ruleset. Gear against one is not + // "unpriced by omission", it is gear that ruleset cannot account for at all. + it('treats an absent catalog as pricing nothing', () => { + expect( + findEquipmentMismatches([{ role: 'attacker', equipment: [worn(SWORD)] }], { + ...SOURCE_DEFAULT_RULESET, + itemCatalog: [], + }), + ).toHaveLength(1); + }); +}); diff --git a/verifier/src/checks/equipment.ts b/verifier/src/checks/equipment.ts index 67d18033..c61524cb 100644 --- a/verifier/src/checks/equipment.ts +++ b/verifier/src/checks/equipment.ts @@ -1,56 +1,33 @@ -import { bonusFromEquipment, type BattleReceipt, type Ruleset } from '@cryptopets/protocol'; +import { bonusFromEquipment, type BattleReceipt, findEquipmentMismatches, type Ruleset } from '@cryptopets/protocol'; import type { CheckResult } from './types'; /** * Confirms each pet's frozen modifiers are the ones its items are supposed to grant - * (roadmap §4). + * (roadmap §4, threat T13). * - * The combat replay proves a fight followed from the numbers in the receipt. It cannot - * prove those numbers were *right*: a receipt that quietly gave one pet +50 ATK from a - * dagger replays perfectly, because the inflated bonus is the very thing being replayed - * against. Self-consistent is not the same as honest. - * - * This closes that gap using two fields that exist for no other reason. The snapshot - * records each item's `itemType` alongside its resolved bonus, and the ruleset the receipt - * names publishes what every combat-affecting item does. So the declared effect and the - * applied effect can be compared, by anyone, years later, from the receipt and its bundle - * alone. + * The comparison itself is `findEquipmentMismatches` in `@cryptopets/protocol`, not a copy + * here. It gained a second caller once the backend began refusing a battle at acceptance + * on the same grounds, and two implementations of one comparison would drift into a battle + * that accepts and then fails to verify — with the comparison the last thing anyone would + * suspect. This function's own job is the part that is specific to verifying: naming the + * check and shaping the result. * * What it still does not prove is that the pet *owned* the item. That is a claim about * chain state at `sourceVersion`, which this package deliberately cannot read — it has no * network access, by design. A verifier that wants that checks `ItemCore.equipmentOf` at * the recorded version itself; this narrows the remaining trust to exactly that one - * question (threat T13). + * question. */ export function checkEquipment(receipt: BattleReceipt, ruleset: Ruleset): CheckResult { const check = 'equipment'; - const declared = new Map((ruleset.itemCatalog ?? []).map((item) => [item.itemType, item])); - - const mismatches: string[] = []; - for (const [role, pet] of [['attacker', receipt.snapshot.attacker], ['defender', receipt.snapshot.defender]] as const) { - for (const entry of pet.equipment ?? []) { - const item = declared.get(entry.itemType); - if (!item) { - // An item the ruleset never priced. The fight used a modifier from - // nowhere, which is unauditable rather than merely unusual. - mismatches.push(`${role} slot ${entry.slot}: item ${entry.itemType} is not in the ruleset's catalog`); - continue; - } - if (item.slot !== entry.slot) { - mismatches.push( - `${role} item ${entry.itemType}: worn in slot ${entry.slot}, catalog says slot ${item.slot}`, - ); - } - for (const field of ['hp', 'atk', 'def', 'int', 'mdef'] as const) { - if (entry[field] !== item[field]) { - mismatches.push( - `${role} item ${entry.itemType}: ${field} applied ${entry[field]}, catalog declares ${item[field]}`, - ); - } - } - } - } + const mismatches = findEquipmentMismatches( + [ + { role: 'attacker', equipment: receipt.snapshot.attacker.equipment }, + { role: 'defender', equipment: receipt.snapshot.defender.equipment }, + ], + ruleset, + ); return mismatches.length === 0 ? { check, ok: true } : { check, ok: false, detail: mismatches.join('; ') }; } From 0a2adf019084a5714f548f53595061393d6677ae Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 14:55:50 -0400 Subject: [PATCH 04/36] fix(backend): typecheck the operator scripts --- backend/package.json | 3 +- .../scripts/grant-defense-authorization.ts | 22 +++++++++--- backend/scripts/seed-item-catalog.ts | 8 ++++- backend/tsconfig.scripts.json | 28 +++++++++++++++ docs/plan-battle-inventory-hardening.md | 34 ++++++++++++------- 5 files changed, 76 insertions(+), 19 deletions(-) create mode 100644 backend/tsconfig.scripts.json diff --git a/backend/package.json b/backend/package.json index 124eedeb..aaedd6fe 100644 --- a/backend/package.json +++ b/backend/package.json @@ -23,7 +23,8 @@ "test": "vitest run", "test:watch": "vitest", "test:coverage": "vitest run --coverage", - "lint": "eslint .", + "typecheck:scripts": "tsc --noEmit -p tsconfig.scripts.json", + "lint": "eslint . && pnpm typecheck:scripts", "lint:fix": "eslint . --fix" }, "dependencies": { diff --git a/backend/scripts/grant-defense-authorization.ts b/backend/scripts/grant-defense-authorization.ts index 823596f4..87da673f 100644 --- a/backend/scripts/grant-defense-authorization.ts +++ b/backend/scripts/grant-defense-authorization.ts @@ -22,8 +22,8 @@ * --max-level 100 highest attacker level accepted, default 100 * --api http://... backend base URL, default http://localhost:3001 */ -import { defenseAuthorizationTypedData } from '@cryptopets/protocol'; -import { Wallet } from 'ethers'; +import { assertChainId, defenseAuthorizationTypedData } from '@cryptopets/protocol'; +import { type TypedDataField, Wallet } from 'ethers'; interface Options { petIds: string[]; @@ -94,8 +94,13 @@ async function main(): Promise { ruleset: { hash: string }; }>(configRes, 'GET /api/battle/config'); - const chainId = config.chainIds.find((id) => id.startsWith('eip155:')); - if (!chainId) throw new Error(`no EVM chain in served config: ${config.chainIds.join(', ')}`); + const evmChain = config.chainIds.find((id) => id.startsWith('eip155:')); + if (!evmChain) throw new Error(`no EVM chain in served config: ${config.chainIds.join(', ')}`); + // Validated rather than asserted. The prefix test above narrows nothing on its own, and + // the value came off the wire, so `assertChainId` is what turns a served string into a + // `ChainId` the protocol will accept — and rejects a malformed one here rather than + // inside the signature. + const chainId = assertChainId(evmChain); const now = Math.floor(Date.now() / 1000); const authorization = { @@ -127,7 +132,14 @@ async function main(): Promise { expiresAt: authorization.expiresAt, revocationNonce: 0, }); - const signature = await wallet.signTypedData(typed.domain, typed.types, typed.message); + // `typed.types` is a readonly tuple, because the protocol builds the EIP-712 type list + // as a literal and a mutable one could be reordered by a caller — which would change + // the digest. ethers wants a mutable `TypedDataField[]`, so the array is copied rather + // than cast: a cast would hand ethers the protocol's own object to do as it likes with. + const types: Record = Object.fromEntries( + Object.entries(typed.types).map(([name, fields]) => [name, fields.map((field) => ({ ...field }))]), + ); + const signature = await wallet.signTypedData(typed.domain, types, typed.message); const token = await authenticate(opts.api, wallet); const res = await fetch(`${opts.api}/api/battle/authorizations`, { diff --git a/backend/scripts/seed-item-catalog.ts b/backend/scripts/seed-item-catalog.ts index e36d6209..d4c7bee6 100644 --- a/backend/scripts/seed-item-catalog.ts +++ b/backend/scripts/seed-item-catalog.ts @@ -26,6 +26,7 @@ */ import 'dotenv/config'; +import { Prisma } from '../src/generated/prisma/client'; import { prisma } from '../src/config/prisma'; import { assertCatalog, SLOT } from '../src/features/inventory/catalog'; import { ITEM_CATALOG } from '../src/features/inventory/catalog.data'; @@ -71,7 +72,12 @@ async function seedDatabase(dryRun: boolean): Promise { category: item.category, slot: item.slot === undefined ? null : SLOT[item.slot], rarity: item.rarity, - effect: item.effect ?? null, + // `Prisma.DbNull`, not `null`. For a nullable Json column Prisma makes the + // distinction explicit: `DbNull` writes SQL NULL, `JsonNull` writes the JSON + // value `null`, and a bare `null` is rejected because it cannot say which was + // meant. SQL NULL is what the reader expects — `asItemEffect` treats it as + // "no effect", and the column is what an inert collectible leaves empty. + effect: item.effect === undefined ? Prisma.DbNull : (item.effect as unknown as Prisma.InputJsonValue), name: item.name, description: item.description, }; diff --git a/backend/tsconfig.scripts.json b/backend/tsconfig.scripts.json new file mode 100644 index 00000000..9f702c46 --- /dev/null +++ b/backend/tsconfig.scripts.json @@ -0,0 +1,28 @@ +{ + // Typechecks the operator scripts, which `tsconfig.json` deliberately does not. + // + // They cannot simply be added to the main config's `include`: `pnpm build` runs `tsc` + // with it, so they would be emitted into `dist/` and shipped as part of the server. + // These are one-shot tools run with `tsx`, not server code. + // + // But `tsx` strips types rather than checking them, so with no config naming these + // files nothing checked them at all, and four type errors had accumulated across + // `grant-defense-authorization.ts` and `seed-item-catalog.ts`. That matters more here + // than almost anywhere else in the repo: these are the files an operator points at the + // production database. + // + // Run with `pnpm --filter backend typecheck:scripts`. + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true + }, + "include": [ + "src/**/*", + "scripts/**/*.ts" + ], + "exclude": [ + "node_modules", + "dist", + "src/**/*.test.ts" + ] +} diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index 472502ab..f2ecb2cc 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -247,18 +247,28 @@ Small, none of them urgent. - [x] **Q3 `verify.worker.ts:60-62` casts to `Record`** to read a shape the codec from B1.1 will type properly. Folded into B1.1 rather than done twice. -### Flagged, not fixed: `backend/scripts/` is not typechecked - -`backend/tsconfig.json` includes `src/**/*` only, so nothing typechecks the operator scripts, -and three of them do not compile today. `grant-defense-authorization.ts` has a `ChainId` cast -and a readonly-vs-mutable `TypedDataField[]` mismatch; `seed-item-catalog.ts` cannot assign a -nullable `effect` to Prisma's `InputJsonValue`. All pre-existing and unrelated to this branch -(none of these files, nor anything they import, is in its diff). They still *run*, since `tsx` -strips types rather than checking them. - -Left alone deliberately, per CLAUDE.md's surgical-changes rule, but worth its own branch: these -are exactly the files an operator runs against production, and they are the only TypeScript in -the repo with no compiler watching them. +- [x] **Q4 `backend/scripts/` had no compiler watching it.** `backend/tsconfig.json` includes + `src/**/*` only, and `tsx` strips types rather than checking them, so nothing checked the + operator scripts and four type errors had accumulated. All pre-existing and unrelated to + this branch, but these are the files an operator points at the production database, so + they were worth fixing before O1 rather than after. + + Fixed rather than suppressed, and each was hiding something: + - `grant-defense-authorization.ts` narrowed a served `chainId` with `startsWith('eip155:')`, + which narrows nothing to the compiler. Now `assertChainId`, so a malformed value is + rejected at the boundary instead of inside the signature. + - The same file handed ethers the protocol's own readonly EIP-712 type list. Copied now, + rather than cast: the list is readonly because reordering it changes the digest. + - `seed-item-catalog.ts` wrote a bare `null` to a nullable Json column. Prisma rejects + that precisely because it cannot tell SQL NULL from JSON `null`; it wants `Prisma.DbNull`, + which is what the reader expects. + + Kept out of the main config on purpose: `pnpm build` runs `tsc` with it, so including the + scripts there would emit them into `dist/` and ship one-shot tools as server code. They get + `tsconfig.scripts.json` and a `typecheck:scripts` script instead, wired into `backend`'s + `lint`, which root `pnpm lint` already runs and `static-checks.yml` already enforces. No + workflow change needed. + Verify: `pnpm --filter backend lint`. ## Operational, unblocked by code From c72b835101ed02dc6e83a9a701601b5026e21e35 Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 15:06:59 -0400 Subject: [PATCH 05/36] fix(backend): stop a self-battle swallowing one of its own drops --- backend/src/features/inventory/drops.ts | 46 +++++++++++-- .../tests/features/inventory/drops.test.ts | 68 +++++++++++++++++++ docs/plan-battle-inventory-hardening.md | 32 +++++++++ 3 files changed, 139 insertions(+), 7 deletions(-) diff --git a/backend/src/features/inventory/drops.ts b/backend/src/features/inventory/drops.ts index 3744361e..d7cf875d 100644 --- a/backend/src/features/inventory/drops.ts +++ b/backend/src/features/inventory/drops.ts @@ -153,9 +153,18 @@ function readUint32(bytes: Uint8Array, offset: number): number { * * Idempotent under the retry that transaction can take. The entitlement's unique key is * (sourceRef, owner, itemType), and sourceRef is the battle id, so a replay of the same - * battle collides with its own earlier row instead of paying twice. Two drops of the same - * item to the same wallet from one battle would collide too, which is why each side rolls - * at most one item. + * battle collides with its own earlier row instead of paying twice. + * + * That same key is why the two sides are merged before writing rather than inserted as + * they come. Nothing stops a player fighting two pets they both own, and then the winner + * and the loser are one wallet; when both rolls land on the same item the two drops share + * a key, and `skipDuplicates` silently keeps one. Measured on the shipped pool that is + * about one in six of the self-battles that pay twice, each one quietly costing the player + * an item they earned. Merging turns that into a single row of quantity 2, which is what + * was owed. + * + * Returns what was written, not what was rolled, so a caller sees the same thing the table + * does. */ export async function recordBattleDrops( tx: Prisma.TransactionClient, @@ -168,15 +177,16 @@ export async function recordBattleDrops( rates?: DropRates; }, ): Promise { - const drops = rollDrops(args.seed, args.battleId, args.winnerOwner, args.loserOwner, args.rates); - if (drops.length === 0) { - return drops; + const rolled = rollDrops(args.seed, args.battleId, args.winnerOwner, args.loserOwner, args.rates); + if (rolled.length === 0) { + return rolled; } + const drops = mergeDrops(rolled); await tx.itemEntitlement.createMany({ data: drops.map((drop) => ({ chain: args.chain, - owner: normalizeAccount(drop.owner), + owner: drop.owner, itemType: drop.itemType, quantity: drop.quantity, source: 'battle_drop', @@ -187,3 +197,25 @@ export async function recordBattleDrops( return drops; } + +/** + * Totals drops that would share an entitlement key, normalizing the owner first. + * + * The normalize has to happen here rather than at the insert, because it is part of the + * key: two spellings of one address are one wallet to the unique index and would be two + * groups to anything grouping on the raw value. + */ +function mergeDrops(drops: readonly Drop[]): Drop[] { + const byKey = new Map(); + for (const drop of drops) { + const owner = normalizeAccount(drop.owner); + const key = `${owner}:${drop.itemType}`; + const existing = byKey.get(key); + if (existing) { + existing.quantity += drop.quantity; + } else { + byKey.set(key, { owner, itemType: drop.itemType, quantity: drop.quantity }); + } + } + return [...byKey.values()]; +} diff --git a/backend/tests/features/inventory/drops.test.ts b/backend/tests/features/inventory/drops.test.ts index b37f4e56..75754d3a 100644 --- a/backend/tests/features/inventory/drops.test.ts +++ b/backend/tests/features/inventory/drops.test.ts @@ -147,4 +147,72 @@ describe('recordBattleDrops', () => { expect(drops).toEqual([]); expect(tx.itemEntitlement.createMany).not.toHaveBeenCalled(); }); + + /** + * A player fighting two pets they both own is the case the unique key does not survive + * naively. Winner and loser are then one wallet, and when both rolls land on the same + * item the two entitlements share (sourceRef, owner, itemType), so `skipDuplicates` + * keeps one and the player silently loses an item they earned. + * + * Nothing forbids the battle: `assertBattleSnapshot` refuses a pet fighting *itself*, + * and the defender's own wallet can sign the authorization. + */ + describe('when the winner and the loser are the same wallet', () => { + /** A battle id where both sides roll the same item, found by scanning the pool. */ + const COLLIDING = (() => { + for (let i = 0; i < 500; i++) { + const drops = rollDrops(SEED, `btl_${i}`, WINNER, WINNER, ALWAYS); + if (drops.length === 2 && drops[0]!.itemType === drops[1]!.itemType) return `btl_${i}`; + } + throw new Error('no colliding battle id in the first 500; the drop pool changed'); + })(); + + it('merges the two drops into one entitlement of quantity 2', async () => { + const tx = fakeTx(); + + const drops = await recordBattleDrops(tx as never, { + chain: 'evm', battleId: COLLIDING, seed: SEED, + winnerOwner: WINNER, loserOwner: WINNER, rates: ALWAYS, + }); + + const { data } = tx.itemEntitlement.createMany.mock.calls[0]![0]; + expect(data).toHaveLength(1); + expect(data[0]).toMatchObject({ owner: WINNER, quantity: 2 }); + // Returned as written, so a caller sees what the table holds. + expect(drops).toEqual([{ owner: WINNER, itemType: data[0].itemType, quantity: 2 }]); + }); + + it('still writes two rows when the same wallet wins two different items', async () => { + const battleId = (() => { + for (let i = 0; i < 500; i++) { + const drops = rollDrops(SEED, `btl_${i}`, WINNER, WINNER, ALWAYS); + if (drops.length === 2 && drops[0]!.itemType !== drops[1]!.itemType) return `btl_${i}`; + } + throw new Error('no two-item battle id in the first 500'); + })(); + const tx = fakeTx(); + + await recordBattleDrops(tx as never, { + chain: 'evm', battleId, seed: SEED, + winnerOwner: WINNER, loserOwner: WINNER, rates: ALWAYS, + }); + + expect(tx.itemEntitlement.createMany.mock.calls[0]![0].data).toHaveLength(2); + }); + + it('merges on the normalized owner, since that is what the unique key stores', async () => { + // Two spellings of one address are one wallet to the index and would otherwise + // be two groups here, which puts the collision straight back. + const tx = fakeTx(); + + await recordBattleDrops(tx as never, { + chain: 'evm', battleId: COLLIDING, seed: SEED, + winnerOwner: WINNER.toUpperCase().replace('0X', '0x'), loserOwner: WINNER, rates: ALWAYS, + }); + + const { data } = tx.itemEntitlement.createMany.mock.calls[0]![0]; + expect(data).toHaveLength(1); + expect(data[0]).toMatchObject({ owner: WINNER, quantity: 2 }); + }); + }); }); diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index f2ecb2cc..dac6967b 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -216,6 +216,38 @@ hundreds, where the largest shipped bonus is 45. Lowering it is a balance call, later widens the ceiling every outstanding authorization implies. Worth a line in `catalog.ts` saying so. +## C4: a self-battle silently swallowed one of its own drops + +Found after the C1 to C3 work, reviewing the drop path rather than the snapshot path. + +`item_entitlement`'s unique key is `(sourceRef, owner, itemType)` and `sourceRef` is the +battle id, which is what makes a retried receipt transaction idempotent. `recordBattleDrops` +inserted the winner's and the loser's drop as separate rows under `skipDuplicates: true`, and +its comment argued the two could never collide because "each side rolls at most one item". + +That holds only while the two sides are different wallets. Nothing forbids a player fighting +two pets they both own: `assertBattleSnapshot` refuses a pet fighting *itself*, and the +defender's own wallet can sign the authorization. Then winner and loser are one wallet, and +when both rolls land on the same item the two entitlements share a key, so `skipDuplicates` +keeps one and the player loses an item they earned. + +Measured on the shipped pool, scanning 500 battle ids with both rates forced to certainty: +**82 collided**, about one in six. At the real rates (25% winner, 5% loser) both sides pay in +roughly 1.25% of battles, so this reaches about one self-battle in 500. Small, silent, and +wrong in the player's disfavour. + +- [x] **C4.1** Merge drops by `(normalized owner, itemType)` before writing, so the case + becomes one row of quantity 2 rather than two rows one of which vanishes. Normalizing + inside the merge rather than at the insert, because the owner is part of the key: two + spellings of one address are one wallet to the index and would be two groups to anything + grouping on the raw value. `recordBattleDrops` now returns what it wrote rather than what + it rolled. + Verify: `pnpm --filter backend exec vitest run tests/features/inventory/drops.test.ts`. + +`rollDrops` is unchanged, deliberately. It is the pure derivation of what a battle owed each +side, and that is what an outsider recomputes from the receipt; reconciling two owed drops with +one storage key is the writer's job, not the derivation's. + ## D2: drops are outside the signed payload Recorded in `drops.ts:14-19` as a known v1 limit and correct as written: derived from the From 441f50f968c36aaf2ffd797ac0edfe1b1c01647d Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 15:15:46 -0400 Subject: [PATCH 06/36] docs: correct the claim that battle drops are recomputable --- CLAUDE.md | 14 ++++--- backend/src/features/inventory/drops.ts | 31 ++++++++++++---- docs/plan-battle-inventory-hardening.md | 49 +++++++++++++++++++------ docs/plan-inventory-items.md | 5 ++- 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 06a8fcbe..fd39d309 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -225,11 +225,15 @@ Three things are easy to get wrong here: emptied slot writes `item_type "0"`, because indexer-go resumes from an `updatedAt` watermark and a deleted row is one it never learns about. Zero is a value, not an absence. - **Battle drops derive from the battle's own drand seed**, committed before the fight - resolves, so nobody including the operator can grind one and anyone holding the receipt - can recompute it. They are written in the *same transaction* as the receipt, the rule - `battle_history` already follows. The honest limit: the drop is not inside the signed - payload in v1, so an outsider can recompute what was owed and notice a discrepancy but - cannot prove one from the receipt alone. + resolves, so nobody including the operator can grind one. They are written in the *same + transaction* as the receipt, the rule `battle_history` already follows. The honest limit + is larger than it used to say here: an outsider holding the receipt **cannot** recompute + the drop at all. The rates and the drop pool are backend constants (`drops.ts`, + `catalog.data.ts`), neither reaches the ruleset, and only the seed and battle id are + signed, so the payout is not pinned by the receipt either. Publishing them would put + non-equipment items into `rulesetHash`, which §4 rules out because adding a collectible + would then re-consent every defender. Tracked as D2 in + `docs/plan-battle-inventory-hardening.md`. Equipment reaching combat is what made this expensive, and it is why `snapshot` and `ruleset` both went to schema v2 (see the combat-simulator section above). The snapshot diff --git a/backend/src/features/inventory/drops.ts b/backend/src/features/inventory/drops.ts index d7cf875d..af4f0657 100644 --- a/backend/src/features/inventory/drops.ts +++ b/backend/src/features/inventory/drops.ts @@ -8,15 +8,30 @@ import type { ItemDefinitionSeed } from './catalog'; * Battle-reward drops (roadmap §4). * * Seeded from the battle's own drand seed rather than from a new randomness source. That - * seed is committed to a future drand round before the fight resolves, so nobody — - * including this server — can grind a drop by re-rolling: changing the outcome would mean - * changing a value that was published in advance. It also means a third party holding the - * receipt can recompute exactly what should have dropped. + * seed is committed to a future drand round before the fight resolves, so nobody including + * this server can grind a drop by re-rolling: changing the outcome would mean changing a + * value that was published in advance. That property is real and it is the reason this + * derives from the seed at all. * - * Be precise about how far that goes. The drop is **not** part of the signed receipt in - * v1, so an outsider can recompute what we owed and notice if we paid something else, but - * cannot prove it from the receipt alone. Putting drops inside the signed payload means a - * receipt schema version and a place in the ruleset hash, which is §4 phase 4 work. + * Be precise about how far it goes, because it is easy to overstate and this comment used + * to. A third party holding the receipt **cannot** recompute what should have dropped. + * Two of the three inputs are unpublished: `DropRates` and `DROP_POOL` are constants in + * this file and in `catalog.data.ts`, and neither reaches the ruleset, so neither is + * covered by `rulesetHash` or by anything the receipt names. Only the seed and the battle + * id are signed. Someone reading this source can reproduce a drop; someone holding only a + * receipt and the published bundle cannot. + * + * Nor is the payout pinned by the receipt. `rates` is an argument, so the same seed and + * battle id yield different answers under different rates, and nothing records which were + * used. The operator cannot re-roll a drop, but can change the odds it was drawn against + * without leaving a trace. + * + * Closing that means publishing the rates and the drop pool, which puts non-equipment + * items into the ruleset. §4 deliberately keeps them out: a `rulesetHash` that moved every + * time a collectible was added would re-prompt every defender for consent and train + * players to click through the one prompt that matters. So this is a standing design + * tension, not a missing field, and it is tracked as D2 in + * `docs/plan-battle-inventory-hardening.md` rather than quietly fixed here. * * The pool is read from the shipped catalog constant rather than from `item_definition`, * deliberately. A replay has to reproduce what a battle dropped, and a table that content diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index dac6967b..72244ffa 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -245,17 +245,44 @@ wrong in the player's disfavour. Verify: `pnpm --filter backend exec vitest run tests/features/inventory/drops.test.ts`. `rollDrops` is unchanged, deliberately. It is the pure derivation of what a battle owed each -side, and that is what an outsider recomputes from the receipt; reconciling two owed drops with -one storage key is the writer's job, not the derivation's. - -## D2: drops are outside the signed payload - -Recorded in `drops.ts:14-19` as a known v1 limit and correct as written: derived from the -battle's own drand seed, written in the receipt's transaction, recomputable by anyone holding -the receipt. What an outsider cannot do is *prove* a discrepancy from the receipt alone. - -Listed here so it is a tracked decision rather than a comment. It needs a receipt schema -version, so it belongs with any other bump rather than on its own. +side; reconciling two owed drops with one storage key is the writer's job, not the +derivation's. + +## D2 (decision): drops are not verifiable, and the reason is not a missing field + +The description this section carried was wrong, in the same way D1's was, and inherited from +`drops.ts`'s own doc comment. Both said a receipt holder could recompute a drop and merely +lacked the means to *prove* a discrepancy. Neither is accurate. + +`rollDrops` reads three inputs. The seed and the battle id are in the signed receipt. The +third, `DropRates`, is a constant in `drops.ts`, and the pool it draws from is +`ITEM_CATALOG` filtered to non-equipment in `catalog.data.ts`. Neither reaches the ruleset, +so neither is covered by `rulesetHash` or by anything else the receipt names. **An outsider +holding a receipt and the published bundle cannot recompute the drop at all.** Someone +reading this source can; that is not the same property. + +The payout is not pinned either. `rates` is a parameter, so the same seed and battle id +produce different answers under different odds, and nothing records which were used. The +anti-grinding property survives all of this and is worth keeping: the operator cannot +re-roll a committed seed. But it can change the odds that seed was drawn against. + +What makes this a decision rather than a fix: closing it means publishing the rates and the +drop pool, which puts non-equipment items into the ruleset. §4 rules that out on purpose, and +CLAUDE.md states why, that a `rulesetHash` moving every time a collectible is added would +re-consent every defender and train players to click through the one prompt that matters. So +verifiable drops and stable consent are in direct tension, and picking between them is a +product call. + +- [x] **D2.1 Make the claim honest.** Corrected in `drops.ts`, `CLAUDE.md`, and + `plan-inventory-items.md` §5, all three of which asserted recomputability. A false + verifiability claim is worse than a documented gap: it is the kind of thing a later + decision gets built on, and it nearly was here. +- [ ] **D2.2 Decide the tension, or decide to keep it.** Options, none of them free: + publish rates and pool in a *separate* digest the receipt names, so drop rules version + independently of consent; or accept that drops are operator-attested in v1 and say so + in the player-facing docs; or fold drops into the receipt and pay the consent cost. + Not scheduled. Nothing is broken today, and the honest comment is the prerequisite for + choosing well. ## D3: shipping Phase 4 is a re-consent event diff --git a/docs/plan-inventory-items.md b/docs/plan-inventory-items.md index ff77e908..70d4132c 100644 --- a/docs/plan-inventory-items.md +++ b/docs/plan-inventory-items.md @@ -39,7 +39,10 @@ protocol objects, and it is sequenced last for that reason. skill modifiers, with the sum clamped to 65535 rather than wrapped. Excluding negative modifiers removes any underflow question against `toUint16`'s wrap semantics. 5. **Drops derive from the battle's existing drand seed** (`keccak(seed, battleId, "DROP")`), - so a drop replays from the receipt like every other outcome. No second randomness system. + so no second randomness system and no drop the operator can grind. Note the second half + of this as written was wrong: a drop does **not** replay from the receipt like other + outcomes, because the rates and the drop pool are unpublished backend constants. See + D2 in [`plan-battle-inventory-hardening.md`](./plan-battle-inventory-hardening.md). ## Environment notes From 3bafd1e965004ce454c32b4fd94575352d4993ed Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 15:26:15 -0400 Subject: [PATCH 07/36] fix(backend): stop an unconfirmed mint paying an entitlement twice --- backend/src/features/inventory/index.ts | 2 +- .../src/features/inventory/inventory.chain.ts | 43 ++++++++++++++++- .../src/features/inventory/inventory.write.ts | 20 ++++++-- .../inventory/inventory.write.test.ts | 45 ++++++++++++++++-- docs/plan-battle-inventory-hardening.md | 46 +++++++++++++++++++ 5 files changed, 147 insertions(+), 9 deletions(-) diff --git a/backend/src/features/inventory/index.ts b/backend/src/features/inventory/index.ts index d496f4d1..e501de80 100644 --- a/backend/src/features/inventory/index.ts +++ b/backend/src/features/inventory/index.ts @@ -43,7 +43,7 @@ export { type UseItemResult, type WriteFailure, } from './inventory.write'; -export { getItemCoreClient, type ItemCoreClient } from './inventory.chain'; +export { getItemCoreClient, type ItemCoreClient, UnconfirmedTxError } from './inventory.chain'; export { DEFAULT_DROP_RATES, recordBattleDrops, diff --git a/backend/src/features/inventory/inventory.chain.ts b/backend/src/features/inventory/inventory.chain.ts index 7e0fc32a..078254f2 100644 --- a/backend/src/features/inventory/inventory.chain.ts +++ b/backend/src/features/inventory/inventory.chain.ts @@ -105,6 +105,29 @@ function buildClient(): ItemCoreClient | null { }; } +/** + * A transaction that was broadcast but whose outcome is unknown. + * + * Distinct from every other failure here, and the distinction is what stops a double mint. + * A caller undoing its own bookkeeping after a failed write is only safe when the write + * definitely did not happen. "Simulate reverted" and "the receipt says reverted" both mean + * that. "The RPC stopped answering while waiting for the receipt" does not: the transaction + * may well be mined, and treating it as a failure is how a claim gets released and paid a + * second time. + */ +export class UnconfirmedTxError extends Error { + constructor( + readonly hash: `0x${string}`, + message: string, + // `override` because Error already declares `cause`. Narrowed to a parameter + // property so a caller can read it without the optional-chaining dance. + override readonly cause?: unknown, + ) { + super(message); + this.name = 'UnconfirmedTxError'; + } +} + /** * Simulates, sends, and waits for the receipt. * @@ -113,6 +136,11 @@ function buildClient(): ItemCoreClient | null { * both callers change state that depends on the transaction having landed: a burn that is * still pending is an item the player could spend again. * + * Failures are sorted into two kinds, because the callers have to treat them differently. + * Anything before the broadcast, and an on-chain revert, mean nothing moved. Anything after + * the broadcast that leaves the outcome unknown raises `UnconfirmedTxError` carrying the + * hash, so a caller can record it and refuse to undo state that may already be real. + * * One at a time, like the settle keeper's submitter. Item writes are rare relative to * block times, so a single in-flight transaction avoids nonce management entirely. */ @@ -134,8 +162,21 @@ async function send( args, }); const hash = await walletClient.writeContract(request as Parameters[0]); - const receipt = await publicClient.waitForTransactionReceipt({ hash }); + + let receipt: Awaited>; + try { + receipt = await publicClient.waitForTransactionReceipt({ hash }); + } catch (error) { + // Broadcast, outcome unknown. A timeout here is the ordinary case: the RPC went + // away, or the transaction is simply slow, and the chain will very likely mine it. + throw new UnconfirmedTxError( + hash, + `ItemCore.${functionName} was broadcast as ${hash} but its receipt could not be read; treat it as possibly mined`, + error, + ); + } if (receipt.status !== 'success') { + // A confirmed revert, which is a definite no. Safe for a caller to undo. throw new Error(`ItemCore.${functionName} reverted on chain (${hash})`); } return hash; diff --git a/backend/src/features/inventory/inventory.write.ts b/backend/src/features/inventory/inventory.write.ts index 56b7dcc4..38e13456 100644 --- a/backend/src/features/inventory/inventory.write.ts +++ b/backend/src/features/inventory/inventory.write.ts @@ -7,7 +7,7 @@ import { findBalance, findDefinitionByType } from '@repositories/inventory.repos import { servedDeploymentId } from '@features/battle/ledger'; import { asItemEffect } from './catalog'; -import { getItemCoreClient } from './inventory.chain'; +import { getItemCoreClient, UnconfirmedTxError } from './inventory.chain'; /** * Inventory writes (roadmap §4): spend a consumable, claim an earned item, grant one. @@ -208,9 +208,23 @@ export async function claimEntitlement(caller: string, entitlementId: string): P await prisma.itemEntitlement.update({ where: { id: entitlementId }, data: { txHash: mintTxHash } }); return { mintTxHash, itemType: entitlement.itemType, quantity: entitlement.quantity }; } catch (error) { + if (error instanceof UnconfirmedTxError) { + // Broadcast, outcome unknown, so the claim stays claimed. Releasing here would + // be the double-mint: the transaction is very likely mined, and a retry would + // send a second one. The hash is recorded so the row names the transaction to + // reconcile against, and so the `txHash: null` guard below keeps meaning what + // it says. Costs at most one item stuck pending until someone looks. + await prisma.itemEntitlement.update({ where: { id: entitlementId }, data: { txHash: error.hash } }); + console.error( + `[inventory] entitlement ${entitlementId} broadcast mint ${error.hash} but could not confirm it; left claimed to avoid a double mint, reconcile by hand`, + error.cause, + ); + throw error; + } // Released, so a failed mint is retryable rather than a permanently burned claim. - // Safe because the mint did not land: the client waits for a receipt and treats a - // reverted one as a throw. + // Safe only for the failures that definitely moved nothing: a simulate revert, a + // send that never left, or a receipt that came back reverted. `UnconfirmedTxError` + // is the one that does not qualify, and it returned above. await prisma.itemEntitlement.updateMany({ where: { id: entitlementId, txHash: null }, data: { claimedAt: null }, diff --git a/backend/tests/features/inventory/inventory.write.test.ts b/backend/tests/features/inventory/inventory.write.test.ts index b60eb96c..9319584d 100644 --- a/backend/tests/features/inventory/inventory.write.test.ts +++ b/backend/tests/features/inventory/inventory.write.test.ts @@ -3,7 +3,13 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const client = { mintTo: vi.fn(), burnFrom: vi.fn() }; const chain = { getItemCoreClient: vi.fn(() => client as { mintTo: unknown; burnFrom: unknown } | null) }; -vi.mock('@features/inventory/inventory.chain', () => ({ +// The error class stays real: `claimEntitlement` branches on `instanceof`, and a stub +// class would let the release path be exercised by an error the production code would +// have treated as unconfirmed. +vi.mock('@features/inventory/inventory.chain', async () => ({ + ...(await vi.importActual( + '@features/inventory/inventory.chain', + )), getItemCoreClient: () => chain.getItemCoreClient(), })); @@ -33,6 +39,7 @@ vi.mock('@config/prisma', () => ({ }, })); +import { UnconfirmedTxError } from '@features/inventory/inventory.chain'; import { claimEntitlement, grantItem, isAdmin, useItem } from '@features/inventory/inventory.write'; import { prisma } from '@config/prisma'; @@ -180,9 +187,10 @@ describe('claimEntitlement', () => { expect(client.mintTo).not.toHaveBeenCalled(); }); - // Released rather than left claimed, so a failed mint is retryable. Safe because the - // client waits for a receipt and treats a reverted one as a throw. - it('releases the claim when the mint fails', async () => { + // Released rather than left claimed, so a failed mint is retryable. Safe only because + // this failure moved nothing: a simulate revert, a send that never left, or a receipt + // that came back reverted. + it('releases the claim when the mint definitely did not land', async () => { vi.mocked(prisma.itemEntitlement.findUnique).mockResolvedValue(ROW as never); vi.mocked(prisma.itemEntitlement.updateMany).mockResolvedValue({ count: 1 } as never); client.mintTo.mockRejectedValue(new Error('rpc down')); @@ -193,6 +201,35 @@ describe('claimEntitlement', () => { }); }); + /** + * The failure the release must not treat like the others: broadcast, outcome unknown. + * + * A receipt read that times out does not mean the mint failed, it means nobody knows. + * The transaction is very likely mined, so releasing the claim would let a retry send a + * second mint and pay the entitlement twice. + */ + it('keeps the claim when the mint was broadcast but could not be confirmed', async () => { + const hash = `0x${'ab'.repeat(32)}` as const; + vi.mocked(prisma.itemEntitlement.findUnique).mockResolvedValue(ROW as never); + vi.mocked(prisma.itemEntitlement.updateMany).mockResolvedValue({ count: 1 } as never); + client.mintTo.mockRejectedValue(new UnconfirmedTxError(hash, 'receipt unreadable')); + + await expect(claimEntitlement(OWNER, 'e1')).rejects.toThrow(UnconfirmedTxError); + + // Never released: the only updateMany is the claim itself, taken before the mint. + const released = vi + .mocked(prisma.itemEntitlement.updateMany) + .mock.calls.filter((call) => (call[0] as { data?: { claimedAt?: unknown } }).data?.claimedAt === null); + expect(released).toHaveLength(0); + + // The hash is recorded, so the row names the transaction to reconcile against and + // the `txHash: null` guard on the release path keeps meaning what it says. + expect(prisma.itemEntitlement.update).toHaveBeenCalledWith({ + where: { id: 'e1' }, + data: { txHash: hash }, + }); + }); + // 404, not 403: someone else's entitlement is indistinguishable from a missing one, so // an id cannot be probed by watching the answer change. it('reports another wallet’s entitlement as missing', async () => { diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index 72244ffa..d6eafb39 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -248,6 +248,52 @@ wrong in the player's disfavour. side; reconciling two owed drops with one storage key is the writer's job, not the derivation's. +## C5: an unconfirmed mint could pay an entitlement twice + +`claimEntitlement` marks the row claimed, mints, and on failure releases the claim so the +player can retry. Its comment justified the release as "safe because the mint did not land: +the client waits for a receipt and treats a reverted one as a throw." + +That covers two of the three ways the mint can fail and misses the third. A revert is a +definite no, and so is a send that never left. But `waitForTransactionReceipt` throwing means +the outcome is *unknown*, not failed: the transaction is broadcast and very likely mined. The +release then hands the player a retry that mints a second time, and `txHash` was never +written, so the `txHash: null` guard on the release did not stop it either. + +Narrow (it needs the RPC to drop between broadcast and receipt) but it pays out real items, +and RPC flakiness on this deployment is documented: `plan-inventory-items.md` records drpc +returning intermittent 500s on `eth_getTransactionCount` during the Base Sepolia deploy, which +is why the backend points at `sepolia.base.org` instead. + +- [x] **C5.1** Sort the failures by what is actually known. `send` now raises + `UnconfirmedTxError` carrying the hash when the broadcast succeeded but the receipt + could not be read; everything else keeps throwing plainly. `claimEntitlement` records + the hash and leaves the row claimed for that case only, so the worst outcome is one + entitlement stuck pending until someone reconciles it, rather than one item minted + twice. + Verify: `pnpm --filter backend exec vitest run tests/features/inventory/inventory.write.test.ts`. + +`useItem`'s burn takes the same client and is deliberately left as it was. Its ordering is +already the conservative one its doc comment describes: an unconfirmed burn costs the player +an item and gives nothing, which is a bad afternoon, where the reverse is a repeatable +exploit. + +## Reviewed and found clean + +Recorded so a later pass does not repeat the work. Neither of these produced a change: + +- **`indexer-go`'s inventory ingest.** Two watermarks, genuinely separate, so a busy balance + stream cannot drag the equipment cursor past unread rows. Coalescing keeps the highest + version per key across all three streams identically, which is required rather than merely + efficient: two rows sharing a key in one `ON CONFLICT` statement is a Postgres error, not a + silent overwrite. Watermarks are in-memory and reprimed by a full scan on restart, and the + periodic reconcile scan covers the one real gap in `updatedAt_gt` polling, which is two + blocks sharing a timestamp. +- **The subgraph half.** Balances are re-read through `balanceOf` rather than accumulated + from deltas, so a missed event stales a row instead of corrupting it. Escrow-on-equip + writes an `ItemBalance` row owned by the `ItemCore` contract itself; that is storage noise + no player read touches, since `findBalances` filters by owner and by `quantity > 0`. + ## D2 (decision): drops are not verifiable, and the reason is not a missing field The description this section carried was wrong, in the same way D1's was, and inherited from From 4fab6dc7c3412f673b5f4cebe238197f4fa4c2df Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 15:33:50 -0400 Subject: [PATCH 08/36] fix(shared): refresh pet progression when a consumable is spent --- docs/plan-battle-inventory-hardening.md | 23 +++++++++++++++ shared/src/hooks/battle/useBattleProgress.ts | 19 +++++++++++- shared/src/hooks/index.ts | 7 ++++- shared/src/hooks/inventory/useSpendItem.ts | 13 ++++++-- shared/tests/hooks/useInventory.test.tsx | 31 ++++++++++++++++++++ 5 files changed, 89 insertions(+), 4 deletions(-) diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index d6eafb39..7c1d2b71 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -278,6 +278,29 @@ already the conservative one its doc comment describes: an unconfirmed burn cost an item and gives nothing, which is a bad afternoon, where the reverse is a repeatable exploit. +## C6: using a consumable left the pet's own numbers stale + +`useSpendItem` invalidated the bag and documented the rest as someone else's job: "the pet's +battle progression moved too, so the caller refreshes that itself." Neither call site in +`components/inventory/index.tsx` did. + +Every effect the route accepts writes `pet_battle_progress`. `grant_xp` moves level and xp; +`clear_battle_cooldown` moves `readyAt`. So the item vanished from the bag and the pet went on +showing its old level, or, for the cooldown tonic, went on showing as resting. That last one is +the worst reading available: the player spends an item specifically to battle again, and the +UI says they still cannot. + +- [x] **C6.1** Invalidate progression in the hook rather than asking callers to remember. + `useBattleProgress` gained `battleProgressQueryKey` / `battleProgressQueryPrefix`, and + `useSpendItem` invalidates the prefix, following `petEquipmentForPetsQueryPrefix` + exactly and for the same reason: progression is cached per *list* of pets a screen + asked about, and a mutation cannot know which lists exist. + Verify: `pnpm --filter @shared/core exec vitest run tests/hooks/useInventory.test.tsx`. + +The general point is worth keeping. A comment asking every future caller to pair a mutation +with an invalidation is a bug waiting for its second caller, and this one did not survive its +first. A mutation that knows what it changed should invalidate it. + ## Reviewed and found clean Recorded so a later pass does not repeat the work. Neither of these produced a change: diff --git a/shared/src/hooks/battle/useBattleProgress.ts b/shared/src/hooks/battle/useBattleProgress.ts index 7ceb8eee..1179938f 100644 --- a/shared/src/hooks/battle/useBattleProgress.ts +++ b/shared/src/hooks/battle/useBattleProgress.ts @@ -62,6 +62,23 @@ export const mergeBattleProgress = (pet: Pet, progress: ProgressDto | undefined) * Degrades to unmerged chain values on any failure. That is the honest fallback: stale * progression is a worse number, an error is a missing pet list. */ +/** + * Every progression query for one chain, whatever set of pets it asked about. + * + * Exported for invalidation, and it has to be the prefix rather than a full key. Anything + * that moves a pet's progression moves one pet, while the cached entries are keyed by the + * *list* a screen asked for, and the mutation has no idea which lists exist. Matching on + * the prefix catches all of them; an exact key would silently miss every one. Same shape + * and same reason as `petEquipmentForPetsQueryPrefix`. + */ +export function battleProgressQueryPrefix(baseURL: string, chain: PetChain | null): unknown[] { + return ['battleProgress', baseURL, chain]; +} + +export function battleProgressQueryKey(baseURL: string, chain: PetChain | null, petIds: string[]): unknown[] { + return [...battleProgressQueryPrefix(baseURL, chain), petIds]; +} + export const useBattleProgress = (chain: PetChain | null, pets: Pet[]): Pet[] => { const apiClient = useApiClient(); const { isAuthenticated } = useAuth(); @@ -71,7 +88,7 @@ export const useBattleProgress = (chain: PetChain | null, pets: Pet[]): Pet[] => const petIds = useMemo(() => pets.map((pet) => pet.id).sort(), [pets]); const query = useQuery({ - queryKey: ['battleProgress', baseURL, chain, petIds], + queryKey: battleProgressQueryKey(baseURL, chain, petIds), enabled: chain != null && isAuthenticated && petIds.length > 0, queryFn: async () => { const { data } = await apiClient.post('/graphql', { diff --git a/shared/src/hooks/index.ts b/shared/src/hooks/index.ts index 8b8442fb..d14fc56d 100644 --- a/shared/src/hooks/index.ts +++ b/shared/src/hooks/index.ts @@ -29,7 +29,12 @@ export { usePetList, type PetListResult } from './pets/usePetList'; export { usePetCooldowns, type PetCooldowns, type PetCooldownStatus } from './pets/usePetCooldowns'; // Backend battle progression. usePetList already applies it to a player's own pets; // exported for anything reading pets from the chain by another route. -export { useBattleProgress, mergeBattleProgress } from './battle/useBattleProgress'; +export { + battleProgressQueryKey, + battleProgressQueryPrefix, + useBattleProgress, + mergeBattleProgress, +} from './battle/useBattleProgress'; export { useCreatePet, type CreatePetArgs, diff --git a/shared/src/hooks/inventory/useSpendItem.ts b/shared/src/hooks/inventory/useSpendItem.ts index da3b765b..9402f808 100644 --- a/shared/src/hooks/inventory/useSpendItem.ts +++ b/shared/src/hooks/inventory/useSpendItem.ts @@ -1,6 +1,7 @@ import { useMutation, useQueryClient } from '@tanstack/react-query'; import { useApiClient } from '../../contexts/ApiClientContext'; import type { PetChain } from '../../types/pet'; +import { battleProgressQueryPrefix } from '../battle/useBattleProgress'; import { inventoryQueryKey } from './useInventory'; /** @@ -54,10 +55,18 @@ export const useSpendItem = (): UseSpendItemResult => { // Invalidated rather than patched, and never optimistically. The burn is a // transaction: until the server says it landed, the item is still the player's, and // a bag that already showed it gone would be lying about a spend that could fail. - // The pet's battle progression moved too, so the caller refreshes that itself — - // this hook does not know which pet query the screen is using. + // + // Both things a consumable moves are refreshed here, not just the bag. Every effect + // this route accepts writes `pet_battle_progress`: `grant_xp` changes level and xp, + // `clear_battle_cooldown` changes readyAt. Leaving that to the caller was the + // arrangement before, and neither call site did it, so a cooldown tonic consumed the + // item and left the pet still showing as resting, the item reading as broken rather + // than as applied. A mutation that knows what it changed should invalidate it; a + // hook the caller has to remember to pair with is a bug waiting for its second + // caller. onSuccess: (_result, args) => { void queryClient.invalidateQueries({ queryKey: inventoryQueryKey(baseURL, args.chain) }); + void queryClient.invalidateQueries({ queryKey: battleProgressQueryPrefix(baseURL, args.chain) }); }, }); diff --git a/shared/tests/hooks/useInventory.test.tsx b/shared/tests/hooks/useInventory.test.tsx index af2d977a..845336f9 100644 --- a/shared/tests/hooks/useInventory.test.tsx +++ b/shared/tests/hooks/useInventory.test.tsx @@ -166,4 +166,35 @@ describe('useSpendItem', () => { result.current.spend({ chain: 'evm', petId: '7', itemType: '100' }), ).rejects.toThrow('You do not hold that item'); }); + + /** + * Both things a consumable moves get refreshed, not just the bag. + * + * Every effect this route accepts writes `pet_battle_progress`: `grant_xp` moves level + * and xp, `clear_battle_cooldown` moves readyAt. This used to be left to the caller, + * and the one caller did not do it, so a cooldown tonic consumed the item and left the + * pet still displayed as resting. The failure was silent in exactly the way a comment + * asking a caller to remember something always eventually is. + */ + it('refreshes the bag and the pet progression that the effect moved', async () => { + const client = new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false } } }); + const invalidated: unknown[][] = []; + vi.spyOn(client, 'invalidateQueries').mockImplementation((filters) => { + invalidated.push((filters as { queryKey: unknown[] }).queryKey); + return Promise.resolve(); + }); + const localWrapper = ({ children }: { children: React.ReactNode }) => ( + {children} + ); + + post.mockResolvedValue({ data: { burnTxHash: '0xburn', level: 5, xp: 0, readyAt: 0, leveledUp: true } }); + const { result } = renderHook(() => useSpendItem(), { wrapper: localWrapper }); + + await result.current.spend({ chain: 'evm', petId: '7', itemType: '100' }); + + expect(invalidated).toContainEqual(['inventory', 'https://api.test', 'evm']); + // The prefix, not a full key: progression is cached per *list* of pets a screen + // asked about, and this mutation cannot know which lists exist. + expect(invalidated).toContainEqual(['battleProgress', 'https://api.test', 'evm']); + }); }); From 8c6618514ce4341b979553581f91485eda4fccf4 Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 15:40:16 -0400 Subject: [PATCH 09/36] docs: record the consent cost of raising MAX_STAT_BONUS --- backend/src/features/inventory/catalog.ts | 13 +++++ docs/plan-battle-inventory-hardening.md | 67 ++++++++++++++++++----- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/backend/src/features/inventory/catalog.ts b/backend/src/features/inventory/catalog.ts index 363f6bba..05b3f0dc 100644 --- a/backend/src/features/inventory/catalog.ts +++ b/backend/src/features/inventory/catalog.ts @@ -55,6 +55,19 @@ export interface ItemDefinitionSeed { * * A pet's extracted attributes land in the low hundreds, so a bonus in the thousands is * a typo rather than a tuning choice. Anything inside these is the designer's call. + * + * `MAX_STAT_BONUS` carries one consequence worth knowing before raising it. A defender's + * `DefenseAuthorization` is bound to `rulesetHash`, which covers the item catalog, so + * consenting to a ruleset is consenting to the strongest loadout that catalog can express. + * Nothing else bounds an attacker's gear: the authorization's level band does not. Raising + * this therefore widens what every *future* consent implies, silently, in a constant no + * defender ever sees. Existing authorizations are safe, since editing the catalog moves + * `rulesetHash` and invalidates them, which is the intended cost of a rules change. + * + * For scale: 500 a stat across three slots is 1500, against base attributes in the low + * hundreds. The shipped catalog's largest single bonus is 45. The gap between those two + * numbers is headroom nobody has argued for, so treat a change here as a balance decision + * rather than a limit being nudged. */ const MAX_STAT_BONUS = 500; const MAX_XP_GRANT = 100_000; diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index 7c1d2b71..6d4c0915 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -6,27 +6,58 @@ is the execution order for what that review found. Each step ends at a command t Branch: `fix/battle-inventory-seam`. +## Status + +Every code item is done. What remains needs a decision or production access, not more code. + +| | Item | State | +|---|---|---| +| B1 | Sign worker rebuilt every snapshot at schema v1, so nothing settled | done | +| C1 | Unreadable catalog effect silently re-priced the ruleset | done | +| C2 | Uncatalogued equipped item fought as nothing | done | +| C3 | TS bonus sum unclamped where Go range-checks | done | +| C4 | Self-battle silently swallowed one of its own drops | done | +| C5 | Unconfirmed mint could pay an entitlement twice | done | +| C6 | Spending a consumable left the pet's own numbers stale | done | +| D1 | Consent and gear. Smaller gap than first stated | done, no schema change | +| D2 | Drops are not verifiable | claim corrected; **the tension is undecided** | +| D3 | Phase 4 ships a re-consent event | **needs a deliberate rollout** | +| Q1-Q4 | Cache reset, stranded comment, worker cast, untypechecked scripts | done | +| O1-O3 | Migration, seeder, end-to-end | **operator calls** | + +Two of these were corrections to this document rather than to the code. D1 and D2 were both +written up as bigger than they are, and in D1's case that nearly bought a permanent protocol +schema version. Where that happened it is recorded in place, because the correction is the +more useful artifact. + ## Verdict The feature is well built. Ownership boundaries are stated and held (indexer writes the projections, the seeder writes the catalog, the player signs the equip), the two live combat ports move together, the golden vectors cover the modifier ordering at a one-point margin, and -the doc comments record reasoning rather than restating code. Every suite is green: +the doc comments record reasoning rather than restating code. + +Every suite was green **as found**, which is the point worth keeping: -| Suite | Result | -|---|---| -| `pnpm --filter backend test` | 914 passed / 89 files | -| `pnpm --filter @cryptopets/protocol test` | 595 passed / 34 files | -| `pnpm --filter @cryptopets/verifier test` | 86 passed / 13 files | -| `pnpm --filter frontend test` | 370 passed / 48 files | -| `pnpm --filter @shared/core test` | 567 passed / 80 files | -| `go test ./internal/{combat,evm,store}` | ok | +| Suite | As found | After | +|---|---|---| +| `pnpm --filter backend test` | 914 passed / 89 files | 939 | +| `pnpm --filter @cryptopets/protocol test` | 595 passed / 34 files | 604 | +| `pnpm --filter @cryptopets/verifier test` | 86 passed / 13 files | 86 | +| `pnpm --filter frontend test` | 370 passed / 48 files | 370 | +| `pnpm --filter @shared/core test` | 567 passed / 80 files | 568 | +| `go test ./internal/{combat,evm,store}` | ok | ok | -The defects are concentrated at one seam: the point where a stored snapshot is read back out -of the ledger. Phase 4 gave the snapshot a schema version and an equipment list, and one of the -three readers was never updated. That reader is the signing worker, so nothing settles. +A green suite is not the same as working software, and B1 is the clean demonstration: no +battle on any deployment running that code could produce a receipt, and 914 tests passed +anyway, because the fixture and the bug shared an assumption. Every fix below is paired with a +test verified to fail without it, which is the only way that assertion means anything. -Severity ordering below is by consequence, not by size of fix. +`contracts/test-vectors/` is unchanged throughout, confirmed by diff against `main`. + +Severity ordering below is by consequence, not by size of fix. The C-numbers are in the order +found, not in severity order: C4 through C6 came out of reviewing the drop and claim paths +after the snapshot work was finished. --- @@ -432,8 +463,14 @@ schema change, so it no longer has to be sequenced against D3's re-consent; D3 i one-time cost that Phase 4 forces on its own. O1 to O3 last, because they are the only steps that touch production. -Everything above D2 is done. What remains is D2 (a tracked v1 limit, not a defect), D3 (ship -the re-consent deliberately), and the three operator steps. +All of that has landed. What remains is D2.2 (decide the verifiable-drops tension, or decide +to keep it), D3 (ship the re-consent deliberately), and the three operator steps, which want +running in that order: apply the migration, seed, then exercise it end to end. + +One note for whoever runs O3. It is the first time either web screen will be opened against +real data, and the review that produced C1 to C6 could not substitute for that: it read the +code, not the rendered page. Expect the remaining defects to be presentational, and expect +them to be found by looking rather than by reading. ## Do not touch From f803d33eefc2c957c2103adce1d122a66f2e5763 Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 15:49:34 -0400 Subject: [PATCH 10/36] feat(frontend): disclose how battle drops are decided --- backend/src/features/inventory/drops.ts | 14 +++-- docs/plan-battle-inventory-hardening.md | 62 +++++++++++++++---- .../src/components/inventory/index.module.css | 11 ++++ frontend/src/components/inventory/index.tsx | 19 ++++++ 4 files changed, 91 insertions(+), 15 deletions(-) diff --git a/backend/src/features/inventory/drops.ts b/backend/src/features/inventory/drops.ts index af4f0657..8fc2de89 100644 --- a/backend/src/features/inventory/drops.ts +++ b/backend/src/features/inventory/drops.ts @@ -21,10 +21,16 @@ import type { ItemDefinitionSeed } from './catalog'; * id are signed. Someone reading this source can reproduce a drop; someone holding only a * receipt and the published bundle cannot. * - * Nor is the payout pinned by the receipt. `rates` is an argument, so the same seed and - * battle id yield different answers under different rates, and nothing records which were - * used. The operator cannot re-roll a drop, but can change the odds it was drawn against - * without leaving a trace. + * Nor is the payout pinned by the receipt: `rates` is an argument, so the same seed and + * battle id yield different answers under different odds, and no row records which applied. + * + * Be equally precise about that, because it is easy to overstate in turn. The only + * production caller (`sign.worker`) passes no rates at all, so the odds in force are + * `DEFAULT_DROP_RATES` below, a constant that changes only by code change and deploy. Git + * history and the deployment record are a real audit trail, just not one a receipt holder + * can check. The parameter is a test seam today; it becomes the gap this paragraph + * describes only if something ever starts passing per-battle rates, which is worth a second + * look if anyone proposes it. * * Closing that means publishing the rates and the drop pool, which puts non-equipment * items into the ruleset. §4 deliberately keeps them out: a `rulesetHash` that moved every diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index 6d4c0915..fa489164 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -20,7 +20,7 @@ Every code item is done. What remains needs a decision or production access, not | C5 | Unconfirmed mint could pay an entitlement twice | done | | C6 | Spending a consumable left the pet's own numbers stale | done | | D1 | Consent and gear. Smaller gap than first stated | done, no schema change | -| D2 | Drops are not verifiable | claim corrected; **the tension is undecided** | +| D2 | Drops are not verifiable | claim corrected, v1 position taken and disclosed; **revisit at phase 04** | | D3 | Phase 4 ships a re-consent event | **needs a deliberate rollout** | | Q1-Q4 | Cache reset, stranded comment, worker cast, untypechecked scripts | done | | O1-O3 | Migration, seeder, end-to-end | **operator calls** | @@ -361,10 +361,19 @@ so neither is covered by `rulesetHash` or by anything else the receipt names. ** holding a receipt and the published bundle cannot recompute the drop at all.** Someone reading this source can; that is not the same property. -The payout is not pinned either. `rates` is a parameter, so the same seed and battle id -produce different answers under different odds, and nothing records which were used. The -anti-grinding property survives all of this and is worth keeping: the operator cannot -re-roll a committed seed. But it can change the odds that seed was drawn against. +The payout is not pinned by the receipt either. `rates` is a parameter, so the same seed and +battle id produce different answers under different odds, and no row records which applied. + +That is worth stating precisely, because the first version of this paragraph overstated it. +The only production caller passes no rates, so the odds in force are `DEFAULT_DROP_RATES`, a +constant that moves by code change and deploy. Git history is a real audit trail, just not +one a receipt holder can check. The anti-grinding property survives everything here and is +worth keeping: the operator cannot re-roll a committed seed. + +**That materially changes which option below is right.** "Operator-attested" is a much +stronger position when the attestation is a versioned constant in a public repository than +when it is a runtime value nobody records, and it is the former. The case for spending a +protocol schema version on this is correspondingly weaker than it looked. What makes this a decision rather than a fix: closing it means publishing the rates and the drop pool, which puts non-equipment items into the ruleset. §4 rules that out on purpose, and @@ -377,12 +386,43 @@ product call. `plan-inventory-items.md` §5, all three of which asserted recomputability. A false verifiability claim is worse than a documented gap: it is the kind of thing a later decision gets built on, and it nearly was here. -- [ ] **D2.2 Decide the tension, or decide to keep it.** Options, none of them free: - publish rates and pool in a *separate* digest the receipt names, so drop rules version - independently of consent; or accept that drops are operator-attested in v1 and say so - in the player-facing docs; or fold drops into the receipt and pay the consent cost. - Not scheduled. Nothing is broken today, and the honest comment is the prerequisite for - choosing well. +- [ ] **D2.2 Decide the tension, or decide to keep it.** Three options, with what each + actually costs now that the inputs are pinned down: + + 1. **Keep it, and say so.** Drops are operator-attested in v1: derived from a committed + seed the operator cannot re-roll, under odds that live in a versioned constant in a + public repository. Cost: a line in the player-facing docs. Buys no cryptographic + property, and forecloses nothing. + 2. **A separate drop-rules digest the receipt names.** Resolves the tension properly: + drop rules get their own hash and version independently of consent, so publishing + them never touches `rulesetHash`. Cost: a receipt schema version, permanently, plus + a second published artifact to serve forever (§H). + 3. **Fold drops into the receipt.** Strongest property, highest price: a receipt schema + version *and* the non-equipment catalog inside `rulesetHash`, which is the + re-consent-on-every-collectible outcome §4 explicitly rejected. + + **Taken: (1).** The gap is real but narrow, and what makes it narrow is that nobody can + grind a drop, which already holds. A permanent schema version is a poor trade for + making a constant checkable when the constant is already public. + + Chosen rather than recommended because the decision was repeatedly deferred back, and + (1) is the option that forecloses nothing: it adds disclosure and no protocol surface, + so (2) or (3) remain open at their original cost. Reverse it by deleting one tooltip. + +- [x] **D2.2a Disclose it where a player meets a drop.** A tooltip on the inventory's + "Waiting to be claimed" heading, saying both halves: the drop was fixed by public + randomness before the battle resolved and nobody can re-roll it, *and* the odds are + not checkable against a single receipt. Saying only the first would be the marketing + version of the same fact. + Verify: `pnpm --filter frontend lint:check && pnpm --filter frontend test`. +- [ ] **D2.2b Revisit at roadmap phase 04, not "eventually".** The condition that changes + this answer is a drop being worth money to someone other than the player who earned + it, and that is already scheduled: `landing.ts` lists a "Pet and item marketplace" in + phase 04, and the FAQ already tells players their items are tradable assets. Once an + item has a market price, "trust the constant in our repo" stops being proportionate + and option (2) is worth its schema version. Worth deciding *before* the marketplace + ships rather than after, since receipts signed in between are the ones that cannot be + upgraded. ## D3: shipping Phase 4 is a re-consent event diff --git a/frontend/src/components/inventory/index.module.css b/frontend/src/components/inventory/index.module.css index 1012abff..901f0595 100644 --- a/frontend/src/components/inventory/index.module.css +++ b/frontend/src/components/inventory/index.module.css @@ -288,6 +288,17 @@ margin-top: 0; } +/* Inline with the heading rather than absolutely placed like `.help`, which anchors to a + tile corner. The heading is uppercase and letter-spaced, so the glyph needs its own + line-height to sit on the text baseline instead of riding above it. */ +.headingHelp { + display: inline-flex; + align-items: center; + margin-left: 6px; + line-height: 0; + vertical-align: middle; +} + .pendingList { display: flex; flex-direction: column; diff --git a/frontend/src/components/inventory/index.tsx b/frontend/src/components/inventory/index.tsx index 5bf16ba3..7d7aef85 100644 --- a/frontend/src/components/inventory/index.tsx +++ b/frontend/src/components/inventory/index.tsx @@ -367,6 +367,25 @@ const Inventory: React.FC = () => {

Waiting to be claimed + {/* Where a player actually meets a drop, so it is where the + honest version of how one is decided belongs. Both halves + matter: nobody can re-roll a drop, and the odds are not + checkable against a single battle. Saying only the first + would be the marketing version. */} + + +

+ A battle drop is decided by the same public randomness that + decided the fight, fixed before the battle resolved. Nobody, + including us, can re-roll one. +

+

+ The drop odds live in our open-source code rather than being + published with each battle, so you can read them, but you + cannot check them against one receipt on your own. +

+ +

{/* Its own strip above the bag, because these are not items yet: claiming is what mints them, and until then there is nothing on From c8aebfd9171bf16fe9acb0c3b5ce1b0b1623a3a7 Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 15:58:07 -0400 Subject: [PATCH 11/36] feat(backend): let a defender read their own consent state --- backend/API.md | 5 +- .../battle/ledger/consent.controller.ts | 34 ++++++++++ .../features/battle/ledger/consent.service.ts | 67 ++++++++++++++++++ backend/src/features/battle/ledger/index.ts | 8 ++- backend/src/routes/battle.ts | 5 ++ .../battle/ledger/consent.service.test.ts | 68 +++++++++++++++++++ docs/plan-battle-inventory-hardening.md | 32 ++++++++- 7 files changed, 216 insertions(+), 3 deletions(-) diff --git a/backend/API.md b/backend/API.md index 79cc0f8b..ac779700 100644 --- a/backend/API.md +++ b/backend/API.md @@ -419,12 +419,15 @@ to check independently. The four write routes are gated on `BATTLE_BACKEND_MODE_ENABLED` and return **503 `backend-battle-mode-disabled`** when it is off. Every read below stays served regardless: switching the mode off stops new battles, it does not retract receipts already issued. -`DELETE /authorizations` is ungated too, since withdrawing consent must keep working. +`DELETE /authorizations` is ungated too, since withdrawing consent must keep working, and +`GET /authorizations` for the same reason: a defender needs to see that their consent went +stale precisely when something is off, and a mode flag should not be what hides it. | POST | `/api/battle/intents` | JWT | Submit a signed battle intent (§D). | | POST | `/api/battle/intents/:intentHash/accept` | JWT | Freeze the snapshot, commit to a future drand round, sign the commitment, and return it synchronously (§E). | | POST | `/api/battle/authorizations` | JWT | Submit a signed standing defence authorization (§D). | | DELETE | `/api/battle/authorizations?chainId=` | JWT | Revoke every live authorization for the caller on one chain. No wallet signature required — refusing battles is never the dangerous direction. | +| GET | `/api/battle/authorizations?chainId=` | JWT | The caller's own live authorizations, plus the `rulesetHash` now being served. Each carries `isStale`, true when it was signed under a different ruleset and therefore covers no battle. Always scoped to the authenticated wallet, never to a queried address: one wallet's consent state says which of their pets can be challenged and until when. | | GET | `/api/battle/config` | none | The `deploymentId`, served `chainIds`, and active ruleset a client needs *before* it can build a signable intent. None of it is derivable client-side, and guessing it fails only after the wallet prompt: a wrong deployment is refused as `wrong-deployment`, a wrong ruleset produces an authorization no battle matches. | | GET | `/api/battle/:battleId` | none | Battle state summary: state, failure reason, both pets, ruleset hash. | | GET | `/api/battle/:battleId/commitment` | none | The signed commitment, exactly as delivered at accept time — the re-fetch path if a client's local copy was lost. | diff --git a/backend/src/features/battle/ledger/consent.controller.ts b/backend/src/features/battle/ledger/consent.controller.ts index dd39180c..c6cd87cd 100644 --- a/backend/src/features/battle/ledger/consent.controller.ts +++ b/backend/src/features/battle/ledger/consent.controller.ts @@ -1,3 +1,4 @@ +import { hashRuleset } from '@cryptopets/protocol'; import type { Response } from 'express'; import type { AuthenticatedRequest } from '@middleware/auth'; @@ -5,10 +6,12 @@ import type { AuthenticatedRequest } from '@middleware/auth'; import { type AuthorizationRejection, type DefenseAuthorizationWire, + listDefenseAuthorizations, revokeDefenseAuthorizations, submitDefenseAuthorization, } from './consent.service'; import type { SignatureFormat } from './intent.service'; +import { servedRuleset } from './ruleset.builder'; const STATUS_BY_REASON: Record = { 'malformed-authorization': 422, @@ -76,3 +79,34 @@ export async function deleteDefenseAuthorizations(req: AuthenticatedRequest, res const { revoked } = await revokeDefenseAuthorizations(chainId, wallet, new Date()); res.status(200).json({ revoked }); } + +/** + * The caller's own live authorizations, each flagged with whether it still applies. + * + * Always the authenticated wallet, never an argument. One wallet's consent state says + * which of their pets can be challenged and until when, which is theirs to see and nobody + * else's to enumerate. + * + * `isStale` is the field this exists for. A rules change invalidates every outstanding + * grant by design, and a defender is the one who has to re-sign but the last to find out: + * being challenged is passive, so their pets just stop being challengeable and only the + * attacker sees an error. + */ +export async function getDefenseAuthorizations(req: AuthenticatedRequest, res: Response): Promise { + const wallet = req.user?.address; + if (!wallet) { + res.status(401).json({ error: 'authentication required' }); + return; + } + const chainId = typeof req.query.chainId === 'string' ? req.query.chainId : undefined; + if (!chainId) { + res.status(422).json({ error: 'chainId is required' }); + return; + } + + // The hash battles are actually being accepted under, from the same builder `accept` + // uses, so "stale" here means exactly what it means there rather than approximately. + const rulesetHash = hashRuleset(await servedRuleset()); + const authorizations = await listDefenseAuthorizations(chainId, wallet, rulesetHash); + res.status(200).json({ rulesetHash, authorizations }); +} diff --git a/backend/src/features/battle/ledger/consent.service.ts b/backend/src/features/battle/ledger/consent.service.ts index 1e317a1a..2c3dc83e 100644 --- a/backend/src/features/battle/ledger/consent.service.ts +++ b/backend/src/features/battle/ledger/consent.service.ts @@ -187,6 +187,73 @@ export async function revokeDefenseAuthorizations( return { revoked: count }; } +/** One of the caller's own authorizations, as the read surface presents it. */ +export interface DefenseAuthorizationSummary { + authorizationHash: string; + allPets: boolean; + petIds: string[]; + minLevel: number; + maxLevel: number; + maxBattlesPerDay: number; + /** Unix seconds. */ + notBefore: number; + expiresAt: number; + rulesetHash: string; + /** + * Whether this authorization still covers battles under the rules now being served. + * + * Computed rather than stored, because it is a comparison against a value that moves: + * a grant is signed over one `rulesetHash`, and a rules change makes it cover nothing + * without touching the row. + */ + isStale: boolean; + createdAt: string; +} + +/** + * Every live authorization the caller has granted, and whether each still applies. + * + * The missing third of the consent API. Granting and revoking both existed; reading did + * not, so a defender had no way to learn they had consented, let alone that their consent + * had gone stale. That matters because a rules change invalidates every outstanding grant + * by design, and being challenged is *passive*: the defender never finds out by trying + * something and failing. Their pets simply stop being challengeable, silently, and the + * only person who sees an error is the attacker, who cannot fix it. + * + * Revoked rows are excluded rather than returned with a flag. A revocation is the owner + * deciding this grant no longer exists, and the row survives only so a verifier can still + * read what a historical receipt was authorized under, which is not this caller's question. + */ +export async function listDefenseAuthorizations( + chainId: string, + defenderOwner: string, + servedRulesetHash: string, +): Promise { + const rows = await prisma.defenseAuthorization.findMany({ + where: { + chainId, + deploymentId: servedDeploymentId(), + defenderOwner: normalizeAccount(defenderOwner), + revokedAt: null, + }, + orderBy: { createdAt: 'desc' }, + }); + + return rows.map((row) => ({ + authorizationHash: row.authorizationHash, + allPets: row.allPets, + petIds: Array.isArray(row.petIds) ? (row.petIds as string[]) : [], + minLevel: row.minLevel, + maxLevel: row.maxLevel, + maxBattlesPerDay: row.maxBattlesPerDay, + notBefore: Number(row.notBefore), + expiresAt: Number(row.expiresAt), + rulesetHash: row.rulesetHash, + isStale: row.rulesetHash.toLowerCase() !== servedRulesetHash.toLowerCase(), + createdAt: row.createdAt.toISOString(), + })); +} + /** What a battle needs an authorization to permit. */ export interface CoverageRequest { chainId: string; diff --git a/backend/src/features/battle/ledger/index.ts b/backend/src/features/battle/ledger/index.ts index abc0a79d..c216869e 100644 --- a/backend/src/features/battle/ledger/index.ts +++ b/backend/src/features/battle/ledger/index.ts @@ -15,7 +15,11 @@ export { type SequencePage, } from './corpus.service'; export { getReceiptsByPet, getReceiptsBySequence, getReceiptsByWallet } from './corpus.controller'; -export { deleteDefenseAuthorizations, postDefenseAuthorization } from './consent.controller'; +export { + deleteDefenseAuthorizations, + getDefenseAuthorizations, + postDefenseAuthorization, +} from './consent.controller'; export { getBattleCombatLog, getBattleCommitment, @@ -51,9 +55,11 @@ export { type ConsentResult, consumeDailyBudget, type CoverageRequest, + type DefenseAuthorizationSummary, type DefenseAuthorizationWire, epochDay, findCoveringAuthorization, + listDefenseAuthorizations, revokeDefenseAuthorizations, type SubmitAuthorizationRequest, type SubmitAuthorizationResult, diff --git a/backend/src/routes/battle.ts b/backend/src/routes/battle.ts index b88d8f04..0ccde903 100644 --- a/backend/src/routes/battle.ts +++ b/backend/src/routes/battle.ts @@ -2,6 +2,7 @@ import express, { Router } from 'express'; import { deleteDefenseAuthorizations, + getDefenseAuthorizations, getBattleCombatLog, getBattleCommitment, getBattleConfigHandler, @@ -42,6 +43,10 @@ router.post('/authorizations', requireBackendBattleMode, verifyToken, battleRoom // Revocation is not gated: withdrawing consent must keep working even after the mode is // switched off, since refusing battles is never the dangerous direction. router.delete('/authorizations', verifyToken, deleteDefenseAuthorizations); +// Reading is ungated for the same reason. A defender needs to see that their consent went +// stale precisely when something is off, and a mode flag should not be what hides it. +// Scoped to the authenticated wallet in the controller, never to a queried address. +router.get('/authorizations', verifyToken, getDefenseAuthorizations); // Authoritative, re-fetchable reads (§J). No auth: every value here is either already // public on chain or is itself a signed artifact anyone is meant to check, so gating diff --git a/backend/tests/features/battle/ledger/consent.service.test.ts b/backend/tests/features/battle/ledger/consent.service.test.ts index 93c02c20..6993d758 100644 --- a/backend/tests/features/battle/ledger/consent.service.test.ts +++ b/backend/tests/features/battle/ledger/consent.service.test.ts @@ -22,6 +22,7 @@ import { consumeDailyBudget, epochDay, findCoveringAuthorization, + listDefenseAuthorizations, revokeDefenseAuthorizations, submitDefenseAuthorization, toProtocolAuthorization, @@ -350,3 +351,70 @@ describe('consumeDailyBudget', () => { await expect(consumeDailyBudget('0xabc', 20, NOW)).rejects.toThrow(/connection reset/); }); }); + +/** + * The read half of the consent API (§D). Granting and revoking both existed; reading did + * not, so a defender could not see that they had consented, nor that a rules change had + * quietly made their consent cover nothing. + */ +describe('listDefenseAuthorizations', () => { + const SERVED = `0x${'11'.repeat(32)}`; + const OLD = `0x${'22'.repeat(32)}`; + + const row = (overrides: Record = {}) => ({ + authorizationHash: `0x${'ab'.repeat(32)}`, + allPets: true, + petIds: [], + minLevel: 1, + maxLevel: 100, + maxBattlesPerDay: 50, + notBefore: 1000n, + expiresAt: 2000n, + rulesetHash: SERVED, + createdAt: new Date('2026-08-01T00:00:00.000Z'), + ...overrides, + }); + + it('flags an authorization signed under the rules now being served as current', async () => { + vi.mocked(prisma.defenseAuthorization.findMany).mockResolvedValue([row()] as never); + + const [entry] = await listDefenseAuthorizations('eip155:84532', '0xABC', SERVED); + + expect(entry!.isStale).toBe(false); + expect(entry!.notBefore).toBe(1000); + expect(entry!.expiresAt).toBe(2000); + }); + + // The field this read exists for. A rules change invalidates every outstanding grant by + // design, and the defender is the one who has to re-sign but the last to notice: being + // challenged is passive, so their pets just stop being challengeable. + it('flags an authorization signed under older rules as stale', async () => { + vi.mocked(prisma.defenseAuthorization.findMany).mockResolvedValue([row({ rulesetHash: OLD })] as never); + + const [entry] = await listDefenseAuthorizations('eip155:84532', '0xABC', SERVED); + + expect(entry!.isStale).toBe(true); + }); + + it('compares the ruleset hash case-insensitively', async () => { + vi.mocked(prisma.defenseAuthorization.findMany).mockResolvedValue([ + row({ rulesetHash: SERVED.toUpperCase().replace('0X', '0x') }), + ] as never); + + expect((await listDefenseAuthorizations('eip155:84532', '0xABC', SERVED))[0]!.isStale).toBe(false); + }); + + it('normalizes the caller and excludes revoked grants', async () => { + vi.mocked(prisma.defenseAuthorization.findMany).mockResolvedValue([] as never); + + await listDefenseAuthorizations('eip155:84532', '0xABCDEF0123456789ABCDEF0123456789ABCDEF01', SERVED); + + const { where } = vi.mocked(prisma.defenseAuthorization.findMany).mock.calls.at(-1)![0]!; + expect(where).toMatchObject({ + defenderOwner: '0xabcdef0123456789abcdef0123456789abcdef01', + // Revoked rows survive so a verifier can read what a historical receipt was + // authorized under, which is not this caller's question. + revokedAt: null, + }); + }); +}); diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index fa489164..93f484bb 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -21,7 +21,8 @@ Every code item is done. What remains needs a decision or production access, not | C6 | Spending a consumable left the pet's own numbers stale | done | | D1 | Consent and gear. Smaller gap than first stated | done, no schema change | | D2 | Drops are not verifiable | claim corrected, v1 position taken and disclosed; **revisit at phase 04** | -| D3 | Phase 4 ships a re-consent event | **needs a deliberate rollout** | +| C7 | No way to tell a defender their consent had gone stale | read surface done; **UI outstanding** | +| D3 | Phase 4 ships a re-consent event | **needs a deliberate rollout**, and C7.2 first | | Q1-Q4 | Cache reset, stranded comment, worker cast, untypechecked scripts | done | | O1-O3 | Migration, seeder, end-to-end | **operator calls** | @@ -424,6 +425,35 @@ product call. ships rather than after, since receipts signed in between are the ones that cannot be upgraded. +## C7: nobody could tell a defender their consent had gone stale + +Found while working out what D3 needs to ship well, which is the point of doing that before +the rollout rather than during it. + +The consent API had `POST /authorizations` and `DELETE /authorizations` and no `GET`. A +defender could grant consent and revoke it, and could not read it. So there was no way to +answer "have I consented?", and more importantly no way to answer "does my consent still +apply?" + +D3 invalidates every outstanding authorization at once. Without a read, here is what a +defender experiences: nothing. Being challenged is *passive*, so their pets simply stop +being challengeable and no screen they visit says otherwise. The only person who sees an +error is the attacker, who gets a clear message (`battleFailureMessage.ts` renders +`ruleset-mismatch` as "This opponent's consent was signed under older rules") and can do +nothing about it. The one person who can fix it is the one person not told. + +- [x] **C7.1** Add `GET /api/battle/authorizations?chainId=`, returning the caller's live + grants plus the `rulesetHash` now being served, each flagged `isStale` when it was + signed under a different one. Ungated like `DELETE`, and for the same reason: a + defender needs to see this precisely when something is off, so a mode flag should not + be what hides it. Always scoped to the authenticated wallet, never a queried address. + Verify: `pnpm --filter backend exec vitest run tests/features/battle/ledger/consent.service.test.ts`. +- [ ] **C7.2 Surface it.** The endpoint makes the state knowable; a screen has to make it + visible. The natural home is wherever `useDefenseAuthorization` is already offered, + showing a stale grant with a re-sign prompt rather than a silent absence. Not built + here, because where it belongs is a UI decision and this had already reached the edge + of what the review could settle on its own. + ## D3: shipping Phase 4 is a re-consent event Already recorded in `plan-inventory-items.md`. `ENGINE_VERSION` 1 to 2 plus the ruleset's item From e7ad2d7095928ef95197e3f98fbd3f0fa43070f0 Mon Sep 17 00:00:00 2001 From: heyradcode Date: Mon, 10 Aug 2026 16:11:57 -0400 Subject: [PATCH 12/36] feat(frontend): show a defender whether their consent still applies --- docs/plan-battle-inventory-hardening.md | 35 +++-- .../panels/defense/index.module.css | 36 +++++ .../pet/interactions/panels/defense/index.tsx | 37 +++++- shared/src/hooks/battle/chainIdFor.ts | 23 ++++ .../hooks/battle/useDefenseAuthorization.ts | 12 +- .../hooks/battle/useDefenseAuthorizations.ts | 124 ++++++++++++++++++ .../src/hooks/battle/useSubmitBattleIntent.ts | 12 +- shared/src/hooks/index.ts | 6 + .../hooks/useDefenseAuthorizations.test.tsx | 115 ++++++++++++++++ 9 files changed, 368 insertions(+), 32 deletions(-) create mode 100644 shared/src/hooks/battle/chainIdFor.ts create mode 100644 shared/src/hooks/battle/useDefenseAuthorizations.ts create mode 100644 shared/tests/hooks/useDefenseAuthorizations.test.tsx diff --git a/docs/plan-battle-inventory-hardening.md b/docs/plan-battle-inventory-hardening.md index 93f484bb..9a5b46b1 100644 --- a/docs/plan-battle-inventory-hardening.md +++ b/docs/plan-battle-inventory-hardening.md @@ -21,8 +21,8 @@ Every code item is done. What remains needs a decision or production access, not | C6 | Spending a consumable left the pet's own numbers stale | done | | D1 | Consent and gear. Smaller gap than first stated | done, no schema change | | D2 | Drops are not verifiable | claim corrected, v1 position taken and disclosed; **revisit at phase 04** | -| C7 | No way to tell a defender their consent had gone stale | read surface done; **UI outstanding** | -| D3 | Phase 4 ships a re-consent event | **needs a deliberate rollout**, and C7.2 first | +| C7 | No way to tell a defender their consent had gone stale | done | +| D3 | Phase 4 ships a re-consent event | **needs a deliberate rollout** | | Q1-Q4 | Cache reset, stranded comment, worker cast, untypechecked scripts | done | | O1-O3 | Migration, seeder, end-to-end | **operator calls** | @@ -42,11 +42,11 @@ Every suite was green **as found**, which is the point worth keeping: | Suite | As found | After | |---|---|---| -| `pnpm --filter backend test` | 914 passed / 89 files | 939 | +| `pnpm --filter backend test` | 914 passed / 89 files | 943 | | `pnpm --filter @cryptopets/protocol test` | 595 passed / 34 files | 604 | | `pnpm --filter @cryptopets/verifier test` | 86 passed / 13 files | 86 | | `pnpm --filter frontend test` | 370 passed / 48 files | 370 | -| `pnpm --filter @shared/core test` | 567 passed / 80 files | 568 | +| `pnpm --filter @shared/core test` | 567 passed / 80 files | 574 | | `go test ./internal/{combat,evm,store}` | ok | ok | A green suite is not the same as working software, and B1 is the clean demonstration: no @@ -448,11 +448,28 @@ nothing about it. The one person who can fix it is the one person not told. defender needs to see this precisely when something is off, so a mode flag should not be what hides it. Always scoped to the authenticated wallet, never a queried address. Verify: `pnpm --filter backend exec vitest run tests/features/battle/ledger/consent.service.test.ts`. -- [ ] **C7.2 Surface it.** The endpoint makes the state knowable; a screen has to make it - visible. The natural home is wherever `useDefenseAuthorization` is already offered, - showing a stale grant with a re-sign prompt rather than a silent absence. Not built - here, because where it belongs is a UI decision and this had already reached the edge - of what the review could settle on its own. +- [x] **C7.2 Surface it.** `useDefenseAuthorizations` in `@shared/core` reads the endpoint + and collapses it to one `ConsentStatus`, and `DefensePanel` states it above the + controls, because it changes what they mean: signing again after a rules change is a + repair, not a duplicate, and a player who cannot see the difference reads the same + button two ways. + + Three states, not two. `stale` is deliberately distinct from `none`: they ask the same + action of the player but are not the same message, and "you have not allowed + challenges" shown to someone who did reads as the app having forgotten. `unknown` + covers not-yet-loaded and no-wallet, so a disconnected visitor is never told nobody can + battle their pets, which would be a false statement about their account. `active` wins + whenever any grant is current, since a defender holding one usable authorization and + three superseded ones is covered and should not be told to re-sign. + + Both writes call `refresh()`, or the banner would contradict the success line directly + beneath it. + Verify: `pnpm --filter @shared/core exec vitest run tests/hooks/useDefenseAuthorizations.test.tsx`. + +Extracted while doing it: `chainIdFor` was written out identically in +`useDefenseAuthorization` and `useSubmitBattleIntent`, and this needed a third copy. It now +lives in `hooks/battle/chainIdFor.ts`. Harmless duplication right up until a deployment +serves two chains of one family and only one caller learns how to choose. ## D3: shipping Phase 4 is a re-consent event diff --git a/frontend/src/components/pet/interactions/panels/defense/index.module.css b/frontend/src/components/pet/interactions/panels/defense/index.module.css index ceea6e47..68718092 100644 --- a/frontend/src/components/pet/interactions/panels/defense/index.module.css +++ b/frontend/src/components/pet/interactions/panels/defense/index.module.css @@ -41,3 +41,39 @@ color: rgb(251 113 133); font-size: 0.85rem; } + +/* Current consent state, stated before the controls because it changes what they mean. + Three tones rather than one: a stale grant is a problem to repair, no grant is a choice + not yet made, and an active grant is reassurance. Rendering all three the same way would + make the one that needs acting on look like the two that do not. */ +.status { + margin: 0 0 14px; + padding: 10px 12px; + border-radius: 10px; + border: 1px solid; + font-size: 0.85rem; + line-height: 1.5; +} + +/* Amber, not red: nothing is broken and nothing was lost. The grant did exactly what it + promised by expiring with the rules it was signed under. */ +.stale { + composes: status; + border-color: rgb(251 191 36 / 45%); + background: rgb(251 191 36 / 8%); + color: rgb(251 191 36 / 92%); +} + +.active { + composes: status; + border-color: rgb(52 211 153 / 35%); + background: rgb(52 211 153 / 8%); + color: rgb(52 211 153 / 92%); +} + +.inactive { + composes: status; + border-color: rgb(148 163 184 / 30%); + background: rgb(148 163 184 / 8%); + opacity: 0.85; +} diff --git a/frontend/src/components/pet/interactions/panels/defense/index.tsx b/frontend/src/components/pet/interactions/panels/defense/index.tsx index 733c7e3e..4e37bc7a 100644 --- a/frontend/src/components/pet/interactions/panels/defense/index.tsx +++ b/frontend/src/components/pet/interactions/panels/defense/index.tsx @@ -1,6 +1,11 @@ import React, { useState } from 'react'; import NeonButton from '@components/ui/neon-button'; -import { useChainCapabilities, useDefenseAuthorization, usePetList } from '@shared/core'; +import { + useChainCapabilities, + useDefenseAuthorization, + useDefenseAuthorizations, + usePetList, +} from '@shared/core'; import { useNotifyError } from '@hooks/useNotifyError'; import Icon, { CheckIcon } from '@components/ui/icon'; import { Tones } from '@constants/tones'; @@ -23,6 +28,10 @@ const DefensePanel: React.FC = ({ isStandaloneView = true }) const { pets } = usePetList(); const notifyError = useNotifyError(); const { grant, revoke, isPending, error } = useDefenseAuthorization(); + // What the panel could not say before: whether consent exists, and whether it still + // applies. A rules change invalidates every grant by design, and being challenged is + // passive, so without this a defender's pets go quiet and nothing here admits it. + const { status, refresh } = useDefenseAuthorizations(); const [allPets, setAllPets] = useState(true); const [selected, setSelected] = useState([]); @@ -39,6 +48,10 @@ const DefensePanel: React.FC = ({ isStandaloneView = true }) setSuccess(null); const hash = await grant(allPets ? { allPets: true } : { petIds: selected }); if (hash) { + // Both writes re-read: the status banner is the only thing that says whether the + // grant took, so leaving it on a cached answer would contradict the success line + // directly underneath it. + refresh(); setSuccess( allPets ? 'Every pet you own can now be challenged.' @@ -50,6 +63,7 @@ const DefensePanel: React.FC = ({ isStandaloneView = true }) const handleRevoke = async () => { setSuccess(null); if (await revoke()) { + refresh(); setSuccess('Consent withdrawn. Your pets can no longer be challenged.'); } }; @@ -66,6 +80,27 @@ const DefensePanel: React.FC = ({ isStandaloneView = true }) )} + {/* Above the controls, because it changes what the buttons mean. Signing + again when a grant went stale is a repair, not a duplicate, and a player + who cannot see the difference reads the same button two ways. */} + {status.kind === 'stale' && ( +

+ The battle rules changed since you allowed challenges, so your consent + no longer covers anything and your pets cannot be challenged. Allow + challenges again to restore it. +

+ )} + {status.kind === 'active' && ( +

+ Your pets can be challenged under the current rules. +

+ )} + {status.kind === 'none' && ( +

+ You have not allowed challenges, so nobody can battle your pets. +

+ )} +