Skip to content

test(admin): add deterministic failure-boundary coverage for get_role - #1598

Open
opeolarewaju5-glitch wants to merge 2 commits into
CredenceOrg:mainfrom
opeolarewaju5-glitch:test/get-role-failure-boundaries
Open

opeolarewaju5-glitch wants to merge 2 commits into
CredenceOrg:mainfrom
opeolarewaju5-glitch:test/get-role-failure-boundaries

Conversation

@opeolarewaju5-glitch

Copy link
Copy Markdown

Closes #1401

Description

Adds deterministic failure-boundary coverage for the get_role read entrypoint in contracts/admin/src/lib.rs, and documents its read-only invariants on the function itself.

get_role is the shared role-resolution primitive behind add_admin, remove_admin, update_admin_role, deactivate_admin, reactivate_admin, and the pausable pause-authority check. Until now it had no direct test coverage — every assertion reached it indirectly through a caller, so its own boundary contract (what it returns, what it rejects, and what it must never do) was unpinned. Its sibling get_admin_role already carried a failure-boundary invariant comment; get_role did not.

Type of Change

  • test — test additions or improvements
  • feat / fix / docs / refactor / ci / chore

No public interface change. get_role(e: Env, address: Address) -> AdminRole keeps its exact signature, return values, and ContractError::NotAdmin (100) failure mode. No migration path is required.

Files changed

