Skip to content

fix: deregister guardian from AllGuardians and slot index in resign_g… - #311

Open
Joycejay17 wants to merge 1 commit into
Vero-protocol:mainfrom
Joycejay17:fix/issue--resign-guardian-deregister
Open

fix: deregister guardian from AllGuardians and slot index in resign_g…#311
Joycejay17 wants to merge 1 commit into
Vero-protocol:mainfrom
Joycejay17:fix/issue--resign-guardian-deregister

Conversation

@Joycejay17

Copy link
Copy Markdown

Summary

Closes #303 - resign_guardian now properly deregisters guardians from all membership structures (AllGuardians and the dense slot index), preventing permanent lockout of resigned guardians and eliminating storage leaks.

Problem

Previously, resign_guardian only cleared the guardian membership flag (DataKey::Guardian) but did not remove the address from:

  • DataKey::AllGuardians set
  • The dense slot index (GuardianIndexAt/GuardianIndexOf/GuardianIndexCount)

This caused:

  1. Permanent lockout: A resigned guardian could never be re-added because add_guardian checks all_guardians.contains(guardian)
  2. Storage leak: get_snapshot_meta().guardian_count remained inflated, and get_guardians_page still included resigned guardians
  3. Inconsistency: resign_guardian and remove_guardian left the contract in different states for the same address

Solution

  • Extracted a shared deregister_guardian helper from remove_guardian that handles complete membership cleanup
  • Updated resign_guardian to call deregister_guardian instead of the bare remove(&g_key)
  • Preserved existing token-refund and timelock logic that runs after deregistration
  • Maintained the NotGuardian early return check

Changes

  • src/guardian.rs: Added deregister_guardian helper function (pub(crate)), refactored remove_guardian to use it
  • src/contracts/vault_ops.rs: Updated resign_guardian to call guardian::deregister_guardian
  • tests/resign_guardian_deregister.rs: Added regression test test_resigned_guardian_can_be_readded

Test plan

  • ✅ New regression test verifies resigned guardian can be re-added
  • ✅ Test confirms guardian_count decreases by 1 after resignation
  • ✅ Test confirms resigned guardian no longer appears in get_guardians_page
  • ✅ Existing test test_resign_guardian_refunds_tokens still passes
  • ✅ Contract tests pass: cargo test --lib
  • ✅ New test passes: cargo test --test resign_guardian_deregister

Verification

After resignation:

  • is_guardian(g) returns false
  • get_snapshot_meta().guardian_count decreases by 1
  • get_guardians_page(0, 50) no longer includes the resigned address
  • add_guardian(admin, g) succeeds and the guardian is properly re-registered
  • Both resign_guardian and remove_guardian now leave byte-identical guardian-index state for the same address

…uardian (Vero-protocol#303)

- Extract deregister_guardian helper from remove_guardian
- Update resign_guardian to call deregister_guardian for proper cleanup
- Add regression test that resigned guardian can be re-added
- Keep token-refund/timelock logic after deregistration

Closes Vero-protocol#303
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.

Deregister the guardian from AllGuardians and the slot index in resign_guardian — a resigned guardian can never be re-added

1 participant