feat: add two-step admin transfer - #34
Conversation
Allow the escrow admin to safely rotate keys through a propose-and-accept flow, preventing accidental lockout while preserving admin-only controls after handover. Co-authored-by: Cursor <cursoragent@cursor.com>
JamesVictor-O
left a comment
JamesVictor-O
left a comment
There was a problem hiding this comment.
The actual two-step admin transfer logic here is well done — matches the issue spec closely and the auth pattern is correct in both directions:
propose_admin: gated by the existingrequire_adminhelper (current admin only), stores the candidate, emitsAdminProposed.accept_admin: correctly callsnew_admin.require_auth()(the candidate proves they hold the key, not the old admin), checks it matchesPendingAdminexactly, then finalizes and clears the pending slot. This is the right way around — a single-step transfer or one that only checks the old admin's auth would defeat the whole point of the pattern.update_fee_bpsas the "prove the new admin has real authority" vehicle is a reasonable call given no other admin-mutating setter existed yet to test against, and it's independently useful (fee_bps was otherwise fixed forever atinitialize).
Test coverage is solid and maps directly onto the issue's acceptance criteria: non-admin propose rejected, pending-candidate storage verified directly, the full propose→accept→old-admin-loses-access→new-admin-gains-access round trip, wrong-address accept rejected, and accept-before-propose rejected. On this branch by itself, cargo fmt --check, cargo build, cargo test --workspace (35 + 7 passing), and cargo clippy -D warnings are all clean.
The blocker is purely a base-branch issue, not your code: this branch is rooted at 75ca6f6 (right after #22 merged), which was main's state before I had to repair it — #21 and #22 had been merged into main with unresolved conflicts (duplicate/colliding Error discriminants, dead code after an early return, a corrupted reclaim_surplus body) that I fixed directly on main on 2026-07-20, and #33 has since added InvalidDeadlineOrder = 20. GitHub is correctly flagging this PR as CONFLICTING in error.rs and test.rs as a result — not because your changes are wrong, but because both files have moved out from under this branch.
Good news: this PR doesn't add any new Error variants (it only reuses the existing Unauthorized and InvalidAmount), so once rebased onto current main, error.rs shouldn't need any changes from this PR at all — the conflict should resolve by just taking main's version of that file. test.rs will need your new test_admin_transfer module (and whichever of your test_admin_basics tests, if any, aren't already covered by main's test_pause/test_version_upgrade modules) reapplied on top of the current file rather than the version this branch currently has.
Could you rebase onto latest main and push? Everything else here looks ready.
Resolve campaign escrow conflicts by preserving upstream fee, treasury, metadata, and dispute updates while keeping the two-step admin transfer flow. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Merged, thanks for this — the two-step propose/accept pattern is exactly right for an admin handover (avoids the single-fat-fingered-address footgun of a direct update_admin). Since this branch was based on a commit just before #31 merged, I double-checked the merge for silent regressions the way I have for a few other PRs this cycle (git merge-tree + git show on the resulting tree, then verified again post-merge on real main): confirmed FeeTooHigh and InvalidCreatorCount (added by #31) survived correctly alongside your new PendingAdmin storage, AdminProposed/AdminTransferred events, and propose_admin/accept_admin. No follow-up fix was needed — clean merge. Verified before merging: fmt, full workspace build, all 68 tests (including your new test_admin_transfer module), clippy -D warnings, and the wasm32v1-none release build. CI is green on main post-merge: https://github.com/Ads-Bazaar/ads-bazaar-contract/actions/runs/30161062224 |
PR49's branch was cut before #31 and #34 merged, so its new PayoutFrozen error variant independently claimed discriminant 24, the same slot #31 had already given to InvalidCreatorCount. Resolved the conflict by bumping PayoutFrozen to 25, keeping the additive-only numbering convention; no other change to the PR's logic. Co-Authored-By: Anuoluwapo Ali <anuoluwapoali25@gmail.com>
…50) storage::add_campaign_applicant rewrote an ever-growing Vec<Address> on every apply_to_campaign call just to answer a yes/no question in has_campaign_applicants. Replaces it with a u32 counter under DataKey::ApplicantCount, matching the existing approved_count pattern on Campaign, so applying costs O(1) storage-writes regardless of how many creators already applied. Adds a regression test that applies 200+ creators and asserts (via env.cost_estimate()) that the storage write cost of a later apply matches an early one, plus confirms update_campaign_metadata's lock-after-first-application behavior from #38 is unaffected. Closes #43 Verified locally in an isolated worktree before merge: - cargo fmt --all -- --check - cargo build --workspace - cargo test --workspace (64 tests) - cargo clippy --workspace --all-targets -- -D warnings - cargo build --workspace --target wasm32v1-none --release - Confirmed via git merge-tree that the merge combines cleanly with #34 and #49, which landed after this branch was cut.
2 participants
Summary
propose_admin/accept_adminfor safe two-step escrow admin rotation.PendingAdmin, emit typed admin handover events, and add an admin-onlyupdate_fee_bpssetter to verify post-transfer authority.Test plan
cargo test -p ads-bazaar-campaign-escrowCloses #17