feat(backend): implement consistency checks for task registration and…

contract/src/lib.rs

@@ -158,37 +158,29 @@ impl SoroTaskContract {
     /// Registers a new task in the marketplace.
     /// Returns the unique sequential ID of the registered task.
     ///
-    // ID ALLOCATION ASSUMPTIONS:
-    // - IDs are sequential integers starting from 1.
-    //   The counter is stored under DataKey::Counter and begins at 0
-    //   (unwrap_or(0)); the first register() call increments it to 1 and
-    //   returns 1.
-    // - No gaps occur under normal registration.
-    //   Each successful register() call increments the counter by exactly 1
-    //   before returning, so consecutive successful calls yield n, n+1, n+2, …
-    // - Concurrent registrations are serialized by the Soroban runtime.
-    //   Soroban executes one transaction at a time per ledger; there is no
-    //   shared-memory concurrency. Each transaction reads, increments, and
-    //   writes DataKey::Counter atomically within its own transaction context.
-    //   Two transactions in the same ledger are ordered by the protocol and
-    //   cannot interleave their storage reads/writes.
-    // - Downstream systems MUST NOT assume:
-    //   * That a cancelled task's ID will be reused — cancelled tasks are
-    //     removed from storage but the counter is never decremented.
-    //   * That IDs are contiguous after a failed (panicking) registration —
-    //     a panic rolls back the entire transaction including the counter
-    //     increment, so the counter does NOT advance on failure; the next
-    //     successful registration will receive the next value as if the
-    //     failure never happened.
-    //   * That the counter value equals the number of live tasks — tasks can
-    //     be cancelled, leaving gaps in the ID space.
-    //   * That IDs are stable across contract re-deployments — a fresh
-    //     deployment resets DataKey::Counter to 0.
+    /// # Task ID Allocation Rules
+    /// - IDs are monotonically increasing sequential integers starting at 1
+    /// - The ID counter is stored persistently and increments by exactly 1 per successful registration
+    /// - IDs are never reused, even if tasks are cancelled (counter only increments)
+    /// - Invalid registrations (e.g., interval=0) do NOT increment the counter
+    /// - No contiguity guarantee: Cancelled tasks leave gaps in the ID sequence
+    ///
+    /// # Downstream Tooling Assumptions (Safe to Make)
+    /// - All task IDs are positive integers (>=1)
+    /// - New registrations will always receive an ID larger than all previous registrations
+    /// - Each ID maps to at most one task at any point in time
+    /// - Concurrent registrations are serialized by the Soroban runtime
+    ///
+    /// # Downstream Tooling Assumptions (Do NOT Make)
+    /// - IDs are contiguous (no gaps) - gaps exist when tasks are cancelled
+    /// - IDs reset on contract upgrade - counter is persistent across upgrades
+    /// - Counter value equals number of live tasks - cancelled tasks leave gaps
+    /// - IDs are stable across contract re-deployments - fresh deployment resets counter to 0
     pub fn register(env: Env, mut config: TaskConfig) -> u64 {
         // Ensure the creator has authorized the registration
         config.creator.require_auth();
 
-        // Validate the task interval
+        // Validate the task interval (panics before counter increment, so no ID is wasted)
         if config.interval == 0 {
             panic_with_error!(&env, Error::InvalidInterval);
         }
@@ -199,24 +191,39 @@ impl SoroTaskContract {
         }
 
         config.is_active = true;
-        // Generate a unique sequential ID
+
+        // Allocate next sequential ID:
+        // 1. Fetch current counter (defaults to 0 if first registration)
+        // 2. Increment by 1 to get new ID
+        // 3. Persist updated counter BEFORE storing task to ensure atomicity
         let mut counter: u64 = env
             .storage()
             .persistent()
             .get(&DataKey::Counter)
             .unwrap_or(0);
         counter += 1;
+
+        // Consistency check: Ensure the new ID doesn't already have a task stored.
+        // This guards against counter corruption or storage inconsistencies.
+        if env
+            .storage()
+            .persistent()
+            .has(&DataKey::Task(counter))
+        {
+            panic_with_error!(&env, Error::AlreadyInitialized);
+        }
+
         env.storage().persistent().set(&DataKey::Counter, &counter);
 
-        // Store the task configuration
+        // Store the task configuration under the new ID
         env.storage()
             .persistent()
             .set(&DataKey::Task(counter), &config);
 
         // Add to the active task index for efficient monitoring.
         add_active_task_id(&env, counter);
 
-        // Emit TaskRegistered event
+        // Emit TaskRegistered event with ID and creator address
         env.events().publish(
             (Symbol::new(&env, "TaskRegistered"), Symbol::new(&env, "v1"), counter),
             config.creator.clone(),
@@ -230,6 +237,20 @@ impl SoroTaskContract {
         env.storage().persistent().get(&DataKey::Task(task_id))
     }
 
+    /// Returns the current task ID counter value.
+    ///
+    /// This is the next ID that will be assigned to a new task registration.
+    /// Useful for downstream tooling to verify ID allocation consistency:
+    /// - The counter should be >= any existing task ID
+    /// - After N successful registrations, counter equals N+1 (since IDs start at 1)
+    /// - Cancelled tasks leave gaps, so this does NOT equal the total number of active tasks
+    pub fn get_counter(env: Env) -> u64 {
+        env.storage()
+            .persistent()
+            .get(&DataKey::Counter)
+            .unwrap_or(0)
+    }
+
     pub fn monitor(env: Env) -> Vec<ExecutableTask> {
         let now = env.ledger().timestamp();
         let mut executable = Vec::new(&env);
@@ -1127,6 +1148,223 @@ mod tests {
         assert_eq!(id2, 2);
     }
 
+    /// Verifies that the ID counter does NOT increment when registration fails due to invalid interval.
+    /// This ensures no IDs are wasted on failed registrations.
+    #[test]
+    fn test_id_counter_not_incremented_on_invalid_registration() {
+        let env = Env::default();
+        env.mock_all_auths();
+
+        let contract_id = env.register_contract(None, SoroTaskContract);
+        let client = SoroTaskContractClient::new(&env, &contract_id);
+
+        let creator = Address::generate(&env);
+        let target = Address::generate(&env);
+
+        let valid_config = TaskConfig {
+            creator: creator.clone(),
+            target: target.clone(),
+            function: Symbol::new(&env, "hello"),
+            args: vec![&env],
+            resolver: None,
+            interval: 3600,
+            last_run: 0,
+            gas_balance: 1000,
+            whitelist: Vec::new(&env),
+            is_active: true,
+            blocked_by: Vec::new(&env),
+        };
+
+        let invalid_config = TaskConfig {
+            creator: creator.clone(),
+            target: target.clone(),
+            function: Symbol::new(&env, "hello"),
+            args: vec![&env],
+            resolver: None,
+            interval: 0, // Invalid: will panic
+            last_run: 0,
+            gas_balance: 1000,
+            whitelist: Vec::new(&env),
+            is_active: true,
+            blocked_by: Vec::new(&env),
+        };
+
+        // Attempt invalid registration (should panic, counter not incremented)
+        let _ = client.try_register(&invalid_config);
+
+        // Valid registration should still get ID 1 (counter wasn't incremented by failed attempt)
+        let id = client.register(&valid_config);
+        assert_eq!(id, 1);
+    }
+
+    /// Verifies that IDs are monotonically increasing across many registrations.
+    /// This ensures the sequential allocation logic works for high volumes.
+    #[test]
+    fn test_sequential_ids_multiple_registrations() {
+        let env = Env::default();
+        env.mock_all_auths();
+
+        let contract_id = env.register_contract(None, SoroTaskContract);
+        let client = SoroTaskContractClient::new(&env, &contract_id);
+
+        let creator = Address::generate(&env);
+        let target = Address::generate(&env);
+
+        let config = TaskConfig {
+            creator: creator.clone(),
+            target: target.clone(),
+            function: Symbol::new(&env, "hello"),
+            args: vec![&env],
+            resolver: None,
+            interval: 3600,
+            last_run: 0,
+            gas_balance: 1000,
+            whitelist: Vec::new(&env),
+            is_active: true,
+            blocked_by: Vec::new(&env),
+        };
+
+        // Register 100 tasks and verify IDs are 1..=100
+        for i in 1..=100u64 {
+            let id = client.register(&config);
+            assert_eq!(id, i, "Task {} should have ID {}", i, i);
+        }
+    }
+
+    /// Verifies that all task IDs are unique, even after cancelling tasks.
+    /// IDs are never reused, so uniqueness is guaranteed.
+    #[test]
+    fn test_id_uniqueness() {
+        let env = Env::default();
+        env.mock_all_auths();
+
+        let contract_id = env.register_contract(None, SoroTaskContract);
+        let client = SoroTaskContractClient::new(&env, &contract_id);
+
+        let creator = Address::generate(&env);
+        let target = Address::generate(&env);
+
+        let config = TaskConfig {
+            creator: creator.clone(),
+            target: target.clone(),
+            function: Symbol::new(&env, "hello"),
+            args: vec![&env],
+            resolver: None,
+            interval: 3600,
+            last_run: 0,
+            gas_balance: 1000,
+            whitelist: Vec::new(&env),
+            is_active: true,
+            blocked_by: Vec::new(&env),
+        };
+
+        let mut ids = Vec::new(&env);
+        for _ in 0..50 {
+            ids.push_back(client.register(&config));
+        }
+
+        // Check all IDs are unique by comparing each pair
+        let len = ids.len();
+        let mut i = 0;
+        while i < len {
+            let id_i = ids.get(i).expect("index out of bounds");
+            let mut j = i + 1;
+            while j < len {
+                let id_j = ids.get(j).expect("index out of bounds");
+                assert_ne!(
+                    id_i, id_j,
+                    "Task IDs {} and {} are duplicate",
+                    id_i, id_j
+                );
+                j += 1;
+            }
+            i += 1;
+        }
+    }
+
+    /// Verifies that cancelling a task does not reuse its ID for new registrations.
+    /// The counter only increments, so new IDs are always larger than cancelled ones.
+    #[test]
+    fn test_id_not_reused_after_cancel() {
+        let env = Env::default();
+        env.mock_all_auths();
+
+        let contract_id = env.register_contract(None, SoroTaskContract);
+        let client = SoroTaskContractClient::new(&env, &contract_id);
+
+        let creator = Address::generate(&env);
+        let target = Address::generate(&env);
+
+        let config = TaskConfig {
+            creator: creator.clone(),
+            target: target.clone(),
+            function: Symbol::new(&env, "hello"),
+            args: vec![&env],
+            resolver: None,
+            interval: 3600,
+            last_run: 0,
+            gas_balance: 1000,
+            whitelist: Vec::new(&env),
+            is_active: true,
+            blocked_by: Vec::new(&env),
+        };
+
+        // Register 3 tasks (IDs 1, 2, 3)
+        let id1 = client.register(&config);
+        let id2 = client.register(&config);
+        let id3 = client.register(&config);
+        assert_eq!(id1, 1);
+        assert_eq!(id2, 2);
+        assert_eq!(id3, 3);
+
+        // Cancel task 2
+        client.cancel_task(&id2);
+
+        // Register another task - should get ID 4 (not reuse 2)
+        let id4 = client.register(&config);
+        assert_eq!(id4, 4);
+    }
+
+    /// Verifies that each new registration receives an ID larger than all previous ones.
+    /// This is a core invariant of the sequential allocation system.
+    #[test]
+    fn test_id_monotonically_increasing() {
+        let env = Env::default();
+        env.mock_all_auths();
+
+        let contract_id = env.register_contract(None, SoroTaskContract);
+        let client = SoroTaskContractClient::new(&env, &contract_id);
+
+        let creator = Address::generate(&env);
+        let target = Address::generate(&env);
+
+        let config = TaskConfig {
+            creator: creator.clone(),
+            target: target.clone(),
+            function: Symbol::new(&env, "hello"),
+            args: vec![&env],
+            resolver: None,
+            interval: 3600,
+            last_run: 0,
+            gas_balance: 1000,
+            whitelist: Vec::new(&env),
+            is_active: true,
+            blocked_by: Vec::new(&env),
+        };
+
+        let mut prev_id = 0u64;
+        for _ in 0..20 {
+            let current_id = client.register(&config);
+            assert!(
+                current_id > prev_id,
+                "New ID {} should be larger than previous ID {}",
+                current_id,
+                prev_id
+            );
+            prev_id = current_id;
+        }
+    }
+
     #[test]
     fn test_register_invalid_interval() {
         let env = Env::default();