test: add failed operation state rollback tests

contracts/savings_vault/src/test/balance_conservation.rs

@@ -147,6 +147,13 @@ fn snapshot(client: &SavingsVaultClient, user: &Address) -> (i128, i128) {
     (client.get_balance(user), client.get_locked_balance(user))
 }
 
+fn token_snapshot(f: &Fixture) -> (i128, i128) {
+    (
+        f.token_client.balance(&f.contract_id),
+        f.token_client.balance(&f.user),
+    )
+}
+
 /// Run one operation sequence, checking conservation after every step.
 ///
 /// Returns the final expected total so callers can make extra assertions if needed.
@@ -160,6 +167,7 @@ fn run_sequence(ops: &[(Op, Expect)]) -> i128 {
 
     for (step, (op, expect)) in ops.iter().enumerate() {
         let before = snapshot(&f.client, &f.user);
+        let tokens_before = token_snapshot(&f);
 
         match (op, expect) {
             (Op::Deposit(amount), Expect::Ok) => {
@@ -177,6 +185,11 @@ fn run_sequence(ops: &[(Op, Expect)]) -> i128 {
                     before,
                     "step {step}: failed deposit must not mutate balances"
                 );
+                assert_eq!(
+                    token_snapshot(&f),
+                    tokens_before,
+                    "step {step}: failed deposit must not move tokens"
+                );
             }
             (Op::Withdraw(amount), Expect::Ok) => {
                 f.client.withdraw(&f.user, amount);
@@ -193,6 +206,11 @@ fn run_sequence(ops: &[(Op, Expect)]) -> i128 {
                     before,
                     "step {step}: failed withdraw must not mutate balances"
                 );
+                assert_eq!(
+                    token_snapshot(&f),
+                    tokens_before,
+                    "step {step}: failed withdraw must not move tokens"
+                );
             }
             (
                 Op::Lock {
@@ -223,6 +241,11 @@ fn run_sequence(ops: &[(Op, Expect)]) -> i128 {
                     before,
                     "step {step}: failed lock must not mutate balances"
                 );
+                assert_eq!(
+                    token_snapshot(&f),
+                    tokens_before,
+                    "step {step}: failed lock must not move tokens"
+                );
             }
             (Op::WithdrawLock(idx), Expect::Ok) => {
                 let lock_id = lock_ids[*idx];
@@ -231,7 +254,26 @@ fn run_sequence(ops: &[(Op, Expect)]) -> i128 {
                 expected_total -= lock_amt;
             }
             (Op::WithdrawLock(_), Expect::Err) => {
-                panic!("step {step}: WithdrawLock cannot fail for valid indices");
+                let lock_id = if *idx < lock_ids.len() {
+                    lock_ids[*idx]
+                } else {
+                    u64::MAX
+                };
+                let res = f.client.try_withdraw_lock(&f.user, &lock_id);
+                assert!(
+                    res.is_err(),
+                    "step {step}: withdraw_lock({lock_id}) was expected to fail"
+                );
+                assert_eq!(
+                    snapshot(&f.client, &f.user),
+                    before,
+                    "step {step}: failed withdraw_lock must not mutate balances"
+                );
+                assert_eq!(
+                    token_snapshot(&f),
+                    tokens_before,
+                    "step {step}: failed withdraw_lock must not move tokens"
+                );
             }
             (Op::SetTime(ts), Expect::Ok) => {
                 set_ledger_timestamp(&f.env, *ts);
@@ -495,6 +537,37 @@ fn conservation_invalid_locks_do_not_mutate() {
     assert_eq!(total, 250);
 }
 
+/// Attempting to withdraw an unmatured lock must fail and leave balances unchanged.
+#[test]
+fn conservation_failed_withdraw_lock_does_not_mutate() {
+    run_sequence(&[
+        (Op::Deposit(500), Expect::Ok),
+        (
+            Op::Lock {
+                amount: 400,
+                unlock_time: 20_000,
+            },
+            Expect::Ok,
+        ),
+        // Lock is not yet matured; withdraw_lock must fail and roll back.
+        (Op::WithdrawLock(0), Expect::Err),
+        (Op::Withdraw(100), Expect::Ok),
+        // After maturity, the lock can be withdrawn.
+        (Op::SetTime(20_000), Expect::Ok),
+        (Op::WithdrawLock(0), Expect::Ok),
+    ]);
+}
+
+/// Withdrawing a non-existent lock must fail and leave balances unchanged.
+#[test]
+fn conservation_invalid_withdraw_lock_id_does_not_mutate() {
+    run_sequence(&[
+        (Op::Deposit(100), Expect::Ok),
+        (Op::WithdrawLock(999), Expect::Err),
+        (Op::Withdraw(100), Expect::Ok),
+    ]);
+}
+
 /// Withdraw that would touch only locked (unmatured) funds must fail and not mutate.
 #[test]
 fn conservation_withdraw_exceeds_available_while_locked_does_not_mutate() {

contracts/savings_vault/src/test/lock_atomicity.rs

@@ -165,3 +165,95 @@ fn test_failed_lock_creates_no_partial_record() {
     assert_eq!(client.get_balance(&user), 900);
     assert_eq!(client.get_locked_balance(&user), 100);
 }
+
+// ---------------------------------------------------------------------------
+// Failed withdrawal leaves state unchanged
+// ---------------------------------------------------------------------------
+
+/// A withdrawal for more than the available balance is rejected and leaves balances untouched.
+#[test]
+fn test_failed_withdrawal_insufficient_balance_leaves_state_intact() {
+    let env = test_env();
+    let (_admin, client) = init_with_admin(&env);
+    let user = Address::generate(&env);
+
+    env.ledger().set_timestamp(1_000);
+    fund(&client, &user, 1_000);
+
+    let balance_before = client.get_balance(&user);
+    let locked_before = client.get_locked_balance(&user);
+
+    let res = client.try_withdraw(&user, &(balance_before + 1));
+    assert!(res.is_err());
+
+    assert_eq!(client.get_balance(&user), balance_before);
+    assert_eq!(client.get_locked_balance(&user), locked_before);
+}
+
+/// A zero-amount withdrawal is rejected and leaves balances untouched.
+#[test]
+fn test_failed_withdrawal_zero_amount_leaves_state_intact() {
+    let env = test_env();
+    let (_admin, client) = init_with_admin(&env);
+    let user = Address::generate(&env);
+
+    env.ledger().set_timestamp(1_000);
+    fund(&client, &user, 1_000);
+
+    let balance_before = client.get_balance(&user);
+    let locked_before = client.get_locked_balance(&user);
+
+    let res = client.try_withdraw(&user, &0);
+    assert!(res.is_err());
+
+    assert_eq!(client.get_balance(&user), balance_before);
+    assert_eq!(client.get_locked_balance(&user), locked_before);
+}
+
+/// A failed withdrawal does not alter existing lock records or locked balances.
+#[test]
+fn test_failed_withdrawal_preserves_locks() {
+    let env = test_env();
+    let (_admin, client) = init_with_admin(&env);
+    let user = Address::generate(&env);
+
+    env.ledger().set_timestamp(1_000);
+    fund(&client, &user, 1_000);
+    let id = client.lock_funds(&user, &300, &(env.ledger().timestamp() + 100));
+    let lock_before = client.get_lock(&user, &id).expect("lock should exist");
+
+    let balance_before = client.get_balance(&user);
+    let res = client.try_withdraw(&user, &(balance_before + 1));
+    assert!(res.is_err());
+
+    assert_eq!(client.get_balance(&user), balance_before);
+    assert_eq!(client.get_locked_balance(&user), 300);
+    let lock_after = client.get_lock(&user, &id).expect("lock should still exist");
+    assert_eq!(lock_after.amount, lock_before.amount);
+    assert_eq!(lock_after.withdrawn, lock_before.withdrawn);
+}
+
+// ---------------------------------------------------------------------------
+// Failed deposit leaves state unchanged
+// ---------------------------------------------------------------------------
+
+/// A zero-amount deposit is rejected and leaves balances untouched.
+#[test]
+fn test_failed_deposit_zero_amount_leaves_state_intact() {
+    let env = test_env();
+    let (_admin, client) = init_with_admin(&env);
+    let user = Address::generate(&env);
+
+    env.ledger().set_timestamp(1_000);
+    fund(&client, &user, 1_000);
+
+    let balance_before = client.get_balance(&user);
+    let locked_before = client.get_locked_balance(&user);
+
+    let res = client.try_deposit(&user, &0);
+    assert!(res.is_err());
+
+    assert_eq!(client.get_balance(&user), balance_before);
+    assert_eq!(client.get_locked_balance(&user), locked_before);
+}
+

contracts/savings_vault/src/test/multi_lock_invariants.rs

@@ -226,46 +226,74 @@ fn multi_lock_failed_operations_do_not_mutate() {
     assert_conserved(&client, &user, expected);
 
     // Create 3 locks: available=400, locked=600
-    client.lock_funds(&user, &200, &3_000);
+    let id1 = client.lock_funds(&user, &200, &3_000);
     client.lock_funds(&user, &300, &5_000);
     client.lock_funds(&user, &100, &7_000);
     assert_conserved(&client, &user, expected);
 
     let before = snapshot(&client, &user);
+    let locks_before = client.list_locks(&user, &0u32, &50u32);
+
+    macro_rules! assert_unchanged {
+        () => {
+            assert_eq!(snapshot(&client, &user), before);
+            assert_eq!(client.list_locks(&user, &0u32, &50u32), locks_before);
+        }
+    }
 
     // Lock more than available
     let res = client.try_lock_funds(&user, &401, &10_000);
     assert!(res.is_err());
-    assert_eq!(snapshot(&client, &user), before);
+    assert_unchanged!();
 
     // Lock zero
     let res = client.try_lock_funds(&user, &0, &10_000);
     assert!(res.is_err());
-    assert_eq!(snapshot(&client, &user), before);
+    assert_unchanged!();
+
+    // Lock negative amount
+    let res = client.try_lock_funds(&user, &-50, &10_000);
+    assert!(res.is_err());
+    assert_unchanged!();
 
     // Lock with past unlock
     let res = client.try_lock_funds(&user, &50, &500);
     assert!(res.is_err());
-    assert_eq!(snapshot(&client, &user), before);
+    assert_unchanged!();
 
     // Withdraw more than available
     let res = client.try_withdraw(&user, &401);
     assert!(res.is_err());
-    assert_eq!(snapshot(&client, &user), before);
+    assert_unchanged!();
 
     // Withdraw zero
     let res = client.try_withdraw(&user, &0);
     assert!(res.is_err());
-    assert_eq!(snapshot(&client, &user), before);
+    assert_unchanged!();
+
+    // Withdraw negative amount
+    let res = client.try_withdraw(&user, &-10);
+    assert!(res.is_err());
+    assert_unchanged!();
 
     // Deposit zero / negative
     let res = client.try_deposit(&user, &0);
     assert!(res.is_err());
-    assert_eq!(snapshot(&client, &user), before);
+    assert_unchanged!();
 
     let res = client.try_deposit(&user, &-10);
     assert!(res.is_err());
-    assert_eq!(snapshot(&client, &user), before);
+    assert_unchanged!();
+
+    // Withdraw_lock on unmatured lock
+    let res = client.try_withdraw_lock(&user, &id1);
+    assert!(res.is_err());
+    assert_unchanged!();
+
+    // Withdraw_lock on nonexistent lock
+    let res = client.try_withdraw_lock(&user, &999);
+    assert!(res.is_err());
+    assert_unchanged!();
 
     // State still intact
     assert_lock_sum_consistency(&env, &client, &user);
@@ -626,6 +654,14 @@ fn multi_lock_deterministic_sequence_invariants() {
             user_idx: 2,
             amount: 30_000,
         },
+        Operation::FailDeposit {
+            user_idx: 0,
+            amount: 0,
+        },
+        Operation::FailDeposit {
+            user_idx: 1,
+            amount: -50,
+        },
         Operation::Lock {
             user_idx: 0,
             amount: 2_000,

contracts/savings_vault/src/test/negative_paths.rs

@@ -219,6 +219,79 @@ fn test_state_remains_consistent_after_failed_lock() {
     assert_eq!(client.get_locked_balance(&user), initial_locked, "Locked balance should not change after failed lock");
 }
 
+#[test]
+fn test_state_remains_consistent_after_failed_withdrawal() {
+    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);
+    let initial_balance = client.get_balance(&user);
+    let initial_locked = client.get_locked_balance(&user);
+
+    // Attempt to withdraw the lock before it matures
+    set_ledger_timestamp(&env, 4999);
+    let res = client.try_withdraw_lock(&user, &lock_id);
+    assert!(res.is_err(), "Early withdrawal should fail");
+
+    assert_eq!(client.get_balance(&user), initial_balance, "Balance should not change after failed withdrawal");
+    assert_eq!(client.get_locked_balance(&user), initial_locked, "Locked balance should not change after failed withdrawal");
+}
+
+#[test]
+fn test_state_remains_consistent_after_failed_withdraw() {
+    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();
+    token_admin.mint(&user, &1000);
+    client.deposit(&user, &1000);
+
+    let initial_balance = client.get_balance(&user);
+    let initial_locked = client.get_locked_balance(&user);
+    let initial_user_tokens = token_client.balance(&user);
+    let initial_vault_tokens = token_client.balance(&contract_id);
+
+    // Attempt to withdraw more than the unlocked balance
+    let res = client.try_withdraw(&user, &1001);
+    assert!(res.is_err(), "Withdraw should fail due to insufficient unlocked balance");
+
+    assert_eq!(client.get_balance(&user), initial_balance, "Balance should not change after failed withdraw");
+    assert_eq!(client.get_locked_balance(&user), initial_locked, "Locked balance should not change after failed withdraw");
+    assert_eq!(token_client.balance(&user), initial_user_tokens, "User tokens should not change after failed withdraw");
+    assert_eq!(token_client.balance(&contract_id), initial_vault_tokens, "Vault tokens should not change after failed withdraw");
+}
+
+#[test]
+fn test_state_remains_consistent_after_invalid_amount() {
+    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();
+    token_admin.mint(&user, &1000);
+    client.deposit(&user, &1000);
+
+    let initial_balance = client.get_balance(&user);
+    let initial_locked = client.get_locked_balance(&user);
+
+    // Invalid deposit amounts must not change user state
+    let res = client.try_deposit(&user, &0);
+    assert!(res.is_err());
+    assert_eq!(client.get_balance(&user), initial_balance, "Balance should not change after zero deposit");
+    assert_eq!(client.get_locked_balance(&user), initial_locked, "Locked balance should not change after zero deposit");
+    let res = client.try_deposit(&user, &-1);
+    assert!(res.is_err());
+    assert_eq!(client.get_balance(&user), initial_balance, "Balance should not change after negative deposit");
+    assert_eq!(client.get_locked_balance(&user), initial_locked, "Locked balance should not change after negative deposit");
+}
+
 #[test]
 fn test_state_consistency_after_failed_token_transfer() {
     let env = test_env();