File Change
contracts/admin/src/test_get_role_failure_boundaries.rs New. 16 focused failure-boundary tests (427 lines).
contracts/admin/src/lib.rs Register the test module alongside the existing #[cfg(test)] mod block; expand the get_role doc comment with determinism / boundary / error invariants.
CHANGELOG.md Unreleased → Added entry (repo checklist requires it when contracts/** is touched).

Failure paths traced

get_role reads exactly one instance-storage key, DataKey::AdminInfo(Address), and returns the stored AdminRole.

Input class Path taken Outcome
Registered admin (active) Some(AdminInfo) stored role (SuperAdmin=3, Admin=2, Operator=1)
Registered but suspended Some(AdminInfo) stored role — suspension is not consulted
Registered but deactivated Some(AdminInfo) stored role — active is not consulted
Unknown / never registered None panic ContractError::NotAdmin (100)
Removed via remove_admin key deleted → None panic NotAdmin (100)
Zero/sentinel address None panic NotAdmin (100)
Called before initialize None panic NotAdmin (100) — a per-address read has no init dependency
Dangling AdminList entry None panic NotAdmin (100) — never returns stale/garbage data
Contract paused no require_not_paused on the read path stored role (reads stay available under pause)

Invariants documented in code

  1. Read-only — no state mutation, no ConfigEpoch advance, no events; a pure function of AdminInfo at the current ledger.
  2. Stored role ≠ effective authority — a suspended or deactivated admin still resolves to their stored role here; callers needing effective authority must use is_admin / has_role_at_least.
  3. Typed failure only — an unregistered address fails with the wire-stable NotAdmin (100) discriminant, never a bare panic!.
  4. Duplicate/retry safe — repeated reads are idempotent; a rejected, stale, or failed mutation never changes what get_role reports and never advances the epoch.

Test matrix

# Test Class Asserts
1 get_role_returns_exact_role_for_each_hierarchy_level success Operator=1, Admin=2, SuperAdmin=3 exactly
2 get_role_is_repeatable_and_leaves_epoch_and_events_untouched duplicate 9 identical reads; epoch + event count unchanged
3 get_role_rejects_unknown_address_with_not_admin invalid try_get_role → Error(Contract, #100), no state change
4 get_role_rejects_zero_address_sentinel_with_not_admin invalid sentinel GAAAA…WHF → #100
5 get_role_before_initialize_rejects_with_not_admin invalid pre-init → #100
6 get_role_rejects_removed_admin invalid after remove_admin → #100
7 get_role_returns_stored_role_while_suspended boundary stored role returned while has_role_at_least is false
8 get_role_is_timestamp_independent_across_suspension_expiry_boundary timing boundary identical result at until-1, ==until, until+1, while has_role_at_least flips at ==until (inclusive expiry)
9 get_role_returns_stored_role_for_deactivated_admin boundary active=false → stored role; is_admin → Role::User
10 get_role_reflects_committed_role_change_and_detects_stale_read stale/retry promote + demote observed by the next read; epoch advances exactly once per commit; repeated same-role write is a no-op
11 get_role_rejects_dangling_admin_list_entry boundary list names the address, AdminInfo gone → #100, never stale data
12 rejected_duplicate_add_admin_leaves_get_role_epoch_and_events_unchanged duplicate duplicate add_admin → AlreadyActive (#405) before any epoch bump
13 rejected_unauthorized_mutation_then_successful_retry_is_consistent retry/recovery unauthorized add_admin → #100 with epoch + events + get_role unchanged; retry then commits exactly once
14 get_role_remains_available_while_paused permission/state reads work under pause; write path fails ContractPaused (#106); unpause restores it
15 unregistered_caller_is_rejected_before_any_state_change permission remove_admin(stranger, target) → #100, target/epoch/events untouched
16 get_role_agrees_with_get_admin_role_across_all_states regression both entrypoints agree for active, suspended, deactivated, removed, unknown

Rejection paths use try_* clients so the real transaction boundary is exercised, with relative get_config_epoch / events().all().len() snapshots for no-op assertions — the same scaffolding as test_pause_failure_boundaries.rs, test_atomic_rollback.rs, and test_suspension.rs.

⚠️ Validation status — please read first

These 16 tests have not been executed. To honour the issue's scope I did not modify anything outside get_role, but main at 3beab889 does not compile, so cargo test cannot run at all. All three blockers below are pre-existing and are not touched by this PR:

  1. contracts/credence_errors/src/lib.rs — 40 compile errors on main. Duplicate enum variants (RoleNotHeldAtLedger = 116 at lines 175 and 676, SignatureExpired = 222 twice, discriminant 232 twice) → E0428/E0081; use soroban_sdk::contracterror; is the crate's only import, so #[contracttype], panic_with_error, Env and Address are unresolved; and three variants referenced by other crates are absent (BytesTooLarge, MaxPauseSignersExceeded, CrossContractCallerMismatch).
  2. Cargo.lock drift. soroban-env-host 22.1.3 declares ed25519-dalek = ">=2.0.0" (open-ended) and the lock resolved it to 3.0.0, which is API-incompatible with that crate's own testutils code. Verified recoverable with cargo update -p ed25519-dalek@3.0.0 --precise 2.2.0 --dry-run.
  3. CI is stubbed. contracts-tests.yml, contracts-lints.yml (rustfmt + clippy) and coverage.yml all declare 'pull_request': null and their jobs only echo a "paused — CI stabilization in progress" message, so they compile nothing.

What was verified locally with Rust 1.89.0 (per rust-toolchain.toml): the diff is confined to test code, a doc comment, and a changelog line — no entrypoint body changed, so gas, WASM size, and on-chain behaviour are untouched.

Evidence to collect once main builds

Scenario Command Expected
Success cargo test -p admin get_role_returns_exact_role_for_each_hierarchy_level 1 passed; 0 failed
Invalid / boundary / regression cargo test -p admin test_get_role_failure_boundaries 16 passed; 0 failed
Full crate regression cargo test -p admin --locked all pre-existing tests pass
Workspace CI parity cargo test --workspace --locked green
Lints cargo fmt --all -- --check && cargo clippy --workspace --all-targets --all-features -- -D warnings exit 0
Wire-stable error codes cargo test -p credence_errors error_codes_wire NotAdmin still 100

Non-goals

No typo/formatting/documentation-only or cosmetic change; no unrelated refactor, dependency upgrade, or broad rewrite (get_role and get_admin_role remain separate entrypoints — deduplicating them is a behaviour-affecting refactor and is not attempted); no safeguard removed and no validation weakened to make a test pass.

Checklist

  • Tests added for the changed functionality
  • Docs updated (failure-boundary invariants documented on get_role)
  • CHANGELOG.md updated (contracts/** touched)
  • Branch follows <type>/<short-description> naming convention
  • Commit message follows conventional commits
  • Local cargo test / clippy evidence — blocked by the pre-existing breakage documented above

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.

Add deterministic failure-boundary coverage for get_role in ./contracts/admin/src/lib.rs

1 participant