Fix/#395 - #599
Open
Namiiikaze wants to merge 3 commits into
Open
Conversation
…tion messages (JSE-ORG#481) Signed-off-by: namdamdoi68-oss <namdamdoi68@gmail.com>
…ions, and enforce required signer config
Contributor
|
Resolve the conflict |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #395 — the auto-release eligibility query matched an impossible state (SHIPPED + deliveredAt set), so the worker never processed any escrows. This is a fresh implementation on a new branch, incorporating all feedback from the review on the previous PR attempt.
Core fix
findAutoReleaseEligible now queries for state: 'DELIVERED' instead of state: 'SHIPPED'. markDelivered is the only writer of deliveredAt and always sets state: 'DELIVERED' in the same update, so the old query could never match anything.
Consolidation
Deleted src/escrow/auto-release.service.ts (the unregistered duplicate implementation), which also had a double-subtraction cutoff bug producing a 9-day window instead of 48 hours.
auto-release.worker.ts is now the single implementation, using the atomic markAutoReleaseSubmitting claim to prevent double-submission across replicas.
Fixed a terminal-state inconsistency found during this work: the worker was calling markAutoReleaseCompleted (→ COMPLETED), while the manual-release flow in escrow.service.ts uses markAutoReleased (→ RELEASED). Standardized on RELEASED everywhere.
Config
AUTO_RELEASE_SOURCE_ADDRESS is now required, validated as a real Stellar public key (checksum-verified via Keypair.fromPublicKey), and the app fails to start with a clear error if it's unset, replacing the old placeholder-literal fallback.
Addressing review feedback from the previous attempt
Missing env var — Added AUTO_RELEASE_SOURCE_ADDRESS to .env.test, .env.example, and the env: blocks of test.yml, integration-test.yml, and e2e-test.yml, using a real, checksum-valid generated key.
Deleted tests that covered real behavior — Kept the repository-level markAutoReleaseSubmitting lock tests intact. Ported the service-level tests (two workers racing, lock released on failure, multiple escrows without collision) to run against the surviving AutoReleaseWorker.
Tests in a file CI doesn't run — Confirmed e2e-test.yml already runs .e2e-spec.ts files, so no relocation was needed; verified this rather than assuming.
Lint — Ran eslint --fix clean (0 errors).
Misleading error message — Reworded the missing-signer error to say the app fails to start, rather than that auto-release is "disabled."
Test fixtures
All auto-release test fixtures across 7+ files were rewritten to create escrows via SHIPPED → markDelivered() (matching real production state transitions) instead of directly constructing the previously-impossible SHIPPED + deliveredAt state.
Bugs found and fixed during implementation (beyond the original scope)
A hand-rolled placeholder Stellar address initially failed real checksum validation; replaced with a properly generated, Keypair.fromPublicKey-verified key everywhere it's used.
A colocated spec file (src/workers/auto-release.worker.spec.ts) was missed in the initial pass since it lives next to its source file rather than under test/; found and fixed to match.
A test helper (createDeliveredEscrow) was silently overriding explicitly-passed state: 'SHIPPED' values, which defeated an exclusion test's purpose; fixed so explicit states are respected.
3 TypeScript errors and 1 Jest syntax error caught via tsc --noEmit and fixed.
Test results
57/57 tests passing across unit, integration, and e2e tiers for all auto-release-related suites.
closes #395