Merge pull request #33 from Temi-suwa18/fix/escrow-fk-integrity-and-sponsor-dashboard

fix(escrow): FK integrity + sponsor dashboard figures survive parent deletion

Author: chonilius

README.md

@@ -262,12 +262,29 @@ Unit tests cover critical domains including:
 - `src/github/webhook-signature.util.spec.ts` — GitHub webhook HMAC-SHA256 signature verification.
 - `src/github/github-webhooks.service.spec.ts` — webhook-to-escrow release logic.
 - `src/bounties/bounties.service.spec.ts` — bounty core management.
+- `src/sponsors/sponsors.service.spec.ts` — sponsor dashboard aggregate queries (budgetLocked/totalSpend read the Escrow/Payment ledger directly).
+- `src/database/escrow-fk-integrity.integration.spec.ts` — **integration** test against a real Postgres (requires `DATABASE_URL`, not mocked): the exactly-one-parent CHECK constraint on `escrows`, and that sponsor dashboard figures survive a parent bounty/milestone being deleted.
 
-`DATABASE_SYNCHRONIZE=true` (set in development) will auto-create tables from entities for fast local iteration. A real migration workflow (`typeorm migration:generate`) is recommended before deploying to production (see Roadmap).
+`DATABASE_SYNCHRONIZE=true` (set in development) will auto-create tables from entities for fast local iteration. Real deployments should run migrations instead — see `src/database/migrations/` and the `migration:*` npm scripts below.
+
+### Migrations
+
+Schema changes are tracked as TypeORM migrations under `src/database/migrations/`, driven by the `DataSource` in `src/database/data-source.ts`:
+
+```bash
+# Generate a migration from entity changes (requires DATABASE_URL pointed at a real DB to diff against)
+npm run migration:generate -- src/database/migrations/SomeChange
+
+# Run all pending migrations
+npm run migration:run
+
+# Revert the most recently applied migration
+npm run migration:revert
+```
 
 ## Roadmap
 
-- [ ] Wire up TypeORM migrations (currently relies on `synchronize` for local dev only).
+- [x] ~~Wire up TypeORM migrations (currently relies on `synchronize` for local dev only).~~ See `src/database/migrations/` and the Migrations section above.
 - [ ] Move GitHub sync from a static PAT to a GitHub App installation-token flow for multi-org, least-privilege access.
 - [ ] Deploy the real escrow contract from `mergefi-contracts` and drop the Soroban dry-run fallback.
 - [ ] Replace the single `TREASURY_SECRET` signer with a proper signing service (KMS / multi-sig) before handling real funds.

package.json

@@ -17,7 +17,12 @@
     "test:watch": "jest --watch",
     "test:cov": "jest --coverage",
     "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand",
-    "test:e2e": "jest --config ./test/jest-e2e.json"
+    "test:e2e": "jest --config ./test/jest-e2e.json",
+    "typeorm": "typeorm-ts-node-commonjs -d src/database/data-source.ts",
+    "migration:generate": "npm run typeorm -- migration:generate",
+    "migration:create": "typeorm-ts-node-commonjs migration:create",
+    "migration:run": "npm run typeorm -- migration:run",
+    "migration:revert": "npm run typeorm -- migration:revert"
   },
   "dependencies": {
     "@nestjs/common": "^11.0.1",

src/bounties/bounties.service.spec.ts

@@ -61,12 +61,17 @@ describe('BountiesService', () => {
       status: BountyStatus.OPEN,
       amount: '100',
       asset: AssetType.USDC,
+      sponsorId: 'sponsor-1',
     });
 
     const bounty = await service.fund('b1', 'GFUNDER');
 
     expect(escrowService.fund).toHaveBeenCalledWith(
-      expect.objectContaining({ bountyId: 'b1', funderAddress: 'GFUNDER' }),
+      expect.objectContaining({
+        bountyId: 'b1',
+        funderAddress: 'GFUNDER',
+        sponsorId: 'sponsor-1',
+      }),
     );
     expect(bounty.status).toBe(BountyStatus.FUNDED);
   });

src/bounties/bounties.service.ts

@@ -45,6 +45,7 @@ export class BountiesService {
       asset: bounty.asset,
       funderAddress,
       bountyId: bounty.id,
+      sponsorId: bounty.sponsorId,
     });
 
     bounty.escrow = escrow;

src/common/entities/bounty.entity.ts

