From 846c52cbb1f8dfec7c84763fdd5f1e7921471489 Mon Sep 17 00:00:00 2001 From: Umar faruk Date: Mon, 31 Aug 2026 12:17:34 +0000 Subject: [PATCH] fix: remove dead writes before panic in accept_intent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Soroban discards all storage mutations from a panicking invocation, so the .set() and bump_intent_ttl() calls immediately before panic_with_error!(Error::IntentExpired) in accept_intent's expiry branch were dead writes that never committed. - Remove the dead .set()/bump_intent_ttl() calls - Replace with a comment explaining why no write is possible here - Add regression test accept_expired_intent_state_unchanged confirming the intent state remains Open (not Expired) after a failed accept, and that solver remains None - Update docs/111-expire-intent-event-coverage.md: status line, §2 past-tense reference, and new §6 documenting the cleanup Closes #111 (dead write cleanup) --- docs/111-expire-intent-event-coverage.md | 25 ++++++++++++-- intent_settlement/src/lib.rs | 10 +++--- intent_settlement/src/test.rs | 43 ++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/docs/111-expire-intent-event-coverage.md b/docs/111-expire-intent-event-coverage.md index 5cd5f90..8afae12 100644 --- a/docs/111-expire-intent-event-coverage.md +++ b/docs/111-expire-intent-event-coverage.md @@ -2,7 +2,7 @@ **Issue:** [#111](https://github.com/stellar-vortex-protocol/vortex-contracts/issues/111) **Branch:** `feat/ops-monitoring-and-health-check` -**Status:** Confirmed — gap documented, no contract change required +**Status:** Confirmed — gap documented, no contract change required; dead writes cleaned up --- @@ -38,8 +38,10 @@ time passing: Because Soroban discards all state writes from a panicking invocation, the `state` field is *not* actually updated to `Expired` here, and no event is emitted — the transaction simply fails. (The preceding - `.set()`/`bump_intent_ttl` calls just before the `panic_with_error!` are - dead writes for the same reason; they never commit.) + `.set()`/`bump_intent_ttl` calls just before the `panic_with_error!` were + dead writes for the same reason; they have since been removed and replaced + with an explanatory comment — see the cleanup in `lib.rs`'s + `accept_intent` expiry branch.) - `expire_intent` is the **only** function that durably writes `state: Expired` and emits `intent_expired` — and only when someone calls it (it's permissionless, but still requires a submitted @@ -141,3 +143,20 @@ consumers are aware of the distinction in §4. Recommendations: --- *Closes #111* + +--- + +## 6. Code Cleanup (dead write removal) + +The `.set()`/`bump_intent_ttl` calls immediately before `panic_with_error!` +in `accept_intent`'s expiry branch — identified as dead writes in §2 above — +have been removed. They were replaced with an inline comment explaining that +Soroban discards all storage mutations from a panicking invocation, so no write +is possible at that call site. + +A regression test (`accept_expired_intent_state_unchanged` in `test.rs`) +was added to make the expected observable behavior explicit: after a failed +`accept_intent` call on a past-deadline intent, `get_intent` must still return +`state: Open` (not `Expired`) and `solver: None`, confirming no partial write +committed. The test also guards against the dead-write pattern being +reintroduced accidentally in the future. diff --git a/intent_settlement/src/lib.rs b/intent_settlement/src/lib.rs index 6915565..234836b 100644 --- a/intent_settlement/src/lib.rs +++ b/intent_settlement/src/lib.rs @@ -1364,10 +1364,12 @@ impl IntentSettlement { // `now >= intent.deadline` rejects at the boundary second (`now == deadline`) // so the full [created_at, deadline) half-open window is available for solvers. if now >= intent.deadline { - env.storage() - .persistent() - .set(&DataKey::Intent(intent_id.clone()), &intent); - Self::bump_intent_ttl(&env, &intent_id); + // Note: no storage write is performed here. Soroban discards all + // state mutations made during a panicking invocation, so any + // `.set()` or `bump_intent_ttl` call immediately before + // `panic_with_error!` would be a dead write that never commits. + // The durable `Open → Expired` transition is handled exclusively + // by `expire_intent`. panic_with_error!(&env, Error::IntentExpired); } diff --git a/intent_settlement/src/test.rs b/intent_settlement/src/test.rs index 9ed8561..ffdb9fe 100644 --- a/intent_settlement/src/test.rs +++ b/intent_settlement/src/test.rs @@ -1213,6 +1213,49 @@ fn accept_expired_intent_fails() { assert_eq!(res, Err(Ok(Error::IntentExpired.into()))); } +/// Regression test: accept_intent's expiry branch panics with IntentExpired +/// and must NOT persist any state change. Soroban discards all storage +/// writes made during a panicking invocation, so a get_intent call after the +/// failed accept must still report the original Open state — not Expired. +/// +/// This guards against the dead-write pattern removed in the cleanup tracked +/// by docs/111-expire-intent-event-coverage.md: even if a `.set()` call were +/// re-introduced immediately before the panic, it would still not commit, but +/// the test makes the expected observable behavior explicit so any regression +/// is caught immediately. +#[test] +fn accept_expired_intent_state_unchanged() { + let ctx = setup(); + ctx.register_solver(); + let id = ctx.submit(); + + // Advance past the intent deadline so accept_intent will hit the expiry branch. + ctx.pass_time(INTENT_EXPIRY + 1); + + // The call must fail with IntentExpired. + let res = ctx.client().try_accept_intent(&ctx.solver, &id); + assert_eq!(res, Err(Ok(Error::IntentExpired.into()))); + + // Because Soroban discards all writes from a panicking invocation, the + // stored intent state must remain Open — exactly as it was before the + // failed accept attempt. + let intent = ctx + .client() + .get_intent(&id) + .expect("intent must still exist after failed accept"); + assert_eq!( + intent.state, + IntentState::Open, + "failed accept_intent must not mutate intent state: expected Open, got {:?}", + intent.state + ); + // The solver field must also remain unset — no partial write committed. + assert!( + intent.solver.is_none(), + "failed accept_intent must not set intent.solver" + ); +} + #[test] fn cannot_accept_already_accepted_intent() { let ctx = setup();