Skip to content

fix(maintenance-pool): drop broken pessimistic lock in deposit (#457) #470

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
Proxima84-code wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(maintenance-pool): drop broken pessimistic lock in deposit (#457) #470

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
10 changes: 2 additions & 8 deletions src/escrow/escrow.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,20 +584,14 @@ export class EscrowService {
recipients: Array<[string, number]>,
manager?: EntityManager,
): Promise<ContractInvocationResult> {
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,
Expand Down
62 changes: 38 additions & 24 deletions src/maintenance-pool/maintenance-pool.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,20 @@ describe('MaintenancePoolService', () => {
});
});

describe('deposit', () => {
describe("deposit", () => {
beforeEach(() => {
// Default mock for deposit's createQueryBuilder (SELECT ... FOR UPDATE)
const mockDepositQueryBuilder = {
setLock: 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);
poolRepo.findOne.mockResolvedValue({
id: "pool-1",
status: MaintenancePoolStatus.ACTIVE,
balance: "100",
monthlyDeposit: "500",
asset: AssetType.USDC,
escrowId: "escrow-1",
});
});

it('rejects when the pool is not ACTIVE', async () => {
poolRepo.createQueryBuilder().getOne.mockResolvedValue({
poolRepo.findOne.mockResolvedValue({
id: 'pool-1',
status: MaintenancePoolStatus.PAUSED,
balance: '0',
Expand All @@ -148,7 +147,7 @@ describe('MaintenancePoolService', () => {
asset: AssetType.USDC,
escrowId: null as string | null,
};
poolRepo.createQueryBuilder().getOne.mockResolvedValue(row);
poolRepo.findOne.mockResolvedValue(row);
poolRepo.update.mockImplementation(
(_id: string, partial: Partial<typeof row>) => {
Object.assign(row, partial);
Expand Down Expand Up @@ -186,7 +185,7 @@ describe('MaintenancePoolService', () => {
// 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({
poolRepo.findOne.mockResolvedValue({
id: 'pool-1',
status: MaintenancePoolStatus.ACTIVE,
balance: '100',
Expand Down Expand Up @@ -221,7 +220,7 @@ describe('MaintenancePoolService', () => {
asset: AssetType.USDC,
escrowId: 'escrow-1',
};
poolRepo.createQueryBuilder().getOne.mockResolvedValue(row);
poolRepo.findOne.mockResolvedValue(row);
poolRepo.increment.mockImplementation(
(_where: { id: string }, column: 'balance', value: number) => {
row[column] = (Number(row[column]) + value).toFixed(7);
Expand Down Expand Up @@ -252,7 +251,7 @@ describe('MaintenancePoolService', () => {
// 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({
poolRepo.findOne.mockResolvedValue({
id: 'pool-1',
status: MaintenancePoolStatus.ACTIVE,
balance: '100',
Expand Down Expand Up @@ -285,6 +284,18 @@ describe('MaintenancePoolService', () => {
// unreachable through this service.
expect(pool.escrowId).toBe('escrow-1');
});
it("does not acquire a pessimistic lock outside a transaction (#457)", async () => {
poolRepo.findOne.mockResolvedValue({
id: "pool-1",
status: MaintenancePoolStatus.ACTIVE,
balance: "100",
asset: AssetType.USDC,
escrowId: "escrow-1",
});
await service.deposit("pool-1", "50", "GFUNDER");
expect(poolRepo.createQueryBuilder).not.toHaveBeenCalled();
});

});

describe('assignReward', () => {
Expand Down Expand Up @@ -336,9 +347,10 @@ describe('MaintenancePoolService', () => {

it('releases the reward and atomically decrements the balance', async () => {
poolRepo.findOne.mockResolvedValue({
id: 'pool-1',
balance: '100',
escrowId: 'escrow-1',
id: "pool-1",
status: MaintenancePoolStatus.ACTIVE,
balance: "100",
escrowId: "escrow-1",
});
// Mock the atomic balance check to succeed
const mockQueryBuilder = {
Expand Down Expand Up @@ -372,9 +384,10 @@ describe('MaintenancePoolService', () => {

it('rejects a reward for a non-maintenance issue before releasing funds', async () => {
poolRepo.findOne.mockResolvedValue({
id: 'pool-1',
balance: '100',
escrowId: 'escrow-1',
id: "pool-1",
status: MaintenancePoolStatus.ACTIVE,
balance: "100",
escrowId: "escrow-1",
});
issueRepo.findOne.mockResolvedValue({
id: 'issue-1',
Expand Down Expand Up @@ -409,9 +422,10 @@ describe('MaintenancePoolService', () => {

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',
id: "pool-1",
status: MaintenancePoolStatus.ACTIVE,
balance: "100",
escrowId: "escrow-1",
});
// Mock the payment query to return an existing payment
const mockPaymentQueryBuilder = {
Expand Down
10 changes: 3 additions & 7 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 @@ -57,13 +57,9 @@ 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();
// Drop pessimistic lock outside transaction (#457). First-time deposit
// race is handled atomically below via conditional CAS update ({ escrowId: null }).
const pool = await this.findOne(id);

if (!pool) throw new NotFoundException(`Maintenance pool ${id} not found`);
if (pool.status !== MaintenancePoolStatus.ACTIVE) {
Expand Down