diff --git a/README_CANCEL_VAULT.md b/README_CANCEL_VAULT.md index 9bbf181..d871015 100644 --- a/README_CANCEL_VAULT.md +++ b/README_CANCEL_VAULT.md @@ -1,23 +1,44 @@ -Cancel Vault: Design, Security Notes, and Tests +# Cancel Vault: Design, Security Notes, and Tests -Summary -- `cancel_vault` requires the caller to be the vault `creator` and only allows cancellation when the vault `status` is `Active`. -- On cancel it refunds the vault's stored token balance (simulated) back to the creator by clearing the contract-held balance and emitting a `vault_cancelled` event with the refunded amount. +## Summary -Behavior & rules -- Cancellation allowed only when `status == Active` (i.e., before validation/completion). -- Caller must be the `creator` (enforced via `Address::require_auth`). -- Refund is performed against a simulated per-vault balance stored in persistent storage under key `("vault_balance", vault_id)`. +- `cancel_vault` requires the vault `creator` to authorize the call. +- Cancellation is only allowed while the vault status is `Active`. +- On cancel, the contract performs a real Soroban token transfer from the contract escrow balance back to the creator, then marks the vault as `Cancelled`. +- A successful cancellation emits `vault_cancelled`. -Security notes -- Real token transfer: the current implementation simulates token movement by updating contract-owned balance storage. For production, replace this with a real token transfer using the Soroban token client and ensure the contract has allowance/escrowed tokens before creating the vault. -- Reentrancy: token transfers must be performed using the recommended Soroban token client patterns; consider checks-effects-interactions ordering (we clear the stored balance and set status before emitting events). -- Authorization: `creator.require_auth()` ensures only the vault owner can cancel. +## Behavior and Rules -Tests -- Unit tests cover: successful cancel by creator, unauthorized cancel (panics), and cancel when status is Completed (returns false). -- A helper `get_vault_balance` exposes the simulated vault balance for assertions in tests. +- `create_vault` transfers the escrow amount from the creator to the current contract address. +- `cancel_vault` transfers exactly `vault.amount` from the current contract address back to `vault.creator`. +- Funds are not sent to `success_destination` or `failure_destination` during cancellation. +- Cancelling a `Completed`, `Failed`, or already `Cancelled` vault returns `Error::VaultNotActive`. +- Cancelling a nonexistent vault returns `Error::VaultNotFound`. -Next steps -- Integrate with a real token contract: add a `token_contract: Address` per vault or instance-level token address and call the token `transfer` to move funds. -- Add more tests that register and interact with a real token contract in test `Env` to assert balances on ledger entries. +## Security Notes + +- Authorization: `vault.creator.require_auth()` ensures only the vault owner can cancel. +- Refund target: the caller cannot choose a refund recipient; funds always return to the stored `vault.creator`. +- Escrow accounting: tests assert the creator balance increases by exactly `vault.amount` and the contract token balance returns to its pre-create level after cancellation. +- Terminal-state safety: cancellation is rejected after release, redirect, or a prior cancellation, preventing double refunds. + +## Tests + +The lifecycle integration tests register a real Soroban token contract in `Env` and assert ledger balances with `TokenClient::balance`. + +Run the focused coverage with: + +```bash +cargo test test_cancel_vault_refunds_creator_and_empties_contract_escrow +cargo test test_cancel_vault_rejects_completed_failed_and_cancelled_vaults +``` + +The cancel coverage checks: + +- creator balance decreases by `vault.amount` on create +- contract escrow balance increases by `vault.amount` on create +- creator balance increases by `vault.amount` on cancel +- contract escrow returns to its pre-create balance on cancel +- success and failure destinations remain untouched +- `vault_cancelled` is emitted +- non-active vaults reject cancellation with `Error::VaultNotActive` diff --git a/TESTING_GUIDE.md b/TESTING_GUIDE.md index 72ba285..6b9d3f2 100644 --- a/TESTING_GUIDE.md +++ b/TESTING_GUIDE.md @@ -97,6 +97,20 @@ cargo test test_vault_data_integrity cargo test test_sequential_operations ``` +### 5. Cancel Vault Refunds + +`tests/lifecycle.rs` covers the real-token refund path for `cancel_vault`. + +```bash +cargo test test_cancel_vault_refunds_creator_and_empties_contract_escrow +cargo test test_cancel_vault_rejects_completed_failed_and_cancelled_vaults +``` + +These tests assert that cancellation transfers exactly `vault.amount` from the +contract escrow back to the creator, leaves success and failure destinations +untouched, emits `vault_cancelled`, and rejects cancellation once a vault is in a +terminal state. + ## Coverage Reports ### Generate HTML Report diff --git a/src/lib.rs b/src/lib.rs index 9893393..091ac8a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -982,6 +982,35 @@ mod tests { assert_eq!(vault.status, VaultStatus::Cancelled); } + #[test] + fn test_cancel_vault_emits_cancelled_event() { + let setup = TestSetup::new(); + let client = setup.client(); + + setup.env.ledger().set_timestamp(setup.start_timestamp); + let vault_id = setup.create_default_vault(); + + client.cancel_vault(&vault_id, &setup.usdc_token); + + let found_cancel_event = setup + .env + .events() + .all() + .iter() + .any(|(contract, topics, _)| { + if contract != setup.contract_id { + return false; + } + + let event_name: Symbol = topics.get(0).unwrap().try_into_val(&setup.env).unwrap(); + let event_vault_id: u32 = topics.get(1).unwrap().try_into_val(&setup.env).unwrap(); + event_name == Symbol::new(&setup.env, "vault_cancelled") + && event_vault_id == vault_id + }); + + assert!(found_cancel_event, "vault_cancelled event must be emitted"); + } + // ----------------------------------------------------------------------- // More upstream tests migrated // ----------------------------------------------------------------------- diff --git a/tests/lifecycle.rs b/tests/lifecycle.rs index 7079583..054446e 100644 --- a/tests/lifecycle.rs +++ b/tests/lifecycle.rs @@ -6,7 +6,7 @@ use soroban_sdk::{ Address, BytesN, Env, }; -use disciplr_vault::{DisciplrVault, DisciplrVaultClient, VaultStatus, MIN_AMOUNT}; +use disciplr_vault::{DisciplrVault, DisciplrVaultClient, Error, VaultStatus, MIN_AMOUNT}; fn setup() -> ( Env, @@ -30,6 +30,16 @@ fn setup() -> ( (env, client, usdc_addr, usdc_asset, usdc_token_client) } +fn assert_contract_error( + result: Result>, + expected: Error, +) { + match result { + Err(Ok(actual)) => assert_eq!(actual, expected), + other => panic!("unexpected result: {other:?}"), + } +} + #[test] fn test_full_lifecycle_success() { let (env, client, usdc, usdc_asset, usdc_token) = setup(); @@ -116,3 +126,128 @@ fn test_full_lifecycle_failure_redirection() { assert_eq!(final_state.status, VaultStatus::Failed); assert_eq!(usdc_token.balance(&failure_dest), MIN_AMOUNT); } + +#[test] +fn test_cancel_vault_refunds_creator_and_empties_contract_escrow() { + let (env, client, usdc, usdc_asset, usdc_token) = setup(); + + let creator = Address::generate(&env); + let success_dest = Address::generate(&env); + let failure_dest = Address::generate(&env); + let now = 1_700_000_000u64; + env.ledger().set_timestamp(now); + + usdc_asset.mint(&creator, &MIN_AMOUNT); + let creator_before_create = usdc_token.balance(&creator); + let contract_before_create = usdc_token.balance(&client.address); + + let vault_id = client.create_vault( + &usdc, + &creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[2u8; 32]), + &None, + &success_dest, + &failure_dest, + ); + + assert_eq!( + usdc_token.balance(&creator), + creator_before_create - MIN_AMOUNT + ); + assert_eq!( + usdc_token.balance(&client.address), + contract_before_create + MIN_AMOUNT + ); + + let creator_before_cancel = usdc_token.balance(&creator); + let contract_before_cancel = usdc_token.balance(&client.address); + + let result = client.cancel_vault(&vault_id, &usdc); + + assert!(result); + assert_eq!( + usdc_token.balance(&creator), + creator_before_cancel + MIN_AMOUNT + ); + assert_eq!( + usdc_token.balance(&client.address), + contract_before_cancel - MIN_AMOUNT + ); + assert_eq!(usdc_token.balance(&client.address), contract_before_create); + assert_eq!(usdc_token.balance(&success_dest), 0); + assert_eq!(usdc_token.balance(&failure_dest), 0); + + let final_state = client.get_vault_state(&vault_id).unwrap(); + assert_eq!(final_state.status, VaultStatus::Cancelled); +} + +#[test] +fn test_cancel_vault_rejects_completed_failed_and_cancelled_vaults() { + let (env, client, usdc, usdc_asset, _) = setup(); + let now = 1_700_000_000u64; + env.ledger().set_timestamp(now); + + let completed_creator = Address::generate(&env); + let failed_creator = Address::generate(&env); + let cancelled_creator = Address::generate(&env); + usdc_asset.mint(&completed_creator, &MIN_AMOUNT); + usdc_asset.mint(&failed_creator, &MIN_AMOUNT); + usdc_asset.mint(&cancelled_creator, &MIN_AMOUNT); + + let completed_id = client.create_vault( + &usdc, + &completed_creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[3u8; 32]), + &None, + &Address::generate(&env), + &Address::generate(&env), + ); + client.validate_milestone(&completed_id); + client.release_funds(&completed_id, &usdc); + assert_contract_error( + client.try_cancel_vault(&completed_id, &usdc), + Error::VaultNotActive, + ); + + let failed_id = client.create_vault( + &usdc, + &failed_creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[4u8; 32]), + &None, + &Address::generate(&env), + &Address::generate(&env), + ); + env.ledger().set_timestamp(now + 86_401); + client.redirect_funds(&failed_id, &usdc); + assert_contract_error( + client.try_cancel_vault(&failed_id, &usdc), + Error::VaultNotActive, + ); + + env.ledger().set_timestamp(now); + let cancelled_id = client.create_vault( + &usdc, + &cancelled_creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[5u8; 32]), + &None, + &Address::generate(&env), + &Address::generate(&env), + ); + client.cancel_vault(&cancelled_id, &usdc); + assert_contract_error( + client.try_cancel_vault(&cancelled_id, &usdc), + Error::VaultNotActive, + ); +} diff --git a/tests/proptest_timestamps.rs b/tests/proptest_timestamps.rs index d8e37e8..bd89ed3 100644 --- a/tests/proptest_timestamps.rs +++ b/tests/proptest_timestamps.rs @@ -279,7 +279,6 @@ fn edge_start_eq_now_succeeds() { assert_eq!(vault.end_timestamp, end); } - #[test] fn edge_start_eq_end_rejected() { let (env, client, usdc, usdc_asset) = setup();