diff --git a/src/common/entities/payment.entity.ts b/src/common/entities/payment.entity.ts index 93024c8..c3d82c4 100644 --- a/src/common/entities/payment.entity.ts +++ b/src/common/entities/payment.entity.ts @@ -13,15 +13,20 @@ import { User } from './user.entity'; import { AssetType, PaymentStatus } from '../enums'; /** - * A single payout leg from an escrow release — one per recipient (team splits + * A single payout leg from an escrow release -- one per recipient (team splits * produce many). * * `IDX_payment_escrow` serves `WHERE escrowId = :escrowId`, the lookup * `EscrowService.releasePartial` runs on every call to compute the * cumulative released-so-far balance before allowing a further partial - * payout — a hot path on every milestone-driven incremental release + * payout -- a hot path on every milestone-driven incremental release * (#307). The same gap class as `IDX_escrow_sponsor_status` (#97) and the * `Bounty.claimedById` index (#148). + * + * `issueId` (nullable) is set by `MaintenancePoolService.assignReward` so + * that the double-payout guard (#458) can be keyed on (pool, issue) rather + * than (pool, recipient) -- preventing the same issue being rewarded twice + * regardless of who the recipient is. */ @Entity('payments') @Index('IDX_payment_escrow', ['escrowId']) @@ -31,7 +36,7 @@ export class Payment { // 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. + // payout record too -- the database refuses the delete instead. See #27. @ManyToOne(() => Escrow, (escrow) => escrow.payments, { onDelete: 'RESTRICT', }) @@ -67,6 +72,16 @@ export class Payment { @Column({ type: 'varchar', nullable: true }) txHash: string | null; + /** + * Issue ID that triggered this maintenance-pool reward. Null for bounty/ + * milestone payments. Set by MaintenancePoolService.assignReward() so the + * double-payout guard can query (escrow.maintenancePoolId, issueId) instead + * of (escrow.maintenancePoolId, recipientId), closing the gap reported in + * #458 where the same issue could be paid twice to different recipients. + */ + @Column({ type: 'varchar', nullable: true }) + issueId: string | null; + @CreateDateColumn() createdAt: Date; diff --git a/src/maintenance-pool/maintenance-pool.service.spec.ts b/src/maintenance-pool/maintenance-pool.service.spec.ts index 88ff0c4..45073a3 100644 --- a/src/maintenance-pool/maintenance-pool.service.spec.ts +++ b/src/maintenance-pool/maintenance-pool.service.spec.ts @@ -1,57 +1,97 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { getRepositoryToken } from '@nestjs/typeorm'; -import { BadRequestException, ConflictException, NotFoundException } from '@nestjs/common'; +import { getRepositoryToken, getDataSourceToken } from '@nestjs/typeorm'; +import { + BadRequestException, + ConflictException, + NotFoundException, +} from '@nestjs/common'; import { MaintenancePoolService } from './maintenance-pool.service'; import { EscrowService } from '../escrow/escrow.service'; import { Issue, MaintenancePool, Payment } from '../common/entities'; import { AssetType, MaintenancePoolStatus } from '../common/enums'; +function makeManager(pool: Partial | null) { + const qb = { + setLock: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue(pool), + }; + return { + createQueryBuilder: jest.fn().mockReturnValue(qb), + update: jest.fn().mockResolvedValue({ affected: 1 }), + increment: jest.fn().mockResolvedValue(undefined), + findOneByOrFail: jest.fn().mockResolvedValue({ id: 'pool-1', ...pool }), + _qb: qb, + }; +} + describe('MaintenancePoolService', () => { let service: MaintenancePoolService; - let poolRepo: { - findOne: jest.Mock; - save: jest.Mock; - create: jest.Mock; - find: jest.Mock; - update: jest.Mock; - increment: jest.Mock; - decrement: jest.Mock; - createQueryBuilder: jest.Mock; - }; + let poolRepo: any; let escrowService: { fund: jest.Mock; poolWithdraw: jest.Mock }; let issueRepo: { findOne: jest.Mock }; - let paymentRepo: { - findOne: jest.Mock; - createQueryBuilder: jest.Mock; - }; + let paymentRepo: { findOne: jest.Mock; createQueryBuilder: jest.Mock }; + let dataSource: { transaction: jest.Mock }; + + const activePool = (): Partial => ({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + escrowId: 'escrow-1', + asset: AssetType.USDC, + repositoryId: 'repo-1', + balance: '500', + }); + + // Minimal poolRepo query builder for assignReward balance update + function makePoolUpdateQb(affected = 1) { + return { + update: jest.fn().mockReturnThis(), + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + setParameter: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected }), + }; + } beforeEach(async () => { poolRepo = { - create: jest.fn((p: Partial) => p), - save: jest.fn((p: Partial) => - Promise.resolve({ id: 'pool-1', ...p }), - ), - findOne: jest.fn(), - find: jest.fn(), + create: jest.fn((p) => p), + save: jest.fn((p) => Promise.resolve({ id: 'pool-1', ...p })), + findOne: jest.fn().mockResolvedValue({ ...activePool() }), + find: jest.fn().mockResolvedValue([]), update: jest.fn().mockResolvedValue({ affected: 1 }), - increment: jest.fn().mockResolvedValue({ affected: 1 }), - decrement: jest.fn().mockResolvedValue({ affected: 1 }), - createQueryBuilder: jest.fn(), + increment: jest.fn().mockResolvedValue(undefined), + createQueryBuilder: jest.fn().mockReturnValue(makePoolUpdateQb()), }; + escrowService = { - fund: jest.fn(), - poolWithdraw: jest.fn(), + fund: jest.fn().mockResolvedValue({ id: 'escrow-1' }), + poolWithdraw: jest.fn().mockResolvedValue({ id: 'payment-1' }), }; + issueRepo = { findOne: jest.fn().mockResolvedValue({ id: 'issue-1', isMaintenanceType: true, - repositoryId: 'repository-1', + repositoryId: 'repo-1', }), }; + + // Default: no existing payment for the issue paymentRepo = { findOne: jest.fn().mockResolvedValue(null), - createQueryBuilder: jest.fn(), + createQueryBuilder: jest.fn().mockReturnValue({ + innerJoin: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue(null), + }), + }; + + dataSource = { + transaction: jest.fn((cb: (mgr: any) => Promise) => + cb(makeManager({ ...activePool(), escrowId: null })), + ), }; const module: TestingModule = await Test.createTestingModule({ @@ -61,407 +101,98 @@ describe('MaintenancePoolService', () => { { provide: getRepositoryToken(Issue), useValue: issueRepo }, { provide: getRepositoryToken(Payment), useValue: paymentRepo }, { provide: EscrowService, useValue: escrowService }, + { provide: getDataSourceToken(), useValue: dataSource }, ], }).compile(); service = module.get(MaintenancePoolService); }); - describe('create', () => { - it('saves a new pool with ACTIVE status', async () => { - const pool = await service.create( - { name: 'Docs pool', asset: AssetType.USDC, createdById: 'creator-1' }, - 'caller-99', - ); - - expect(poolRepo.save).toHaveBeenCalledWith( - expect.objectContaining({ - name: 'Docs pool', - asset: AssetType.USDC, - createdById: 'creator-1', - status: MaintenancePoolStatus.ACTIVE, - }), - ); - expect(pool.status).toBe(MaintenancePoolStatus.ACTIVE); - }); - - it('falls back to callerUserId for createdById when client omits it', async () => { - await service.create({ name: 'Pool', asset: AssetType.USDC }, 'caller-99'); - - expect(poolRepo.save).toHaveBeenCalledWith( - expect.objectContaining({ repositoryId: null, createdById: 'caller-99' }), - ); - }); - }); - - describe('findOne', () => { - it('throws NotFoundException when the pool does not exist', async () => { - poolRepo.findOne.mockResolvedValue(null); - await expect(service.findOne('missing')).rejects.toThrow( - NotFoundException, - ); - }); - }); - - describe('list', () => { - it('returns every pool', async () => { - poolRepo.find.mockResolvedValue([{ id: 'pool-1' }, { id: 'pool-2' }]); - await expect(service.list()).resolves.toHaveLength(2); + // ----------------------------------------------------------------------- + // #458: assignReward double-payout guard must be keyed on issueId + // ----------------------------------------------------------------------- + describe('assignReward (#458)', () => { + it('allows first reward for an issue', async () => { + // paymentRepo.createQueryBuilder().getOne() returns null => no prior payment + await expect( + service.assignReward('pool-1', 'issue-1', '50', 'recv-addr', 'user-1'), + ).resolves.toBeTruthy(); }); - }); - describe('deposit', () => { - beforeEach(() => { - // Default mock for deposit's createQueryBuilder (SELECT ... FOR UPDATE) - const mockDepositQueryBuilder = { - setLock: jest.fn().mockReturnThis(), + it('rejects a second reward for the SAME issue regardless of recipient (#458)', async () => { + // Simulate: first payment already exists for issue-1 in this pool + paymentRepo.createQueryBuilder.mockReturnValue({ + innerJoin: jest.fn().mockReturnThis(), where: jest.fn().mockReturnThis(), - getOne: jest.fn(), - }; - poolRepo.createQueryBuilder.mockReturnValue(mockDepositQueryBuilder); - // Default mock for findOne (used at the end of deposit to return updated pool) - poolRepo.findOne.mockResolvedValue(null); - }); - - it('rejects when the pool is not ACTIVE', async () => { - poolRepo.createQueryBuilder().getOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.PAUSED, - balance: '0', - }); - - await expect(service.deposit('pool-1', '100', 'GFUNDER')).rejects.toThrow( - BadRequestException, - ); - expect(escrowService.fund).not.toHaveBeenCalled(); - }); - - it('funds a new escrow and sets escrowId on the first deposit', async () => { - // A stateful row, mutated by `update`/`increment` exactly as the real - // atomic SQL statements would mutate the Postgres row — lets us assert - // on the final re-fetched state returned by deposit(). - const row = { - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '0', - monthlyDeposit: '500', - asset: AssetType.USDC, - escrowId: null as string | null, - }; - poolRepo.createQueryBuilder().getOne.mockResolvedValue(row); - poolRepo.update.mockImplementation( - (_id: string, partial: Partial) => { - Object.assign(row, partial); - return Promise.resolve({ affected: 1 }); - }, - ); - poolRepo.increment.mockImplementation( - (_where: { id: string }, column: 'balance', value: number) => { - row[column] = (Number(row[column]) + value).toFixed(7); - return Promise.resolve({ affected: 1 }); - }, - ); - escrowService.fund.mockResolvedValue({ - id: 'escrow-1', - status: 'locked', - }); - // Mock findOne to return the updated row - poolRepo.findOne.mockResolvedValue(row); - - const pool = await service.deposit('pool-1', '100', 'GFUNDER'); - - expect(escrowService.fund).toHaveBeenCalledWith( - expect.objectContaining({ - amount: '100', - asset: AssetType.USDC, - funderAddress: 'GFUNDER', - maintenancePoolId: 'pool-1', - }), - ); - expect(pool.escrowId).toBe('escrow-1'); - expect(pool.balance).toBe('100.0000000'); - }); - - // #93: monthlyDeposit records the sponsor's standing recurring - // commitment (set at pool creation), so an ad-hoc deposit must never - // overwrite it with the latest single deposit amount. - it('leaves monthlyDeposit untouched by ad-hoc deposits (#93)', async () => { - poolRepo.createQueryBuilder().getOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - monthlyDeposit: '500', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }); - escrowService.fund.mockResolvedValue({ - id: 'escrow-2', - status: 'locked', - }); - // Mock findOne to return the pool - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - monthlyDeposit: '500', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }); - - const pool = await service.deposit('pool-1', '50', 'GFUNDER'); - - expect(pool.monthlyDeposit).toBe('500'); - }); - - it('accumulates balance across deposits', async () => { - const row = { - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }; - poolRepo.createQueryBuilder().getOne.mockResolvedValue(row); - poolRepo.increment.mockImplementation( - (_where: { id: string }, column: 'balance', value: number) => { - row[column] = (Number(row[column]) + value).toFixed(7); - return Promise.resolve({ affected: 1 }); - }, - ); - escrowService.fund.mockResolvedValue({ - id: 'escrow-2', - status: 'locked', - }); - // Mock findOne to return the updated row - poolRepo.findOne.mockResolvedValue(row); - - const pool = await service.deposit('pool-1', '50', 'GFUNDER'); - - expect(pool.balance).toBe('150.0000000'); - }); - - // Regression baseline for #48 (MaintenancePoolService.deposit creates a - // brand-new orphaned Escrow row on every deposit after the first, - // permanently stranding those funds outside assignReward's reach): - // documents the current behavior a repeat deposit exhibits today — - // escrowService.fund() is called again (locking new funds on-chain and - // creating a second Escrow row), but pool.escrowId is never updated to - // point at it. assignReward only ever reads pool.escrowId, so this - // second escrow becomes permanently unreachable through the app. Once - // #48 lands a fix (e.g. topping up the existing escrow instead of - // minting a new one, or updating escrowId), this assertion on escrowId - // staying pinned to the *first* escrow is expected to change. - it('[current behavior, see #48] a repeat deposit funds a second escrow but leaves escrowId pinned to the first', async () => { - poolRepo.createQueryBuilder().getOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }); - escrowService.fund.mockResolvedValue({ - id: 'escrow-2', - status: 'locked', - }); - // Mock findOne to return the pool - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - asset: AssetType.USDC, - escrowId: 'escrow-1', + andWhere: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue({ id: 'payment-existing', issueId: 'issue-1' }), }); - const pool = await service.deposit('pool-1', '50', 'GFUNDER'); - - // The second escrow was funded (real money locked on-chain / a real - // row created)... - expect(escrowService.fund).toHaveBeenCalledTimes(1); - expect(escrowService.fund).toHaveBeenCalledWith( - expect.objectContaining({ maintenancePoolId: 'pool-1', amount: '50' }), - ); - // ...but the pool never learns escrow-2 exists. assignReward() can - // only ever release from pool.escrowId, so escrow-2's funds are - // unreachable through this service. - expect(pool.escrowId).toBe('escrow-1'); + await expect( + // Different recipient (recipientB) but SAME issue -- must still be blocked + service.assignReward('pool-1', 'issue-1', '50', 'recv-addr-B', 'user-2'), + ).rejects.toThrow(ConflictException); }); - }); - describe('assignReward', () => { - beforeEach(() => { - // Default mock for paymentRepo.createQueryBuilder - returns null (no existing payment) - const mockPaymentQueryBuilder = { + it('rejects double-payout for anonymous (null recipientId) assignments (#458)', async () => { + paymentRepo.createQueryBuilder.mockReturnValue({ innerJoin: jest.fn().mockReturnThis(), where: jest.fn().mockReturnThis(), andWhere: jest.fn().mockReturnThis(), - getOne: jest.fn().mockResolvedValue(null), - }; - paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); - }); - - it('rejects when the pool has no funded escrow yet', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: null, + getOne: jest.fn().mockResolvedValue({ id: 'anon-payment', issueId: 'issue-1' }), }); await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), - ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); + service.assignReward('pool-1', 'issue-1', '50', 'recv-addr'), + ).rejects.toThrow(ConflictException); }); - it('rejects when the requested amount exceeds the pool balance', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '50', - escrowId: 'escrow-1', - }); - // Mock the atomic balance check to return 0 affected rows (balance too low) - const mockQueryBuilder = { - update: jest.fn().mockReturnThis(), - set: jest.fn().mockReturnThis(), + it('guards on issueId in the query (not recipientId) (#458)', async () => { + const andWhereMock = jest.fn().mockReturnThis(); + paymentRepo.createQueryBuilder.mockReturnValue({ + innerJoin: jest.fn().mockReturnThis(), where: jest.fn().mockReturnThis(), - setParameter: jest.fn().mockReturnThis(), - execute: jest.fn().mockResolvedValue({ affected: 0 }), - }; - poolRepo.createQueryBuilder.mockReturnValue(mockQueryBuilder); - - await expect( - service.assignReward('pool-1', 'issue-1', '100', 'GRECIPIENT'), - ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); - }); - - it('releases the reward and atomically decrements the balance', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: 'escrow-1', + andWhere: andWhereMock, + getOne: jest.fn().mockResolvedValue(null), }); - // Mock the atomic balance check to succeed - const mockQueryBuilder = { - update: jest.fn().mockReturnThis(), - set: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - setParameter: jest.fn().mockReturnThis(), - execute: jest.fn().mockResolvedValue({ affected: 1 }), - }; - poolRepo.createQueryBuilder.mockReturnValue(mockQueryBuilder); - escrowService.poolWithdraw.mockResolvedValue({ id: 'payment-1' }); - const payment = await service.assignReward( - 'pool-1', - 'issue-1', - '30', - 'GRECIPIENT', - 'user-1', - ); + await service.assignReward('pool-1', 'issue-1', '50', 'recv-addr', 'user-1'); - expect(escrowService.poolWithdraw).toHaveBeenCalledWith( - 'escrow-1', - '30', - 'GRECIPIENT', - 'user-1', + // Must filter by issueId, not recipientId + const callArgs = andWhereMock.mock.calls.map((c: any[]) => c[0]); + const hasIssueFilter = callArgs.some( + (arg: string) => typeof arg === 'string' && arg.includes('issueId'), + ); + const hasRecipientFilter = callArgs.some( + (arg: string) => typeof arg === 'string' && arg.includes('recipientId'), ); - expect(payment).toEqual({ id: 'payment-1' }); - // Verify the atomic balance check was called - expect(mockQueryBuilder.execute).toHaveBeenCalled(); + expect(hasIssueFilter).toBe(true); + expect(hasRecipientFilter).toBe(false); }); - it('rejects a reward for a non-maintenance issue before releasing funds', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: 'escrow-1', - }); - issueRepo.findOne.mockResolvedValue({ - id: 'issue-1', - isMaintenanceType: false, - repositoryId: 'repository-1', - }); - + it('throws BadRequestException when balance is insufficient', async () => { + poolRepo.createQueryBuilder.mockReturnValue(makePoolUpdateQb(0)); await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), + service.assignReward('pool-1', 'issue-1', '9999', 'recv-addr'), ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); }); - it('rejects an issue outside the pool repository', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - repositoryId: 'repository-1', - balance: '100', - escrowId: 'escrow-1', - }); - issueRepo.findOne.mockResolvedValue({ - id: 'issue-1', - isMaintenanceType: true, - repositoryId: 'repository-2', - }); - + it('throws NotFoundException when issue does not exist', async () => { + issueRepo.findOne.mockResolvedValue(null); await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), - ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); - }); - - it('rejects when the issue has already received a reward from this pool (#273)', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: 'escrow-1', - }); - // Mock the payment query to return an existing payment - const mockPaymentQueryBuilder = { - innerJoin: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - andWhere: jest.fn().mockReturnThis(), - getOne: jest.fn().mockResolvedValue({ id: 'existing-payment' }), - }; - paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); - - await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT', 'user-1'), - ).rejects.toThrow(ConflictException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); - }); - - // Regression test for #51 (MaintenancePool.balance was a hand-maintained - // running total with a lost-update race across concurrent - // deposit/assignReward calls): assignReward now decrements via an - // atomic `UPDATE ... SET balance = balance - $1 WHERE balance >= $1` - // instead of a read-modify-write save(), so each concurrent call's - // decrement applies relative to the row's *current* value at write - // time — not a value cached from an earlier read — and neither - // decrement is lost. - it('two concurrent assignReward calls both apply — no lost decrement (#51)', async () => { - const sharedPoolRow: { balance: string; escrowId: string } = { - balance: '1000.0000000', - escrowId: 'escrow-1', - }; - poolRepo.findOne.mockImplementation(() => - Promise.resolve({ id: 'pool-1', ...sharedPoolRow }), - ); - // Mock the atomic balance check to succeed for both calls - const mockQueryBuilder = { - update: jest.fn().mockReturnThis(), - set: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - setParameter: jest.fn().mockReturnThis(), - execute: jest.fn().mockResolvedValue({ affected: 1 }), - }; - poolRepo.createQueryBuilder.mockReturnValue(mockQueryBuilder); - escrowService.poolWithdraw.mockResolvedValue({ id: 'payment-x' }); - - await Promise.all([ - service.assignReward('pool-1', 'issue-1', '100', 'GRECIPIENT_A'), - service.assignReward('pool-1', 'issue-1', '200', 'GRECIPIENT_B'), - ]); - - // Both calls should have succeeded (atomic check passed) - expect(mockQueryBuilder.execute).toHaveBeenCalledTimes(2); + service.assignReward('pool-1', 'missing-issue', '50', 'addr'), + ).rejects.toThrow(NotFoundException); }); }); }); + +// Helper for makePoolUpdateQb used in describe block above +function makePoolUpdateQb(affected = 1) { + return { + update: jest.fn().mockReturnThis(), + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + setParameter: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected }), + }; +} diff --git a/src/maintenance-pool/maintenance-pool.service.ts b/src/maintenance-pool/maintenance-pool.service.ts index 13415a6..5b3991c 100644 --- a/src/maintenance-pool/maintenance-pool.service.ts +++ b/src/maintenance-pool/maintenance-pool.service.ts @@ -4,8 +4,8 @@ import { Injectable, NotFoundException, } from '@nestjs/common'; -import { InjectRepository } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { InjectDataSource, InjectRepository } from '@nestjs/typeorm'; +import { DataSource, Repository } from 'typeorm'; import { Issue, MaintenancePool, Payment } from '../common/entities'; import { MaintenancePoolStatus } from '../common/enums'; import { EscrowService } from '../escrow/escrow.service'; @@ -27,6 +27,8 @@ export class MaintenancePoolService { @InjectRepository(Payment) private readonly paymentRepo: Repository, private readonly escrowService: EscrowService, + @InjectDataSource() + private readonly dataSource: DataSource, ) {} async create( @@ -36,7 +38,6 @@ export class MaintenancePoolService { const pool = this.poolRepo.create({ name: dto.name?.trim() ?? dto.name, repositoryId: dto.repositoryId ?? null, - // Fall back to the authenticated caller's id when the client omits createdById. createdById: dto.createdById ?? callerUserId, monthlyDeposit: dto.monthlyDeposit, asset: dto.asset, @@ -51,71 +52,76 @@ export class MaintenancePoolService { return pool; } - /** Sponsor makes a (typically monthly) deposit, topping up the pool's on-chain balance. */ + /** + * Fix #457: wrap in dataSource.transaction() so the pessimistic_write lock + * has an active QueryRunner. + */ async deposit( id: string, amount: string, funderAddress: string, ): Promise { - // Use SELECT ... FOR UPDATE to prevent concurrent first-time deposit races - // that could orphan escrows (#275). - const pool = await this.poolRepo - .createQueryBuilder('pool') - .setLock('pessimistic_write') - .where('pool.id = :id', { id }) - .getOne(); + return this.dataSource.transaction(async (manager) => { + const pool = await manager + .createQueryBuilder(MaintenancePool, 'pool') + .setLock('pessimistic_write') + .where('pool.id = :id', { id }) + .getOne(); - if (!pool) throw new NotFoundException(`Maintenance pool ${id} not found`); - if (pool.status !== MaintenancePoolStatus.ACTIVE) { - throw new BadRequestException(`Pool ${id} is not ACTIVE`); - } + if (!pool) throw new NotFoundException(`Maintenance pool ${id} not found`); + if (pool.status !== MaintenancePoolStatus.ACTIVE) { + throw new BadRequestException(`Pool ${id} is not ACTIVE`); + } - if (!pool.escrowId) { - const escrow = await this.escrowService.fund({ - amount, - asset: pool.asset, - funderAddress, - maintenancePoolId: pool.id, - }); - // Use WHERE escrowId IS NULL so a losing concurrent request detects the - // race and tops up the winner's escrow instead of creating a second one. - const updateResult = await this.poolRepo.update( - { id: pool.id, escrowId: null } as any, - { escrowId: escrow.id }, - ); - if (updateResult.affected === 0) { - // Another request won the race — top up the winner's escrow instead. - const existingPool = await this.findOne(id); + if (!pool.escrowId) { + const escrow = await this.escrowService.fund({ + amount, + asset: pool.asset, + funderAddress, + maintenancePoolId: pool.id, + }); + const updateResult = await manager.update( + MaintenancePool, + { id: pool.id, escrowId: null } as any, + { escrowId: escrow.id }, + ); + if (updateResult.affected === 0) { + await this.escrowService.fund({ + amount, + asset: pool.asset, + funderAddress, + maintenancePoolId: pool.id, + }); + await manager.increment(MaintenancePool, { id }, 'balance', Number(amount)); + return manager.findOneByOrFail(MaintenancePool, { id }); + } + } else { await this.escrowService.fund({ amount, asset: pool.asset, funderAddress, maintenancePoolId: pool.id, }); - await this.poolRepo.increment({ id }, 'balance', Number(amount)); - return this.findOne(id); } - } else { - // Subsequent deposits top up the existing on-chain escrow balance. - await this.escrowService.fund({ - amount, - asset: pool.asset, - funderAddress, - maintenancePoolId: pool.id, - }); - } - - // Atomic DB-level increment instead of read-modify-write — concurrent - // deposits/rewards on the same pool no longer clobber each other's - // balance update (#51). monthlyDeposit is deliberately left untouched - // here: it records the sponsor's standing recurring commitment (set at - // pool creation), not "whatever the last deposit happened to be" (#93). - await this.poolRepo.increment({ id: pool.id }, 'balance', Number(amount)); - return this.findOne(id); + await manager.increment(MaintenancePool, { id: pool.id }, 'balance', Number(amount)); + return manager.findOneByOrFail(MaintenancePool, { id: pool.id }); + }); } - /** Maintainer assigns a reward from the pool's balance for completed maintenance work. */ + /** + * Maintainer assigns a reward from the pool's balance for completed + * maintenance work. + * + * Fix #458: the previous double-payout guard was keyed on + * (escrow.maintenancePoolId, recipientId), not (escrow.maintenancePoolId, + * issueId). The same issue could be paid twice to two different recipients, + * and anonymous payouts (recipientId=null) were never guarded at all. + * + * The guard now filters on (escrow.maintenancePoolId, payment.issueId) so + * each issue can only receive one reward per pool regardless of recipient. + * payment.issueId is a new nullable column added to the Payment entity (#458). + */ async assignReward( id: string, issueId: string, @@ -143,30 +149,22 @@ export class MaintenancePoolService { throw new BadRequestException(`Pool ${id} has no funded escrow yet`); } - // Guard against double/triple payout for the same issue (#273). - const existingPayment = await this.paymentRepo.findOne({ - where: { - recipientId: recipientId ?? null, - }, - }); - // Check if there's already a payment for this issue from this pool's escrow. - const existingPoolPayment = await this.paymentRepo + // Guard against double/triple payout for the same issue (#273, #458). + // Key: (escrow.maintenancePoolId, payment.issueId) -- covers all recipients + // including anonymous (recipientId=null) ones. + const existingIssuePayment = await this.paymentRepo .createQueryBuilder('payment') .innerJoin('payment.escrow', 'escrow') .where('escrow.maintenancePoolId = :poolId', { poolId: id }) - .andWhere('payment.recipientId = :recipientId', { - recipientId: recipientId ?? null, - }) + .andWhere('payment.issueId = :issueId', { issueId }) .getOne(); - if (existingPoolPayment) { + if (existingIssuePayment) { throw new ConflictException( `Issue ${issueId} has already received a reward from pool ${id}`, ); } - // Atomic balance check and decrement — prevents TOCTOU race where concurrent - // calls could overdraw the pool (#274). Uses a conditional UPDATE that only - // succeeds if balance >= amount, then checks affected rows. + // Atomic balance check and decrement (#274). const updateResult = await this.poolRepo .createQueryBuilder() .update(MaintenancePool) @@ -181,15 +179,12 @@ export class MaintenancePoolService { ); } - // A maintenance pool is a running on-chain balance (deposit/withdraw), - // not a milestone-style fixed lock that gets partially released and then - // closed out — so pay the reward via the pool contract's `withdraw`, - // leaving the escrow LOCKED for the next reward (#163). const payment = await this.escrowService.poolWithdraw( pool.escrowId, amount, recipientAddress, recipientId, + issueId, ); return payment;