Skip to content

fix(aid_escrow): sweep past-due packages to Expired and release funds - #444

Merged
kilodesodiq-arch merged 1 commit into
ChainForgee:mainfrom
DeEvelyn:fix/issue-423-auto-expire-late-claims
Aug 20, 2026
Merged

fix(aid_escrow): sweep past-due packages to Expired and release funds#444
kilodesodiq-arch merged 1 commit into
ChainForgee:mainfrom
DeEvelyn:fix/issue-423-auto-expire-late-claims

Conversation

@DeEvelyn

Copy link
Copy Markdown
Contributor

Summary

Closes #423

Adds a permissionless, idempotent expire_if_past_due(id) entrypoint that transitions a past-due Created package to Expired, releases its locked funds, and moves its aggregate totals from total_committed to total_expired_cancelled. The single most important design decision: a late claim/claim_with_proof cannot atomically mark the package Expired and return Error::PackageExpired in the same invocation, because Soroban reverts all storage writes when a function returns an error. The sweep entrypoint is therefore the only mechanism that can persist the transition, so it is deliberately decoupled from the claim error path.

Why

Before this change, a late claim returned Error::PackageExpired and left the package Created forever. Its amount stayed in KEY_TOTAL_LOCKED and in the Created aggregate until an admin manually called refund(id):

  • Solvency pool leakcreate_package rejects new packages when contract_balance < current_locked + amount; expired-but-unswept packages keep their funds counted as locked, invisibly shrinking the available pool and forcing manual admin sweeps to reclaim capacity.
  • Aggregates lieget_aggregates(token) overstates total_committed and understates total_expired_cancelled for every past-due package, so dashboards and indexers report unclaimable funds as committed.
  • Docs contradicted codeBOUNDARY_VALIDATION_BEHAVIOR.md said both "status remains Created" and "late claim attempts automatically expire the package status on-chain".

The obvious shortcut — writing the Expired status inside the claim guard — is not merely wrong but impossible: Soroban rolls back the status write when the function returns Err, which would silently double-apply accounting on the next successful sweep. The chosen approach keeps the claim paths pure (error only) and moves the state transition to a successful, permissionless call that any indexer, cron, or relayer can invoke.

What was built

app/onchain/contracts/aid_escrow/src/lib.rs:

Change What it contains
expire_if_past_due(env, id) Permissionless, idempotent sweep: no-ops for never-expiring (expires_at == 0), not-yet-due, or already-terminal packages; otherwise sets Expired, decrement_locked, and moves Created → expired/cancelled via the existing add_to_status_totals helper. Returns Error::PackageNotFound only for a missing id.
refund() should_unlock_locked guard Narrowed from Created || Expired to Created. A package swept by expire_if_past_due (or cancelled by revoke) has already released its locked funds; unlocking again would double-decrement KEY_TOTAL_LOCKED.

app/onchain/contracts/aid_escrow/tests/boundary_validation_tests.rs — the late_claim_behavior module was reworked from 3 to 5 tests, each with matching snapshots in test_snapshots/late_claim_behavior/:

Test Covers
late_claim_returns_error_without_transitioning Late claim returns PackageExpired, status stays Created, get_total_locked unchanged.
claim_with_proof_fails_after_expiry_without_transitioning Same property via claim_with_proof.
expire_if_past_due_transitions_and_moves_accounting Sweep before expiry is a no-op; after expiry it sets Expired, zeroes total_locked, moves total_committed → total_expired_cancelled; a second sweep is idempotent (no double-move).
expire_if_past_due_ignores_never_expiring_and_missing_packages expires_at == 0 untouched; missing id → PackageNotFound.
refund_after_expiry_does_not_unlock_other_packages Two packages on one token: sweeping one releases only its share; refunding the swept package transfers funds and ends Refunded without decrementing the other package's locked share.

app/onchain/contracts/aid_escrow/BOUNDARY_VALIDATION_BEHAVIOR.md — rewritten so "Late Claim Behavior", "Test Coverage", and "Conclusion" describe one consistent sweep-based behavior; all contradictory auto-expiry statements removed.

