Repository navigation
feat: per-network config map, staleness-aware median pricing, and liquidity-depth slippage endpoint - #484
Conversation
…uidity-depth slippage endpoint (Divinity-Labs#81, Divinity-Labs#84, Divinity-Labs#113)
Miracle656
left a comment
There was a problem hiding this comment.
The staleness machinery is good and the shape is right — excluding stale sources with a reason and an ageSeconds, returning the excluded list alongside the included one, and falling back by source priority rather than silently. A caller can see what was used and what was thrown away, which is what makes a median trustworthy rather than just a number.
One blocker: this can answer a price request with 0.
export function calculateMedian(values: number[]): number {
if (values.length === 0) return 0
…
}and the caller:
let medianPrice = 0
if (included.length > 0) {
medianPrice = calculateMedian(included.map((s) => s.price))
} else if (sources.length > 0) {
…fallback…
}When a watched pair has no price rows at all, sources.length === 0, neither branch runs, and the route returns 200 with medianPrice: 0, sourceCount: 0, fallbackUsed: false. The same happens if every source is filtered out as invalid before the fallback sees it.
Zero is not a price. A caller sizing a trade against it, or dividing by it, gets a catastrophic answer — and 0 is indistinguishable from a genuine quote for a worthless asset, so nothing downstream can defend itself. This is the exact failure #356 was praised for avoiding: reports an empty market explicitly instead of returning a zero-price book.
What I would like:
calculateMedian([])should not return0. Throw, or returnnulland make the type say so — the signature currently promises anumberit cannot honestly produce.- The route should answer "no data" distinctly: a 404 beside the existing
Pair not watched404, or a 200 whose body hasmedianPrice: nulland a reason. Either is fine; silently-zero is not. - A test for the empty case, and one for "every source excluded as invalid", since that path reaches the same place by a different route.
Two smaller notes
freshnessSec is parsed but not bounded. ?freshness=999999999 makes staleness meaningless and ?freshness=1 excludes everything and lands you on the fallback — or, with no sources, on the zero above. An upper bound and a documented default would make the parameter honest.
The fallback picks the most recent valid source when no priority matches, which is reasonable — but the result says fallbackUsed: true without saying how stale the chosen source was. ageSeconds on the fallback entry, as you already do for exclusions, would let a caller decide whether to trust it. Right now a thirty-second-old fallback and a three-day-old one look identical.
Everything else passes — 698 of 699 locally, and the one failure is graphqlSchemaDrift from CRLF in my own working copy, not yours.
Fix the zero and I will merge it.
Miracle656
left a comment
There was a problem hiding this comment.
Changes requested: one issue per PR, and only issues assigned to you
This is a process requirement, applied to all 23 of your open PRs in this repo. It is not a judgement about the code, which I have not been able to review — and that is the first half of the problem.
The bundling
Across your open PRs, 150 distinct issues are claimed. This one claims several. #483 claims 22, #481 claims 20, #477 claims 20, #474 claims 20.
A PR that closes twenty issues cannot be reviewed. Each of those issues has its own acceptance criteria — typically five — so twenty issues is a hundred separate things to verify in one diff, and if any one of them is wrong the whole PR is blocked while it is fixed. Every batch I published says the issues are deliberately independent so they can be worked in parallel, by different people, without colliding. Bundling them removes exactly that property.
The assignments
135 of the 150 issues claimed are assigned to other contributors — 67 different people.
This PR is an example: it names issues currently assigned to other contributors who are working on them now. Several have open PRs of their own. If this merged as it stands, those people would have done the work and received nothing for it, and I would have no way to tell the two efforts apart.
I am not assuming anything about why. Assignments change, Drips and GitHub can disagree, and a wide-ranging branch picks up issue numbers easily. But the effect on the people holding those assignments is the same regardless of intent, which is why I have to act on it.
What I need
- One issue per PR. The issue number in the title, its acceptance criteria met, and nothing else in the diff.
- Only issues assigned to you, or unassigned ones you have claimed through Drips. If an issue is assigned to someone else, it is theirs.
- Re-open the work that meets both conditions as individual PRs. I will review them in the order they arrive and at the same pace as everyone else's.
Nothing is being closed. These stay open and are yours to re-cut, withdraw or split as you choose.
Two things worth knowing before you re-cut
Some of this has already landed. #488 (SSRF guard for bazaar_search) and #487 (the paid_call host allow-list) cover the same ground as your #449; #486 covers the MCP README. Check current main before rebuilding something that is already there.
main has moved a long way today — #306, #309, #252, #256, #355, #381, #390, #402, #476, #488 and more. Cut from current main, not from where these branches started.
If you believe an issue named here is genuinely yours, say which and I will check the assignment myself — I would rather be corrected than have someone lose work.
…reshness, and report fallback age (Divinity-Labs#81)
|
Hi @Miracle656, Thank you for the thorough review and actionable guidance! We have addressed all points in commit
Ready for merge! |
Summary of Changes
This PR implements the final foundational suite across configuration architecture, staleness-aware price aggregation, and liquidity-depth slippage estimation:
1. Per-Network Config Map Architecture (#113)
2. Staleness-Aware Median Pricing with Source Fallback (#81)
3. Liquidity Depth & Slippage Estimation Endpoint (#84)
Closes #113
Closes #81
Closes #84