Feat/rust fix

contracts/teachlink/src/repository/THREAD_SAFETY.md

@@ -0,0 +1,113 @@
+# Escrow Repository Thread-Safety Documentation
+
+## Overview
+
+The escrow repository has been updated to prevent race conditions during concurrent approval operations. This document explains the thread-safety guarantees and proper usage patterns.
+
+## Race Condition Issue
+
+**Previous Implementation**: The original code had a race condition when multiple approvals occurred concurrently:
+
+1. Thread A reads escrow approval count
+2. Thread B reads escrow approval count (same value)
+3. Thread A increments and saves (+1)
+4. Thread B increments and saves (+1, but from old value)
+
+**Result**: Only one increment instead of two, leading to inconsistent state.
+
+## Solution
+
+### Atomic Approval Method
+
+The `EscrowAggregateRepository::approve_escrow()` method provides atomic approval operations:
+
+```rust
+pub fn approve_escrow(&self, escrow_id: u64, signer: &Address) -> Result<u32, StorageError>
+```
+
+This method:
+1. **Validates** the signer is authorized for the escrow
+2. **Checks** if already approved (prevents duplicates)
+3. **Records** the approval atomically
+4. **Increments** the approval count atomically
+5. **Returns** the new approval count
+
+### Thread-Safety Guarantees
+
+- **Atomicity**: All approval operations are performed in a single atomic transaction
+- **Idempotency**: Multiple approval attempts by the same signer have no effect beyond the first
+- **Consistency**: Approval count always reflects actual unique approvals
+- **Isolation**: Concurrent approvals don't interfere with each other
+
+## Usage Examples
+
+### Basic Approval
+```rust
+use crate::repository::EscrowAggregateRepository;
+
+let repo = EscrowAggregateRepository::new(&env);
+
+// Approve escrow for a signer
+match repo.approve_escrow(escrow_id, &signer_address) {
+    Ok(new_count) => println!("Approval recorded. New count: {}", new_count),
+    Err(StorageError::AlreadyExists) => println!("Already approved"),
+    Err(StorageError::Unauthorized) => println!("Signer not authorized"),
+    Err(StorageError::NotFound) => println!("Escrow not found"),
+    Err(_) => println!("Other error"),
+}
+```
+
+### Check Approval Status
+```rust
+if repo.has_approved(escrow_id, &signer_address) {
+    println!("Signer has already approved");
+}
+```
+
+### Get All Approvals
+```rust
+let approvals = repo.get_escrow_approvals(escrow_id)?;
+println!("Approved by {} signers", approvals.len());
+```
+
+## Testing
+
+Comprehensive concurrency tests are provided in `concurrency_tests.rs`:
+
+- `test_atomic_escrow_approval_prevents_race_conditions`: Validates sequential approvals
+- `test_duplicate_approval_prevention`: Ensures idempotent operations
+- `test_unauthorized_signer_rejection`: Validates authorization checks
+- `test_approval_state_consistency`: Verifies state consistency across operations
+
+## Migration Guide
+
+**Before** (unsafe):
+```rust
+// Separate operations - race condition prone
+if !approval_repo.has_approved(&key) {
+    approval_repo.approve(&key)?;
+    escrow_repo.increment_approval_count(escrow_id)?;
+}
+```
+
+**After** (thread-safe):
+```rust
+// Single atomic operation
+let new_count = aggregate_repo.approve_escrow(escrow_id, &signer)?;
+```
+
+## Error Handling
+
+The atomic approval method returns specific errors:
+
+- `StorageError::NotFound`: Escrow doesn't exist
+- `StorageError::Unauthorized`: Signer is not authorized for this escrow
+- `StorageError::AlreadyExists`: Signer has already approved
+- Other storage errors for underlying issues
+
+## Performance Considerations
+
+- Atomic operations may have slightly higher overhead than separate calls
+- The idempotency check prevents unnecessary storage operations
+- Authorization validation happens before storage operations
+- All operations are O(1) for storage access

contracts/teachlink/src/repository/concurrency_tests.rs

