Feat/accounts balance endpoint - #48
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.
The accounts balance endpoint is solid work — Horizon lookup, proper 404 handling, and tests. A few things to fix:
1. Remove scratch/generate_fixtures.js
This file appears in #45, #46, and now here. It's a debug script and shouldn't be in the codebase. git rm scratch/generate_fixtures.js.
2. Stacked PRs create duplicate diff
Your PR includes all the changes from #45 (decoder) and #46 (token caching) on top of the accounts endpoint. This makes the diff hard to review in isolation. Once #45 and #46 are merged (after they remove the scratch file), please rebase this PR so the diff only shows the accounts-related additions.
3. Unchecked checklist
Please check the boxes that apply (npx tsc --noEmit, npm run build, tests added) so we know the PR was self-validated before review.
46ecffd to
3227bf7
Compare
|
I've completed the validation and cleanup for the accounts-balance-endpoint PR (#48). Here is the update on the requested changes: Removed scratch/generate_fixtures.js: I've confirmed that this file is now removed from all three branches (#45, #46, and #48). Since #48 is rebased on the others, the cleanup is propagated through the entire stack. Branch: feat/accounts-balance-endpoint |
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!
3227bf7 to
2a0df87
Compare
2a0df87 to
5809806
Compare
|
Friendly nudge — a couple of things to resolve here: (1) the account balance need is now largely covered by the merged If you're still up for it, please rebase on the latest |
|
CI is failing on this PR — the |
|
Conflicting with |
…-endpoint # Conflicts: # package-lock.json # package.json # prisma/schema.prisma # src/__tests__/metrics.test.ts # src/__tests__/tokenCache.test.ts # src/api.ts # src/db.ts # src/indexer.ts # src/metrics.ts # src/rpc.ts # src/tokenCache.ts
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.
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Approved and merging — rebased and reworked on your branch (cfbaf72), since the wave has closed.
The best thing about this PR is the honesty in the response. derived_from_ledger: true plus a note saying the figure may not include pre-indexer history is the correct instinct, and it is the one most people skip. A balance endpoint that returns a bare number implies an on-chain read, and a caller has no way to discover otherwise — the value is a plausible-looking number that happens to be low for anyone who held tokens before the indexer's start ledger. Saying so in the payload is the difference between a useful endpoint and a misleading one. I kept it, in camelCase to match the rest of the API, and made the note more explicit about why.
Three fixes to the query:
- The table reference was unqualified —
FROM "TokenTransfer". Every other raw query indb.tsuses"wraith"."TokenTransfer", because the models declare@@schema("wraith"). Unqualified, it resolves only ifsearch_pathhappens to include the schema — so it works locally and fails on a deployment that setssearch_pathdifferently. That is the worst failure shape: correct in dev, broken in prod, with an error message pointing at a missing relation rather than at a missing qualifier. - No network predicate. Summing both chains' transfers for one address produces a number that corresponds to no balance anywhere. It takes the network now and filters on it, with the route reading the selector so an unrecognised network 400s rather than silently answering for the default.
- The metrics timer was not in a
finally, so a throwing query leaked it. UsesobserveDbQuery, which times failures too — the query that runs for eight seconds and then fails is exactly the one you want in the histogram.
I also mounted it on the existing accounts router instead of a second one, so it sits beside /summary and /transfers and inherits the network middleware, and added the raw stroop amount alongside the display string — otherwise a consumer doing arithmetic has to parse the decimal back into an integer and guess the scale.
On the rebase: main has moved a long way since June. The token cache landed via #46 earlier today and the metrics module via #175, so those parts of this PR are duplicates now and are dropped. What is left is the balance endpoint, which is what the PR is named for.
Your tests carried over with the response shape updated, plus network scoping and the raw/display split.
Verified: tsc --noEmit clean, full suite 402 passed.
2 participants
Summary
Related issue
Type of change
Checklist
npx tsc --noEmitpassesnpm run buildpasses