[Naajih09]

Closes #298

What this does

Pricing previously used a single fee constant for every AMM pool. Protocols set fees per pool, so pools with a different fee were priced wrongly with no error. This change reads each pool's fee from the venue, caches it, and uses it wherever a fee is needed.

• Reads the fee for each AMM pool from the venue.
• Caches the fee and tests the cache, including expiry and refresh.
• Excludes any pool whose fee cannot be read from execution estimates, and reports it in the response so the exclusion is visible.
• Removes the fee constant from the pricing path.
• Echoes network in every response it touches and treats ?network= as required on any new behavior, per the issue's guidance.

Provenance

Responses that use a pool's fee include the pool ID, the fee in basis points, and when that fee was read. Excluded pools are listed with the reason.

Scope

SDEX order-book levels do not have a per-pool fee, so this PR covers AMM pools only. Any SDEX fee handling stays unchanged.

Known limits

• Fee source. I have not yet confirmed the exact field the venue exposes for each pool's fee. This needs to be checked against the protocol's documentation before merge.
• Cache lifetime. The cache duration is a judgment call. I'll state the value used and why in the PR review.
• Not run against a live service. Tests run locally with fixtures. Verification against live pools is still needed.

How to verify

[drips-wave]

@Naajih09 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! 🚀

Learn more about application limits

[Miracle656]

Fails on its own branch, so this is not about merge order:

src/api/rest.ts(281,33): error TS2554: Expected 4 arguments, but got 3
src/api/rest.ts(286,9):  error TS2783: 'pairKey' is specified more than once, so this usage will be overwritten

4 type errors and 2 failing tests merged alone against current main.

The second one is worth looking at closely rather than just deleting the duplicate. 'pairKey' is specified more than once is the shape a repeated base merge produces — a key gets re-added alongside the one already there, the last wins, and whichever value was meant to apply silently does not. We have had the same thing happen in package.json on this project. Check which pairKey is the one you intend before removing either.

The arity error on line 281 suggests a helper gained a parameter underneath you, which a rebase will surface properly.

Reading per-pool AMM fees from the venue rather than assuming a flat rate is a real improvement and I want it — assumed fees are how a routing recommendation quietly becomes wrong. Fix the two errors and I will take it.

[Miracle656]

Scanned clean. Reading per-pool AMM fees from the venue instead of assuming a flat rate is a real improvement and I still want it — src/pricing/pool-fee.ts is good, with a cache, an explicit PoolFeeOptions, and getPoolFee returning a result type rather than a bare number so an unreadable fee can be distinguished from a zero one. The route-level tests in tests/depth.test.ts exercise the behaviour that matters, including a pool being excluded with a reason.

It still does not compile, though, and this time it is not a stale base. Merged against current main, npx tsc --noEmit gives 6 errors and they all come from one unfinished change.

1. Two names are referenced but never defined — anywhere

src/pricing/depth.ts(264): Cannot find name 'PoolFeeOptions'
src/pricing/depth.ts(265): Cannot find name 'DepthOutcome'
src/pricing/depth.ts(270): Cannot find name 'loadLatestAmmPools'

PoolFeeOptions is the easy one — it exists in src/pricing/pool-fee.ts, it just is not imported into depth.ts.

The other two are not defined in src/ or in tests/:

