PaymentService (get_quote + execute_send, the primary money-movement path) has zero test coverage

[abayomicornelius]

Overview

src/services/payment.rs — PaymentService, the service behind POST /api/payments/quote (rate/path quoting) and POST /api/payments/send (the primary money-movement endpoint of the entire application) — has zero test coverage. There is no #[cfg(test)] mod tests block in the file at all.

This is a striking gap given the rest of the codebase's testing discipline: services/batch.rs (the other payment-submission path) has 5 tests covering empty-batch rejection, non-positive-amount rejection, invalid-XDR-before-touching-the-database, and both branches of classify_submission_error; services/escrow.rs has 6 tests covering its full authorization matrix; services/subscription.rs, services/payment_request.rs, services/reconciliation.rs, services/tx_hash.rs, and services/soroban.rs all have their own dedicated test modules. PaymentService — arguably the first and most heavily-used payment path in the API — has none.

Concretely untested logic in this file includes:

• get_quote's send-amount validation (payment.rs:32-41) — the exact same f64 parse-and-compare pattern already flagged as a precision risk in EscrowService uses f64 for amount validation, risking precision loss on financial values #29 for EscrowService, unverified here by any test either way.
• get_quote's "pick the best path" selection (payment.rs:59-66):

  let best = paths.iter().max_by(|a, b| {
      let fa: f64 = a.destination_amount.parse().unwrap_or(0.0);
      let fb: f64 = b.destination_amount.parse().unwrap_or(0.0);
      fa.partial_cmp(&fb).unwrap_or(std::cmp::Ordering::Equal)
  }).ok_or(AppError::NoPathFound)?;

  No test exercises this against multiple candidate paths to confirm it actually picks the highest-destination_amount one, nor exercises the unwrap_or(Equal) fallback for a malformed/unparseable destination_amount from Horizon.
• The implied-rate computation (payment.rs:69-74) and its division-by-zero guard (if send_f > 0.0 { ... } else { "0" }) — untested.
• execute_send's branching between "existing transaction_id supplied" vs "create a fresh record" (payment.rs:114-160), including the ownership check (if tx.user_id != user_id { return Err(Forbidden) }) — none of these branches are exercised by any test.
• execute_send's success/failure status-update paths (payment.rs:163-196) — untested, unlike the equivalent logic in batch.rs, which has explicit classify_submission_error unit tests.

Requirements

• Add a #[cfg(test)] mod tests block to src/services/payment.rs following the same patterns already established elsewhere in the codebase: a wiremock-backed StellarService for get_quote's Horizon-dependent logic (mirroring the pattern in services/reconciliation.rs's db_tests), and a PgPool::connect_lazy(...) dummy pool plus pure-logic tests for branches that don't need real I/O (mirroring services/batch.rs's dummy_pool() helper).
• At minimum, cover: quote rejects non-positive/unparseable amount; quote selects the path with the highest destination_amount among multiple candidates; quote's rate computation for a zero send amount doesn't panic/divide-by-zero; execute_send rejects empty signed_xdr before any DB/network call (mirroring batch.rs's rejects_invalid_signed_xdr_before_touching_the_database test); execute_send with a transaction_id owned by a different user is rejected Forbidden.

Acceptance Criteria

• src/services/payment.rs has a test module covering the scenarios listed above.
• Tests run under plain cargo test without requiring a live Postgres instance where the logic under test doesn't touch the database (matching the dummy_pool()/lazy-connect pattern already used in batch.rs and payment_request.rs).
• The best-path-selection logic specifically has a test with ≥2 candidate paths of differing destination_amount, asserting the higher one is chosen — this is exactly the kind of comparator logic (partial_cmp(...).unwrap_or(Ordering::Equal)) that silently does the wrong thing on subtly malformed input with no test to catch it.

Additional Notes

Why this is worth a dedicated issue rather than folding into a generic "add more tests" effort

As with the companion middleware/auth.rs testing-gap issue, this isn't part of a blanket "no tests exist" problem — most of the codebase's services do have tests. This is a specific, sharp gap in the highest-traffic money-movement file, which happens to sit right next to a sibling file (batch.rs) that demonstrates exactly what adequate coverage for this kind of logic looks like.

Testing strategy

• services/batch.rs's existing test module is the closest template: same dummy_pool() pattern, same "assert rejection happens before any DB/network I/O" style of test for cheap validation checks.
• get_quote's Horizon-dependent tests need a wiremock-mocked /paths/strict-send response with multiple path records to meaningfully test the max_by selection — a single-path mock would pass trivially without actually proving the comparator picks correctly.

Cross-references

• Complements, but doesn't overlap, the (separately filed) testing gap in middleware/auth.rs — that covers the authentication path gating every endpoint; this covers the money-movement path itself.