fix(escrow): typed EscrowNotFound error instead of .expect() panics

contracts/escrow/README.md

@@ -11,9 +11,15 @@ When a user sends money via RemitX, the funds can optionally be locked in this e
 | Function | Status | Description |
 |----------|--------|-------------|
 | `deposit(sender, recipient, amount, asset, expires_at) -> BytesN<32>` | Stub | Locks funds in escrow, returns escrow ID |
-| `release(escrow_id)` | Stub | Releases funds to recipient |
-| `refund(escrow_id)` | Stub | Refunds funds to sender after expiry |
-| `get_escrow(escrow_id) -> EscrowState` | **Implemented** | Read-only state getter |
+| `release(escrow_id) -> Result<(), Error>` | Stub | Releases funds to recipient |
+| `refund(escrow_id) -> Result<(), Error>` | Stub | Refunds funds to sender after expiry |
+| `get_escrow(escrow_id) -> Result<EscrowState, Error>` | **Implemented** | Read-only state getter |
+
+`release()`, `refund()`, and `get_escrow()` return `Error::EscrowNotFound`
+(via `soroban_sdk::contracterror`) instead of panicking with an untyped
+message when `escrow_id` doesn't exist. Other invalid states (wrong status,
+not-yet-expired/already-expired) still `panic!()` - covering those with
+typed errors too is tracked separately in #333.
 
 ## What's Implemented vs. Stubbed
 

contracts/escrow/src/lib.rs

@@ -23,11 +23,29 @@
 
 #![no_std]
 use soroban_sdk::{
-    contract, contractimpl, contractmeta, token, xdr::ToXdr, Address, BytesN, Env, Symbol,
+    contract, contracterror, contractimpl, contractmeta, token, xdr::ToXdr, Address, BytesN, Env,
+    Symbol,
 };
 
 contractmeta!(key = "RemitX Escrow", val = "0.1.0");
 
