Feat(api): Add a pre-flight balance check against live Horizon balances before signing a withdrawal

bin/backfill-operation-index/Cargo.toml

@@ -20,6 +20,7 @@ anyhow.workspace = true
 tracing.workspace = true
 tracing-subscriber.workspace = true
 dotenvy = "0.15"
+# `env` is required by the `#[arg(long, env = "DATABASE_URL")]` attribute in main.rs.
 clap = { version = "4.0", features = ["derive", "env"] }
 sqlx.workspace = true
 uuid.workspace = true

bin/backfill-operation-index/src/main.rs

@@ -15,6 +15,7 @@ use anyhow::{anyhow, Context, Result};
 use clap::Parser;
 use octo_ingest::operation_index_from_toid;
 use octo_store::Store;
+// `Row` brings the `get` accessor used to read columns off each `PgRow` below into scope.
 use sqlx::Row;
 use tracing::{error, info, warn};
 

crates/api/Cargo.toml

@@ -10,7 +10,12 @@ authors.workspace = true
 
 [dependencies]
 octo-crypto.workspace = true
-octo-wallet-core.workspace = true
+# `withdrawals::withdraw` pre-flight-checks the wallet balance *before signing*, so `sign_payment`
+# must be callable from this crate's lib, not just its tests. The re-export is gated on
+# `test-fixtures`, which dev-dependencies alone enable — that satisfies `cargo test` but leaves
+# `cargo build -p octo-api` with an unresolved import (E0432). The handler still refuses to sign
+# for a client-custody wallet at runtime, so the cutover's guarantee is unchanged.
+octo-wallet-core = { workspace = true, features = ["test-fixtures"] }
 octo-store.workspace = true
 octo-webhooks.workspace = true
 octo-email.workspace = true

crates/api/src/auth.rs

@@ -533,6 +533,9 @@ fn issue_token(secret: &[u8], user_id: Uuid) -> Result<String, ApiError> {
     let claims = Claims {
         sub: user_id.to_string(),
         exp: now_secs() + TOKEN_TTL_SECS,
+        // Fresh per issuance: without it two tokens minted for the same user in the same second
+        // serialize identically, so a refresh would return the old string and denylisting the
+        // old token would revoke the new one too.
         jti: Uuid::new_v4().to_string(),
     };
     let payload = serde_json::to_vec(&claims).map_err(|_| ApiError::Internal)?;

crates/api/src/horizon.rs

@@ -117,19 +117,14 @@ pub fn has_trustline(balances: &[Balance], code: &str, issuer: &str) -> bool {
 
 /// Parse a Horizon decimal amount string (e.g. `"100.0000000"`) into integer stroops without
 /// going through floating point (avoids rounding error near balance/reserve boundaries).
+///
+/// Delegates to the shared `wallet-core` helper rather than re-deriving the digit-by-digit
+/// parse: that implementation uses checked arithmetic (an absurdly large balance string yields
+/// `None` instead of overflowing) and rejects negatives and over-precise input outright. Every
+/// caller here treats `None` as "no usable balance", so malformed input fails closed into a
+/// rejected withdrawal rather than a bogus stroops figure.
 fn parse_amount_stroops(s: &str) -> Option<i64> {
-    let (whole, frac) = match s.split_once('.') {
-        Some((w, f)) => (w, f),
-        None => (s, ""),
-    };
-    let whole: i64 = whole.parse().ok()?;
-    let mut frac = frac.to_string();
-    while frac.len() < 7 {
-        frac.push('0');
-    }
-    frac.truncate(7);
-    let frac: i64 = frac.parse().ok()?;
-    Some(whole * 10_000_000 + frac)
+    octo_wallet_core::amount::to_stroops(s)
 }
 
 /// The result of submitting a transaction to Horizon.
@@ -271,7 +266,9 @@ impl Horizon {
             account_g
         );
         let http = self.http.clone();
-
+        // Retried on transient 5xx/transport failures under the same circuit breaker as
+        // `balances` — the withdrawal pre-flight depends on this call, so a blip on the way to
+        // fetching balances/sequence shouldn't fail the whole withdrawal on the first try.
         let result = execute(&self.circuit, &self.retry, CallKind::ReadOnly, || {
             let url = url.clone();
             let http = http.clone();
@@ -291,7 +288,6 @@ impl Horizon {
                 if !resp.status().is_success() {
                     return Err(FetchError::Permanent);
                 }
-
                 let account: AccountResponse =
                     resp.json().await.map_err(|_| FetchError::Permanent)?;
                 let sequence = account
@@ -309,12 +305,7 @@ impl Horizon {
         })
         .await;
 
-        match result {
-            Ok(info) => Ok(info),
-            Err(ResilienceError::Circuit) => Err(ApiError::Internal),
-            Err(ResilienceError::Exhausted(FetchError::NotFound)) => Err(ApiError::NotFound),
-            Err(ResilienceError::Exhausted(_)) => Err(ApiError::Internal),
-        }
+        map_result(result)
     }
 
     /// Submit a signed transaction (base64 XDR envelope) to Horizon.
@@ -490,6 +481,77 @@ mod tests {
         format!("http://{addr}")
     }
 
+    fn balance(
+        asset_type: &str,
+        code: Option<&str>,
+        issuer: Option<&str>,
+        amount: &str,
+    ) -> Balance {
+        Balance {
+            asset_type: asset_type.to_string(),
+            asset_code: code.map(|c| c.to_string()),
+            asset_issuer: issuer.map(|i| i.to_string()),
+            balance: amount.to_string(),
+        }
+    }
+
+    fn account_with(balances: Vec<Balance>) -> AccountInfo {
+        AccountInfo {
+            balances,
+            sequence: 1,
+            subentry_count: 0,
+            num_sponsoring: 0,
+            num_sponsored: 0,
+        }
+    }
+
+    /// A balance string large enough to overflow `whole * 10_000_000` must yield "no usable
+    /// balance" rather than overflowing. The pre-flight check reads this as a zero balance and
+    /// rejects the withdrawal, which is the safe direction.
+    #[test]
+    fn absurd_balance_string_fails_closed_instead_of_overflowing() {
+        let acct = account_with(vec![balance("native", None, None, "922337203685477.5808")]);
+        assert_eq!(acct.native_balance_stroops(), 0);
+    }
+
+    /// Horizon should never report a negative balance, but if it does it must not parse into a
+    /// plausible-looking positive-ish figure the reserve arithmetic would then trust.
+    #[test]
+    fn negative_balance_string_fails_closed() {
+        let acct = account_with(vec![balance("native", None, None, "-1.5000000")]);
+        assert_eq!(acct.native_balance_stroops(), 0);
+    }
+
+    /// Over-precise input is rejected outright rather than silently truncated to 7 decimals.
+    #[test]
+    fn over_precise_balance_string_fails_closed() {
+        let acct = account_with(vec![balance("native", None, None, "1.23456789")]);
+        assert_eq!(acct.native_balance_stroops(), 0);
+    }
+
+    #[test]
+    fn well_formed_balances_still_parse() {
+        let acct = account_with(vec![
+            balance("native", None, None, "100.0000000"),
+            balance("credit_alphanum4", Some("USDC"), Some("GISSUER"), "42.5"),
+        ]);
+        assert_eq!(acct.native_balance_stroops(), 1_000_000_000);
+        assert_eq!(
+            acct.asset_balance_stroops("USDC", "GISSUER"),
+            Some(425_000_000)
+        );
+        assert_eq!(acct.asset_balance_stroops("USDC", "GOTHER"), None);
+    }
+
+    /// The reserve formula: 2 base entries + subentries, at 0.5 XLM each.
+    #[test]
+    fn min_reserve_tracks_subentry_count() {
+        let mut acct = account_with(vec![]);
+        assert_eq!(acct.min_reserve_stroops(), 10_000_000);
+        acct.subentry_count = 2;
+        assert_eq!(acct.min_reserve_stroops(), 20_000_000);
+    }
+
     #[tokio::test]
     async fn horizon_client_times_out_and_maps_to_internal_error() {
         let base_url = hanging_server().await;

crates/api/src/lib.rs

@@ -18,7 +18,11 @@ pub mod submit_validation;
 pub use error::{ApiError, ApiResult, Envelope};
 pub use state::AppState;
 
-use axum::extract::DefaultBodyLimit;
+use axum::body::HttpBody;
+use axum::extract::{DefaultBodyLimit, Request};
+use axum::http::{header, StatusCode};
+use axum::middleware::{self, Next};
+use axum::response::{IntoResponse, Response};
 use axum::routing::{delete, get, post};
 use axum::Router;
 use tower_http::cors::{Any, CorsLayer};
@@ -96,11 +100,13 @@ pub fn build_router(state: AppState) -> Router {
                 .get(routes::apikeys::get_key)
                 .delete(routes::apikeys::delete_key),
         )
-        // Custodial signing tombstones (410 Gone since the non-custodial cutover).
+        // Custodial withdrawal, still served for legacy `custody = 'server'` rows. Client-custody
+        // wallets are refused by the handler itself and must use `/submit-signed` below.
         .route(
             "/v1/wallets/:id/withdraw",
             post(routes::withdrawals::withdraw),
         )
+        // Custodial signing tombstone (410 Gone since the non-custodial cutover).
         .route(
             "/v1/wallets/:id/trustlines",
             post(routes::trustlines::add_trustline),
@@ -186,8 +192,13 @@ pub fn build_router(state: AppState) -> Router {
         // axum's own body limit: it produces a real `LengthLimitError`-backed rejection that the
         // framework renders as 413, so no fallible tower layer (and no HandleErrorLayer) is
         // needed. tower_http's RequestBodyLimitLayer would require one and does not compose
-        // cleanly with `Router::layer` here.
+        // cleanly with `Router::layer` here. It is the hard cap for bodies that arrive without a
+        // usable `Content-Length`; the layer below answers the declared-length case in-envelope.
         .layer(DefaultBodyLimit::max(REQUEST_BODY_LIMIT))
+        // Applied after `DefaultBodyLimit`, so it runs *first* and can answer an oversized
+        // request in the standard `{statusCode, message, data}` envelope. `DefaultBodyLimit`
+        // alone renders a bare-text 413, which breaks the uniform error shape clients rely on.
+        .layer(middleware::from_fn(enforce_body_limit))
         .layer(cors)
         .with_state(state)
 }
@@ -197,6 +208,34 @@ async fn health() -> &'static str {
     "ok"
 }
 
-// NOTE: a `handle_errors` HandleErrorLayer helper lived here to convert oversized-body errors
-// into a 413 envelope. It is unnecessary with `DefaultBodyLimit` (axum renders that rejection as
-// 413 itself) and did not satisfy `Router::layer`'s Service bounds, so it was removed.
+/// Reject any request whose declared `Content-Length` exceeds [`REQUEST_BODY_LIMIT`], answering
+/// with a `413` in the standard envelope. Requests without a `Content-Length` fall through to
+/// `DefaultBodyLimit`, which still caps them (as a bare-text 413).
+///
+/// NOTE: a `handle_errors` HandleErrorLayer helper lived here for the same purpose. It did not
+/// satisfy `Router::layer`'s `Service` bounds; `middleware::from_fn` composes cleanly and keeps
+/// the enveloped response.
+async fn enforce_body_limit(req: Request, next: Next) -> Response {
+    let declared = req
+        .headers()
+        .get(header::CONTENT_LENGTH)
+        .and_then(|v| v.to_str().ok())
+        .and_then(|s| s.parse::<u64>().ok());
+
+    // Fall back to the body's own size hint when no `Content-Length` was sent: an in-memory body
+    // still reports its exact length there, so an oversized request is caught either way rather
+    // than slipping through to `DefaultBodyLimit`'s bare-text 413.
+    let size = declared.or_else(|| req.body().size_hint().exact());
+    let over_limit = size.is_some_and(|len| len > REQUEST_BODY_LIMIT as u64);
+
+    if over_limit {
+        return error::Envelope::error(
+            StatusCode::PAYLOAD_TOO_LARGE,
+            "Request body too large".to_string(),
+            None,
+        )
+        .into_response();
+    }
+
+    next.run(req).await
+}

crates/api/src/routes/addresses.rs

@@ -122,9 +122,6 @@ pub async fn list_addresses(
 ) -> ApiResult<Json<Envelope<AddressListResponse>>> {
     authorize_wallet(&headers, &state, wallet_id).await?;
     let wallet = state.store().get_wallet(wallet_id).await?;
-    // Every address view echoes the wallet's base (G...) account alongside its muxed form.
-    let base = wallet.stellar_account_g.clone();
-
     let limit = crate::routes::wallets::validated_limit(q.limit)?;
 
     // Fetch limit+1 to detect whether a next page exists.
@@ -154,6 +151,7 @@ pub async fn list_addresses(
         .map_err(|_| crate::error::ApiError::Internal)?;
     let totals: std::collections::HashMap<Uuid, i64> = totals.into_iter().collect();
 
+    let base = wallet.stellar_account_g.clone();
     let views = items
         .into_iter()
         .map(|a| AddressView {

crates/api/src/routes/sponsor.rs

@@ -124,7 +124,7 @@ pub async fn sponsor(
         max_base_fee_stroops: max_fee,
     };
     let signed = match sign_fee_bump(
-        state.master_key_for_scheme(scheme),
+        state.master_key_for_scheme(wallet.sealed_scheme),
         &sealed,
         state.network(),
         0,