Skip to content

Refactor: extract pending-admin cleanup to shared helper - #77

Open
onyekachi66 wants to merge 1 commit into
Orbit-Wal:mainfrom
onyekachi66:fix/pending-admin-structural-cleanup
Open

Refactor: extract pending-admin cleanup to shared helper#77
onyekachi66 wants to merge 1 commit into
Orbit-Wal:mainfrom
onyekachi66:fix/pending-admin-structural-cleanup

Conversation

@onyekachi66

@onyekachi66 onyekachi66 commented Aug 26, 2026

Copy link
Copy Markdown

Closes #44

Root cause

The invariant that every admin-rotation path must remove PendingAdmin(old_admin) was implicit and enforced only by each function (accept_admin and execute_recovery) remembering to execute the cleanup independently. Because PendingAdmin is keyed by the admin's address rather than being a singleton, this design is structurally fragile: if a third admin-rotation mechanism is ever added, it would be easy to forget this cleanup, leading to orphaned pending-admin entries accumulating under stale addresses.

What changed and why

Factored the 'clear any pending admin proposal for the outgoing admin' logic into a new internal helper set_admin_and_clear_pending to ensure structural enforcement of this invariant. Both existing admin-rotation paths (accept_admin and execute_recovery) now route through this helper, centralizing the invariant so future rotation paths cannot forget to clean it up. I chose a shared helper over individual function checks because it enforces the cleanup at the structural level anytime the admin role is transferred.

Definition of done — addressed item by item

  • Explicit test asserting PendingAdmin(old_admin) is gone after accept_admin: Added test_pending_admin_entry_removed_after_normal_accept which explicitly tests this.
  • Shared helper introduced (optional but recommended) so the invariant is structural rather than per-function-remembered: Created the set_admin_and_clear_pending internal helper and routed both accept_admin and execute_recovery to use it.

Evidence this actually runs

<!-- NIL-->

Tests

Added test_pending_admin_entry_removed_after_normal_accept. It asserts that after a normal accept_admin flow, a fresh proposal targeting the old admin address finds no leftover pending entry and returns the NoPendingAdmin error.

Regression check

The adjacent behavior is the guardian recovery path, which also clears the pending admin entry. I verified this via the existing test_recovery_clears_any_in_flight_normal_admin_transfer test, which explicitly ensures that the recovery path also leaves no stale PendingAdmin entries behind.

Checklist

  • Every Definition of done bullet above is checked and explained, not just checked
  • Evidence block above is filled in with real output, not omitted
  • New/updated tests are included and shown passing
  • No leftover console.log/TODO/debug code
  • Related/adjacent behavior re-verified, not assumed unaffected

Factored the 'clear any pending admin proposal for the outgoing admin' logic into a new internal helper set_admin_and_clear_pending to ensure structural enforcement of this invariant. Both existing admin-rotation paths (accept_admin and execute_recovery) now route through this helper. Also added an explicit regression test to assert that PendingAdmin(old_admin) is successfully removed after a normal accept_admin flow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No test confirms a stale PendingAdmin storage key becomes orphaned garbage after an admin transfer

1 participant