@@ -67,9 +67,12 @@ export class Bounty {
   @Column({ type: 'timestamptz', nullable: true })
   deadline: Date | null;
 
+  // cascade excludes 'remove'/'soft-remove': removing a Bounty entity via the
+  // ORM must not also remove its Escrow — the escrow row (and the funds it
+  // represents) must outlive the bounty record. See #27.
   @OneToOne(() => Escrow, (escrow) => escrow.bounty, {
     nullable: true,
-    cascade: true,
+    cascade: ['insert', 'update'],
   })
   escrow: Escrow | null;
 

src/common/entities/escrow.entity.ts

@@ -1,4 +1,5 @@
 import {
+  Check,
   Column,
   CreateDateColumn,
   Entity,
@@ -20,15 +21,39 @@ import { AssetType, EscrowStatus } from '../enums';
  * The actual funds custody lives in the deployed escrow contract on Stellar;
  * this table mirrors state so the API can serve fast reads and so we have an
  * audit trail independent of Horizon/RPC availability.
+ *
+ * The parent link (bounty/milestone/maintenancePool) is `onDelete: 'SET
+ * NULL'`, not CASCADE: deleting a bounty/milestone must never delete the
+ * escrow row (and, transitively, its payments) out from under real,
+ * possibly still-LOCKED, funds. See #27 — an escrow whose parent was
+ * deleted stays a first-class, still-queryable ledger row, orphaned but
+ * intact, attributable to its sponsor via the denormalized `sponsorId`
+ * below (which survives independently of the parent row).
+ *
+ * The CHECK constraint below only enforces *at most one* parent, not
+ * *exactly* one: `ON DELETE SET NULL` nulls out an escrow's only parent
+ * column, which is precisely the state an orphaned-by-deletion escrow ends
+ * up in (0 of 3 set) — a CHECK requiring exactly one would make that very
+ * SET NULL fail with a constraint violation the moment it fires. "Exactly
+ * one at creation" is enforced instead where it belongs: application-side,
+ * in EscrowService.fund (see assertExactlyOneParent).
  */
 @Entity('escrows')
+@Check(
+  'CHK_escrow_at_most_one_parent',
+  `(
+    (CASE WHEN "bountyId" IS NOT NULL THEN 1 ELSE 0 END) +
+    (CASE WHEN "milestoneId" IS NOT NULL THEN 1 ELSE 0 END) +
+    (CASE WHEN "maintenancePoolId" IS NOT NULL THEN 1 ELSE 0 END)
+  ) <= 1`,
+)
 export class Escrow {
   @PrimaryGeneratedColumn('uuid')
   id: string;
 
   @OneToOne(() => Bounty, (bounty) => bounty.escrow, {
     nullable: true,
-    onDelete: 'CASCADE',
+    onDelete: 'SET NULL',
   })
   @JoinColumn()
   bounty: Bounty | null;
@@ -38,7 +63,7 @@ export class Escrow {
 
   @OneToOne(() => Milestone, (milestone) => milestone.escrow, {
     nullable: true,
-    onDelete: 'CASCADE',
+    onDelete: 'SET NULL',
   })
   @JoinColumn()
   milestone: Milestone | null;
@@ -48,14 +73,25 @@ export class Escrow {
 
   @OneToOne(() => MaintenancePool, (pool) => pool.escrow, {
     nullable: true,
-    onDelete: 'CASCADE',
+    onDelete: 'SET NULL',
   })
   @JoinColumn()
   maintenancePool: MaintenancePool | null;
 
   @Column({ type: 'varchar', nullable: true })
   maintenancePoolId: string | null;
 
+  /**
+   * Denormalized sponsor identity, captured from the parent bounty/milestone
+   * at fund time. Sponsor-dashboard aggregates (src/sponsors/sponsors.service.ts)
+   * read this column directly rather than joining through bounty/milestone,
+   * so a locked or spent escrow is still correctly attributed to its sponsor
+   * even after the parent record is deleted (#27). Null for
+   * maintenance-pool escrows, which aren't sponsor-attributed the same way.
+   */
+  @Column({ type: 'varchar', nullable: true })
+  sponsorId: string | null;
+
   /** Deployed Soroban contract ID this escrow instance is held by. */
   @Column({ type: 'varchar', nullable: true })
   contractId: string | null;

src/common/entities/payment.entity.ts

@@ -17,7 +17,12 @@ export class Payment {
   @PrimaryGeneratedColumn('uuid')
   id: string;
 
-  @ManyToOne(() => Escrow, (escrow) => escrow.payments, { onDelete: 'CASCADE' })
+  // RESTRICT, not CASCADE: a Payment is a record of money that actually
+  // moved. Deleting its parent Escrow must never silently delete that
+  // payout record too — the database refuses the delete instead. See #27.
+  @ManyToOne(() => Escrow, (escrow) => escrow.payments, {
+    onDelete: 'RESTRICT',
+  })
   @JoinColumn()
   escrow: Escrow;
 

src/database/data-source.ts

@@ -0,0 +1,26 @@
+import 'reflect-metadata';
+import { DataSource } from 'typeorm';
+import { entities } from '../common/entities/typeorm-entities';
+
+/**
+ * TypeORM CLI DataSource — used only for `migration:generate`/`migration:run`/
+ * `migration:revert` (see package.json scripts). The running application
+ * connects via `TypeOrmModule.forRootAsync` in src/app.module.ts instead;
+ * the two are kept in sync by importing the same `entities` list.
+ *
+ * Before this, the app relied entirely on `synchronize` and had no migration
+ * history at all (#27) — every schema change (including the exactly-one-
+ * parent CHECK constraint this DataSource ships the first migration for)
+ * now goes through a reviewable, revertible migration file instead.
+ */
+export const AppDataSource = new DataSource({
+  type: 'postgres',
+  url:
+    process.env.DATABASE_URL ??
+    'postgresql://postgres:postgres@localhost:5432/mergefi',
+  entities,
+  migrations: [__dirname + '/migrations/*{.ts,.js}'],
+  migrationsTableName: 'migrations',
+  synchronize: false,
+  logging: process.env.DATABASE_LOGGING === 'true',
+});