Add a regression test documenting FIX - #213
Merged
Jagadeeshftw merged 1 commit intoJul 28, 2026
Merged
Conversation
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.
Pin the load-bearing route order in routes/liquidity.ts
Closes the GET /api/v1/liquidity/entries route-shadowing issue.
Problem
routes/liquidity.ts registers GET /entries before GET /:asset. Express matches
routes in registration order and /:asset is a catch-all that matches ANY single
path segment, so /entries only reaches the entries handler because it happens to
be registered first.
Nothing enforced that. A routine-looking refactor -- reordering handlers,
alphabetising them, moving the catch-all "to the bottom of the GETs" and
accidentally landing it above /entries -- would silently reroute
GET /api/v1/liquidity/entries into getPool("ENTRIES"). No compile error, no
failing test. Consumers expecting { entries: [...] } would start receiving a 404
error body instead.
Worth noting the sibling route /withdrawals already had both an explanatory
comment and a precedence test guarding exactly this. /entries had neither. This
PR closes that asymmetry.
What changed
Three parts: the regression test (the issue's core ask), the code comment, and
the doc notes.
"the /entries static route takes precedence over /:asset"
With no liquidity recorded at all, a /:asset lookup for "ENTRIES" would 404.
Asserts 200 and an exact { entries: [] } body, and explicitly asserts the
absence of asset / total / error keys.
"keeps /entries resolving to the entries list even with liquidity recorded"
Same guarantee once real entries exist, so the test is not passing merely
because the store is empty.
"still resolves /entries to the entries list when an asset named ENTRIES
exists"
The adversarial case. An asset literally named "ENTRIES" is recorded first,
so a swapped registration order returns HTTP 200 with a valid pool body --
meaning any status-code-only assertion would pass straight through the bug.
This test pins the response shape instead: entries must be an array, and
total / anchors (the Pool shape) must be absent.
The third test is the one that matters. Tests 1 and 2 catch the common failure
(404); test 3 catches the silent one.
docs: -- comment directly above the two registrations
A block comment above router.get("/entries", ...) states that route order is
load-bearing, explains the Express matching rule that makes it so, names the
concrete failure mode (getPool("ENTRIES")), and points at the tests that fail if
the order is swapped. A second, shorter CATCH-ALL note above
router.get("/:asset", ...) records that it must remain the last GET in the
router.
docs: -- README and openapi.ts
The issue asked for a doc note only if either file documents route ordering
elsewhere. Neither did -- grep for route-ordering prose across README.md,
CHANGELOG.md, docs/ARCHITECTURE.md and PULL_REQUEST.md returned nothing -- so
strictly the comment plus tests would have sufficed. Both were updated anyway,
since both already list these endpoints adjacently and are exactly where someone
would look:
endpoint list.
recording the constraint.
No behaviour changed. Not a single line of runtime logic was touched -- the diff
in liquidity.ts is comments only.
Validation
Full pipeline green on current main (cc429b5):
LINT exit 0
BUILD exit 0 (tsc --noEmit and npm run build)
TEST 40 suites, 460 tests passed (baseline was 40 / 457)
Coverage 97.15% statements overall (bar: 95%); routes/liquidity.ts at 100%
statements / lines / functions, services/liquidityService.ts at 100%.
Mutation-tested -- the tests were proven to fail, not merely to pass
The acceptance criterion is that the new test "would fail if the two route
registrations were swapped". That was verified directly rather than assumed, by
mutating the source, running the suite, and reverting:
1 Full swap of the /entries and /:asset blocks
-> all 3 new tests fail
2 Surgical move of /entries alone to below /:asset
-> all 3 new tests fail
3 Re-run of mutation 2 after rebasing onto updated main (cc429b5)
-> all 3 new tests fail
4 Source restored from backup
-> 460 / 460 green
Mutation 2 is the realistic refactor and the one the tests are designed for.
Mutation 3 re-confirms the guard after the branch moved underneath the work. All
mutations were reverted; the diff below contains none of them.
Rebase note
This work was authored against 9a547ab; main has since advanced to cc429b5
(commits "Add since timestamp" and "Add a test verifying GET FIX (#210)").
Rebased and re-validated -- reapplied with zero conflicts.
Despite its title, cc429b5 does NOT address this issue: it touches anchors.ts,
settlements.ts and utils/csv.ts, and leaves routes/liquidity.ts and
routes/liquidity.test.ts untouched. There is no overlap or duplicated work. The
only shared file with upstream is openapi.ts, where the incoming edit sits at the
/metrics/history entry (~line 176) and this one at /liquidity/entries (~line 64).
Reviewer notes
static single-segment liquidity GET belongs above it, alongside /entries and
/withdrawals -- and is worth a precedence test of its own.
substance; the uncovered range now points at the new comment block, the same
optional-parameter-default artifact already visible in routes/quote.ts. No
real branch went uncovered.
(the sibling PR that documented and tested the /withdrawals ordering), which
added none; only 1 of the last 6 commits touched CHANGELOG.md. Happy to add
an [Unreleased] entry if the maintainer prefers the newer convention.
Files changed
src/routes/liquidity.test.ts Added 3 route-precedence regression tests
src/routes/liquidity.ts Added ordering comment above /entries and
CATCH-ALL note above /:asset (comments only)
src/openapi.ts /liquidity/entries description records the
ordering constraint
README.md "Route ordering (liquidity)" note
No files were created or deleted. 4 files changed, 91 insertions(+), 2
deletions(-).
Closes #149