@@ -0,0 +1,279 @@
+//! Concurrency Tests for Escrow Repository
+//!
+//! This module contains tests specifically designed to validate thread-safety
+//! and race condition prevention in the escrow approval system.
+
+use crate::repository::escrow_repository::EscrowAggregateRepository;
+use crate::repository::StorageError;
+use crate::types::{Escrow, EscrowRole, EscrowSigner, EscrowStatus};
+use soroban_sdk::{Address, Env, Vec};
+
+#[test]
+fn test_atomic_escrow_approval_prevents_race_conditions() {
+    let env = Env::default();
+    let repo = EscrowAggregateRepository::new(&env);
+
+    let depositor = Address::generate(&env);
+    let beneficiary = Address::generate(&env);
+    let signer1 = Address::generate(&env);
+    let signer2 = Address::generate(&env);
+    let token = Address::generate(&env);
+
+    // Create escrow with multiple signers
+    let mut signers = Vec::new(&env);
+    signers.push_back(EscrowSigner {
+        address: signer1.clone(),
+        role: EscrowRole::Primary,
+        weight: 1,
+    });
+    signers.push_back(EscrowSigner {
+        address: signer2.clone(),
+        role: EscrowRole::Secondary,
+        weight: 1,
+    });
+
+    let escrow = Escrow {
+        id: 1,
+        depositor: depositor.clone(),
+        beneficiary: beneficiary.clone(),
+        token: token.clone(),
+        amount: 1000,
+        signers: signers.clone(),
+        threshold: 2,
+        approval_count: 0,
+        release_time: None,
+        refund_time: None,
+        arbitrator: depositor.clone(),
+        status: EscrowStatus::Pending,
+        created_at: env.ledger().timestamp(),
+        dispute_reason: None,
+    };
+
+    repo.escrows
+        .save_escrow(&escrow)
+        .expect("Should save escrow");
+
+    // Test sequential approvals work correctly
+    let count1 = repo
+        .approve_escrow(1, &signer1)
+        .expect("First approval should succeed");
+    assert_eq!(count1, 1);
+
+    let count2 = repo
+        .approve_escrow(1, &signer2)
+        .expect("Second approval should succeed");
+    assert_eq!(count2, 2);
+
+    // Verify final state
+    let updated_escrow = repo.escrows.get_escrow(1).unwrap();
+    assert_eq!(updated_escrow.approval_count, 2);
+}
+
+#[test]
+fn test_duplicate_approval_prevention() {
+    let env = Env::default();
+    let repo = EscrowAggregateRepository::new(&env);
+
+    let depositor = Address::generate(&env);
+    let beneficiary = Address::generate(&env);
+    let signer = Address::generate(&env);
+    let token = Address::generate(&env);
+
+    let mut signers = Vec::new(&env);
+    signers.push_back(EscrowSigner {
+        address: signer.clone(),
+        role: EscrowRole::Primary,
+        weight: 1,
+    });
+
+    let escrow = Escrow {
+        id: 1,
+        depositor: depositor.clone(),
+        beneficiary: beneficiary.clone(),
+        token: token.clone(),
+        amount: 1000,
+        signers: signers.clone(),
+        threshold: 1,
+        approval_count: 0,
+        release_time: None,
+        refund_time: None,
+        arbitrator: depositor.clone(),
+        status: EscrowStatus::Pending,
+        created_at: env.ledger().timestamp(),
+        dispute_reason: None,
+    };
+
+    repo.escrows
+        .save_escrow(&escrow)
+        .expect("Should save escrow");
+
+    // First approval should succeed
+    let count1 = repo
+        .approve_escrow(1, &signer)
+        .expect("First approval should succeed");
+    assert_eq!(count1, 1);
+
+    // Subsequent approvals should fail
+    for _ in 0..5 {
+        let result = repo.approve_escrow(1, &signer);
+        assert!(
+            matches!(result, Err(StorageError::AlreadyExists)),
+            "Duplicate approval should fail with AlreadyExists"
+        );
+    }
+
+    // Count should still be 1
+    let final_escrow = repo.escrows.get_escrow(1).unwrap();
+    assert_eq!(final_escrow.approval_count, 1);
+}
+
+#[test]
+fn test_unauthorized_signer_rejection() {
+    let env = Env::default();
+    let repo = EscrowAggregateRepository::new(&env);
+
+    let depositor = Address::generate(&env);
+    let beneficiary = Address::generate(&env);
+    let authorized_signer = Address::generate(&env);
+    let unauthorized_signer = Address::generate(&env);
+    let token = Address::generate(&env);
+
+    let mut signers = Vec::new(&env);
+    signers.push_back(EscrowSigner {
+        address: authorized_signer.clone(),
+        role: EscrowRole::Primary,
+        weight: 1,
+    });
+
+    let escrow = Escrow {
+        id: 1,
+        depositor: depositor.clone(),
+        beneficiary: beneficiary.clone(),
+        token: token.clone(),
+        amount: 1000,
+        signers: signers.clone(),
+        threshold: 1,
+        approval_count: 0,
+        release_time: None,
+        refund_time: None,
+        arbitrator: depositor.clone(),
+        status: EscrowStatus::Pending,
+        created_at: env.ledger().timestamp(),
+        dispute_reason: None,
+    };
+
+    repo.escrows
+        .save_escrow(&escrow)
+        .expect("Should save escrow");
+
+    // Authorized approval should work
+    let result = repo.approve_escrow(1, &authorized_signer);
+    assert!(
+        result.is_ok(),
+        "Authorized signer should be able to approve"
+    );
+
+    // Unauthorized approval should fail
+    let result = repo.approve_escrow(1, &unauthorized_signer);
+    assert!(
+        matches!(result, Err(StorageError::Unauthorized)),
+        "Unauthorized signer should be rejected"
+    );
+}
+
+#[test]
+fn test_nonexistent_escrow_handling() {
+    let env = Env::default();
+    let repo = EscrowAggregateRepository::new(&env);
+
+    let signer = Address::generate(&env);
+
+    // Attempting to approve non-existent escrow should fail
+    let result = repo.approve_escrow(999, &signer);
+    assert!(
+        matches!(result, Err(StorageError::NotFound)),
+        "Approving non-existent escrow should fail"
+    );
+}
+
+#[test]
+fn test_approval_state_consistency() {
+    let env = Env::default();
+    let repo = EscrowAggregateRepository::new(&env);
+
+    let depositor = Address::generate(&env);
+    let beneficiary = Address::generate(&env);
+    let signer1 = Address::generate(&env);
+    let signer2 = Address::generate(&env);
+    let signer3 = Address::generate(&env);
+    let token = Address::generate(&env);
+
+    let mut signers = Vec::new(&env);
+    signers.push_back(EscrowSigner {
+        address: signer1.clone(),
+        role: EscrowRole::Primary,
+        weight: 1,
+    });
+    signers.push_back(EscrowSigner {
+        address: signer2.clone(),
+        role: EscrowRole::Secondary,
+        weight: 1,
+    });
+    signers.push_back(EscrowSigner {
+        address: signer3.clone(),
+        role: EscrowRole::Secondary,
+        weight: 1,
+    });
+
+    let escrow = Escrow {
+        id: 1,
+        depositor: depositor.clone(),
+        beneficiary: beneficiary.clone(),
+        token: token.clone(),
+        amount: 1000,
+        signers: signers.clone(),
+        threshold: 2,
+        approval_count: 0,
+        release_time: None,
+        refund_time: None,
+        arbitrator: depositor.clone(),
+        status: EscrowStatus::Pending,
+        created_at: env.ledger().timestamp(),
+        dispute_reason: None,
+    };
+
+    repo.escrows
+        .save_escrow(&escrow)
+        .expect("Should save escrow");
+
+    // Approve with signer1
+    repo.approve_escrow(1, &signer1).expect("Should approve");
+
+    // Check state consistency
+    assert!(repo.has_approved(1, &signer1));
+    assert!(!repo.has_approved(1, &signer2));
+    assert!(!repo.has_approved(1, &signer3));
+
+    let escrow_state = repo.escrows.get_escrow(1).unwrap();
+    assert_eq!(escrow_state.approval_count, 1);
+
+    let approvals = repo.get_escrow_approvals(1).unwrap();
+    assert_eq!(approvals.len(), 1);
+    assert!(approvals.contains(&signer1));
+
+    // Approve with signer2
+    repo.approve_escrow(1, &signer2).expect("Should approve");
+
+    // Check state consistency again
+    assert!(repo.has_approved(1, &signer1));
+    assert!(repo.has_approved(1, &signer2));
+    assert!(!repo.has_approved(1, &signer3));
+
+    let escrow_state = repo.escrows.get_escrow(1).unwrap();
+    assert_eq!(escrow_state.approval_count, 2);
+
+    let approvals = repo.get_escrow_approvals(1).unwrap();
+    assert_eq!(approvals.len(), 2);
+    assert!(approvals.contains(&signer1));
+    assert!(approvals.contains(&signer2));
+}