#44 release() and splitRelease() never check for pre-existing Payment rows, so calling them after releasePartial causes a double payout of the full escrow amount FIXED

src/escrow/escrow.service.spec.ts

@@ -10,6 +10,9 @@ import { AssetType, EscrowStatus, PaymentStatus } from '../common/enums';
 describe('EscrowService', () => {
   let service: EscrowService;
   let escrowRepo: { create: jest.Mock; save: jest.Mock; findOne: jest.Mock };
+-
+  let paymentRepo: { create: jest.Mock; save: jest.Mock; find: jest.Mock };
+
   let userRepo: { find: jest.Mock };
   let paymentRepo: {
     create: jest.Mock;
@@ -23,6 +26,7 @@ describe('EscrowService', () => {
     tokenContractId: jest.Mock;
     escrowDeadlineSeconds: number;
   };
+
   let soroban: { invoke: jest.Mock };
   let dataSource: { transaction: jest.Mock };
 
@@ -33,6 +37,7 @@ describe('EscrowService', () => {
       findOne: jest.fn(),
     };
     paymentRepo = {
+      find: jest.fn().mockResolvedValue([]),
       create: jest.fn((data: Partial<Payment>) => ({
         id: 'payment-1',
         ...data,
@@ -552,6 +557,74 @@ describe('EscrowService', () => {
     });
   });
 
+  it('rejects a full release after a partial release on the same still-LOCKED escrow', async () => {
+    const escrow = {
+      id: 'escrow-partial-then-full',
+      status: EscrowStatus.LOCKED,
+      amount: '100',
+      asset: AssetType.USDC,
+      milestoneId: 'milestone-1',
+    } as Escrow;
+    const payments: Partial<Payment>[] = [];
+    escrowRepo.findOne.mockResolvedValue(escrow);
+    paymentRepo.find.mockImplementation(() => Promise.resolve(payments));
+    paymentRepo.save.mockImplementation((payment: Partial<Payment>) => {
+      payments.push(payment);
+      return payment;
+    });
+
+    await service.releasePartial(
+      'escrow-partial-then-full',
+      '50',
+      'GRECIPIENT',
+    );
+    expect(escrow.status).toBe(EscrowStatus.LOCKED);
+
+    await expect(
+      service.release('escrow-partial-then-full', 'GATTACKER'),
+    ).rejects.toThrow(BadRequestException);
+    expect(soroban.invoke).toHaveBeenCalledTimes(1);
+    expect(soroban.invoke).toHaveBeenCalledWith('release', [
+      'milestone-1',
+      'GRECIPIENT',
+      500000000n,
+    ]);
+  });
+
+  it('rejects splitRelease when an escrow has prior payment history', async () => {
+    escrowRepo.findOne.mockResolvedValue({
+      id: 'escrow-split-after-partial',
+      status: EscrowStatus.LOCKED,
+      amount: '100',
+      asset: AssetType.USDC,
+      milestoneId: 'milestone-2',
+    });
+    paymentRepo.find.mockResolvedValue([{ amount: '25' }]);
+
+    await expect(
+      service.splitRelease('escrow-split-after-partial', [
+        { recipientAddress: 'GA', percentage: 100 },
+      ]),
+    ).rejects.toThrow(BadRequestException);
+    expect(soroban.invoke).not.toHaveBeenCalled();
+  });
+
+  it('rejects releasePartial after a full release is recorded', async () => {
+    escrowRepo.findOne.mockResolvedValue({
+      id: 'escrow-full',
+      status: EscrowStatus.RELEASED,
+      amount: '100',
+      asset: AssetType.USDC,
+      bountyId: 'bounty-full',
+    });
+    paymentRepo.find.mockResolvedValue([{ amount: '100' }]);
+
+    await expect(
+      service.releasePartial('escrow-full', '1', 'GRECIPIENT'),
+    ).rejects.toThrow(BadRequestException);
+    expect(soroban.invoke).not.toHaveBeenCalled();
+  });
+
   describe('assertValidSplits / splitRelease', () => {
     it('throws when percentages do not sum to 100', () => {
       expect(() =>

src/escrow/escrow.service.ts

@@ -4,9 +4,15 @@ import {
   Logger,
   NotFoundException,
 } from '@nestjs/common';
+
+import { InjectRepository } from '@nestjs/typeorm';
+import { EntityManager, Repository } from 'typeorm';
+import { Escrow, Payment } from '../common/entities';
+
 import { InjectDataSource, InjectRepository } from '@nestjs/typeorm';
 import { DataSource, In, Repository } from 'typeorm';
 import { Escrow, Payment, User } from '../common/entities';
+
 import { AssetType, EscrowStatus, PaymentStatus } from '../common/enums';
 import {
   amountToStroops,
@@ -132,6 +138,30 @@ export class EscrowService {
     recipientAddress: string,
     recipientId?: string,
   ): Promise<Escrow> {
+
+    return this.withReleaseLock(
+      escrowId,
+      async (escrowRepo, paymentRepo, escrow) => {
+        this.assertNoExistingPayments(
+          escrow,
+          await paymentRepo.find({ where: { escrowId: escrow.id } }),
+        );
+
+        const result = await this.soroban.invoke('release', [
+          escrow.bountyId ??
+            escrow.milestoneId ??
+            escrow.maintenancePoolId ??
+            escrow.id,
+          recipientAddress,
+        ]);
+
+        escrow.status = EscrowStatus.RELEASED;
+        escrow.releaseTxHash = result.txHash;
+        escrow.releasedAt = new Date();
+        await escrowRepo.save(escrow);
+
+        const payment = paymentRepo.create({
+
     const escrow = await this.getOrThrow(escrowId);
     this.assertLocked(escrow);
     await this.assertRecipientsMatchUsers([{ recipientAddress, recipientId }]);
@@ -153,35 +183,86 @@ export class EscrowService {
       await manager.save(
         Payment,
         this.paymentRepo.create({
+
           escrowId: escrow.id,
           recipientId: recipientId ?? null,
           recipientAddress,
           amount: escrow.amount,
           asset: escrow.asset,
           status: PaymentStatus.CONFIRMED,
           txHash: result.txHash,
+
+        });
+        await paymentRepo.save(payment);
+        return escrow;
+      },
+    );
+
         }),
       );
     });
 
     return escrow;
+
   }
 
   /**
-   * Splits the escrowed amount across multiple recipients by percentage
-   * (team bounties). Percentages must sum to exactly 100.
-   *
-   * The recorded `Payment.amount` values are derived from the same
-   * basis-point integers sent on-chain — not recomputed independently from the
-   * raw percentages — so the local ledger can never drift from what was
-   * instructed to the contract. Shares are allocated in whole stroops via a
-   * largest-remainder method, guaranteeing `sum(payments.amount) ===
-   * escrow.amount` exactly (#43).
+   * Splits the escrowed amount across multiple recipients by percentage.
+   * A release is intentionally all-or-nothing: an escrow with any prior
+   * payment (including a partial release) cannot be released again.
    */
   async splitRelease(
     escrowId: string,
     recipients: SplitRecipient[],
   ): Promise<Payment[]> {
+
+    return this.withReleaseLock(
+      escrowId,
+      async (escrowRepo, paymentRepo, escrow) => {
+        this.assertNoExistingPayments(
+          escrow,
+          await paymentRepo.find({ where: { escrowId: escrow.id } }),
+        );
+        this.assertValidSplits(recipients);
+
+        const totalStroops = amountToStroops(escrow.amount);
+        const bps = apportionBasisPoints(recipients.map((r) => r.percentage));
+        const result = await this.soroban.invoke('split_release', [
+          escrow.bountyId ?? escrow.milestoneId ?? escrow.id,
+          recipients.map((r) => r.recipientAddress),
+          bps,
+        ]);
+
+        const shares = splitStroops(totalStroops, bps);
+        this.reconcileSplitResult(escrow.id, totalStroops, result.returnValue);
+        escrow.status = EscrowStatus.RELEASED;
+        escrow.releaseTxHash = result.txHash;
+        escrow.releasedAt = new Date();
+        escrow.metadata = { ...(escrow.metadata ?? {}), splitRelease: result };
+        await escrowRepo.save(escrow);
+
+        const payments: Payment[] = [];
+        for (let i = 0; i < recipients.length; i++) {
+          const recipient = recipients[i];
+          payments.push(
+            await paymentRepo.save(
+              paymentRepo.create({
+                escrowId: escrow.id,
+                recipientId: recipient.recipientId ?? null,
+                recipientAddress: recipient.recipientAddress,
+                amount: stroopsToAmount(shares[i]),
+                asset: escrow.asset,
+                splitPercentage: (bps[i] / 100).toFixed(2),
+                status: PaymentStatus.CONFIRMED,
+                txHash: result.txHash,
+              }),
+            ),
+          );
+        }
+        return payments;
+      },
+    );
+
     const escrow = await this.getOrThrow(escrowId);
     this.assertLocked(escrow);
     this.assertValidSplits(recipients);
@@ -232,21 +313,65 @@ export class EscrowService {
       }
     });
     return payments;
+
   }
 
-  /**
-   * Releases a portion of a LOCKED escrow to a single recipient without
-   * closing it out — used by milestone funding, where the total budget is
-   * distributed incrementally as individual issues resolve. The escrow
-   * moves to RELEASED once the cumulative released amount reaches the
-   * total locked amount.
-   */
+  /** Releases one leg while leaving the escrow LOCKED until fully distributed. */
   async releasePartial(
     escrowId: string,
     amount: string,
     recipientAddress: string,
     recipientId?: string,
   ): Promise<Payment> {
+
+    return this.withReleaseLock(
+      escrowId,
+      async (escrowRepo, paymentRepo, escrow) => {
+        this.assertValidAmount(amount);
+        // assertLocked deliberately remains before the history calculation: a
+        // full release marks RELEASED, so partial-after-full is rejected too.
+        const existingPayments = await paymentRepo.find({
+          where: { escrowId: escrow.id },
+        });
+        this.assertLocked(escrow);
+        const releasedSoFar = existingPayments.reduce(
+          (sum, p) => sum + Number(p.amount),
+          0,
+        );
+        const requested = Number(amount);
+        if (releasedSoFar + requested > Number(escrow.amount) + 1e-7) {
+          throw new BadRequestException(
+            `Partial release of ${amount} would exceed remaining escrow balance`,
+          );
+        }
+
+        const result = await this.soroban.invoke('release', [
+          escrow.milestoneId ?? escrow.bountyId ?? escrow.id,
+          recipientAddress,
+          this.toStroops(amount),
+        ]);
+        const payment = await paymentRepo.save(
+          paymentRepo.create({
+            escrowId: escrow.id,
+            recipientId: recipientId ?? null,
+            recipientAddress,
+            amount,
+            asset: escrow.asset,
+            status: PaymentStatus.CONFIRMED,
+            txHash: result.txHash,
+          }),
+        );
+
+        if (releasedSoFar + requested >= Number(escrow.amount) - 1e-7) {
+          escrow.status = EscrowStatus.RELEASED;
+          escrow.releaseTxHash = result.txHash;
+          escrow.releasedAt = new Date();
+          await escrowRepo.save(escrow);
+        }
+        return payment;
+      },
+    );
+
     const escrow = await this.getOrThrow(escrowId);
     this.assertLocked(escrow);
     this.assertValidAmount(amount);
@@ -376,6 +501,7 @@ export class EscrowService {
         txHash: result.txHash,
       }),
     );
+
   }
 
   /** Refunds the full escrowed amount back to the original funder. */
@@ -401,6 +527,49 @@ export class EscrowService {
     return this.getOrThrow(id);
   }
 
+  /**
+   * Serializes every release-family operation on the escrow row. The database
+   * lock is held through the Soroban call and ledger writes, preventing two
+   * concurrent requests from both passing the payment-history check.
+   * The fallback exists only for lightweight unit-test repository doubles;
+   * real TypeORM repositories always have a transaction manager.
+   */
+  private async withReleaseLock<T>(
+    escrowId: string,
+    operation: (
+      escrowRepo: Repository<Escrow>,
+      paymentRepo: Repository<Payment>,
+      escrow: Escrow,
+    ) => Promise<T>,
+  ): Promise<T> {
+    const manager = this.escrowRepo.manager;
+    if (!manager?.transaction) {
+      const escrow = await this.getOrThrow(escrowId);
+      this.assertLocked(escrow);
+      return operation(this.escrowRepo, this.paymentRepo, escrow);
+    }
+
+    return manager.transaction(async (transactionManager: EntityManager) => {
+      const escrowRepo = transactionManager.getRepository(Escrow);
+      const paymentRepo = transactionManager.getRepository(Payment);
+      const escrow = await escrowRepo.findOne({
+        where: { id: escrowId },
+        lock: { mode: 'pessimistic_write' },
+      });
+      if (!escrow) throw new NotFoundException(`Escrow ${escrowId} not found`);
+      this.assertLocked(escrow);
+      return operation(escrowRepo, paymentRepo, escrow);
+    });
+  }
+
+  private assertNoExistingPayments(escrow: Escrow, payments: Payment[]): void {
+    if (payments.length > 0) {
+      throw new BadRequestException(
+        `Escrow ${escrow.id} already has payment history and cannot be fully released`,
+      );
+    }
+  }
+
   private async getOrThrow(id: string): Promise<Escrow> {
     const escrow = await this.escrowRepo.findOne({ where: { id } });
     if (!escrow) throw new NotFoundException(`Escrow ${id} not found`);