Skip to content

docs(test/escrow): document Error catalog and add security tests (Iss…#370

Open
spartan124 wants to merge 1 commit into
Talenttrust:mainfrom
spartan124:docs/error-catalog
Open

docs(test/escrow): document Error catalog and add security tests (Iss…#370
spartan124 wants to merge 1 commit into
Talenttrust:mainfrom
spartan124:docs/error-catalog

Conversation

@spartan124
Copy link
Copy Markdown

@spartan124 spartan124 commented May 28, 2026

Closes #344


PR Title

docs(test/escrow): add ordered Escrow error catalog + security tests (Issue #344)


Summary

This PR delivers the documentation + test scaffolding requested in
@Talenttrust/Talenttrust-Contracts/issues/344, scoped strictly to the escrow Soroban
contract in contracts/escrow.

It includes:

  • An ordered error-code catalog documenting every types::Error variant (codes 1..=23)
    with entrypoints + trigger conditions, and clearly marking reserved/unused codes.
  • Updated security notes reflecting implemented controls, known gaps, and integration risks.
  • A contracts/escrow/src/test/security.rs test suite written against the existing test harness
    (contracts/escrow/src/test/mod.rs) to validate security assumptions and error paths when the crate
    is buildable.

Changes

Documentation

  • docs/escrow/ERROR_CATALOG.md

    • Documents all escrow error codes defined in contracts/escrow/src/types.rs (Error, #[repr(u32)])
    • Codes documented in strict numeric order: 1 → 23
    • For each code: numeric value, entrypoint(s), precise trigger condition, and status (live vs reserved)
  • docs/escrow/SECURITY.md

    • Security assumptions and controls (auth, fail-closed, atomicity, TTL storage, accounting invariants)
    • Entrypoint → error mapping for integrators/auditors
    • Notes on any features not currently testable / not implemented

Tests

  • contracts/escrow/src/test/security.rs
    • Uses the existing escrow test harness (register_client, create_contract, complete_contract, assert_contract_error)
    • Asserts on crate::types::Error variants (not numeric literals)
    • Covers edge cases requested by the issue where supported by current entrypoints:
      • unauthorized callers (deposit)
      • double release
      • double refund
      • invalid milestone index (bounds)
      • refund request empty / duplicate index
      • fail-closed / atomicity checks (state unchanged after rejected operations)
    • Includes ignored placeholders for:
      • approval TTL expiry validation (depends on approval wiring + harness signature support)
      • fee rounding (fees are not implemented as public entrypoints yet)
      • paused state (if pause controls are not present / not exposed)

Notes: Formatting / Build Blockers

While preparing this work, cargo fmt --check failed due to a module path collision:

  • contracts/escrow/src/test.rs
  • contracts/escrow/src/test/mod.rs

Rust (and rustfmt) cannot resolve mod test; when both exist. This PR removes/renames the redundant
module file so formatting and compilation can proceed consistently.


Test Output

Local Commands

cargo fmt
cargo fmt --check
cargo test

Current Status

At time of writing, cargo test is blocked by pre-existing compilation failures in:

  • contracts/escrow/src/lib.rs
  • contracts/escrow/src/proptest.rs

These failures are unrelated to the documentation and test additions in this PR. This PR does not
attempt to refactor beyond the escrow scope, but it does include the tests so they can run once
the compile blockers are resolved.

If/when the escrow crate compiles, the security tests are expected to run under:

cargo test -p escrow

Security Notes (Reviewer Focus)

  • Fail-closed / atomicity: refund validation checks should reject before any mutation; tests verify
    refund state and contract totals do not change on validation failure.
  • Authorization-first: deposit must reject unauthorized callers with UnauthorizedRole and should not
    mutate balance/state.
  • State machine: deposit only in Created, release only in Funded; tests validate expected failures.
  • TTL approvals: documented and a test placeholder is provided; enable once approval wiring/harness
    supports setting ReleaseAuthorization in tests.
  • Fee accounting: not implemented as public entrypoints; documented as not live, with a placeholder
    test for future implementation.

Checklist (Issue #344)

  • Document each error code in order (1..=23) with trigger conditions and entrypoints
  • Mark unused/reserved codes explicitly
  • Add/extend contracts/escrow/src/test/security.rs using existing harness
  • Validate security assumptions where testable today (auth, fail-closed, atomicity, state gating)
  • Keep changes in-scope (contracts/escrow + docs/escrow)

Talenttrust#344)

- Add docs/escrow/ERROR_CATALOG.md documenting all codes in `types::Error` (1..=23),
  with entrypoints, trigger conditions, and live vs reserved status.
- Update docs/escrow/SECURITY.md with security assumptions, entrypoint→error mapping,
  and integration guidance.
- Implement contracts/escrow/src/test/security.rs using existing test harness helpers
  (register_client/create_contract/complete_contract/assert_contract_error).
- Add fail-closed and atomicity checks (no state mutation on rejected refund/release).
- Add edge case coverage: unauthorized deposit, double release/refund, invalid index,
  empty/duplicate refund list; placeholders/ignored tests for TTL approvals and fees.

Note: rustfmt/cargo tooling requires resolving duplicate module path for `mod test;`
(src/test.rs vs src/test/mod.rs). This change keeps the directory module and removes/
renames the duplicate file to unblock formatting.

Co-authored-by: Copilot <copilot@github.com>
@drips-wave
Copy link
Copy Markdown

drips-wave Bot commented May 28, 2026

@spartan124 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

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.

Document the EscrowError catalog with trigger conditions

1 participant