Skip to content

Replace storage-read panics with typed errors in escrow contract#281

Merged
Xuccessor merged 2 commits into
BountyOnChain:mainfrom
Olorunfemi20:fix/156-escrow-contract-error-handling
Jun 20, 2026
Merged

Replace storage-read panics with typed errors in escrow contract#281
Xuccessor merged 2 commits into
BountyOnChain:mainfrom
Olorunfemi20:fix/156-escrow-contract-error-handling

Conversation

@Olorunfemi20

Copy link
Copy Markdown
Contributor

Closes #156

Problem

The EscrowContract read persistent storage with .unwrap() in 8 places. A
missing key (uninitialized contract, race condition, key typo, or corruption)
caused the whole transaction to panic and revert with a generic "host error",
costing users gas with no actionable reason and risking locked funds.

Changes

  • Added a ContractError enum (#[contracterror], #[repr(u32)]) with 12
    descriptive, documented variants, including the required NotInitialized,
    AlreadyInitialized, Unauthorized, InvalidStatus, InvalidWinner, and
    InsufficientBalance.
  • Replaced all 8 .unwrap() storage reads (and the .expect() on the pending
    operation) with typed read helpers using .ok_or(ContractError::NotInitialized).
  • Every public function now returns Result<…, ContractError>; getters return
    Result<T, ContractError>.
  • Converted the assert_owner/assert_contributor/assert_arbitrator/
    assert_status/assert_unlocked helpers and the timelock helpers from
    assert!/panic! to Result returns, threaded with ?.
  • Validation branches now return precise errors (InvalidAmount,
    AlreadyInitialized, Unauthorized, InvalidStatus, InvalidWinner,
    PendingOperationExists, NoPendingOperation, OperationLocked,
    OperationAlreadyUnlocked, InvalidOperation).

Tests

  • All #[should_panic] tests replaced with try_* calls asserting the exact
    Err variant (the token-allowance trap uses is_err()).
  • Added error-path tests: fund/approve on an uninitialized contract,
    double-fund, resolve with an invalid winner, and execute with no pending
    operation.

Note: cargo was unavailable in my environment, so cargo test and
cargo clippy still need to be run locally/CI to confirm green.

Copy link
Copy Markdown
Contributor

Hey 👋 Nice work on this refactor — swapping assert! panics for a typed ContractError enum and Result-returning functions is a clean improvement, and the spec updates exercise the new failure paths. Merging. 🚀

@Xuccessor Xuccessor merged commit edcf9d8 into BountyOnChain:main Jun 20, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SECURITY] Soroban contract uses unwrap() — panics in production

2 participants