diff --git a/contracts/savings_vault/src/test/mod.rs b/contracts/savings_vault/src/test/mod.rs index eb6dd9f..834a27c 100644 --- a/contracts/savings_vault/src/test/mod.rs +++ b/contracts/savings_vault/src/test/mod.rs @@ -27,6 +27,7 @@ mod maximum_lock_duration; mod minimum_deposit_amount; mod minimum_lock_duration; mod multi_lock_invariants; +mod negative_paths; mod pause; mod pause_state_read; mod pause_transition; @@ -1482,6 +1483,34 @@ fn test_withdraw_emits_event() { ); } +#[test] +fn test_withdraw_lock_emits_event() { + use soroban_sdk::{symbol_short, TryIntoVal}; + + let env = test_env(); + let (contract_id, client) = init_contract(&env); + let (env, _admin, client, _token_client, token_admin) = test_token(env, contract_id, client); + + let user = new_user(&env); + token_admin.mint(&user, &1000); + set_ledger_timestamp(&env, 1_000); + + deposit_balance(&client, &user, 500); + let id = client.lock_funds(&user, &200, &2_000); + + set_ledger_timestamp(&env, 2_000); + client.withdraw_lock(&user, &id); + + let events = env.events().all(); + let (_contract, topics, data) = events.get(events.len() - 1).unwrap(); + let topic0: soroban_sdk::Symbol = topics.get(0).unwrap().try_into_val(&env).unwrap(); + let topic1: Address = topics.get(1).unwrap().try_into_val(&env).unwrap(); + let (amount, new_balance): (i128, i128) = data.try_into_val(&env).unwrap(); + assert_eq!(topic0, symbol_short!("wdr_lock")); + assert_eq!(topic1, user); + assert_eq!((amount, new_balance), (200_i128, 300_i128)); +} + #[test] fn test_lock_funds_emits_event() { use soroban_sdk::{symbol_short, TryIntoVal}; diff --git a/contracts/savings_vault/src/test/negative_paths.rs b/contracts/savings_vault/src/test/negative_paths.rs new file mode 100644 index 0000000..0c85c17 --- /dev/null +++ b/contracts/savings_vault/src/test/negative_paths.rs @@ -0,0 +1,242 @@ +//! Comprehensive negative-path test suite for the Savings Vault contract. +//! +//! Covers unauthorized access, invalid inputs, early withdrawals, +//! missing initialization, and state consistency after failures. + +use super::*; +use soroban_sdk::testutils::{Address as _, Events}; +use soroban_sdk::{Address, Env, IntoVal}; +use crate::ContractError; +use test_helpers::*; + +// ========================================================================= +// 1. Missing Initialization +// ========================================================================= + +#[test] +#[should_panic] +fn test_uninitialized_deposit_panics() { + let env = test_env(); + let contract_id = env.register(SavingsVault, ()); + let client = SavingsVaultClient::new(&env, &contract_id); + let user = Address::generate(&env); + client.deposit(&user, &100); +} + +#[test] +#[should_panic] +fn test_uninitialized_withdraw_panics() { + let env = test_env(); + let contract_id = env.register(SavingsVault, ()); + let client = SavingsVaultClient::new(&env, &contract_id); + let user = Address::generate(&env); + client.withdraw(&user, &100); +} + +#[test] +#[should_panic] +fn test_uninitialized_lock_funds_panics() { + let env = test_env(); + let contract_id = env.register(SavingsVault, ()); + let client = SavingsVaultClient::new(&env, &contract_id); + let user = Address::generate(&env); + client.lock_funds(&user, &100, &2_000); +} + +#[test] +#[should_panic] +fn test_uninitialized_pause_panics() { + let env = test_env(); + let contract_id = env.register(SavingsVault, ()); + let client = SavingsVaultClient::new(&env, &contract_id); + let admin = Address::generate(&env); + client.pause(&admin, &3600); +} + +// ========================================================================= +// 2. Unauthorized Access (Admin Boundaries) +// ========================================================================= + +#[test] +#[should_panic] +fn test_unauthorized_pause_fails() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _vault_admin) = vault_with_sac(&env); + let rando = Address::generate(&env); + + // Attempt to pause as a non-admin user + client.pause(&rando, &3600); +} + +#[test] +#[should_panic] +fn test_unauthorized_unpause_fails() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, vault_admin) = vault_with_sac(&env); + let rando = Address::generate(&env); + + env.mock_all_auths(); + client.pause(&vault_admin, &3600); + + // Intentionally omit mock_all_auths() to test auth rejection + client.unpause(&rando); +} + +#[test] +#[should_panic] +fn test_unauthorized_set_min_deposit_amount_fails() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _vault_admin) = vault_with_sac(&env); + let rando = Address::generate(&env); + + client.set_min_deposit_amount(&rando, &1000); +} + +// ========================================================================= +// 3. Invalid Inputs +// ========================================================================= + +#[test] +fn test_invalid_deposit_amounts_fail() { + let env = test_env(); + let (_contract_id, client, _token_client, _token_admin, _vault_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + + // Case 1: Zero deposit + let res = client.try_deposit(&user, &0); + assert!(res.is_err()); + + // Case 2: Negative deposit + let res = client.try_deposit(&user, &-1); + assert!(res.is_err()); +} + +#[test] +fn test_deposit_below_minimum_fails() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, vault_admin) = vault_with_sac(&env); + + env.mock_all_auths(); + client.set_min_deposit_amount(&vault_admin, &1000); + + let user = Address::generate(&env); + token_admin.mint(&user, &2000); + + // Attempt deposit below minimum floor + let res = client.try_deposit(&user, &500); + assert!(res.is_err()); +} + +#[test] +fn test_lock_duration_boundary_failures() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, vault_admin) = vault_with_sac(&env); + + env.mock_all_auths(); + client.set_max_lock_duration(&vault_admin, &10_000); + client.set_min_lock_duration(&vault_admin, &1_000); + + let user = Address::generate(&env); + token_admin.mint(&user, &1000); + client.deposit(&user, &1000); + + set_ledger_timestamp(&env, 1000); + + // Case 1: Duration too long + let res = client.try_lock_funds(&user, &100, &12_000); // 11,000s duration + assert!(res.is_err()); + + // Case 2: Duration too short + let res = client.try_lock_funds(&user, &100, &1_500); // 500s duration + assert!(res.is_err()); +} + +// ========================================================================= +// 4. Early Withdrawals & Invalid Lock States +// ========================================================================= + +#[test] +fn test_early_lock_withdrawal_fails() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _vault_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + + env.mock_all_auths(); + set_ledger_timestamp(&env, 1000); + token_admin.mint(&user, &1000); + client.deposit(&user, &1000); + + let lock_id = client.lock_funds(&user, &500, &5000); + + // Attempt withdrawal at T=4999 (1s before maturity) + set_ledger_timestamp(&env, 4999); + let res = client.try_withdraw_lock(&user, &lock_id); + assert!(res.is_err()); +} + +#[test] +fn test_extend_withdrawn_lock_fails() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _vault_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + + env.mock_all_auths(); + set_ledger_timestamp(&env, 1000); + token_admin.mint(&user, &1000); + client.deposit(&user, &1000); + + let lock_id = client.lock_funds(&user, &500, &5000); + set_ledger_timestamp(&env, 5000); + client.withdraw_lock(&user, &lock_id); + + // Attempt to extend a lock that has already been withdrawn + let res = client.try_extend_lock(&user, &lock_id, &10_000); + assert!(res.is_err()); +} + +// ========================================================================= +// 5. State Consistency After Failures +// ========================================================================= + +#[test] +fn test_state_remains_consistent_after_failed_lock() { + let env = test_env(); + let (_contract_id, client, _token_client, token_admin, _vault_admin) = vault_with_sac(&env); + let user = Address::generate(&env); + + env.mock_all_auths(); + set_ledger_timestamp(&env, 1000); + token_admin.mint(&user, &1000); + client.deposit(&user, &1000); + + let initial_balance = client.get_balance(&user); + let initial_locked = client.get_locked_balance(&user); + + // Attempt to lock more than available + let _ = client.try_lock_funds(&user, &1001, &5000); + + assert_eq!(client.get_balance(&user), initial_balance, "Balance should not change after failed lock"); + assert_eq!(client.get_locked_balance(&user), initial_locked, "Locked balance should not change after failed lock"); +} + +#[test] +fn test_state_consistency_after_failed_token_transfer() { + let env = test_env(); + let (contract_id, client) = init_contract(&env); + let (env, _admin, client, token_client, token_admin) = test_token(env, contract_id.clone(), client); + let user = Address::generate(&env); + + // User has 50 tokens, tries to deposit 100 + token_admin.mint(&user, &50); + let initial_user_tokens = token_client.balance(&user); + let initial_vault_tokens = token_client.balance(&contract_id); + + env.mock_all_auths(); + let res = client.try_deposit(&user, &100); + assert!(res.is_err(), "Deposit should fail due to insufficient SAC balance"); + + // Verify internal accounting and external token balances are unchanged + assert_eq!(client.get_balance(&user), 0, "Internal balance should not be credited"); + assert_eq!(token_client.balance(&user), initial_user_tokens, "User tokens should not have moved"); + assert_eq!(token_client.balance(&contract_id), initial_vault_tokens, "Vault tokens should not have changed"); +} diff --git a/docs/contribution-quality-gate.md b/docs/contribution-quality-gate.md new file mode 100644 index 0000000..414e6a0 --- /dev/null +++ b/docs/contribution-quality-gate.md @@ -0,0 +1,62 @@ +# Contribution Quality Gate + +This document defines the objective criteria for "payment-ready" or "production-ready" work in the PocketPay Contracts repository. To maintain high security and reliability, every Pull Request (PR) must pass through this quality gate before being considered for final approval. + +## 1. Quality Gate Checklist + +Every PR must satisfy the following checklist. If any item is missing or incomplete, the PR will be flagged for further work. + +### Implementation +- [ ] **Completeness**: The feature or fix is fully implemented according to the issue description. +- [ ] **No Placeholders**: All `TODO`, `FIXME`, and `HACK` comments related to the change have been resolved. +- [ ] **Storage Integrity**: Storage usage follows the [Storage Audit Map](storage-audit.md) and correctly manages Persistent vs. Instance storage. +- [ ] **Authorization**: All state-changing functions correctly enforce `require_auth()` for the appropriate parties. +- [ ] **Error Handling**: Uses structured `ContractError` codes instead of generic panics where appropriate. + +### Testing +- [ ] **Unit Tests**: Every new or modified function has unit tests for both success paths and failure paths (e.g., unauthorized access, invalid inputs). +- [ ] **Invariant Verification**: Changes to accounting logic are verified by new or existing [Property Tests](../contracts/savings_vault/src/test/property_vault_accounting.rs). +- [ ] **Event Snapshots**: Any changes to event schemas have updated [Snapshots](../contracts/savings_vault/test_snapshots/). +- [ ] **Local Verification**: All tests pass locally using `cargo test`. + +### Documentation +- [ ] **README/Docs**: New features or architectural changes are documented in the relevant `docs/` files. +- [ ] **Acceptance Criteria**: The PR description explicitly lists how each Acceptance Criterion from the original issue was met. + +### CI & Tooling +- [ ] **Formatting**: Code is formatted via `cargo fmt`. +- [ ] **Lints**: `cargo clippy --tests` passes with no warnings. +- [ ] **Build**: `make build-release` succeeds and the WASM size remains within acceptable limits. + +--- + +## 2. Contract-Specific Testing Expectations + +Soroban smart contracts require rigorous testing due to their immutable nature once deployed. We expect: + +1. **Authorization Boundaries**: Tests must explicitly verify that a function fails when called by an unauthorized address. +2. **State Rollbacks**: Tests must verify that if a transaction fails (e.g., token transfer failure), no storage mutations occur. +3. **Maturity Checks**: For time-locked funds, tests must verify behavior exactly at, before, and after the maturity timestamp. +4. **Edge Case Amounts**: Test with `0`, `1`, `MAX`, and amounts just below/above configured limits. + +--- + +## 3. Examples of Incomplete Work + +To avoid common pitfalls, here are examples of PRs that do **not** pass the quality gate: + +- **"Happy Path Only"**: A PR that adds a `deposit` feature but only tests a successful deposit, skipping unauthorized or over-limit tests. +- **"Logic without Invariants"**: A PR that changes how balances are calculated but doesn't update the property-based tests that ensure total funds remain constant. +- **"Stale Docs"**: A PR that changes a function signature in the contract but leaves the `api-reference.md` or `README.md` outdated. +- **"Ignoring Clippy"**: A PR where `cargo clippy` emits warnings that the contributor describes as "unrelated" or "minor". +- **"WIP in Main"**: PRs marked as "Work In Progress" or containing half-implemented logic should not be requested for final review. + +--- + +## 4. How to Use This Gate + +1. **Before Opening a PR**: Review your work against this checklist. +2. **In Your PR Description**: Reference this quality gate and confirm that all items are checked. +3. **Reviewers**: Use this checklist as the primary framework for your review. If the gate isn't met, request changes immediately. + +For more details on local reproduction of tests, see the [Test Reproduction Guide](reproducing-test-failures.md). diff --git a/docs/test-coverage.md b/docs/test-coverage.md index 2c16b45..afb28e6 100644 --- a/docs/test-coverage.md +++ b/docs/test-coverage.md @@ -7,7 +7,22 @@ new tests. It is a coverage **map**, not a line-coverage report — see Source: `contracts/savings_vault/src/test/` (`mod.rs`, `initialization.rs`, `balance_conservation.rs`, `withdraw_lock.rs`, `maximum_amount_boundary.rs`, -`unauthorized_access.rs`, `lock_read_helpers.rs`, `replay_protection.rs`). +`unauthorized_access.rs`, `lock_read_helpers.rs`, `replay_protection.rs`, `negative_paths.rs`). + +## Negative Path & Error States + +This section explicitly maps tests for invalid inputs, unauthorized access, and lifecycle violations (issue #442). + +| Category | Behaviour | Status | Test(s) | +|---|---|---|---| +| **Auth** | Unauthorized user calls state-changing functions | Covered | `test_unauthorized_deposit_fails`, `test_unauthorized_withdraw_fails`, `test_unauthorized_lock_fails`, `test_unauthorized_withdraw_lock_fails`, `test_unauthorized_extend_lock_fails` | +| **Auth** | Non-admin user calls admin-gated functions | Covered | `test_unauthorized_pause_fails`, `test_unauthorized_unpause_fails`, `test_unauthorized_set_min_deposit_amount_fails`, `test_transfer_admin_not_authorized_panics` | +| **Lifecycle** | Calling functions before `initialize` | Covered | `test_uninitialized_deposit_panics`, `test_uninitialized_withdraw_panics`, `test_uninitialized_lock_funds_panics`, `test_uninitialized_pause_panics`, `test_get_admin_before_initialization_panics` | +| **Validation** | Invalid deposit amounts (zero, negative, below floor) | Covered | `test_deposit_zero_panics`, `test_deposit_negative_panics`, `test_invalid_deposit_amounts_fail`, `test_deposit_below_minimum_fails` | +| **Validation** | Invalid lock durations (past time, min/max violation) | Covered | `test_lock_past_time_panics`, `test_lock_duration_boundary_failures` | +| **Lock State** | Withdrawing immature locks (early withdrawal) | Covered | `test_withdraw_immature_lock_fails`, `test_early_lock_withdrawal_fails`, `test_can_withdraw_before_unlock` | +| **Lock State** | Extending or withdrawing a spent lock | Covered | `test_withdraw_repeated_lock_fails`, `test_extend_withdrawn_lock_fails`, `error_code_5002_lock_already_withdrawn` | +| **Rollback** | State remains unchanged after any contract error | Covered | `test_state_remains_consistent_after_failed_lock`, `test_state_consistency_after_failed_token_transfer`, `test_failed_withdraw_does_not_change_available_balance_panics` | ## Initialization @@ -43,7 +58,7 @@ Source: `contracts/savings_vault/src/test/` (`mod.rs`, `initialization.rs`, | `withdraw` emits an event | Covered | `test_withdraw_emits_event` | | Withdrawals near `i128::MAX` don't overflow | Covered | `test_withdraw_i128_max_after_deposit_succeeds`, `test_withdraw_over_large_balance_does_not_mutate`, `test_withdraw_partial_from_large_balance_preserves_remainder`, `test_large_withdraw_spans_available_and_matured_locks` | | `withdraw_lock` (withdraw a single matured lock by ID) | Covered | `test_withdraw_matured_lock_success`, `test_withdraw_immature_lock_fails`, `test_withdraw_nonexistent_lock_fails`, `test_withdraw_repeated_lock_fails`, `test_withdraw_wrong_user_lock_fails`, `test_unauthorized_withdraw_lock_fails` | -| `withdraw_lock` emits an event | **Gap** | No dedicated `test_withdraw_lock_emits_event`-style test, unlike `deposit`/`withdraw`/`lock_funds`. The event is emitted in `lib.rs` but not asserted. | +| `withdraw_lock` emits an event | Covered | `test_withdraw_lock_emits_event` | ## Locking @@ -80,14 +95,8 @@ Source: `contracts/savings_vault/src/test/` (`mod.rs`, `initialization.rs`, ## Known Test Gaps -- **`withdraw_lock` event emission is not asserted.** `deposit`, `withdraw`, - `lock_funds`, and `initialize` each have a `*_emits_event` test; `withdraw_lock` - does not, even though `lib.rs` publishes an event for it. - **`list_locks` pagination edge cases.** Zero-limit and out-of-range-offset behaviour for `list_locks` isn't explicitly covered. -- **No fuzz/property-based tests.** Coverage relies on hand-written cases - (including a table-driven test in `balance_conservation.rs`) rather than - randomized or property-based testing. ## Additional Notes