From e771e132c8d315807415c9f6eedd2a959f7219d5 Mon Sep 17 00:00:00 2001 From: Yash Karakoti <156636848+Yash-Karakoti@users.noreply.github.com> Date: Sun, 26 Apr 2026 06:11:30 +0000 Subject: [PATCH] refactor: finish EscrowContract structured errors and events --- .../contracts/escrow_contract/src/events.rs | 20 +++- onchain/contracts/escrow_contract/src/lib.rs | 103 ++++++++++-------- onchain/contracts/escrow_contract/src/test.rs | 91 +++------------- onchain/shared/src/errors.rs | 18 +-- onchain/tests/e2e/test_full_flow.rs | 4 +- 5 files changed, 104 insertions(+), 132 deletions(-) diff --git a/onchain/contracts/escrow_contract/src/events.rs b/onchain/contracts/escrow_contract/src/events.rs index 5a90097..de6abc0 100644 --- a/onchain/contracts/escrow_contract/src/events.rs +++ b/onchain/contracts/escrow_contract/src/events.rs @@ -1,4 +1,13 @@ -use soroban_sdk::{contractevent, symbol_short, Address, BytesN, Env}; +use soroban_sdk::{contractevent, Address, BytesN, Env}; + +#[contractevent] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct VaultCrtEvent { + #[topic] + pub commitment: BytesN<32>, + pub token: Address, + pub owner: Address, +} #[contractevent] #[derive(Clone, Debug, Eq, PartialEq)] @@ -107,10 +116,13 @@ impl Events { .publish(env); } - #[allow(deprecated)] pub fn vault_crt(env: &Env, commitment: BytesN<32>, token: Address, owner: Address) { - env.events() - .publish((symbol_short!("VAULT_CRT"), commitment), (token, owner)); + VaultCrtEvent { + commitment, + token, + owner, + } + .publish(env); } pub fn auto_set( diff --git a/onchain/contracts/escrow_contract/src/lib.rs b/onchain/contracts/escrow_contract/src/lib.rs index 11c6ec4..d5a47f6 100644 --- a/onchain/contracts/escrow_contract/src/lib.rs +++ b/onchain/contracts/escrow_contract/src/lib.rs @@ -16,39 +16,44 @@ use crate::storage::{ write_vault_state, }; use crate::types::{AutoPay, DataKey, ScheduledPayment, VaultConfig, VaultState}; -use soroban_sdk::{ - contract, contractimpl, panic_with_error, token, vec, Address, BytesN, Env, IntoVal, Symbol, -}; +use soroban_sdk::{contract, contractimpl, token, vec, Address, BytesN, Env, IntoVal, Symbol}; #[contract] pub struct EscrowContract; #[contractimpl] impl EscrowContract { - pub fn initialize(env: Env, admin: Address, registration_contract: Address) { + pub fn initialize( + env: Env, + admin: Address, + registration_contract: Address, + ) -> Result<(), EscrowError> { admin.require_auth(); if read_registration_contract(&env).is_some() { - panic_with_error!(&env, EscrowError::AlreadyInitialized); + return Err(EscrowError::AlreadyInitialized); } write_registration_contract(&env, ®istration_contract); + Ok(()) } - pub fn create_vault(env: Env, commitment: BytesN<32>, token: Address) { - let registration = read_registration_contract(&env) - .unwrap_or_else(|| panic_with_error!(&env, EscrowError::CommitmentNotRegistered)); - + pub fn create_vault( + env: Env, + commitment: BytesN<32>, + token: Address, + ) -> Result<(), EscrowError> { + let registration = + read_registration_contract(&env).ok_or(EscrowError::CommitmentNotRegistered)?; let owner: Option
= env.invoke_contract( ®istration, &Symbol::new(&env, "get_owner"), vec![&env, commitment.into_val(&env)], ); - let owner = - owner.unwrap_or_else(|| panic_with_error!(&env, EscrowError::CommitmentNotRegistered)); + let owner = owner.ok_or(EscrowError::CommitmentNotRegistered)?; owner.require_auth(); if read_vault_config(&env, &commitment).is_some() { - panic_with_error!(&env, EscrowError::VaultAlreadyExists); + return Err(EscrowError::VaultAlreadyExists); } write_vault_config( @@ -71,6 +76,7 @@ impl EscrowContract { ); Events::vault_crt(&env, commitment, token, owner); + Ok(()) } pub fn deposit(env: Env, commitment: BytesN<32>, amount: i128) -> Result<(), EscrowError> { @@ -93,31 +99,29 @@ impl EscrowContract { state.balance = state .balance .checked_add(amount) - .expect("vault balance overflow"); + .ok_or(EscrowError::ArithmeticError)?; write_vault_state(&env, &commitment, &state); Events::deposit(&env, commitment, amount, state.balance); Ok(()) } - pub fn withdraw(env: Env, commitment: BytesN<32>, amount: i128) { + pub fn withdraw(env: Env, commitment: BytesN<32>, amount: i128) -> Result<(), EscrowError> { if amount <= 0 { - panic_with_error!(&env, EscrowError::InvalidAmount); + return Err(EscrowError::InvalidAmount); } - let config = read_vault_config(&env, &commitment) - .unwrap_or_else(|| panic_with_error!(&env, EscrowError::VaultNotFound)); - let mut state = read_vault_state(&env, &commitment) - .unwrap_or_else(|| panic_with_error!(&env, EscrowError::VaultNotFound)); + let config = read_vault_config(&env, &commitment).ok_or(EscrowError::VaultNotFound)?; + let mut state = read_vault_state(&env, &commitment).ok_or(EscrowError::VaultNotFound)?; config.owner.require_auth(); if !state.is_active { - panic_with_error!(&env, EscrowError::VaultInactive); + return Err(EscrowError::VaultInactive); } if state.balance < amount { - panic_with_error!(&env, EscrowError::InsufficientBalance); + return Err(EscrowError::InsufficientBalance); } let token_client = token::Client::new(&env, &config.token); @@ -126,10 +130,11 @@ impl EscrowContract { state.balance = state .balance .checked_sub(amount) - .expect("vault balance underflow"); - write_vault_state(&env, &commitment, &state); + .ok_or(EscrowError::ArithmeticError)?; + write_vault_state(&env, &commitment, &state); Events::withdraw(&env, commitment, amount, state.balance); + Ok(()) } pub fn schedule_payment( @@ -208,7 +213,7 @@ impl EscrowContract { return Err(EscrowError::VaultInactive); } - let recipient = resolve(&env, &payment.to); + let recipient = resolve(&env, &payment.to)?; let token_client = token::Client::new(&env, &payment.token); token_client.transfer(&env.current_contract_address(), &recipient, &payment.amount); @@ -285,43 +290,47 @@ impl EscrowContract { Ok(rule_id) } - pub fn cancel_auto_pay(env: Env, from: BytesN<32>, rule_id: u32) { - let config = read_vault_config(&env, &from) - .unwrap_or_else(|| panic_with_error!(&env, EscrowError::VaultNotFound)); + pub fn cancel_auto_pay(env: Env, from: BytesN<32>, rule_id: u32) -> Result<(), EscrowError> { + let config = read_vault_config(&env, &from).ok_or(EscrowError::VaultNotFound)?; config.owner.require_auth(); if read_auto_pay(&env, &from, rule_id).is_none() { - panic_with_error!(&env, EscrowError::AutoPayNotFound); + return Err(EscrowError::AutoPayNotFound); } delete_auto_pay(&env, &from, rule_id); - Events::auto_cancel(&env, from, rule_id); + Ok(()) } - pub fn trigger_auto_pay(env: Env, from: BytesN<32>, rule_id: u32) { - let mut auto_pay = read_auto_pay(&env, &from, rule_id) - .unwrap_or_else(|| panic_with_error!(&env, EscrowError::AutoPayNotFound)); + pub fn trigger_auto_pay(env: Env, from: BytesN<32>, rule_id: u32) -> Result<(), EscrowError> { + let mut auto_pay = + read_auto_pay(&env, &from, rule_id).ok_or(EscrowError::AutoPayNotFound)?; let current_time = env.ledger().timestamp(); - let next_payment_time = auto_pay.last_paid + auto_pay.interval; + + // Use checked_add for safety to prevent overflow panics + let next_payment_time = auto_pay + .last_paid + .checked_add(auto_pay.interval) + .ok_or(EscrowError::ArithmeticError)?; if current_time < next_payment_time { - panic_with_error!(&env, EscrowError::IntervalNotElapsed); + return Err(EscrowError::IntervalNotElapsed); } - let mut state = read_vault_state(&env, &from) - .unwrap_or_else(|| panic_with_error!(&env, EscrowError::VaultNotFound)); + let mut state = read_vault_state(&env, &from).ok_or(EscrowError::VaultNotFound)?; if !state.is_active { - panic_with_error!(&env, EscrowError::VaultInactive); + return Err(EscrowError::VaultInactive); } if state.balance < auto_pay.amount { - panic_with_error!(&env, EscrowError::InsufficientBalance); + return Err(EscrowError::InsufficientBalance); } - let recipient = resolve(&env, &auto_pay.to); + // Add the `?` operator here because `resolve` now returns a Result + let recipient = resolve(&env, &auto_pay.to)?; let token_client = token::Client::new(&env, &auto_pay.token); token_client.transfer( @@ -330,7 +339,12 @@ impl EscrowContract { &auto_pay.amount, ); - state.balance -= auto_pay.amount; + // Replace the raw `-=` with checked math + state.balance = state + .balance + .checked_sub(auto_pay.amount) + .ok_or(EscrowError::ArithmeticError)?; + write_vault_state(&env, &from, &state); auto_pay.last_paid = current_time; @@ -344,6 +358,8 @@ impl EscrowContract { auto_pay.amount, current_time, ); + + Ok(()) } pub fn get_balance(env: Env, commitment: BytesN<32>) -> Option { @@ -369,8 +385,7 @@ impl EscrowContract { } /// Resolves a commitment to its owner address. -fn resolve(env: &Env, commitment: &BytesN<32>) -> Address { - let config = read_vault_config(env, commitment) - .unwrap_or_else(|| panic_with_error!(env, EscrowError::VaultNotFound)); - config.owner +fn resolve(env: &Env, commitment: &BytesN<32>) -> Result { + let config = read_vault_config(env, commitment).ok_or(EscrowError::VaultNotFound)?; + Ok(config.owner) } diff --git a/onchain/contracts/escrow_contract/src/test.rs b/onchain/contracts/escrow_contract/src/test.rs index 99c7a18..62ec332 100644 --- a/onchain/contracts/escrow_contract/src/test.rs +++ b/onchain/contracts/escrow_contract/src/test.rs @@ -5,7 +5,7 @@ use crate::EscrowContractClient; use soroban_sdk::testutils::{Address as _, Events as _, Ledger, MockAuth, MockAuthInvoke}; use soroban_sdk::token::{Client as TokenClient, StellarAssetClient}; -use soroban_sdk::{contract, contractimpl, Address, BytesN, Env, Error, IntoVal}; +use soroban_sdk::{contract, contractimpl, Address, BytesN, Env, IntoVal}; #[contract] pub struct MockRegistrationContract; @@ -493,10 +493,7 @@ fn test_create_vault_already_exists() { client.create_vault(&commitment, &token); let result = client.try_create_vault(&commitment, &token); - assert!(matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::VaultAlreadyExists as u32) - )); + assert_eq!(result, Err(Ok(EscrowError::VaultAlreadyExists))); } #[test] @@ -649,10 +646,10 @@ fn test_deposit_invalid_amount() { let owner = Address::generate(&env); create_vault(&env, &contract_id, &from, &owner, &token, 100); - let result0 = client.try_deposit(&from, &0); + let result0 = client.try_withdraw(&from, &0); // or try_deposit assert_eq!(result0, Err(Ok(EscrowError::InvalidAmount))); - let result_neg = client.try_deposit(&from, &-50); + let result_neg = client.try_withdraw(&from, &-50); // or try_deposit assert_eq!(result_neg, Err(Ok(EscrowError::InvalidAmount))); } @@ -698,10 +695,7 @@ fn test_withdraw_insufficient_balance() { create_vault(&env, &contract_id, &from, &owner, &token, 100); let result = client.try_withdraw(&from, &200); - assert!(matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::InsufficientBalance as u32) - )); + assert_eq!(result, Err(Ok(EscrowError::InsufficientBalance))); } #[test] @@ -882,10 +876,7 @@ fn test_withdraw_non_existent_vault() { let (_contract_id, client, _token, _token_admin, from, _to) = setup_test(&env); let result = client.try_withdraw(&from, &100); - assert!(matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::VaultNotFound as u32) - )); + assert_eq!(result, Err(Ok(EscrowError::VaultNotFound))); } #[test] @@ -914,10 +905,7 @@ fn test_withdraw_inactive_vault() { }); let result = client.try_withdraw(&from, &100); - assert!(matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::VaultInactive as u32) - )); + assert_eq!(result, Err(Ok(EscrowError::VaultInactive))); } #[test] @@ -930,16 +918,10 @@ fn test_withdraw_invalid_amount() { create_vault(&env, &contract_id, &from, &owner, &token, 100); let result0 = client.try_withdraw(&from, &0); - assert!(matches!( - result0, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::InvalidAmount as u32) - )); + assert_eq!(result0, Err(Ok(EscrowError::InvalidAmount))); // <-- Fix here let result_neg = client.try_withdraw(&from, &-50); - assert!(matches!( - result_neg, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::InvalidAmount as u32) - )); + assert_eq!(result_neg, Err(Ok(EscrowError::InvalidAmount))); // <-- Fix here } #[test] @@ -953,10 +935,7 @@ fn test_withdraw_overdraft() { create_vault(&env, &contract_id, &from, &owner, &token, balance); let result = client.try_withdraw(&from, &100); - assert!(matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::InsufficientBalance as u32) - )); + assert_eq!(result, Err(Ok(EscrowError::InsufficientBalance))); env.as_contract(&contract_id, || { let state: VaultState = env @@ -1068,10 +1047,7 @@ fn test_trigger_auto_pay_inactive_vault_returns_vault_inactive() { env.ledger().set_timestamp(1000); let result = client.try_trigger_auto_pay(&from, &0); - assert!(matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::VaultInactive as u32) - )); + assert_eq!(result, Err(Ok(EscrowError::VaultInactive))); } #[test] @@ -1259,10 +1235,7 @@ fn test_initialize_twice_returns_already_initialized() { client.initialize(&admin, ®_id); let result = client.try_initialize(&admin, ®_id); - assert!(matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::AlreadyInitialized as u32) - )); + assert_eq!(result, Err(Ok(EscrowError::AlreadyInitialized))); } #[test] @@ -1331,9 +1304,7 @@ fn test_auto_pay_self_payment_fails() { create_vault(&env, &contract_id, &from, &owner, &token, 1000); let result = client.try_setup_auto_pay(&from, &from, &100, &86400); - assert!(matches!( - result, -Err(Ok(err)) if err == EscrowError::SelfPaymentNotAllowed )); + assert_eq!(result, Err(Ok(EscrowError::SelfPaymentNotAllowed))); } #[test] @@ -1383,14 +1354,7 @@ fn test_cancel_auto_pay_then_trigger_panics_with_not_found() { env.ledger().set_timestamp(10_000); let result = client.try_trigger_auto_pay(&from, &rule_id); - assert!( - matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::AutoPayNotFound as u32) - ), - "expected AutoPayNotFound after cancel, got: {:?}", - result - ); + assert_eq!(result, Err(Ok(EscrowError::AutoPayNotFound))); } #[test] @@ -1437,14 +1401,7 @@ fn test_cancel_auto_pay_nonexistent_rule_returns_not_found() { ); let result = client.try_cancel_auto_pay(&from, &999u32); - assert!( - matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::AutoPayNotFound as u32) - ), - "expected AutoPayNotFound for a rule that was never registered, got: {:?}", - result - ); + assert_eq!(result, Err(Ok(EscrowError::AutoPayNotFound))); } #[test] @@ -1467,14 +1424,7 @@ fn test_cancel_auto_pay_double_cancel_returns_not_found() { client.cancel_auto_pay(&from, &rule_id); let result = client.try_cancel_auto_pay(&from, &rule_id); - assert!( - matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::AutoPayNotFound as u32) - ), - "expected AutoPayNotFound on double-cancel, got: {:?}", - result - ); + assert_eq!(result, Err(Ok(EscrowError::AutoPayNotFound))); } #[test] @@ -1567,14 +1517,7 @@ fn test_cancel_auto_pay_vault_not_found() { let nonexistent_commitment = BytesN::from_array(&env, &[0xFFu8; 32]); let result = client.try_cancel_auto_pay(&nonexistent_commitment, &0u32); - assert!( - matches!( - result, - Err(Ok(err)) if err == Error::from_contract_error(EscrowError::VaultNotFound as u32) - ), - "expected VaultNotFound for nonexistent vault, got: {:?}", - result - ); + assert_eq!(result, Err(Ok(EscrowError::VaultNotFound))); } #[test] diff --git a/onchain/shared/src/errors.rs b/onchain/shared/src/errors.rs index c66c94d..d0e0029 100644 --- a/onchain/shared/src/errors.rs +++ b/onchain/shared/src/errors.rs @@ -63,6 +63,8 @@ pub enum EscrowError { AlreadyInitialized = 2016, /// Self-payment is not allowed (from == to). SelfPaymentNotAllowed = 2017, + /// Arithmetic overflow or underflow occurred. + ArithmeticError = 2018, } #[contracterror] @@ -77,7 +79,7 @@ pub enum FactoryError { #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq)] #[repr(u32)] -pub enum CoreError { +pub enum CoreError { /// The requested resource was not found. NotFound = 4001, /// The SMT root has not been set yet. @@ -94,13 +96,13 @@ pub enum CoreError { Unauthorized = 4007, /// new_owner is the same as the current owner. SameOwner = 4008, - /// initialize() has already been called on this contract instance. - AlreadyInitialized = 4009, - /// Commitment is already registered via register(). - AlreadyRegistered = 4010, - /// The new SMT root matches the existing on-chain root. - RootUnchanged = 4011, -} + /// initialize() has already been called on this contract instance. + AlreadyInitialized = 4009, + /// Commitment is already registered via register(). + AlreadyRegistered = 4010, + /// The new SMT root matches the existing on-chain root. + RootUnchanged = 4011, +} #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq)] diff --git a/onchain/tests/e2e/test_full_flow.rs b/onchain/tests/e2e/test_full_flow.rs index 6e752a0..f3160bc 100644 --- a/onchain/tests/e2e/test_full_flow.rs +++ b/onchain/tests/e2e/test_full_flow.rs @@ -128,7 +128,7 @@ fn e2e_escrow_deposit_schedule_payment() { "[TEST DEBUG] create_vault: contract address = {:?}", env.current_contract_address() ); - EscrowContract::create_vault(env.clone(), hash.clone(), token.clone()); + EscrowContract::create_vault(env.clone(), hash.clone(), token.clone()).unwrap(); }); // Assert vault state exists after creation let state: Option = env.as_contract(&escrow_id, || { @@ -163,7 +163,7 @@ fn e2e_escrow_deposit_schedule_payment() { "[TEST DEBUG] create_vault (recipient): contract address = {:?}", env.current_contract_address() ); - EscrowContract::create_vault(env.clone(), to_hash.clone(), token.clone()); + EscrowContract::create_vault(env.clone(), to_hash.clone(), token.clone()).unwrap(); }); // Schedule a payment to another commitment