Skip to content

SettlementStatus transitions are unguarded — nothing prevents a cancelled settlement being executed #227

Description

@Jagadeeshftw

Priority: High  ·  Area: Settlement lifecycle / state machine  ·  Est. effort: 8–12 h

📌 Problem

src/models/settlement.ts:6 declares the lifecycle:

/** Lifecycle state of a settlement, mirroring the on-chain contract. */
export type SettlementStatus = "pending" | "executed" | "cancelled";

The type constrains which values are legal. It does not constrain which transitions are legal — that is a separate concern, and the model provides only isSettlementStatus, a value guard.

The transitions that must be impossible are obvious from the domain:

  • executed → pending (un-executing a completed settlement)
  • cancelled → executed (executing something already cancelled)
  • executed → executed (double execution)
  • cancelled → cancelled, executed → cancelled

Establishing whether these are currently prevented is the first deliverable. Read src/services/settlementService.ts and src/routes/settlements.ts and report which transitions are guarded, where, and which are not. The comment says the status mirrors the on-chain contract — so a divergence between what this service permits and what the contract permits is itself a finding worth reporting.

Compounding it: repositories are in-memory with no transactions (tracked separately), so even a guard implemented as read-then-write is subject to a check-then-act race between concurrent requests.

🎯 Design decision required

State and defend:

  1. Where the guard belongs — the model (a transition function that is the only way to change status), the service, or the repository? Putting it in the model makes an invalid transition unrepresentable; putting it in the service is easier but leaves other callers unguarded. Argue one.
  2. Atomicity. A guard that reads current status, validates, then writes is a race. State how concurrent execute-and-cancel requests are serialised. Given there is no database yet, say what you can guarantee now and what depends on the persistence work.
  3. Error semantics. What does an invalid transition return — 409 Conflict, 400, or 422? Pick one, apply it consistently, and reflect it in src/openapi.ts.

🧩 Requirements and context

  • Report the current behaviour before changing it. If executing a cancelled settlement currently succeeds, that is a live bug and should be the headline of your PR.
  • Every illegal transition needs a test asserting it is rejected with the chosen status code.
  • A concurrency test must cover simultaneous execute and cancel on the same settlement — exactly one must win, deterministically.
  • Do not change the set of statuses.
  • All 42 test files must pass; extend src/repositories/settlementRepository.test.ts and the service's tests.
  • Coordinate with the persistence issue — do not implement a database here, but say what your guard requires from it.

🛠️ Suggested execution

  1. Write tests attempting every illegal transition; record what currently happens.
  2. Report any that succeed as live bugs.
  3. Implement the guard per your decision.
  4. Add the concurrency test.
  5. Update src/openapi.ts with the error response.

✅ Acceptance criteria

  • The PR reports which transitions were guarded and which were not, before the change.
  • Any live bug found is stated prominently.
  • Every illegal transition is rejected, each with its own test.
  • A concurrency test shows exactly one of a simultaneous execute/cancel pair succeeding.
  • The error semantics are consistent and documented in src/openapi.ts.
  • The status set is unchanged.
  • npm run lint, npm run build and npm test pass.

🚫 Out of scope

  • The persistence layer — separate issue, though you must state your dependency on it.
  • Adding new settlement statuses.
  • On-chain contract changes.

🧪 Verification

npm ci
npm test src/repositories src/services
npm run lint && npm run build && npm test

📤 What your PR must include

  1. The current-behaviour report per transition.
  2. Any live bug found.
  3. Your guard-placement and atomicity decisions.
  4. The concurrency test.
  5. Closes #<n>.

🔒 Security notes

Settlement status is the record of whether value moved. If cancelled → executed is reachable, a settlement a counterparty believes was called off can still complete; if executed → executed is reachable, it completes twice. Because the status is documented as mirroring the on-chain contract, any transition this service permits that the contract does not creates a divergence between off-chain records and on-chain reality — which is the harder class of bug to detect after the fact.

📋 Guidelines

  • Minimum 95% test coverage on changed lines
  • Clear documentation
  • Timeframe: 96 hours from assignment
  • One logical change per commit; no merge commits

💬 Join our community

Working on this, or want to sanity-check your approach before you start? Come and ask — the maintainers are there and happy to help.

Telegram: https://t.me/Grainlify

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

GrantFox OSSGrantFox open-source programMaybe RewardedGrantFox: potentially rewarded contributionThird CampaignGrantFox third campaign issuebugSomething isn't workingpriority:highHigh difficulty / architectural or cross-cutting

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions