fix(router-access): align expiry semantics and fix RoleMemberCount bookkeeping - #931
Merged
Maki-Zeninn merged 1 commit intoJul 26, 2026
Conversation
…okkeeping Closes Maki-Zeninn#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.
|
@joshfatoye0011-bit Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #716 ## Summary Three related bugs fixed in contracts/router-access/src/lib.rs. ### 1. Expiry comparison inconsistency (>= → >) is_role_expired and has_direct_role_internal both used >=, so a role whose expires_at equalled the current timestamp was already considered expired.
outer-core's is_route_expired uses >, making expires_at the last valid ledger sequence. Both sites changed to > to match that convention and document the choice. ### 2. RoleMemberCount never incremented on grant grant_role_internal had a comment saying it would 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
evoke_role. ## Tests Added est_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.