+/// Typed, documented error codes returned by this contract.
+///
+/// Scope note: this only covers the "escrow not found" case flagged in
+/// issue #332 (`get_escrow`/`release`/`refund` previously used
+/// `.expect("Escrow not found")`, which panics with an untyped message).
+/// The other `panic!()` calls in this module (bad amount, wrong status,
+/// not-yet-expired, ...) are a separate, broader scope tracked by #333
+/// ("Define a contracterror enum for all escrow failure modes") and are
+/// intentionally left as-is here to avoid scope creep.
+#[contracterror]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[repr(u32)]
+pub enum Error {
+    /// No escrow exists for the given `escrow_id`.
+    EscrowNotFound = 1,
+}
+
 #[derive(Clone, Debug, Eq, PartialEq)]
 #[soroban_sdk::contracttype]
 pub enum EscrowStatus {
@@ -135,12 +153,16 @@ impl EscrowContract {
     /// contracts/escrow/README.md for context.
     ///
     /// Asserts the escrow is `Locked` and not expired before releasing.
-    pub fn release(env: Env, escrow_id: BytesN<32>) {
+    ///
+    /// Returns `Error::EscrowNotFound` if no escrow exists for `escrow_id`.
+    /// Other invalid states (wrong status, expired) still panic - see the
+    /// scope note on `Error`.
+    pub fn release(env: Env, escrow_id: BytesN<32>) -> Result<(), Error> {
         let state: EscrowState = env
             .storage()
             .instance()
             .get(&EscrowDataKey::Escrow(escrow_id.clone()))
-            .expect("Escrow not found");
+            .ok_or(Error::EscrowNotFound)?;
 
         // Assert the escrow is Locked
         if state.status != EscrowStatus::Locked {
@@ -169,17 +191,23 @@ impl EscrowContract {
             (Symbol::new(&env, "released"), escrow_id),
             (updated.recipient, updated.amount),
         );
+
+        Ok(())
     }
 
     /// Refund escrowed funds to the sender if the escrow has expired.
     ///
     /// Asserts the escrow is `Locked` and has expired before refunding.
-    pub fn refund(env: Env, escrow_id: BytesN<32>) {
+    ///
+    /// Returns `Error::EscrowNotFound` if no escrow exists for `escrow_id`.
+    /// Other invalid states (wrong status, not yet expired) still panic -
+    /// see the scope note on `Error`.
+    pub fn refund(env: Env, escrow_id: BytesN<32>) -> Result<(), Error> {
         let state: EscrowState = env
             .storage()
             .instance()
             .get(&EscrowDataKey::Escrow(escrow_id.clone()))
-            .expect("Escrow not found");
+            .ok_or(Error::EscrowNotFound)?;
 
         // Assert the escrow is Locked
         if state.status != EscrowStatus::Locked {
@@ -208,16 +236,19 @@ impl EscrowContract {
             (Symbol::new(&env, "refunded"), escrow_id),
             (updated.sender, updated.amount),
         );
+
+        Ok(())
     }
 
     /// Read-only getter for escrow state.
     ///
-    /// This is fully implemented since it's a simple storage read.
-    pub fn get_escrow(env: Env, escrow_id: BytesN<32>) -> EscrowState {
+    /// Returns `Error::EscrowNotFound` if no escrow exists for `escrow_id`,
+    /// instead of panicking with an untyped message.
+    pub fn get_escrow(env: Env, escrow_id: BytesN<32>) -> Result<EscrowState, Error> {
         env.storage()
             .instance()
             .get(&EscrowDataKey::Escrow(escrow_id))
-            .expect("Escrow not found")
+            .ok_or(Error::EscrowNotFound)
     }
 
     /// Read-only getter for the total number of escrows created.
@@ -230,4 +261,4 @@ impl EscrowContract {
 }
 
 #[cfg(test)]
-mod test;
+mod test;

contracts/escrow/src/test.rs

@@ -264,19 +264,44 @@ fn test_double_refund_prevented() {
 }
 
 #[test]
-#[should_panic(expected = "Escrow not found")]
-fn test_release_nonexistent_escrow() {
+fn test_release_nonexistent_escrow_returns_typed_error() {
     let env = Env::default();
     let (h, _) = setup(&env, 3600);
     let fake_id = BytesN::from_array(&env, &[0u8; 32]);
-    h.escrow.release(&fake_id);
+    let result = h.escrow.try_release(&fake_id);
+    assert_eq!(result, Err(Ok(Error::EscrowNotFound)));
 }
 
 #[test]
-#[should_panic(expected = "Escrow not found")]
-fn test_refund_nonexistent_escrow() {
+fn test_refund_nonexistent_escrow_returns_typed_error() {
     let env = Env::default();
     let (h, _) = setup(&env, 3600);
     let fake_id = BytesN::from_array(&env, &[0u8; 32]);
-    h.escrow.refund(&fake_id);
-}
+    let result = h.escrow.try_refund(&fake_id);
+    assert_eq!(result, Err(Ok(Error::EscrowNotFound)));
+}
+
+#[test]
+fn test_get_escrow_nonexistent_returns_typed_error() {
+    // EscrowState intentionally doesn't derive PartialEq (out of scope for
+    // this fix), so match the error shape directly instead of assert_eq!.
+    let env = Env::default();
+    let (h, _) = setup(&env, 3600);
+    let fake_id = BytesN::from_array(&env, &[0u8; 32]);
+    let result = h.escrow.try_get_escrow(&fake_id);
+    match result {
+        Err(Ok(Error::EscrowNotFound)) => {}
+        other => panic!("expected Err(Ok(EscrowNotFound)), got {:?}", other),
+    }
+}
+
+#[test]
+fn test_get_escrow_found_still_returns_state_directly() {
+    // Non-try client methods should keep working unchanged for the
+    // success path (panic-on-error is only observable when the escrow
+    // is actually missing, covered above).
+    let env = Env::default();
+    let (h, id) = setup(&env, 3600);
+    let state = h.escrow.get_escrow(&id);
+    assert_eq!(state.status, EscrowStatus::Locked);
+}