Non-Custodial Partner Program — aggregated partner statistics endpoints - #4587
Open
joshuakrueger-dfx wants to merge 5 commits into
Open
Non-Custodial Partner Program — aggregated partner statistics endpoints#4587joshuakrueger-dfx wants to merge 5 commits into
joshuakrueger-dfx wants to merge 5 commits into
Conversation
Columns such as `created` are `timestamp without time zone`: the Postgres driver serializes a JS Date in the process-local wall-clock and Postgres drops the offset, so a non-UTC process shifts stored values and every day bucket derived from them. Pins `TZ=UTC` in the image and checks the process timezone at boot. The check warns rather than aborts — it must not turn a timezone misconfiguration into a failed rollout — and covers the winter/summer trap by testing the offset year-round, not just at the current date. Boot failures now surface as an explicit "Bootstrap failed" log with exit 1 instead of an unhandled rejection.
`GET /v1/statistic/partner` and `GET /v1/statistic/partner/timeline`, scoped to the wallet in the company JWT. There is deliberately no `walletId` parameter, so a partner cannot express a request for someone else's data, and the lookup fails closed when the JWT carries no wallet. Both routes return aggregates only: volume and transaction counts by direction, active and new users, breakdowns by asset, fiat currency, blockchain and payment method, plus the referral position in EUR. Nothing in the implementation is partner-specific — any wallet gets its own numbers through the same route. Aggregates alone are not anonymous, so suppression blocks the whole group when any direction falls below the threshold (per-cell nulling would let `total - visible` recover the hidden member), applies the timeline threshold per direction rather than to the bucket sum, binds the threshold to distinct users as well as transactions, and snaps periods to UTC day boundaries with a minimum span of one day so two windows differing by hours cannot isolate a single transaction. A dedicated guard budgets queries by wallet id — the shared rate-limit guard keys by IP prefix and bypasses known addresses, which puts no bound on repeated scrapes from one partner JWT. Query parameters are rejected rather than coerced when they arrive as arrays, non-existent calendar days are refused, and query concurrency is bounded.
joshuakrueger-dfx
marked this pull request as ready for review
August 1, 2026 22:24
joshuakrueger-dfx
requested review from
TaprootFreak and
davidleomay
as code owners
August 1, 2026 22:24
The endpoints exist so a partner can answer "how is my integration doing" without holding records about individual customers. That property comes from what the payload contains — sums, counts and named breakdown rows, no user id, address, name, email, IBAN or transaction id, and no user row ever loaded into the process. It does not come from withholding thin days. The threshold did withhold them, and it protected nothing: the same CLIENT_COMPANY credential already reaches GET /v2/kyc/client/payments, which returns the underlying transactions in full, including customer IBANs, up to 1000 per call and without a consent filter. A day with four transactions was hidden from the partner in the aggregate while the same partner could read those four transactions individually next door. So the layer goes, and with it the fields that only existed to describe it — meta.suppressionThreshold, meta.suppressedCount, bucket.suppressed — and the nullability that meant "withheld" rather than "no data". `averageTransactionVolume` stays nullable, where null still means there were no transactions. The guards that matter are untouched and still fail loudly: removing the wallet scope from one query fails 2 of 51, removing the AML filter 1 of 51, widening a period bound 3 of 51.
The real-Postgres integration spec still expected `activeUsers` to be null for a cohort of three, which was the old threshold behaviour. It now asserts the set count itself — three, and explicitly not six, which is what a UNION ALL would produce. That is the property the test is named after; the withholding was incidental to it. This only fails against a real database, so it ran in CI and not in the local gate.
Two published field descriptions still described the removed threshold: lifetime volume promised "null fields when tradingUsers < k", and registeredUsers was labelled "always visible", which only meant anything in contrast to fields that were not. Neither is true any more, and both are read by partners in the API docs rather than in the code. Descriptions only; no field, type or behaviour changes.
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
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.
Why
Not symptom-driven: no incident triggered this — wallet partners integrating DFX have no way to
see how their integration performs, and the only route that answers today hands out per-transaction
detail.
Scale: Cake alone has 126,988 users in production and is the first of several wallet partners;
/v2/kyc/client/paymentsserves the sameCLIENT_COMPANYrole up to 1000 individual transactionsper call, including customer IBANs, with free date filters.
Smaller fix considered: point partners at that existing endpoint and let them aggregate —
insufficient because it makes every partner store per-transaction records about their customers,
including names and IBANs, purely to compute counts and volumes they can be handed directly.
What
GET /v1/statistic/partnerandGET /v1/statistic/partner/timeline, scoped to the wallet in thecompany JWT. There is deliberately no
walletIdparameter — a partner cannot express a requestfor someone else's data. Both return aggregates only: volume and transaction counts by direction,
active and new users, breakdowns by asset, fiat currency, blockchain and payment method, plus the
referral position in EUR.
Generic by design: there is no partner-specific code anywhere. Any wallet gets its own numbers
through the same route.
The point: statistics without personal data
A partner should be able to answer "how is my integration doing" without holding records about
individual customers. That is the whole purpose of these two routes.
What leaves the process, field by field: sums, counts, distinct-user totals, and named breakdown
rows. What does not leave it: no user id, no address, no name, no email, no IBAN, no transaction
id, no referral code — nothing that identifies a person. The aggregation happens in SQL; no user row
is ever loaded into the process.
The endpoints do not withhold thin days. A day with a single transaction reports that single
transaction, like any other day. Hiding it would deny the partner numbers they legitimately need
while protecting nothing: the same credential already reaches
/v2/kyc/client/payments, whichreturns the underlying transactions in full, including IBANs and without a consent filter. A
threshold on the aggregates would have guarded a door whose side entrance stands open.
Correctness
referral.creditOpenusedpartnerRefCredit − paidRefCredit.paidRefCreditcovers both pots;the three sibling sites use
refCredit + partnerRefCredit − paidRefCredit. The old formulaunderstated the figure and could go negative.
bucket by a day. Truncation is now bound to UTC in SQL and the bucket keys follow.
map.seton bucket keys silently dropped a bucket on collision; it accumulates now.2024-06-31are rejected rather than silently rolled into the next month, and a query parameterarriving as an array is rejected instead of being coerced.
null, and a missing wallet rowthrows instead of reporting zero.
Scope and load
single-row lookup in SQL already exists (
kyc.service.tsfilterswallet: { id: walletId }); whatis new is doing it inside aggregate queries over the transaction tables. The partner endpoints that
read many rows load the wallet with its users and pass the ids on (
kyc-client.service.ts,transaction.service.tsbatching at 100). Measured against production, that does not carry anaggregate endpoint: Cake has 126,988 users, so the existing pattern would mean 1,270 sequential
batches per query and roughly 22,800 round trips per summary call, plus every user row in memory.
The SQL scope keeps it at 18 queries and loads no user rows. Every user-touching query carries the
clause, and a test fails when any one of the six loses it — removal, falsification and widening
alike.
buy_crypto(130,180 rows) andbuy_fiat(68,862 rows) — queried against production on2026-07-31, not estimated. At that size a filtered aggregate is milliseconds; an index for a
dashboard call that runs twice an hour would be cost without a case. If either table grows by an
order of magnitude, the question is worth reopening.
every SQL execution passes through, so nested fan-outs share one budget of 4. The previous
per-call-site cap did not compose: four breakdown tasks each opened their own pool of workers and
a single request reached 11 simultaneous queries against a default pool of 10.
RateLimitGuardkeys by IP prefix andbypasses known and Azure addresses, so it puts no bound on repeated scrapes from a single partner
JWT. The new guard keys by the wallet id in the JWT, has no bypass list, and fails closed if the
wallet id is absent: 120 requests per hour per wallet on each route. It counts in memory per
instance, and like every guard in this repo it is inactive unless
REQUEST_LIMIT_CHECK=true.The process runs in UTC, and now says so
Columns such as
createdaretimestamp without time zone. The Postgres driver serializes JS Datevalues in process-local wall time and Postgres drops the offset, so a non-UTC process stores shifted
values and the day buckets drift. Nothing pinned the timezone: it held only because
node:20-alpinehas no
/etc/localtimeand therefore defaults to UTC.ENV TZ=UTCin the runtime image makes thatexplicit, and a boot check logs the detected zone. It only warns — the deploy start command lives
outside this repo and may set
TZitself, so a hard abort would risk the deploy for a premisenothing here can verify.
Verification
dda8236e4: 370 suites (361 passed, 9 skipped), 7038 tests (6831 passed,207 skipped), 0 failures.
lintempty,format:checkclean,tsc --noEmitclean,buildclean, coverage gate green. Measured off-CI with the repo's own scripts. The head commit on top of
it changes two Swagger description strings only, and is covered by CI rather than by that run.
America/Los_Angeles.
getStatistics,getTimeline,aggregateByDirection— against real rows. It previously re-implemented the querybuilders by hand, so a broken join would have left it green while it claimed "real Postgres". It
requires a UTC process timezone and skips loudly otherwise, rather than failing on a shifted bound.
It needs a database, so it is the one part of this branch that only runs in CI — and it is what
caught a stale expectation the mock-backed suite could not see: 1 failed / 4 passed before,
5 passed after.
afterwards. Several assertions turned out to be vacuously true — a filtered list checked with
everypasses as soon as the condition is missing — so the wallet scope, the AML filter, theperiod bounds and the group-by lists now pin expected counts against anchored patterns. Removing,
widening or dropping any of those clauses fails a test.
What the mock-backed specs still do not pin, found by re-running the catalogue against this
revision and worth a reviewer's eye:
/^tx\.amlCheck = :check$/but never asserts the parameter, so swappingPassforPending—which would report unchecked traffic as passed volume — keeps all 51 unit tests green. Only the
database-backed spec catches it (4 of 5 red).
PARTNER_STATISTIC_QUERY_CONCURRENCYandMAX_PERIOD_DAYSare asserted against themselves(
expect(observed).toBeLessThanOrEqual(THE_CONSTANT)), so the values are tautological in bothdirections: raising the concurrency budget to 20 or the span cap to 36,600 days stays green.
AT TIME ZONE 'UTC'is pinned only as a string in the group-by comparison. The integration specnamed for it refuses to run unless the process is already UTC, where both SQL forms behave
identically — so the semantic property has no test.
These are gaps in the tests, not in the code; the clauses themselves are present and correct. They
are listed here rather than quietly fixed because the fix belongs in a separate, reviewable change.
For reviewers
TZwas pinned nowhere in the repo; this PR sets it and logs the detected zone. Two existingmodules build bucket keys from local date parts on the assumption that "the app and DB both run
UTC"; they are worth a separate look.