Release: develop -> main - #4504
Merged
Merged
Conversation
…points (#4498) * Document the `wait` naming convention for long-polling endpoints An endpoint that blocks on purpose must carry `wait` as its own path segment, and an endpoint expected to answer quickly must not use the word anywhere in its path. Monitoring depends on it: the "Slowest requests" panel of the dfx-api-traces dashboard excludes routes matching `^.*/wait(/.*)?$` on http.route, because a long poll's duration measures how long a customer took to act rather than how long the API computed. Naming a blocking endpoint without `wait` turns it into a permanent latency outlier that masks real regressions; naming a fast endpoint with `wait` silently removes it from the latency table. * Tighten the wait-naming section after review Four corrections: - The rule said a fast endpoint must not use `wait` "anywhere in its path", while the same section states that only a complete `wait` segment matches. Reworded to "as a path segment" so the rule and its matching semantics agree. - "cancelled" -> "canceled": CONTRIBUTING.md requires American English spelling, and this was the only occurrence in the file. - Dropped the internal dashboard name, the concrete row cap and the reference to the infrastructure repository. This is a public repo; the rule stands on its own without naming internal monitoring assets. - Added a cross-reference under Naming Conventions, where URL routes are already covered, so the rule is discoverable from there. * List the blocking endpoints that carry no wait segment Review found the endpoint table incomplete: three further routes block on an external event without a `wait` segment. - `GET /v1/lnurlp/:id` polls for a pending payment via Util.poll, default 10 s and extendable by the caller through `params.timeout`. It shows up in latency monitoring with peaks around 20 s. - `GET /v1/node/:node/tx/:txId` and its `:mode` variant poll for a confirmation for up to 600 s. None of them can reasonably be renamed: the LNURL pay-request path is encoded into LNURLs already in circulation, and the node routes are admin-only and excluded from the API docs. They are now listed as explicit exemptions rather than left unmentioned, so the rule describes the codebase as it is instead of claiming a clean slate. Also corrects the resolution condition: for MULTIPLE-mode payment links the waiter resolves once a quote reaches the completion threshold, while the payment itself can stay Pending. * Make the wait segment the default and exemptions explicit The previous wording bound only new endpoints and closed the list of exemptions outright. That is the wrong shape: the segment is the standard, and an exemption stays possible — but only as an explicit, justified entry in the table. A blocking route that neither carries the segment nor appears in the table is now stated to be a defect, with the two acceptable fixes named (rename, or document). This keeps the rule enforceable without pretending the three existing exemptions can be legislated away. * Separate passive waiting from waiting on your own operation Review showed the definition was too broad. Five further routes match "response time governed by an external event" without carrying a wait segment: - GET /v1/lnurlp/tx/:id (waits for an on-chain confirmation) - PUT /v1/sell/paymentInfos/:id/confirm, authorization branch - PUT /v1/swap/paymentInfos/:id/confirm, authorization branch - PUT /v1/realunit/sell/:id/confirm, eip7702 branch - PUT /v1/realunit/transfer/:id/confirm Listing them as exemptions would have been the wrong fix. They differ in kind from a long poll: each starts an operation and then waits for that operation to complete, so the duration measures work the API set in motion. That is exactly the signal latency monitoring should surface — PUT /v1/realunit/sell/:id/confirm has been observed at 47 s. Hiding it would defeat the purpose. The rule now says so explicitly: `wait` is for endpoints that do nothing but wait for another actor, and endpoints awaiting their own operation must not carry the segment. * Scope the defect wording to passive waiting only Review found an internal contradiction: the section first states that endpoints awaiting their own operation must not be named `wait`, then declares unconditionally that "a blocking route that is neither named `wait` nor listed here is a defect". Read literally, that reclassified the five routes just legitimized two paragraphs earlier as undocumented defects. Both the rule sentence and the monitoring bullet now say "passively waiting" instead of "blocking", and the rule states explicitly that routes awaiting their own operation need no table entry. * Reclassify GET /v1/lnurlp/tx/:id as a passive waiter Review caught a misclassification. The route was listed as "waits for its own operation", but the payment request tells the payer to broadcast the transaction themselves and send back only the hash (payment-request.mapper.ts): "Broadcast the signed transaction to the blockchain and send the transaction hash back via the endpoint". The API then polls for a confirmation of a transaction it never sent — passive waiting on an external actor, the same class as the other exemptions. Its `hex` branch does broadcast, but returns right after txInMempool without awaiting confirmation, so it is not a long poll at all. The route therefore moves into the exemption table. Like the LNURL pay-request path, it cannot be renamed: the URL is handed to the payer inside the payment request. * Correct the hex-branch note for the ICP case The exemption row claimed the `hex` branch "broadcasts and returns immediately — it does not wait". That holds for the EVM and Firo paths, but the ICP sub-branch waits for the payer's allowance first (payment-quote.service.ts:711, Util.retry with 3 attempts at 2 s) before broadcasting. The classification of the route is unaffected — it stays a passive waiter and an exemption — but the stated reason was inaccurate.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
July 30, 2026 12:49
`Util.retry(action, 3, 2000)` runs three attempts in total, not three retries after a first failure: the parameter is named `tryCount` and the recursion guard is `tryCount > 1` (util.ts:685). The wait therefore spans at most two delays of 2 s, not three. Follow-up to #4498, where the wording said "3 retries at 2 s".
…4506) * perf: skip balancesByFinancialType in financial log query on demand The financial-overview screen calls GET /v1/dashboard/financial/log every minute but never reads balancesByType; that sub-tree still made up 82% of the response and roughly half the DB time (measured 2030ms vs 1078ms for the same query). Add an opt-out query parameter that drops the jsonb sub-tree from the SELECT list instead of just discarding it after loading. Existing callers that don't pass the parameter keep getting the exact same response as before. * style: apply prettier line width to the new service signature * test(dashboard): pin the byType query parameter at the controller The endpoint had no controller test at all, so a transposed argument or an inverted string check would have passed every existing service and repository test. Six inputs are pinned; only the exact string false disables the subtree. * test(dashboard): pin dailySample and the from edge cases too dailySample is evaluated by the same rule as byType but was only ever tested with true and false, so inverting its check would have passed. It now gets the same six inputs, each holding byType at the opposite value so a transposition of the two arguments cannot hide. Adds the two untested from branches: omitted yields undefined, and an unparseable value throws without reaching the service. * test(dashboard): cover the empty-string case for from An empty from takes the same early-return branch as a missing one, but only this case catches a mutation of that branch to an undefined check: an empty string would then reach Date parsing and throw instead of yielding undefined.
…er aggregate (#4497) * perf(trading): index trading_order by (tradingRuleId, id) and rewrite the per-rule max-order lookup TradingRuleService.getCurrentTradingOrders ran a table-wide GROUP BY MAX(id) aggregate over trading_order once per minute (LogJobService), scanning 5.4M rows to compute 17 maxima (474 ms, ~680 MB read per call). The obvious per-rule rewrite is not safe on its own: measured in production against the current index set, a correlated MAX(id) subquery per rule takes 4,329 ms — 9x slower than today, because Postgres has no index to jump straight to a rule's row range and PostgreSQL 17.10 has no index skip scan. So this ships the composite index and the query rewrite together, never apart: migration/1785480000000-AddTradingOrderRuleIdIndex.js adds ("tradingRuleId", "id"), and getCurrentTradingOrders now looks up the max id per rule directly, letting Postgres use that index (17 cheap lookups instead of one full scan). The existing single-column index on tradingRuleId is left in place; the migration docstring notes the resulting redundancy. Added trading-rule.service.pg.spec.ts (pg-mem, real generated SQL) to pin the rewrite: highest id per rule, rules without orders produce no entry, an order with no matching rule is excluded, and the result is checked against the pre-rewrite aggregate on the same seeded data. * fix(trading): resolve migration ordering collision, document N+1 trade-off AddTradingOrderRuleIdIndex1785480000000 sorted before the already-merged ClearDevUserSignatures1785500000000, so it would run before an already-executed migration. Renamed to 1785510000000 (class name and name property updated to match); the generated index name IDX_710fd49e19d248643cb2afa70f is unaffected, since TypeORM derives it from the table and column names, not the timestamp. Also documents in getCurrentTradingOrders that the per-rule MAX(id) lookups are an N+1 pattern that scales linearly with the rule count (currently 17 rules -> 19 queries), and why a single correlated-subquery alternative was skipped: it would give up testability against the lightweight mirror entities used in trading-rule.service.pg.spec.ts. * revert(trading): keep the single-statement aggregate, ship the index alone The per-rule rewrite is withdrawn. It traded one READ COMMITTED snapshot for N, so two rules could report maxima from different points in time — not acceptable where LogJobService writes the FinanceLog from the result. The coherent single-statement alternative is a correlated subquery, which pg-mem cannot execute at all, so it could not be covered by a test that runs. The index stays: measured against a Postgres 17.10 rebuild with production planner settings, the unchanged aggregate moves from a Parallel Seq Scan over 93,486 blocks to a Parallel Index Only Scan over 15,020 with zero heap fetches. The docstring drops the claim that index and rewrite must ship together. The test loses its comparison against a re-created copy of the same aggregate, which could no longer fail, and gains an empty-rule-table case plus an explicit assertion that no null reaches the In(...) list. * docs(migration): drop an unmeasured table size from the docstring intro The opening sentence claimed ~920 MB, which matches none of the measurements this migration cites — production reports 698 MB for the heap and the rebuild 730 MB. The intro now carries no figures at all; the measured ones stay further down, each with its source named. * fix(test): give the pg-mem mirror entity the relation the join needs The suite was written for a query shape that took no join, so the mirror entity carried only the tradingRuleId column. Restoring the single-statement aggregate brought back innerJoin on a relation path, which TypeORM resolves through entity metadata — all three tests aborted with 'Relation with property path tradingRule in entity was not found'. Foreign key creation stays off so the deliberately orphaned fixture remains insertable. * fix(test): narrow the findBy spy argument instead of casting to a made-up shape The cast asserted a hand-written shape onto findBy's argument type, which also allows an array — TypeScript rejected it as insufficiently overlapping. The argument is now narrowed: an array condition throws, then instanceof FindOperator establishes the real TypeORM type. The assertion itself is unchanged, and every unexpected shape fails loudly rather than skipping. * docs(test): say what the operator guard actually checks The comment claimed a different operator would fail loudly, but the guard tests the FindOperator base class — Any, Not and Between all pass it. The guard's real job is rejecting a bare value that would slip past the array check, and the operator choice is not what this test is about.
…endpoint (#4512) * perf(statistic): load only status settings for the status endpoint GET /v1/statistic/status is polled about six times a minute and returns 32 bytes. It went through SettingService.getAll(), which reads the whole setting table and filtered for the Status suffix in JS afterwards. Measured in production: the table holds 76 rows totalling 1586 kB of values, of which the two Status keys are 14 bytes. A single unrelated entry (ledgerCutoverBoundary.trading_order) accounts for 1534 kB and was transferred and materialised into entities on every poll. Filter in the database instead, per CONTRIBUTING ("filter in SQL not JS"). getAll() stays as it is — the settings endpoint still needs every row. The response is unchanged. * refactor(setting): use find with Like and a deterministic order Review follow-ups: - Prefer the repository method over the query builder, per CONTRIBUTING; every other suffix filter in this codebase uses Like in find options. - Order by id so the projected-name reduce in StatisticService has a defined winner. Two distinct keys can map to the same projected name (StatusfooStatus and fooStatusStatus both become fooStatus); the old find() path did not guarantee an order either, so this pins behaviour that was previously left to the query plan. - Add a direct repository test asserting the exact find options, so a swapped pattern (Status% instead of %Status) or a dropped order fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist