fix(billing): decode contract reverts + treat reverted tx as failure (#481)

Two independent correctness bugs surfaced while debugging k6 smoke
500/402 failures on staging:

1. `get_billing_wallet_address` / `get_account_wallet_address` propagated
   the raw alloy error from `.call()` without running it through
   `decode_contract_revert`. The resulting string is just the 4-byte
   selector, so `billing::wallet_resolution_err`'s substring match on
   "AccountDoesNotExist" never fired — a missing account was returned as a
   500 ("internal billing lookup error") instead of the intended 400.
   Now both helpers decode the revert, so a missing account maps to 400
   and the error string is human-readable in logs.

2. `send_transaction` treated any returned receipt as success. A
   transaction that reverts on-chain still yields a receipt (EIP-658
   status 0), so a reverted write (e.g. `newAccount`) was reported as
   succeeded. Added `receipt_to_result`, which inspects the status and
   surfaces an on-chain revert as an error, applied at both receipt sites.

Adds regression tests for the selector→name mapping and the
wallet-resolution status mapping (400 vs 500).

Co-authored-by: Chris Cassano <chris@litprotocol.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: 3 people

lit-api-server/src/accounts/decode_revert.rs

@@ -97,3 +97,29 @@ pub fn decode_contract_revert(err: &alloy::contract::Error) -> String {
 
     format!("{err}")
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use alloy::sol_types::SolError;
+
+    use crate::accounts::contracts::account_config_contract::AccountConfig;
+
+    /// The `AccountDoesNotExist` selector must resolve to its name. Billing's
+    /// `wallet_resolution_err` substring-matches on this name to return a 400
+    /// (account not found) rather than a 500, so a selector/name drift here
+    /// would silently turn missing-account lookups back into 500s.
+    #[test]
+    fn account_does_not_exist_selector_resolves_to_name() {
+        let selector = AccountConfig::AccountDoesNotExist::SELECTOR;
+        assert_eq!(
+            account_config_error_name(&selector),
+            Some("AccountDoesNotExist")
+        );
+    }
+
+    #[test]
+    fn unknown_selector_resolves_to_none() {
+        assert_eq!(account_config_error_name(&[0x00, 0x00, 0x00, 0x00]), None);
+    }
+}

lit-api-server/src/accounts/mod.rs

@@ -768,7 +768,15 @@ pub async fn can_execute_action_and_use_wallet(
 pub async fn get_account_wallet_address(key_or_hash: &str) -> Result<String> {
     let contract = get_read_only_account_config_contract().await?;
     let key_hash = usage_api_key_to_hash(key_or_hash);
-    let wallet_address = contract.getAccountWalletAddress(key_hash).call().await?;
+    // Decode contract reverts (e.g. `AccountDoesNotExist`) into a human-readable
+    // string. Without this the raw alloy error is just the 4-byte selector, which
+    // callers like `billing::wallet_resolution_err` substring-match on to map to
+    // the right HTTP status (a missing account is a 400, not a 500).
+    let wallet_address = contract
+        .getAccountWalletAddress(key_hash)
+        .call()
+        .await
+        .map_err(|e| anyhow::anyhow!("{}", decode_contract_revert(&e)))?;
     if wallet_address == Address::ZERO {
         anyhow::bail!("account has no wallet address");
     }
@@ -793,7 +801,15 @@ pub async fn get_account_wallet_address(key_or_hash: &str) -> Result<String> {
 pub async fn get_billing_wallet_address(key_or_hash: &str) -> Result<String> {
     let contract = get_read_only_account_config_contract().await?;
     let key_hash = usage_api_key_to_hash(key_or_hash);
-    let wallet_address = contract.getBillingWalletAddress(key_hash).call().await?;
+    // Decode contract reverts (e.g. `AccountDoesNotExist`) into a human-readable
+    // string so `billing::wallet_resolution_err` can map a missing account to a
+    // 400 instead of a confusing 500. A raw alloy revert is just the 4-byte
+    // selector, which the substring match never catches.
+    let wallet_address = contract
+        .getBillingWalletAddress(key_hash)
+        .call()
+        .await
+        .map_err(|e| anyhow::anyhow!("{}", decode_contract_revert(&e)))?;
     if wallet_address == Address::ZERO {
         anyhow::bail!("account has no wallet address");
     }

lit-api-server/src/accounts/signable_contract.rs

@@ -116,6 +116,25 @@ pub(crate) async fn get_read_only_account_config_contract() -> Result<AccountCon
     Ok(contract)
 }
 
+/// Convert a mined transaction receipt into a success/failure result.
+///
+/// `get_receipt()` resolves successfully even when the transaction reverted
+/// on-chain — the receipt simply carries an EIP-658 status of 0. Treating that
+/// as success would report a failed write (e.g. a reverted `newAccount`) as
+/// having succeeded, leaving callers to act on state that was never persisted.
+/// We inspect the status explicitly and surface a revert as an error.
+fn receipt_to_result(receipt: alloy::rpc::types::TransactionReceipt) -> Result<bool> {
+    if receipt.status() {
+        Ok(true)
+    } else {
+        Err(anyhow::anyhow!(
+            "transaction reverted on-chain (tx hash: {:#x}, block: {:?})",
+            receipt.transaction_hash,
+            receipt.block_number
+        ))
+    }
+}
+
 pub async fn send_transaction<D>(
     function_call: CallBuilder<&SigningClient, D, Ethereum>,
     signer_pool: std::sync::Arc<SignerPool>,
@@ -139,7 +158,7 @@ where
     let first_err = match function_call.send().await {
         Ok(tx) => {
             let result = match tx.get_receipt().await {
-                Ok(_) => Ok(true),
+                Ok(receipt) => receipt_to_result(receipt),
                 Err(e) => Err(anyhow::Error::from(e)),
             };
             signer_pool.release(signer_address).await?;
@@ -205,7 +224,7 @@ where
     };
 
     let result = match tx.get_receipt().await {
-        Ok(_) => Ok(true),
+        Ok(receipt) => receipt_to_result(receipt),
         Err(e) => Err(e.into()),
     };
 

lit-api-server/src/core/v1/endpoints/billing.rs

@@ -162,3 +162,37 @@ async fn billing_confirm_payment_impl(
         .map_err(|e| ApiStatus::internal_server_error(e, "Stripe error"))?;
     Ok(AccountOpResponse { success: true })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use rocket::http::Status;
+
+    /// A missing account must map to 400, not 500. The wallet-lookup helpers
+    /// run the contract revert through `decode_contract_revert`, so the error
+    /// string contains the `AccountDoesNotExist` error name — this asserts the
+    /// substring match still routes it to a client error.
+    #[test]
+    fn account_does_not_exist_maps_to_400() {
+        let err = anyhow::anyhow!("Contract error: AccountDoesNotExist (0xd4a84737...)");
+        assert_eq!(wallet_resolution_err(err).status, Status::BadRequest);
+    }
+
+    #[test]
+    fn missing_wallet_address_maps_to_400() {
+        let err = anyhow::anyhow!("account has no wallet address");
+        assert_eq!(wallet_resolution_err(err).status, Status::BadRequest);
+    }
+
+    /// RPC/transport failures (anything that isn't a known missing-account
+    /// revert) stay 500 so transient infra problems aren't reported to clients
+    /// as a bad API key.
+    #[test]
+    fn other_errors_map_to_500() {
+        let err = anyhow::anyhow!("error sending request: connection refused");
+        assert_eq!(
+            wallet_resolution_err(err).status,
+            Status::InternalServerError
+        );
+    }
+}