fix(escrow): FK integrity + sponsor dashboard figures survive parent deletion - #33
Merged
chonilius merged 13 commits intoJul 17, 2026
Conversation
…is deleted Escrow.bounty/milestone/maintenancePool were all onDelete: 'CASCADE', so deleting a Bounty/Milestone/MaintenancePool silently deleted its Escrow row too — including funds still LOCKED on-chain. Change all three to 'SET NULL': the parent link is cleared but the escrow ledger row (and the real money it represents) survives. Also narrow Bounty.escrow's ORM-level `cascade: true` to ['insert', 'update'] so removing a Bounty entity via the ORM (not just raw SQL) doesn't separately force a cascaded escrow removal either. Part of MergeFi#27.
… is deleted Payment.escrow was onDelete: 'CASCADE'. A Payment is a record of money that already moved (a confirmed payout leg); deleting its parent Escrow must never silently delete that record too. Change to 'RESTRICT' so the database refuses the delete instead. Part of MergeFi#27.
Escrow now carries a denormalized sponsorId (see previous commit), but nothing populated it. Add sponsorId to FundEscrowInput and store it on the created row in EscrowService.fund, defaulting to null for funding paths that don't have a sponsor concept (maintenance pools). Part of MergeFi#27.
…fund Wires BountiesService.fund's already-available bounty.sponsorId into the new FundEscrowInput.sponsorId field, so bounty-funded escrows are correctly attributed to their sponsor independent of the bounty row. Part of MergeFi#27.
…vice.fund Same fix as the previous BountiesService commit, applied to MilestonesService.fund. Also adds milestones.service.spec.ts, which didn't exist before, covering the fund() path including the null- sponsorId case (milestones don't require a sponsor). Part of MergeFi#27.
…e Escrow/Payment ledger directly budgetLocked and totalSpend summed Bounty.amount filtered by Bounty.status — a hand-maintained workflow-state proxy for the real money figures, and one that only ever considered bounty-funded escrows (milestone budgets were invisible to the dashboard entirely). Worse, this proxy silently keeps "working" even after an escrow's underlying funds are stranded by a deleted parent, and silently goes to zero if a bounty itself is deleted — neither reflects reality. Rewrite both to query Escrow (budgetLocked, status = LOCKED) and Payment (totalSpend, status = CONFIRMED) directly, filtered by the denormalized escrow.sponsorId. This is the actual ledger source of truth per MergeFi#27's requirements, and it's now robust to the parent bounty/milestone being deleted since sponsorId doesn't depend on that join. Also fix recentPayments' query, which inner-joined payment -> escrow -> bounty and so silently excluded any milestone- funded payment (escrow.bounty is never set for those) and any payment whose escrow's parent bounty was later deleted. Join on escrow.sponsorId instead, same fix applied consistently. Adds sponsors.service.spec.ts (didn't exist before) covering the query construction for all three. Part of MergeFi#27.
The project had no migration history at all — schema was managed entirely by synchronize, with no way to make a reviewable, revertible schema change (see MergeFi#27's Difficulty Justification). Add a CLI DataSource (src/database/data-source.ts, reusing the same entities list app.module.ts's TypeOrmModule.forRootAsync uses) and migration:generate/create/run/revert npm scripts, following the standard TypeORM CLI setup. Part of MergeFi#27.
Ships the schema changes from the preceding commits as an actual
migration for any environment that already has data (rather than
relying on a fresh synchronize):
- Adds escrows.sponsorId, backfilled from each row's existing parent
bounty/milestone's sponsorId.
- Re-points escrows.bountyId/milestoneId/maintenancePoolId's FKs
from ON DELETE CASCADE to SET NULL, and payments.escrowId's from
CASCADE to RESTRICT.
- Adds the CHK_escrow_exactly_one_parent CHECK constraint.
The FK-replacement helper looks up each constraint's actual name via
pg_constraint/pg_attribute instead of hardcoding TypeORM's
auto-generated (content-hashed) name, so it works regardless of which
environment's synchronize run originally created it.
Part of MergeFi#27.
Every other .spec.ts in this repo mocks its repositories, which can't exercise actual database-level FK/CHECK constraint behavior — the whole subject of MergeFi#27. This connects to a real Postgres (DATABASE_URL, matching CI's postgres service / docker-compose's db service) with synchronize: true to build the schema straight from the entity decorators, then verifies: - CHK_escrow_exactly_one_parent rejects an all-null parent and a multiply-set parent (against genuinely existing rows, so the only possible failure is the CHECK, not an incidental FK violation on a dangling id), and allows exactly-one-set. - Deleting a bounty SET NULLs its escrow's bountyId rather than deleting the escrow row; the escrow's amount/status survive unchanged. - Deleting an escrow that still has payment records is RESTRICTed. - SponsorsService.budgetLocked/totalSpend, wired to real repositories, keep returning the correct figure for a sponsor after the bounty funding/paying it is deleted — the MergeFi#27 acceptance criterion this whole change exists to satisfy. Part of MergeFi#27.
Updates the Roadmap's 'wire up TypeORM migrations' item to done, adds a Migrations section with the migration:* npm script usage, and notes the new sponsors.service.spec.ts / real-Postgres integration test in the test coverage list. Part of MergeFi#27.
|
@Temi-suwa18 is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
…y-one CI caught this: the exactly-one CHECK constraint and ON DELETE SET NULL are fundamentally incompatible. Deleting a bounty drives its escrow's bountyId to NULL — the exact scenario the SET NULL change exists to support — which correctly leaves zero parents set. An "exactly one" CHECK rejects that update outright, so the SET NULL itself was failing with a constraint violation: QueryFailedError: new row for relation "escrows" violates check constraint "CHK_escrow_exactly_one_parent" Relax the constraint (and matching migration) to "at most one" (0 or 1, never 2+), renamed CHK_escrow_at_most_one_parent. Zero parents is now a valid DB-level state — it's what a legitimately orphaned escrow looks like after its parent is deleted. "Exactly one" is still enforced, just at the layer that can actually tell the difference between "never had a parent" and "orphaned by deletion": application-side, in EscrowService (next commit).
…rvice Companion to the previous commit's CHECK-constraint relaxation: since the DB no longer forbids zero parents (it has to, for orphaned rows), a newly-created escrow with zero or multiple parents would otherwise slip through uncaught. Add assertExactlyOneParent to EscrowService.fund's validation chain, and tests for both the zero-parent and multiple-parent rejection cases.
Renames the describe block and regex matchers to CHK_escrow_at_most_one_parent, removes the now-invalid "rejects zero parents" DB-level test (zero is valid at the DB layer — see the entity's doc comment), and adds a test explicitly asserting zero parents is allowed at the DB level, since that's exactly the state the SET NULL tests below it produce.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
closes #27.
Traced
src/sponsors/sponsors.service.ts's "budget locked in escrow" and "total spend" figures back to their actual source of truth and fixed every data-integrity gap the issue called out:1.
Escrow's parent FKs wereonDelete: 'CASCADE'bounty/milestone/maintenancePoolwere all CASCADE, andBounty.escrowalso had ORM-levelcascade: true. Deleting aBountyrow would silently delete itsEscrowrow — even ifLOCKEDwith real on-chain funds — and that cascaded again into deletingPaymentrows (Payment.escrowwas also CASCADE), destroying confirmed payout records. Changed toSET NULL(escrow parents) andRESTRICT(payment→escrow, since a payment is money that already moved and must never disappear because its escrow got cleaned up).2. No constraint enforcing "exactly one of bountyId/milestoneId/maintenancePoolId"
Added
@Check('CHK_escrow_exactly_one_parent', ...)on theEscrowentity, shipped via an actual migration (see below).3. Dashboard figures were a drifting proxy, not the ledger
budgetLocked/totalSpendsummedBounty.amountfiltered byBounty.status— a hand-maintained workflow-state proxy that (a) only ever considered bounty-funded escrows, silently ignoring milestone budgets, and (b) depends on the parentBountyrow surviving, so it would go to zero the instant a bounty was deleted regardless of the escrow's real state. Rewrote both to queryEscrow/Paymentdirectly. To make that survive parent deletion, added a denormalizedsponsorIdcolumn onEscrow(populated at fund time from the bounty/milestone's sponsor) that dashboard queries key off instead of joining through the deletable parent.recentPaymentshad the sameescrow.bounty.sponsorIdjoin bug (also silently excluding milestone-funded payments entirely) — fixed the same way.4. No migration tooling existed at all
The project relied entirely on
synchronize, with zero migration history. Added a CLIDataSource(src/database/data-source.ts),migration:generate/create/run/revertnpm scripts, and the actual first migration (backfillssponsorIdfor existing rows, re-points the FKON DELETEactions by looking up each constraint's real name viapg_constraint/pg_attributerather than hardcoding TypeORM's generated name, and adds the CHECK constraint).Test plan
npx jest(unit, mocked repos) — 60/60 passing, including newsponsors.service.spec.ts,milestones.service.spec.ts, and sponsorId-passthrough coverage inescrow.service.spec.ts/bounties.service.spec.tsnpm run lint— cleannpm run build— cleannpx tsc --noEmit— cleansrc/database/escrow-fk-integrity.integration.spec.ts— a real Postgres integration test (not mocked; this bug is inherently DB-level and can't be exercised by mocked repositories) covering: the CHECK constraint rejecting all-null and multiply-set parents (against genuinely existing rows, so failures can't be misattributed to a coincidental FK violation) and allowing exactly-one; a deleted bountySET NULLs its escrow instead of deleting the row; deleting an escrow with payment records isRESTRICTed; and — the issue's core acceptance criterion —SponsorsService.budgetLocked/totalSpend, wired to real repositories, keep returning the correct figure after the sponsoring bounty is deleted.I could not run this integration test locally — no Docker/Postgres is available in my environment — so I could not execute it against a live database myself. It's written to run against CI's existing
postgres:16-alpineservice (same asdocker-compose.yml'sdbservice, whichnpm run test/npm run test:covalready target perci.yml), so it will run automatically as part of the existing CI test steps. I reviewed the SQL and TypeORM calls carefully, but please pay extra attention to this file in review since I couldn't self-verify it end-to-end.