Repository navigation
fix: align aggregate-refresh cache key with /price route - #431
AllysiadevOps wants to merge 5 commits into
Conversation
|
@AllysiadevOps 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! 🚀 |
tsc --noEmit clean vitest 97 files, 1127 passing (up from 93 / 1068) dropped-tests none #325 GET /movers for market gainers and losers #327 GET /correlation for basket construction #485 net arbitrage spread against fees #494 declare the resources capability and expose pages over MCP Two fixes on merge. #325 shipped the route file and the OpenAPI entry but never registered it. The repo's own guard caught it: AssertionError: Documented but unregistered: ["GET /movers"] So the endpoint was documented, generated into clients, and did not exist. registerMoversRoutes already followed the house signature, so wiring it was an import and a call next to registerScreenerRoutes. That guard is worth its weight -- a documented route that 404s is worse than an undocumented one, because a client is generated against it. #327 added a console.error to src/routes/correlation.ts, which the logging hygiene test forbids. Now createLogger, the same change #388 and #318 needed in the previous batch. Three PRs in two batches have tripped that test, which suggests the rule is not discoverable enough from the outside -- worth a line in CONTRIBUTING rather than catching it per-PR. NOT merged, each failing on its own branch rather than from merge order: #460 Phoenix AMM adapter -- 5 failures alone, including its own networkVenueConfig and venues/registry tests #353 1 tsc error #354 1 tsc error #482 4 tsc errors #393 #431 #434 #472 conflict Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Miracle656
left a comment
There was a problem hiding this comment.
Screened against current main: this has 2 type errors on its own branch, so it needs work beyond a rebase.
Run npx tsc --noEmit after rebasing and you will see them. A fair number of PRs here today failed the same way, and in most cases it was a helper that gained a parameter, or a symbol that lives on another branch and never arrived with this one.
Two guards worth knowing about before you re-request, because both caught PRs today:
console.*is forbidden in non-test source — usecreateLoggertests/openapi.test.tsfails if a route is documented but not registered insrc/app.ts, or registered but not documented
Get tsc and vitest both clean locally and I will review it properly.
Miracle656
left a comment
There was a problem hiding this comment.
Scanned clean. A refresh path writing under a different cache key than the read path is a good bug to go after — it does not fail loudly, it just means the refresh never serves anyone and the route keeps recomputing.
The branch is 79 commits behind main, and merging it as-is would revert shipped work.
Against current main the diff removes roughly 6,000 lines from src/ that are not yours — files that landed after you branched, which git removes because your branch predates them: src/reconciliation/dispersion.ts (312, #448), src/__tests__/venueReconciliation.test.ts (385), src/__tests__/observationAndSourceTime.test.ts (335, #240), and src/routes/facilitator.ts, whose x402 spend ceiling landed this morning.
Please rebase on current main and re-request. npx tsc --noEmit and npx vitest run both pass on main right now, so anything they report afterwards is genuinely yours.
Specifically worth re-checking after the rebase: the /price cache key on main is now decimal-format aware —
const cacheKey = decimalFormat === 'string' ? ... — because the same pair can be served as JSON numbers or as exact decimal strings, and those must not share an entry. So "aligning" the aggregate-refresh key means matching that discriminator too, not just the pair and network. If the refresh writes one format and a caller asks for the other, you get a miss that looks exactly like the bug you are fixing.
|
@Miracle656 — this PR is ready for another review. I resolved the merge conflicts with the main branch (pushed 09ef1f2). |
3df8443 to
900a15e
Compare
|
Correction to my earlier review — I got the main point wrong. I said this branch was far behind Measured properly against current Nothing would be reverted. I apologise — that framing was alarming and wrong, and it may have made this look like far more work than it is. What I said about the code itself still stands, and those points are unchanged. The cache-key alignment still needs to account for |
900a15e to
706951e
Compare
|
@Miracle656 — this PR is ready for another review. I fixed the failing CI checks on this PR (pushed 706951e). |
|
@Miracle656 — this PR is ready for another review. I resolved the merge conflicts with the main branch (pushed 6d5be8d). |
6d5be8d to
d0bb13d
Compare
|
Warning Possible malicious code: please review before merging. An automated security scan flagged files in this pull request that appear to contain malicious code. Code like this can run automatically when a project is opened in an editor or its dependencies are installed, so it could compromise the machine of anyone who checks out this branch. Automated scans can be wrong, so this may be a false positive. Maintainers: please review the flagged files carefully before merging, on GitHub in your browser rather than by checking out the branch. The malicious part is often hidden far off to the right of an ordinary-looking line, so scroll sideways through each file. @AllysiadevOps: this pull request adds or changes these files, possibly without your knowledge. Malware like this spreads by adding itself to repositories on machines it has infected, so your machine might be compromised. Please undo these changes in this pull request, and don't open or run other projects on that machine until you've checked it. Flagged files at commit Only merge once you're confident these files are safe. ℹ️ Posted automatically by Drips Wave. Questions? Contact support@drips.network. |
Overview
This PR fixes the dead warm-cache path in the aggregate-refresh worker. The worker was writing cache entries under a bare pair key while the
/priceroute read them under a network-prefixed key, so every/pricerequest was a cold miss. The fix makes the route and the worker produce an identical cache key, and adds a test that proves it.Related Issue
Changes
🔑 Cache key alignment
[MODIFY]
src/api/rest.ts/pricehandler now reads the cached price using the same barepairKeythe aggregate-refresh worker writes with, instead of the double-prefixed${network}:${pair.pairKey}form.redis.tsremains the single owner of thelens:${activeNetwork}:price:prefix, so the worker and the route now agree on the full key without either caller re-adding a network segment.[ADD]
src/__tests__/aggregateRefreshNetwork.test.tsredis.set/redis.getto assert the worker and the/priceroute produce an identical cache key for the same pair.X-Cache: HIT.Verification Results
redis.tsowns thelens:${activeNetwork}:price:prefix; callers pass the bare pair keyredis.set/redis.getproves worker and route produce an identical keyaggregateRefreshNetwork.test.tsX-Cache: HITaggregateRefreshNetwork.test.tsaggregateRefreshNetwork.test.tsCloses #166