$ git grep -n "DepthOutcome\|loadLatestAmmPools" -- src tests
src/pricing/depth.ts:265:): Promise<DepthOutcome> {
src/pricing/depth.ts:270:  const pools = await loadLatestAmmPools(network, pairKey)

Two references, no declarations. I think loadLatestAmmPools exists but in the wrong place — the SQL at depth.ts:238-250 selects latest pool rows and maps them to { poolId, reserveA, reserveB, observedAt }, which is exactly what the call expects, but it is sitting inside getAMMDepth. That is also error 2:

src/pricing/depth.ts(243): Type '{ poolId; reserveA; reserveB; observedAt }[]' is missing ... from type 'DepthResult'

getAMMDepth is declared to return DepthResult and is returning pool rows. So that body belongs in its own function, and getAMMDepth's was lost.

DepthOutcome needs writing. The docstring above getDepth describes it well — "if no pool survives, the result is unavailable with no numbers, never zeros" — which is a discriminated union, and defining it is a decision about your public shape, not something I should guess at. I have deliberately not invented it.

2. The signature changed, including parameter order, and the caller did not

src/api/rest.ts(454): Expected 4 arguments, but got 3

main: getDepth(pairKey, amount, network)
this branch: getDepth(network, pairKey, amount, feeOpts)

Please look hard at this one before fixing it, because the obvious fix is a trap. The arity error is loud, but the reorder is silent: network and pairKey are both string, so if the call site is repaired by appending a fourth argument without reordering the first two, it compiles cleanly and then prices every pair against a network named "XLM/USDC". That is the kind of bug that reaches production looking like missing data.

If the new order is deliberate, say so in the PR and update rest.ts:454. If not, keeping (pairKey, amount, network, feeOpts) means the existing call only needs a fourth argument and no reordering at all — which I would prefer, for exactly the reason above.

One thing to know about the tests

tests/depth.test.ts goes through buildApp and asserts the JSON body, so it never calls getDepth directly. That is a fine way to test the route, but it means the new getDepth is currently unexercised — a green suite will not tell you whether it works. Worth one direct test of the unavailable branch once DepthOutcome exists, since "no pool survived" is the case that must not come back as zeros.

Get npx tsc --noEmit clean and re-request; the rest of this is in good shape and I will take it quickly.

https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB

[Miracle656]

@Naajih09 merged into main (2c53923) — and properly merged this time, so it shows under your name.

I finished it rather than bouncing it back again, since you had done the hard part and the rest was reconstruction. Here is exactly what I changed, so nothing is a surprise.

src/pricing/pool-fee.ts landed as you wrote it. That file is the heart of this: a cached getPoolFee that returns a result rather than a number, so an unreadable fee is distinguishable from a fee of zero and the pool gets excluded instead of priced with a guess. That is the whole point of #219 and you got it right.

The five things that were missing

DepthOutcome and DepthUnavailable — I defined these from what your getDepth already builds, so the shape is yours, not mine: an unavailable arm carrying network, pairKey, reason and excludedPools, and no numbers at all. That makes "no pool could price this" impossible to confuse with a price of zero at the type level, which is stronger than leaving it to convention.

loadLatestAmmPools existed only as a call. Its body was inside getAMMDepth — which had no callers anywhere and was declared Promise<DepthResult | null> while returning pool rows. So that function became the loader. Worth knowing: its SELECT listed reserve_a, reserve_b, fee_bp while the mapper read r.pool_id and r.timestamp, so both would have been undefined at runtime, silently. Those are in the column list now.

getPoolFee and PoolFeeOptions imported; the route passes the per-request network's Horizon URL.

503, not 200. Your tests expect 503: returns unavailable with no numbers when every pool is excluded, but the route returned 200 unconditionally — and depthResponseSchema had additionalProperties: false with spotPrice required, so Fastify's serialiser rejected the new shape and every request came back 500. The schema now covers both arms and the route answers 503 when unavailable. That is the right code: the pair is watched, we just cannot price it right now.

Number() instead of parseFloat() for ?amount=. parseFloat('100abc') is 100, so a typo silently priced a different trade than the caller asked for. Your "trailing junk" test is what caught it.

Two test notes

The pre-existing src/__tests__/depth.test.ts asserted the old contract — spotPrice 0, empty books when no pool exists. That is precisely what this PR removes, so those tests were defending the bug. I rewrote them against the new contract rather than deleting them, and added a third case: a pool whose fee cannot be read is excluded and named.

In your tests/depth.test.ts, vi.mock('../src/pairs') never took effect — findPair is local to src/api/rest.ts and reads getNetworkConfig(network).pairs. The watched pair comes from the config mock now, which is also why the suite needed REQUIRE_API_KEY=false once the real config was in play.

tsc clean, 1,755 tests pass.

Thanks for flagging that this one was still open — that was the right call, and sorry it took two rounds plus a nudge.

https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB