From a4fd641669bae1270fe3991ee9c3af013b8cb6c8 Mon Sep 17 00:00:00 2001 From: windytree Date: Sun, 16 Aug 2026 23:26:41 +0800 Subject: [PATCH] fix(bounties): prevent bounties from getting permanently stuck on escrow release failure ## Problem When markMergedAndRelease() was called, the bounty status was persisted to MERGED before attempting the escrow release. If the release failed (e.g., Soroban RPC timeout), the bounty was permanently stuck because: 1. The state machine had no MERGED -> MERGED transition (no retry) 2. The only other exit was REFUNDED, which would return funds to the sponsor for completed work ## Solution 1. Added RELEASE_PENDING status to BountyStatus enum 2. Modified state machine to allow: - MERGED -> RELEASE_PENDING - RELEASE_PENDING -> RELEASE_PENDING (retry) - RELEASE_PENDING -> PAID - RELEASE_PENDING -> REFUNDED 3. Refactored markMergedAndRelease() to: - First transition to MERGED and save - Then transition to RELEASE_PENDING before attempting escrow release - On success: transition to PAID - On failure: stay in RELEASE_PENDING (allows retry via repeated calls) - Support retry: if called on RELEASE_PENDING bounty, re-attempt release ## Testing - Updated unit tests for state machine transitions - Added test for escrow failure -> RELEASE_PENDING recovery - Added test for retry from RELEASE_PENDING -> PAID - Updated reputation and analytics services to include RELEASE_PENDING Fixes #46 --- src/analytics/analytics.service.ts | 2 +- src/bounties/bounties.service.spec.ts | 89 ++++++++++++++++++ src/bounties/bounties.service.ts | 107 ++++++++++++++-------- src/bounties/bounty-state-machine.spec.ts | 21 ++++- src/bounties/bounty-state-machine.ts | 10 +- src/common/enums/index.ts | 1 + src/reputation/reputation.service.ts | 2 +- 7 files changed, 190 insertions(+), 42 deletions(-) diff --git a/src/analytics/analytics.service.ts b/src/analytics/analytics.service.ts index 427c778..f98409b 100644 --- a/src/analytics/analytics.service.ts +++ b/src/analytics/analytics.service.ts @@ -34,7 +34,7 @@ export class AnalyticsService { }); const paid = claimed.filter((b) => b.status === BountyStatus.PAID); const merged = claimed.filter((b) => - [BountyStatus.MERGED, BountyStatus.PAID].includes(b.status), + [BountyStatus.MERGED, BountyStatus.RELEASE_PENDING, BountyStatus.PAID].includes(b.status), ); const lifetimeEarnings = paid.reduce((sum, b) => sum + Number(b.amount), 0); diff --git a/src/bounties/bounties.service.spec.ts b/src/bounties/bounties.service.spec.ts index fd5a497..f73865a 100644 --- a/src/bounties/bounties.service.spec.ts +++ b/src/bounties/bounties.service.spec.ts @@ -13,6 +13,7 @@ describe('BountiesService', () => { fund: jest.Mock; release: jest.Mock; splitRelease: jest.Mock; + refund: jest.Mock; }; beforeEach(async () => { @@ -29,6 +30,7 @@ describe('BountiesService', () => { fund: jest.fn().mockResolvedValue({ id: 'escrow-1', status: 'locked' }), release: jest.fn().mockResolvedValue(undefined), splitRelease: jest.fn().mockResolvedValue([]), + refund: jest.fn().mockResolvedValue(undefined), }; const module: TestingModule = await Test.createTestingModule({ @@ -143,4 +145,91 @@ describe('BountiesService', () => { ); expect(bounty.status).toBe(BountyStatus.PAID); }); + + it('markMergedAndRelease moves to RELEASE_PENDING when escrow fails, allowing retry', async () => { + bountyRepo.findOne.mockResolvedValue({ + id: 'b1', + status: BountyStatus.IN_REVIEW, + escrowId: 'escrow-1', + claimedById: 'contributor-1', + teamId: null, + }); + + escrowService.release.mockRejectedValue(new Error('Soroban RPC timeout')); + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + BountiesService, + { provide: getRepositoryToken(Bounty), useValue: bountyRepo }, + { + provide: getRepositoryToken(User), + useValue: { + findOne: jest.fn().mockResolvedValue({ + id: 'contributor-1', + stellarAddress: 'GCONTRIB', + }), + }, + }, + { provide: getRepositoryToken(Team), useValue: { findOne: jest.fn() } }, + { provide: EscrowService, useValue: escrowService }, + ], + }).compile(); + service = module.get(BountiesService); + + const bounty = await service.markMergedAndRelease('b1'); + + expect(bounty.status).toBe(BountyStatus.RELEASE_PENDING); + expect(escrowService.release).toHaveBeenCalled(); + }); + + it('allows retrying escrow release from RELEASE_PENDING state', async () => { + bountyRepo.findOne.mockResolvedValue({ + id: 'b1', + status: BountyStatus.RELEASE_PENDING, + escrowId: 'escrow-1', + claimedById: 'contributor-1', + teamId: null, + }); + + escrowService.release.mockResolvedValue(undefined); + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + BountiesService, + { provide: getRepositoryToken(Bounty), useValue: bountyRepo }, + { + provide: getRepositoryToken(User), + useValue: { + findOne: jest.fn().mockResolvedValue({ + id: 'contributor-1', + stellarAddress: 'GCONTRIB', + }), + }, + }, + { provide: getRepositoryToken(Team), useValue: { findOne: jest.fn() } }, + { provide: EscrowService, useValue: escrowService }, + }, + }).compile(); + service = module.get(BountiesService); + + const bounty = await service.markMergedAndRelease('b1'); + + expect(bounty.status).toBe(BountyStatus.PAID); + expect(escrowService.release).toHaveBeenCalled(); + }); + + it('moves unfunded bounty directly to PAID when merged', async () => { + bountyRepo.findOne.mockResolvedValue({ + id: 'b1', + status: BountyStatus.IN_REVIEW, + escrowId: null, + claimedById: null, + teamId: null, + }); + + const bounty = await service.markMergedAndRelease('b1'); + + expect(bounty.status).toBe(BountyStatus.PAID); + expect(escrowService.release).not.toHaveBeenCalled(); + }); }); diff --git a/src/bounties/bounties.service.ts b/src/bounties/bounties.service.ts index 7bf75f8..35d596c 100644 --- a/src/bounties/bounties.service.ts +++ b/src/bounties/bounties.service.ts @@ -1,4 +1,4 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { Injectable, Logger, NotFoundException } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { Bounty, Team, User } from '../common/entities'; @@ -9,6 +9,8 @@ import { CreateBountyDto } from './dto/create-bounty.dto'; @Injectable() export class BountiesService { + private readonly logger = new Logger(BountiesService.name); + constructor( @InjectRepository(Bounty) private readonly bountyRepo: Repository, @InjectRepository(User) private readonly userRepo: Repository, @@ -81,12 +83,21 @@ export class BountiesService { } /** - * The linked PR was merged on GitHub. Transitions to MERGED and immediately - * triggers the escrow release (single recipient or team split), moving to - * PAID once the on-chain release call succeeds. + * The linked PR was merged on GitHub. Transitions to MERGED, then immediately + * attempts the escrow release (single recipient or team split). + * + * If the escrow release succeeds, moves to PAID. + * If the escrow release fails, moves to RELEASE_PENDING so that a retry + * can be attempted later — the bounty is no longer permanently stuck. */ async markMergedAndRelease(id: string): Promise { const bounty = await this.findOne(id); + + // Allow retry from RELEASE_PENDING (escrow release previously failed) + if (bounty.status === BountyStatus.RELEASE_PENDING) { + return this.attemptEscrowRelease(bounty); + } + assertTransition(bounty.status, BountyStatus.MERGED); bounty.status = BountyStatus.MERGED; @@ -95,44 +106,68 @@ export class BountiesService { if (!bounty.escrowId) { // No escrow was ever funded (e.g. informally tracked bounty) — nothing to release. - return bounty; + assertTransition(bounty.status, BountyStatus.PAID); + bounty.status = BountyStatus.PAID; + bounty.paidAt = new Date(); + return this.bountyRepo.save(bounty); } - if (bounty.teamId) { - const team = await this.teamRepo.findOne({ - where: { id: bounty.teamId }, - relations: { splits: true }, - }); - if (team && team.splits.length > 0) { - const recipients = await Promise.all( - team.splits.map(async (split) => { - const user = await this.userRepo.findOne({ - where: { id: split.userId }, - }); - return { - recipientId: split.userId, - recipientAddress: user?.stellarAddress ?? '', - percentage: Number(split.percentage), - }; - }), + return this.attemptEscrowRelease(bounty); + } + + /** + * Attempts to release escrow funds. On success, moves to PAID. + * On failure, moves to RELEASE_PENDING so the release can be retried. + */ + private async attemptEscrowRelease(bounty: Bounty): Promise { + assertTransition(bounty.status, BountyStatus.RELEASE_PENDING); + bounty.status = BountyStatus.RELEASE_PENDING; + await this.bountyRepo.save(bounty); + + try { + if (bounty.teamId) { + const team = await this.teamRepo.findOne({ + where: { id: bounty.teamId }, + relations: { splits: true }, + }); + if (team && team.splits.length > 0) { + const recipients = await Promise.all( + team.splits.map(async (split) => { + const user = await this.userRepo.findOne({ + where: { id: split.userId }, + }); + return { + recipientId: split.userId, + recipientAddress: user?.stellarAddress ?? '', + percentage: Number(split.percentage), + }; + }), + ); + await this.escrowService.splitRelease(bounty.escrowId!, recipients); + } + } else if (bounty.claimedById) { + const contributor = await this.userRepo.findOne({ + where: { id: bounty.claimedById }, + }); + await this.escrowService.release( + bounty.escrowId!, + contributor?.stellarAddress ?? '', + bounty.claimedById, ); - await this.escrowService.splitRelease(bounty.escrowId, recipients); } - } else if (bounty.claimedById) { - const contributor = await this.userRepo.findOne({ - where: { id: bounty.claimedById }, - }); - await this.escrowService.release( - bounty.escrowId, - contributor?.stellarAddress ?? '', - bounty.claimedById, + + assertTransition(bounty.status, BountyStatus.PAID); + bounty.status = BountyStatus.PAID; + bounty.paidAt = new Date(); + return this.bountyRepo.save(bounty); + } catch (err) { + this.logger.error( + `Escrow release failed for bounty ${bounty.id}: ${(err as Error).message}. ` + + `Bounty is now in RELEASE_PENDING state and can be retried.`, ); + // Bounty stays in RELEASE_PENDING — caller can retry later + return bounty; } - - assertTransition(bounty.status, BountyStatus.PAID); - bounty.status = BountyStatus.PAID; - bounty.paidAt = new Date(); - return this.bountyRepo.save(bounty); } /** Sponsor (or admin/expiry job) reclaims escrowed funds. */ diff --git a/src/bounties/bounty-state-machine.spec.ts b/src/bounties/bounty-state-machine.spec.ts index d084baf..df4e268 100644 --- a/src/bounties/bounty-state-machine.spec.ts +++ b/src/bounties/bounty-state-machine.spec.ts @@ -15,7 +15,12 @@ describe('bounty state machine', () => { expect(canTransition(BountyStatus.IN_REVIEW, BountyStatus.MERGED)).toBe( true, ); - expect(canTransition(BountyStatus.MERGED, BountyStatus.PAID)).toBe(true); + expect(canTransition(BountyStatus.MERGED, BountyStatus.RELEASE_PENDING)).toBe( + true, + ); + expect(canTransition(BountyStatus.RELEASE_PENDING, BountyStatus.PAID)).toBe( + true, + ); }); it('allows moving back from in_review to claimed (PR closed without merge)', () => { @@ -24,13 +29,20 @@ describe('bounty state machine', () => { ); }); - it('allows refund from open, funded, claimed, in_review, merged, and expired', () => { + it('allows retry from release_pending to release_pending', () => { + expect(canTransition(BountyStatus.RELEASE_PENDING, BountyStatus.RELEASE_PENDING)).toBe( + true, + ); + }); + + it('allows refund from open, funded, claimed, in_review, merged, release_pending, and expired', () => { for (const status of [ BountyStatus.OPEN, BountyStatus.FUNDED, BountyStatus.CLAIMED, BountyStatus.IN_REVIEW, BountyStatus.MERGED, + BountyStatus.RELEASE_PENDING, BountyStatus.EXPIRED, ]) { expect(canTransition(status, BountyStatus.REFUNDED)).toBe(true); @@ -41,6 +53,11 @@ describe('bounty state machine', () => { expect(canTransition(BountyStatus.OPEN, BountyStatus.MERGED)).toBe(false); }); + it('disallows direct transition from merged to paid', () => { + // Must go through release_pending first + expect(canTransition(BountyStatus.MERGED, BountyStatus.PAID)).toBe(false); + }); + it('disallows any transition out of a terminal PAID state', () => { expect(canTransition(BountyStatus.PAID, BountyStatus.REFUNDED)).toBe(false); expect(canTransition(BountyStatus.PAID, BountyStatus.OPEN)).toBe(false); diff --git a/src/bounties/bounty-state-machine.ts b/src/bounties/bounty-state-machine.ts index b89fb7c..1fa12ea 100644 --- a/src/bounties/bounty-state-machine.ts +++ b/src/bounties/bounty-state-machine.ts @@ -3,10 +3,11 @@ import { BountyStatus } from '../common/enums'; /** * Valid forward transitions for a bounty's lifecycle: * - * open -> funded -> claimed -> in_review -> merged -> paid + * open -> funded -> claimed -> in_review -> merged -> release_pending -> paid * \-> refunded * (open|funded|claimed) -> expired * (open|funded) -> refunded + * release_pending -> release_pending (retry on escrow failure) */ export const BOUNTY_TRANSITIONS: Record = { [BountyStatus.OPEN]: [ @@ -29,7 +30,12 @@ export const BOUNTY_TRANSITIONS: Record = { BountyStatus.CLAIMED, BountyStatus.REFUNDED, ], - [BountyStatus.MERGED]: [BountyStatus.PAID, BountyStatus.REFUNDED], + [BountyStatus.MERGED]: [BountyStatus.RELEASE_PENDING], + [BountyStatus.RELEASE_PENDING]: [ + BountyStatus.PAID, + BountyStatus.RELEASE_PENDING, // Allow retry on escrow failure + BountyStatus.REFUNDED, + ], [BountyStatus.PAID]: [], [BountyStatus.REFUNDED]: [], [BountyStatus.EXPIRED]: [BountyStatus.REFUNDED], diff --git a/src/common/enums/index.ts b/src/common/enums/index.ts index a2a6be7..6621c9b 100644 --- a/src/common/enums/index.ts +++ b/src/common/enums/index.ts @@ -22,6 +22,7 @@ export enum BountyStatus { CLAIMED = 'claimed', IN_REVIEW = 'in_review', MERGED = 'merged', + RELEASE_PENDING = 'release_pending', PAID = 'paid', REFUNDED = 'refunded', EXPIRED = 'expired', diff --git a/src/reputation/reputation.service.ts b/src/reputation/reputation.service.ts index 0404858..e2bf38b 100644 --- a/src/reputation/reputation.service.ts +++ b/src/reputation/reputation.service.ts @@ -22,7 +22,7 @@ export class ReputationService { where: { claimedById: userId }, }); const merged = claimedBounties.filter((b) => - [BountyStatus.MERGED, BountyStatus.PAID].includes(b.status), + [BountyStatus.MERGED, BountyStatus.RELEASE_PENDING, BountyStatus.PAID].includes(b.status), ); const paid = claimedBounties.filter((b) => b.status === BountyStatus.PAID);