Feat/token metadata caching - #46
Conversation
|
@K1NGD4VID 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! 🚀 |
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Token metadata caching is the right approach for #37. Three things to address:
1. Remove scratch/generate_fixtures.js
Same as #45 — this debug script shouldn't be committed. git rm scratch/generate_fixtures.js.
2. Stacked on #45
Your diff includes all of #45's changes (decoder rename, decoder tests, fixtures). If #45 merges first your diff will shrink automatically to just the caching additions. Please either: (a) wait for #45 to merge and rebase, or (b) confirm you want this PR to supersede #45 and we'll close that one.
3. prisma/schema.prisma changes
You're adding to the Prisma schema here and also in #48. Make sure only one PR owns schema changes to avoid conflicts.
… and RPC fallback
b0a2f1c to
7555083
Compare
|
I've addressed the feedback for both PR #37 (feat/token-metadata-caching) and PR #45 (test/xdr-decoder-unit-tests). Here’s the summary of the changes: Removed scratch/generate_fixtures.js: I've removed this script from the feat/token-metadata-caching branch. I also ensured it was cleaned up on the test/xdr-decoder-unit-tests branch as previously requested. feat/token-metadata-caching: Rebased on #45, script removed, force-pushed. |
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Good progress — scratch/generate_fixtures.js is gone. The token metadata cache (memory → DB → RPC) is the right architecture.\n\nOne thing to fix before merging:\n\nRebase on main — PR #45 (XDR decoder) merged today, so your branch still carries all of #45's changes in the diff. After rebasing, the diff will shrink to just the caching-related additions (tokenCache.ts, tokenCache.test.ts, schema changes, etc.) making it much easier to review in isolation.\n\n\ngit fetch origin\ngit rebase origin/main\ngit push --force-with-lease\n\n\nOnce rebased I'll approve immediately.
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
This PR has fallen behind main and now shows merge conflicts (and it's been a few weeks since the last update). The feature itself is welcome — could you rebase on the latest main and resolve the conflicts? Once it's mergeable I'll review and merge. Thanks!
|
Friendly nudge — this is CONFLICTING, and its token-metadata caching overlaps the If you're still up for it, please rebase on the latest |
|
This PR has |
|
This is conflicting with |
The cache was complete but unreachable: initTokenCache and getTokenMetadata were imported into src/indexer.ts and never called, so TokenMetadata stayed empty and GET /tokens always returned []. Same shape of failure as Miracle656#137 — nothing errors, the feature simply never runs. Wiring: - initTokenCache(net) when a loop starts, so the first batch does not pay an RPC round-trip per contract already in the database. - pollOnce resolves metadata for each distinct contract in the batch. Only a cache miss reaches RPC, and a miss happens once per contract for the life of the database, so this is one extra call the first time a token is seen and free after. Best-effort: a token whose metadata cannot be read is still worth indexing transfers for. - GET /tokens is scoped to the selected network and names it in the response. Network keying, which this branch predates: - TokenMetadata was keyed `contractId @id`. A contract id is only unique within a chain, so a testnet token at the same address as a mainnet one would share the row — serving the wrong symbol and, far worse, the wrong `decimals`, which silently rescales every amount rendered from that token by orders of magnitude. Now @@id([network, contractId]), with the in-memory cache keyed `network:contractId` to match. - fetchTokenMetadata took no network and used getRpc() plus a passphrase read from STELLAR_NETWORK. With one loop per network (Miracle656#161) that simulates a mainnet contract call against testnet, returning nothing or a different token. It now takes the network and derives both from it. - Added the migration this PR never had (20260901140000), ordered after add_network for the same reason as the others. Tests: added _resetTokenCache so cases stop leaking module-level state into each other — the original beforeEach had a comment noting it could not clear the map — plus coverage that the same contract id on two networks resolves to two different tokens, that the RPC is asked for the network the answer is cached under, and that a failed DB seed still serves from RPC. Merge: three import conflicts in api.ts, indexer.ts and rpc.ts, all additive unions with main. tsc clean; full suite 396 passed.
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Approved and merging — I wired it in and rebased it onto main (4d34d3a), since the wave has closed.
The cache design is right: memory → database → RPC, with the database tier there so a restart or a second process doesn't re-pay RPC for tokens already resolved. That middle tier is the one people skip, and it's the one that matters when you're running more than one instance.
It was unreachable, though. initTokenCache and getTokenMetadata were imported into src/indexer.ts and never called, so TokenMetadata stayed empty and GET /tokens always returned []. Same shape as the tombstone detector in #155 — nothing errors, no test goes red, the feature simply never runs.
Now wired:
initTokenCache(net)at loop start, so the first batch doesn't pay a round-trip per contract already in the database.pollOnceresolves metadata for each distinct contract in the batch. Only a miss reaches RPC, and a miss happens once per contract for the life of the database — so this is one extra call the first time a token is seen and free thereafter. Best-effort: a token whose metadata can't be read is still worth indexing transfers for.GET /tokensis scoped to the selected network and names it in the response.
The network keying is the substantive change, and this is the one I'd flag hardest. The model was contractId @id. A contract id is only unique within a chain, so a testnet token at the same address as a mainnet one shares the row. That gets you the wrong symbol — which someone would notice — and the wrong decimals, which nobody would: it silently rescales every amount rendered from that token by orders of magnitude, and the number still looks like a plausible number. Now @@id([network, contractId]), with the in-memory map keyed network:contractId to match.
Same class of bug in fetchTokenMetadata: no network parameter, getRpc() for the endpoint, and a passphrase read straight from STELLAR_NETWORK. With one loop per network (#161) that simulates a mainnet contract call against testnet and gets back nothing, or a different token. It takes the network now and derives both from it.
I also added the migration this PR never had, ordered after add_network.
On the tests — the original beforeEach carried this comment:
// Clear the internal Map by some means?
// Since it's a module-level constant, I might need to reset it.That instinct was correct and worth acting on: without a reset, a "cache hit" assertion can be satisfied by a value some earlier test left behind, and the test passes for the wrong reason. There's a _resetTokenCache export now. Added coverage that the same contract id on two networks resolves to two different tokens, that the RPC is asked for the network the answer gets cached under, and that a failed DB seed still serves from RPC — a cold cache is recoverable, refusing to start is not.
Verified: tsc --noEmit clean, full suite 396 passed.
Good idea, and it now actually runs.
Derived per-token balances for an address, summing what it received and subtracting what it sent across the indexed history. Rebased onto main, which has moved a long way since this branch: the token cache landed via Miracle656#46 and the metrics module via Miracle656#175, so those parts of this PR are dropped as duplicates and what remains is the balance endpoint itself. Three fixes to the query on the way in: - The table reference was unqualified, `FROM "TokenTransfer"`. Every other raw query in db.ts uses `"wraith"."TokenTransfer"`, because the models declare @@Schema("wraith") — unqualified it resolves only if search_path happens to include the schema, so it would work locally and fail on a deployment that sets search_path differently. - No network predicate. Summing both chains' transfers for one address gives a number that corresponds to no balance anywhere. Now takes the network and filters on it, with the route reading it from the selector so an unknown network 400s instead of silently answering for the default. - The metrics timer was started and stopped around the query but not in a finally, so a throw leaked it. Uses observeDbQuery, which times failures too — a query that takes eight seconds and then fails is the one worth seeing. Mounted on the existing accounts router rather than a second one, so it sits beside /summary and /transfers and inherits the network middleware. The response keeps this PR's honesty about what the number is — a sum over the indexed window, not an on-chain read — and returns both the raw stroop amount and the display string, so a consumer doing arithmetic does not have to parse the decimal back and guess the scale. tsc clean; full suite 402 passed.
* feat: implement tiered token metadata caching with Prisma persistence and RPC fallback * feat: implement Prometheus metrics collection with registry and endpoint testing * feat: implement accounts balance route with ledger-derived token balances * chore: add vitest as devDependency for test:integration * fix: exclude broken upstream tests from jest (opa, integration) * Add GET /accounts/:address/balance, network-scoped and schema-qualified Derived per-token balances for an address, summing what it received and subtracting what it sent across the indexed history. Rebased onto main, which has moved a long way since this branch: the token cache landed via #46 and the metrics module via #175, so those parts of this PR are dropped as duplicates and what remains is the balance endpoint itself. Three fixes to the query on the way in: - The table reference was unqualified, `FROM "TokenTransfer"`. Every other raw query in db.ts uses `"wraith"."TokenTransfer"`, because the models declare @@Schema("wraith") — unqualified it resolves only if search_path happens to include the schema, so it would work locally and fail on a deployment that sets search_path differently. - No network predicate. Summing both chains' transfers for one address gives a number that corresponds to no balance anywhere. Now takes the network and filters on it, with the route reading it from the selector so an unknown network 400s instead of silently answering for the default. - The metrics timer was started and stopped around the query but not in a finally, so a throw leaked it. Uses observeDbQuery, which times failures too — a query that takes eight seconds and then fails is the one worth seeing. Mounted on the existing accounts router rather than a second one, so it sits beside /summary and /transfers and inherits the network middleware. The response keeps this PR's honesty about what the number is — a sum over the indexed window, not an on-chain read — and returns both the raw stroop amount and the display string, so a consumer doing arithmetic does not have to parse the decimal back and guess the scale. tsc clean; full suite 402 passed. --------- Co-authored-by: Miracle656 <iupacnumen2020@gmail.com>
2 participants
Summary
Related issue
Type of change
Checklist
npx tsc --noEmitpassesnpm run buildpasses