From e93d9ee3007ec0f2772030347f1576f8f1b25aa2 Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:28:52 +0100 Subject: [PATCH 01/13] fix(escrow): stop CASCADE-deleting the escrow ledger when its parent is deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #27. --- src/common/entities/bounty.entity.ts | 5 +++- src/common/entities/escrow.entity.ts | 34 +++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 4 deletions(-) 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..0a6ac7e 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,31 @@ 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). */ @Entity('escrows') +@Check( + 'CHK_escrow_exactly_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 +55,7 @@ export class Escrow { @OneToOne(() => Milestone, (milestone) => milestone.escrow, { nullable: true, - onDelete: 'CASCADE', + onDelete: 'SET NULL', }) @JoinColumn() milestone: Milestone | null; @@ -48,7 +65,7 @@ export class Escrow { @OneToOne(() => MaintenancePool, (pool) => pool.escrow, { nullable: true, - onDelete: 'CASCADE', + onDelete: 'SET NULL', }) @JoinColumn() maintenancePool: MaintenancePool | null; @@ -56,6 +73,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; From 921117e632513b3f0a3866579fdc550f7f87a8ea Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:29:00 +0100 Subject: [PATCH 02/13] fix(payment): stop CASCADE-deleting payment records when their escrow 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 #27. --- src/common/entities/payment.entity.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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; From 61ca89160d1f4d270f7e9962eac7a5ebe28653ad Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:29:33 +0100 Subject: [PATCH 03/13] feat(escrow): accept and persist sponsorId on FundEscrowInput 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 #27. --- src/escrow/escrow.service.spec.ts | 28 ++++++++++++++++++++++++++++ src/escrow/escrow.service.ts | 9 +++++++++ 2 files changed, 37 insertions(+) diff --git a/src/escrow/escrow.service.spec.ts b/src/escrow/escrow.service.spec.ts index 4469c59..fc8c288 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')); diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 8078a50..4fad228 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); From 2c436361047c4d9f03ea4f16bb31736c7bc153f2 Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:29:44 +0100 Subject: [PATCH 04/13] feat(bounties): pass the bounty's sponsorId through to EscrowService.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 #27. --- src/bounties/bounties.service.spec.ts | 7 ++++++- src/bounties/bounties.service.ts | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) 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; From fc67b8ff917bcd2b4dba9f2644645c3c4a6edff5 Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:29:44 +0100 Subject: [PATCH 05/13] feat(milestones): pass the milestone's sponsorId through to EscrowService.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 #27. --- src/milestones/milestones.service.spec.ts | 100 ++++++++++++++++++++++ src/milestones/milestones.service.ts | 1 + 2 files changed, 101 insertions(+) create mode 100644 src/milestones/milestones.service.spec.ts 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; From b8774d2055fb2eb42ceb51a01eaf78ade29d5e04 Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:29:59 +0100 Subject: [PATCH 06/13] fix(sponsors): compute budgetLocked/totalSpend/recentPayments from the Escrow/Payment ledger directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #27. --- src/sponsors/sponsors.module.ts | 4 +- src/sponsors/sponsors.service.spec.ts | 143 ++++++++++++++++++++++++++ src/sponsors/sponsors.service.ts | 69 ++++++++----- 3 files changed, 191 insertions(+), 25 deletions(-) create mode 100644 src/sponsors/sponsors.service.spec.ts 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(); From 5e84f796eca014eea0668d28943ade3e9ed3eaad Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:30:09 +0100 Subject: [PATCH 07/13] feat(db): add TypeORM migration tooling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #27. --- package.json | 7 ++++++- src/database/data-source.ts | 26 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 src/database/data-source.ts 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/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', +}); From b2c8fc47afaeb8521c8882155627f5e0b9cd7cc6 Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:30:17 +0100 Subject: [PATCH 08/13] =?UTF-8?q?feat(db):=20first=20migration=20=E2=80=94?= =?UTF-8?q?=20escrow=20FK=20integrity=20+=20sponsorId=20backfill?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #27. --- ...272650000-EscrowFkIntegrityAndSponsorId.ts | 168 ++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts diff --git a/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts b/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts new file mode 100644 index 0000000..369c132 --- /dev/null +++ b/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts @@ -0,0 +1,168 @@ +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 exactly one of `bountyId` / + * `milestoneId` / `maintenancePoolId` is non-null on every Escrow row. + */ +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_exactly_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_exactly_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}`, + ); + } +} From f4ccc42ee7d5e9cb97f9f426d69273d489d18de7 Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:30:27 +0100 Subject: [PATCH 09/13] test(db): integration test against real Postgres for FK/CHECK behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #27 acceptance criterion this whole change exists to satisfy. Part of #27. --- .../escrow-fk-integrity.integration.spec.ts | 292 ++++++++++++++++++ 1 file changed, 292 insertions(+) create mode 100644 src/database/escrow-fk-integrity.integration.spec.ts 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..0de845a --- /dev/null +++ b/src/database/escrow-fk-integrity.integration.spec.ts @@ -0,0 +1,292 @@ +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_exactly_one_parent', () => { + it('rejects an escrow with no parent set at all', async () => { + await expect( + escrowRepo.query( + `INSERT INTO escrows (id, amount, asset, status) VALUES (gen_random_uuid(), '10', 'USDC', 'pending')`, + ), + ).rejects.toThrow(/CHK_escrow_exactly_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_exactly_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(); + }); + }); + + 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); + }); + }); +}); From 416a65c0fe44cdbb59794c05419a2a801ac06dec Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 08:30:37 +0100 Subject: [PATCH 10/13] docs: document the new migration workflow and integration test 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 #27. --- README.md | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) 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. From 6fe7bc8f49c1cb0ea956957e2932841ba22a857e Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 09:25:59 +0100 Subject: [PATCH 11/13] fix(escrow): relax CHECK constraint to at-most-one-parent, not exactly-one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- src/common/entities/escrow.entity.ts | 12 ++++++++++-- .../1784272650000-EscrowFkIntegrityAndSponsorId.ts | 13 +++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/common/entities/escrow.entity.ts b/src/common/entities/escrow.entity.ts index 0a6ac7e..ff2556f 100644 --- a/src/common/entities/escrow.entity.ts +++ b/src/common/entities/escrow.entity.ts @@ -29,15 +29,23 @@ import { AssetType, EscrowStatus } from '../enums'; * 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_exactly_one_parent', + '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`, + ) <= 1`, ) export class Escrow { @PrimaryGeneratedColumn('uuid') diff --git a/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts b/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts index 369c132..99eea9a 100644 --- a/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts +++ b/src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts @@ -17,8 +17,13 @@ import { MigrationInterface, QueryRunner } from 'typeorm'; * 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 exactly one of `bountyId` / + * 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'; @@ -72,20 +77,20 @@ export class EscrowFkIntegrityAndSponsorId1784272650000 implements MigrationInte await queryRunner.query(` ALTER TABLE "escrows" - ADD CONSTRAINT "CHK_escrow_exactly_one_parent" + 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 + ) <= 1 ) `); } public async down(queryRunner: QueryRunner): Promise { await queryRunner.query( - `ALTER TABLE "escrows" DROP CONSTRAINT IF EXISTS "CHK_escrow_exactly_one_parent"`, + `ALTER TABLE "escrows" DROP CONSTRAINT IF EXISTS "CHK_escrow_at_most_one_parent"`, ); await this.replaceForeignKeyOnDelete( From fe72b0e53ca5c99bf7d353751df8bedd5d0b4ccf Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 09:26:17 +0100 Subject: [PATCH 12/13] feat(escrow): enforce exactly-one-parent at creation time in EscrowService 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. --- src/escrow/escrow.service.spec.ts | 33 +++++++++++++++++++++++++++++++ src/escrow/escrow.service.ts | 23 +++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/escrow/escrow.service.spec.ts b/src/escrow/escrow.service.spec.ts index fc8c288..9078f10 100644 --- a/src/escrow/escrow.service.spec.ts +++ b/src/escrow/escrow.service.spec.ts @@ -99,6 +99,7 @@ describe('EscrowService', () => { amount: '10', asset: AssetType.XLM, funderAddress: 'G...', + bountyId: 'bounty-1', }), ).rejects.toThrow('simulation failed'); @@ -148,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 4fad228..eb35471 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -294,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 { From 73b86656ca247928e0571c7b6313dd119ebb31a3 Mon Sep 17 00:00:00 2001 From: Temi-suwa18 <271503102+Temi-suwa18@users.noreply.github.com> Date: Fri, 17 Jul 2026 09:26:17 +0100 Subject: [PATCH 13/13] test(db): update integration test for the at-most-one-parent constraint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../escrow-fk-integrity.integration.spec.ts | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/database/escrow-fk-integrity.integration.spec.ts b/src/database/escrow-fk-integrity.integration.spec.ts index 0de845a..83b1ec3 100644 --- a/src/database/escrow-fk-integrity.integration.spec.ts +++ b/src/database/escrow-fk-integrity.integration.spec.ts @@ -143,15 +143,7 @@ describe('Escrow FK integrity + sponsor dashboard reconciliation (integration)', ); } - describe('CHK_escrow_exactly_one_parent', () => { - it('rejects an escrow with no parent set at all', async () => { - await expect( - escrowRepo.query( - `INSERT INTO escrows (id, amount, asset, status) VALUES (gen_random_uuid(), '10', 'USDC', 'pending')`, - ), - ).rejects.toThrow(/CHK_escrow_exactly_one_parent/); - }); - + 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 @@ -166,7 +158,7 @@ describe('Escrow FK integrity + sponsor dashboard reconciliation (integration)', VALUES (gen_random_uuid(), $1, $2, '10', 'USDC', 'pending')`, [bounty.id, milestone.id], ), - ).rejects.toThrow(/CHK_escrow_exactly_one_parent/); + ).rejects.toThrow(/CHK_escrow_at_most_one_parent/); }); it('allows an escrow with exactly one parent set', async () => { @@ -185,6 +177,25 @@ describe('Escrow FK integrity + sponsor dashboard reconciliation (integration)', 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', () => {