Repository navigation
test: add deterministic failure-boundary coverage for completes in ./contracts/admin/src/lib.rs (#1423) - #1563
Open
Omolola-art wants to merge 7 commits into
Conversation
The workspace did not compile on main, which blocked every contract test (including the admin suite targeted by this issue). Two independent breakages: 1. contracts/credence_errors/src/lib.rs — a botched merge (c2d6b98) left the file in a non-compiling state: the soroban_sdk import list had been reduced to just `contracterror` (so `contracttype`, `panic_with_error`, `Address` and `Env` were all unresolved), three enum variants were deleted while their uses remained elsewhere in the file, two variants were renamed onto codes that already existed, and two discriminants were duplicated. This restores the pre-merge-damage revision, which compiles and whose own test suite (test_errors.rs) already asserts the restored codes (e.g. InvalidCurrency == 234). 2. Cargo.lock — soroban-env-host 22.1.3 accepts `ed25519-dalek >= 2.0.0` but is only compatible with the 2.x rand_core. Resolution had picked 3.0.0, whose rand_core 0.10 traits are unsatisfied, so the build failed inside a third-party crate. Pinning to 2.2.0 removes the conflicting 3.0.0 dependency tree and nothing else. No error codes are renumbered by this change: the restored definitions match the wire-stable table that the surviving call sites and tests use. Refs CredenceOrg#1423
Adds test_completes_failure_boundaries.rs covering accept_ownership, the entry point that completes the two-step ownership transfer in lib.rs. It is the only path by which durable control of the contract changes hands, so it is asserted across every input class: valid, invalid, duplicate, and boundary. Coverage (15 tests): * valid — an eligible pending SuperAdmin accepts after the timelock, becomes owner, consumes the proposal, advances the epoch exactly once, and publishes exactly admin_rotated + ownership_transfer_accepted. * invalid — completion requires the pending owner (a different SuperAdmin and a stranger are both rejected with NotAdmin); no proposal is NoPendingAdmin; a paused contract is ContractPaused and recovers on unpause. * duplicate — replaying a consumed proposal is rejected three times over and republishes nothing, so a retried transaction cannot re-run the rotation. * boundary — the timelock is an inclusive lower bound (rejected at eligible_at - 1, accepted at eligible_at, and the rejection is a no-op so the same proposal succeeds one second later); a proposal stamped near u64::MAX is rejected with Overflow rather than wrapping into an immediately-satisfiable timelock; a candidate demoted, deactivated, suspended, or removed during the timelock cannot complete. * recovery — an ineligible candidate is not a dead end: the owner keeps control and can replace the proposal; a suspension that merely expires does not block completion; a rejected attempt is retryable without re-proposing once the blocking condition is cleared. Every negative test asserts the full no-op invariant from the retry contract documented at the top of lib.rs: the owner is unchanged, the pending proposal is unchanged, the config epoch does not advance, and no contract event is published. The authorization and state-validation logic already in accept_ownership was found correct and is unchanged. This commit adds tests only. Verified by mutation testing — each of these mutations to lib.rs is caught: * timelock off-by-one (`now < eligible_at` -> `eligible_at - 1`) * removal of require_effective_super_admin at acceptance (7 tests fail) * removal of the pending-owner authorization check * not consuming the proposal on success (5 tests fail) Refs CredenceOrg#1423
The admin test suite could not run, and once made to run, 30 tests failed for reasons unrelated to their subject matter. All of it stems from one misunderstanding plus three smaller defects. The misunderstanding: in soroban-sdk 22 `env.events().all()` returns the events of the most recent top-level invocation, not a cumulative log. Every "publishes no event" assertion therefore compared two unrelated snapshots and could never hold. Assertions now read the log immediately after the call under test, and multi-call tests accumulate per invocation via a new `role_event_tags` helper. Note that reading the count *after* an intervening client call also resets the frame, so the reads are ordered before the state/epoch assertions. Smaller defects, each fixed at the cause rather than by loosening the assertion: * test_ownership_transfer expected error CredenceOrg#109 and CredenceOrg#107 for cases that now correctly raise NoPendingAdmin (115) and AdminUnchanged (111). The expectations were stale against the wire-stable error table. * test_suspension's min-admins test suspended the caller itself, so it tripped the earlier self-suspension guard (111) and never reached the min-admins guard. That guard is only reachable with min_admins >= 2, so the test now sets up two admins and suspends one of them. * test_ownership_transfer's setup_three_super_admins called mock_all_auths and then re-entered as_contract, which fails with "frame is already authorized"; the calls are now split into separate frames. * test_atomic_rollback, test_emergency, and the four event-reading test modules needed the Events trait import and a corrected set_pause_signer arity. * test_basic.rs was overwritten by CredenceOrg#1478 with 875 lines of code that never compiled (44 `assert_eq(` instead of `assert_eq!(`, and calls to assign_role / reinstate_admin / suspend_admin_with_reason / env.register / mock_all_authentications, none of which exist). It is restored to its last-good revision, which is why this commit deletes 875 lines: that code could not build in the first place, so it was never running coverage. Also removes the duplicate `is_admin -> bool` added by CredenceOrg#1496, which collided with the pre-existing `is_admin -> Role`; all existing callers and tests use the Role signature, so that one is kept. Without this the admin crate does not compile at all. After this, `cargo test -p admin` is green: 236 lib + 3 integration tests, 0 failures. The repaired tests were themselves checked to still fail under mutation (removing the ROLE_REVOKED event, and removing the min-admins guard each fail the tests that cover them). Refs CredenceOrg#1423
|
@Omolola-art 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! 🚀 |
Contributor
|
This pull request currently has a merge conflict with Could you please resolve the conflict (merge or rebase on the latest |
Merge upstream/main (d05ade3) into test/issue-1423-admin-completes-coverage. Conflict resolution: - contracts/admin/src/lib.rs, test_basic.rs: keep ours - contracts/credence_errors/src/lib.rs: take theirs. Ours removed 10 wire-stable ContractError variants (e.g. DuplicateIdempotencyKey, 20 call sites in credence_bond) and renumbered SignatureExpired, which upstream's tip commit d05ade3 deliberately restored. Preserved root workspace files deleted by upstream commit c9e5c7b ("Extend adversarial regression cases ... CredenceOrg#1578"), a test-only PR that removed 59 tracked files including Cargo.toml, leaving upstream/main with no workspace root. Cargo.lock intentionally left at upstream's version so dependency updates are not clobbered. Also fixed two enum defects in credence_errors inherited from upstream: - restored SnapshotGenerationMismatch/CooldownRequestAlreadyPending/ CooldownRequestNotFound/CooldownPeriodNotElapsed (235-238), deleted by upstream test commit e311fbc while still referenced by credence_bond and asserted in test_errors.rs - removed 14 duplicate variant definitions (E0428/E0081) re-declared after the 500s block, and restored ContractIdMismatch = 221, which upstream had overwritten with a duplicated BytesTooLarge = 239 Dropped build_log.txt and test_out.txt build artifacts added by c9e5c7b. Verified: no conflict markers, no staged deletions, no duplicate variant names or discriminants, all 116 variants covered by the exhaustive category()/description()/is_recoverable() impls, and every referenced ContractError variant resolves. cargo check was not run (no Rust toolchain in this environment).
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.
Summary
Adds deterministic failure-boundary coverage for the
completesentry point in./contracts/admin/src/lib.rs— i.e.accept_ownership, the function thatcompletes the two-step ownership transfer. It is the only path by which durable
control of the contract changes hands.
The authorization and state-validation logic already in
accept_ownershipwasreviewed and found correct; it is unchanged. This PR adds tests, and repairs
the pre-existing breakage that made the admin suite unrunnable.
Closes #1423
Important:
maindid not build or testThe admin suite could not be executed at all before this PR. Three independent
breakages, all predating this work:
contracts/credence_errors/src/lib.rsdid not compile. A botched merge(
c2d6b98b) reduced thesoroban_sdkimport list to justcontracterror(leaving
contracttype,panic_with_error,Address,Envunresolved),deleted three enum variants whose uses remained, renamed two variants onto
already-used codes, and duplicated two discriminants — 40 errors. Because
every contract depends on this crate, no contract test could run.
Cargo.lockresolved an incompatibleed25519-dalek3.0.0.soroban-env-host 22.1.3declaresed25519-dalek >= 2.0.0but only workswith the 2.x
rand_core; the 3.0.0 tree'srand_core0.10 traits areunsatisfied, failing inside a third-party crate.
contracts/admindid not compile — a duplicateis_admin -> bool(test: add deterministic failure-boundary coverage for is_admin #1496)collided with the pre-existing
is_admin -> Role.Once made to compile, 30 further tests failed, all from the same root cause
(detailed below).
crates/interfacesis also corrupt on main (base64 blob inconsts.rs, stray#[config(test)]]ingovernable.rs, from #1480) and stilldoes not build. That is untouched here and out of scope for this issue, but it
does mean
cargo build --workspaceremains red for reasons unrelated to admin.Acceptance criteria → code and tests
1. Deterministic behavior for valid, invalid, duplicate, and boundary inputs
New module:
contracts/admin/src/test_completes_failure_boundaries.rs(15 tests).eligible_candidate_completes_transfer_exactly_once+1, exactlyadmin_rotated+ownership_transfer_acceptedcompletion_requires_the_pending_ownerNotAdmin; proposal survivescompletion_without_a_proposal_is_rejectedNoPendingAdminpaused_contract_blocks_completion_and_recovers_on_unpauseContractPaused; proposal preserved; succeeds after unpausereplay_after_completion_is_rejected_and_emits_nothingNoPendingAdmin, no events, epoch frozentimelock_boundary_is_inclusive_and_rejection_is_recoverableeligible_at - 1, accepted ateligible_attimelock_overflow_is_rejectedu64::MAXproposal stamp →Overflow, not a wrapped, immediately-satisfiable timelockcandidate_demoted_during_timelock_cannot_completeNotAdmincandidate_suspended_during_timelock_cannot_completeAdminSuspendedcandidate_removed_during_timelock_cannot_completeNotAdmincandidate_deactivated_during_timelock_cannot_completeAlreadyDeactivatedowner_recovers_by_replacing_an_ineligible_candidatecandidate_whose_suspension_expired_during_timelock_can_completerejected_attempt_is_retryable_without_reproposingevent_helper_sees_contract_events_and_ignores_diagnostics2. Authorization, validation, and state-transition invariants remain enforced
Every negative test asserts the full no-op invariant from the retry contract
documented at the top of
lib.rs, via theassert_no_ophelper:get_config_epoch()unchanged,3. Retries, partial failure, and concurrency cannot produce an unsafe result
(
replay_after_completion_is_rejected_and_emits_nothing).condition clears, which is the retry contract's core promise.
a proposal is only an intent, so a candidate who is demoted, deactivated,
suspended, or removed while the clock runs cannot take ownership. This is the
anti-stale-proposal guarantee from
df30d820.4. Focused tests cover success, rejection, boundary, and regression scenarios
See the table above, plus the 30 pre-existing tests repaired below.
5. Existing callers remain compatible
No public interface changed. The only signature-affecting edit is the removal of
the duplicate
is_admin -> bool(which made the crate uncompilable); thesurviving
is_admin -> Roleis the one every existing caller and test uses.Test execution evidence
Toolchain:
rustc 1.89.0(perrust-toolchain.toml), installed for this run —the container had no Rust toolchain.
The new module in isolation:
cargo fmt -p admin -- --checkclean;cargo clippy -p admin --all-targetsreports 0 errors.
Mutation testing — the tests are not vacuous
Each mutation below was applied to
lib.rstemporarily, then reverted. Every oneis caught:
lib.rsnow < eligible_at→eligible_at - 1)require_effective_super_adminat acceptanceROLE_REVOKEDevent fromdeactivate_adminMinAdminssuspension guardThe last two also confirm the repaired pre-existing tests still detect
regressions rather than passing by default.
Repairing the 30 pre-existing failures
The dominant cause was a single misunderstanding: in soroban-sdk 22,
env.events().all()returns the events of the most recent top-levelinvocation, not a cumulative log. Every "publishes no event" assertion
therefore compared two unrelated snapshots and could never hold. Assertions now
read the log immediately after the call under test, and multi-call tests
accumulate per invocation via a new
role_event_tagshelper. (Reading the countafter an intervening client call also resets the frame, so the reads are
ordered before the state/epoch assertions.)
Each remaining defect was fixed at the cause, not by loosening an assertion:
test_ownership_transferexpected errors#109/#107for cases thatnow correctly raise
NoPendingAdmin (115)/AdminUnchanged (111)— staleagainst the wire-stable error table.
test_suspension::test_suspend_below_min_admins_rejectedsuspended thecaller itself, so it tripped the earlier self-suspension guard (
111) andnever reached the min-admins guard. That guard is only reachable with
min_admins >= 2; the test now sets up two admins and suspends one.setup_three_super_adminscalledmock_all_authsthen re-enteredas_contract, failing withframe is already authorized; the calls are nowsplit into separate frames.
test_atomic_rollback/test_emergencyneeded theEventstrait importand a corrected
set_pause_signerarity.test_basic.rswas overwritten by test: extend adversarial regression cases for admin test_basic #1478 with 875 lines that never compiled(44 ×
assert_eq(instead ofassert_eq!(, plus calls toassign_role,reinstate_admin,suspend_admin_with_reason,env.register, andmock_all_authentications— none of which exist). Restored to its last-goodrevision. This is why the suite shows 236 rather than a higher number: that
code could not build, so it was never running coverage. This is the one
change most worth a reviewer's attention, since it is a large deletion of
another contributor's work.
Security and failure-mode notes
contract's actual behavior.
credence_errorsrestores wire-stable codes; no renumbering. The restoreddefinitions match what surviving call sites and
test_errors.rsalreadyassert (e.g.
InvalidCurrency == 234).the
assert_no_ophelper proves a rejected call leaks no partial state and noevent.
Out of scope
crates/interfacesis still corrupt on main (from test: add boundary and recovery coverage for governable interface #1480) and still does notbuild;
cargo build --workspaceremains red for that reason. Not touched.contracts-tests.ymlis a no-op "while main-branchCI is being stabilized"), so the commands above were run locally.