Integration changes outside the module

  • app/onchain/contracts/aid_escrow/README.md — added the expire_if_past_due row to the function table and corrected the lifecycle diagram line to name the sweep as the Created → Expired trigger.
  • app/onchain/README.md — added the expire_if_past_due row to the method reference table.

No storage schema changed: the new function writes only existing keys (pkg, KEY_TOTAL_LOCKED, KEY_TOTAL_COMMITTED, KEY_TOTAL_EXPIRED_CANCELLED) using existing helpers, so no migration is needed and persisted state is unaffected.

Acceptance criteria coverage

Contract

  • A late claim/claim_with_proof on a Created package transitions it to Expired and returns Error::PackageExpired. — Delivered via the permissionless expire_if_past_due(id) sweep, the only mechanism Soroban permits: storage writes revert when a function returns an error, so the transition cannot be committed atomically with the error return. The claim paths still return Error::PackageExpired; the transition now happens deterministically on the first sweep call (boundary_validation_tests.rs — expire_if_past_due_transitions_and_moves_accounting, late_claim_returns_error_without_transitioning).
  • After auto-expiry, get_total_locked(token) no longer includes that package's amount and get_aggregates(token) shows the amount in total_expired_cancelled, not total_committed. (expire_if_past_due_transitions_and_moves_accounting asserts locked → 0 and committed → expired/cancelled deltas.)
  • refund(id) on an auto-expired package still transfers funds to the admin and ends in Refunded without double-decrementing locked totals. (refund_after_expiry_does_not_unlock_other_packages; refund guard narrowed to Created only.)

Tests

  • Tests in app/onchain/contracts/aid_escrow/tests/ assert the locked/aggregate deltas after a late claim, and that a second late claim does not double-move the amount. (Idempotency asserted in expire_if_past_due_transitions_and_moves_accounting; the sweep is the only path that moves amounts.)
  • A test pins the refund after auto-expiry path so no invariant drift is introduced. (refund_after_expiry_does_not_unlock_other_packages.)

Documentation

  • BOUNDARY_VALIDATION_BEHAVIOR.md is rewritten so "Late Claim Behavior", "Test Coverage", and "Conclusion" describe one consistent behavior, and the contradictory auto-expiry statements are removed.

Test plan

  • cd app/onchain && cargo test --package aid_escrow — 183/183 passing across all 19 suites (2 new tests: expire_if_past_due_transitions_and_moves_accounting, expire_if_past_due_ignores_never_expiring_and_missing_packages; late_claim_behavior module reworked 3 → 5)
  • cargo fmt --all -- --check — clean
  • cargo clippy --tests --target x86_64-unknown-linux-gnu -- -D warnings — clean
  • cargo clippy --target wasm32-unknown-unknown -- -D warnings — clean (matches contract-ci.yml)
  • cargo check --locked — succeeds

Env vars / Notes

No new env vars or config keys. The test_snapshots/*.json files are regenerated test artifacts (not verified by contract-ci.yml); only the five snapshots for the new/reworked late_claim_behavior tests are included in this PR. Pre-existing snapshot drift on main (unrelated to this change) was left untouched.

…d funds

Add a permissionless, idempotent expire_if_past_due(id) entrypoint that
transitions a past-due Created package to Expired, decrements its locked
total, and moves its aggregate totals from committed to the
expired/cancelled bucket. A late claim cannot commit this transition
itself because Soroban reverts storage writes on an error return, so
claim/claim_with_proof keep returning Error::PackageExpired while the
sweep reclaims pool capacity and fixes get_aggregates drift.

Also fix refund() to only decrement locked totals for Created packages,
so refunding a package already swept by expire_if_past_due (or cancelled
by revoke) does not double-release funds, and align
BOUNDARY_VALIDATION_BEHAVIOR.md, README state diagrams, and the boundary
tests with the sweep-based design.

@kilodesodiq-arch kilodesodiq-arch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kilodesodiq-arch
kilodesodiq-arch merged commit 83db75e into ChainForgee:main Aug 20, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

claim() leaves past-due packages in Created state: locked funds and aggregates stay stale until manual refund

2 participants