docs: spec a deferred diamond-cleanup queue (park-and-drain facet removals)#2049
Closed
0xDEnYO wants to merge 2 commits into
Closed
docs: spec a deferred diamond-cleanup queue (park-and-drain facet removals)#20490xDEnYO wants to merge 2 commits into
0xDEnYO wants to merge 2 commits into
Conversation
…-and-drain facet removals) Design spec (no code) for parking deprecation-driven facet removals in a durable queue and draining them opportunistically on the next multisig action per network, instead of an eager fleet-wide removal at deprecation time. Builds on PR #2047 / docs/FacetRemovalReconciliation.md. Compares store options (recommends a new sc_private.parkedTasks Mongo collection), identifies the sendOrPropose/runPropose drain chokepoint, and makes the originating-PR link visible to the multisig reviewer at signing time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 59 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8 tasks
0xDEnYO
added a commit
that referenced
this pull request
Jul 15, 2026
…ue (#2051) * feat(safe): parked-tasks store layer for deferred diamond-cleanup queue Persistence layer for the deferred diamond-cleanup queue (design: PR #2049, docs/DeferredDiamondCleanupQueue.md). New sc_private.parkedTasks collection modelled on the timelock-operations/queue sibling and reusing the Safe/Mongo plumbing: - IParkedTask schema (spec §4); selectors resolved from the loupe at drain, not stored, so a parked list can never go stale - computeTaskKey + partial unique index on taskKey over open statuses {queued, proposed} (dedup independent of the non-deterministic timelock salt) - enqueue (E11000 -> null dedup; refuses a missing/blank prUrl), list, atomic claimForProposal flip, and the executed/superseded/cancelled/revert transitions - injectable Collection I/O; 100% unit coverage except the thin live-Mongo adapter Store layer only — no drain chokepoint, no removal engine; independent of PR #2047. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(safe): parked-tasks read + enqueue CLIs - list-parked-tasks.ts: read-only queue view grouped by network (facet | status | age | origin PR | safeTxHash) with per-network queued/proposed counts; --network/--pr/--status/--json, exit 0/1/2. Mirrors list-pending-proposals.ts. - enqueue-parked-task.ts: parks one facet-removal task; refuses without prUrl, validates addresses via viem, production-only in v1, resolves enqueuer from git user.email. Delegates dedup/insert to the tested enqueueParkedTask helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(safe): tighten parked-tasks after adversarial review - markCancelled: restrict to queued only — cancelling a proposed task would orphan its live Safe removal proposal from the origin-PR linkage (spec §6); the revert-to-queued -> cancel path handles that case explicitly - test: in-memory fake now honors findOneAndUpdate's returnDocument option, so a regression dropping returnDocument:'after' would fail the post-update assertions - list-parked-tasks: de-duplicate --network values (was double-listing/counting) - doc: note the safeTxHash setter is intentionally deferred to the drain PR Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(safe): address CodeRabbit review on parked-tasks - enqueueParkedTask now normalizes at the store chokepoint (trims network/ facetName/prUrl, rejects a blank facetName) so every caller dedups consistently — a stray space no longer mints a distinct undeduplicated task; enqueue CLI adds a matching fast-fail before connecting (CR: enqueue-parked-task.ts) - enqueue/list CLIs: set process.exitCode=1 instead of process.exit(1) in the inner catch so the finally block still closes the Mongo client (CR major) - getParkedTasksCollection: use fetchWithTimeout for the VPN probe so a slow ipify response can't hang the CLI (CR + reuse-helpers rule) - tests: cover facetName blank-check + network/facetName/prUrl trimming Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(safe): move parked-tasks queue to non-sensitive MONGODB_URI cluster Addresses review (gvladika): the store's cluster choice rested on a false premise. The cited precedent (timelock-queue.ts) runs on the non-sensitive MONGODB_URI cluster, not sc_private. Nothing parked is secret (public facet names, on-chain addresses, PR URLs) and the security boundary is on-chain, so un-gated is correct and unblocks non-interactive consumers (CI backlog reports, reconcile/TTL jobs, agent-driven /deprecate-contract) that cannot open a tunnel. - getParkedTasksCollection now mirrors getTimelockQueueCollection: MONGODB_URI, DB deferred-cleanup, no VPN/ipify gate. That gate was retired code copied from the pre-EXSC-615 getSafeMongoCollection (removed from main in f5b88ca); under lifi-connect port-forwarding it could never pass, so both CLIs always exited 2. - CLIs: recoverable-misconfig (exit 2) detection now keys on MONGODB_URI. - Corrected the false timelock-queue precedent claim in the module header. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…merged store #2051 Update the design spec to the decisions agreed in #dev-sc-review (Goran + Daniel): - Store (§5, §9, Q1): the queue lives on the NON-sensitive MONGODB_URI cluster (DB deferred-cleanup, collection parkedTasks), mirroring timelock-queue.ts — not the sc_private/VPN-gated signing store. Nothing parked is secret; the security boundary is on-chain; un-gated so CI/reconcile/agent consumers reach it without a tunnel. Chosen over a git file because the state is mutable cross-session. Shipped in #2051, so store-layer facts are now verified against origin/main. - Drain chokepoint (§6, §10, Q2): hook runPropose (propose-to-safe.ts:58), NOT sendOrPropose / main() / the multisig-rollout skill. runPropose is the true funnel — main() and proposeDiamondCut (EVM+Tron) both route through it; the agentic facet-cut path never touches main(). Extract a pure _runPropose; public runPropose drains in a try/catch so the primary proposal and exit code are unaffected. Honest gap: sendOrPropose-only paths fall to the cold-network backstop. - Concurrency (§6, §7): claimForProposal() atomic queued->proposed flip is THE dedup gate, independent of the salt-nondeterministic intentHash. - Opt-out flag (§6, §11): DRAIN_PARKED_TASKS ON for rollouts, OFF for emergencies. - Visibility (§6): drained removal logged loudly + PR link into confirm-safe-tx detailLines + list-pending-proposals + Slack. - §13 marks store + CLIs DONE (#2051); §14 marks Q1/Q2 resolved and keeps Q4 (batching) / Q8 (enqueue timing) as clearly-labelled open recommendations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Folding this into #2047. The deferred-cleanup-queue spec ( |
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
2 participants
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.
Which Linear task belongs to this PR?
Part of the deferred diamond-cleanup effort (child of EXP-350, alongside the
EXSC-193 removal work). The store layer this spec describes has since shipped in
#2051. A dedicated SmartContract (EXSC) ticket for the spec still needs to be
created/linked — flagged for follow-up.
Why did I implement it this way?
Follow-up to PR #2047 (
docs/FacetRemovalReconciliation.md). Today/deprecate-contractstep 6 proposes the on-chain facet removal immediately — across ~71 mainnet
diamonds that's a mass Safe-signing event of irreversible
diamondCuts, i.e. signerfatigue, for a cleanup that isn't time-critical.
This spec parks each removal in a durable queue at deprecation time (carrying a link
to the deprecation PR) and drains it opportunistically — the next time a facet cut
happens on that network, the cleanup rides along in the same signing session. The
originating-PR link is surfaced to the multisig reviewer at signing time (first-class
requirement). No governance change — removals stay Safe- + timelock-gated.
Doc:
docs/DeferredDiamondCleanupQueue.md.What changed in this revision
This revision encodes the decisions agreed in the #dev-sc-review thread (Goran +
Daniel) and reflects the merged store layer (#2051). The two questions previously
flagged for a read are now resolved:
MONGODB_URIcluster (DB
deferred-cleanup, collectionparkedTasks), mirroring thetimelock-operations/queuesibling — not thesc_private/tunnel-gated signingstore the earlier draft named. Nothing parked is secret; the security boundary is
on-chain; un-gated so CI / reconcile / agent-driven
/deprecate-contractreach itwithout a tunnel. Chosen over a git file because the state is mutable cross-session.
Shipped in feat(safe): parked-tasks store layer for deferred diamond-cleanup queue #2051.
runPropose(propose-to-safe.ts:58) —not
sendOrPropose, notmain(), not themultisig-rolloutskill.runProposeisthe true funnel:
main()andproposeDiamondCut(EVM + Tron) both route through it,and the agentic facet-cut path never touches
main(). PublicrunProposedrains ina
try/catchso the primary proposal and exit code are unaffected.Also encoded:
claimForProposal()as the atomic dedup gate (§6/§7), theDRAIN_PARKED_TASKSflag semantics (ON for rollouts, OFF for emergencies, §6/§11), andthe loud-log + PR-link-to-signer visibility requirement (§6). §13 marks the store +
CLIs done (#2051); §14 keeps Q4 (batching) and Q8 (enqueue timing) as clearly-labelled
open recommendations rather than pretending they're settled.
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)