diff --git a/README.md b/README.md index bac44a0..fa60de5 100644 --- a/README.md +++ b/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. diff --git a/package.json b/package.json index dc6a024..57dd01d 100644 --- a/package.json +++ b/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", diff --git a/src/bounties/bounties.service.spec.ts b/src/bounties/bounties.service.spec.ts index 4738a82..fd5a497 100644 --- a/src/bounties/bounties.service.spec.ts +++ b/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); }); diff --git a/src/bounties/bounties.service.ts b/src/bounties/bounties.service.ts index 70fdcb9..7bf75f8 100644 --- a/src/bounties/bounties.service.ts +++ b/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; diff --git a/src/common/entities/bounty.entity.ts b/src/common/entities/bounty.entity.ts index d222579..74e35db 100644 --- a/src/common/entities/bounty.entity.ts +++ b/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; diff --git a/src/common/entities/escrow.entity.ts b/src/common/entities/escrow.entity.ts index 9cba6ea..ff2556f 100644 --- a/src/common/entities/escrow.entity.ts +++ b/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,7 +73,7 @@ export class Escrow { @OneToOne(() => MaintenancePool, (pool) => pool.escrow, { nullable: true, - onDelete: 'CASCADE', + onDelete: 'SET NULL', }) @JoinColumn() maintenancePool: MaintenancePool | null; @@ -56,6 +81,17 @@ export class Escrow { @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; diff --git a/src/common/entities/payment.entity.ts b/src/common/entities/payment.entity.ts index 2abe94a..4b9037a 100644 --- a/src/common/entities/payment.entity.ts +++ b/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; diff --git a/src/database/data-source.ts b/src/database/data-source.ts new file mode 100644 index 0000000..b7518c6 --- /dev/null +++ b/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', +}); diff --git a/src/database/escrow-fk-integrity.integration.spec.ts b/src/database/escrow-fk-integrity.integration.spec.ts new file mode 100644 index 0000000..83b1ec3 --- /dev/null +++ b/src/database/escrow-fk-integrity.integration.spec.ts @@ -0,0 +1,303 @@ +import { DataSource, Repository } from 'typeorm'; +import { entities } from '../common/entities/typeorm-entities'; +import { + Bounty, + Escrow, + Milestone, + Payment, + Repository as Repo, + Issue, + User, +} from '../common/entities'; +import { + AssetType, + BountyStatus, + EscrowStatus, + PaymentStatus, +} from '../common/enums'; +import { SponsorsService } from '../sponsors/sponsors.service'; + +/** + * Integration tests for #27: these hit a real Postgres (see + * .github/workflows/ci.yml's `postgres` service / docker-compose.yml's `db` + * service — DATABASE_URL must point at a real, disposable database). Every + * other .spec.ts in this repo mocks its repositories; the whole point of + * this bug is DB-level FK/CHECK behavior that a mocked repository can't + * exercise, so this file uses `synchronize: true` against a real connection + * to build the schema straight from the (now-fixed) entity decorators. + */ +describe('Escrow FK integrity + sponsor dashboard reconciliation (integration)', () => { + let dataSource: DataSource; + let bountyRepo: Repository; + let milestoneRepo: Repository; + let paymentRepo: Repository; + let escrowRepo: Repository; + let repoRepo: Repository; + let issueRepo: Repository; + let userRepo: Repository; + let sponsorsService: SponsorsService; + + beforeAll(async () => { + dataSource = new DataSource({ + type: 'postgres', + url: + process.env.DATABASE_URL ?? + 'postgresql://postgres:postgres@localhost:5432/mergefi', + entities, + synchronize: true, + dropSchema: true, + }); + await dataSource.initialize(); + + bountyRepo = dataSource.getRepository(Bounty); + milestoneRepo = dataSource.getRepository(Milestone); + paymentRepo = dataSource.getRepository(Payment); + escrowRepo = dataSource.getRepository(Escrow); + repoRepo = dataSource.getRepository(Repo); + issueRepo = dataSource.getRepository(Issue); + userRepo = dataSource.getRepository(User); + + sponsorsService = new SponsorsService( + bountyRepo, + milestoneRepo, + paymentRepo, + escrowRepo, + ); + }, 30_000); + + afterAll(async () => { + if (dataSource?.isInitialized) await dataSource.destroy(); + }); + + afterEach(async () => { + // Delete in child-to-parent order — payments/escrows first now that + // their FKs are SET NULL/RESTRICT instead of CASCADE, they won't be + // cleaned up automatically by deleting bounties/milestones. + await paymentRepo.query('DELETE FROM payments'); + await escrowRepo.query('DELETE FROM escrows'); + await bountyRepo.query('DELETE FROM bounties'); + await issueRepo.query('DELETE FROM issues'); + await milestoneRepo.query('DELETE FROM milestones'); + await repoRepo.query('DELETE FROM repositories'); + await userRepo.query('DELETE FROM users'); + }); + + async function makeSponsor(): Promise { + return userRepo.save( + userRepo.create({ username: `sponsor-${Date.now()}-${Math.random()}` }), + ); + } + + async function makeBounty( + sponsorId: string, + amount = '100', + ): Promise { + const repository = await repoRepo.save( + repoRepo.create({ + githubRepoId: `repo-${Date.now()}-${Math.random()}`, + owner: 'octocat', + name: `repo-${Math.random()}`, + fullName: 'octocat/repo', + }), + ); + const issue = await issueRepo.save( + issueRepo.create({ + repositoryId: repository.id, + githubIssueId: `issue-${Date.now()}-${Math.random()}`, + number: 1, + title: 'Fix the bug', + githubUrl: 'https://github.com/octocat/repo/issues/1', + }), + ); + return bountyRepo.save( + bountyRepo.create({ + issueId: issue.id, + sponsorId, + amount, + asset: AssetType.USDC, + status: BountyStatus.FUNDED, + }), + ); + } + + async function makeMilestone( + sponsorId: string, + budget = '100', + ): Promise { + const repository = await repoRepo.save( + repoRepo.create({ + githubRepoId: `repo-${Date.now()}-${Math.random()}`, + owner: 'octocat', + name: `repo-${Math.random()}`, + fullName: 'octocat/repo', + }), + ); + return milestoneRepo.save( + milestoneRepo.create({ + repositoryId: repository.id, + sponsorId, + title: 'Q1 roadmap', + budget, + asset: AssetType.USDC, + }), + ); + } + + describe('CHK_escrow_at_most_one_parent', () => { + it('rejects an escrow with more than one parent set', async () => { + const sponsor = await makeSponsor(); + // Both parents reference genuinely existing rows, so the only way + // this insert can fail is the CHECK constraint — not a coincidental + // FK violation on a dangling id. + const bounty = await makeBounty(sponsor.id); + const milestone = await makeMilestone(sponsor.id); + + await expect( + escrowRepo.query( + `INSERT INTO escrows (id, "bountyId", "milestoneId", amount, asset, status) + VALUES (gen_random_uuid(), $1, $2, '10', 'USDC', 'pending')`, + [bounty.id, milestone.id], + ), + ).rejects.toThrow(/CHK_escrow_at_most_one_parent/); + }); + + it('allows an escrow with exactly one parent set', async () => { + const sponsor = await makeSponsor(); + const bounty = await makeBounty(sponsor.id); + + const escrow = await escrowRepo.save( + escrowRepo.create({ + bountyId: bounty.id, + sponsorId: sponsor.id, + amount: '10', + asset: AssetType.USDC, + status: EscrowStatus.LOCKED, + }), + ); + + expect(escrow.id).toBeDefined(); + }); + + it('allows an escrow with zero parents set (the orphaned-by-deletion state)', async () => { + // The DB-level constraint deliberately allows this — it's exactly + // the state ON DELETE SET NULL produces when an escrow's parent is + // deleted (see the next describe block). "Exactly one" is an + // application-level rule enforced at creation time in + // EscrowService.assertExactlyOneParent, not a DB invariant, because + // the DB has no way to distinguish "never had a parent" from + // "orphaned by a legitimate deletion". + const escrow = await escrowRepo.save( + escrowRepo.create({ + amount: '10', + asset: AssetType.USDC, + status: EscrowStatus.LOCKED, + }), + ); + + expect(escrow.id).toBeDefined(); + }); + }); + + describe('parent deletion no longer destroys the escrow ledger row', () => { + it('SET NULLs escrows.bountyId instead of deleting the row when the bounty is deleted', async () => { + const sponsor = await makeSponsor(); + const bounty = await makeBounty(sponsor.id); + const escrow = await escrowRepo.save( + escrowRepo.create({ + bountyId: bounty.id, + sponsorId: sponsor.id, + amount: '250', + asset: AssetType.USDC, + status: EscrowStatus.LOCKED, + }), + ); + + await bountyRepo.delete(bounty.id); + + const survived = await escrowRepo.findOne({ where: { id: escrow.id } }); + expect(survived).not.toBeNull(); + expect(survived?.bountyId).toBeNull(); + expect(survived?.status).toBe(EscrowStatus.LOCKED); + expect(Number(survived?.amount)).toBe(250); + }); + + it('RESTRICTs deleting an escrow that still has payment records', async () => { + const sponsor = await makeSponsor(); + const bounty = await makeBounty(sponsor.id); + const escrow = await escrowRepo.save( + escrowRepo.create({ + bountyId: bounty.id, + sponsorId: sponsor.id, + amount: '250', + asset: AssetType.USDC, + status: EscrowStatus.RELEASED, + }), + ); + await paymentRepo.save( + paymentRepo.create({ + escrowId: escrow.id, + recipientAddress: 'GRECIPIENT', + amount: '250', + asset: AssetType.USDC, + status: PaymentStatus.CONFIRMED, + }), + ); + + await expect(escrowRepo.delete(escrow.id)).rejects.toThrow(); + }); + }); + + describe('sponsor dashboard figures survive parent bounty deletion (#27 acceptance criterion)', () => { + it('budgetLocked keeps counting a stranded escrow after its bounty is deleted', async () => { + const sponsor = await makeSponsor(); + const bounty = await makeBounty(sponsor.id, '400'); + await escrowRepo.save( + escrowRepo.create({ + bountyId: bounty.id, + sponsorId: sponsor.id, + amount: '400', + asset: AssetType.USDC, + status: EscrowStatus.LOCKED, + }), + ); + + expect(await sponsorsService.budgetLocked(sponsor.id)).toBe(400); + + await bountyRepo.delete(bounty.id); + + // The old implementation summed Bounty.amount by Bounty.status, so + // this figure would silently drop to 0 the instant the bounty row + // was gone — even though the funds are still LOCKED on-chain. + expect(await sponsorsService.budgetLocked(sponsor.id)).toBe(400); + }); + + it('totalSpend keeps counting a confirmed payment after its bounty is deleted', async () => { + const sponsor = await makeSponsor(); + const bounty = await makeBounty(sponsor.id, '150'); + const escrow = await escrowRepo.save( + escrowRepo.create({ + bountyId: bounty.id, + sponsorId: sponsor.id, + amount: '150', + asset: AssetType.USDC, + status: EscrowStatus.RELEASED, + }), + ); + await paymentRepo.save( + paymentRepo.create({ + escrowId: escrow.id, + recipientAddress: 'GRECIPIENT', + amount: '150', + asset: AssetType.USDC, + status: PaymentStatus.CONFIRMED, + }), + ); + + expect(await sponsorsService.totalSpend(sponsor.id)).toBe(150); + + await bountyRepo.delete(bounty.id); + + expect(await sponsorsService.totalSpend(sponsor.id)).toBe(150); + }); + }); +}); diff --git a/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts b/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts new file mode 100644 index 0000000..99eea9a --- /dev/null +++ b/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts @@ -0,0 +1,173 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * First migration in the project (previously schema was managed entirely by + * `synchronize`, with no migration history at all — see #27's "Difficulty + * Justification"). Fixes the escrow FK-integrity gaps described in #27: + * + * 1. Adds `escrows.sponsorId`, a denormalized copy of the owning + * bounty/milestone's sponsor, backfilled from existing rows. Sponsor + * dashboard aggregates read this column directly so they stay correct + * even if the parent bounty/milestone row is later deleted. + * 2. Re-points `escrows.bountyId` / `milestoneId` / `maintenancePoolId`'s + * foreign keys from `ON DELETE CASCADE` to `ON DELETE SET NULL` — + * deleting a bounty/milestone/pool must never delete the escrow ledger + * row for funds that may still be LOCKED on-chain. + * 3. Re-points `payments.escrowId`'s foreign key from `ON DELETE CASCADE` + * to `ON DELETE RESTRICT` — a Payment is a record of money that already + * moved; deleting its parent Escrow must never silently delete that + * record too. + * 4. Adds a CHECK constraint enforcing that *at most* one of `bountyId` / + * `milestoneId` / `maintenancePoolId` is non-null on every Escrow row. + * Not "exactly one": `ON DELETE SET NULL` (see 2.) legitimately drives + * an orphaned escrow's parent count to zero, and a stricter "exactly + * one" CHECK would make that very SET NULL fail. "Exactly one at + * creation" is enforced application-side instead, in + * EscrowService.assertExactlyOneParent. + */ +export class EscrowFkIntegrityAndSponsorId1784272650000 implements MigrationInterface { + name = 'EscrowFkIntegrityAndSponsorId1784272650000'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `ALTER TABLE "escrows" ADD COLUMN IF NOT EXISTS "sponsorId" varchar`, + ); + + await queryRunner.query(` + UPDATE "escrows" e + SET "sponsorId" = b."sponsorId" + FROM "bounties" b + WHERE e."bountyId" = b."id" AND e."sponsorId" IS NULL + `); + await queryRunner.query(` + UPDATE "escrows" e + SET "sponsorId" = m."sponsorId" + FROM "milestones" m + WHERE e."milestoneId" = m."id" AND e."sponsorId" IS NULL + `); + + await this.replaceForeignKeyOnDelete( + queryRunner, + 'escrows', + 'bountyId', + 'bounties', + 'SET NULL', + ); + await this.replaceForeignKeyOnDelete( + queryRunner, + 'escrows', + 'milestoneId', + 'milestones', + 'SET NULL', + ); + await this.replaceForeignKeyOnDelete( + queryRunner, + 'escrows', + 'maintenancePoolId', + 'maintenance_pools', + 'SET NULL', + ); + await this.replaceForeignKeyOnDelete( + queryRunner, + 'payments', + 'escrowId', + 'escrows', + 'RESTRICT', + ); + + await queryRunner.query(` + ALTER TABLE "escrows" + ADD CONSTRAINT "CHK_escrow_at_most_one_parent" + CHECK ( + ( + (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 + ) + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `ALTER TABLE "escrows" DROP CONSTRAINT IF EXISTS "CHK_escrow_at_most_one_parent"`, + ); + + await this.replaceForeignKeyOnDelete( + queryRunner, + 'payments', + 'escrowId', + 'escrows', + 'CASCADE', + ); + await this.replaceForeignKeyOnDelete( + queryRunner, + 'escrows', + 'maintenancePoolId', + 'maintenance_pools', + 'CASCADE', + ); + await this.replaceForeignKeyOnDelete( + queryRunner, + 'escrows', + 'milestoneId', + 'milestones', + 'CASCADE', + ); + await this.replaceForeignKeyOnDelete( + queryRunner, + 'escrows', + 'bountyId', + 'bounties', + 'CASCADE', + ); + + await queryRunner.query( + `ALTER TABLE "escrows" DROP COLUMN IF EXISTS "sponsorId"`, + ); + } + + /** + * Finds the existing single-column foreign key from `table.column` and + * replaces its ON DELETE action in place, preserving whatever name + * `synchronize` (or a previous migration) originally gave it — this + * avoids hardcoding TypeORM's auto-generated constraint name, which is a + * content hash we can't reliably predict across environments. + */ + private async replaceForeignKeyOnDelete( + queryRunner: QueryRunner, + table: string, + column: string, + refTable: string, + onDelete: 'SET NULL' | 'CASCADE' | 'RESTRICT', + ): Promise { + const rows = (await queryRunner.query( + ` + SELECT con.conname + FROM pg_constraint con + JOIN pg_class rel ON rel.oid = con.conrelid + JOIN pg_attribute att + ON att.attrelid = con.conrelid AND att.attnum = ANY(con.conkey) + WHERE con.contype = 'f' + AND rel.relname = $1 + AND att.attname = $2 + `, + [table, column], + )) as Array<{ conname: string }>; + + if (rows.length === 0) { + // No pre-existing FK to replace (e.g. a database that never ran + // `synchronize`) — the entity decorator already declares the correct + // ON DELETE action, so there's nothing to fix up here. + return; + } + + const { conname } = rows[0]; + await queryRunner.query( + `ALTER TABLE "${table}" DROP CONSTRAINT "${conname}"`, + ); + await queryRunner.query( + `ALTER TABLE "${table}" ADD CONSTRAINT "${conname}" FOREIGN KEY ("${column}") REFERENCES "${refTable}"("id") ON DELETE ${onDelete}`, + ); + } +} diff --git a/src/escrow/escrow.service.spec.ts b/src/escrow/escrow.service.spec.ts index 4469c59..9078f10 100644 --- a/src/escrow/escrow.service.spec.ts +++ b/src/escrow/escrow.service.spec.ts @@ -63,6 +63,34 @@ describe('EscrowService', () => { expect(escrow.fundTxHash).toBe('tx-hash-123'); }); + it('persists the denormalized sponsorId on the created escrow row', async () => { + const escrow = await service.fund({ + amount: '100.0000000', + asset: AssetType.USDC, + funderAddress: 'GABC...FUNDER', + bountyId: 'bounty-1', + sponsorId: 'sponsor-1', + }); + + expect(escrowRepo.create).toHaveBeenCalledWith( + expect.objectContaining({ sponsorId: 'sponsor-1' }), + ); + expect(escrow.sponsorId).toBe('sponsor-1'); + }); + + it('defaults sponsorId to null when omitted (e.g. maintenance-pool escrows)', async () => { + await service.fund({ + amount: '100.0000000', + asset: AssetType.USDC, + funderAddress: 'GABC...FUNDER', + maintenancePoolId: 'pool-1', + }); + + expect(escrowRepo.create).toHaveBeenCalledWith( + expect.objectContaining({ sponsorId: null }), + ); + }); + it('marks the escrow FAILED and rethrows when the contract call fails', async () => { soroban.invoke.mockRejectedValueOnce(new Error('simulation failed')); @@ -71,6 +99,7 @@ describe('EscrowService', () => { amount: '10', asset: AssetType.XLM, funderAddress: 'G...', + bountyId: 'bounty-1', }), ).rejects.toThrow('simulation failed'); @@ -120,6 +149,38 @@ describe('EscrowService', () => { expect(escrowRepo.save).not.toHaveBeenCalled(); expect(soroban.invoke).not.toHaveBeenCalled(); }); + + it('rejects funding with no parent (bounty/milestone/pool) set at all', async () => { + await expect( + service.fund({ + amount: '10.0000000', + asset: AssetType.USDC, + funderAddress: 'G...FUNDER', + }), + ).rejects.toThrow( + 'Exactly one of bountyId, milestoneId, or maintenancePoolId is required', + ); + + expect(escrowRepo.create).not.toHaveBeenCalled(); + expect(soroban.invoke).not.toHaveBeenCalled(); + }); + + it('rejects funding with more than one parent set', async () => { + await expect( + service.fund({ + amount: '10.0000000', + asset: AssetType.USDC, + funderAddress: 'G...FUNDER', + bountyId: 'bounty-1', + milestoneId: 'milestone-1', + }), + ).rejects.toThrow( + 'Exactly one of bountyId, milestoneId, or maintenancePoolId is required', + ); + + expect(escrowRepo.create).not.toHaveBeenCalled(); + expect(soroban.invoke).not.toHaveBeenCalled(); + }); }); describe('release', () => { diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 8078a50..eb35471 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -22,6 +22,14 @@ export interface FundEscrowInput { bountyId?: string; milestoneId?: string; maintenancePoolId?: string; + /** + * Denormalized sponsor identity, stored directly on the Escrow row rather + * than only reachable via a join to bounty/milestone. This is what lets + * sponsor-dashboard aggregates stay correct even after the parent + * bounty/milestone is deleted (#27) — omit for maintenance-pool escrows, + * which aren't sponsor-attributed. + */ + sponsorId?: string | null; } export interface SplitRecipient { @@ -53,6 +61,7 @@ export class EscrowService { bountyId: input.bountyId ?? null, milestoneId: input.milestoneId ?? null, maintenancePoolId: input.maintenancePoolId ?? null, + sponsorId: input.sponsorId ?? null, }); await this.escrowRepo.save(escrow); @@ -285,6 +294,29 @@ export class EscrowService { `Unsupported escrow asset: ${String(input.asset)}`, ); } + this.assertExactlyOneParent(input); + } + + /** + * A newly-created escrow must belong to exactly one of + * bounty/milestone/maintenancePool. This is deliberately an + * application-level check rather than a DB CHECK constraint: the + * database only enforces "at most one" (CHK_escrow_at_most_one_parent), + * because ON DELETE SET NULL legitimately drives an existing escrow's + * parent count to zero when its parent is deleted, and a stricter + * "exactly one" constraint would make that very SET NULL fail (#27). + */ + private assertExactlyOneParent(input: FundEscrowInput): void { + const parentCount = [ + input.bountyId, + input.milestoneId, + input.maintenancePoolId, + ].filter((id) => id != null).length; + if (parentCount !== 1) { + throw new BadRequestException( + 'Exactly one of bountyId, milestoneId, or maintenancePoolId is required', + ); + } } private assertValidAmount(amount: string): void { diff --git a/src/milestones/milestones.service.spec.ts b/src/milestones/milestones.service.spec.ts new file mode 100644 index 0000000..492c562 --- /dev/null +++ b/src/milestones/milestones.service.spec.ts @@ -0,0 +1,100 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { MilestonesService } from './milestones.service'; +import { EscrowService } from '../escrow/escrow.service'; +import { Issue, Milestone } from '../common/entities'; +import { AssetType, MilestoneStatus } from '../common/enums'; + +describe('MilestonesService', () => { + let service: MilestonesService; + let milestoneRepo: { findOne: jest.Mock; save: jest.Mock }; + let escrowService: { fund: jest.Mock }; + + beforeEach(async () => { + milestoneRepo = { + findOne: jest.fn(), + save: jest.fn((m: Partial) => Promise.resolve(m)), + }; + escrowService = { + fund: jest.fn().mockResolvedValue({ id: 'escrow-1', status: 'locked' }), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + MilestonesService, + { provide: getRepositoryToken(Milestone), useValue: milestoneRepo }, + { + provide: getRepositoryToken(Issue), + useValue: { findOne: jest.fn() }, + }, + { provide: EscrowService, useValue: escrowService }, + ], + }).compile(); + + service = module.get(MilestonesService); + }); + + describe('fund', () => { + it('locks escrow for the milestone budget and moves OPEN -> FUNDED', async () => { + milestoneRepo.findOne.mockResolvedValue({ + id: 'm1', + status: MilestoneStatus.OPEN, + budget: '500', + asset: AssetType.USDC, + sponsorId: 'sponsor-1', + }); + + const milestone = await service.fund('m1', 'GFUNDER'); + + expect(milestone.status).toBe(MilestoneStatus.FUNDED); + expect(milestone.escrowId).toBe('escrow-1'); + }); + + it('passes milestoneId and the milestone sponsorId through to EscrowService.fund', async () => { + milestoneRepo.findOne.mockResolvedValue({ + id: 'm1', + status: MilestoneStatus.OPEN, + budget: '500', + asset: AssetType.USDC, + sponsorId: 'sponsor-1', + }); + + await service.fund('m1', 'GFUNDER'); + + expect(escrowService.fund).toHaveBeenCalledWith( + expect.objectContaining({ + milestoneId: 'm1', + funderAddress: 'GFUNDER', + sponsorId: 'sponsor-1', + }), + ); + }); + + it('still forwards a null sponsorId when the milestone has none set', async () => { + milestoneRepo.findOne.mockResolvedValue({ + id: 'm2', + status: MilestoneStatus.OPEN, + budget: '500', + asset: AssetType.USDC, + sponsorId: null, + }); + + await service.fund('m2', 'GFUNDER'); + + expect(escrowService.fund).toHaveBeenCalledWith( + expect.objectContaining({ sponsorId: null }), + ); + }); + + it('rejects funding a milestone that is not OPEN', async () => { + milestoneRepo.findOne.mockResolvedValue({ + id: 'm1', + status: MilestoneStatus.FUNDED, + }); + + await expect(service.fund('m1', 'GFUNDER')).rejects.toThrow( + 'Milestone m1 is not OPEN (current: funded)', + ); + }); + }); +}); diff --git a/src/milestones/milestones.service.ts b/src/milestones/milestones.service.ts index 88a1d2e..43bb7fb 100644 --- a/src/milestones/milestones.service.ts +++ b/src/milestones/milestones.service.ts @@ -56,6 +56,7 @@ export class MilestonesService { asset: milestone.asset, funderAddress, milestoneId: milestone.id, + sponsorId: milestone.sponsorId, }); milestone.escrow = escrow; diff --git a/src/sponsors/sponsors.module.ts b/src/sponsors/sponsors.module.ts index d5a2c98..fcb5d50 100644 --- a/src/sponsors/sponsors.module.ts +++ b/src/sponsors/sponsors.module.ts @@ -1,11 +1,11 @@ import { Module } from '@nestjs/common'; import { TypeOrmModule } from '@nestjs/typeorm'; -import { Bounty, Milestone, Payment } from '../common/entities'; +import { Bounty, Escrow, Milestone, Payment } from '../common/entities'; import { SponsorsService } from './sponsors.service'; import { SponsorsController } from './sponsors.controller'; @Module({ - imports: [TypeOrmModule.forFeature([Bounty, Milestone, Payment])], + imports: [TypeOrmModule.forFeature([Bounty, Milestone, Payment, Escrow])], controllers: [SponsorsController], providers: [SponsorsService], exports: [SponsorsService], diff --git a/src/sponsors/sponsors.service.spec.ts b/src/sponsors/sponsors.service.spec.ts new file mode 100644 index 0000000..82ae627 --- /dev/null +++ b/src/sponsors/sponsors.service.spec.ts @@ -0,0 +1,143 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { SponsorsService } from './sponsors.service'; +import { Bounty, Escrow, Milestone, Payment } from '../common/entities'; +import { EscrowStatus, PaymentStatus } from '../common/enums'; + +/** + * Minimal fluent mock of TypeORM's QueryBuilder: every chainable method + * returns the same object, and the two terminal methods resolve to + * whatever the test configures via `result`. + */ +function createMockQueryBuilder(result: { raw?: unknown; many?: unknown[] }) { + const qb = { + select: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + innerJoin: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + getRawOne: jest.fn().mockResolvedValue(result.raw), + getMany: jest.fn().mockResolvedValue(result.many ?? []), + }; + return qb; +} + +describe('SponsorsService', () => { + let service: SponsorsService; + let bountyRepo: { createQueryBuilder: jest.Mock }; + let milestoneRepo: { find: jest.Mock }; + let paymentRepo: { createQueryBuilder: jest.Mock }; + let escrowRepo: { createQueryBuilder: jest.Mock }; + + beforeEach(async () => { + bountyRepo = { createQueryBuilder: jest.fn() }; + milestoneRepo = { find: jest.fn().mockResolvedValue([]) }; + paymentRepo = { createQueryBuilder: jest.fn() }; + escrowRepo = { createQueryBuilder: jest.fn() }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + SponsorsService, + { provide: getRepositoryToken(Bounty), useValue: bountyRepo }, + { provide: getRepositoryToken(Milestone), useValue: milestoneRepo }, + { provide: getRepositoryToken(Payment), useValue: paymentRepo }, + { provide: getRepositoryToken(Escrow), useValue: escrowRepo }, + ], + }).compile(); + + service = module.get(SponsorsService); + }); + + describe('budgetLocked', () => { + it('sums Escrow.amount directly, filtered by sponsorId and LOCKED status', async () => { + const qb = createMockQueryBuilder({ raw: { total: '1250.5000000' } }); + escrowRepo.createQueryBuilder.mockReturnValue(qb); + + const total = await service.budgetLocked('sponsor-1'); + + expect(escrowRepo.createQueryBuilder).toHaveBeenCalledWith('escrow'); + expect(qb.where).toHaveBeenCalledWith('escrow.sponsorId = :sponsorId', { + sponsorId: 'sponsor-1', + }); + expect(qb.andWhere).toHaveBeenCalledWith('escrow.status = :status', { + status: EscrowStatus.LOCKED, + }); + expect(total).toBe(1250.5); + }); + + it('does not query the Bounty table at all (no drifted-proxy risk)', async () => { + const qb = createMockQueryBuilder({ raw: { total: '0' } }); + escrowRepo.createQueryBuilder.mockReturnValue(qb); + + await service.budgetLocked('sponsor-1'); + + expect(bountyRepo.createQueryBuilder).not.toHaveBeenCalled(); + }); + + it('returns 0 when there is no locked escrow for the sponsor', async () => { + const qb = createMockQueryBuilder({ raw: undefined }); + escrowRepo.createQueryBuilder.mockReturnValue(qb); + + expect(await service.budgetLocked('sponsor-1')).toBe(0); + }); + }); + + describe('totalSpend', () => { + it('sums confirmed Payment.amount joined to escrow.sponsorId', async () => { + const qb = createMockQueryBuilder({ raw: { total: '300.0000000' } }); + paymentRepo.createQueryBuilder.mockReturnValue(qb); + + const total = await service.totalSpend('sponsor-1'); + + expect(paymentRepo.createQueryBuilder).toHaveBeenCalledWith('payment'); + expect(qb.innerJoin).toHaveBeenCalledWith('payment.escrow', 'escrow'); + expect(qb.where).toHaveBeenCalledWith('escrow.sponsorId = :sponsorId', { + sponsorId: 'sponsor-1', + }); + expect(qb.andWhere).toHaveBeenCalledWith('payment.status = :status', { + status: PaymentStatus.CONFIRMED, + }); + expect(total).toBe(300); + }); + + it('returns 0 when the sponsor has no confirmed payments', async () => { + const qb = createMockQueryBuilder({ raw: undefined }); + paymentRepo.createQueryBuilder.mockReturnValue(qb); + + expect(await service.totalSpend('sponsor-1')).toBe(0); + }); + }); + + describe('dashboard', () => { + it('joins recentPayments on escrow.sponsorId, not escrow.bounty.sponsorId', async () => { + bountyRepo.createQueryBuilder.mockReturnValue( + createMockQueryBuilder({ many: [] }), + ); + escrowRepo.createQueryBuilder.mockReturnValue( + createMockQueryBuilder({ raw: { total: '0' } }), + ); + const paymentsQb = createMockQueryBuilder({ many: [] }); + paymentRepo.createQueryBuilder.mockImplementation(() => paymentsQb); + + await service.dashboard('sponsor-1'); + + // recentPayments's join must key off escrow.sponsorId so it still + // finds payments after the parent bounty/milestone is deleted, and so + // milestone-funded payments (which never had a `bounty` at all) show + // up too — see #27. + expect(paymentsQb.innerJoin).toHaveBeenCalledWith( + 'payment.escrow', + 'escrow', + ); + expect(paymentsQb.innerJoin).not.toHaveBeenCalledWith( + 'escrow.bounty', + 'bounty', + ); + expect(paymentsQb.where).toHaveBeenCalledWith( + 'escrow.sponsorId = :sponsorId', + { sponsorId: 'sponsor-1' }, + ); + }); + }); +}); diff --git a/src/sponsors/sponsors.service.ts b/src/sponsors/sponsors.service.ts index a0792de..b1f44f4 100644 --- a/src/sponsors/sponsors.service.ts +++ b/src/sponsors/sponsors.service.ts @@ -1,8 +1,13 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { Bounty, Milestone, Payment } from '../common/entities'; -import { BountyStatus, MilestoneStatus } from '../common/enums'; +import { Bounty, Escrow, Milestone, Payment } from '../common/entities'; +import { + BountyStatus, + EscrowStatus, + MilestoneStatus, + PaymentStatus, +} from '../common/enums'; export interface SponsorDashboard { activeBounties: Bounty[]; @@ -20,6 +25,8 @@ export class SponsorsService { private readonly milestoneRepo: Repository, @InjectRepository(Payment) private readonly paymentRepo: Repository, + @InjectRepository(Escrow) + private readonly escrowRepo: Repository, ) {} /** Bounties this sponsor has created that are still in flight (not paid/refunded/expired). */ @@ -37,31 +44,43 @@ export class SponsorsService { .getMany(); } - /** Total amount this sponsor has paid out across all completed bounties. */ + /** + * Total amount actually paid out to this sponsor's contributors, summed + * directly from the Payment ledger (the source of truth for money that + * moved) rather than `Bounty.status`, which is a workflow-state proxy that + * can drift from the real escrow/payment history. Joins through + * `escrow.sponsorId` (denormalized at fund time — see Escrow entity) so + * this stays correct even if the parent bounty/milestone is later deleted + * (#27). + */ async totalSpend(sponsorId: string): Promise { - const row = await this.bountyRepo - .createQueryBuilder('bounty') - .select('COALESCE(SUM(bounty.amount), 0)', 'total') - .where('bounty.sponsorId = :sponsorId', { sponsorId }) - .andWhere('bounty.status = :status', { status: BountyStatus.PAID }) + const row = await this.paymentRepo + .createQueryBuilder('payment') + .innerJoin('payment.escrow', 'escrow') + .select('COALESCE(SUM(payment.amount), 0)', 'total') + .where('escrow.sponsorId = :sponsorId', { sponsorId }) + .andWhere('payment.status = :status', { + status: PaymentStatus.CONFIRMED, + }) .getRawOne<{ total: string }>(); return Number(row?.total ?? 0); } - /** Amount currently locked in escrow for this sponsor's funded-but-unpaid bounties. */ + /** + * Amount currently locked in escrow for this sponsor, summed directly + * from the Escrow ledger (source of truth) rather than `Bounty.amount` + * filtered by `Bounty.status`. The prior implementation only considered + * bounty-funded escrows and depended on the parent Bounty row surviving; + * this reads `Escrow.status`/`Escrow.sponsorId` directly, so it covers + * milestone-funded escrows too and remains correct even if the parent + * bounty/milestone is later deleted (#27). + */ async budgetLocked(sponsorId: string): Promise { - const row = await this.bountyRepo - .createQueryBuilder('bounty') - .select('COALESCE(SUM(bounty.amount), 0)', 'total') - .where('bounty.sponsorId = :sponsorId', { sponsorId }) - .andWhere('bounty.status IN (:...locked)', { - locked: [ - BountyStatus.FUNDED, - BountyStatus.CLAIMED, - BountyStatus.IN_REVIEW, - BountyStatus.MERGED, - ], - }) + const row = await this.escrowRepo + .createQueryBuilder('escrow') + .select('COALESCE(SUM(escrow.amount), 0)', 'total') + .where('escrow.sponsorId = :sponsorId', { sponsorId }) + .andWhere('escrow.status = :status', { status: EscrowStatus.LOCKED }) .getRawOne<{ total: string }>(); return Number(row?.total ?? 0); } @@ -97,11 +116,15 @@ export class SponsorsService { this.activeMilestones(sponsorId), ]); + // Joins on escrow.sponsorId directly (not escrow.bounty.sponsorId): the + // old join required the escrow's parent bounty row to still exist and + // silently excluded milestone-funded payments entirely. sponsorId lives + // on the escrow itself, so this covers both funding paths and survives + // parent-bounty deletion (#27). const recentPayments = await this.paymentRepo .createQueryBuilder('payment') .innerJoin('payment.escrow', 'escrow') - .innerJoin('escrow.bounty', 'bounty') - .where('bounty.sponsorId = :sponsorId', { sponsorId }) + .where('escrow.sponsorId = :sponsorId', { sponsorId }) .orderBy('payment.createdAt', 'DESC') .take(20) .getMany();