fix/refactor: resolve issues #1086, #1087, #1088, #1089

contracts/router-access/src/lib.rs

@@ -805,35 +805,54 @@ impl RouterAccess {
         let raw_key = DataKey::HasRole(role.clone(), account.clone());
         let has_raw_assignment = env.storage().instance().has(&raw_key);
 
+        let expiry_timestamp = match expires_in {
+            Some(seconds) => env
+                .ledger()
+                .timestamp()
+                .checked_add(seconds)
+                .ok_or(AccessError::InvalidExpiry)?,
+            None => u64::MAX,
+        };
+
         // If there is an existing unexpired assignment, only treat it as a duplicate error when
         // the requested expiry matches the existing expiry.
         //
         // This allows admins to extend/shorten expiry (or remove it by granting with `None`).
         let currently_active = has_raw_assignment && Self::has_role_internal(env, account, role);
         if currently_active {
-            let existing_expiry: Option<u64> = env
+            let existing_expiry = env
                 .storage()
                 .instance()
-                .get::<DataKey, u64>(&DataKey::RoleExpiry(role.clone(), account.clone()));
-
-            let requested_expiry = match expires_in {
-                Some(seconds) => env
-                    .ledger()
-                    .timestamp()
-                    .checked_add(seconds)
-                    .ok_or(AccessError::InvalidExpiry)?,
-                None => u64::MAX,
-            };
-
-            let existing_expiry = existing_expiry.unwrap_or(u64::MAX);
-            if existing_expiry == requested_expiry {
+                .get::<DataKey, u64>(&DataKey::RoleExpiry(role.clone(), account.clone()))
+                .unwrap_or(u64::MAX);
+
+            if existing_expiry == expiry_timestamp {
                 return Err(AccessError::AlreadyHasRole);
             }
         }
 
         // Track this role in AllRoles if it's the first time we've seen it
         Self::track_role_in_all_roles(env, role)?;
 
+        env.storage()
+            .instance()
+            .set(&DataKey::HasRole(role.clone(), account.clone()), &true);
+
+        // Increment RoleMemberCount when the account transitions from inactive to active.
+        // This covers two cases:
+        //   1. Brand-new grant (no prior assignment).
+        //   2. Re-grant of a previously expired role (raw assignment exists but was inactive).
+        // An expiry update on a live role must NOT increment to avoid double-counting.
+        if !currently_active {
+            let count: u32 = env
+                .storage()
+                .instance()
+                .get::<DataKey, u32>(&DataKey::RoleMemberCount(role.clone()))
+                .unwrap_or(0);
+            env.storage()
+                .instance()
+                .set(&DataKey::RoleMemberCount(role.clone()), &(count + 1));
+        }
         let expiry_timestamp = match expires_in {
             Some(seconds) => env
                 .ledger()

contracts/router-execution/src/lib.rs

@@ -1721,6 +1721,8 @@ mod tests {
             function: function.clone(),
             simulate_first: false,
             max_retries: 0,
+            args: Vec::new(&env),
+            amount: 1_000_000,
         };
 
         let result = client.execute(&caller, &request);
@@ -1773,6 +1775,8 @@ mod tests {
             function: function.clone(),
             simulate_first: false,
             max_retries: 2,
+            args: Vec::new(&env),
+            amount: 1_000_000,
         };
 
         let result = client.execute(&caller, &request);

contracts/router-quote/src/lib.rs

@@ -111,6 +111,18 @@ const MAX_TRACKED_ROUTES: u32 = 500;
 #[contract]
 pub struct RouterQuote;
 
+/// Returns the index at which `key` should be inserted into `len` items so
+/// the sequence stays ordered, given a comparator `should_insert_before`
+/// that returns true when the new item belongs before the item at `idx`.
+fn find_insertion_index(len: u32, mut should_insert_before: impl FnMut(u32) -> bool) -> u32 {
+    for idx in 0..len {
+        if should_insert_before(idx) {
+            return idx;
+        }
+    }
+    len
+}
+
 #[contractimpl]
 impl RouterQuote {
     /// Initialize the quote contract with an admin address and default fee.
@@ -269,14 +281,9 @@ impl RouterQuote {
                 return Err(QuoteError::InvalidFeeBps);
             }
 
-            let mut position = sorted_tiers.len();
-            for index in 0..sorted_tiers.len() {
-                let current = sorted_tiers.get(index).unwrap();
-                if tier.min_amount < current.min_amount {
-                    position = index;
-                    break;
-                }
-            }
+            let position = find_insertion_index(sorted_tiers.len(), |idx| {
+                tier.min_amount < sorted_tiers.get(idx).unwrap().min_amount
+            });
             sorted_tiers.insert(position, tier.clone());
         }
 
@@ -508,13 +515,9 @@ impl RouterQuote {
         for request in requests.iter() {
             if let Ok(response) = Self::get_quote(env.clone(), request) {
                 // Insertion sort: find position where response.amount_out fits.
-                let mut pos = sorted.len();
-                for i in 0..sorted.len() {
-                    if response.amount_out > sorted.get(i).unwrap().amount_out {
-                        pos = i;
-                        break;
-                    }
-                }
+                let pos = find_insertion_index(sorted.len(), |idx| {
+                    response.amount_out > sorted.get(idx).unwrap().amount_out
+                });
                 sorted.insert(pos, response);
             }
         }

contracts/router-timelock/src/lib.rs

@@ -1113,6 +1113,74 @@ mod tests {
         assert_eq!(topic, Symbol::new(&env, router_common::EVENT_OP_CANCELLED));
     }
 
+    #[test]
+    fn test_cancel_nonexistent_op_fails() {
+        let (env, admin, client) = setup();
+        let fake_id = Bytes::from_array(&env, &[0u8; 32]);
+        assert_eq!(
+            client.try_cancel(&admin, &fake_id),
+            Err(Ok(TimelockError::NotFound))
+        );
+    }
+
+    #[test]
+    fn test_cancel_unauthorized_fails() {
+        let (env, admin, client) = setup();
+        let attacker = Address::generate(&env);
+        let target = Address::generate(&env);
+        let op_id = client.queue(
+            &admin,
+            &String::from_str(&env, "d"),
+            &target,
+            &3600,
+            &GRACE,
+            &Vec::new(&env),
+        );
+        assert_eq!(
+            client.try_cancel(&attacker, &op_id),
+            Err(Ok(TimelockError::Unauthorized))
+        );
+    }
+
+    #[test]
+    fn test_cancel_already_cancelled_fails() {
+        let (env, admin, client) = setup();
+        let target = Address::generate(&env);
+        let op_id = client.queue(
+            &admin,
+            &String::from_str(&env, "d"),
+            &target,
+            &3600,
+            &GRACE,
+            &Vec::new(&env),
+        );
+        client.cancel(&admin, &op_id);
+        assert_eq!(
+            client.try_cancel(&admin, &op_id),
+            Err(Ok(TimelockError::Cancelled))
+        );
+    }
+
+    #[test]
+    fn test_cancel_already_executed_fails() {
+        let (env, admin, client) = setup();
+        let target = Address::generate(&env);
+        let op_id = client.queue(
+            &admin,
+            &String::from_str(&env, "d"),
+            &target,
+            &3600,
+            &GRACE,
+            &Vec::new(&env),
+        );
+        env.ledger().with_mut(|l| l.timestamp += 3601);
+        client.execute(&admin, &op_id);
+        assert_eq!(
+            client.try_cancel(&admin, &op_id),
+            Err(Ok(TimelockError::AlreadyExecuted))
+        );
+    }
+
     // ── validation ────────────────────────────────────────────────────────────
 
     #[test]