diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 6d2dd289..c252993f 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -584,20 +584,14 @@ export class EscrowService { recipients: Array<[string, number]>, manager?: EntityManager, ): Promise { - return this.invokeOnLockedEscrow(escrow, operation, () => - this.soroban.invoke( - 'release', - // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301). - [u64(this.onChainKeyFor(escrow)), recipients], - this.contractOpts(escrow), - ), return this.invokeOnLockedEscrow( escrow, operation, () => this.soroban.invoke( 'release', - [this.onChainKeyFor(escrow), recipients], + // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301). + [u64(this.onChainKeyFor(escrow)), recipients], this.contractOpts(escrow), ), manager, diff --git a/src/github/github-webhooks.service.spec.ts b/src/github/github-webhooks.service.spec.ts index 45bb6fe8..a35ac3be 100644 --- a/src/github/github-webhooks.service.spec.ts +++ b/src/github/github-webhooks.service.spec.ts @@ -869,6 +869,9 @@ describe('GithubWebhooksService', () => { expect(bountiesService.markInReview).not.toHaveBeenCalled(); expect(bountiesService.markMergedAndRelease).not.toHaveBeenCalled(); + }); + }); + // #308: a redelivered X-GitHub-Delivery used to hit the unique constraint // on webhook_events.deliveryId and escape handleEvent as a 500, so every // redelivery of that event failed forever. diff --git a/src/milestones/milestones.controller.spec.ts b/src/milestones/milestones.controller.spec.ts index 8d87497d..06a90603 100644 --- a/src/milestones/milestones.controller.spec.ts +++ b/src/milestones/milestones.controller.spec.ts @@ -1,3 +1,4 @@ +import type { Request } from 'express'; import { Test, TestingModule } from '@nestjs/testing'; import { plainToInstance } from 'class-transformer'; import { validate } from 'class-validator'; @@ -5,8 +6,10 @@ import { MilestonesController } from './milestones.controller'; import { MilestonesService } from './milestones.service'; import { CreateMilestoneDto } from './dto/create-milestone.dto'; import { AssetType, MilestoneStatus } from '../common/enums'; +import { getRepositoryToken } from '@nestjs/typeorm'; import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; import { RolesGuard } from '../auth/guards/roles.guard'; +import { IdempotencyKey } from '../common/entities/idempotency-key.entity'; describe('MilestonesController', () => { let controller: MilestonesController; @@ -33,7 +36,13 @@ describe('MilestonesController', () => { const module: TestingModule = await Test.createTestingModule({ controllers: [MilestonesController], - providers: [{ provide: MilestonesService, useValue: milestonesService }], + providers: [ + { provide: MilestonesService, useValue: milestonesService }, + { + provide: getRepositoryToken(IdempotencyKey), + useValue: { findOne: jest.fn(), save: jest.fn(), create: jest.fn() }, + }, + ], }) .overrideGuard(JwtAuthGuard) .useValue({ canActivate: () => true }) @@ -53,9 +62,14 @@ describe('MilestonesController', () => { asset: AssetType.USDC, }; - await controller.create(dto); + await controller.create(dto, { + user: { userId: 'sponsor-1' }, + } as unknown as Request); - expect(milestonesService.create).toHaveBeenCalledWith(dto); + expect(milestonesService.create).toHaveBeenCalledWith( + dto, + 'sponsor-1', + ); }); }); @@ -77,9 +91,15 @@ describe('MilestonesController', () => { describe('fund', () => { it('calls milestonesService.fund with id and funderAddress', async () => { - await controller.fund('m1', { funderAddress: 'GFUNDER' }); - - expect(milestonesService.fund).toHaveBeenCalledWith('m1', 'GFUNDER'); + await controller.fund('m1', { funderAddress: 'GFUNDER' }, { + user: { userId: 'sponsor-1' }, + } as unknown as Request); + + expect(milestonesService.fund).toHaveBeenCalledWith( + 'm1', + 'GFUNDER', + 'sponsor-1', + ); }); }); diff --git a/src/milestones/milestones.controller.ts b/src/milestones/milestones.controller.ts index 0aaf9d99..c6c30245 100644 --- a/src/milestones/milestones.controller.ts +++ b/src/milestones/milestones.controller.ts @@ -143,4 +143,19 @@ export class MilestonesController { allocateBudget(@Param('id', new ParseUUIDPipe()) id: string) { return this.milestonesService.allocateBudget(id); } + + @ApiOperation({ + summary: 'Close a milestone and refund its unspent balance', + }) + @ApiBearerAuth() + @ApiStandardErrorResponses() + @UseGuards(JwtAuthGuard) + @Post(':id/close') + close(@Param('id', new ParseUUIDPipe()) id: string, @Req() req: Request) { + // There was no way to reach MilestoneStatus.CLOSED or refund the unspent + // escrow — a sponsor couldn't reclaim an unspent budget (#587). Sponsor- + // only; the service enforces it. + const userId = (req.user as { userId?: string }).userId; + return this.milestonesService.close(id, userId); + } } diff --git a/src/milestones/milestones.service.spec.ts b/src/milestones/milestones.service.spec.ts index 612bde8d..9c451dcc 100644 --- a/src/milestones/milestones.service.spec.ts +++ b/src/milestones/milestones.service.spec.ts @@ -1,11 +1,12 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { getRepositoryToken } from '@nestjs/typeorm'; import { DataSource } from 'typeorm'; import { MilestonesService } from './milestones.service'; import { EscrowService } from '../escrow/escrow.service'; import { Issue, Milestone } from '../common/entities'; import { AssetType, MilestoneStatus } from '../common/enums'; +import { InvalidMilestoneTransitionError } from './milestone-state-machine'; describe('MilestonesService', () => { let service: MilestonesService; @@ -21,6 +22,7 @@ describe('MilestonesService', () => { let escrowService: { fund: jest.Mock; releasePartial: jest.Mock; + refund: jest.Mock; }; let dataSource: { transaction: jest.Mock; @@ -43,6 +45,7 @@ describe('MilestonesService', () => { amount: '100', status: 'confirmed', }), + refund: jest.fn().mockResolvedValue(undefined), }; dataSource = { transaction: jest.fn().mockImplementation((fn: Function) => @@ -75,7 +78,7 @@ describe('MilestonesService', () => { sponsorId: 'sponsor-1', }); - const milestone = await service.fund('m1', 'GFUNDER'); + const milestone = await service.fund('m1', 'GFUNDER', 'sponsor-1'); expect(milestone.status).toBe(MilestoneStatus.FUNDED); expect(milestone.escrowId).toBe('escrow-1'); @@ -90,7 +93,7 @@ describe('MilestonesService', () => { sponsorId: 'sponsor-1', }); - await service.fund('m1', 'GFUNDER'); + await service.fund('m1', 'GFUNDER', 'sponsor-1'); expect(escrowService.fund).toHaveBeenCalledWith( expect.objectContaining({ @@ -110,7 +113,7 @@ describe('MilestonesService', () => { sponsorId: null, }); - await service.fund('m2', 'GFUNDER'); + await service.fund('m2', 'GFUNDER', 'sponsor-1'); expect(escrowService.fund).toHaveBeenCalledWith( expect.objectContaining({ sponsorId: null }), @@ -123,7 +126,7 @@ describe('MilestonesService', () => { status: MilestoneStatus.FUNDED, }); - await expect(service.fund('m1', 'GFUNDER')).rejects.toThrow( + await expect(service.fund('m1', 'GFUNDER', 'sponsor-1')).rejects.toThrow( 'Milestone m1 is not OPEN (current: funded)', ); }); @@ -263,6 +266,7 @@ describe('MilestonesService', () => { '250.0000000', 'RECIPIENT_ADDR', 'user-1', + expect.anything(), ); expect(payment.id).toBe('payment-1'); expect(dataSource.transaction).toHaveBeenCalled(); @@ -341,7 +345,7 @@ describe('MilestonesService', () => { await expect( service.resolveIssue('m1', 'i1', 'RECIPIENT_ADDR'), - ).rejects.toThrow('Milestone m1 is not accepting distributions'); + ).rejects.toThrow(InvalidMilestoneTransitionError); }); it('rejects when the issue does not belong to the milestone (#114)', async () => { @@ -406,7 +410,53 @@ describe('MilestonesService', () => { '333.3333333', 'RECIPIENT_ADDR', undefined, + expect.anything(), ); }); }); + + describe('close', () => { + it('refunds the escrow and moves the milestone to CLOSED', async () => { + milestoneRepo.findOne.mockResolvedValue({ + id: 'm1', + status: MilestoneStatus.FUNDED, + escrowId: 'escrow-1', + sponsorId: 'sponsor-1', + }); + + const closed = await service.close('m1', 'sponsor-1'); + + expect(escrowService.refund).toHaveBeenCalledWith('escrow-1'); + expect(closed.status).toBe(MilestoneStatus.CLOSED); + expect(milestoneRepo.save).toHaveBeenCalled(); + }); + + it('rejects a caller that is not the milestone sponsor (#587)', async () => { + milestoneRepo.findOne.mockResolvedValue({ + id: 'm1', + status: MilestoneStatus.FUNDED, + escrowId: 'escrow-1', + sponsorId: 'sponsor-1', + }); + + await expect(service.close('m1', 'mallory')).rejects.toThrow( + ForbiddenException, + ); + expect(escrowService.refund).not.toHaveBeenCalled(); + }); + + it('skips the escrow refund when nothing was funded', async () => { + milestoneRepo.findOne.mockResolvedValue({ + id: 'm1', + status: MilestoneStatus.OPEN, + escrowId: null, + sponsorId: 'sponsor-1', + }); + + const closed = await service.close('m1', 'sponsor-1'); + + expect(escrowService.refund).not.toHaveBeenCalled(); + expect(closed.status).toBe(MilestoneStatus.CLOSED); + }); + }); }); diff --git a/src/milestones/milestones.service.ts b/src/milestones/milestones.service.ts index c2b7ee8b..7ed7399a 100644 --- a/src/milestones/milestones.service.ts +++ b/src/milestones/milestones.service.ts @@ -130,6 +130,30 @@ export class MilestonesService { return this.issueRepo.save(issue); } + /** + * Sponsor-only close — transitions the milestone to CLOSED and refunds the + * escrow's remaining unspent balance. MilestoneStatus.CLOSED existed and the + * state machine allowed (OPEN|FUNDED|IN_PROGRESS) -> CLOSED, but no service/ + * controller path reached it (#587) — a sponsor had no way to reclaim the + * unspent part of a milestone budget. + */ + async close(milestoneId: string, callerUserId: string): Promise { + const milestone = await this.findOne(milestoneId); + if (milestone.sponsorId && milestone.sponsorId !== callerUserId) { + throw new ForbiddenException('Only the milestone sponsor can close it'); + } + assertTransition(milestone.status, MilestoneStatus.CLOSED); + + // Refund the remaining escrow balance — the sponsor keeps what wasn't + // distributed. releasePartial paid the contributors; refund returns the + // rest (#587). + if (milestone.escrowId) { + await this.escrowService.refund(milestone.escrowId); + } + milestone.status = MilestoneStatus.CLOSED; + return this.milestoneRepo.save(milestone); + } + /** * Distributes this milestone's budget proportionally as its issues resolve. * TODO: support per-issue weighted budgets; currently splits the remaining diff --git a/src/sponsors/sponsors.controller.ts b/src/sponsors/sponsors.controller.ts index b7a48cca..c097bf1e 100644 --- a/src/sponsors/sponsors.controller.ts +++ b/src/sponsors/sponsors.controller.ts @@ -26,25 +26,6 @@ function validatePage(limit?: number, offset?: number) { throw new BadRequestException('offset must be >= 0'); } return { limit, offset }; -/** - * Parse optional limit/offset query params, rejecting NaN, negatives, - * non-integers, and limits above MAX_PAGE_LIMIT with a 400 (#280). - */ -function parsePagination(limit?: string, offset?: string) { - const parse = (name: string, raw: string | undefined, min: number, max: number) => { - if (raw === undefined || raw === '') return undefined; - const n = Number(raw); - if (!Number.isInteger(n) || n < min || n > max) { - throw new BadRequestException( - `${name} must be an integer between ${min} and ${max}`, - ); - } - return n; - }; - return { - limit: parse('limit', limit, 1, MAX_PAGE_LIMIT), - offset: parse('offset', offset, 0, Number.MAX_SAFE_INTEGER), - }; } @ApiTags('sponsors') @@ -73,7 +54,6 @@ export class SponsorsController { ) { this.assertOwnsSponsor(user, id); return this.sponsorsService.dashboard(id, validatePage(limit, offset)); - return this.sponsorsService.dashboard(id, parsePagination(limit, offset)); } @ApiBearerAuth() @@ -91,6 +71,5 @@ export class SponsorsController { id, validatePage(limit, offset), ); - return this.sponsorsService.milestoneProgress(id, parsePagination(limit, offset)); } }