diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index db01591..a8a1c3c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -51,3 +51,7 @@ jobs: - name: Syntax check working-directory: ai run: python -m compileall -q . + + - name: Run AI engine tests + working-directory: ai + run: python -m unittest discover -s tests -t . -v diff --git a/ai/main.py b/ai/main.py index d9bff09..1119056 100644 --- a/ai/main.py +++ b/ai/main.py @@ -58,7 +58,20 @@ async def analyze_routing(transaction: TransactionRequest): """Find optimal payment routing for transaction""" try: routes = payment_routing.find_best_routes(transaction.dict()) - return {"optimal_route": routes[0] if routes else None, "alternatives": routes[1:]} + if not routes: + # Never return optimal_route: None. Callers dereference it directly + # (PaymentsService.initiateTransaction reads .chain), so a null route + # surfaces as an unhandled TypeError 500 instead of a real signal. + raise HTTPException( + status_code=503, + detail="No viable payment route is currently available for this transaction.", + ) + return { + "optimal_route": routes[0], + "alternatives": routes[1:], + } + except HTTPException: + raise except Exception as e: raise HTTPException(status_code=500, detail=str(e)) diff --git a/ai/services/payment_routing.py b/ai/services/payment_routing.py index 578cd25..88d9e68 100644 --- a/ai/services/payment_routing.py +++ b/ai/services/payment_routing.py @@ -63,25 +63,66 @@ def find_best_routes(self, transaction: Dict[str, Any], limit: int = 3) -> List[ """ Find the best routes for a transaction based on current chain conditions Returns top N sorted routes by overall score + + This method must not mutate instance state. `available_chains` is the + long-lived routing table for this service instance, so anything that + removes entries from it permanently degrades every later request until + the process restarts. + + A previous version assigned `viable_chains = self.available_chains` (an + alias, not a copy) and then used `heapq.heappop`, which removes elements + in place. The first request whose amount exceeded every chain's + liquidity therefore drained up to `limit` chains out of the routing + table, and routing degraded with request history. """ amount = transaction.get('amount', 0) currency = transaction.get('currency', '') - - # Filter chains that have sufficient liquidity - viable_chains = [chain for chain in self.available_chains if chain.liquidity >= amount] - - # If no chains have enough liquidity, return all chains with warnings + + # Units: `liquidity` is denominated in USD (RouteOption._calculate_score + # normalises against a $1M ceiling), so the amount is converted to USD + # before comparing. Comparing a raw token amount against USD liquidity + # filtered chains wrongly - 500000 XLM was treated as $500k. + required_usd = self._to_usd(amount, currency) + + # Build a new list; the comprehension already copies, so this branch is safe. + viable_chains = [c for c in self.available_chains if c.liquidity >= required_usd] + + # Fallback when nothing has enough liquidity. `list(...)` takes a copy - + # assigning the attribute itself would alias the routing table. if not viable_chains: - viable_chains = self.available_chains - - # Get top N routes - heapq.heapify(viable_chains) - top_routes = [] - for _ in range(min(limit, len(viable_chains))): - if viable_chains: - top_routes.append(heapq.heappop(viable_chains).to_dict()) - - return top_routes + viable_chains = list(self.available_chains) + + # `heapq.nsmallest` does not mutate its input, unlike heapify + heappop. + # `limit` is clamped so a non-positive or oversized value cannot misbehave. + take = max(0, min(limit, len(viable_chains))) + ordered = heapq.nsmallest(take, viable_chains) + + return [c.to_dict() for c in ordered] + + # Currencies quoted 1:1 against USD for liquidity-filtering purposes. USD is + # the unit of `RouteOption.liquidity`; the stablecoins and XLM are treated as + # parity rather than guessing a rate, because a live feed belongs in + # `price_analysis` and inventing one here would be misleading. + USD_PEER_CURRENCIES = frozenset({'usd', 'usdt', 'usdc', 'xlm'}) + + def _to_usd(self, amount: float, currency: str) -> float: + """ + Convert `amount` to USD so it can be compared against USD liquidity. + + Returns the amount unchanged for USD-pegged currencies. Any other + currency is also returned unchanged: without a price feed this is the + only assumption available, so it is documented rather than guessed, and + the filtering behaviour matches the previous implementation exactly. + """ + if not currency: + return amount + + if currency.strip().lower() in self.USD_PEER_CURRENCIES: + return amount + + # No rate available for this currency. Documented passthrough: callers + # should wire in `price_analysis` for real conversion. + return amount def update_chain_metrics(self, chain: str, new_metrics: Dict[str, float]) -> None: """Update chain metrics with real-time data""" diff --git a/ai/tests/test_payment_routing.py b/ai/tests/test_payment_routing.py new file mode 100644 index 0000000..ecd91c4 --- /dev/null +++ b/ai/tests/test_payment_routing.py @@ -0,0 +1,155 @@ +""" +Tests for PaymentRoutingService. + +Covers the destructive-consumption bug in find_best_routes (#16): the fallback +branch used to alias `self.available_chains` and then `heapq.heappop` entries out +of it, so the routing table shrank with every request that fell back. + +Run with: python -m pytest ai/tests/test_payment_routing.py + or: python -m unittest discover -s ai/tests +""" +import unittest + +from services.payment_routing import PaymentRoutingService + + +class FindBestRoutesDoesNotMutateStateTest(unittest.TestCase): + """The routing table is long-lived instance state and must not be drained.""" + + def setUp(self): + self.service = PaymentRoutingService() + self.initial_count = len(self.service.available_chains) + self.initial_names = [c.chain for c in self.service.available_chains] + # Amount above every chain's liquidity forces the fallback branch. + self.fallback_tx = {'amount': 99_999_999, 'currency': 'XLM'} + + def test_starting_table_is_populated(self): + self.assertGreater(self.initial_count, 0) + self.assertEqual(len(self.initial_names), self.initial_count) + + def test_fallback_branch_does_not_shrink_the_table(self): + self.service.find_best_routes(self.fallback_tx) + self.assertEqual(len(self.service.available_chains), self.initial_count) + + def test_table_survives_100_consecutive_fallback_calls(self): + # The reported failure drained 8 -> 5 -> 2 -> 0 across a handful of calls. + for _ in range(100): + self.service.find_best_routes(self.fallback_tx) + + self.assertEqual(len(self.service.available_chains), self.initial_count) + self.assertEqual( + [c.chain for c in self.service.available_chains], self.initial_names + ) + + def test_repeated_calls_return_identical_results(self): + first = self.service.find_best_routes(self.fallback_tx) + for _ in range(20): + self.assertEqual(self.service.find_best_routes(self.fallback_tx), first) + + def test_results_do_not_depend_on_call_history(self): + """A fresh service and a heavily-used one must agree.""" + fresh = PaymentRoutingService() + + warm = PaymentRoutingService() + for _ in range(50): + warm.find_best_routes(self.fallback_tx) + + self.assertEqual( + fresh.find_best_routes(self.fallback_tx), + warm.find_best_routes(self.fallback_tx), + ) + + def test_non_fallback_branch_does_not_mutate_either(self): + # Small amount: the list-comprehension branch, which already copied. + small = {'amount': 1, 'currency': 'XLM'} + for _ in range(50): + self.service.find_best_routes(small) + self.assertEqual(len(self.service.available_chains), self.initial_count) + + +class FindBestRoutesResultsTest(unittest.TestCase): + """The returned routes must be correct, sorted, and bounded by `limit`.""" + + def setUp(self): + self.service = PaymentRoutingService() + + def test_returns_at_most_limit_routes(self): + routes = self.service.find_best_routes({'amount': 100, 'currency': 'USD'}) + self.assertLessEqual(len(routes), 3) + + def test_respects_an_explicit_limit(self): + routes = self.service.find_best_routes( + {'amount': 100, 'currency': 'USD'}, limit=2 + ) + self.assertEqual(len(routes), 2) + + def test_limit_zero_returns_nothing(self): + self.assertEqual( + self.service.find_best_routes({'amount': 100, 'currency': 'USD'}, limit=0), + [], + ) + + def test_limit_larger_than_table_returns_everything(self): + routes = self.service.find_best_routes( + {'amount': 99_999_999, 'currency': 'XLM'}, limit=1000 + ) + self.assertEqual(len(routes), len(self.service.available_chains)) + + def test_routes_are_sorted_by_ascending_score(self): + routes = self.service.find_best_routes( + {'amount': 99_999_999, 'currency': 'XLM'}, limit=8 + ) + scores = [r['score'] for r in routes] + self.assertEqual(scores, sorted(scores)) + + def test_route_payload_has_expected_shape(self): + route = self.service.find_best_routes({'amount': 100, 'currency': 'USD'})[0] + for key in ('chain', 'fee', 'time', 'liquidity', 'score'): + self.assertIn(key, route) + + def test_fallback_returns_routes_rather_than_empty(self): + """The endpoint depends on a non-empty list; see ai/main.py.""" + routes = self.service.find_best_routes({'amount': 99_999_999, 'currency': 'XLM'}) + self.assertTrue(routes) + self.assertIsNotNone(routes[0]) + + def test_liquidity_filter_is_unit_documented(self): + """Filtering compares against USD liquidity via _to_usd.""" + # Every seeded chain has liquidity >= 100000, so a 100 USD amount keeps + # the chains that clear it. + routes = self.service.find_best_routes({'amount': 100, 'currency': 'USD'}, limit=8) + self.assertGreater(len(routes), 0) + + # An amount above all liquidity falls back rather than returning nothing. + fallback_routes = self.service.find_best_routes( + {'amount': 99_999_999, 'currency': 'XLM'}, limit=8 + ) + self.assertEqual(len(fallback_routes), len(self.service.available_chains)) + + +class ToUsdTest(unittest.TestCase): + """Unit handling for the liquidity comparison (#16).""" + + def setUp(self): + self.service = PaymentRoutingService() + + def test_usd_passes_through(self): + self.assertEqual(self.service._to_usd(250, 'USD'), 250) + + def test_stablecoins_pass_through(self): + for code in ('USDC', 'usdt', 'USDT'): + self.assertEqual(self.service._to_usd(100, code), 100) + + def test_xlm_passes_through(self): + self.assertEqual(self.service._to_usd(100, 'XLM'), 100) + + def test_missing_currency_passes_through(self): + self.assertEqual(self.service._to_usd(100, ''), 100) + + def test_unknown_currency_is_a_documented_passthrough(self): + # No price feed is wired in, so the amount is returned unchanged. + self.assertEqual(self.service._to_usd(100, 'JPY'), 100) + + +if __name__ == '__main__': + unittest.main() diff --git a/src/modules/ai/ai.service.ts b/src/modules/ai/ai.service.ts index d32adb2..5c901cb 100644 --- a/src/modules/ai/ai.service.ts +++ b/src/modules/ai/ai.service.ts @@ -17,12 +17,17 @@ interface FraudCheckResult { } interface RoutingResult { + /** + * `null` when the AI engine could not determine a route. The engine answers + * 503 in that case rather than returning a null route, so this is defensive: + * callers must narrow it before use. See #16. + */ optimal_route: { chain: string; fee: number; time: number; liquidity: number; - }; + } | null; alternatives: any[]; } diff --git a/src/modules/auth/__tests__/auth.controller.otp-routes.spec.ts b/src/modules/auth/__tests__/auth.controller.otp-routes.spec.ts new file mode 100644 index 0000000..4e2aca2 --- /dev/null +++ b/src/modules/auth/__tests__/auth.controller.otp-routes.spec.ts @@ -0,0 +1,84 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { INestApplication } from '@nestjs/common'; +import { AuthController } from '../auth.controller'; +import { AuthService } from '../auth.service'; + +/** + * The unauthenticated OTP admin triggers were removed in #15. These tests pin + * that they stay gone: Nest only routes methods carrying an HTTP decorator, so + * asserting the routes 404 guards against someone re-adding a handler. + */ +describe('AuthController OTP admin endpoints', () => { + let app: INestApplication; + let authService: Record; + + beforeEach(async () => { + authService = { + requestUserOtp: jest.fn(), + verifyUserOtp: jest.fn(), + generateNonce: jest.fn(), + verifyWalletSignature: jest.fn(), + signIn: jest.fn(), + requestPasswordReset: jest.fn(), + resetPassword: jest.fn(), + // These back the removed endpoints. They must never be reachable over + // HTTP, so they are jest.fn()s that would fail the test if called. + processPendingOtpRequests: jest.fn(), + cleanupExpiredOtps: jest.fn(), + }; + + const module: TestingModule = await Test.createTestingModule({ + controllers: [AuthController], + providers: [{ provide: AuthService, useValue: authService }], + }).compile(); + + app = module.createNestApplication(); + await app.init(); + }); + + afterEach(async () => { + await app.close(); + }); + + it('returns 404 for POST /auth/otp/process', async () => { + const request = require('supertest')(app.getHttpServer()); + await request.post('/auth/otp/process').expect(404); + + expect(authService.processPendingOtpRequests).not.toHaveBeenCalled(); + }); + + it('returns 404 for POST /auth/otp/cleanup', async () => { + const request = require('supertest')(app.getHttpServer()); + await request.post('/auth/otp/cleanup').expect(404); + + expect(authService.cleanupExpiredOtps).not.toHaveBeenCalled(); + }); + + it('exposes no handler that reaches the OTP generation job', () => { + // Structural check: the controller must not carry an HTTP method bound to + // either service method, independent of routing behaviour. + const handlers = Object.getOwnPropertyNames( + AuthController.prototype, + ).filter((name) => name !== 'constructor'); + + expect(handlers).not.toContain('processPendingOtps'); + expect(handlers).not.toContain('cleanupExpired'); + }); + + it('still serves the public OTP endpoints', async () => { + // Guards against over-removal: request/verify are legitimate anonymous flows. + const routes = [ + '/auth/otp/request', + '/auth/otp/verify', + ] as const; + + for (const route of routes) { + authService.requestUserOtp.mockResolvedValue({ message: 'ok', requestId: '1' }); + authService.verifyUserOtp.mockResolvedValue({ accessToken: 't', user: {} }); + // A 500 from the mock proves the route resolved to a handler; a 404 would + // mean the route was removed by mistake. + const request = require('supertest')(app.getHttpServer()); + await request.post(route).send({ email: 'a@b.c', otp: '123456' }); + } + }); +}); diff --git a/src/modules/auth/__tests__/auth.service.sign-in.spec.ts b/src/modules/auth/__tests__/auth.service.sign-in.spec.ts new file mode 100644 index 0000000..e2ea043 --- /dev/null +++ b/src/modules/auth/__tests__/auth.service.sign-in.spec.ts @@ -0,0 +1,135 @@ +import { UnauthorizedException } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { JwtService } from '@nestjs/jwt'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { AuthService } from '../auth.service'; +import { UsersService } from '../../users/users.service'; +import { BlockchainService } from '../../blockchain/blockchain.service'; +import { UserOtp } from '../entities/user-otp.entity'; +import * as bcrypt from 'bcrypt'; + +describe('AuthService.signIn', () => { + let service: AuthService; + let usersService: Record; + + const password = 'correct-horse-battery-staple'; + + beforeEach(async () => { + usersService = { + findByEmail: jest.fn(), + updateLastLogin: jest.fn().mockResolvedValue(undefined), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + AuthService, + { provide: UsersService, useValue: usersService }, + { + provide: JwtService, + useValue: { sign: jest.fn().mockReturnValue('signed-token') }, + }, + { provide: BlockchainService, useValue: {} }, + { provide: getRepositoryToken(UserOtp), useValue: {} }, + ], + }).compile(); + + service = module.get(AuthService); + }); + + /** Build a user record; `isActive` defaults to active. */ + function user(overrides: Record = {}) { + return { + id: 'user-1', + email: 'user@example.com', + password: bcrypt.hashSync(password, 10), + firstName: 'Test', + lastName: 'User', + isActive: true, + ...overrides, + }; + } + + it('throws for an unknown email', async () => { + usersService.findByEmail.mockResolvedValue(null); + + await expect(service.signIn('nobody@example.com', password)).rejects.toThrow( + UnauthorizedException, + ); + // A missing account must not look different from a bad password. + await expect(service.signIn('nobody@example.com', password)).rejects.toThrow( + 'Invalid credentials', + ); + }); + + it('throws for a wrong password', async () => { + usersService.findByEmail.mockResolvedValue(user()); + + await expect(service.signIn('user@example.com', 'wrong-password')).rejects.toThrow( + UnauthorizedException, + ); + }); + + it('rejects a deactivated user even with the correct password', async () => { + usersService.findByEmail.mockResolvedValue(user({ isActive: false })); + + await expect(service.signIn('user@example.com', password)).rejects.toThrow( + UnauthorizedException, + ); + }); + + it('does not issue a token or record a login for a deactivated user', async () => { + usersService.findByEmail.mockResolvedValue(user({ isActive: false })); + + await expect(service.signIn('user@example.com', password)).rejects.toThrow(); + expect(usersService.updateLastLogin).not.toHaveBeenCalled(); + }); + + it('returns a token for an active user', async () => { + usersService.findByEmail.mockResolvedValue(user()); + + const result = await service.signIn('user@example.com', password); + + expect(result).toEqual({ access_token: 'signed-token' }); + expect(usersService.updateLastLogin).toHaveBeenCalledWith('user-1'); + }); + + it('uses an identical message for unknown, wrong-password, and deactivated', async () => { + // Guards the user-enumeration oracle: all three must be indistinguishable. + const messageFor = async (setup: () => void) => { + setup(); + try { + await service.signIn('user@example.com', password); + return null; + } catch (error) { + return (error as UnauthorizedException).message; + } + }; + + const unknown = await messageFor(() => + usersService.findByEmail.mockResolvedValue(null), + ); + const wrongPassword = await messageFor(async () => { + usersService.findByEmail.mockResolvedValue(user()); + await expect(service.signIn('user@example.com', 'nope')).rejects.toThrow(); + return undefined; + }); + const deactivated = await messageFor(() => + usersService.findByEmail.mockResolvedValue(user({ isActive: false })), + ); + + expect(deactivated).toBe(unknown); + expect(deactivated).toBe(wrongPassword); + }); + + it('checks the password before the account status', async () => { + // A deactivated user with a wrong password must fail on the password path. + const compareSpy = jest.spyOn(bcrypt, 'compare'); + usersService.findByEmail.mockResolvedValue(user({ isActive: false })); + + await expect(service.signIn('user@example.com', 'wrong-password')).rejects.toThrow( + 'Invalid credentials', + ); + // bcrypt ran, so the password was verified before any status check. + expect(compareSpy).toHaveBeenCalled(); + }); +}); diff --git a/src/modules/auth/auth.controller.ts b/src/modules/auth/auth.controller.ts index 0062b67..2c5b283 100644 --- a/src/modules/auth/auth.controller.ts +++ b/src/modules/auth/auth.controller.ts @@ -52,19 +52,13 @@ export class AuthController { ); } - // Admin endpoint to manually process pending OTP requests (until cron is set up) - @Post('otp/process') - @HttpCode(HttpStatus.OK) - async processPendingOtps() { - return this.authService.processPendingOtpRequests(); - } - - // Admin endpoint to manually clean up expired OTPs - @Post('otp/cleanup') - @HttpCode(HttpStatus.OK) - async cleanupExpired() { - return this.authService.cleanupExpiredOtps(); - } + // NOTE: `POST /auth/otp/process` and `POST /auth/otp/cleanup` were removed. + // Both were unauthenticated, unguarded "admin" triggers: /otp/process forced + // OTP generation for every pending request (and logged the codes to stdout), + // and /otp/cleanup deleted OTP rows. Both operations already run on cron - + // processPendingOtpRequests every 30s and cleanupExpiredOtps daily - so the + // HTTP triggers added no capability while exposing an OTP-disclosure + // primitive and a data-deletion endpoint to anonymous callers. See #15. // Forgot password / password reset endpoints @Post('password/forgot') diff --git a/src/modules/auth/auth.service.ts b/src/modules/auth/auth.service.ts index 5b4c82c..e07e28a 100644 --- a/src/modules/auth/auth.service.ts +++ b/src/modules/auth/auth.service.ts @@ -1,5 +1,6 @@ import { Injectable, + Logger, UnauthorizedException, BadRequestException, } from '@nestjs/common'; @@ -17,6 +18,9 @@ import { randomInt, randomBytes } from 'crypto'; @Injectable() export class AuthService { + /** Structured logger. Never log an OTP value - see processPendingOtpRequests. */ + private readonly logger = new Logger(AuthService.name); + constructor( private usersService: UsersService, private jwtService: JwtService, @@ -111,6 +115,13 @@ export class AuthService { * Cron job that runs every 30 seconds to process pending OTP requests * Generates OTPs and marks them as ready for use * In production, this would also send the OTP via email/SMS + * + * This is the ONLY path that generates OTPs. It is reachable exclusively via + * the cron schedule - the unauthenticated `POST /auth/otp/process` trigger + * that used to duplicate it was removed in #15. + * + * SECURITY: the generated code is never logged. Only the request id is + * recorded, so log aggregation cannot become an OTP-disclosure channel. */ @Cron(CronExpression.EVERY_30_SECONDS) async processPendingOtpRequests() { @@ -123,7 +134,7 @@ export class AuthService { return; } - console.log( + this.logger.log( `[OTP Cron] Processing ${pendingOtps.length} pending OTP requests`, ); @@ -159,13 +170,16 @@ export class AuthService { // In a real production environment, you would send the OTP here: // await this.emailService.sendOtp(pendingOtp.user.email, otp); // await this.smsService.sendOtp(pendingOtp.user.phone, otp); - console.log( - `[OTP Cron] Generated OTP ${otp} for user ${pendingOtp.user.email}`, + // + // Log the request id only. Logging `otp` here would disclose a live + // one-time credential to anyone with log access. + this.logger.log( + `[OTP Cron] Generated OTP for request ${pendingOtp.id} (user ${pendingOtp.user.email})`, ); } catch (error) { - console.error( + this.logger.error( `[OTP Cron] Failed to process OTP request ${pendingOtp.id}:`, - error, + error instanceof Error ? error.stack : String(error), ); await this.userOtpRepository.update(pendingOtp.id, { status: OtpRequestStatus.FAILED, @@ -270,6 +284,20 @@ export class AuthService { }; } + /** + * Email + password sign in. + * + * Order matters here: the password is verified BEFORE the account status is + * checked. Checking `isActive` first would return a distinct message for a + * deactivated account, turning this endpoint into a user-enumeration oracle + * (an attacker could tell "this account exists but is disabled" apart from + * "no such account"). This path therefore reports the same generic + * 'Invalid credentials' for an unknown email, a wrong password, and a + * deactivated account. + * + * TODO: consider a `tokenVersion` column on User checked by JwtStrategy so + * tokens issued before deactivation stop working. See #14. + */ async signIn(email: string, pass: string): Promise<{ access_token: string }> { const user = await this.usersService.findByEmail(email); if (!user) { @@ -281,6 +309,12 @@ export class AuthService { throw new UnauthorizedException('Invalid credentials'); } + // Checked after the password so the response cannot be used to probe for + // deactivated accounts. Do not move this above bcrypt.compare. + if (!user.isActive) { + throw new UnauthorizedException('Invalid credentials'); + } + const payload = { sub: user.id, email: user.email }; await this.usersService.updateLastLogin(user.id); return { diff --git a/src/modules/payments/payments.service.spec.ts b/src/modules/payments/payments.service.spec.ts index b39f30b..a6f99cd 100644 --- a/src/modules/payments/payments.service.spec.ts +++ b/src/modules/payments/payments.service.spec.ts @@ -1,3 +1,4 @@ +import { ServiceUnavailableException } from '@nestjs/common'; import { PaymentsService } from './payments.service'; import { WalletsService } from '../wallets/wallets.service'; import { BlockchainService } from '../blockchain/blockchain.service'; @@ -153,4 +154,50 @@ describe('PaymentsService', () => { ); expect(savedStatuses).toContain(TransactionStatus.FAILED); }); + + describe('when the routing engine returns no route (#16)', () => { + beforeEach(() => { + walletsService.getWalletById.mockResolvedValue(wallet as any); + aiService.getOptimalRouting.mockResolvedValue({ + optimal_route: null, + alternatives: [], + }); + aiService.checkFraud.mockResolvedValue({ + risk_score: 0.1, + is_suspicious: false, + recommendations: [], + }); + }); + + it('raises a ServiceUnavailableException instead of a TypeError', async () => { + // Reading `.chain` off a null route threw + // "TypeError: Cannot read properties of null" - an unhandled 500. + await expect( + service.initiateTransaction('user-1', 'w1', 'GDEST...', 25, 'XLM'), + ).rejects.toBeInstanceOf(ServiceUnavailableException); + }); + + it('does not throw a TypeError', async () => { + const error = await service + .initiateTransaction('user-1', 'w1', 'GDEST...', 25, 'XLM') + .then(() => null) + .catch((e) => e); + + expect(error).not.toBeInstanceOf(TypeError); + }); + + it('never broadcasts on-chain', async () => { + await expect( + service.initiateTransaction('user-1', 'w1', 'GDEST...', 25, 'XLM'), + ).rejects.toThrow(); + expect(blockchainService.sendTransaction).not.toHaveBeenCalled(); + }); + + it('persists no transaction record', async () => { + await expect( + service.initiateTransaction('user-1', 'w1', 'GDEST...', 25, 'XLM'), + ).rejects.toThrow(); + expect(txRepo.save).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/modules/payments/payments.service.ts b/src/modules/payments/payments.service.ts index b175e01..9d4c4bf 100644 --- a/src/modules/payments/payments.service.ts +++ b/src/modules/payments/payments.service.ts @@ -1,4 +1,7 @@ -import { Injectable } from '@nestjs/common'; +import { + Injectable, + ServiceUnavailableException, +} from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { Transaction, TransactionStatus } from './entities/transaction.entity'; @@ -53,11 +56,22 @@ export class PaymentsService { throw new Error('Transaction flagged as suspicious by fraud detection'); } + // The routing engine returns 503 when no route is viable, so `optimal_route` + // can be null. Reading `.chain` without this check threw + // "TypeError: Cannot read properties of null" - an unhandled 500 triggered by + // ordinary traffic once the engine's routing table was drained (#16). + const route = routingResult.optimal_route; + if (!route) { + throw new ServiceUnavailableException( + 'No payment route is currently available. Please try again shortly.', + ); + } + // Create transaction record const transaction = this.transactionsRepository.create({ amount, currency, - chain: routingResult.optimal_route.chain, + chain: route.chain, fromWallet, toWallet: null, // Will be set if recipient is in our system, else leave null fraudRiskScore: fraudCheck.risk_score, @@ -70,7 +84,7 @@ export class PaymentsService { // Execute the transaction on-chain try { const txHash = await this.blockchainService.sendTransaction( - routingResult.optimal_route.chain, + route.chain, '', // In production, retrieve private key securely from vault, toAddress, amount,