Two findings from the review of #4440 that are worth acting on but did not block that merge. Neither is introduced by #4440 — both predate it — and neither leaves an order waiting on a person, which is why they were recorded rather than fixed there.
1. ScryptCancellation.SETTLED conflates two qualities of evidence
SETTLED is returned for two different answers (scrypt.service.ts, cancelIfOutstandingCore):
- a terminal cancel with nothing filled — the venue states outright that nothing can execute;
UnknownOrder on the cancel — the venue does not recognise the reference, which is an inference from its own words, not a statement about execution.
Callers cannot tell them apart, and two of them give an order up on either: the quarantine cleanup in attemptQuarantineCleanup, and the stuck-pending exit added in #4440.
For withdrawals this is settled ground — every Scrypt withdrawal address is DFX-owned, so a repeated payout is an internal rebooking. For trades it is not: giving one up without a genuinely confirmed cancel lets the onFail chain place a second, competing buy on another venue, and that is real market exposure.
The inference is well argued where it is documented (SCRYPT_UNKNOWN_ORDER), and the reasoning holds in both current call sites — but it holds by argument, not by construction, and nothing stops the next call site from relying on SETTLED where the weaker evidence is not good enough.
Worth modelling the two as distinct values, so a caller has to state which evidence it accepts. The trade paths would then take only the confirmed cancel; the withdrawal path could keep accepting both.
2. In-process state is per instance, not per deployment
Three maps hold reconciliation state in process memory:
pendingSince and pendingCancelAttempts (scrypt.service.ts)uncertainResolveAttempts (liquidity-management-pipeline.service.ts)
They are correct for a single instance and degrade safely on restart — a fresh clock only ever lengthens a wait, never shortens it. But with more than one instance running the pipeline, each keeps its own copy: the cancel-retry floor would be applied per instance rather than per reference, and a pending clock would start again on whichever instance next observes the reference.
Consequences are bounded (more venue writes than intended, a longer wait before a bound is reached — no incorrect abandonment), which is why this is not urgent. It is worth deciding deliberately, though: either state that the pipeline is single-instance by design, or move the throttles to shared state.
Both points were raised across the review passes on #4440 and verified against the code there.
Two findings from the review of #4440 that are worth acting on but did not block that merge. Neither is introduced by #4440 — both predate it — and neither leaves an order waiting on a person, which is why they were recorded rather than fixed there.
1.
ScryptCancellation.SETTLEDconflates two qualities of evidenceSETTLEDis returned for two different answers (scrypt.service.ts,cancelIfOutstandingCore):UnknownOrderon the cancel — the venue does not recognise the reference, which is an inference from its own words, not a statement about execution.Callers cannot tell them apart, and two of them give an order up on either: the quarantine cleanup in
attemptQuarantineCleanup, and the stuck-pending exit added in #4440.For withdrawals this is settled ground — every Scrypt withdrawal address is DFX-owned, so a repeated payout is an internal rebooking. For trades it is not: giving one up without a genuinely confirmed cancel lets the onFail chain place a second, competing buy on another venue, and that is real market exposure.
The inference is well argued where it is documented (
SCRYPT_UNKNOWN_ORDER), and the reasoning holds in both current call sites — but it holds by argument, not by construction, and nothing stops the next call site from relying onSETTLEDwhere the weaker evidence is not good enough.Worth modelling the two as distinct values, so a caller has to state which evidence it accepts. The trade paths would then take only the confirmed cancel; the withdrawal path could keep accepting both.
2. In-process state is per instance, not per deployment
Three maps hold reconciliation state in process memory:
pendingSinceandpendingCancelAttempts(scrypt.service.ts)uncertainResolveAttempts(liquidity-management-pipeline.service.ts)They are correct for a single instance and degrade safely on restart — a fresh clock only ever lengthens a wait, never shortens it. But with more than one instance running the pipeline, each keeps its own copy: the cancel-retry floor would be applied per instance rather than per reference, and a pending clock would start again on whichever instance next observes the reference.
Consequences are bounded (more venue writes than intended, a longer wait before a bound is reached — no incorrect abandonment), which is why this is not urgent. It is worth deciding deliberately, though: either state that the pipeline is single-instance by design, or move the throttles to shared state.
Both points were raised across the review passes on #4440 and verified against the code there.