From a8f621be209838866f4eaded783d443236859451 Mon Sep 17 00:00:00 2001 From: alagbesadat208-prog Date: Wed, 28 Oct 2026 13:59:06 +0000 Subject: [PATCH] fix(backend): align stroop units in loan, score, dispute and deposit paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requesting a loan compared a human-readable token amount against a pool balance in stroops, the score breakdown dropped every computed statistic for users without a scores row, contesting a default wrote an invalid event outside a transaction, and deposit slippage bounds skipped the 10^7 scaling. Compare in stroops, aggregate the score CTE, write the dispute and its event atomically with the NOT NULL columns filled, and scale minSharesOut through the money policy with schema coverage. Closes #2019 Closes #2020 Closes #2021 Closes #2022 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- .../integration/loanDisputeFlow.test.ts | 19 ++++++- backend/src/__tests__/loanDispute.test.ts | 26 ++++++++- backend/src/__tests__/loanEndpoints.test.ts | 49 +++++++++++++++++ backend/src/__tests__/scoreBreakdown.test.ts | 37 +++++++++++++ backend/src/__tests__/validation.test.ts | 27 ++++++++++ backend/src/controllers/loanController.ts | 54 +++++++++++++------ backend/src/controllers/scoreController.ts | 5 +- backend/src/schemas/poolSchemas.ts | 1 + .../__tests__/sorobanService.stroops.test.ts | 18 +++++++ backend/src/services/sorobanService.ts | 4 +- 10 files changed, 217 insertions(+), 23 deletions(-) diff --git a/backend/src/__tests__/integration/loanDisputeFlow.test.ts b/backend/src/__tests__/integration/loanDisputeFlow.test.ts index cc0bf3731..6530394e9 100644 --- a/backend/src/__tests__/integration/loanDisputeFlow.test.ts +++ b/backend/src/__tests__/integration/loanDisputeFlow.test.ts @@ -8,15 +8,20 @@ const mockQuery = jest.fn<(...args: unknown[]) => Promise>(); const mockNotifyAdmins = jest.fn<(...args: unknown[]) => Promise>(); const mockCreateNotification = jest.fn<(...args: unknown[]) => Promise>(); +// Run the transaction callback on the shared mock query, matching the shape a +// real `withTransaction` gives its callback. +const mockWithTransaction = jest.fn( + async (fn: (client: { query: typeof mockQuery }) => Promise) => fn({ query: mockQuery }), +); jest.unstable_mockModule('../../db/connection.ts', () => ({ query: mockQuery, default: { query: mockQuery, connect: jest.fn(), end: jest.fn() }, - withTransaction: jest.fn(), + withTransaction: mockWithTransaction, })); jest.unstable_mockModule('../../db/connection.js', () => ({ query: mockQuery, default: { query: mockQuery, connect: jest.fn(), end: jest.fn() }, - withTransaction: jest.fn(), + withTransaction: mockWithTransaction, })); jest.unstable_mockModule('../../db/transaction.js', () => ({ withTransaction: jest.fn(), @@ -176,6 +181,16 @@ describe('loan dispute resolution integration flow', () => { expect.stringContaining('INSERT INTO loan_disputes'), [String(LOAN_ID), TEST_PUBLIC_KEY, 'Indexer lag caused an incorrect default event.'], ], + [ + expect.stringContaining('LoanDisputed'), + [ + expect.any(String), + String(LOAN_ID), + TEST_PUBLIC_KEY, + `dispute:${DISPUTE_ID}`, + expect.any(String), + ], + ], [ expect.stringContaining("UPDATE loan_disputes SET status = 'resolved'"), [ diff --git a/backend/src/__tests__/loanDispute.test.ts b/backend/src/__tests__/loanDispute.test.ts index c0f858b05..7c76cd4da 100644 --- a/backend/src/__tests__/loanDispute.test.ts +++ b/backend/src/__tests__/loanDispute.test.ts @@ -8,10 +8,15 @@ import { jest } from '@jest/globals'; // ESM-compatible mocking const mockQuery = jest.fn<(...args: unknown[]) => Promise>(); +// Run the transaction callback on the shared mock query, so a test observes +// the same statement ordering a real transaction would produce. +const mockWithTransaction = jest.fn( + async (fn: (client: { query: typeof mockQuery }) => Promise) => fn({ query: mockQuery }), +); jest.unstable_mockModule('../db/connection.js', () => ({ query: mockQuery, default: { query: mockQuery, connect: jest.fn(), end: jest.fn() }, - withTransaction: jest.fn(), + withTransaction: mockWithTransaction, })); jest.unstable_mockModule('../db/transaction.js', () => ({ withTransaction: jest.fn(), @@ -119,6 +124,7 @@ afterAll(() => { beforeEach(() => { mockReverseDefault.mockReset(); + mockWithTransaction.mockClear(); }); // ─── Tests ──────────────────────────────────────────────────────────────────── @@ -147,7 +153,7 @@ describe('Loan Dispute/Appeal Mechanism', () => { * requireLoanBorrowerAccess: * [1] SELECT address FROM contract_events WHERE loan_id = 42 → TEST_PUBLIC_KEY ✓ * - * contestDefault: + * contestDefault (inside withTransaction): * [2] SELECT contract_events WHERE event_type='LoanDefaulted' → found (address = borrower) * [3] INSERT loan_disputes RETURNING id → disputeId * [4] INSERT contract_events LoanDisputed @@ -167,6 +173,22 @@ describe('Loan Dispute/Appeal Mechanism', () => { expect(res.body.success).toBe(true); disputeId = res.body.disputeId ?? disputeId; + + // Both inserts must share one transaction, and the event must satisfy the + // NOT NULL columns on contract_events (#140). + expect(mockWithTransaction).toHaveBeenCalledTimes(1); + const eventInsert = mockQuery.mock.calls.find( + (call) => typeof call[0] === 'string' && (call[0] as string).includes('LoanDisputed'), + ); + expect(eventInsert).toBeDefined(); + expect(eventInsert![0]).toContain('event_id'); + expect(eventInsert![1]).toEqual([ + expect.any(String), + String(LOAN_ID), + TEST_PUBLIC_KEY, + `dispute:${DISPUTE_ID}`, + process.env.LOAN_MANAGER_CONTRACT_ID || 'offchain_dispute', + ]); }); it('should reject a contest when the defaulted loan belongs to another borrower', async () => { diff --git a/backend/src/__tests__/loanEndpoints.test.ts b/backend/src/__tests__/loanEndpoints.test.ts index a75e96e1a..86de4f4c0 100644 --- a/backend/src/__tests__/loanEndpoints.test.ts +++ b/backend/src/__tests__/loanEndpoints.test.ts @@ -104,6 +104,8 @@ const mockSubmitSignedTx = jest.fn< (signedTxXdr: string) => Promise<{ txHash: string; status: string; resultXdr?: string }> >(); +// Raw stroops, as the real service returns them. +const mockGetPoolBalance = jest.fn<() => Promise>().mockResolvedValue(100_000_000_000); // 10,000 tokens await jest.unstable_mockModule('../services/sorobanService.js', () => ({ sorobanService: { buildRequestLoanTx: mockBuildRequestLoanTx, @@ -114,6 +116,7 @@ await jest.unstable_mockModule('../services/sorobanService.js', () => ({ buildExtendLoanTx: mockBuildExtendLoanTx, buildLiquidateTx: mockBuildLiquidateTx, submitSignedTx: mockSubmitSignedTx, + getPoolBalance: mockGetPoolBalance, }, })); @@ -262,6 +265,52 @@ describe('POST /api/loans/request', () => { .send({ borrowerPublicKey: TEST_BORROWER }); expect(response.status).toBe(400); }); + + describe('pool liquidity pre-flight check (#139)', () => { + const originalNodeEnv = process.env.NODE_ENV; + + beforeEach(() => { + // The pre-flight check is skipped under NODE_ENV=test. + process.env.NODE_ENV = 'development'; + mockGetPoolBalance.mockResolvedValue(100_000_000_000); + }); + + afterEach(() => { + process.env.NODE_ENV = originalNodeEnv; + mockGetPoolBalance.mockReset(); + mockGetPoolBalance.mockResolvedValue(100_000_000_000); + }); + + it('rejects a request larger than the pool balance measured in tokens', async () => { + // 1,000 stroops is 0.0001 tokens — far below the 1,000 token request. + mockGetPoolBalance.mockResolvedValueOnce(1_000); + + const response = await request(app) + .post('/api/loans/request') + .set(bearer(TEST_BORROWER)) + .send({ amount: 1000, borrowerPublicKey: TEST_BORROWER }); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('0.0001 tokens'); + expect(mockBuildRequestLoanTx).not.toHaveBeenCalled(); + }); + + it('accepts a request covered by the pool balance', async () => { + mockGetPoolBalance.mockResolvedValueOnce(10_000_000_000); // 1,000 tokens + mockBuildRequestLoanTx.mockResolvedValueOnce({ + unsignedTxXdr: 'AAAA...base64xdr', + networkPassphrase: 'Test SDF Network ; September 2015', + }); + + const response = await request(app) + .post('/api/loans/request') + .set(bearer(TEST_BORROWER)) + .send({ amount: 1000, borrowerPublicKey: TEST_BORROWER }); + + expect(response.status).toBe(200); + expect(mockGetPoolBalance).toHaveBeenCalledTimes(1); + }); + }); }); // --------------------------------------------------------------------------- diff --git a/backend/src/__tests__/scoreBreakdown.test.ts b/backend/src/__tests__/scoreBreakdown.test.ts index e19285f04..b7be3f5a6 100644 --- a/backend/src/__tests__/scoreBreakdown.test.ts +++ b/backend/src/__tests__/scoreBreakdown.test.ts @@ -118,6 +118,43 @@ describe('GET /api/score/:userId/breakdown', () => { expect(response.body.history).toHaveLength(3); }); + it('aggregates the current score so a user without a scores row still gets their event stats', async () => { + mockedQuery.mockResolvedValueOnce({ + rows: [ + { + current_score: 500, + total_loans: 2, + repaid_count: 1, + defaulted_count: 0, + total_repaid: 250, + on_time_count: 1, + late_count: 0, + avg_repayment_ledgers: 8640, + }, + ], + command: 'SELECT', + rowCount: 1, + oid: 0, + fields: [], + }); + mockedQuery.mockResolvedValueOnce({ + rows: [{ event_type: 'LoanRepaid', ledger_closed_at: '2026-03-01T10:00:00Z' }], + command: 'SELECT', + rowCount: 1, + oid: 0, + fields: [], + }); + + await request(app).get('/api/score/newuser/breakdown').set(bearer('newuser')); + + // Without an aggregate the CTE yields 0 rows and the CROSS JOINs below + // discard every computed statistic (#141). + const breakdownSql = String(mockedQuery.mock.calls[0][0]); + expect(breakdownSql).toMatch( + /current_score_cte AS \(\s*SELECT COALESCE\(MAX\(current_score\), 500\)/, + ); + }); + it('should return default values for a user with no history', async () => { // Mock empty breakdown and history queries mockedQuery diff --git a/backend/src/__tests__/validation.test.ts b/backend/src/__tests__/validation.test.ts index 94b0acee2..8807231c3 100644 --- a/backend/src/__tests__/validation.test.ts +++ b/backend/src/__tests__/validation.test.ts @@ -39,6 +39,7 @@ await jest.unstable_mockModule('../services/cacheService.js', () => ({ await import('../db/connection.js'); const { default: app } = await import('../app.js'); const { generateJwtToken } = await import('../services/authService.js'); +const { buildPoolTransactionSchema } = await import('../schemas/poolSchemas.js'); const TEST_WALLET = Keypair.random().publicKey(); const OTHER_WALLET = Keypair.random().publicKey(); @@ -189,4 +190,30 @@ describe('Input Validation', () => { expect(response.status).toBe(404); }); }); + + describe('buildPoolTransactionSchema', () => { + // The deposit route previously stripped minSharesOut before it reached the + // controller, so the slippage bound was never validated (#142). + const baseDeposit = { + depositorPublicKey: TEST_WALLET, + token: OTHER_WALLET, + amount: 100, + }; + + it('accepts a deposit carrying minSharesOut', () => { + expect( + buildPoolTransactionSchema.safeParse({ ...baseDeposit, minSharesOut: 95 }).success, + ).toBe(true); + }); + + it('allows minSharesOut to be omitted', () => { + expect(buildPoolTransactionSchema.safeParse(baseDeposit).success).toBe(true); + }); + + it('rejects a negative minSharesOut', () => { + expect( + buildPoolTransactionSchema.safeParse({ ...baseDeposit, minSharesOut: -1 }).success, + ).toBe(false); + }); + }); }); diff --git a/backend/src/controllers/loanController.ts b/backend/src/controllers/loanController.ts index ac87797b0..0fa16996d 100644 --- a/backend/src/controllers/loanController.ts +++ b/backend/src/controllers/loanController.ts @@ -1,5 +1,6 @@ +import { randomUUID } from 'node:crypto'; import type { Request, Response, NextFunction } from 'express'; -import { query } from '../db/connection.js'; +import { query, withTransaction } from '../db/connection.js'; import { withStellarAndDbTransaction } from '../db/transaction.js'; import { AppError } from '../errors/AppError.js'; import { asyncHandler } from '../utils/asyncHandler.js'; @@ -12,7 +13,7 @@ import logger from '../utils/logger.js'; import { cacheService } from '../services/cacheService.js'; import { notificationService } from '../services/notificationService.js'; import { invalidateOnRepay, invalidateOnLoanRequest } from '../utils/cacheKeys.js'; -import { roundToCents } from '../money/decimal.js'; +import { fromStroops, roundToCents, toStroops } from '../money/decimal.js'; import { parseStroopAmount, remainingPrincipal, @@ -189,18 +190,28 @@ export const contestDefault = asyncHandler( throw AppError.forbidden('You are not the borrower of this loan'); } - // Insert dispute record and return disputeId - const disputeResult = await query( - `INSERT INTO loan_disputes (loan_id, borrower, reason, status) VALUES ($1, $2, $3, 'open') RETURNING id`, - [loanId, borrower, reason], - ); + // The dispute row and its marker event must land together: writing the + // dispute first left an orphaned `loan_disputes` row behind when the event + // insert failed (#140). + // + // `contract_events` declares event_id, ledger, tx_hash and contract_id as + // NOT NULL, so this synthetic off-chain marker has to supply all of them + // (same shape as the admin dispute endpoints). + const contractId = process.env.LOAN_MANAGER_CONTRACT_ID || 'offchain_dispute'; + const disputeRow = await withTransaction(async (client) => { + const disputeResult = await client.query( + `INSERT INTO loan_disputes (loan_id, borrower, reason, status) VALUES ($1, $2, $3, 'open') RETURNING id`, + [loanId, borrower, reason], + ); + const disputeId = disputeResult.rows[0].id as number; - // Optionally: update loan status to 'disputed' in your loan status tracking (if applicable) - // If you have a loan status table/column, update it here. If only events, you may want to insert a new event: - await query( - `INSERT INTO contract_events (loan_id, address, event_type, amount, ledger, ledger_closed_at) VALUES ($1, $2, 'LoanDisputed', NULL, NULL, NOW())`, - [loanId, borrower], - ); + await client.query( + `INSERT INTO contract_events (event_id, loan_id, address, event_type, amount, ledger, ledger_closed_at, tx_hash, contract_id) VALUES ($1, $2, $3, 'LoanDisputed', NULL, 0, NOW(), $4, $5)`, + [randomUUID(), loanId, borrower, `dispute:${disputeId}`, contractId], + ); + + return { disputeId }; + }); logger.withContext().info('Loan default contested', { loanId, borrower, reason }); @@ -214,7 +225,7 @@ export const contestDefault = asyncHandler( res.json({ success: true, - disputeId: disputeResult.rows[0].id, + disputeId: disputeRow.disputeId, message: 'Loan default contested. Admins will review your dispute.', }); }, @@ -856,6 +867,9 @@ export const getLoanAmortizationSchedule = asyncHandler(async (req: Request, res }); }); +/** Format stroops as a token amount without trailing settlement zeros. */ +const formatTokenAmount = (stroops: bigint): string => fromStroops(stroops).replace(/\.?0+$/, ''); + /** * POST /api/loans/request */ @@ -881,12 +895,18 @@ export const requestLoan = asyncHandler(async (req: Request, res: Response) => { typeof (sorobanService as unknown as { getPoolBalance?: () => Promise }) .getPoolBalance === 'function' ) { - const poolBalance = await ( + const poolBalanceStroops = await ( sorobanService as unknown as { getPoolBalance: () => Promise } ).getPoolBalance(); - if (amount > poolBalance) { + // `amount` is a human-readable token amount (e.g. 1000 USDC) while + // `getPoolBalance()` returns raw stroops (10^7 per token), so the comparison + // has to happen in a single unit — stroops — instead of comparing the two + // numbers directly (#139). + const requestedStroops = toStroops(amount.toString()); + const availableStroops = BigInt(Math.floor(poolBalanceStroops)); + if (requestedStroops > availableStroops) { throw AppError.badRequest( - 'Insufficient pool liquidity to cover this loan', + `Insufficient pool liquidity to cover this loan: requested ${amount} tokens, pool holds ${formatTokenAmount(availableStroops)} tokens`, ErrorCode.INSUFFICIENT_BALANCE, ); } diff --git a/backend/src/controllers/scoreController.ts b/backend/src/controllers/scoreController.ts index 67f394652..dbbe4aac0 100644 --- a/backend/src/controllers/scoreController.ts +++ b/backend/src/controllers/scoreController.ts @@ -171,8 +171,11 @@ export const getScoreBreakdown = asyncHandler(async (req: Request, res: Response const breakdownResult = await query( `WITH -- Current score from scores table + -- MAX() keeps this CTE at exactly one row: a plain COALESCE(current_score, 500) + -- returned zero rows for users without a scores row yet, and the CROSS + -- JOINs below then discarded every computed loan/timing statistic (#141). current_score_cte AS ( - SELECT COALESCE(current_score, 500) AS current_score + SELECT COALESCE(MAX(current_score), 500) AS current_score FROM scores WHERE user_id = $1 ), diff --git a/backend/src/schemas/poolSchemas.ts b/backend/src/schemas/poolSchemas.ts index 68562d9f8..5bd13fd28 100644 --- a/backend/src/schemas/poolSchemas.ts +++ b/backend/src/schemas/poolSchemas.ts @@ -6,6 +6,7 @@ export const buildPoolTransactionSchema = z.object({ depositorPublicKey: stellarAddressSchema, token: stellarAddressSchema, amount: positiveAmountSchema, + minSharesOut: z.number().nonnegative().optional(), minAssetsOut: z.number().int().nonnegative().optional(), }); diff --git a/backend/src/services/__tests__/sorobanService.stroops.test.ts b/backend/src/services/__tests__/sorobanService.stroops.test.ts index a7ff7ab89..eb0da4395 100644 --- a/backend/src/services/__tests__/sorobanService.stroops.test.ts +++ b/backend/src/services/__tests__/sorobanService.stroops.test.ts @@ -89,6 +89,24 @@ describe('SorobanService token amounts are scaled to stroops (10^7)', () => { expect(functionName).toBe('deposit'); expect(scValToNative(args[2])).toBe(125_000_000n); + expect(scValToNative(args[3])).toBe(0n); + }); + + it('buildDepositTx scales minSharesOut to stroops', async () => { + const { functionName, args } = await invokeArgsFor(() => + sorobanService.buildDepositTx(userAddress, tokenAddress, 12.5, 95), + ); + + expect(functionName).toBe('deposit'); + expect(scValToNative(args[3])).toBe(950_000_000n); + }); + + it('buildDepositTx accepts a fractional minSharesOut instead of throwing', async () => { + const { args } = await invokeArgsFor(() => + sorobanService.buildDepositTx(userAddress, tokenAddress, 12.5, 95.5), + ); + + expect(scValToNative(args[3])).toBe(955_000_000n); }); it('buildRefinanceLoanTx scales the new amount to stroops', async () => { diff --git a/backend/src/services/sorobanService.ts b/backend/src/services/sorobanService.ts index f3b0b0e3d..3c203cb30 100644 --- a/backend/src/services/sorobanService.ts +++ b/backend/src/services/sorobanService.ts @@ -311,7 +311,9 @@ class SorobanService { type: 'address', }); const amountScVal = nativeToScVal(toStroops(amount.toString()), { type: 'i128' }); - const minSharesOutScVal = nativeToScVal(BigInt(minSharesOut), { type: 'i128' }); + // Shares are denominated in 7-decimal stroop units on-chain, exactly like + // the deposited amount, so the slippage bound has to be scaled too (#142). + const minSharesOutScVal = nativeToScVal(toStroops(minSharesOut.toString()), { type: 'i128' }); const tx = new TransactionBuilder(account, { fee: BASE_FEE,