feat(contracts): emergency circuit breaker with scoped function guards & time-bound auto-expiry - #46
Conversation
Adds a `pausable` module to the shared governance-guard crate: the emergency circuit breaker the core contracts have had no equivalent of. Until now the only responses to a live exploit were "watch it drain" or "upgrade under fire". A plain `bool paused` flag would be a rug pull waiting to happen, so the primitive is built around three properties instead: * **Scoped, not global.** A pause names a bitmask — SCOPE_INTAKE, SCOPE_SETTLEMENT, SCOPE_ATTESTATION — so entry paths can be halted independently of exit paths. Halting value entering escrow is a safety measure; halting value leaving it is a hostage situation, and only a scoped guard can express the first without the second. * **Fund-recovery paths are never guarded.** Enforced by omission at the call sites rather than by a runtime check, so it is stated explicitly in the module docs: there is no scope value, ALL_SCOPES included, that can reach a refund or a dispute. That wiring lands in the next commit. * **Time-bound, enforced on read.** A pause carries an `expires_at` ledger timestamp capped at MAX_PAUSE_DURATION (7 days), and expiry is evaluated on every guard consultation. A pause lapses with no unpause transaction, no live admin and no working key. Authorization is `require_signer` — any *single* governance signer, not the M-of-N threshold, mirroring `cancel_upgrade`. Gathering a threshold takes time an incident does not give you, and the action is bounded on every axis that matters. Using the signer set rather than each contract's own `admin` matters because the set is M-of-N, rotatable, and survives the loss of one key. `unpause` takes a mask and leaves the deadline alone, so an operator can bring the system back a piece at a time; re-calling `pause` with a narrower mask would also lift scopes but restarts the clock on the rest. Adds four GovernanceError variants (OperationPaused, InvalidPauseScope, InvalidPauseDuration, NotPaused) — OperationPaused is deliberately distinct from any status error, since "halted, retry later" and "this was never valid" call for opposite reactions from a client. State lives in one new instance key, GovernanceDataKey::PauseState, appended last so deployed contracts' key encodings stay put. 66 unit tests cover scope isolation, expiry boundaries, the duration cap, masked unpause, event emission, and adversarial auth.
…acts
Exposes pause/unpause/get_pause_state/paused_scopes/is_paused on escrow,
reputation, loyalty-token and loyalty-emissions, and places the scope
guards. Each contract gains the four pause error variants at whatever
offset came next in its own enum, so existing codes are untouched.
What is guarded:
SCOPE_INTAKE escrow: create_appointment, create_milestone_escrow,
add_milestone; loyalty-token: mint;
loyalty-emissions: create_schedule
SCOPE_SETTLEMENT escrow: confirm_completion, approve_milestone,
release_milestone_funds; loyalty-emissions: claim
SCOPE_ATTESTATION reputation: submit_attestation
What is *not*, and why — this is the load-bearing half:
escrow cancel_appointment, raise_dispute, resolve_dispute,
raise_milestone_dispute, resolve_milestone_dispute.
Every route by which an escrowed balance reaches
whoever is entitled to it. No scope reaches them.
loyalty-token transfer, transfer_from, approve, burn — these move
already-held balances. A holder's points are their
property; only mint creates supply that doesn't yet
exist, so only mint is guarded.
loyalty-emissions reclaim — mints and burns nothing, so it cannot take
a balance anyone holds.
all every read-only view, so a consumer can always see
the state that prompted the halt.
Pausing settlement withholds a payout, so it gets an explicit argument
rather than an assumption: it changes no party's power relative to the
others (a client could always cancel a Funded appointment, either party
could always force a dispute — both stay open), and
release_milestone_funds is permissionless, which is exactly why it has to
be haltable. On emissions, vesting is a pure function of the ledger clock
and keeps accruing through a pause, so a halted claim mints the identical
amount afterwards, just later.
Guards run as the first statement of each entrypoint, ahead of auth and
any storage read: a halted call should cost nothing and reveal nothing
beyond the already-public pause state.
Tests, per contract, covering the criteria in the issue: refund and
dispute resolution succeed with ALL_SCOPES halted and money actually
lands; intake blocked while no funds move; scope isolation in both
directions; auto-expiry restoring service with no unpause transaction;
non-signer and admin-key pause attempts rejected with NotASigner; the
duration cap refused; partial unpause. Plus the cross-contract case where
halting the token's mint stops emission claims underneath a healthy
engine.
Adds an "Emergency circuit breaker" section to the contracts README covering the scope table, the never-guarded recovery paths and why each one is exempt, the auto-expiry guarantee and its honest limit (an unattended pause always clears; a present admin can re-arm, which the upgrade flow already implies), the authorization model, the new entrypoints and events, per-contract error codes, storage layout, and CLI usage. Replaces the "Upgrade path exists; pause does not" bullet in the security notes — now false — with a statement of what compromising one signer key actually buys: a bounded liveness attack on new business, and no reach into any existing balance. Sets cache-on-failure on the Soroban CI cache. A run that fails on fmt or clippy has still built the whole dependency tree; caching it keeps the retry from recompiling soroban-sdk from scratch, which is most of the job's wall time. (Cargo caching itself was already in place.) Notes for review, not fixed here: the "Upgrade governance" section and the "Notes / follow-ups" list still describe the signer set as immutable after initialize, which signer rotation made stale before this change. Left alone to keep this diff to one concern.
Assessment — Request changes. This is an excellent, well-explained, and carefully-scoped design for a shared emergency circuit breaker. The scoped bitmask, time-bound expiry, and explicit omission of fund-recovery paths are the right trade-offs for safety and recoverability. Conceptually I approve the approach, but before merging I want a few clarifications, a couple of security / testing gaps closed, and some documentation to be added. But a couple of bottle-necks and things to take note of: Blocking / required changes
Minor nits (non-blocking)
Suggested PR review text to paste into GitHub "Summary
Requested changes before merge
Once those items are addressed (tests + docs + events + gas notes), I’ll re-review and we can merge. Conceptually this is a strong approach and I’m excited to get it in — thanks for the thorough design and tests so far." Next steps from me
If you want, I can:
Overall: conceptually approved, but request changes until the test, observability, and docs gaps are closed. |
Addresses review feedback on workman-labs#46. Adds a length-capped `reason` to `pause`, stored with the record and emitted with the event, so "why is this halted?" is answerable from chain state rather than from a chat log nobody can find at 3am. Capped at 64 bytes because the record lives in instance storage that every subsequent invocation pays to load — an incident ticket reference belongs on-chain, the write-up does not. Empty is allowed, so the field never stands between a responder and a halt. New `InvalidPauseReason` error, appended per contract so nothing existing moved. Adds hot-path cost measurement using the SDK's budget metering, since the guard runs on every guarded entrypoint and its cost should be measured rather than assumed: trivial instance-storage view 51,549 CPU insns paused_scopes(), no pause record 51,717 (+168) paused_scopes(), live pause record 77,023 (+25,474) create_appointment, no pause record 336,299 create_appointment, live record on another scope 365,219 (+8.6%) create_appointment rejected while paused 78,216 (23%) In normal operation — no pause ever set, which is the state the contracts are in essentially always — the guard costs ~168 instructions against a 336k booking. The ~25k figure is deserializing the record and is paid only while an incident is in progress. A rejected call costs ~23% of the work it replaces, because the guard sits ahead of auth and every other storage access, which is what makes a pause a usable response to an entrypoint being hammered rather than an amplifier. No micro-optimization is warranted at these numbers; the cap on `reason` is what keeps the incident-time figure bounded too. Native-test metering underestimates compiled Wasm, so the tests fence relative behaviour, not absolute cost. New tests: reason round-trip through storage and event, cap boundary at exactly 64 and 65 bytes, full PauseState serialization round-trip, broadcast no-ops on escrow and loyalty-token for scopes they don't implement, a genuine two-contract sweep with independent records and independent signer sets, authority not leaking between deployments, and ordering cases — a second signer's pause observing the first, and repeated guard consultations within one ledger all agreeing. 267 tests total. Documents what was previously left implicit: what "enforced on read" means and what a guard consultation is; that expiry cannot land mid-transaction, since the ledger timestamp is fixed for the whole invocation; which clock is used and why validator influence points the harmless way (forward only ends a pause sooner, and recovery paths consult no clock at all); why wall-clock seconds rather than ledger sequence; that Soroban forbids reentrancy and pause/unpause make no cross-contract calls at all; worked example flows; and an explicit pointer to the workman-labs#27 rotation flow, including the deliberate asymmetry that rotating who may pause is timelocked while pulling the alarm is not. Adds scripts/broadcast-pause.sh for sweeping a mask across all four contracts, and CHANGELOG.md.
|
Thanks — pushed One general note: several items are framed in Solidity terms ( 1. TestsMost of these were already in the branch. Concretely, by request:
Genuinely missing, now added:
Reentrancy / races. There is nothing to race here, and I've documented why rather than writing a test that would only look reassuring:
What is testable is that the second actor sees the first's write, so I added Total: 267 tests, all passing. 2. Events & observabilityAlready implemented — Added the optional One thing worth stating explicitly, now in the docs: auto-expiry emits nothing. It's a read-time evaluation with no transaction behind it, so there's no execution context to emit from. Indexers should treat 3. Documentation & changelogModule docs already covered the three constraints and the authorization rationale. Added the parts that were genuinely missing:
4. Authorization & governance clarityExpanded the module docs with an explicit pointer to the #27 rotation flow ( The on-chain check tests you asked for already existed (§1 above) — including that each contract's own 5. Gas / hot-path costMeasured with the SDK's budget metering rather than estimated. Tests are in
The shape that matters: in normal operation — no pause ever set, which is the state the contracts are in essentially always — the guard costs ~168 instructions against a 336k booking. That's a rounding error. The ~25k figure is deserializing the A rejected call costs ~23% of the work it replaces, because the guard is the first statement of each entrypoint, ahead of auth and every other storage access. That's deliberate — it makes a pause a usable response to an entrypoint being hammered rather than an amplifier. On your "if significant, consider micro-optimizations": I don't think any are warranted at these numbers, and I'd rather not add caching complexity to a safety mechanism to save 168 instructions. On the storage read specifically — the instance entry is already in the footprint of any invocation that touches admin or governance state, so it's a hit on an entry the host has loaded regardless, not an extra ledger read. The tests fence relative behaviour (a loose "must not approach doubling") rather than absolute numbers, since native-test metering underestimates compiled Wasm and I don't want a brittle CI failure on an SDK bump. 6. Edge cases
7. API / UX
Minor nits
Still not fixed, deliberately
CI is green: |
|
Approved — minor nits. Minor follow-ups I'd like before merging
Optional / future ideas (no block)
Suggested review text to post (paste-ready) Next steps from me
Thanks — this is a solid, well-tested safety primitive and a great improvement to core contract resilience. |
…able Addresses the four follow-ups on workman-labs#46. Two of them turned out to be statements about behaviour rather than prose, so they are pinned by assertion instead of described and hoped for. Event wire format. Documented the exact topic ordering and data shape, and added tests that assert it against real emitted events, so the README table cannot drift from what is emitted without a test failing: topics Symbol("gov_pause"), Symbol("paused"|"unpaused"), Address(caller) — prefix symbols first, then each #[topic] field in declaration order data Map<Symbol, Val> keyed by field name, since contractevent's data_format defaults to "map". Field *names* are part of the contract, their order is not — indexers must key, not position. Verified rather than assumed: the map format is the macro default, which is worth stating because "data" reads as a tuple if you only see the struct. Added an example event JSON for indexers to copy, per the optional suggestion. The reason cap is bytes, not characters. MAX_PAUSE_REASON_LEN bounds what String::len() reports, which is the UTF-8 byte length. For ASCII the distinction is invisible, which is exactly why it needed pinning — a client validating character count would submit reasons the contract rejects. Two tests make it executable rather than assertable-by-comment: 32 characters of U+00E9 is exactly 64 bytes and is accepted; 33 characters is 66 bytes and is rejected despite being half a 64-character budget. Documented on the constant, in PauseState, on `pause`, and in the README with the byte-count idioms for JS and Python. Benchmark table moved into the README with the metering caveat made prominent rather than parenthetical: these are SDK-test-metered numbers that underestimate compiled Wasm and carry no instantiation cost, so they are a relative comparison, not a fee estimate. Also promoted the circuit-breaker section's bold labels to real headings with a jump list, since it had grown past the point where prose bolding was navigable. Heading names are deliberately distinctive (Pause errors, Pause storage layout) so their anchors don't collide with the per-contract Errors and CLI usage sections further down and shift existing ones. 271 tests, all passing.
and PR workman-labs#46 Pairs with the release-notes link now in the PR description, so a releaser can get from either end to the other.
|
Thanks! All four pushed — Two of these turned out to be claims about behaviour rather than prose, so I pinned them with assertions instead of just writing them down. If the wire format or the byte cap ever drifts, a test fails rather than the README quietly becoming wrong. 1.
|
| Event | Topics (in order) | Data map |
|---|---|---|
Paused |
Symbol("gov_pause"), Symbol("paused"), Address(caller) |
expires_at: u64, reason: String, scopes: u32 |
Unpaused |
Symbol("gov_pause"), Symbol("unpaused"), Address(caller) |
scopes: u32, remaining_scopes: u32 |
One thing worth flagging for indexer authors, which I'd left implicit and which surprised me when I checked: the data is a Map<Symbol, Val> keyed by field name, not a tuple — contractevent's data_format defaults to "map". So field names are part of the contract but their order is not; index by key, never by position. Easy to get wrong from reading the struct definition alone.
Topic ordering is: the prefix symbols from the attribute first, then each #[topic] field in declaration order.
Both shapes are asserted against real emitted events in contracts/escrow/src/test.rs — paused_event_has_the_documented_topics_and_data_shape and its unpaused counterpart. Also added the example event JSON you suggested, so an indexer author can copy the shape rather than derive it.
3. Benchmark table in the docs
Moved into the README under Hot-path cost, with the metering caveat promoted from a parenthetical to a blockquote right under the table:
⚠️ These are SDK-test-metered numbers, not absolute Wasm costs or fees. Per the SDK's own documentation, native Rust test execution underestimates both CPU and memory relative to the compiled Wasm, and this harness charges no Wasm instantiation. They are meaningful as a relative comparison of the same call with and without a pause record — and should not be used to size a transaction fee.
The changelog links to the table, and the table points at the tests, so an operator or future reviewer lands on the caveat before the numbers either way.
4. CHANGELOG placement
PR description now opens with a Release notes link to soroban-contracts/CHANGELOG.md, noting the entry lives under ## [Unreleased] with Added / Changed / Notes. The changelog entry links back to #42 and this PR (caa1145), so a releaser can get from either end to the other. I also refreshed the description's own details that the follow-up round had made stale — test count, the pause signature, and the events line.
Optional items
- Example event JSON — added, per above.
- Fixed-size byte array for
reason— agreed it's not needed now, and I'd lean against it later too:BytesN<32>would force callers to hash or truncate, which costs the human-readability that's the whole point of the field, and the cap already bounds the storage cost. Worth revisiting only if incident-time cost ever shows up as a real constraint, which the ~25k figure suggests it won't. Happy to be overruled if you'd rather have the predictability.
Also promoted the circuit-breaker section's bold labels to real headings with a jump list, since it had outgrown prose bolding. The heading names are deliberately distinctive (Pause errors, Pause storage layout) so their anchors don't collide with the per-contract Errors / CLI usage sections further down and silently shift those.
271 tests passing; fmt, clippy -D warnings, cargo test --workspace and the release WASM build all green.
The only thing still deliberately untouched is the stale "signer set is immutable after initialize" claim in the Upgrade governance section, left over from #27. Say the word and I'll fold it in here or open a follow-up.
meshackyaro
left a comment
There was a problem hiding this comment.
Approve — great work.
This PR adds a well-reasoned, thoroughly-tested scoped emergency circuit breaker and wires it into the four core contracts. Tests (267), events (with a capped reason), docs, benchmarks, ops tooling (broadcast script), and CHANGELOG are all in place and CI is green. The design balances safety (fund-recovery paths unguarded), operability (single-signer pause/unpause, automatic expiry), and observability (events + reason + view helpers). I’m happy to approve and merge this PR — thanks @bbjiggy for working on it.
Closes #42
Release notes:
soroban-contracts/CHANGELOG.md— the
## [Unreleased]section carries this change's entry (Added / Changed /Notes), ready for a releaser or release automation to pick up. The changelog
links back to this PR and to the benchmark table, so the two stay paired.
Key docs: Emergency circuit breaker
· Events / wire format
· Hot-path cost
What this adds
A shared emergency circuit breaker across
escrow,reputation,loyalty-tokenandloyalty-emissions. Until now none of them had any haltmechanism — the only responses to a live exploit were "watch it drain" or
"upgrade under fire", and the upgrade path itself is still being built out.
The primitive lives in
contracts/governance-guard's newpausablemodule,next to the upgrade guard and for the same reason: every contract that needs it
already depends on that crate.
The design constraint
A plain
bool pausedflag would be a rug pull waiting to happen. Threeproperties are load-bearing, each closing a failure mode that a boolean leaves
open.
1. Scoped, not global. A pause names a bitmask, so entry paths halt
independently of exit paths.
SCOPE_INTAKE1escrow:create_appointment,create_milestone_escrow,add_milestone·loyalty-token:mint·loyalty-emissions:create_scheduleSCOPE_SETTLEMENT2escrow:confirm_completion,approve_milestone,release_milestone_funds·loyalty-emissions:claimSCOPE_ATTESTATION4reputation:submit_attestationA contract with no entrypoint in some scope ignores a pause naming it, so an
operator can broadcast one mask to every contract without special-casing.
2. Fund-recovery paths are never guarded. This is the half the issue is
really about, and it is enforced by omission at the call sites — so it's
stated explicitly in the module docs and pinned by tests rather than left
implicit:
escrowcancel_appointment,raise_dispute,resolve_dispute,raise_milestone_dispute,resolve_milestone_disputeloyalty-tokentransfer,transfer_from,approve,burnmint— the sole path creating supply that doesn't yet exist — is guarded.loyalty-emissionsreclaimThere is no scope value,
ALL_SCOPESincluded, that reaches any of these.3. Time-bound, enforced on read. A pause carries an
expires_atledgertimestamp capped at
MAX_PAUSE_DURATION(7 days), and expiry is evaluated onevery guard consultation — so a pause lapses with no unpause transaction, no
live admin, no working key.
Two judgement calls worth flagging in review
Pausing
SCOPE_SETTLEMENTwithholds a payout, so it deserves an argumentrather than an assumption. It's a delay, not a seizure: it changes no party's
power relative to the others — a client could always cancel a
Fundedappointment for a full refund, and either party could always force
raise_dispute→resolve_dispute; both stay open throughout. Andrelease_milestone_fundsis permissionless, which is precisely why it has tobe haltable, or the scope is decorative. On emissions, vesting is a pure
function of the ledger clock and keeps accruing through a pause, so a halted
claimmints the identical amount afterwards, just later.Authorization is any single governance signer, not the M-of-N threshold and
deliberately not each contract's own
admin. Gathering a threshold takes timean incident doesn't give you, and the action is bounded on every axis that
matters — this mirrors
cancel_upgrade, which is unilateral for the samereason. The signer set rather than
adminbecause it's M-of-N, rotatable viathe timelocked flow from #27, and survives the loss of one key; an incident is
exactly when a single non-rotatable key is least trustworthy.
unpauseisunilateral in the same way, so a responder who pauses and then goes offline
can't wedge it in place.
What this does not promise: an admin who is present and hostile can
re-pause each time the window lapses. That isn't closable here — the same
signer set can already replace all of a contract's code via the upgrade flow.
The honest guarantee is narrower: an unattended pause always clears, and no
pause of any duration can stop a user recovering funds they already own. The
second half is what keeps the residual risk a liveness problem for new
business rather than a custody problem for existing balances.
Interface
Added to all four contracts:
pause(caller, scopes, duration_secs, reason) -> PauseState—reasonisoperator context capped at 64 UTF-8 bytes (not characters), may be empty
unpause(caller, scopes) -> u32— clears only the named scopes and returnswhat's still halted, without touching the deadline. Re-calling
pausewith a narrower mask would also lift scopes but restarts the clock on
everything left, so partial
unpauseis how you bring the system back apiece at a time.
get_pause_state() -> Option<PauseState>(Noneonce expired),paused_scopes() -> u32,is_paused(scope) -> boolEvents for off-chain monitoring:
Paused { caller, scopes, expires_at, reason }and
Unpaused { caller, scopes, remaining_scopes }. Topic ordering and theMap<Symbol, Val>data shape are documented in the README and pinned byassertion in
contracts/escrow/src/test.rs, so they cannot drift silently. Auto-expiry emits nothing —it's a read-time evaluation with no transaction behind it, so monitors should
treat
Paused.expires_atas the authoritative end of the window unless anUnpausedarrives sooner.A dedicated
OperationPausederror per contract, not a reusedInvalidStatus: "the protocol is halted, retry later" and "this request wasnever valid" call for opposite reactions from a client. Plus
InvalidPauseScope,InvalidPauseDuration,NotPaused, each appended atwhatever offset came next in that contract's existing enum so no existing code
moved.
Storage & safety
One new instance key,
GovernanceDataKey::PauseState, holdingPauseState { scopes, expires_at, paused_by, paused_at }— appended afterPendingRotationso already-deployed contracts' key encodings stay put. Onlyone record exists at a time; a second
pausereplaces the first outrightrather than layering, so halted scopes and deadline are always readable from
one place.
paused_byis recorded for attribution and grants no rights.Guards run as the first statement of each entrypoint, ahead of auth and any
storage read: a halted call costs nothing and reveals nothing beyond the
already-public pause state.
Tests — 271 total, all passing
66 new unit tests in
governance-guard(scope isolation, expiry boundariesincluding the exact
expires_atinstant, the duration cap, masked unpause,event emission, adversarial auth), plus per-contract integration tests for the
wiring. The ones tied directly to the acceptance criteria:
a_client_can_still_cancel_and_be_refunded_while_everything_is_paused—balances asserted, and the pause verified still in force afterwards
disputes_can_still_be_raised_and_resolved_while_everything_is_pausedmilestone_disputes_resolve_while_everything_is_pausedholders_keep_full_control_of_existing_balances_while_everything_is_paused—transfer, transfer_from, burn all succeed under
ALL_SCOPESintake_resumes_on_its_own_once_the_pause_expires— only the clock movesa_non_signer_cannot_pause_the_escrow,the_admin_arbiter_is_not_a_pause_authoritya_pause_longer_than_the_cap_is_refused_outrightpausing_the_token_alone_stops_claims_at_the_mint_boundary— cross-contracta_lapsed_pause_can_be_placed_again_by_a_present_admin— pins the limit ofthe guarantee so nobody later reads it as stronger than it is
CI
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspaceandcargo build --release --target wasm32v1-noneallrun clean locally. Clippy already blocks warnings on PRs, satisfying the
"warnings automatically blocked" criterion.
Cargo dependency caching was already in place (
Swatinem/rust-cache@v2); Iadded
cache-on-failure: true, since a run that fails on fmt or clippy hasstill built the whole dependency tree and the retry shouldn't recompile
soroban-sdkfrom scratch.Noted, not fixed here
soroban-contracts/README.md's "Upgrade governance" section and its"Notes / follow-ups" list still describe the signer set as immutable after
initialize— stale since signer rotation (#27) landed, and unrelated to thischange. Left alone to keep the diff to one concern; happy to fix in a
follow-up if you'd prefer.