feat: implement contract pausability (circuit breaker)#30
Merged
Bosun-Josh121 merged 6 commits intoLightForgeHub:mainfrom Feb 26, 2026
Merged
Conversation
Add the circuit breaker data layer: IsPaused DataKey with set_paused/is_paused helpers, and ContractPaused error variant (code 8) for enforcement.
Emit a "paused" event with a bool indicating the new pause state whenever the admin toggles the circuit breaker.
Add admin-only pause() and unpause() contract methods with require_auth. Add is_paused guard at the top of all state-changing functions (book_session, finalize_session, reclaim_stale_session, reject_session) that returns ContractPaused error when the circuit breaker is active. Read-only getters remain unaffected.
Cover all acceptance criteria: - pause blocks book_session, finalize_session, reclaim_stale_session, reject_session - unpause resumes normal operations - read-only getters remain active while paused
📝 WalkthroughWalkthroughAdds an admin-controlled circuit breaker to the payment-vault contract: persistent paused state, pause/unpause admin methods, paused checks blocking state-changing flows, an event for pause state, and tests validating pausability while preserving read-only access. Minor import/formatting tweaks elsewhere. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
actor Admin
end
rect rgba(200,255,200,0.5)
participant Contract
participant Storage
participant User
end
Admin->>Contract: pause(env)
activate Contract
Contract->>Contract: admin.require_auth()
Contract->>Storage: set_paused(env, true)
Storage-->>Contract: ok
Contract->>Contract: emit contract_paused(true)
Contract-->>Admin: Ok
deactivate Contract
User->>Contract: book_session(...)
activate Contract
Contract->>Storage: is_paused(env)
Storage-->>Contract: true
Contract-->>User: Err(ContractPaused)
deactivate Contract
Admin->>Contract: unpause(env)
activate Contract
Contract->>Contract: admin.require_auth()
Contract->>Storage: set_paused(env, false)
Storage-->>Contract: ok
Contract->>Contract: emit contract_paused(false)
Contract-->>Admin: Ok
deactivate Contract
User->>Contract: book_session(...)
activate Contract
Contract->>Storage: is_paused(env)
Storage-->>Contract: false
Contract->>Contract: proceed with booking
Contract-->>User: Ok(booking_id)
deactivate Contract
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/payment-vault-contract/src/test.rs (1)
669-671: Assert the specificVaultError::ContractPauseddiscriminant, not justis_err().Each paused-state check asserts
result.is_err(), but this passes even if the error isBookingNotFoundor any other unrelated variant. Pinning the error type confirms the pause guard — not some other code path — is being exercised.💡 Example stronger assertion
use crate::error::VaultError; let result = client.try_book_session(&user, &expert, &10, &100); assert_eq!( result.unwrap_err().unwrap(), VaultError::ContractPaused );Also applies to: 701-702, 733-734, 761-762, 786-787
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 669 - 671, The test currently only checks that try_book_session(...) returns an error; update the assertions to assert the specific VaultError::ContractPaused discriminant so the pause guard is verified (replace assert!(result.is_err()) with an equality check like assert_eq!(result.unwrap_err().unwrap(), VaultError::ContractPaused)). Apply this change for the failing checks that call try_book_session in the same test (and the other occurrences noted), and ensure you import or reference crate::error::VaultError if needed.contracts/payment-vault-contract/src/events.rs (1)
27-31: Consider disambiguating the pause/unpause event topic.The single
"paused"topic is emitted for bothpause()andunpause()calls; consumers must inspect the boolean payload to distinguish them. Two approaches (pick one):
- Keep one function but use a more neutral name like
contract_pause_toggled, or- Use two separate topics (
symbol_short!("paused")/symbol_short!("unpaused")).This is a UX concern for indexers and off-chain listeners — the current implementation is not incorrect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/events.rs` around lines 27 - 31, The event topic "paused" emitted by contract_paused is ambiguous for pause vs unpause; either rename the function/topic to something neutral or split into distinct topics. Option A: keep the function but rename it to contract_pause_toggled (or similar) and keep the payload boolean, leaving the topic as symbol_short!("paused") to indicate a toggle; Option B: create two separate events (e.g., contract_paused and contract_unpaused) and publish symbol_short!("paused") for the pause path and symbol_short!("unpaused") for the unpause path, each emitting a clear payload or unit to disambiguate for indexers. Ensure you update any callers of contract_paused to the new name(s) and adjust tests/consumers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Around line 26-40: The pause/unpause functions should first read the current
paused state (e.g. via storage::is_paused(env) or storage::get_paused(env)) and
if the requested state equals the current state return a clear error (add
VaultError::AlreadyPaused and VaultError::AlreadyUnpaused or similar) instead of
proceeding; update contract::pause and contract::unpause to require auth, check
the current state, return the new error when redundant, and only call
storage::set_paused(...) and events::contract_paused(...) when an actual state
change occurs.
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 648-833: The tests currently call env.mock_all_auths() which
disables Soroban auth checks, so add new tests (e.g.,
test_non_admin_cannot_pause and test_non_admin_cannot_unpause) that initialize
via create_client and client.init but then clear or replace mocked auths (use
env.set_auths or equivalent) to supply only a non-admin signer before calling
client.try_pause() and client.try_unpause(), and assert those calls return Err;
reference try_pause, try_unpause, create_client, client.init,
env.mock_all_auths, and env.set_auths to locate where to change the auth setup
and assertions.
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/events.rs`:
- Around line 27-31: The event topic "paused" emitted by contract_paused is
ambiguous for pause vs unpause; either rename the function/topic to something
neutral or split into distinct topics. Option A: keep the function but rename it
to contract_pause_toggled (or similar) and keep the payload boolean, leaving the
topic as symbol_short!("paused") to indicate a toggle; Option B: create two
separate events (e.g., contract_paused and contract_unpaused) and publish
symbol_short!("paused") for the pause path and symbol_short!("unpaused") for the
unpause path, each emitting a clear payload or unit to disambiguate for
indexers. Ensure you update any callers of contract_paused to the new name(s)
and adjust tests/consumers accordingly.
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 669-671: The test currently only checks that try_book_session(...)
returns an error; update the assertions to assert the specific
VaultError::ContractPaused discriminant so the pause guard is verified (replace
assert!(result.is_err()) with an equality check like
assert_eq!(result.unwrap_err().unwrap(), VaultError::ContractPaused)). Apply
this change for the failing checks that call try_book_session in the same test
(and the other occurrences noted), and ensure you import or reference
crate::error::VaultError if needed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/identity-registry-contract/src/events.rscontracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/error.rscontracts/payment-vault-contract/src/events.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rscontracts/payment-vault-contract/src/types.rs
Comment on lines
+26
to
+40
| pub fn pause(env: &Env) -> Result<(), VaultError> { | ||
| let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?; | ||
| admin.require_auth(); | ||
| storage::set_paused(env, true); | ||
| events::contract_paused(env, true); | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn unpause(env: &Env) -> Result<(), VaultError> { | ||
| let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?; | ||
| admin.require_auth(); | ||
| storage::set_paused(env, false); | ||
| events::contract_paused(env, false); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
pause/unpause should guard against redundant state transitions.
Calling pause() when the contract is already paused silently succeeds and emits a spurious paused=true event; unpause() when already unpaused does the same. This creates misleading audit trails for indexers and monitoring tools and gives callers no signal that the call was a no-op.
Consider returning an error (or at minimum skipping the event) when the requested state matches the current state:
🛡️ Proposed fix for idempotency guards
pub fn pause(env: &Env) -> Result<(), VaultError> {
let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?;
admin.require_auth();
+ if storage::is_paused(env) {
+ return Err(VaultError::ContractPaused); // already paused
+ }
storage::set_paused(env, true);
events::contract_paused(env, true);
Ok(())
}
pub fn unpause(env: &Env) -> Result<(), VaultError> {
let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?;
admin.require_auth();
+ if !storage::is_paused(env) {
+ return Err(VaultError::NotInitialized); // reuse or add a dedicated NotPaused variant
+ }
storage::set_paused(env, false);
events::contract_paused(env, false);
Ok(())
}Alternatively, add a dedicated VaultError::NotPaused = 9 variant for clarity instead of reusing NotInitialized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/contract.rs` around lines 26 - 40, The
pause/unpause functions should first read the current paused state (e.g. via
storage::is_paused(env) or storage::get_paused(env)) and if the requested state
equals the current state return a clear error (add VaultError::AlreadyPaused and
VaultError::AlreadyUnpaused or similar) instead of proceeding; update
contract::pause and contract::unpause to require auth, check the current state,
return the new error when redundant, and only call storage::set_paused(...) and
events::contract_paused(...) when an actual state change occurs.
Comment on lines
+648
to
+833
| #[test] | ||
| fn test_pause_blocks_book_session() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let admin = Address::generate(&env); | ||
| let user = Address::generate(&env); | ||
| let expert = Address::generate(&env); | ||
| let oracle = Address::generate(&env); | ||
|
|
||
| let token_admin = Address::generate(&env); | ||
| let token = create_token_contract(&env, &token_admin); | ||
| token.mint(&user, &10_000); | ||
|
|
||
| let client = create_client(&env); | ||
| client.init(&admin, &token.address, &oracle); | ||
|
|
||
| // Admin pauses the contract | ||
| let result = client.try_pause(); | ||
| assert!(result.is_ok()); | ||
|
|
||
| // User tries to book a session while paused (should fail) | ||
| let result = client.try_book_session(&user, &expert, &10, &100); | ||
| assert!(result.is_err()); | ||
|
|
||
| // Verify user's balance is unchanged | ||
| assert_eq!(token.balance(&user), 10_000); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_pause_blocks_finalize_session() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let admin = Address::generate(&env); | ||
| let user = Address::generate(&env); | ||
| let expert = Address::generate(&env); | ||
| let oracle = Address::generate(&env); | ||
|
|
||
| let token_admin = Address::generate(&env); | ||
| let token = create_token_contract(&env, &token_admin); | ||
| token.mint(&user, &10_000); | ||
|
|
||
| let client = create_client(&env); | ||
| client.init(&admin, &token.address, &oracle); | ||
|
|
||
| // Book session while unpaused | ||
| let booking_id = client.book_session(&user, &expert, &10, &100); | ||
|
|
||
| // Admin pauses the contract | ||
| client.pause(); | ||
|
|
||
| // Oracle tries to finalize while paused (should fail) | ||
| let result = client.try_finalize_session(&booking_id, &50); | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_pause_blocks_reclaim_stale_session() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let admin = Address::generate(&env); | ||
| let user = Address::generate(&env); | ||
| let expert = Address::generate(&env); | ||
| let oracle = Address::generate(&env); | ||
|
|
||
| let token_admin = Address::generate(&env); | ||
| let token = create_token_contract(&env, &token_admin); | ||
| token.mint(&user, &10_000); | ||
|
|
||
| let client = create_client(&env); | ||
| client.init(&admin, &token.address, &oracle); | ||
|
|
||
| // Book session while unpaused | ||
| let booking_id = client.book_session(&user, &expert, &10, &100); | ||
|
|
||
| // Advance time past reclaim timeout | ||
| env.ledger() | ||
| .set_timestamp(env.ledger().timestamp() + 90_000); | ||
|
|
||
| // Admin pauses the contract | ||
| client.pause(); | ||
|
|
||
| // User tries to reclaim while paused (should fail) | ||
| let result = client.try_reclaim_stale_session(&user, &booking_id); | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_pause_blocks_reject_session() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let admin = Address::generate(&env); | ||
| let user = Address::generate(&env); | ||
| let expert = Address::generate(&env); | ||
| let oracle = Address::generate(&env); | ||
|
|
||
| let token_admin = Address::generate(&env); | ||
| let token = create_token_contract(&env, &token_admin); | ||
| token.mint(&user, &10_000); | ||
|
|
||
| let client = create_client(&env); | ||
| client.init(&admin, &token.address, &oracle); | ||
|
|
||
| // Book session while unpaused | ||
| let booking_id = client.book_session(&user, &expert, &10, &100); | ||
|
|
||
| // Admin pauses the contract | ||
| client.pause(); | ||
|
|
||
| // Expert tries to reject while paused (should fail) | ||
| let result = client.try_reject_session(&expert, &booking_id); | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_unpause_resumes_operations() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let admin = Address::generate(&env); | ||
| let user = Address::generate(&env); | ||
| let expert = Address::generate(&env); | ||
| let oracle = Address::generate(&env); | ||
|
|
||
| let token_admin = Address::generate(&env); | ||
| let token = create_token_contract(&env, &token_admin); | ||
| token.mint(&user, &10_000); | ||
|
|
||
| let client = create_client(&env); | ||
| client.init(&admin, &token.address, &oracle); | ||
|
|
||
| // Admin pauses the contract | ||
| client.pause(); | ||
|
|
||
| // Booking should fail while paused | ||
| let result = client.try_book_session(&user, &expert, &10, &100); | ||
| assert!(result.is_err()); | ||
|
|
||
| // Admin unpauses the contract | ||
| let result = client.try_unpause(); | ||
| assert!(result.is_ok()); | ||
|
|
||
| // Booking should succeed after unpause | ||
| let booking_id = client.book_session(&user, &expert, &10, &100); | ||
| assert_eq!(booking_id, 1); | ||
| assert_eq!(token.balance(&user), 9_000); | ||
| assert_eq!(token.balance(&client.address), 1_000); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_read_only_functions_work_while_paused() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let admin = Address::generate(&env); | ||
| let user = Address::generate(&env); | ||
| let expert = Address::generate(&env); | ||
| let oracle = Address::generate(&env); | ||
|
|
||
| let token_admin = Address::generate(&env); | ||
| let token = create_token_contract(&env, &token_admin); | ||
| token.mint(&user, &10_000); | ||
|
|
||
| let client = create_client(&env); | ||
| client.init(&admin, &token.address, &oracle); | ||
|
|
||
| // Book a session before pausing | ||
| let booking_id = client.book_session(&user, &expert, &10, &100); | ||
|
|
||
| // Admin pauses the contract | ||
| client.pause(); | ||
|
|
||
| // Read-only functions should still work | ||
| let booking = client.get_booking(&booking_id); | ||
| assert!(booking.is_some()); | ||
| assert_eq!(booking.unwrap().id, booking_id); | ||
|
|
||
| let user_bookings = client.get_user_bookings(&user); | ||
| assert_eq!(user_bookings.len(), 1); | ||
|
|
||
| let expert_bookings = client.get_expert_bookings(&expert); | ||
| assert_eq!(expert_bookings.len(), 1); | ||
| } |
There was a problem hiding this comment.
Add a test verifying that unauthorized callers cannot pause/unpause.
All pausability tests use mock_all_auths(), which bypasses Soroban's authorization checks entirely. There is no test verifying that a non-admin (e.g., user or oracle) calling pause() or unpause() is rejected. For a security-critical circuit breaker this is an important coverage gap.
💡 Suggested test skeleton
#[test]
fn test_non_admin_cannot_pause() {
let env = Env::default();
env.mock_all_auths(); // set up contract
let admin = Address::generate(&env);
let non_admin = Address::generate(&env);
let token = Address::generate(&env);
let oracle = Address::generate(&env);
let client = create_client(&env);
client.init(&admin, &token, &oracle);
// Clear mocked auths; supply only non_admin's auth
env.set_auths(&[/* non_admin auth for pause */]);
let result = client.try_pause();
assert!(result.is_err());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/test.rs` around lines 648 - 833, The
tests currently call env.mock_all_auths() which disables Soroban auth checks, so
add new tests (e.g., test_non_admin_cannot_pause and
test_non_admin_cannot_unpause) that initialize via create_client and client.init
but then clear or replace mocked auths (use env.set_auths or equivalent) to
supply only a non-admin signer before calling client.try_pause() and
client.try_unpause(), and assert those calls return Err; reference try_pause,
try_unpause, create_client, client.init, env.mock_all_auths, and env.set_auths
to locate where to change the auth setup and assertions.
Collaborator
|
Please resolve conflicts @KevinMB0220 |
Keep both features: circuit breaker (pause/unpause) and expert rate management (set_my_rate). Assign ContractPaused=8, ExpertRateNotSet=9. Update pausability tests to use the new set_my_rate + book_session API. All 28 tests pass.
Contributor
Author
Done |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/payment-vault-contract/src/contract.rs (1)
42-53:⚠️ Potential issue | 🟠 MajorAdd pause guard to
set_my_rateto align with circuit breaker objectives.
set_my_ratemodifies persistent state but lacks the pause guard check used in other state-modifying functions.reclaim_stale_sessionandreject_sessionboth checkif storage::is_paused(env)before proceeding. Since the PR objectives require pause to block "all financial movements and state changes," add the same guard:if storage::is_paused(env) { return Err(VaultError::ContractPaused); }at the beginning of
set_my_ratefor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/contract.rs` around lines 42 - 53, The set_my_rate function lacks the contract pause guard; add an initial check in set_my_rate to call storage::is_paused(env) and return Err(VaultError::ContractPaused) when true, mirroring the guards used in reclaim_stale_session and reject_session so no state changes occur while paused; place this check right after expert.require_auth() and before validating or updating storage and emitting events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Around line 42-53: The set_my_rate function lacks the contract pause guard;
add an initial check in set_my_rate to call storage::is_paused(env) and return
Err(VaultError::ContractPaused) when true, mirroring the guards used in
reclaim_stale_session and reject_session so no state changes occur while paused;
place this check right after expert.require_auth() and before validating or
updating storage and emitting events.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/error.rscontracts/payment-vault-contract/src/events.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/payment-vault-contract/src/lib.rs
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.
Summary
Closes #27
Implements an emergency circuit breaker for the
payment-vault-contractthat allows the Admin to instantly freeze all financial movements and state changes.DataKey::IsPausedwithset_paused(env, bool)andis_paused(env) -> boolhelpersVaultError::ContractPaused = 8variantpause()andunpause()withadmin.require_auth()enforcementbook_session,finalize_session,reclaim_stale_session, andreject_sessionall returnContractPausedwhen the circuit breaker is activeget_booking,get_user_bookings,get_expert_bookings) remain fully operational while paused"paused"event with a bool value on each toggleTest plan
test_pause_blocks_book_session— booking fails withContractPausedwhile pausedtest_pause_blocks_finalize_session— finalization blocked while pausedtest_pause_blocks_reclaim_stale_session— reclaim blocked while pausedtest_pause_blocks_reject_session— rejection blocked while pausedtest_unpause_resumes_operations— pause → fail → unpause → succeed flowtest_read_only_functions_work_while_paused— getters remain activecargo test),cargo fmtandcargo clippycleanSummary by CodeRabbit
New Features
Tests