Harden auth deactivation, remove unauthenticated OTP endpoints, stop routing table drain

.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

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))
 

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"""

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()

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[];
 }
 

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<string, jest.Mock>;
+
+  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' });
+    }
+  });
+});