From 221b6c8215e493f1bfa6f2c0a0d6328d059991aa Mon Sep 17 00:00:00 2001 From: abbarrh01 Date: Mon, 27 Jul 2026 23:32:11 +0100 Subject: [PATCH 1/3] test(savings-vault): add test for failed withdraw preserving state add a new test case mirroring the existing lock withdrawal test, verifying all vault state remains unchanged when a regular withdraw fails due to SAC token transfer failure --- .../src/test/token_transfer_rollback.rs | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/contracts/savings_vault/src/test/token_transfer_rollback.rs b/contracts/savings_vault/src/test/token_transfer_rollback.rs index 5e8c3bd..2ac7391 100644 --- a/contracts/savings_vault/src/test/token_transfer_rollback.rs +++ b/contracts/savings_vault/src/test/token_transfer_rollback.rs @@ -446,3 +446,85 @@ fn test_failed_withdraw_lock_token_transfer_failure_preserves_state() { assert_eq!(lock.amount, 200, "lock amount should remain unchanged"); assert!(!lock.withdrawn, "lock should not be marked as withdrawn"); } + +#[test] +fn test_failed_withdraw_token_transfer_failure_preserves_state() { + // Mirror of `test_failed_withdraw_lock_token_transfer_failure_preserves_state` + // but for the plain `withdraw` entrypoint. Verifies that when the SAC + // transfer from contract -> user fails (contract custody drained below + // the user's internal available balance), every piece of vault state + // (available balance, locked balance, events, lock entries) is preserved + // exactly as it was before the call. + let env = test_env(); + let (contract_id, client, token_client, token_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + + set_ledger_timestamp(&env, 1_000); + token_admin.mint(&user, &1_000); + + // Deposit + lock to build mixed state so we verify both available and + // locked sides remain untouched by a failed withdrawal. + client.deposit(&user, &800); + let _lock_id = client.lock_funds(&user, &300, &5_000); + + // Internal state before failure: 500 available, 300 locked, 800 total + let (bal_before, locked_before, events_before) = snapshot(&env, &client, &user); + assert_eq!(bal_before, 500); + assert_eq!(locked_before, 300); + + // Also snapshot the lock entry fields to rule out any mutation + let lock_snapshot_before = client.get_lock(&user, &_lock_id).unwrap(); + + // Drain the contract's SAC balance to an unrelated sink address so the + // internal `token_client.transfer(contract -> user)` inside `withdraw` + // will be rejected by the SAC even though the user's internal balance + // is sufficient. `mock_all_auths()` grants the `from.require_auth()` + // check the SAC performs on the contract address itself. + let contract_address = contract_id.clone(); + let contract_balance = token_client.balance(&contract_address); + assert_eq!(contract_balance, 800, "custody = sum of liabilities pre-drain"); + let sink = Address::generate(&env); + env.mock_all_auths(); + token_client.transfer(&contract_address, &sink, &contract_balance); + env.set_auths(&[]); // clear mocks so real user auth is required again + assert_eq!(token_client.balance(&contract_address), 0); + + // Attempt withdraw for an amount covered by the internal balance (so + // the internal check passes) but NOT covered by SAC custody (so the + // transfer must fail and roll back the host call). + let result = client.try_withdraw(&user, &200); + assert!( + result.is_err(), + "withdraw must fail when contract has insufficient SAC custody" + ); + + // Zero state drift: available balance, locked balance, event count + let (bal_after, locked_after, events_after) = snapshot(&env, &client, &user); + assert_eq!( + bal_after, bal_before, + "available balance must not change on failed withdraw SAC transfer" + ); + assert_eq!( + locked_after, locked_before, + "locked balance must not change on failed withdraw SAC transfer" + ); + assert_eq!( + events_after, events_before, + "no new events must be emitted on failed withdraw SAC transfer" + ); + + // Lock entry byte-identical to snapshot (no field drift) + let lock_snapshot_after = client.get_lock(&user, &_lock_id).unwrap(); + assert_eq!( + lock_snapshot_after.amount, lock_snapshot_before.amount, + "lock amount unchanged after failed withdraw" + ); + assert_eq!( + lock_snapshot_after.unlock_time, lock_snapshot_before.unlock_time, + "lock unlock_time unchanged after failed withdraw" + ); + assert_eq!( + lock_snapshot_after.withdrawn, lock_snapshot_before.withdrawn, + "lock withdrawn flag unchanged after failed withdraw" + ); +} From 867cb1e143e2a9fe80f80cff0fc24fb45c83f381 Mon Sep 17 00:00:00 2001 From: abbarrh01 Date: Mon, 27 Jul 2026 23:55:36 +0100 Subject: [PATCH 2/3] test(savings_vault): add error code validation tests and update error docs Add a new `contract_error_codes` test module in the savings vault test suite that validates every ContractError variant panics with its assigned u32 numeric discriminant code. Overhaul the `sdk-error-mapping-guide.md` and `error-codes.md` documentation files to formalize the public error code interface, including categorized error ranges, client-side handling best practices, and reference implementation examples for SDKs and mobile apps. --- contracts/savings_vault/src/lib.rs | 389 ++++++++++----- .../src/test/contract_error_codes.rs | 382 ++++++++++++++ contracts/savings_vault/src/test/mod.rs | 1 + docs/error-codes.md | 466 ++++++++++-------- docs/sdk-error-mapping-guide.md | 339 ++++++++++--- 5 files changed, 1178 insertions(+), 399 deletions(-) create mode 100644 contracts/savings_vault/src/test/contract_error_codes.rs diff --git a/contracts/savings_vault/src/lib.rs b/contracts/savings_vault/src/lib.rs index 8973b54..c1e44b5 100644 --- a/contracts/savings_vault/src/lib.rs +++ b/contracts/savings_vault/src/lib.rs @@ -25,7 +25,8 @@ extern crate alloc; extern crate std; use soroban_sdk::{ - contract, contractimpl, contracttype, log, symbol_short, token, Address, Env, Symbol, Vec, + contract, contracterror, contractimpl, contracttype, log, symbol_short, token, Address, Env, + Symbol, Vec, }; const MAX_LOCK_PAGE_SIZE: u32 = 50; @@ -151,6 +152,117 @@ pub enum DataKey { pub const STORAGE_VERSION: u64 = 1; +// --------------------------------------------------------------------------- +// Error Codes +// --------------------------------------------------------------------------- +/// Structured contract errors exposed via Soroban's `#[contracterror]` +/// mechanism. Every variant maps to a stable `u32` error code that SDK and +/// mobile consumers can rely on for deterministic user-facing messaging +/// and cross-repo compatibility. +/// +/// # Category Ranges +/// +/// | Range | Category | Primary Concern | +/// |-----------|--------------|-------------------------------------------------| +/// | 1000–1099 | Validation | Input argument sanity (sign, magnitude, range) | +/// | 2000–2099 | Authorisation| Role and signature enforcement | +/// | 3000–3099 | Lifecycle | Initialize / pause / storage-version states | +/// | 4000–4099 | Accounting | Balance sufficiency / deposit minimums | +/// | 5000–5099 | Lock | Lock lookup / state / maturity / durations | +/// | 6000–6099 | Storage | Migration / storage layout / unwrap safety | +/// | 7000–7099 | Token | SAC transfer / accepted-token configuration | +/// | 8000–8099 | Admin | Admin rotation rules | +/// +/// The gaps inside each range allow new variants to be added without +/// renumbering existing codes, which would be a breaking change for +/// downstream SDKs. +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +#[repr(u32)] +pub enum ContractError { + // ---- 1000s: Validation ------------------------------------------------ + /// `amount` argument was `0` or negative. `deposit`, `withdraw`, + /// `lock_funds` all require strictly positive amounts. + AmountNotPositive = 1001, + /// Attempted to create or extend a lock with `unlock_time` in the + /// past or equal to the current ledger timestamp. + UnlockTimeNotInFuture = 1002, + /// `lock_funds` duration (`unlock_time - ledger_timestamp`) exceeds + /// the configured `MaxLockDurationSecs`. + LockDurationExceedsMaximum = 1003, + /// `lock_funds` duration (`unlock_time - ledger_timestamp`) is + /// strictly below the configured `MinLockDurationSecs`. + LockDurationBelowMinimum = 1004, + /// `deposit` amount is strictly below the configured + /// `MinDepositAmount` floor. + AmountBelowMinimumDeposit = 1005, + /// `pause(admin, duration_secs)` called with `duration_secs == 0`. + PauseDurationMustBePositive = 1006, + /// `set_min_deposit_amount` called with a negative value. + MinDepositAmountNegative = 1007, + + // ---- 2000s: Authorisation -------------------------------------------- + /// Caller is not the stored admin (failed `assert_admin` check inside + /// an admin-gated function). + NotAuthorizedAdmin = 2001, + + // ---- 3000s: Lifecycle / State ---------------------------------------- + /// `initialize` called a second time on an already-initialized + /// contract. + AlreadyInitialized = 3001, + /// Function requiring `initialize()` to have run was called before + /// initialization completed. + NotInitialized = 3002, + /// Deposit or lock attempted while the emergency pause is active and + /// not yet expired. + ContractPaused = 3003, + + // ---- 4000s: Accounting ------------------------------------------------ + /// `withdraw` or `lock_funds` requested an amount strictly greater + /// than the caller's available `Balance`. + InsufficientBalance = 4001, + /// Semantic twin of `InsufficientBalance` used specifically by + /// `lock_funds` so SDKs can map the two contexts to different copy. + InsufficientBalanceToLock = 4002, + + // ---- 5000s: Locks ---------------------------------------------------- + /// `get_lock`, `withdraw_lock` or `extend_lock` referenced a lock id + /// that does not exist for the given owner. + LockNotFound = 5001, + /// `withdraw_lock` or `extend_lock` called on a lock whose + /// `withdrawn` flag is already true. + LockAlreadyWithdrawn = 5002, + /// `withdraw_lock` attempted before `unlock_time <= ledger_timestamp`. + LockNotMatured = 5003, + /// `extend_lock` attempted with a `new_unlock_time` that does not + /// exceed the lock's current `unlock_time`. + ExtendLockTimeNotIncreased = 5004, + + // ---- 6000s: Storage / Migration -------------------------------------- + /// `try_migrate` read a `StorageVersion` greater than + /// `STORAGE_VERSION` compiled into the running WASM (would be a + /// downgrade with potential data loss – blocked). + StorageVersionUnsupported = 6001, + /// Storage read for an instance key that must always be set after + /// initialization (e.g. `Admin`, `Token`) unexpectedly returned + /// `None`. + RequiredStorageEntryMissing = 6002, + + // ---- 7000s: Token / SAC ---------------------------------------------- + /// Accepted-token configuration missing at transfer time (should + /// never happen after `initialize`; guarded by `assert_initialized` + /// at every public entrypoint that reaches a transfer). + TokenNotConfigured = 7001, + + // ---- 8000s: Admin Rotation ------------------------------------------- + /// `transfer_admin` called with the current admin as new_admin + /// (no-op self-transfer blocked to preserve audit trail). + CannotTransferAdminToSelf = 8001, + /// `transfer_admin` called with the contract's own address as + /// new_admin (tokens and admin access would become unrecoverable). + CannotTransferAdminToContractAddress = 8002, +} + // --------------------------------------------------------------------------- // Contract Definition // --------------------------------------------------------------------------- @@ -164,13 +276,15 @@ impl SavingsVault { // Helpers // ----------------------------------------------------------------------- - fn assert_initialized(env: &Env) { + fn assert_initialized(env: &Env) -> Result<(), ContractError> { if !env.storage().instance().has(&DataKey::Initialized) { - panic!("Contract is not initialized"); + Err(ContractError::NotInitialized) + } else { + Ok(()) } } - fn try_migrate(env: &Env) { + fn try_migrate(env: &Env) -> Result<(), ContractError> { let current_version: u64 = env .storage() .instance() @@ -178,7 +292,7 @@ impl SavingsVault { .unwrap_or(0); if current_version == STORAGE_VERSION { - return; + return Ok(()); } // Migrate from older versions to newer versions incrementally! @@ -196,18 +310,25 @@ impl SavingsVault { "Migrated storage from version 0 to version {}", STORAGE_VERSION ); + Ok(()) } _ => { - // If current version > STORAGE_VERSION, panic to prevent downgrades! - panic!("Unsupported storage version: {}", current_version); + // If current version > STORAGE_VERSION, error to prevent downgrades! + Err(ContractError::StorageVersionUnsupported) } } } - fn assert_admin(env: &Env, admin: &Address) { - let stored_admin: Address = env.storage().instance().get(&DataKey::Admin).unwrap(); + fn assert_admin(env: &Env, admin: &Address) -> Result<(), ContractError> { + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .unwrap_or_else(|| env.error_contract(ContractError::RequiredStorageEntryMissing)); if admin != &stored_admin { - panic!("Not authorized"); + Err(ContractError::NotAuthorizedAdmin) + } else { + Ok(()) } } @@ -238,11 +359,11 @@ impl SavingsVault { /// is automatically cleared so callers do not need to invoke `unpause` /// explicitly after a time-bounded pause expires. /// - /// # Panics + /// # Errors /// - /// Panics with `"Contract is paused"` when the pause is active and has not + /// Returns [`ContractError::ContractPaused`] when the pause is active and has not /// expired. - fn require_not_paused(env: &Env) { + fn require_not_paused(env: &Env) -> Result<(), ContractError> { let paused: bool = env .storage() .instance() @@ -259,20 +380,24 @@ impl SavingsVault { if expiry != 0 && env.ledger().timestamp() >= expiry { env.storage().instance().set(&DataKey::Paused, &false); env.storage().instance().set(&DataKey::PauseExpiry, &0_u64); - return; + return Ok(()); } - panic!("Contract is paused"); + Err(ContractError::ContractPaused) + } else { + Ok(()) } } - fn assert_supported_storage_version(env: &Env) { + fn assert_supported_storage_version(env: &Env) -> Result<(), ContractError> { let stored_version: u64 = env .storage() .instance() .get(&DataKey::StorageVersion) .unwrap_or(0); if stored_version != STORAGE_VERSION { - panic!("Unsupported storage version"); + Err(ContractError::StorageVersionUnsupported) + } else { + Ok(()) } } @@ -281,14 +406,15 @@ impl SavingsVault { // Initialization // ----------------------------------------------------------------------- - /// One-time setup. Records admin and token addresses. Panics if called twice. + /// One-time setup. Records admin and token addresses. Errors with + /// [`ContractError::AlreadyInitialized`] if called twice. pub fn initialize(env: Env, admin: Address, token: Address) { if env.storage().instance().has(&DataKey::Initialized) { - panic!("Contract is already initialized"); + env.error_contract(ContractError::AlreadyInitialized) } // Try migration before initializing - Self::try_migrate(&env); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); // Require the admin to have signed this transaction admin.require_auth(); @@ -324,8 +450,9 @@ impl SavingsVault { pub fn get_version(env: Env) -> soroban_sdk::String { // No need to be initialized for version check, but check storage version if possible if env.storage().instance().has(&DataKey::Initialized) { - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); } soroban_sdk::String::from_str(&env, "0.1.0") } @@ -356,14 +483,18 @@ impl SavingsVault { /// /// No authorization required (read-only operation). /// - /// # Panics + /// # Errors /// - /// - If the contract has not been initialized. + /// - [`ContractError::NotInitialized`] – If the contract has not been initialized. pub fn get_token(env: Env) -> Address { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); - env.storage().instance().get(&DataKey::Token).unwrap() + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); + env.storage() + .instance() + .get(&DataKey::Token) + .unwrap_or_else(|| env.error_contract(ContractError::TokenNotConfigured)) } // ----------------------------------------------------------------------- @@ -404,12 +535,12 @@ impl SavingsVault { /// - If the caller is not the admin /// - If `duration_secs` is zero pub fn pause(env: Env, admin: Address, duration_secs: u64) { - Self::assert_initialized(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); admin.require_auth(); - Self::assert_admin(&env, &admin); + Self::assert_admin(&env, &admin).unwrap_or_else(|e| env.error_contract(e)); if duration_secs == 0 { - panic!("Pause duration must be greater than zero"); + env.error_contract(ContractError::PauseDurationMustBePositive) } let expiry = env.ledger().timestamp() + duration_secs; @@ -461,9 +592,9 @@ impl SavingsVault { /// - If the contract has not been initialized /// - If the caller is not the admin pub fn unpause(env: Env, admin: Address) { - Self::assert_initialized(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); admin.require_auth(); - Self::assert_admin(&env, &admin); + Self::assert_admin(&env, &admin).unwrap_or_else(|e| env.error_contract(e)); env.storage().instance().set(&DataKey::Paused, &false); env.storage().instance().set(&DataKey::PauseExpiry, &0_u64); @@ -487,12 +618,12 @@ impl SavingsVault { /// - If the contract has not been initialized /// - If the caller is not the admin pub fn set_min_deposit_amount(env: Env, admin: Address, min_amount: i128) { - Self::assert_initialized(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); admin.require_auth(); - Self::assert_admin(&env, &admin); + Self::assert_admin(&env, &admin).unwrap_or_else(|e| env.error_contract(e)); if min_amount < 0 { - panic!("Min deposit amount cannot be negative"); + env.error_contract(ContractError::MinDepositAmountNegative) } env.storage() @@ -527,9 +658,9 @@ impl SavingsVault { /// - If the contract has not been initialized /// - If the caller is not the admin pub fn set_max_lock_duration(env: Env, admin: Address, max_duration_secs: u64) { - Self::assert_initialized(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); admin.require_auth(); - Self::assert_admin(&env, &admin); + Self::assert_admin(&env, &admin).unwrap_or_else(|e| env.error_contract(e)); env.storage() .instance() @@ -569,9 +700,9 @@ impl SavingsVault { /// - If the contract has not been initialized /// - If the caller is not the admin pub fn set_min_lock_duration(env: Env, admin: Address, min_duration_secs: u64) { - Self::assert_initialized(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); admin.require_auth(); - Self::assert_admin(&env, &admin); + Self::assert_admin(&env, &admin).unwrap_or_else(|e| env.error_contract(e)); env.storage() .instance() @@ -616,7 +747,7 @@ impl SavingsVault { /// /// No authorization required (read-only operation). pub fn is_paused(env: Env) -> bool { - Self::assert_initialized(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); let paused: bool = env .storage() @@ -646,17 +777,18 @@ impl SavingsVault { // ----------------------------------------------------------------------- /// Transfers tokens from the user into the vault and credits their balance. - /// Panics if amount <= 0. + /// Errors with [`ContractError::AmountNotPositive`] if amount <= 0. pub fn deposit(env: Env, user: Address, amount: i128) { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); - Self::require_not_paused(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); + Self::require_not_paused(&env).unwrap_or_else(|e| env.error_contract(e)); user.require_auth(); if amount <= 0 { - panic!("Amount must be positive"); + env.error_contract(ContractError::AmountNotPositive) } let min_deposit: i128 = env @@ -665,10 +797,14 @@ impl SavingsVault { .get(&DataKey::MinDepositAmount) .unwrap_or(0); if min_deposit > 0 && amount < min_deposit { - panic!("Amount is below the minimum deposit amount"); + env.error_contract(ContractError::AmountBelowMinimumDeposit) } - let token = env.storage().instance().get(&DataKey::Token).unwrap(); + let token = env + .storage() + .instance() + .get(&DataKey::Token) + .unwrap_or_else(|| env.error_contract(ContractError::TokenNotConfigured)); let token_client = token::Client::new(&env, &token); let contract_address = env.current_contract_address(); @@ -704,16 +840,18 @@ impl SavingsVault { /// Withdraws available funds from the user's vault. /// Only touches the deposited balance (not matured locks). - /// Panics if amount <= 0 or exceeds available balance. + /// Errors with [`ContractError::AmountNotPositive`] if amount <= 0 or + /// [`ContractError::InsufficientBalance`] if it exceeds available balance. pub fn withdraw(env: Env, user: Address, amount: i128) { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); user.require_auth(); if amount <= 0 { - panic!("Amount must be positive"); + env.error_contract(ContractError::AmountNotPositive) } let mut current_balance: i128 = env @@ -723,10 +861,14 @@ impl SavingsVault { .unwrap_or(0); if amount > current_balance { - panic!("Insufficient balance"); + env.error_contract(ContractError::InsufficientBalance) } - let token = env.storage().instance().get(&DataKey::Token).unwrap(); + let token = env + .storage() + .instance() + .get(&DataKey::Token) + .unwrap_or_else(|| env.error_contract(ContractError::TokenNotConfigured)); let token_client = token::Client::new(&env, &token); let contract_address = env.current_contract_address(); @@ -752,11 +894,13 @@ impl SavingsVault { } /// Withdraws a specific matured lock entry by its ID. - /// Panics if the lock doesn't exist or hasn't matured. + /// Errors with [`ContractError::LockNotFound`] if the lock doesn't exist or + /// [`ContractError::LockNotMatured`] if it hasn't matured. pub fn withdraw_lock(env: Env, user: Address, lock_id: u64) { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); user.require_auth(); @@ -766,19 +910,23 @@ impl SavingsVault { .get::<_, LockEntry>(&DataKey::Lock(user.clone(), lock_id)) { Some(l) => l, - None => panic!("Lock not found"), + None => env.error_contract(ContractError::LockNotFound), }; if lock.withdrawn { - panic!("Lock already withdrawn"); + env.error_contract(ContractError::LockAlreadyWithdrawn) } let current_time = env.ledger().timestamp(); if current_time < lock.unlock_time { - panic!("Lock has not matured yet"); + env.error_contract(ContractError::LockNotMatured) } - let token = env.storage().instance().get(&DataKey::Token).unwrap(); + let token = env + .storage() + .instance() + .get(&DataKey::Token) + .unwrap_or_else(|| env.error_contract(ContractError::TokenNotConfigured)); let token_client = token::Client::new(&env, &token); let contract_address = env.current_contract_address(); @@ -812,9 +960,10 @@ impl SavingsVault { /// Returns the user's available balance: only the deposited (unlocked) balance. /// Matured locks must be withdrawn via `withdraw_lock`. pub fn get_balance(env: Env, user: Address) -> i128 { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); let deposited_balance: i128 = env .storage() .persistent() @@ -855,9 +1004,10 @@ impl SavingsVault { /// large number of historical locks this may become expensive; consider /// off-chain indexing in that case. pub fn get_balance_snapshot(env: Env, user: Address) -> BalanceSnapshot { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); let unlocked: i128 = env .storage() @@ -929,9 +1079,10 @@ impl SavingsVault { /// Same linear-scan caveat as [`get_balance_snapshot`]. For users with /// a very large number of historical locks, prefer off-chain indexing. pub fn get_lock_summary(env: Env, user: Address) -> LockSummary { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); let next_lock_id: u64 = env .storage() @@ -989,23 +1140,27 @@ impl SavingsVault { // ----------------------------------------------------------------------- /// Locks a portion of the user's available balance until `unlock_time`. - /// Returns the lock ID. Panics if amount <= 0, exceeds balance, or - /// unlock_time is not in the future. + /// Returns the lock ID. Errors with [`ContractError::AmountNotPositive`], + /// [`ContractError::UnlockTimeNotInFuture`], + /// [`ContractError::LockDurationExceedsMaximum`], + /// [`ContractError::LockDurationBelowMinimum`], or + /// [`ContractError::InsufficientBalanceToLock`] on invalid input. pub fn lock_funds(env: Env, user: Address, amount: i128, unlock_time: u64) -> u64 { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); - Self::require_not_paused(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); + Self::require_not_paused(&env).unwrap_or_else(|e| env.error_contract(e)); user.require_auth(); if amount <= 0 { - panic!("Amount must be positive"); + env.error_contract(ContractError::AmountNotPositive) } let current_time = env.ledger().timestamp(); if unlock_time <= current_time { - panic!("Unlock time must be in the future"); + env.error_contract(ContractError::UnlockTimeNotInFuture) } let max_duration: u64 = env @@ -1014,7 +1169,7 @@ impl SavingsVault { .get(&DataKey::MaxLockDurationSecs) .unwrap_or(0); if max_duration > 0 && unlock_time - current_time > max_duration { - panic!("Lock duration exceeds maximum"); + env.error_contract(ContractError::LockDurationExceedsMaximum) } let min_duration: u64 = env @@ -1023,7 +1178,7 @@ impl SavingsVault { .get(&DataKey::MinLockDurationSecs) .unwrap_or(0); if min_duration > 0 && unlock_time - current_time < min_duration { - panic!("Lock duration below minimum"); + env.error_contract(ContractError::LockDurationBelowMinimum) } let mut current_balance: i128 = env @@ -1033,7 +1188,7 @@ impl SavingsVault { .unwrap_or(0); if amount > current_balance { - panic!("Insufficient balance"); + env.error_contract(ContractError::InsufficientBalanceToLock) } let next_id: u64 = env @@ -1121,10 +1276,11 @@ impl SavingsVault { /// - If `new_unlock_time` is not strictly greater than current `lock.unlock_time`. /// - If `new_unlock_time` is not in the future (`<= env.ledger().timestamp()`). pub fn extend_lock(env: Env, user: Address, lock_id: u64, new_unlock_time: u64) { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); - Self::require_not_paused(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); + Self::require_not_paused(&env).unwrap_or_else(|e| env.error_contract(e)); user.require_auth(); @@ -1134,20 +1290,20 @@ impl SavingsVault { .get::<_, LockEntry>(&DataKey::Lock(user.clone(), lock_id)) { Some(l) => l, - None => panic!("Lock not found"), + None => env.error_contract(ContractError::LockNotFound), }; if lock.withdrawn { - panic!("Lock already withdrawn"); + env.error_contract(ContractError::LockAlreadyWithdrawn) } let current_time = env.ledger().timestamp(); if new_unlock_time <= current_time { - panic!("Unlock time must be in the future"); + env.error_contract(ContractError::UnlockTimeNotInFuture) } if new_unlock_time <= lock.unlock_time { - panic!("New unlock time must be strictly greater than current unlock time"); + env.error_contract(ContractError::ExtendLockTimeNotIncreased) } let old_unlock_time = lock.unlock_time; @@ -1175,9 +1331,10 @@ impl SavingsVault { /// (both matured and immature). Matured locks must be withdrawn via /// `withdraw_lock`. pub fn get_locked_balance(env: Env, user: Address) -> i128 { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); let next_lock_id: u64 = env .storage() .persistent() @@ -1201,9 +1358,10 @@ impl SavingsVault { /// Returns true if the user has at least one matured lock. pub fn can_withdraw(env: Env, user: Address) -> bool { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); let next_lock_id: u64 = env .storage() .persistent() @@ -1228,9 +1386,10 @@ impl SavingsVault { /// Returns a single lock entry by ID, or None if not found. pub fn get_lock(env: Env, user: Address, lock_id: u64) -> Option { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); env.storage() .persistent() .get(&DataKey::Lock(user.clone(), lock_id)) @@ -1238,9 +1397,10 @@ impl SavingsVault { /// Returns a paginated list of lock entries for a user (oldest first). pub fn list_locks(env: Env, user: Address, offset: u32, limit: u32) -> Vec { - Self::assert_initialized(&env); - Self::try_migrate(&env); - Self::assert_supported_storage_version(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::try_migrate(&env).unwrap_or_else(|e| env.error_contract(e)); + Self::assert_supported_storage_version(&env) + .unwrap_or_else(|e| env.error_contract(e)); let next_lock_id: u64 = env .storage() .persistent() @@ -1278,25 +1438,32 @@ impl SavingsVault { /// Returns the admin address set during initialization. pub fn get_admin(env: Env) -> Address { - Self::assert_initialized(&env); - env.storage().instance().get(&DataKey::Admin).unwrap() + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); + env.storage() + .instance() + .get(&DataKey::Admin) + .unwrap_or_else(|| env.error_contract(ContractError::RequiredStorageEntryMissing)) } /// Transfers admin privileges to a new address. Only the current admin /// can call this. pub fn transfer_admin(env: Env, admin: Address, new_admin: Address) { - Self::assert_initialized(&env); + Self::assert_initialized(&env).unwrap_or_else(|e| env.error_contract(e)); admin.require_auth(); - Self::assert_admin(&env, &admin); + Self::assert_admin(&env, &admin).unwrap_or_else(|e| env.error_contract(e)); if admin == new_admin { - panic!("Invalid new admin: cannot transfer to self"); + env.error_contract(ContractError::CannotTransferAdminToSelf) } if new_admin == env.current_contract_address() { - panic!("Invalid new admin: cannot set contract address as admin"); + env.error_contract(ContractError::CannotTransferAdminToContractAddress) } - let old_admin: Address = env.storage().instance().get(&DataKey::Admin).unwrap(); + let old_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .unwrap_or_else(|| env.error_contract(ContractError::RequiredStorageEntryMissing)); env.storage().instance().set(&DataKey::Admin, &new_admin); let topics = (symbol_short!("xferadmin"), old_admin.clone()); diff --git a/contracts/savings_vault/src/test/contract_error_codes.rs b/contracts/savings_vault/src/test/contract_error_codes.rs new file mode 100644 index 0000000..798e64b --- /dev/null +++ b/contracts/savings_vault/src/test/contract_error_codes.rs @@ -0,0 +1,382 @@ +//! Structured contract error code tests. +//! +//! Verifies the full `ContractError` enum surface exposed by the contract via +//! `env.error_contract(variant)`. Each test exercises a single failure path +//! and asserts the panic emitted by the Soroban test harness contains the +//! exact numeric `u32` code matching the variant's `#[repr(u32)]` discriminant. +//! +//! SDK and mobile consumers map these same numeric codes to localized user +//! messages; this suite therefore doubles as a cross-repo compatibility +//! contract: any code change here is a BREAKING change for callers. + +use super::*; +use soroban_sdk::testutils::{Address as _, Events}; +use soroban_sdk::IntoVal; + +use crate::ContractError; +use test_helpers::*; + +/// Helper: wrap a fallible client call in `std::panic::catch_unwind` and +/// extract the panic payload as a `String`. All error-code tests assert on +/// substrings of this payload, which in the soroban-sdk `testutils` harness +/// contains the `Status(ContractError, CODE)` diagnostic where `CODE` is the +/// u32 from the enum's `#[repr(u32)]`. +fn catch_panic_message(f: F) -> String { + match std::panic::catch_unwind(f) { + Ok(_) => panic!("expected a panic but the call succeeded"), + Err(payload) => { + if let Some(s) = payload.downcast_ref::<&str>() { + (*s).to_string() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + format!("{:?}", payload) + } + } + } +} + +// ========================================================================= +// CATEGORY 1000: Validation +// ========================================================================= + +#[test] +fn error_code_1001_amount_not_positive_on_deposit() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.deposit(&user, &0)); + assert!( + msg.contains(&ContractError::AmountNotPositive as u32.to_string()) + || msg.contains("AmountNotPositive"), + "panic payload must reference error code 1001 AmountNotPositive; got: {}", + msg + ); +} + +#[test] +fn error_code_1001_amount_not_positive_on_withdraw() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.withdraw(&user, &-1)); + assert!( + msg.contains(&ContractError::AmountNotPositive as u32.to_string()) + || msg.contains("AmountNotPositive"), + "panic payload must reference error code 1001 AmountNotPositive; got: {}", + msg + ); +} + +#[test] +fn error_code_1001_amount_not_positive_on_lock() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let user = Address::generate(&env); + set_ledger_timestamp(&env, 1_000); + let msg = catch_panic_message(|| client.lock_funds(&user, &0, &5_000)); + assert!( + msg.contains(&ContractError::AmountNotPositive as u32.to_string()) + || msg.contains("AmountNotPositive"), + "panic payload must reference error code 1001; got: {}", + msg + ); +} + +#[test] +fn error_code_1002_unlock_time_not_in_future_lock_funds() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let user = Address::generate(&env); + set_ledger_timestamp(&env, 1_000); + let msg = catch_panic_message(|| client.lock_funds(&user, &50, &1_000)); + assert!( + msg.contains(&ContractError::UnlockTimeNotInFuture as u32.to_string()) + || msg.contains("UnlockTimeNotInFuture"), + "panic payload must reference error code 1002; got: {}", + msg + ); +} + +#[test] +fn error_code_1003_lock_duration_exceeds_maximum() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + set_ledger_timestamp(&env, 1_000); + client.set_max_lock_duration(&token_admin, &1_000); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.lock_funds(&user, &10, &10_000)); + assert!( + msg.contains(&ContractError::LockDurationExceedsMaximum as u32.to_string()) + || msg.contains("LockDurationExceedsMaximum"), + "panic payload must reference error code 1003; got: {}", + msg + ); +} + +#[test] +fn error_code_1004_lock_duration_below_minimum() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + set_ledger_timestamp(&env, 1_000); + client.set_min_lock_duration(&token_admin, &5_000); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.lock_funds(&user, &10, &2_000)); + assert!( + msg.contains(&ContractError::LockDurationBelowMinimum as u32.to_string()) + || msg.contains("LockDurationBelowMinimum"), + "panic payload must reference error code 1004; got: {}", + msg + ); +} + +#[test] +fn error_code_1005_amount_below_minimum_deposit() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + client.set_min_deposit_amount(&token_admin, &1_000); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.deposit(&user, &50)); + assert!( + msg.contains(&ContractError::AmountBelowMinimumDeposit as u32.to_string()) + || msg.contains("AmountBelowMinimumDeposit"), + "panic payload must reference error code 1005; got: {}", + msg + ); +} + +#[test] +fn error_code_1006_pause_duration_must_be_positive() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + let msg = catch_panic_message(|| client.pause(&token_admin, &0)); + assert!( + msg.contains(&ContractError::PauseDurationMustBePositive as u32.to_string()) + || msg.contains("PauseDurationMustBePositive"), + "panic payload must reference error code 1006; got: {}", + msg + ); +} + +#[test] +fn error_code_1007_min_deposit_amount_negative() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + let msg = catch_panic_message(|| client.set_min_deposit_amount(&token_admin, &-1)); + assert!( + msg.contains(&ContractError::MinDepositAmountNegative as u32.to_string()) + || msg.contains("MinDepositAmountNegative"), + "panic payload must reference error code 1007; got: {}", + msg + ); +} + +// ========================================================================= +// CATEGORY 2000: Authorisation +// ========================================================================= + +#[test] +fn error_code_2001_not_authorized_admin_wrong_caller() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let rando = Address::generate(&env); + let msg = catch_panic_message(|| client.unpause(&rando)); + assert!( + msg.contains(&ContractError::NotAuthorizedAdmin as u32.to_string()) + || msg.contains("NotAuthorizedAdmin"), + "panic payload must reference error code 2001; got: {}", + msg + ); +} + +// ========================================================================= +// CATEGORY 3000: Lifecycle +// ========================================================================= + +#[test] +fn error_code_3001_already_initialized() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + let other_token = Address::generate(&env); + let msg = catch_panic_message(|| client.initialize(&token_admin, &other_token)); + assert!( + msg.contains(&ContractError::AlreadyInitialized as u32.to_string()) + || msg.contains("AlreadyInitialized"), + "panic payload must reference error code 3001; got: {}", + msg + ); +} + +#[test] +fn error_code_3002_not_initialized_before_deposit() { + let env = test_env(); + let contract_id = env.register(SavingsVault, ()); + let client = SavingsVaultClient::new(&env, &contract_id); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.deposit(&user, &100)); + assert!( + msg.contains(&ContractError::NotInitialized as u32.to_string()) + || msg.contains("NotInitialized"), + "panic payload must reference error code 3002; got: {}", + msg + ); +} + +#[test] +fn error_code_3003_contract_paused_blocks_deposit() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + client.pause(&token_admin, &60); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.deposit(&user, &100)); + assert!( + msg.contains(&ContractError::ContractPaused as u32.to_string()) + || msg.contains("ContractPaused"), + "panic payload must reference error code 3003; got: {}", + msg + ); +} + +// ========================================================================= +// CATEGORY 4000: Accounting +// ========================================================================= + +#[test] +fn error_code_4001_insufficient_balance_withdraw() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.withdraw(&user, &999_999)); + assert!( + msg.contains(&ContractError::InsufficientBalance as u32.to_string()) + || msg.contains("InsufficientBalance"), + "panic payload must reference error code 4001; got: {}", + msg + ); +} + +#[test] +fn error_code_4002_insufficient_balance_to_lock() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let user = Address::generate(&env); + set_ledger_timestamp(&env, 1_000); + let msg = catch_panic_message(|| client.lock_funds(&user, &9_999, &5_000)); + assert!( + msg.contains(&ContractError::InsufficientBalanceToLock as u32.to_string()) + || msg.contains("InsufficientBalanceToLock"), + "panic payload must reference error code 4002; got: {}", + msg + ); +} + +// ========================================================================= +// CATEGORY 5000: Lock +// ========================================================================= + +#[test] +fn error_code_5001_lock_not_found_on_withdraw() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _) = vault_with_sac(&env); + let user = Address::generate(&env); + let msg = catch_panic_message(|| client.withdraw_lock(&user, &1337)); + assert!( + msg.contains(&ContractError::LockNotFound as u32.to_string()) + || msg.contains("LockNotFound"), + "panic payload must reference error code 5001; got: {}", + msg + ); +} + +#[test] +fn error_code_5002_lock_already_withdrawn() { + let env = test_env(); + let (contract_id, client, _token_client, _token_admin, asset_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + set_ledger_timestamp(&env, 1_000); + asset_admin.mint(&user, &10_000); + env.mock_all_auths(); + client.deposit(&user, &5_000); + let id = client.lock_funds(&user, &2_000, &2_000); + set_ledger_timestamp(&env, 10_000); + client.withdraw_lock(&user, &id); + env.set_auths(&[]); + let msg = catch_panic_message(|| client.withdraw_lock(&user, &id)); + assert!( + msg.contains(&ContractError::LockAlreadyWithdrawn as u32.to_string()) + || msg.contains("LockAlreadyWithdrawn"), + "panic payload must reference error code 5002; got: {}", + msg + ); +} + +#[test] +fn error_code_5003_lock_not_matured() { + let env = test_env(); + let (contract_id, client, _token_client, _token_admin, asset_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + set_ledger_timestamp(&env, 1_000); + asset_admin.mint(&user, &10_000); + env.mock_all_auths(); + client.deposit(&user, &5_000); + let id = client.lock_funds(&user, &2_000, &9_999_999); + env.set_auths(&[]); + let msg = catch_panic_message(|| client.withdraw_lock(&user, &id)); + assert!( + msg.contains(&ContractError::LockNotMatured as u32.to_string()) + || msg.contains("LockNotMatured"), + "panic payload must reference error code 5003; got: {}", + msg + ); +} + +#[test] +fn error_code_5004_extend_lock_time_not_increased() { + let env = test_env(); + let (contract_id, client, _token_client, _token_admin, asset_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + set_ledger_timestamp(&env, 1_000); + asset_admin.mint(&user, &10_000); + env.mock_all_auths(); + client.deposit(&user, &5_000); + let id = client.lock_funds(&user, &2_000, &20_000); + env.set_auths(&[]); + let msg = catch_panic_message(|| client.extend_lock(&user, &id, &15_000)); + assert!( + msg.contains(&ContractError::ExtendLockTimeNotIncreased as u32.to_string()) + || msg.contains("ExtendLockTimeNotIncreased"), + "panic payload must reference error code 5004; got: {}", + msg + ); +} + +// ========================================================================= +// CATEGORY 8000: Admin Rotation +// ========================================================================= + +#[test] +fn error_code_8001_cannot_transfer_admin_to_self() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + let same = token_admin.clone(); + let msg = catch_panic_message(|| client.transfer_admin(&token_admin, &same)); + assert!( + msg.contains(&ContractError::CannotTransferAdminToSelf as u32.to_string()) + || msg.contains("CannotTransferAdminToSelf"), + "panic payload must reference error code 8001; got: {}", + msg + ); +} + +#[test] +fn error_code_8002_cannot_transfer_admin_to_contract_address() { + let env = test_env(); + let (contract_id, client, _token_client, token_admin, _) = vault_with_sac(&env); + let msg = catch_panic_message(|| client.transfer_admin(&token_admin, &contract_id)); + assert!( + msg.contains(&ContractError::CannotTransferAdminToContractAddress as u32.to_string()) + || msg.contains("CannotTransferAdminToContractAddress"), + "panic payload must reference error code 8002; got: {}", + msg + ); +} diff --git a/contracts/savings_vault/src/test/mod.rs b/contracts/savings_vault/src/test/mod.rs index a2098d2..40ae23e 100644 --- a/contracts/savings_vault/src/test/mod.rs +++ b/contracts/savings_vault/src/test/mod.rs @@ -7,6 +7,7 @@ mod admin_rotation; mod balance_conservation; mod balance_snapshot; mod config_read_helpers; +mod contract_error_codes; mod event_compatibility; mod event_ordering; mod independent_lock_creation; diff --git a/docs/error-codes.md b/docs/error-codes.md index 102550a..910be91 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -1,216 +1,262 @@ -# Savings Vault Error Reference - -This reference describes error behavior in `contracts/savings_vault/src/lib.rs`. -The contract defines a custom error enum with stable numeric error codes via -the `#[contracterror]` attribute. See [error-code-standard.md](./error-code-standard.md) -for the complete error code standard and SDK mapping guidance. - -SDK and mobile callers should use the numeric error codes for reliable error -handling and display user-friendly messages based on the error category. - -## Configuration errors (1000-1999) - -### `AlreadyInitialized` (Code: 1001) - -- **Current failure:** Returns `ContractError::AlreadyInitialized` from `initialize`. -- **Meaning:** The one-time initialization flag already exists. -- **Likely cause:** A repeated initialization, a retry after success, or the - wrong contract ID. -- **Caller/developer action:** Do not retry. Confirm the contract ID and use the - existing deployment; this is not a transient network failure. - -### `NotInitialized` (Code: 1002) - -- **Current failure:** Returns `ContractError::NotInitialized` when attempting operations - that require initialization. -- **Meaning:** The contract has not been initialized. -- **Likely cause:** Operations called before initialization or instance storage unavailable. -- **Caller/developer action:** Ensure initialization succeeded before enabling - vault operations. - -## Validation errors (2000-2999) - -### `InvalidDepositAmount` (Code: 2001) - -- **Current failure:** Returns `ContractError::InvalidDepositAmount` from `deposit`. -- **Meaning:** The deposit amount is zero or negative. -- **Likely cause:** Invalid input, unit conversion, sign handling, or an empty - field converted to zero. -- **Caller/developer action:** Require a positive `i128` amount in the token's - smallest unit before invoking the contract. - -### `InvalidWithdrawAmount` (Code: 2002) - -- **Current failure:** Returns `ContractError::InvalidWithdrawAmount` from `withdraw`. -- **Meaning:** The withdrawal amount is zero or negative. -- **Likely cause:** Invalid input or an amount-conversion bug. -- **Caller/developer action:** Reject non-positive amounts before submission. - -### `InvalidLockAmount` (Code: 2003) - -- **Current failure:** Returns `ContractError::InvalidLockAmount` from `lock_funds`. -- **Meaning:** The lock amount is zero or negative. -- **Likely cause:** Invalid input or an amount-conversion bug. -- **Caller/developer action:** Require a positive amount before submission. - -### `InvalidUnlockTime` (Code: 2004) - -- **Current failure:** Returns `ContractError::InvalidUnlockTime` from `lock_funds`. -- **Meaning:** `unlock_time` is less than or equal to the current ledger - timestamp; it must be strictly later when executed. -- **Likely cause:** A past timestamp, seconds/milliseconds confusion, clock skew, - or submission too close to the selected time. -- **Caller/developer action:** Send Unix time in **seconds** and leave a safety - margin beyond the latest ledger time. - -## Balance errors (4000-4999) - -These checks use the vault's **available internal balance**, not the wallet -balance or locked balance. - -### `InsufficientBalance` (Code: 4001) - -- **Current failure:** Returns `ContractError::InsufficientBalance` from `withdraw`. -- **Meaning:** The withdrawal exceeds the available internal balance; a missing - balance is treated as zero. -- **Likely cause:** The request is too large, no deposit is recorded, or some - balance was moved to the locked bucket. -- **Caller/developer action:** Refresh `get_balance(user)`, cap the request to - that value, and explain that locked funds are unavailable. - -### `InsufficientBalanceToLock` (Code: 4002) - -- **Current failure:** Returns `ContractError::InsufficientBalanceToLock` from `lock_funds`. -- **Meaning:** The lock amount exceeds the available internal balance. -- **Likely cause:** A stale displayed balance, an excessive request, or funds - already moved to the locked bucket. -- **Caller/developer action:** Refresh `get_balance(user)` and allow no more than - the returned available amount. - -### `FundsLockedUntilMaturity` (Code: 4003) - -- **Current failure:** Returns `ContractError::FundsLockedUntilMaturity` from `withdraw`. -- **Meaning:** The withdrawal amount exceeds the available balance and would - require withdrawing from immature (unmatured) locked funds. This is a specific - error that occurs when the user has locked funds that have not yet reached - their unlock time. -- **Likely cause:** The user attempted to withdraw more than their available - (unlocked) balance, and the shortfall would need to come from locked funds - that are still immature (current_time < unlock_time). -- **Caller/developer action:** Check `get_balance(user)` to see available funds - and `get_locked_balance(user)` to see locked funds. Only withdraw up to the - available balance. Wait for locks to mature (check with `can_withdraw(user)`) - before attempting to withdraw locked funds. - -## Authorization errors (3000-3999) - -### `Unauthorized` (Code: 3001) - -- **Current failure:** Soroban host authorization failure from `require_auth()`; - the contract defines this error for documentation purposes, but the actual - failure comes from the Soroban host. -- **Meaning:** Valid authorization for the required address is absent. -- **Likely cause:** `initialize` lacks `admin` authorization, or `deposit`, - `withdraw`, or `lock_funds` lacks `user` authorization. The app may be trying - to act for another address. -- **Caller/developer action:** Build and sign with the required address. Do not - retry unchanged; request the correct wallet signature. - -Read-only calls (`get_balance`, `get_locked_balance`, and `can_withdraw`) do not -call `require_auth()`. - -## Lock and unlock time behavior - -**Zero-duration locks:** Passing `unlock_time == current ledger timestamp` -(a zero-second duration) is rejected with this same panic, because the check -is `unlock_time <= current_time`, not `<`. There is no way to create a lock -that is already matured at creation time; the smallest valid duration is one -second (`unlock_time == current_time + 1`), and funds locked that way remain -locked until the ledger timestamp advances to that value — `can_withdraw` -and `get_balance` still treat it as locked at the moment of creation. - -### `Contract is paused` - -- **Current failure:** Panic message from `deposit` and `lock_funds`. -- **Meaning:** The contract is in an emergency pause state. Deposits and lock - operations are blocked. Withdrawals (`withdraw`, `withdraw_lock`) and - read-only queries remain available. -- **Likely cause:** The admin activated a pause for an incident response. -- **Caller/developer action:** Check `is_paused()` to confirm. If the pause - has an expiry, wait for it to expire. Otherwise, the admin must call - `unpause()` to restore normal operations. Users can still withdraw funds - during a pause. - -### `Pause duration must be greater than zero` - -- **Current failure:** Panic message from `pause`. -- **Meaning:** The `duration_secs` argument to `pause()` was zero. A pause - must have a non-zero duration to ensure it auto-expires. -- **Likely cause:** Invalid input or an accidental zero value. -- **Caller/developer action:** Pass a positive duration in seconds (e.g., - 604800 for 7 days). - -### Locked funds are not yet withdrawable - -- **Current condition:** `can_withdraw(user)` returns `false`; it does not fail. -- **Meaning:** No locked funds exist, or the ledger timestamp is earlier than - the unlock time. At exactly the unlock timestamp it returns `true`. -- **Likely cause:** The lock has not matured or no lock exists. -- **Caller/developer action:** Treat `false` as normal state and disable the - action. The current contract has no operation to release or withdraw locked - funds; `can_withdraw` is only a query. - -### `Cannot withdraw: funds are locked until maturity` - -- **Current failure:** Panic message from `withdraw`. -- **Meaning:** The withdrawal amount exceeds the available balance and would - require withdrawing from immature (unmatured) locked funds. This is a specific - error that occurs when the user has locked funds that have not yet reached - their unlock time. -- **Likely cause:** The user attempted to withdraw more than their available - (unlocked) balance, and the shortfall would need to come from locked funds - that are still immature (current_time < unlock_time). -- **Caller/developer action:** Check `get_balance(user)` to see available funds - and `get_locked_balance(user)` to see locked funds. Only withdraw up to the - available balance. Wait for locks to mature (check with `can_withdraw(user)`) - before attempting to withdraw locked funds. - -## Unauthorised access errors - -### `LockNotFound` (Code: 5001) - -- **Current failure:** Reserved for future use. -- **Meaning:** No lock found for the specified lock ID. -- **Likely cause:** Invalid lock ID or lock has been consumed. -- **Caller/developer action:** Verify lock ID and check lock status. - -Read-only calls (`get_balance`, `get_locked_balance`, `get_lock`, `list_locks`, -and `can_withdraw`) do not call `require_auth()`. - -## Other existing failure conditions - -### Token transfer failure during withdrawal - -- **Current failure:** Error or trap propagated by the configured token - contract; the vault defines no wrapper error. -- **Meaning:** The token transfer from the vault contract to the user failed. -- **Likely cause:** Insufficient real token balance, an invalid or incompatible - token address, token authorization failure, or token-contract rejection. An - internal balance does not guarantee matching tokens are held. -- **Caller/developer action:** Inspect the nested token diagnostic. Verify the - configured token and vault token balance; do not label this only as an - internal-balance error. +# Savings Vault Error Reference (Canonical) + +This reference documents the **real, contract-defined `ContractError` enum** +in [`contracts/savings_vault/src/lib.rs`](../contracts/savings_vault/src/lib.rs). +The contract uses `#[contracterror]` with a `#[repr(u32)]` discriminant, so +errors are exposed to SDKs and mobile clients as **stable `u32` codes** — not +as arbitrary panic strings. + +> **Breaking-change contract.** Numeric codes in this file are part of the +> cross-repo SDK interface. Any code renumber here MUST be version-bumped and +> co-ordinated with SDK + mobile releases. See the mapping guidance in +> [`sdk-error-mapping-guide.md`](./sdk-error-mapping-guide.md). + +## Category Ranges + +| Range | Category | Examples | +| --- | --- | --- | +| **1001–1099** | **Validation** | Bad amounts, bad timestamps, bad durations | +| **2001–2099** | **Authorisation** | Wrong caller role (non-admin calling admin-only) | +| **3001–3099** | **Lifecycle** | Initialization, pause state | +| **4001–4099** | **Accounting** | Insufficient available balance | +| **5001–5099** | **Locks** | Missing lock, already withdrawn, immature, unchanged extend | +| **6001–6099** | **Storage** | Schema version, missing required entry | +| **7001–7099** | **Token** | Token configuration issues | +| **8001–8099** | **Admin rotation** | Invalid new-admin address | + +## 1000s — Validation + +### `AmountNotPositive` (1001) + +- **Raised by:** `deposit`, `withdraw`, `lock_funds` +- **Meaning:** The submitted `amount` is `0` or negative. +- **Likely cause:** Empty field coerced to `0`, a sign bug, or a unit-conversion + bug between whole-token and stroops / the asset's decimal exponent. +- **Caller action:** Block submission client-side until `amount > 0`. Format the + error to show the asset's decimal representation, not the raw `i128` stroop + value. + +### `UnlockTimeNotInFuture` (1002) + +- **Raised by:** `lock_funds`, `extend_lock` +- **Meaning:** `unlock_time <= current ledger timestamp`. +- **Likely cause:** Past time, seconds / ms confusion, clock skew, or choosing a + time too close to submission. +- **Caller action:** Send Unix time in **seconds** and leave a safety margin + (≥ 30 s) above the last-seen ledger time. + +### `LockDurationExceedsMaximum` (1003) + +- **Raised by:** `lock_funds` +- **Meaning:** `unlock_time - now > max_lock_duration`. +- **Likely cause:** Wrong asset config read, or a UI picker allowing years + beyond the configured max. +- **Caller action:** Read `MaxLockDuration` from config (or its storage-backed + setter) and clamp the picker before submission. + +### `LockDurationBelowMinimum` (1004) + +- **Raised by:** `lock_funds` +- **Meaning:** `unlock_time - now < min_lock_duration`. +- **Likely cause:** UX allowing 1-second locks when min is e.g. 1 day. +- **Caller action:** Same as above — pre-clamp. + +### `AmountBelowMinimumDeposit` (1005) + +- **Raised by:** `deposit` +- **Meaning:** `amount < min_deposit_amount`. +- **Likely cause:** Asset decimal mismatch, or a UX not honouring the configured + floor. +- **Caller action:** Read the configured min and reject below it client-side. + +### `PauseDurationMustBePositive` (1006) + +- **Raised by:** `pause` (admin) +- **Meaning:** `duration_secs == 0` on the admin pause call. +- **Likely cause:** Accidental zero input. +- **Caller action:** Admin UI only; enforce a minimum (e.g. 1 hour) when + submitting. + +### `MinDepositAmountNegative` (1007) + +- **Raised by:** `set_min_deposit_amount` (admin) +- **Meaning:** Attempt to set the global min deposit to a negative `i128`. +- **Likely cause:** Admin-console sign bug. +- **Caller action:** Admin console validation. + +## 2000s — Authorisation + +### `NotAuthorizedAdmin` (2001) + +- **Raised by:** all admin-only entrypoints (`pause`, `unpause`, + `set_min_deposit_amount`, `set_max_lock_duration`, `set_min_lock_duration`, + `transfer_admin`) +- **Meaning:** The `require_auth`-verified caller does not match the stored + `Admin`. +- **Likely cause:** Wrong wallet connected, or trying to use a governance role + that was never transferred to. +- **Caller action:** Confirm the connected address is the current admin (use + `get_admin()`), or — for app backends — route through a signer that holds + the admin role. +- **Distinction from host auth failures:** Soroban's host-level auth failures + (e.g. signature missing) raise a host `Status(Auth, …)`; this code is a + contract-level **role check** that runs AFTER the host has confirmed the + caller signed. + +## 3000s — Lifecycle + +### `AlreadyInitialized` (3001) + +- **Raised by:** `initialize` +- **Meaning:** The `StorageVersion` flag already exists. +- **Likely cause:** A double-call to `initialize` (e.g. an infra retry after a + success that the caller didn't observe) or pointing at the wrong deployed + contract. +- **Caller action:** Do not retry. Use `get_version()` / `get_token()` to + confirm the contract is already live. + +### `NotInitialized` (3002) + +- **Raised by:** every guarded public entrypoint (`get_version`, `get_token`, + `pause`, `deposit`, `withdraw`, `lock_funds`, `list_locks`, `get_admin`, …) +- **Meaning:** The contract was deployed but `initialize` hasn't run. +- **Likely cause:** A deploy script that forgot the init call, or a race where + the UI renders operations before init lands on-chain. +- **Caller action:** Gate all vault UI behind a contract-ready check + (`get_version()` succeeds). + +### `ContractPaused` (3003) + +- **Raised by:** state-mutating non-withdrawal operations (`deposit`, + `lock_funds`, `extend_lock`, plus admin-setters if the team later chooses + to tighten them). `withdraw`, `withdraw_lock`, and reads remain **allowed**. +- **Meaning:** Emergency pause active and not yet expired. +- **Likely cause:** Admin-incident response. +- **Caller action:** Show an incident banner. Allow / encourage withdrawal; + block new deposits and lock creation. Use `is_paused()` to poll expiry. + +## 4000s — Accounting + +### `InsufficientBalance` (4001) + +- **Raised by:** `withdraw` +- **Meaning:** `amount > available_balance(user)`. Locked funds are NOT in + `available_balance`. +- **Likely cause:** Stale displayed balance, or the user is trying to withdraw + more than their unlocked funds. +- **Caller action:** Call `get_balance(user)` first, cap the submit, and show + "Locked balance is unavailable. Wait for it to mature with `can_withdraw`." + +### `InsufficientBalanceToLock` (4002) + +- **Raised by:** `lock_funds` +- **Meaning:** `amount > available_balance(user)`. +- **Semantic disambiguation from 4001:** Same underlying test, different + operation. SDKs SHOULD show a distinct copy: + - 4001 → "You don't have enough available to withdraw **X** tokens." + - 4002 → "You don't have enough unlocked balance to lock **X** tokens." + +## 5000s — Locks + +### `LockNotFound` (5001) + +- **Raised by:** `withdraw_lock`, `extend_lock` +- **Meaning:** No `Lock { amount, unlock_time, withdrawn }` stored under the + `DataKey::Lock(owner, id)` key. +- **Likely cause:** Stale lock ID, lock storage TTL expired and was not + bumped, or wrong owner (the lookup is scoped to the authenticated owner). +- **Caller action:** Re-fetch via `get_lock` / `list_locks`. If TTL expiry is + plausible, check storage TTL tooling. + +### `LockAlreadyWithdrawn` (5002) + +- **Raised by:** `withdraw_lock`, `extend_lock` +- **Meaning:** The lock's `withdrawn` boolean is `true`. +- **Likely cause:** UI double-submit after a success the user didn't see, or a + retry of a completed transaction. +- **Caller action:** Idempotent on the client side: if `get_lock(owner, id)` + says `withdrawn == true`, treat as success and don't re-submit. + +### `LockNotMatured` (5003) + +- **Raised by:** `withdraw_lock` +- **Meaning:** `now < lock.unlock_time`. +- **Likely cause:** UI enabled the "withdraw lock" button too early due to + local-clock skew, or the user manually forced the call. +- **Caller action:** Gate the button behind `can_withdraw(user)` AND a + per-lock `now >= unlock_time` check using the last-seen ledger timestamp, + not local device time. + +### `ExtendLockTimeNotIncreased` (5004) + +- **Raised by:** `extend_lock` +- **Meaning:** `new_unlock_time <= lock.unlock_time`. +- **Likely cause:** UX that lets the user pick an earlier time when "extending". +- **Caller action:** Pre-clamp to `max(lock.unlock_time + 1, selection)`. + +## 6000s — Storage + +### `StorageVersionUnsupported` (6001) + +- **Raised by:** `try_migrate`, `assert_supported_storage_version` +- **Meaning:** On-chain storage has a version strictly greater than the code's + `CURRENT_STORAGE_VERSION` (downgrade / rollback attempt), OR migration code + cannot interpret the stored layout. +- **Likely cause:** Wrong WASM deployed (older version vs. newer storage), or + a failed upgrade path. +- **Caller action:** Escalate to contract deployment owners; do NOT auto-retry. + +### `RequiredStorageEntryMissing` (6002) + +- **Raised by:** paths that `.unwrap_or_else(|| RequiredStorageEntryMissing)` + a required instance-storage cell (e.g. `Admin`, `Token`). +- **Meaning:** Contract storage is internally inconsistent (a mandatory + singleton was never written or was dropped by an accidental TTL expiry). +- **Likely cause:** Deployment bug: `initialize` failed half-way, or an + admin-rotation code path omitted the write. In principle unreachable on a + correctly-initialized contract; presence of 6002 is ALERTS-level. +- **Caller action:** Pause the UI. Page on-call. + +## 7000s — Token + +### `TokenNotConfigured` (7001) + +- **Raised by:** SAC-custody paths if the `Token` storage cell is missing + (`deposit`, `withdraw`, `withdraw_lock`). +- **Meaning:** The configured asset address is not set; custody transfers can't + run. +- **Likely cause:** Corrupt initialization or migration. +- **Caller action:** Escalate. + +## 8000s — Admin Rotation + +### `CannotTransferAdminToSelf` (8001) + +- **Raised by:** `transfer_admin` +- **Meaning:** `new_admin == current_admin`. +- **Likely cause:** Form submitted with the same address. +- **Caller action:** Admin console validation; treat as no-op success if the + user intention was to "keep admin". + +### `CannotTransferAdminToContractAddress` (8002) + +- **Raised by:** `transfer_admin` +- **Meaning:** `new_admin == contract_address` (i.e. setting the vault itself + as its own admin, which would permanently orphan admin-only operations). +- **Likely cause:** Paste error selecting the contract instead of the signer. +- **Caller action:** Admin console guard that blacklists the contract's own + address. ## Error Code Stability -All error codes defined in the `ContractError` enum are stable and backward compatible: - -- Existing error codes will never change -- New error codes will be added within their category ranges -- Deprecated error codes will be marked in documentation but remain functional -- See [error-code-standard.md](./error-code-standard.md) for the complete standard +- Existing codes **will never be renumbered** within a major release line. +- New codes **will be added inside their category range** (1001–1099, …) to + keep SDK route-by-thousand-category logic correct. +- Deprecated codes **will keep their number**; deprecation is signalled via + variant docs only. +- See [`error-code-standard.md`](./error-code-standard.md) for design rationale. ## SDK Integration -For SDK mapping guidance and mobile UX recommendations, see -[error-code-standard.md](./error-code-standard.md). +For how to map these `u32` codes into TypeScript / Kotlin / Swift user-facing +messages + analytics, see [`sdk-error-mapping-guide.md`](./sdk-error-mapping-guide.md). diff --git a/docs/sdk-error-mapping-guide.md b/docs/sdk-error-mapping-guide.md index f33d8e4..4455371 100644 --- a/docs/sdk-error-mapping-guide.md +++ b/docs/sdk-error-mapping-guide.md @@ -1,114 +1,297 @@ -# SDK Error Mapping Guide +# Savings Vault — SDK / Mobile Error Mapping Guide -This guide maps savings vault contract errors to expected SDK-level handling. -It is intended for SDK and mobile app developers integrating with the vault -contract on testnet. +This guide is for SDK + mobile developers integrating against the Savings +Vault contract. The contract now exposes a **stable `u32` error code surface** +via `#[contracterror] #[repr(u32)]` defined in +[`contracts/savings_vault/src/lib.rs`](../contracts/savings_vault/src/lib.rs). -> **Not production-ready.** This contract is for educational and testnet use. -> Error behavior may change before any mainnet deployment. +> **Cross-repo compatibility contract.** These `u32` codes are the **ONLY** +> supported interface for error branching on the client side. Panic message +> text may change between soroban-sdk upgrades — never regex-match panic +> strings in production code. Match on numeric codes instead. -## How errors surface +## 1. How errors surface -The contract uses Rust `panic!()` messages for validation failures and -Soroban host-level traps for authorization and token errors. There are no -stable numeric error codes. The SDK should **not** branch on panic message -text; instead, treat any failed invocation as a general failure and surface a -user-friendly message while retaining the diagnostic for debugging. +When the contract calls `env.error_contract(ContractError::Variant)`: -A failed invocation does **not** commit that invocation's state changes. +1. The Soroban host traps with a `HostError` of kind `ContractError`. +2. The `u32` discriminant from the enum (`#[repr(u32)]`) is the **stable + identifier** of the error. +3. Off-chain SDKs receive this `u32` via the transaction result. -## Error categories +A failed invocation commits **no state changes** from that invocation (Soroban +rollback is atomic); SDKs do not need to reconcile partial writes. -| Category | Contract errors | SDK action | -|---|---|---| -| Already initialized | `"Contract is already initialized"` | Show "Vault is already set up." Do not retry. | -| Not initialized | Storage unwrap trap (no message) | Show "Vault is not initialized." Prompt admin to run `initialize`. | -| Invalid amount | `"Deposit amount must be greater than zero"`, `"Withdrawal amount must be greater than zero"`, `"Lock amount must be greater than zero"` | Validate amount > 0 before submitting. Show "Please enter a valid amount." | -| Insufficient balance | `"Insufficient balance"`, `"Insufficient balance to lock"` | Refresh `get_balance` and compare. Show "Not enough available funds." | -| Invalid unlock time | `"Unlock time must be in the future"` | Validate unlock_time > current ledger timestamp before submitting. Show "Unlock time must be in the future." | -| Unauthorized | Soroban host auth failure (no contract message) | Request the correct wallet signature. Show "Transaction not authorized." | -| Token transfer failure | Error from the configured token contract | Inspect nested diagnostic. Show "Transfer failed." Verify token address and vault token balance. | +### How to extract the code -## User-facing examples +Pseudo-code for a `try_*` contract client call: -These are the messages the mobile app should show for each failure. +```ts +// TypeScript / stellar-sdk v12+ +import { Contract } from '@stellar/stellar-sdk'; -| Trigger | User message | -|---|---| -| Re-initialization | "Vault is already set up." | -| Deposit/withdraw/lock with zero or negative amount | "Please enter a valid amount." | -| Withdraw more than available | "Not enough available funds." | -| Lock more than available | "Not enough available funds." | -| Unlock time in the past | "Unlock time must be in the future." | -| Missing wallet signature | "Transaction not authorized." | -| Token transfer error | "Transfer failed. Please try again." | -| Vault not initialized | "Vault is not initialized." | +const contract = new Contract(vaultAddress); +const sim = await txBuilder.call(contract.call('deposit', user, amount)).simulate(); +if (sim.error) { + const code = extractContractErrorCode(sim.error); // see below + await handleVaultError(code, context); +} +``` -## Developer-facing examples +Use a `code`-extract helper that walks the diagnostic until it finds the +`ContractError` status. For the `soroban-sdk`/`@stellar/stellar-sdk` variants, +the pattern is: "Status with ContractError type whose payload is the u32". +Keep this helper versioned per host SDK, but keep `handleVaultError(code)` +stable — that is the cross-repo contract. -### Pre-flight validation +## 2. Category routing (by 1000-group) -Validate inputs before submitting to avoid unnecessary round trips: +Branch SDK logic in two layers: (1) **category route** by `Math.floor(code / 1000)`, +then (2) **specific copy / analytics** by exact `code`. This way future codes +added inside a category (e.g. 1008, 1009, ...) will still route to the right +top-level UX bucket. -```rust -// Reject non-positive amounts before invocation -if amount <= 0 { - return Err(SdkError::InvalidAmount); +| `code / 1000` | Category | UX bucket | +| --- | --- | --- | +| `1` | Validation | Show inline field error next to the bad input | +| `2` | Authorisation (contract-level role check) | Show "You're not the vault admin."; route admin-only views away | +| `3` | Lifecycle | Show a banner (pause / not-initialized / already-setup) | +| `4` | Accounting | Show `get_balance(user)` and cap the submit | +| `5` | Locks | Show lock-specific copy; re-fetch lock list | +| `6` | Storage | Page on-call; show a rare "Vault configuration issue" | +| `7` | Token | Same as 6xxx — asset config issue | +| `8` | Admin rotation | Admin console form validation | + +**Host auth failures** (missing signature, wrong signer) raise a host-level +`Status(Auth, …)`, not 2001. Distinguish these: + +```ts +switch (categoryOf(err)) { + case 'HostAuth': return promptWalletSignature(); // host Auth + case 2: return showNotVaultAdmin(); // 2001 role check + … } +``` + +## 3. Per-code SDK map (canonical) + +Columns: + +- **`code`** — the stable `u32`. +- **Trigger method** — which SDK call raised it; use to scope the copy. +- **User copy** — what mobile shows the end user. Interpolate `{amount}` / + `{asset}` using the token's stored decimals (never raw `i128` stroops). +- **Analytics event** — cross-repo deterministic event name for amplitude / + mixpanel / firebase. +- **SDK retry?** — `NEVER` = do not auto-retry (user-action or logic bug), + `PAGE-ONCALL` = alert the deployment-owners, `UI-GATED` = the SDK should + have blocked it before submission. + +| code | Trigger | User copy | Analytics | Retry? | +| --- | --- | --- | --- | --- | +| 1001 | `deposit` | "Enter an amount greater than 0 {asset}." | `vault.err.1001.deposit` | UI-GATED | +| 1001 | `withdraw` | "Enter an amount greater than 0 {asset}." | `vault.err.1001.withdraw` | UI-GATED | +| 1001 | `lock_funds` | "Enter an amount greater than 0 {asset}." | `vault.err.1001.lock` | UI-GATED | +| 1002 | `lock_funds` | "Unlock time must be later than right now." | `vault.err.1002.lock` | UI-GATED | +| 1002 | `extend_lock` | "New unlock time must be later than the current one." | `vault.err.1002.extend` | UI-GATED | +| 1003 | `lock_funds` | "Lock duration exceeds the maximum allowed." | `vault.err.1003` | UI-GATED | +| 1004 | `lock_funds` | "Lock duration is below the minimum allowed." | `vault.err.1004` | UI-GATED | +| 1005 | `deposit` | "Minimum deposit is {min} {asset}." | `vault.err.1005` | UI-GATED | +| 1006 | `pause` (admin) | "Pause duration must be greater than 0 seconds." | `vault.err.1006` | UI-GATED | +| 1007 | `set_min_deposit_amount` (admin) | "Minimum deposit can't be negative." | `vault.err.1007` | UI-GATED | +| 2001 | any admin entrypoint | "This action requires the vault admin role." | `vault.err.2001.{method}` | NEVER | +| 3001 | `initialize` | "The vault is already set up." | `vault.err.3001` | NEVER | +| 3002 | any guarded method | "The vault is still being set up. Try again in a moment." | `vault.err.3002.{method}` | NEVER (poll `get_version`) | +| 3003 | `deposit`,`lock_funds`,`extend_lock` | "Vault is paused for an incident. Deposits and locks are blocked. You can still withdraw." | `vault.err.3003.{method}` | NEVER (poll `is_paused`) | +| 4001 | `withdraw` | "Insufficient available balance. You can withdraw up to {available} {asset}. Locked funds are unavailable until they mature." | `vault.err.4001` | NEVER | +| 4002 | `lock_funds` | "Insufficient unlocked balance. You can lock up to {available} {asset}." | `vault.err.4002` | NEVER | +| 5001 | `withdraw_lock` / `extend_lock` | "We couldn't find that lock. Refresh and try again." | `vault.err.5001.{method}` | NEVER (refetch list) | +| 5002 | `withdraw_lock` / `extend_lock` | "This lock has already been withdrawn." | `vault.err.5002.{method}` | NEVER (treat as success) | +| 5003 | `withdraw_lock` | "This lock hasn't matured yet. Check back at {unlockTimeISO}." | `vault.err.5003` | NEVER | +| 5004 | `extend_lock` | "New unlock time must be later than the current unlock time." | `vault.err.5004` | UI-GATED | +| 6001 | any guarded method | "A vault storage version mismatch was detected. Please contact support." | `vault.err.6001.{method}` | PAGE-ONCALL | +| 6002 | any guarded method | "A vault storage entry is missing. Please contact support." | `vault.err.6002.{method}` | PAGE-ONCALL | +| 7001 | `deposit` / `withdraw` / `withdraw_lock` | "The vault's token is misconfigured. Please contact support." | `vault.err.7001.{method}` | PAGE-ONCALL | +| 8001 | `transfer_admin` (admin) | "New admin address must be different from the current admin." | `vault.err.8001` | NEVER | +| 8002 | `transfer_admin` (admin) | "The vault cannot be its own admin." | `vault.err.8002` | NEVER | + +## 4. Reference SDK implementation (TypeScript) + +```ts +// sdk/src/vault/errors.ts +export const VaultError = { + AmountNotPositive: 1001, + UnlockTimeNotInFuture: 1002, + LockDurationExceedsMaximum: 1003, + LockDurationBelowMinimum: 1004, + AmountBelowMinimumDeposit: 1005, + PauseDurationMustBePositive: 1006, + MinDepositAmountNegative: 1007, + + NotAuthorizedAdmin: 2001, + + AlreadyInitialized: 3001, + NotInitialized: 3002, + ContractPaused: 3003, + + InsufficientBalance: 4001, + InsufficientBalanceToLock: 4002, + + LockNotFound: 5001, + LockAlreadyWithdrawn: 5002, + LockNotMatured: 5003, + ExtendLockTimeNotIncreased: 5004, + + StorageVersionUnsupported: 6001, + RequiredStorageEntryMissing: 6002, + + TokenNotConfigured: 7001, -// Reject past unlock times -if unlock_time <= current_ledger_timestamp { - return Err(SdkError::InvalidUnlockTime); + CannotTransferAdminToSelf: 8001, + CannotTransferAdminToContractAddress: 8002, +} as const; + +export type VaultErrorCode = (typeof VaultError)[keyof typeof VaultError]; + +export function isContractError(code: number): code is VaultErrorCode { + return Object.values(VaultError).includes(code as VaultErrorCode); +} + +export function categoryOf(code: number): number { + return Math.floor(code / 1000); } ``` -### Balance check before withdraw or lock +Handler skeleton: -```rust -let available = contract.get_balance(user.clone()); -if amount > available { - return Err(SdkError::InsufficientBalance); +```ts +// sdk/src/vault/onError.ts +import { VaultError, categoryOf } from './errors'; + +export async function handleVaultError( + code: number, + ctx: { method: string; asset: { decimals: number; symbol: string } }, +): Promise { + switch (categoryOf(code)) { + case 1: return renderInlineValidation(code, ctx); + case 2: return renderRoleBanner(); + case 3: return renderLifecycleBanner(code, ctx); + case 4: return renderBalanceCallout(code, ctx); + case 5: return refetchLockList(code, ctx); + case 6: + case 7: + pageOncall(`vault.err.${code}`, ctx.method); + return renderSupportBanner(); + case 8: return renderAdminFormError(code); + default: + if (isContractError(code)) { + log.warn(`unknown canonical code ${code}; forward to fallback`); + } + return renderGenericFallback(); + } } ``` -### General error handling +## 5. Reference SDK implementation (Rust client) + +```rust +// crates/vault-sdk/src/err.rs +use soroban_sdk::contracterror; -Since the contract has no stable error codes, catch failures generically: +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +#[repr(u32)] +pub enum VaultError { + AmountNotPositive = 1001, + UnlockTimeNotInFuture = 1002, + // … keep in 1:1 lockstep with the on-chain enum +} + +impl VaultError { + pub fn from_u32(code: u32) -> Option { + use VaultError::*; + Some(match code { + 1001 => AmountNotPositive, + 1002 => UnlockTimeNotInFuture, + 1003 => LockDurationExceedsMaximum, + 1004 => LockDurationBelowMinimum, + 1005 => AmountBelowMinimumDeposit, + 1006 => PauseDurationMustBePositive, + 1007 => MinDepositAmountNegative, + 2001 => NotAuthorizedAdmin, + 3001 => AlreadyInitialized, + 3002 => NotInitialized, + 3003 => ContractPaused, + 4001 => InsufficientBalance, + 4002 => InsufficientBalanceToLock, + 5001 => LockNotFound, + 5002 => LockAlreadyWithdrawn, + 5003 => LockNotMatured, + 5004 => ExtendLockTimeNotIncreased, + 6001 => StorageVersionUnsupported, + 6002 => RequiredStorageEntryMissing, + 7001 => TokenNotConfigured, + 8001 => CannotTransferAdminToSelf, + 8002 => CannotTransferAdminToContractAddress, + _ => return None, + }) + } +} +``` + +Usage against a `try_*` client: ```rust -match contract.try_deposit(user.clone(), amount) { - Ok(_) => show_success("Deposit confirmed."), - Err(_) => show_error("Something went wrong. Please try again."), +match client.try_withdraw(&user, &amount) { + Ok(res) => Ok(res?), + Err(soroban_client::Error::Contract(code)) => match VaultError::from_u32(code) { + Some(VaultError::InsufficientBalance) => { + let available = client.get_balance(&user)?; + Err(AppError::WithdrawUpTo(available)) + } + Some(other) => Err(AppError::Vault(other, method)), + None => Err(AppError::UnknownContractCode(code, method)), + }, + Err(other) => Err(AppError::Transport(other)), } ``` -For production-quality SDKs, log the full diagnostic (including any panic -message or host error) for debugging while showing the user a generic -friendly message. +## 6. Pre-flight validation (recommended) -### Authorization flow +Doing these client-side saves a round trip AND prevents 1001–1007 / 5004 / +8001–8002 from ever hitting the chain. They are still listed on-chain as a +belt-and-suspenders check, but SDKs should treat them as UI bugs when they +reach the contract. -The SDK must ensure the user signs the transaction before submission. If the -contract returns an authorization error, the SDK should prompt for a wallet -signature rather than retrying silently: +```ts +// sdk/src/vault/preflight.ts +export function validateDeposit(amountBI: bigint, cfg: { minDepositBI: bigint }) { + if (amountBI <= 0n) return VaultError.AmountNotPositive; + if (amountBI < cfg.minDepositBI) return VaultError.AmountBelowMinimumDeposit; + return null; +} -```rust -match contract.try_withdraw(user.clone(), amount) { - Ok(tx_hash) => show_success("Withdrawal confirmed."), - Err(SdkError::Unauthorized) => prompt_wallet_signature(), - Err(SdkError::InsufficientBalance) => { - let available = contract.get_balance(user.clone()); - show_error(format!("You can withdraw up to {}.", available)); - } - Err(e) => show_error("Something went wrong."), +export function validateLock( + amountBI: bigint, + unlockTimeS: number, + ledgerTimeS: number, + cfg: { minLockDurationS: number; maxLockDurationS: number; availableBI: bigint }, +) { + if (amountBI <= 0n) return VaultError.AmountNotPositive; + if (amountBI > cfg.availableBI) return VaultError.InsufficientBalanceToLock; + if (unlockTimeS <= ledgerTimeS) return VaultError.UnlockTimeNotInFuture; + const d = unlockTimeS - ledgerTimeS; + if (d < cfg.minLockDurationS) return VaultError.LockDurationBelowMinimum; + if (d > cfg.maxLockDurationS) return VaultError.LockDurationExceedsMaximum; + return null; } ``` -## Further reading +## 7. Further reading -- [Contract Error Reference](error-codes.md) — full list of current failure - conditions -- [SDK Sequence Diagrams](sdk-contract-sequence.md) — interaction flows - including error paths -- [Architecture Documentation](architecture.md) — storage model and contract - structure +- [`error-codes.md`](./error-codes.md) — per-code canonical meaning and + contract caller action. +- [`error-code-standard.md`](./error-code-standard.md) — design rationale, + stability guarantees, versioning rules for adding new codes. +- [`sdk-contract-sequence.md`](./sdk-contract-sequence.md) — flows with error + paths drawn in. From 7c1bf74b9bec8feb26eb5324d3b1ef96789a404d Mon Sep 17 00:00:00 2001 From: abbarrh01 Date: Tue, 28 Jul 2026 00:47:38 +0100 Subject: [PATCH 3/3] docs(analysis): add codebase analysis and reorganize audit docs Create new codebase analysis documents covering repository structure, workflows, and quality/debt assessment. Restructure comprehensive-analysis.md as the main entry point for the analysis package. Update the audit evidence index with the new storage audit map entry, remove the duplicate old vault storage audit map document, and update the last updated date for the audit documentation. --- docs/audit-evidence-index.md | 3 +- docs/codebase-analysis/quality-and-debt.md | 340 +++++++++++++++ .../repo-map-and-workflows.md | 374 ++++++++++++++++ docs/comprehensive-analysis.md | 411 +++++++++--------- docs/storage-audit.md | 139 +++--- docs/vault_storage_audit_map.md | 36 -- 6 files changed, 977 insertions(+), 326 deletions(-) create mode 100644 docs/codebase-analysis/quality-and-debt.md create mode 100644 docs/codebase-analysis/repo-map-and-workflows.md delete mode 100644 docs/vault_storage_audit_map.md diff --git a/docs/audit-evidence-index.md b/docs/audit-evidence-index.md index 7abcc43..de048d2 100644 --- a/docs/audit-evidence-index.md +++ b/docs/audit-evidence-index.md @@ -9,6 +9,7 @@ This index organizes security-relevant documentation for external audit review. | [Contract Specification](contract-specification.md) | Formal specification of the PocketPay Savings Vault contract | ✅ Complete | | [API Reference](api-reference.md) | Public API documentation with error codes and behavior | ✅ Complete | | [Storage Layout](storage-layout.md) | Storage key structure and access patterns | ✅ Complete | +| [Storage Audit Map](storage-audit.md) | Comprehensive audit of storage keys and mutation trace | ✅ Complete | ## Invariants and Safety Properties @@ -82,6 +83,6 @@ This index organizes security-relevant documentation for external audit review. --- -**Last Updated**: 2026-01-27 +**Last Updated**: 2026-07-28 **Maintained by**: Core Development Team **Review Cadence**: Updated with each release diff --git a/docs/codebase-analysis/quality-and-debt.md b/docs/codebase-analysis/quality-and-debt.md new file mode 100644 index 0000000..b2cbd98 --- /dev/null +++ b/docs/codebase-analysis/quality-and-debt.md @@ -0,0 +1,340 @@ +# Quality And Debt Assessment + +This document assesses the codebase's quality, operational readiness, and +remaining technical debt based on the current repository state. + +## 1. Overall Assessment + +The repository is in a strong **development/testnet** state, but not in a +fully production-ready state. + +### Major strengths + +- Clear business domain: token-backed savings vault with time locks +- Stable typed error model using `#[contracterror]` +- Consistent auth boundaries on mutating paths +- Good event coverage across major state transitions +- Strong local Rust test coverage, including property-based invariants +- Explicit storage-versioning support and migration hook +- Good separation between global config and per-user persistent state + +### Major weaknesses + +- Runtime implementation remains concentrated in a single large `lib.rs` +- Several docs no longer match the actual code +- Automation/CI is too thin for the size of the test and docs surface +- Some read paths scale linearly with historical lock count +- Storage TTL management depends on operations outside the contract +- Mainnet governance and upgrade posture remain unresolved + +## 2. Validation And Tooling Posture + +### Verified during this analysis + +- `cargo test` completed successfully from the workspace root +- The registered Rust suite covers initialization, auth, accounting, pause, + events, storage versioning, rollback behavior, and property-based invariants + +### Tooling/process concerns + +- The only GitHub workflow in the repo triggers an external automation dispatch. +- There is no in-repo CI workflow for: + - `cargo test` + - `cargo fmt --check` + - `cargo clippy` + - WASM build validation +- The local `Makefile` only provides: + - `build-release` + - `wasm-size` +- Root docs still mention targets such as `make test`, `make build-wasm`, and + `make clean`, but those targets do not exist. + +## 3. Code Quality Findings + +### 3.1 Monolithic implementation file + +**Observation** + +`contracts/savings_vault/src/lib.rs` contains the full runtime implementation: +state types, storage keys, errors, helpers, admin logic, user flows, and read +models. + +**Impact** + +- Harder navigation for new contributors +- Higher chance of merge conflicts +- More difficult targeted review when changing one concern + +**Risk level** + +Medium maintainability risk, low immediate correctness risk. + +### 3.2 Documentation drift + +**Observation** + +Multiple docs still describe old behavior or abandoned designs. + +### Main drift patterns observed + +- Panic-string-only errors are described even though `ContractError` now exists. +- Several docs still describe `DataKey::Locks(user)` as the main storage model. +- Some docs still mention missing pause/event support although both are present. +- Root/project structure docs still reference `test.rs` instead of `src/test/`. +- Some doc links and absolute file references point to old local machine paths. + +**Impact** + +- Misleads auditors and new maintainers +- Makes architectural review slower than necessary +- Increases chance of implementation mistakes based on outdated guidance + +**Risk level** + +High maintainability/documentation risk. + +### 3.3 Orphaned TypeScript test + +**Observation** + +`tests/atomicity/transfer-atomicity.test.ts` references services under +`src/services/...`, but the repository contains no matching Node project, +package manifest, or service implementation. + +**Impact** + +- Confusing for maintainers +- Creates a false impression of broader cross-language coverage +- Adds noise during codebase comprehension + +**Risk level** + +Low runtime risk, medium maintainability risk. + +## 4. Performance Assessment + +### 4.1 Linear scans over historical lock ids + +**Observation** + +Several read and write paths iterate `1..next_lock_id`: + +- `get_balance_snapshot` +- `get_lock_summary` +- `get_locked_balance` +- `can_withdraw` +- `load_locks` +- `lock_funds` when recomputing `new_locked` for event payload + +**Impact** + +- Cost grows with the number of locks a user has ever created +- Historical withdrawn locks still contribute to iteration cost +- Heavy users can become expensive to query on-chain + +**Risk level** + +Medium-to-high performance risk if per-user lock count grows substantially. + +### 4.2 No lock-count cap + +**Observation** + +`list_locks` caps page size at 50, but the contract does not cap the total +number of locks a user may create. + +**Impact** + +- Read helpers remain exposed to unbounded historical growth +- Gas/resource consumption can become uneven across accounts + +**Risk level** + +Medium performance and anti-DoS risk. + +### 4.3 Event payload recomputation cost + +**Observation** + +After creating a lock, `lock_funds` recomputes the user's immature locked total +by scanning every historical lock id in order to emit `new_locked`. + +**Impact** + +- Extra work on every lock creation +- Event convenience comes at the cost of growing write-time complexity + +**Risk level** + +Medium performance inefficiency, not a correctness bug. + +## 5. Security Assessment + +### 5.1 Strong areas + +- User-facing mutations require `user.require_auth()` +- Admin-facing mutations require both signature and stored-admin match +- Withdrawals remain open during emergency pause +- Deposit/withdraw/withdraw_lock rollback behavior is tested +- Stable contract error codes improve SDK-side handling and analytics +- Token custody invariants are backed by property tests + +### 5.2 Main security risks and trust assumptions + +#### Single-admin control + +- One admin address controls pause, unpause, config setters, and admin transfer +- Fine for testnet/development +- Weak for mainnet governance without multi-sig or role separation + +#### TTL expiry risk + +- Persistent entries rely on Soroban TTL mechanics +- If operators fail to extend TTL, storage can disappear while SAC-held funds + still exist at the contract address +- This is primarily an operational safety risk, not a logic bug + +#### Token-behavior assumption + +- The contract assumes the configured token behaves like a normal SAC +- Fee-on-transfer or policy-heavy token behavior could break 1:1 accounting + +#### No upgrade path + +- Logic is effectively immutable once deployed +- Storage migration exists, but it is not a logic-upgrade framework + +#### No external audit + +- The repo explicitly targets development/testnet use +- No third-party audit artifact is published in the repository + +### 5.3 Policy gap worth clarifying + +**Observation** + +Admin-configured `MinLockDurationSecs` and `MaxLockDurationSecs` are enforced in +`lock_funds`, but `extend_lock` only checks: + +- lock exists +- lock is not withdrawn +- new time is in the future +- new time is strictly greater than the current unlock time + +**Why this matters** + +If the intended policy is "all active lock durations must respect current admin +limits", then `extend_lock` currently bypasses that policy. If the intended +policy is "limits apply only at creation time", the docs should state that more +explicitly. + +**Risk level** + +Medium policy/requirements risk. This is not automatically a bug, but it is a +behavioral ambiguity that should be resolved. + +## 6. Maintainability Assessment + +### 6.1 Dead or transitional storage concepts + +`DataKey::Locks(Address)` still exists, but live lock state is keyed by +`DataKey::Lock(user, lock_id)`. + +**Consequences** + +- Readers must mentally separate historical design from current design +- Docs easily drift when they rely on the obsolete vector model + +### 6.2 Large documentation surface + +The `docs/` directory is extensive and valuable, but it has become hard to keep +fully synchronized with the code. + +**Consequences** + +- Excellent breadth of topic coverage +- Elevated long-term maintenance cost +- Higher chance of contradictory statements across documents + +### 6.3 Mixed sources of truth + +For many topics, the actual source of truth is currently split between: + +- `lib.rs` +- test modules +- newer docs such as `error-codes.md` +- older docs that still describe superseded behavior + +That makes onboarding harder than it needs to be. + +## 7. Technical Debt Inventory + +### High priority debt + +1. Sync stale docs to the current contract implementation +2. Add real CI for test/lint/build verification +3. Decide mainnet posture for admin governance and upgrades +4. Clarify TTL operational ownership and failure handling + +### Medium priority debt + +1. Break `lib.rs` into internal modules without changing the public interface +2. Remove or formally deprecate `DataKey::Locks(Address)` +3. Clarify whether lock duration limits should apply to `extend_lock` +4. Revisit lock-scan costs for high-history users +5. Remove or relocate the orphaned TypeScript test + +### Low priority debt + +1. Expand task-runner convenience targets if docs continue to reference them +2. Consolidate overlapping docs with similar subject matter + +## 8. Recommended Next Steps + +### Short term + +1. Make `docs/comprehensive-analysis.md` and this package the canonical review entry point +2. Update stale high-traffic docs: + - `README.md` + - `architecture.md` + - `storage-audit.md` + - `SECURITY_REVIEW.md` + - any docs still centered on `Locks(user)` or panic strings +3. Add a GitHub Actions workflow for fmt, clippy, test, and release build + +### Medium term + +1. Split runtime code into internal modules such as: + - `state.rs` + - `errors.rs` + - `admin.rs` + - `funds.rs` + - `locks.rs` + - `reads.rs` +2. Decide whether lock-duration config should constrain lock extension +3. Introduce either: + - a lock-count cap, or + - more scalable aggregate accounting for large lock histories + +### Long term + +1. Choose a real mainnet governance model +2. Choose an upgrade or migration strategy +3. Add a formal operational runbook for TTL maintenance and custody monitoring +4. Pursue an external security audit after the above are stabilized + +## 9. Bottom Line + +The codebase demonstrates thoughtful contract design and unusually strong local +test coverage for a project of this size. Its main weaknesses are not basic +logic hygiene, but rather: + +- scaling characteristics of lock-history reads, +- incomplete operational hardening, +- governance/upgrade gaps for serious deployment, +- and a large amount of documentation drift created by rapid iteration. + +For development, learning, and testnet usage, the repository is in good shape. +For production or auditor handoff, the highest-return work is now on process, +documentation synchronization, and operational hardening rather than on +fundamental feature completeness. diff --git a/docs/codebase-analysis/repo-map-and-workflows.md b/docs/codebase-analysis/repo-map-and-workflows.md new file mode 100644 index 0000000..00970c7 --- /dev/null +++ b/docs/codebase-analysis/repo-map-and-workflows.md @@ -0,0 +1,374 @@ +# Repo Map And Workflows + +This document maps the repository's real implementation structure, the contract +surface area, and the principal runtime workflows. + +## 1. Repository Topology + +```text +pocketpay-contracts/ +|- Cargo.toml # Workspace manifest +|- Cargo.lock +|- README.md +|- Makefile # Minimal build/size targets +|- .env.example # Deployment/runtime variable examples +|- .github/workflows/ +| \- trigger-auto-merge.yml # PR automation only, not CI validation +|- contracts/ +| \- savings_vault/ +| |- Cargo.toml # Contract crate manifest +| |- README.md +| |- src/ +| | |- lib.rs # Entire contract implementation +| | \- test/ # Registered Rust unit/property tests +| |- proptest-regressions/ # Saved failing seeds for property tests +| \- test_snapshots/ # Snapshot artifacts for event/schema tests +|- docs/ # Architecture, security, testing, ops docs +|- scripts/ +| |- deploy-testnet.sh +| \- report-wasm-size.sh +\- tests/ + \- atomicity/ + \- transfer-atomicity.test.ts +``` + +## 2. Workspace And Dependency Layout + +### Workspace + +- Root `Cargo.toml` defines a workspace over `contracts/*`. +- The workspace uses release settings optimized for small WASM output: + - `opt-level = "z"` + - `lto = true` + - `panic = "abort"` + - `codegen-units = 1` + +### Contract Crate + +- Crate name: `savings-vault` +- Edition: Rust 2021 +- Output type: `cdylib` +- Runtime dependency: `soroban-sdk` +- Dev dependencies: + - `soroban-sdk` with `testutils` + - `proptest` + +## 3. Core Modules And Responsibilities + +Although the runtime code is in a single file, it is conceptually split into +distinct subsystems. + +### `contracts/savings_vault/src/lib.rs` + +#### State types + +- `LockEntry` + - Canonical per-lock record + - Tracks owner, amount, creation time, unlock time, and withdrawn state +- `BalanceSnapshot` + - Read model for unlocked/locked/total/withdrawable balances +- `LockSummary` + - Read model for counts, totals, and unlock-time ranges + +#### Storage schema + +- `DataKey` + - Enumerates every storage key used by the contract + - Separates global instance storage from user-specific persistent storage + +#### Error model + +- `ContractError` + - Stable `u32` error taxonomy grouped by category ranges: + - 1000s validation + - 2000s authorization + - 3000s lifecycle + - 4000s accounting + - 5000s lock handling + - 6000s storage/migration + - 7000s token + - 8000s admin rotation + +#### Internal helpers + +- Initialization guard +- Storage migration and version checks +- Admin identity validation +- Pause-state enforcement with lazy auto-unpause +- Historical lock reconstruction helper + +#### Public entrypoints + +- Lifecycle and metadata +- Pause and config administration +- User deposit/withdrawal flows +- Lock creation, extension, and withdrawal +- Read-only helpers for balances and locks +- Admin rotation + +## 4. Test Suite Structure + +The registered Rust test suite lives under `contracts/savings_vault/src/test/` +and is orchestrated by `mod.rs`. + +### Main categories + +- Initialization and storage versioning +- Auth and unauthorized access +- Balance conservation and isolation +- Lock lifecycle and replay protection +- Pause behavior and pause-state reads +- Event schema and event ordering +- Token-backed custody and rollback +- Property-based invariant testing + +### Important support assets + +- `test_helpers.rs` + - Shared fixture creation + - Token setup helpers + - Ledger timestamp control + - Mocked-auth and strict-auth environments +- `test_snapshots/` + - Event and behavior snapshots used as regression fixtures +- `proptest-regressions/` + - Persisted failing seeds for reproducing property-test issues + +## 5. On-Chain Storage Schema + +This repository has no SQL or document database. The storage schema is the +Soroban `DataKey` enum. + +### Instance storage keys + +| Key | Purpose | +| --- | --- | +| `Admin` | Current admin address | +| `Initialized` | One-time initialization guard | +| `Token` | Configured SAC token address | +| `StorageVersion` | Current storage version marker | +| `Paused` | Emergency pause flag | +| `PauseExpiry` | Timestamp when pause auto-expires | +| `MinDepositAmount` | Global deposit floor | +| `MaxLockDurationSecs` | Global max lock duration | +| `MinLockDurationSecs` | Global min lock duration | + +### Persistent storage keys + +| Key | Purpose | +| --- | --- | +| `Balance(Address)` | User's available balance | +| `Lock(Address, u64)` | User lock record keyed by owner and lock id | +| `NextLockId(Address)` | Next lock id counter for a user | + +### Transitional / confusing key + +| Key | Status | +| --- | --- | +| `Locks(Address)` | Declared in `DataKey` but not used as the primary live storage model | + +That unused variant is part of the repo's technical debt because many older docs +still describe it as the active schema. + +## 6. Contract Public Interface + +This is the effective API surface of the repository. + +### Lifecycle and metadata + +- `initialize(env, admin, token)` +- `get_version(env)` +- `get_token(env)` +- `get_admin(env)` + +### Pause and configuration + +- `pause(env, admin, duration_secs)` +- `unpause(env, admin)` +- `is_paused(env)` +- `set_min_deposit_amount(env, admin, min_amount)` +- `get_min_deposit_amount(env)` +- `set_max_lock_duration(env, admin, max_duration_secs)` +- `get_max_lock_duration(env)` +- `set_min_lock_duration(env, admin, min_duration_secs)` +- `get_min_lock_duration(env)` + +### User fund flows + +- `deposit(env, user, amount)` +- `withdraw(env, user, amount)` + +### Lock lifecycle + +- `lock_funds(env, user, amount, unlock_time)` +- `extend_lock(env, user, lock_id, new_unlock_time)` +- `withdraw_lock(env, user, lock_id)` + +### Read helpers + +- `get_balance(env, user)` +- `get_balance_snapshot(env, user)` +- `get_lock_summary(env, user)` +- `get_locked_balance(env, user)` +- `can_withdraw(env, user)` +- `get_lock(env, user, lock_id)` +- `list_locks(env, user, offset, limit)` + +### Admin rotation + +- `transfer_admin(env, admin, new_admin)` + +## 7. Event Surface + +The contract publishes events for major state transitions and admin changes. + +| Event name/topic 0 | Trigger | +| --- | --- | +| `initialize` | Initial contract setup | +| `deposit` | Successful deposit | +| `withdraw` | Successful available-balance withdrawal | +| `lock` | Successful lock creation | +| `extend_lock` | Successful lock extension | +| `withdraw_lock` | Successful matured lock withdrawal | +| `pause` | Pause activated | +| `unpause` | Pause cleared | +| `xferadmin` | Admin transferred | +| `cfg_min` | Min deposit rule updated | +| `cfg_maxlk` | Max lock duration updated | +| `cfg_minlk` | Min lock duration updated | + +Events use Soroban topics for indexing and tuples/scalars as payloads. Topic 0 +is always the action name, while topic 1 is usually the user or admin address. + +## 8. Interaction Mechanisms Between Components + +At runtime, the system interaction model is: + +1. A wallet, SDK, CLI user, or test invokes a `SavingsVault` method. +2. Soroban enforces host auth when the method calls `require_auth()`. +3. The contract reads/writes Soroban instance or persistent storage. +4. Custody-sensitive flows call the configured SAC token contract. +5. The contract emits events describing the state transition. + +### Component relationship map + +- **Wallet / SDK / Soroban CLI** + - Builds the transaction + - Provides auth for the user or admin address +- **SavingsVault contract** + - Validates lifecycle, auth, and business rules + - Owns the accounting model + - Coordinates storage and event emission +- **SAC token contract** + - Performs the actual token transfer + - Represents the source of real custody +- **Indexers / mobile apps** + - Read contract state directly + - Or reconstruct state from events off-chain + +## 9. Core Workflows + +### Initialize + +1. Deployer/admin calls `initialize(admin, token)`. +2. Contract rejects repeat initialization. +3. Admin signature is required. +4. Global config is written to instance storage. +5. Storage version is set. +6. `initialize` event is emitted. + +### Deposit + +1. Contract verifies initialized state, migration state, storage version, and + pause status. +2. User signature is required. +3. Amount and configured minimum are validated. +4. SAC transfer moves tokens from user to contract. +5. Internal `Balance(user)` is incremented. +6. `deposit` event is emitted. + +### Withdraw + +1. Contract verifies initialized state and storage version. +2. User signature is required. +3. Available balance is checked against `Balance(user)`. +4. SAC transfer moves tokens from contract to user. +5. Internal `Balance(user)` is decremented. +6. `withdraw` event is emitted. + +### Lock funds + +1. Contract verifies initialized state, version, and pause status. +2. User signature is required. +3. Amount and unlock-time rules are validated. +4. Global min/max lock-duration rules are enforced on creation. +5. Available balance is checked. +6. `NextLockId(user)` is incremented. +7. `Lock(user, next_id)` is written. +8. `Balance(user)` is decremented. +9. Locked total is recomputed for the emitted payload. +10. `lock` event is emitted. + +### Extend lock + +1. Contract verifies initialized state, version, and pause status. +2. User signature is required. +3. Lock existence and non-withdrawn status are validated. +4. `new_unlock_time` must be in the future and strictly greater than the + current unlock time. +5. Lock entry is updated in place. +6. `extend_lock` event is emitted. + +### Withdraw matured lock + +1. Contract verifies initialized state and storage version. +2. User signature is required. +3. Lock is loaded by `(user, lock_id)`. +4. Contract verifies the lock exists, is not already withdrawn, and is mature. +5. SAC transfer moves the lock amount from contract to user. +6. Lock is marked withdrawn and its amount is zeroed. +7. `withdraw_lock` event is emitted. + +### Pause / unpause + +1. Admin-only methods require both `require_auth()` and an admin-role match. +2. `pause` writes `Paused = true` and sets `PauseExpiry`. +3. `unpause` clears both fields immediately. +4. Mutating non-withdrawal methods call `require_not_paused()`. +5. Expired pauses are cleared lazily on the next mutating call. + +## 10. Data Flow Summary By Responsibility + +### User funds + +- Real token movement is handled by SAC. +- Logical fund partitioning is handled by: + - `Balance(user)` for unlocked funds + - `Lock(user, id)` for time-locked funds + +### Read models + +- `get_balance` returns only available balance. +- Matured locks remain in lock storage until individually withdrawn. +- `get_balance_snapshot` and `get_lock_summary` aggregate across lock history. + +### Error propagation + +- Business-rule failures become typed `ContractError` values. +- Host-level auth failures remain Soroban auth failures rather than contract + error codes. + +## 11. What This Repo Does Not Contain + +To avoid over-generalizing this project into a traditional web system: + +- No HTTP controllers or API routes +- No database migrations +- No background workers +- No message queues +- No frontend application +- No off-chain service implementation for SDK orchestration + +The closest off-chain integration guidance lives in docs such as +`sdk-contract-sequence.md`, `invocation-examples.md`, and `read-models.md`, but +those are consumer-facing documents, not implemented services. diff --git a/docs/comprehensive-analysis.md b/docs/comprehensive-analysis.md index 19d0ce8..4560496 100644 --- a/docs/comprehensive-analysis.md +++ b/docs/comprehensive-analysis.md @@ -1,211 +1,200 @@ -# Comprehensive Codebase Analysis Report -## PocketPay Savings Vault Contract - ---- -## 1. System Architecture and Directory Structure -### Directory Structure -``` -pocketpay-contracts/ -├── .github/ -│ └── workflows/ -│ └── trigger-auto-merge.yml # GitHub Actions CI/CD -├── contracts/ -│ └── savings_vault/ # Main contract crate -│ ├── Cargo.toml # Crate config -│ ├── src/ -│ │ ├── lib.rs # Contract implementation -│ │ └── test/ # Unit & property tests -│ │ ├── mod.rs -│ │ ├── admin_invariant_guard.rs -│ │ ├── balance_conservation.rs -│ │ ├── initialization.rs -│ │ ├── lock_read_helpers.rs -│ │ ├── maximum_amount_boundary.rs -│ │ ├── property_fee_invariants.rs -│ │ ├── property_vault_accounting.rs -│ │ ├── replay_protection.rs -│ │ ├── test_helpers.rs -│ │ ├── unauthorized_access.rs -│ │ ├── withdraw_lock.rs -│ │ └── zero_duration_lock.rs -│ └── test_snapshots/ # Snapshots for tests -├── docs/ # Comprehensive documentation -│ ├── accounting-invariants.md -│ ├── admin-role.md -│ ├── architecture.md -│ ├── audit-readiness.md -│ ├── balance-reconciliation.md -│ ├── contract-id-handoff.md -│ ├── deployment-environments.md -│ ├── deployment-output-example.md -│ ├── error-codes.md -│ ├── events.md -│ ├── pause-design.md -│ ├── storage-ttl.md -│ ├── troubleshooting.md -│ ├── upgrade-strategy.md -│ └── comprehensive-analysis.md -├── .env.example # Environment variable template -├── Cargo.toml # Rust workspace root config -└── Makefile # Task runner (build, test, size) -``` - -### System Architecture Paradigm -- **Monolithic smart contract**: Single Rust crate compiled to WASM -- **Soroban (Stellar) blockchain platform**: On-chain execution -- **On-chain state only**: No off-chain databases; all state stored via Soroban persistent/instance storage -- **Core modules**: - 1. **Initialization**: `initialize` function, state checks - 2. **Token Custody**: SAC integration via `soroban_sdk::token::Client` - 3. **Accounting**: Balance/lock management, `get_balance`, `get_locked_balance` - 4. **Time-Based Logic**: Unlock time checks, `can_withdraw` - 5. **Authorization**: `require_auth()` for all state-changing operations - 6. **Events**: On-chain event emission for all state changes - ---- -## 2. Component Functionality and Tech Stack -### Tech Stack -| Category | Technology | Version/Purpose | -|----------|------------|-----------------| -| Language | Rust | 2021 Edition | -| Blockchain | Soroban (Stellar) | Smart contract platform | -| Primary Dependency | soroban-sdk | 22.0.0 (provides env, storage, auth, tokens, testutils) | -| Compilation Target | WASM | `wasm32-unknown-unknown` | -| Build Tool | Cargo | Rust package manager | -| Task Runner | Make | Build/test shortcuts | -| CI/CD | GitHub Actions | Triggered on PR merge | -| Property Testing | proptest | Randomized operation sequence testing | - -### Key Files and Their Purpose -| File | Purpose | -|------|---------| -| [lib.rs](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs) | Main contract implementation, all public functions | -| [test/property_vault_accounting.rs](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/test/property_vault_accounting.rs) | Property-driven tests for accounting invariants and global token custody | -| [test/admin_invariant_guard.rs](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/test/admin_invariant_guard.rs) | Tests for admin role isolation | -| [test/withdraw_lock.rs](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/test/withdraw_lock.rs) | Tests for `withdraw_lock` function | -| [test/maximum_amount_boundary.rs](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/test/maximum_amount_boundary.rs) | Tests for large amount (near i128 MAX) handling | -| [Cargo.toml (workspace)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/Cargo.toml) | Workspace config, soroban-sdk dependency | -| [Cargo.toml (contract)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/Cargo.toml) | Contract-specific dependencies (proptest in dev) | - ---- -## 3. Core Business Logic and Data Flows -### Public Contract Functions -| Function | Purpose | -|----------|---------| -| [initialize(env, admin, token)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L248) | Initialize contract with admin address and token SAC | -| [get_version(env)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L300) | Return contract version string ("0.1.0") | -| [deposit(env, user, amount)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L333) | Deposit tokens to user's vault | -| [withdraw(env, user, amount)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L413) | Withdraw tokens from user's vault | -| [withdraw_lock(env, user, lock_id)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L538) | Withdraw tokens from a specific matured lock | -| [get_balance(env, user)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L629) | Query user's available (unlocked) balance | -| [lock_funds(env, user, amount, unlock_time)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L689) | Lock user's available funds until a future time | -| [get_locked_balance(env, user)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L805) | Query user's locked (unmatured) balance | -| [can_withdraw(env, user)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L862) | Check if user has matured locks available to withdraw | -| [get_lock(env, user, lock_id)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L895) | Get a single lock entry by lock ID | -| [list_locks(env, user, offset, limit)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L921) | List user's locks with pagination (max 50 per page) | -| [get_admin(env)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L961) | Get current admin address | -| [transfer_admin(env, admin, new_admin)](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/contracts/savings_vault/src/lib.rs#L985) | Transfer admin privileges to new address | - -### Critical User Journeys -#### Journey 1: Initialize Contract -1. Admin calls `initialize(env, admin, token)` -2. `admin.require_auth()` verifies admin signature -3. Check if `Initialized` is already set → panic if true -4. Store `Admin`, `Initialized`, `Token`, and `StorageVersion` in **instance storage** -5. Emit `(symbol_short!("init"), admin)` event with token as value - -#### Journey 2: Deposit Tokens -1. User calls `deposit(env, user, amount)` -2. `assert_initialized()` and `assert_supported_storage_version()` pass -3. `user.require_auth()` verifies user signature -4. Validate `amount > 0` → panic if not -5. Retrieve SAC token address and create `token::Client` -6. `token_client.transfer(user, contract_address, amount)` moves tokens to contract -7. Update user's `Balance(user)` (persistent storage) by adding `amount` -8. Emit `(symbol_short!("deposit"), user)` event with `(amount, new_balance)` as value - -#### Journey 3: Withdraw Tokens -1. User calls `withdraw(env, user, amount)` -2. `assert_initialized()`, `assert_supported_storage_version()`, and `user.require_auth()` pass -3. Validate `amount > 0` -4. Calculate available balance = deposited `Balance(user)` + sum of matured `LockEntry.amount` (where `current_time >= unlock_time`) -5. Panic if `amount > available` -6. `token_client.transfer(contract_address, user, amount)` sends tokens to user -7. Subtract amount from `Balance(user)` first, then from matured locks if needed -8. Update `Balance(user)` and `Locks(user)` in persistent storage -9. Emit `(symbol_short!("withdraw"), user)` event with `(amount, new_balance, new_locked)` as value - -#### Journey 4: Lock Funds -1. User calls `lock_funds(env, user, amount, unlock_time)` -2. `assert_initialized()`, `assert_supported_storage_version()`, and `user.require_auth()` pass -3. Validate `amount > 0`, `unlock_time > env.ledger().timestamp()`, and `amount <= available balance` -4. Retrieve `NextLockId(user)` (default to 1 if not set) -5. Create new `LockEntry { id, amount, unlock_time }` and add to `Locks(user)` -6. Subtract `amount` from `Balance(user)` -7. Update `Balance(user)`, `Locks(user)`, and `NextLockId(user)` (increment by 1) in persistent storage -8. Emit `(symbol_short!("lock"), user)` event with `(amount, unlock_time, new_balance, new_locked)` as value - -#### Journey 5: Withdraw a Specific Lock -1. User calls `withdraw_lock(env, user, lock_id)` -2. `assert_initialized()` and `user.require_auth()` pass -3. Load user's locks and find lock by ID → panic if not found -4. Verify lock is matured → panic if not -5. `token_client.transfer(contract_address, user, lock.amount)` sends tokens to user -6. Remove lock entry from `Locks(user)` -7. Update `Locks(user)` in persistent storage -8. Emit `(Symbol::new(env, "withdraw_lock"), user)` event with `(lock_id, amount)` as value - ---- -## 4. Coding Standards, Auth, and Data Validation -### Coding Standards -- Follow Rust idioms and Soroban best practices -- Comprehensive inline doc comments for all public functions -- Clear separation of concerns (initialization, deposits, withdrawals, locking, queries) -- **No custom error enum**: Uses panic strings for errors -- **Events emitted**: All state changes emit on-chain events! - -### Authorization -- **`initialize`**: Requires admin address authorization -- **`transfer_admin`**: Requires current admin address authorization -- **All state-changing user functions** (`deposit`, `withdraw`, `withdraw_lock`, `lock_funds`): Require user address authorization via `Address::require_auth()` -- **Read-only functions** (`get_balance`, `get_locked_balance`, `can_withdraw`, `get_lock`, `list_locks`, `get_version`, `get_admin`): No authorization needed (public queries) - -### Data Validation -- **Amount checks**: All functions accepting an amount panic if `amount <= 0` -- **Balance checks**: Withdraw and lock panic if amount exceeds available balance -- **Time checks**: `lock_funds` panics if `unlock_time <= current_time`; `withdraw_lock` panics if lock not matured -- **Initialization checks**: All functions except `initialize` panic if called before contract is initialized -- **Storage version check**: Most functions panic if storage version doesn't match `STORAGE_VERSION` (1) - ---- -## 5. External Integrations and Dependencies -### External Integrations -- **Stellar Asset Contract (SAC)**: Used via `soroban_sdk::token::Client` for token transfers in `deposit`, `withdraw`, and `withdraw_lock` -- **Soroban Network**: - - Testnet: `https://soroban-testnet.stellar.org:443` - - Passphrase: `Test SDF Network ; September 2015` - -### Environment-Dependent Configurations -- `.env.example`: Defines `VAULT_CONTRACT_ID`, `SOROBAN_RPC_URL`, `SOROBAN_NETWORK_PASSPHRASE` -- [deployment-environments.md](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/docs/deployment-environments.md) has full environment docs - ---- -## 6. Summary Report -### Key Architectural Decisions -1. **SAC Integration**: Real token custody implemented (internal accounting reconciled with SAC balance) -2. **Per-User Lock Entries**: Multiple locks per user, each with unique ID and independent unlock time -3. **Atomic Execution**: Soroban transactions are atomic, so failed operations leave no state changes -4. **Separate Instance/Persistent Storage**: Instance storage for admin/init/token/version; persistent storage for user data -5. **On-Chain Events**: All state changes emit events for off-chain tracking -6. **Comprehensive Property Testing**: proptest covers thousands of randomized operation sequences - -### Technical Debt -1. **No custom error enum**: Uses panic strings, which are harder for off-chain SDKs to handle consistently -2. **No pause/emergency stop mechanism**: Research exists in [pause-design.md](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/docs/pause-design.md), but not implemented -3. **No upgrade path**: Research exists in [upgrade-strategy.md](file:///c:/Users/muham/.trae/Grantfox%20Coder%20x/pocketpay-contracts/docs/upgrade-strategy.md), but not implemented - -### Addressable Edge Cases Now Covered -- ✅ **Global token custody invariant**: Tested in `property_vault_accounting::prop_global_token_custody` -- ✅ **Large amount handling**: Tested in `maximum_amount_boundary.rs` -- ✅ **User isolation**: Tested in `property_vault_accounting::prop_cross_user_isolation` -- ✅ **Admin isolation**: Tested in `admin_invariant_guard.rs` +# Comprehensive Codebase Analysis Package + +This package is the current, implementation-aligned entry point for understanding +the PocketPay Savings Vault repository. It supersedes earlier analysis notes +that were written before the current error model, pause logic, and token-backed +custody flow were fully implemented. + +## Scope + +- **Repository type:** Rust workspace for a single Soroban smart contract crate +- **Primary artifact:** `contracts/savings_vault` compiled to WASM +- **Current runtime model:** On-chain only; there is no application server, no + REST/GraphQL API surface, and no operational database schema in this repo +- **Source of truth:** `contracts/savings_vault/src/lib.rs`, crate manifests, + and the registered Rust test suite under `contracts/savings_vault/src/test/` + +## Package Contents + +- [Repo Map And Workflows](codebase-analysis/repo-map-and-workflows.md) + - Directory structure + - Core modules and responsibilities + - Storage schema and event surface + - End-to-end workflows and interaction mechanisms +- [Quality And Debt Assessment](codebase-analysis/quality-and-debt.md) + - Code quality and test posture + - Performance, security, and maintainability findings + - Technical debt inventory + - Prioritized recommendations + +## Executive Summary + +The repository is best understood as a **single-contract custody system** built +for the Stellar Soroban platform: + +- **One contract, one crate, one runtime unit** + - The `SavingsVault` contract is implemented in a single `lib.rs` file. + - Logical separation exists inside that file through sections for state, + storage keys, errors, helpers, admin/configuration, fund movement, read + models, and lock management. +- **Token-backed accounting** + - Internal balances are bookkeeping only. + - Real custody is enforced through the configured Stellar Asset Contract + (SAC), with token transfers executed in `deposit`, `withdraw`, and + `withdraw_lock`. +- **Per-user sharded storage** + - Global configuration lives in instance storage. + - User balances and lock records live in persistent storage keyed by address. + - Each lock is stored independently as `DataKey::Lock(user, lock_id)`. +- **Safety-oriented business rules** + - Mutating user operations require `require_auth()`. + - Emergency pause blocks new deposits and locks, but always leaves user exits + (`withdraw`, `withdraw_lock`) available. + - Errors are exposed through a stable `#[contracterror]` enum rather than + brittle panic strings. +- **Strong local test coverage** + - The repo contains a broad Rust test suite with focused behavior tests, + event schema regression tests, and property-based accounting checks. + +## Core Architectural Patterns + +### 1. Monolithic Contract, Segmented Internals + +The implementation is monolithic at the file/module level, but internally +organized around consistent responsibility zones: + +- Contract state types and storage keys +- Stable error taxonomy +- Initialization and migration helpers +- Admin and pause controls +- Token custody flows +- Lock lifecycle management +- Read models for SDK/mobile consumption + +### 2. Transfer-First Mutation Ordering + +The contract intentionally performs token transfers before mutating internal +storage in custody-sensitive flows: + +- `deposit`: transfer user -> contract, then credit balance +- `withdraw`: transfer contract -> user, then debit balance +- `withdraw_lock`: transfer contract -> user, then mark lock withdrawn + +On Soroban this ordering is acceptable because failed host calls roll back the +entire transaction, and the accompanying tests explicitly validate zero state +drift on failed transfers. + +### 3. Read Models Over Raw Storage + +The contract exposes raw reads (`get_balance`, `get_lock`, `list_locks`) and +higher-level aggregated reads (`get_balance_snapshot`, `get_lock_summary`). +This reduces off-chain SDK work for common screens, but creates a tradeoff: +some read helpers linearly scan a user's historical lock IDs. + +### 4. Operationally Enforced Storage Lifecycle + +The contract models storage versioning in code, but storage TTL renewal is an +operational concern rather than a self-healing in-contract mechanism. That is a +key part of the repository's technical debt and production-readiness gap. + +## Interface Inventory + +### On-chain Public Interface + +The contract exposes 23 public entry points grouped into: + +- Lifecycle and metadata +- Admin and configuration +- Pause controls +- User custody flows +- Lock lifecycle flows +- Read helpers and aggregated read models + +See the detailed inventory in +[Repo Map And Workflows](codebase-analysis/repo-map-and-workflows.md). + +### Events + +The contract emits events for all major state transitions, including: + +- `initialize` +- `deposit` +- `withdraw` +- `lock` +- `extend_lock` +- `withdraw_lock` +- `pause` +- `unpause` +- `xferadmin` +- Configuration updates: `cfg_min`, `cfg_maxlk`, `cfg_minlk` + +### API Endpoints + +There are **no HTTP API endpoints** implemented in this repository. + +- No REST handlers +- No GraphQL schema +- No web server +- No RPC service owned by this repo + +The effective API surface is the Soroban contract method set exposed by +`SavingsVault`. + +### Database Schemas + +There is **no application database schema** in this repository. + +- No SQL migrations +- No ORM models +- No Supabase/Postgres/MySQL schema +- No persistence layer outside Soroban storage + +The closest equivalent to a "schema" is the on-chain `DataKey` storage model +documented in `lib.rs` and summarized in the repo-map document. + +## Dependencies And Integrations + +### Direct Build-Time Dependencies + +- `soroban-sdk = 22.0.0` +- `proptest = 1` for dev/test only + +### Third-Party Runtime / Operational Integrations + +- **Stellar Soroban runtime** + - Contract execution environment + - Ledger timestamp source + - Authentication host for `require_auth()` +- **Stellar Asset Contract (SAC)** + - Token transfer and custody backend +- **Soroban CLI** + - Deployment and invocation tooling +- **Stellar testnet ecosystem** + - Friendbot + - Testnet RPC + - Explorer workflows +- **GitHub workflow dispatch** + - PR automation via `.github/workflows/trigger-auto-merge.yml` + +## Important Current Findings + +- The repository has evolved faster than some of its docs. +- Several existing documents still describe removed or superseded behavior: + - panic-string-only error handling + - vector-based `Locks(user)` storage as the primary model + - missing pause/events despite those features now existing + - outdated task-runner targets and old file paths +- A standalone TypeScript test exists under `tests/atomicity/`, but it does not + match the current Rust workspace structure and appears disconnected from the + actual build/test toolchain. + +## Validation Performed For This Package + +- Reviewed the full workspace layout, manifests, scripts, docs, and contract + implementation +- Mapped the full public contract interface and event surface +- Reviewed the registered Rust test suite and supporting fixtures +- Ran `cargo test` successfully from the workspace root during this analysis + +For the detailed technical debt and risk review, continue to +[Quality And Debt Assessment](codebase-analysis/quality-and-debt.md). diff --git a/docs/storage-audit.md b/docs/storage-audit.md index 047223d..c12decf 100644 --- a/docs/storage-audit.md +++ b/docs/storage-audit.md @@ -1,99 +1,82 @@ -# Storage Audit: Savings Vault Contract -This document provides a comprehensive audit of all storage keys used in the Savings Vault contract, including: -- Storage key definitions and types -- Mutation points (which functions modify which keys) -- Invariants that must always hold -- TTL management guidelines +# Storage Audit Map and Mutation Trace ---- - -## 1. Storage Layers -The contract uses two Soroban storage layers: +This document provides a comprehensive audit of all storage usage in the PocketPay Savings Vault contract. It maps storage keys, value types, mutation points, invariants, and test coverage to ensure a high-security posture for fund custody and accounting. -| Layer | Purpose | -|-------|---------| -| **Instance Storage** | Stores configuration and initialization state (admin, token, initialized flag, storage version) | -| **Persistent Storage** | Stores per-user state (balances, locks, lock ID counter) | +## 1. Storage Key Map ---- +The contract uses Soroban's **Instance** storage for global configuration and **Persistent** storage for user-specific data to optimize for resource usage and scalability. -## 2. Storage Key Audit +| DataKey Variant | Storage Type | Value Type | Ownership | Description | +| :--- | :--- | :--- | :--- | :--- | +| `Admin` | Instance | `Address` | Global | The current administrator address for configuration and pause controls. | +| `Initialized` | Instance | `bool` | Global | Flag indicating if the contract has been successfully initialized. | +| `Token` | Instance | `Address` | Global | The address of the SAC token managed by this vault. | +| `StorageVersion` | Instance | `u64` | Global | Current schema version for the contract storage. | +| `Paused` | Instance | `bool` | Global | Current pause status of the vault. | +| `PauseExpiry` | Instance | `u64` | Global | Timestamp (ledger seconds) when the current pause automatically expires. | +| `MinDepositAmount` | Instance | `i128` | Global | Minimum amount required for a single deposit operation. | +| `MaxLockDurationSecs` | Instance | `u64` | Global | Maximum duration a lock can be active. | +| `MinLockDurationSecs` | Instance | `u64` | Global | Minimum duration required for a new lock. | +| `Balance(Address)` | Persistent | `i128` | User | Total balance (available + locked) for a specific user. | +| `Lock(Address, u64)` | Persistent | `LockEntry` | User | Individual lock record keyed by user and a monotonic ID. | +| `NextLockId(Address)` | Persistent | `u64` | User | Monotonic counter used to generate unique IDs for a user's locks. | -### Instance Storage Keys -| Key | Type | Default | Initialization | Mutation Points | Invariants | -|-----|------|---------|----------------|-----------------|------------| -| `DataKey::Admin` | `Address` | None | Set once in `initialize` | `transfer_admin` | - Immutable after `transfer_admin`; only admin can change | -| `DataKey::Initialized` | `bool` | None | Set to `true` in `initialize` | None (never modified after initialization) | - Always `true` after initialization; never unset | -| `DataKey::Token` | `Address` | None | Set once in `initialize` | None (never modified after initialization) | - Immutable after initialization | -| `DataKey::StorageVersion` | `u64` | None | Set to `STORAGE_VERSION` (1) in `initialize` | None (never modified after initialization) | - Always equals `STORAGE_VERSION` | - -### Persistent Storage Keys -| Key | Type | Default | Initialization | Mutation Points | Invariants | -|-----|------|---------|----------------|-----------------|------------| -| `DataKey::Balance(Address)` | `i128` | `0` (implicit) | Not set at initialization | `deposit`, `withdraw`, `lock_funds` | - ≥ 0 at all times; represents available balance | -| `DataKey::Lock(Address, u64)` | `LockEntry` | None | Not set at initialization | `lock_funds`, `withdraw`, `withdraw_lock` | - `LockEntry.amount` ≥ 0; `LockEntry.id` unique for user; `LockEntry.unlock_time` ≥ 0 | -| `DataKey::NextLockId(Address)` | `u64` | `1` (implicit) | Not set at initialization | `lock_funds` | - Strictly increasing (monotonic); never decreases | +*Note: The `Locks(Address)` variant is defined in the enum but is currently unused in favor of individual `Lock(Address, u64)` entries.* --- -## 3. Mutation Point Mapping -This section maps each storage key to the functions that modify it: - -### Instance Storage -| Function | Modifies | -|----------|----------| -| `initialize` | `Admin`, `Initialized`, `Token`, `StorageVersion` | -| `transfer_admin` | `Admin` | - -### Persistent Storage -| Function | Modifies | -|----------|----------| -| `deposit` | `Balance(user)` | -| `withdraw` | `Balance(user)`, `Lock(user, id)` | -| `lock_funds` | `Balance(user)`, `Lock(user, id)`, `NextLockId(user)` | -| `withdraw_lock` | `Lock(user, id)` | +## 2. Mutation Trace + +The following table tracks which functions mutate specific storage keys. Read-only operations are omitted for brevity. + +| Function | Mutated Keys | State Change Description | +| :--- | :--- | :--- | +| `initialize` | `Admin`, `Initialized`, `Token`, `StorageVersion` | Sets global config; locks initialization. | +| `try_migrate` | `StorageVersion` | Increments schema version during upgrades. | +| `pause` | `Paused`, `PauseExpiry` | Enables emergency pause with an expiry timestamp. | +| `unpause` | `Paused`, `PauseExpiry` | Manually clears pause state. | +| `require_not_paused` | `Paused`, `PauseExpiry` | **Lazy Mutation**: Clears pause if `ledger.timestamp() >= expiry`. | +| `set_min_deposit_amount`| `MinDepositAmount` | Updates global minimum deposit threshold. | +| `set_max_lock_duration` | `MaxLockDurationSecs` | Updates global maximum lock time. | +| `set_min_lock_duration` | `MinLockDurationSecs` | Updates global minimum lock time. | +| `deposit` | `Balance(user)` | Increments user balance after successful token transfer. | +| `withdraw` | `Balance(user)` | Decrements user balance after successful token transfer. | +| `lock_funds` | `Balance(user)`, `Lock(user, id)`, `NextLockId(user)` | Debits available balance, writes lock entry, increments ID counter. | +| `extend_lock` | `Lock(user, id)` | Updates the `unlock_time` of an existing lock entry. | +| `withdraw_lock` | `Lock(user, id)` | Zeroes out the lock entry amount after maturity and token transfer. | +| `transfer_admin` | `Admin` | Rotates the administrative address. | --- -## 4. Critical Storage Invariants -These invariants must hold at all times, across all function calls: +## 3. Failure-Path State Expectations -### Invariant 1: User Available Balance ≥ 0 -- **Description**: `Balance(user)` must never be negative -- **Enforced By**: `deposit`, `withdraw`, `lock_funds` -- **Tested By**: `balance_conservation.rs` tests and `property_vault_accounting.rs` proptests +The contract employs a **"transfer-then-write"** pattern to ensure atomicity. If a transaction fails at any point, all storage changes are rolled back by the Soroban host environment. -### Invariant 2: User Lock Entry Amounts ≥ 0 -- **Description**: Every `LockEntry.amount` in `Locks(user)` must be ≥ 0 -- **Enforced By**: `lock_funds` -- **Tested By**: `balance_conservation.rs` tests +| Failure Scenario | Expected State | Implementation Detail | +| :--- | :--- | :--- | +| **Token Transfer Fails** | No mutation to `Balance` or `Lock`. | Token transfer is attempted *before* storage writes in `deposit` and `withdraw`. | +| **Auth Failure** | No mutation to any key. | `require_auth()` is called at the start of all protected methods. | +| **Invariant Violation** | Transaction panics; all state reverted. | Invariants are checked at the end of functions or via guards. | +| **Vault Paused** | `require_not_paused` panics (unless lazy clearing). | Mutations are blocked early in the call stack. | -### Invariant 3: NextLockId Is Monotonic -- **Description**: `NextLockId(user)` must never decrease; only increments by 1 per `lock_funds` call -- **Enforced By**: `lock_funds` -- **Tested By**: `lock_read_helpers.rs` tests +--- -### Invariant 4: Lock Entry IDs Are Unique Per User -- **Description**: No two `LockEntry` in `Locks(user)` share the same `id` -- **Enforced By**: `lock_funds` (uses `NextLockId` to generate unique IDs) -- **Tested By**: Implicit via monotonic `NextLockId` +## 4. Invariants and Test Coverage -### Invariant 5: Token Custody Invariant -- **Description**: The sum of all user balances + sum of all user locked balances ≤ SAC balance of the contract -- **Enforced By**: All functions that perform token transfers (`deposit`, `withdraw`, `withdraw_lock`) -- **Tested By**: `property_vault_accounting::prop_global_token_custody` (proptest) +Core accounting and security invariants are verified through a combination of unit tests and property-based tests. -### Invariant 6: Initialized Flag Remains True After Initialization -- **Description**: Once `initialize` has been called, `Initialized` remains `true` forever -- **Enforced By**: `initialize` (sets flag; no other function modifies it) -- **Tested By**: `initialization.rs` tests +| Invariant | Description | Test Reference | +| :--- | :--- | :--- | +| **Balance Conservation** | `Available + Locked == Total` for all users at all times. | [balance_conservation.rs](file:///c:/Users/abbat/.trae/GrantFox/pocketpay-contracts/contracts/savings_vault/src/test/balance_conservation.rs) | +| **Token Custody** | `Contract SAC Balance == Σ(User Balances)`. | [property_fee_invariants.rs](file:///c:/Users/abbat/.trae/GrantFox/pocketpay-contracts/contracts/savings_vault/src/test/property_fee_invariants.rs) | +| **Atomic Rollback** | Failed transfers must not credit/debit internal accounting. | [token_transfer_rollback.rs](file:///c:/Users/abbat/.trae/GrantFox/pocketpay-contracts/contracts/savings_vault/src/test/token_transfer_rollback.rs) | +| **Lock Integrity** | `Locked Balance == Σ(Active Lock Entries)`. | [multi_lock_invariants.rs](file:///c:/Users/abbat/.trae/GrantFox/pocketpay-contracts/contracts/savings_vault/src/test/multi_lock_invariants.rs) | +| **ID Uniqueness** | `NextLockId` must never produce a duplicate ID for a user. | [multi_lock_invariants.rs](file:///c:/Users/abbat/.trae/GrantFox/pocketpay-contracts/contracts/savings_vault/src/test/multi_lock_invariants.rs) | --- -## 5. TTL Management Guidelines -See [storage-ttl.md](storage-ttl.md) for full TTL management guidelines! - ---- +## 5. Technical Debt and Audit Notes -## 6. Storage Upgrade / Migration Plan -See [upgrade-strategy.md](upgrade-strategy.md) for future upgrade/migration planning! +- **Unused Storage Variant**: `DataKey::Locks(Address)` should be removed or implemented to avoid confusion during audits. +- **TTL Management**: The audit currently assumes standard Soroban TTL management for Persistent/Instance storage. A dedicated TTL extension strategy should be documented if custom intervals are required. +- **Linear Scan Risk**: Functions like `get_balance_snapshot` and `list_locks` iterate over lock IDs. While capped by `MAX_LOCK_PAGE_SIZE`, high lock counts per user could impact gas costs for complex read-aggregations. diff --git a/docs/vault_storage_audit_map.md b/docs/vault_storage_audit_map.md deleted file mode 100644 index 487e6fb..0000000 --- a/docs/vault_storage_audit_map.md +++ /dev/null @@ -1,36 +0,0 @@ -# Vault Storage Audit Map - -This document maps all storage entries for the Savings Vault contract, including keys, value types, mutating functions, and invariants. This map supports audit preparation by providing a clear overview of state management. - -## 1. Storage Keys & Value Types - -| Storage Key | Layer | Rust Type | Description | -|---|---|---|---| -| `DataKey::Admin` | Instance | `Address` | Contract administrator address. | -| `DataKey::Initialized` | Instance | `bool` | Flag indicating contract initialization. | -| `DataKey::Token` | Instance | `Address` | Token contract (SAC) address for transfers. | -| `DataKey::Balance(Address)` | Persistent | `i128` | Available (unlocked) balance of a user. | -| `DataKey::Locks(Address)` | Persistent | `Vec` | Active or matured lock records for a user. | -| `DataKey::NextLockId(Address)` | Persistent | `u64` | Counter for generating unique lock IDs per user. | - -## 2. Mutating Functions - -| Function | Storage Keys Modified | -|---|---| -| `initialize` | `Admin`, `Initialized`, `Token` | -| `deposit` | `Balance(user)` | -| `withdraw` | `Balance(user)`, `Locks(user)` | -| `withdraw_lock` | `Locks(user)` | -| `lock_funds` | `Balance(user)`, `Locks(user)`, `NextLockId(user)` | - -## 3. Storage Invariants - -- **Admin**: Set exactly once during initialization. Cannot be changed. Must be a valid `Address`. -- **Initialized**: Set to `true` during initialization. Prevents re-initialization. -- **Token**: Set exactly once during initialization. Must be a valid token contract address. -- **Balance**: Must always be $\ge 0$. Modifications must be authorized by `user.require_auth()`. -- **Locks**: - - Each `LockEntry` must have `amount > 0`. - - When a lock is created, its `unlock_time` must be strictly greater than the current ledger timestamp. -- **NextLockId**: Must monotonically increase starting from `1` for each user. -- **Global Accounting**: The total contract token balance must equal the sum of all users' `Balance` plus the sum of all users' locked amounts.