Improve SEP-24 transaction status synchronization and error handling - #483
Conversation
- Map SEP-24 statuses to the internal pending/completed/failed states via an exhaustive, explicit boundary (KNOWN_SEP24_STATUSES + isKnownSep24Status). - Validate and reject malformed anchor responses before persisting: strict transaction schema, non-empty status requirement. - Use conditional (snapshot-guarded) DB updates when advancing sessions so a stale or out-of-order writer can never regress a terminal state. - Treat transient/indeterminate network failures as retryable (never falsely mark the transaction failed); only terminal anchor failures map to error. - Handle unknown anchor statuses explicitly with an operationally useful log entry, keeping them pending rather than misclassifying them. - Sanitize anchor failure details (failureReason) and surface them only through the intended API fields. - Keep user/transaction ownership checks enforced on status retrieval. - Add tests covering every SEP-24 status, malformed/unknown responses, transient HTTP failures, stale updates, and terminal-state protection.
|
MergeKeeper review Scope: in scope for linked issue The pull request correctly implements SEP-24 transaction status synchronization, validation, and terminal state protections according to the issue requirements. Reviewed commit: |
|
@Orah-dev 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! 🚀 |
|
MergeKeeper merge status Status: blocked Reason: One or more required CI checks failed. Failing checks:
Next steps:
|
…ences The merge into feat/sep24-status-sync-improvements left src/services/anchor.ts referencing symbols that no longer exist after the retry refactor: - 'isTransient' (undefined): derive it from the canonical retry classification (isRetryableFailure(classifyUpstreamFailure(cause))) instead of a dangling var. - 'safeFailureMessage' (undefined): import it from ./job-retry. - 'log' (removed in favour of 'retryLog'): route the two unknown-status warnings through retryLog, which already exposes the same warn(entry, message) shape. This restores a clean 'tsc -p tsconfig.json' build.
- Health routes: split into lightweight /health liveness probe (no deps) and /health/ready readiness probe (checks database + Horizon) - Treasury proposals: return INVALID_ASSET_ISSUER for invalid asset issuer instead of generic VALIDATION_ERROR - Authorization tests: use a valid Stellar public key (Keypair.random) instead of the non-valid GAAAA... test key - Withdraw tests: fix memo format to use MP: prefix required by schema - Worker tests: mock process.exit in shutdown tests to prevent vitest from catching the real process.exit calls - Add missing $disconnect mock to worker-shutdown-drain prisma mock All 1534 tests pass (97 test files), tsc --noEmit clean. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Needs review Linked to The PR includes numerous unrelated changes such as health route restructuring, treasury proposal validation updates, and test additions that exceed the scope of SEP-24 transaction status synchronization and error handling. Reviewed commit: |
- Revert health route to /health readiness check (as on main), restore routes.test.ts to match the original health response shape - Revert treasury-proposals to use Zod validation error code (VALIDATION_ERROR) as expected by merged test - Update mapAnchorStatus to pass through known SEP-24 statuses as-is (expired, no_market, too_small, too_large, pending_*) instead of collapsing them — matches main's anchor.test.ts - Add specific error detail in pollTransaction for missing transaction/status fields (matches main's error message tests) - Update sep24-status-sync.test.ts expectations to match the pass-through mapping from main All 1708 tests pass (102 files), tsc --noEmit clean. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
MergeKeeper review unavailable AI provider review response did not contain valid JSON No approval or merge action was taken. |
2 participants
Summary
Closes #116
Make SEP-24 anchor status synchronization reliable and easier for clients to consume. This PR hardens the anchor status-mapping boundary and the worker's reconciliation loop so that:
pending,completed, andfailedstates.error.Changes
src/services/anchor.ts— status-mapping boundary & response validationKNOWN_SEP24_STATUSESset plusisKnownSep24Status()so recognized values and true unknowns are distinguished explicitly.sep24TransactionResponseSchemawith.strict(), astatus.min(1)requirement, and asuperRefinethat rejects whitespace-only candidate statuses as malformed (permanent error) instead of silently round-tripping topending.PollResultnow carries arecognizedflag;mapAnchorStatuslogs an explicit warning for unknown statuses while still falling back to the safepending_anchorintermediate (a future anchor-only status is never mistaken for a failure or a terminal state).src/services/anchor-status.ts— monotonic, race-safe transitionsapplyAnchorSessionTransitionaccepts an optionalexpectedCurrentStatusstale-write guard. When supplied, the status write is performed with a conditionalupdateManykeyed on the snapshot's status, so a stale or out-of-order writer is rejected at the database layer, not just in-process.isTerminalAnchorStatusnow also treatserroras terminal (it can only still advance torefunded), keeping the local semantics consistent with the SEP-24 terminal set.src/worker/index.ts—reconcileSingleAnchorerrorand stop retrying, and the underlying transition uses the conditional stale-write guard.outcome: "unknown_status"log and remain pending.expectedCurrentStatus, and asupersededlog is emitted when a concurrent writer wins the race.Tests
tests/sep24-status-sync.test.tsto cover: every supported SEP-24 status,isKnownSep24Status/unknown-status recognition, whitespace-only status rejection, malformed (non-JSON / missing-field) responses, transient vs permanent HTTP & network failures, stale-update/terminal-state protection, and the new conditional stale-write guard behavior.anchorService.getToml(keeping the realpollTransactionagainstglobal.fetch).updateManywrites.Validation
npm test— 755 passing, 0 failures (55 test files).npm run build— passes (TypeScript,strict).npm run lint— 0 errors (16 pre-existing warnings unrelated to this change).