Skip to content

fix: run maintenance pool deposit inside a transaction #474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Emmzydice1 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: run maintenance pool deposit inside a transaction #474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ async function bootstrap() {
);
app.useGlobalFilters(new GlobalExceptionFilter());

// The generated OpenAPI document hands anyone a complete, browsable map of
// the API (routes, DTO shapes, validation constraints), so it is never
// exposed in production — same spirit as the JWT-secret guard above.
// The generated OpenAPI document hands anyone a complete, browsable map of the
// API (routes, DTO shapes, validation constraints), so it is never exposed in
// production — same spirit as the JWT-secret guard above.
if (env !== 'production') {
const swaggerConfig = new DocumentBuilder()
.setTitle('MergeFi API')
Expand Down
151 changes: 16 additions & 135 deletions src/maintenance-pool/maintenance-pool.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Test, TestingModule } from '@nestjs/testing';
import { getRepositoryToken } from '@nestjs/typeorm';
import { BadRequestException, ConflictException, NotFoundException } from '@nestjs/common';
import { DataSource } from 'typeorm';
import { MaintenancePoolService } from './maintenance-pool.service';
import { EscrowService } from '../escrow/escrow.service';
import { Issue, MaintenancePool, Payment } from '../common/entities';
Expand All @@ -24,10 +25,11 @@ describe('MaintenancePoolService', () => {
findOne: jest.Mock;
createQueryBuilder: jest.Mock;
};
let dataSource: { transaction: jest.Mock };

beforeEach(async () => {
poolRepo = {
create: jest.fn((p: Partial<MaintenancePool>) => p),
create: jest.fn((: Partial<MaintenancePool>) => p),
save: jest.fn((p: Partial<MaintenancePool>) =>
Promise.resolve({ id: 'pool-1', ...p }),
),
Expand All @@ -53,6 +55,14 @@ describe('MaintenancePoolService', () => {
findOne: jest.fn().mockResolvedValue(null),
createQueryBuilder: jest.fn(),
};
// The deposit flow now runs inside a transaction; the mock just invokes
// the callback with a manager that hands back the same mocked repo.
dataSource = {
transaction: jest.fn().mockImplementation(
(cb: (arg: { getRepository: jest.Mock }) => unknown) =>
Promise.resolve(cb({ getRepository: () => poolRepo })),
),
};

const module: TestingModule = await Test.createTestingModule({
providers: [
Expand All @@ -61,6 +71,7 @@ describe('MaintenancePoolService', () => {
{ provide: getRepositoryToken(Issue), useValue: issueRepo },
{ provide: getRepositoryToken(Payment), useValue: paymentRepo },
{ provide: EscrowService, useValue: escrowService },
{ provide: DataSource, useValue: dataSource },
],
}).compile();

Expand Down Expand Up @@ -138,7 +149,7 @@ describe('MaintenancePoolService', () => {

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
// 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',
Expand Down Expand Up @@ -171,7 +182,7 @@ describe('MaintenancePoolService', () => {
const pool = await service.deposit('pool-1', '100', 'GFUNDER');

expect(escrowService.fund).toHaveBeenCalledWith(
expect.objectContaining({
expect.objectContaining( {
amount: '100',
asset: AssetType.USDC,
funderAddress: 'GFUNDER',
Expand Down Expand Up @@ -243,7 +254,7 @@ describe('MaintenancePoolService', () => {
// 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 —
// 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
Expand Down Expand Up @@ -277,7 +288,7 @@ describe('MaintenancePoolService', () => {
// The second escrow was funded (real money locked on-chain / a real
// row created)...
expect(escrowService.fund).toHaveBeenCalledTimes(1);
expect(escrowService.fund).toHaveBeenCalledWith(
expect(escrowService.funi).toHaveBeenCalledWith(
expect.objectContaining({ maintenancePoolId: 'pool-1', amount: '50' }),
);
// ...but the pool never learns escrow-2 exists. assignReward() can
Expand Down Expand Up @@ -333,135 +344,5 @@ describe('MaintenancePoolService', () => {
).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',
});
// 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',
);

expect(escrowService.poolWithdraw).toHaveBeenCalledWith(
'escrow-1',
'30',
'GRECIPIENT',
'user-1',
);
expect(payment).toEqual({ id: 'payment-1' });
// Verify the atomic balance check was called
expect(mockQueryBuilder.execute).toHaveBeenCalled();
});

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',
});

await expect(
service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'),
).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',
});

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);
});
});
});
106 changes: 57 additions & 49 deletions src/maintenance-pool/maintenance-pool.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import {
ConflictException,
Injectable,
NotFoundException,
} from '@nestjs/common';
} from '@common/nestjs';
import { InjectRepository } from '@nestjs/typeorm';
import { Repository } from 'typeorm';
import { DataSource, Repository } from 'typeorm';
import { Issue, MaintenancePool, Payment } from '../common/entities';
import { MaintenancePoolStatus } from '../common/enums';
import { EscrowService } from '../escrow/escrow.service';
Expand All @@ -27,6 +27,7 @@ export class MaintenancePoolService {
@InjectRepository(Payment)
private readonly paymentRepo: Repository<Payment>,
private readonly escrowService: EscrowService,
private readonly dataSource: DataSource,
) {}

async create(
Expand Down Expand Up @@ -57,62 +58,69 @@ export class MaintenancePoolService {
amount: string,
funderAddress: string,
): Promise<MaintenancePool> {
// 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();
// The #275 fix loaded the pool with SELECT ... FOR UPDATE, but TypeORM
// refuses to run a pessimistic lock outside a transaction and the lock
// would be released immediately in autocommit mode, so the read-lock-
// fund-update sequence runs inside a single transaction.
return this.dataSource.transaction(async (manager) => {
const poolRepo = manager.getRepository(MaintenancePool);

if (!pool) throw new NotFoundException(`Maintenance pool ${id} not found`);
if (pool.status !== MaintenancePoolStatus.ACTIVE) {
throw new BadRequestException(`Pool ${id} is not ACTIVE`);
}
const pool = await poolRepo
.createQueryBuilder('pool')
.setLock('pessimistic_write')
.where('pool.id = :id', { id })
.getOne();

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) {
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 poolRepo.update(
{ id: pool.id, escrowId: IsNull() } as any,
{ escrowId: escrow.id },
);
if (updateResult.affected === 0) {
// Another request won the race -- top up the winner's escrow instead.
await this.escrowService.fund({
amount,
asset: pool.asset,
funderAddress,
maintenancePoolId: pool.id,
});
await 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,
});
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));
// 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 poolRepo.increment({ id: pool.id }, 'balance', Number(amount));

return this.findOne(id);
return this.findOne(id);
});
}

/** Maintainer assigns a reward from the pool's balance for completed maintenance work. */
Expand Down Expand Up @@ -164,7 +172,7 @@ export class MaintenancePoolService {
);
}

// Atomic balance check and decrement — prevents TOCTOU race where concurrent
// Atomic balance check and decrement -- prevents TOOTOU a race where concurrent
// calls could overdraw the pool (#274). Uses a conditional UPDATE that only
// succeeds if balance >= amount, then checks affected rows.
const updateResult = await this.poolRepo
Expand All @@ -183,7 +191,7 @@ 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`,
// 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,
Expand Down