fix(router-access): align expiry semantics and fix RoleMemberCount bookkeeping Closes #716 Three related bugs in contracts/router-access/src/lib.rs: 1. Expiry comparison inconsistency (>= vs >) is_role_expired and has_direct_role_internal used >= so a role whose expires_at equalled the current timestamp was already considered expired. router-core's is_route_expired uses >, making expires_at the last valid ledger sequence. Both sites changed to > to match that convention. 2. RoleMemberCount never incremented on grant grant_role_internal had a comment promising to maintain RoleMemberCount without iterating RoleMembers, but the increment was never written. Fixed by capturing currently_active before any writes, then incrementing only when !currently_active (new grant or re-grant after expiry). An expiry update on a live role does not increment to avoid double-counting. 3. Premature storage removal in expire_role caused count decrement to be skipped expire_role removed HasRole and RoleExpiry directly before delegating to deactivate_role_grant. Because deactivate_role_grant calls has_role_internal to decide whether to decrement the count, those early removes made it always see the role as inactive and skip the decrement. Removed the redundant removes so deactivate_role_grant is the single cleanup path for both expire_role and revoke_role. Added test_role_valid_at_exact_expiry_timestamp_expired_one_second_after to pin the new boundary semantics: role is valid when current == expires_at, expired only when current > expires_at. All 50 router-access tests pass.

Author: joshfatoye0011-bit

contracts/router-access/src/lib.rs

@@ -137,6 +137,12 @@ impl RouterAccess {
     }
 
     /// Check if a role has expired for an address.
+    ///
+    /// Returns `true` only when the current ledger timestamp **strictly exceeds**
+    /// `expires_at`, matching the convention used throughout this suite:
+    /// `expires_at` is the **last valid** timestamp, so the role is still active
+    /// when `current_timestamp == expires_at` and expired only once
+    /// `current_timestamp > expires_at`.
     pub fn is_role_expired(env: Env, role: String, target: Address) -> bool {
         // View helper: counter is maintained for active members, but expiry still
         // uses RoleExpiry storage.
@@ -147,7 +153,7 @@ impl RouterAccess {
             .get::<DataKey, u64>(&DataKey::RoleExpiry(role, target))
         {
             let current_timestamp = env.ledger().timestamp();
-            current_timestamp >= expires_at
+            current_timestamp > expires_at
         } else {
             false
         }
@@ -561,12 +567,6 @@ impl RouterAccess {
     ) -> Result<(), AccessError> {
         caller.require_auth();
         router_common::require_admin_simple!(&env, &caller, &DataKey::SuperAdmin, AccessError)?;
-        env.storage()
-            .instance()
-            .remove(&DataKey::RoleExpiry(role.clone(), target.clone()));
-        env.storage()
-            .instance()
-            .remove(&DataKey::HasRole(role.clone(), target.clone()));
         Self::require_super_admin(&env, &caller)?;
         Self::deactivate_role_grant(&env, &role, &target);
         env.events().publish(
@@ -733,7 +733,8 @@ impl RouterAccess {
         // the requested expiry matches the existing expiry.
         //
         // This allows admins to extend/shorten expiry (or remove it by granting with `None`).
-        if has_raw_assignment && Self::has_role_internal(env, account, role) {
+        let currently_active = has_raw_assignment && Self::has_role_internal(env, account, role);
+        if currently_active {
             let existing_expiry: Option<u64> = env
                 .storage()
                 .instance()
@@ -770,6 +771,22 @@ impl RouterAccess {
             .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 mut members: Vec<Address> = env
             .storage()
             .instance()
@@ -885,14 +902,17 @@ impl RouterAccess {
             return false;
         }
 
-        // Check if role has expired
+        // Check if role has expired.
+        // `expires_at` is the last valid timestamp: the role is still active when
+        // `current_timestamp == expires_at` and only expired once it strictly
+        // exceeds `expires_at`, consistent with `is_route_expired` in router-core.
         if let Some(expires_at) = env
             .storage()
             .instance()
             .get::<DataKey, u64>(&DataKey::RoleExpiry(role.clone(), account.clone()))
         {
             let current_timestamp = env.ledger().timestamp();
-            if current_timestamp >= expires_at {
+            if current_timestamp > expires_at {
                 return false;
             }
         }
@@ -950,6 +970,32 @@ mod tests {
         assert!(!client.has_role(&user, &role));
     }
 
+    /// `expires_at` is the **last valid** timestamp: the role must still be
+    /// active when `current_timestamp == expires_at` and only expired once
+    /// `current_timestamp > expires_at`. This mirrors the semantics of
+    /// `is_route_expired` in router-core and makes the boundary consistent
+    /// across the entire suite.
+    #[test]
+    fn test_role_valid_at_exact_expiry_timestamp_expired_one_second_after() {
+        let (env, admin, client) = setup();
+        let role = String::from_str(&env, "operator");
+        let user = Address::generate(&env);
+
+        let now = env.ledger().timestamp();
+        client.grant_role(&admin, &user, &role, &Some(10));
+        // expires_at == now + 10
+
+        // At exactly expires_at the role is still valid.
+        env.ledger().set_timestamp(now + 10);
+        assert!(client.has_role(&user, &role));
+        assert!(!client.is_role_expired(&role, &user));
+
+        // One second past expires_at the role is expired.
+        env.ledger().set_timestamp(now + 11);
+        assert!(!client.has_role(&user, &role));
+        assert!(client.is_role_expired(&role, &user));
+    }
+
     #[test]
     fn test_set_role_admin_emits_event() {
         let (env, admin, client) = setup();