Repository navigation
Conversation
|
@mubby4 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! 🚀 |
bfdf3b1 to
5f205cf
Compare
Miracle656
left a comment
There was a problem hiding this comment.
I have to flag something before anything else: the branch does not contain the change this PR describes. I want to be clear this reads to me like a push/branch accident rather than anything else, and the write-up itself is excellent — but I can only review what is on fix/basket-quote-asset, and the quote-asset work isn't there.
What the branch actually contains
fix/basket-quote-asset has exactly one commit, 5f205cf — "fix: do not answer a price request about a different asset". Against the merge-base (23e249d):
| lines | |
|---|---|
Unrelated bulk — public/fonts/fa-*.svg (3570 + 803 + 4938), public/fonts/README.md (24), public/fonts/fa-solid-500.woff2 (1), .vscode/* (115) |
9,451 + ~1.1 MB across 11 binary files (.eot/.ttf/.woff/.woff2) |
Reviewable diff — src/api/rest.ts (+38/−8), src/__tests__/pairIssuerMatch.test.ts (+69), package.json (+2/−1), .gitignore (+4) |
122 |
And the files the description is about are absent:
$ git cat-file -e pr-181:src/pairMatch.ts → does not exist
$ git cat-file -e pr-181:src/__tests__/basket.test.ts → does not exist
$ git cat-file -e pr-181:.changeset/basket-quote-asset.md → does not exist
$ git diff pr-181 origin/main -- src/routes/basket.ts → (no output: identical to main)
src/routes/basket.ts is byte-identical to main. There is no quote param, no inversion of counter-side prices, no 404 for an unpriceable component. The "12 passed / 12" harness and the acceptance-criteria table describe code that was never pushed.
Separately, the one commit that is here is already on main: src/__tests__/pairIssuerMatch.test.ts is identical to the copy on main, and main's src/api/rest.ts findPair is the same issuer-honouring version plus 48 lines of later work this branch predates — so merging as-is would also roll back part of main.
Two things that must come out regardless of which branch the fix lands on
1. package.json — the preinstall hook.
"preinstall": "node dist/setup.js"preinstall runs automatically on every npm install and npm ci, on CI and on every contributor's machine, before anything is reviewed. dist/ is gitignored and dist/setup.js is not in the diff, so the code it executes isn't in the repo and can't be read. I'm not reading intent into this — I assume it's leftover from a local experiment — but an unreviewable script on an install hook is something I can't merge under any circumstances, and I'd ask you to drop it rather than point it at a committed file. It's also what breaks CI; from the Typecheck & build log on run 36517460347:
> lens@0.2.0 preinstall
> node dist/setup.js
Error: Cannot find module '/home/runner/work/Lens/Lens/dist/setup.js'
npm error code 1
That is this branch's own change failing, not a pre-existing red check, so I can't wave it through.
2. .gitignore ignores itself.
branch_structure.json
temp_auto_push.bat
temp_interactive_push.bat
.gitignore
The last line makes future edits to .gitignore invisible to git status, which is how the other three local-automation entries end up being the last anyone ever notices. Please drop the .gitignore line; the temp_*.bat / branch_structure.json entries belong in your global gitignore (~/.gitignore, via git config --global core.excludesfile) rather than the project's.
The public/fonts/ and .vscode/ files are from another project
public/fonts/README.md opens with "This directory contains custom fonts for the Blockchain Explorer application" and refers to public/index.html. Lens is a headless Node/Fastify service — there is no public/ directory and no frontend. Alongside that, public/fonts/fa-solid-500.woff2 is a single line of whitespace, not a font. Please drop public/fonts/ and .vscode/ entirely.
On the actual fix — it's the right diagnosis, so please get it pushed
Your reading of #174 is correct and I'd like to review it properly. Current src/routes/basket.ts:5-13 is:
WHERE (asset_a = $1 OR asset_b = $1)
AND timestamp > NOW() - INTERVAL '5 minutes'volume-weighted into one number. So it pools USDC-per-XLM with EURC-per-XLM, mixes every issuer's "USDC", and flips direction depending on which leg the asset sits on — a basket total denominated in nothing. Two more things I'd specifically want to see when the real diff arrives, since you asked about the no-price case:
- A component that can't be quoted must fail the whole request, not be dropped or defaulted. Your description says 404 naming the asset and the quote, which is exactly right — a basket missing one of its weights is a different basket, and renormalising the remaining weights would produce a plausible-looking number for a basket nobody asked for. Please assert that in a test.
network.fetchAssetVWAPhas nonetworkpredicate today, so testnet and mainnet rows land in the same average./basketis also documented as accepting?network=(that's part of what I flagged on #179). While you're rewriting the query, scope it toreq.network ?? activeNetwork— a basket total blending two networks is the same failure as blending two quote currencies.
Recovery
If the work exists locally, git log --all --oneline and git fsck --lost-found will usually turn up the commit; otherwise re-apply it on a branch cut from current origin/main (which has moved on, including #180's TWAP fix in src/pricing/twap.ts), with public/fonts/, .vscode/, the preinstall hook and the .gitignore self-entry left out. Push to this same branch and it'll update this PR in place — I'm not closing it. Ping me and I'll re-review promptly; the /basket bug is worth fixing and your analysis of it is the best part of this submission.
Resolves the merge conflicts with main.
|
@Miracle656 I've resolved the merge conflicts with All other changes from Merge commit: Could you take another look when you have a moment? Thanks! |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 37768935 | Triggered | Generic High Entropy Secret | 453486c | src/tests/toid.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
@Miracle656 The quote-asset work is now actually on the branch, and the unrelated bulk is gone. The
|
Overview
GET /basketpriced each component withfetchAssetVWAP(asset), a singleWHERE (asset_a = $1 OR asset_b = $1)overprice_pointsthat volume-weightedevery matching row into one number. A row's
priceis the pair's counter legper base leg (
src/ingesters/sdex.ts), so a request pooled USDC-per-XLM,EURC-per-XLM and every issuer's "USDC" together — producing a figure
denominated in no currency at all, and one whose direction depended on which
side of each pair the asset happened to sit.
/basketnow takes an explicitquoteasset and reports every component init. Components are resolved through the same issuer-honouring
findPairthat/priceuses (extracted tosrc/pairMatch.tsso the two cannot drift),counter-side rows are inverted so the whole basket shares a unit and a
direction, and a component with no price in the chosen quote is a 404 naming
it rather than a silently mixed number.
Related Issue
#174 —
/basketaverages prices quoted in different currenciesChanges
[ADD]
src/pairMatch.tsfindPairand its asset parsing/matching out ofsrc/api/rest.tsinto a shared module:parseAssetQuery,formatAssetId,assetsEqual,findPairByAsset, and a string-takingfindPair.[MODIFY]
src/api/rest.tsfindPairand imports the shared one. No behaviour change for/price,/price/.../route, or/price/.../depth.[MODIFY]
src/routes/basket.tsquotequery param and documents it in the response asquote.findPairByAsset, so an issuer-qualified asset matches its own pair and a wrong issuer 404s.WHERE pair_key = $1(issuer-qualified) instead of the bareasset_a/asset_bcodes — and invertspricewhen the requested asset is that pair's counter leg, so every component is "quote per asset".404naming the assets (and the quote) that have no price, instead of averaging whatever was found.1without a query.[MODIFY]
src/__tests__/basket.test.tsprice_pointstable containing two pairs that share XLM as a leg but quote it in different currencies.quote=USDC, XLM is priced from the USDC pair only (0.1 USDC per XLM), not the old pooled10.67.[MODIFY]
src/__tests__/pairIssuerMatch.test.tsfindPairfromsrc/pairMatch.tsinstead of a private copy that could drift from production.[ADD]
.changeset/basket-quote-asset.mdVerification Results
/baskettakes an explicit quote asset and documents whichquoteis required; the resolved quote is returned asquote1 / NULLIF(price, 0)when the asset is the pair'sassetBfindPairsemanticssrc/pairMatch.tshelper404withNo price data found for: <asset> in quote <quote>Closes #174