Do not return duplicate token ids from the api server

[oliv3rdrt]

The /token endpoint was returning the same token id several times (#1982).

The cause is that ml.fungible_token is keyed on (token_id, block_height), so a token that has been updated has one row for every height it changed at. The query behind the endpoint selected token_id without deduplicating, so those tokens came back once per stored version. The count_tokens CTE had the same problem, as it counted rows rather than distinct tokens, which also skewed the offset used for the nft part of the query.

The fix selects distinct ids and counts distinct token ids, in both get_token_ids and get_token_ids_by_ticker.

I also extended the storage test suite to store a token again at a later height and assert that the returned ids contain no duplicates. The existing test only ever stored each token at a single height, which is why this was not caught.

On verification: I ran the in-memory suite, which passes, though the in-memory backend never had this problem since it is keyed by token id. The postgres suite needs podman, which I could not run locally, so the postgres side of this is verified by reading the schema rather than by running it. Worth a second pair of eyes on the SQL for that reason.

While writing the test I noticed the in-memory backend asserts on storing the same nft id twice ("multiple nft issuances with same token_id"), so nft issuance looks like a one time event. I kept DISTINCT on the nft part anyway since the table permits multiple heights, but happy to drop it if that is unnecessary.

[erubboli]

The DISTINCT/count(DISTINCT) change is directionally correct (DISTINCT applies before ORDER BY/OFFSET/LIMIT in each branch), but there are two blocking issues the new test fails on the postgres backend, and the seam arithmetic now over-fetches in more cases plus test-coverage gaps. Details inline. Two items with no diff line to anchor on:

• Pre-existing, on the touched functions' first lines: offset as i64 (queries.rs:2525, 2568) wraps a client-controlled u64 >= 2^63 to a negative bigint → Postgres rejects with OFFSET must not be negative → HTTP 500. get_offset_and_items only caps items, not offset. A try_into() with an empty-page fallback (or an offset cap in the handler) would fix both endpoints.
• Changelog: api-server/CHANGELOG.md [Unreleased] is empty, but this changes client-visible behavior of /v2/token and /v2/token/ticker/{ticker} (updated tokens no longer appear once per stored version, page composition shifts) — the kind of change earlier entries document for these same endpoints.

[erubboli]

Blocking: the NFT branch's LIMIT now over-fetches in more cases. The formula CASE WHEN ($1 - count >= -$2) THEN ($2 + $1 - count) ELSE 0 END has no upper cap, so any offset greater than the fungible count returns more than items rows, with overlapping pages and switching the CTE to count(DISTINCT token_id) shrinks count, strictly widening the offsets that trigger it.

Example: 3 tokens each updated 5 times (15 rows, 3 distinct) plus 1000 NFTs. ?offset=10&items=10 returned exactly 10 entries pre-PR (count=15 → NFT LIMIT 5) but 17 post-PR (count=3 → NFT OFFSET 7 LIMIT 17). With 0 matching tokens and many NFTs (the common case for ticker searches that only match NFTs), ?offset=500000&items=10 returns ~500k ids the handler does no clamping, so MAX_NUM_ITEMS=100 is silently bypassed.

Minimal fix (here and in get_token_ids_by_ticker):

LIMIT LEAST($2, GREATEST($2 + $1 - (SELECT * FROM count_tokens), 0))

or see the outer-pagination suggestion below, which removes the arithmetic entirely.

[oliv3rdrt]

Confirmed, I get the same numbers on your example: 10 rows before, 17 after. The arithmetic was already wrong, my change just widens when it bites. Taking the outer pagination version instead, which removes it entirely.

[erubboli]

Suggestion (subsumes the LIMIT bug and the count-CTE cost): wrap both deduplicated id sets in one outer ORDER BY/OFFSET/LIMIT a pattern this file already uses (get_latest_blocktimestamps, get_best_block):

SELECT id FROM (
    (SELECT DISTINCT 0 AS grp, token_id AS id FROM ml.fungible_token)
    UNION ALL
    (SELECT DISTINCT 1, nft_id FROM ml.nft_issuance)
) t ORDER BY grp, id OFFSET $1 LIMIT $2

No seam arithmetic to get wrong, at most items rows guaranteed, tokens-first order preserved, and it adds the outer ORDER BY that UNION ALL alone doesn't guarantee. It also drops the count_tokens CTE, which is a per-request O(all historical rows) sort on an unauthenticated endpoint (count(DISTINCT) disables parallel aggregation and Postgres has no loose index scan) at 100k tokens × 20 updates, every page-1 request sorts 2M values just to compute the seam. If the CTE stays, count(*) FROM (SELECT token_id FROM ml.fungible_token GROUP BY token_id) at least keeps hash-agg/parallelism.

Longer-term: this table now has a fourth idiom for "versioned rows, want one per id" (ORDER BY block_height DESC LIMIT 1, MAX(block_height), ROW_NUMBER, and now query-time DISTINCT). A latest_fungible_token cache table like the two existing latest_*_cache tables would make listing, counting, and ticker search O(page) and structurally duplicate-free.

[oliv3rdrt]

Taking this. No seam to get wrong, bounded rows, and it drops the per request count(DISTINCT). The cache table sounds right but feels like separate work, happy to do it here if you'd prefer.

[erubboli]

Blocking: this panics on the postgres backend the only backend the SQL fix applies to. A second set_fungible_token_issuance for the same token id does a plain INSERT into ml.coin_or_token_decimals (PK on coin_or_token_id alone, no ON CONFLICT), so the containerized postgres suite (ML_CONTAINERIZED_TESTS=1 in CI) dies with duplicate key value violates unique constraint at this .unwrap(), before reaching the new assertions. It passes in-memory only because that impl upserts.

Use set_fungible_token_data for the second version instead it has ON CONFLICT, doesn't touch the decimals table, and is the path real updates actually take (scanner-lib/src/blockchain_state/mod.rs), so the fixture then matches how duplicate rows arise in production.

[oliv3rdrt]

Confirmed. coin_or_token_decimals has its PK on coin_or_token_id alone and no ON CONFLICT, so it fails before reaching the assertions. This was the gap I flagged in the description, I couldn't run the containerised suite locally. Switching to set_fungible_token_data.

[erubboli]

The assertions are vacuous and only cover half the fix. ids.len() == unique_ids.len() is satisfied by an empty or truncated result e.g. reverting only the CTE to count(token_id) (keeping SELECT DISTINCT) makes one NFT unreachable on any page, yet both assertions still pass.

• The expected set (all_ids) is already in scope: assert set-equality against it (the idiom used ~70 lines above), which subsumes the no-duplicates check.
• Add a seam-crossing page after the duplicate insert (e.g. get_token_ids(3, 3)) that's the only way to exercise the count(DISTINCT) change.
• The DISTINCT nft_id half has zero coverage: no test creates a second ml.nft_issuance row, though update_nft_owner inserts one per owner change, so NFT duplicates are genuinely reachable (it's not dead code). Needs a postgres-suite case going through the owner-update path, since the in-memory backend asserts against multiple issuances per id.

[oliv3rdrt]

Agreed, the length check passes on an empty result. Switching to set equality against all_ids plus a page across the seam. I also got the NFT part wrong in the description. update_nft_owner inserts a row per owner change, so duplicates are reachable. Adding coverage through that path.

[oliv3rdrt]

Pushed, thanks for the review.

Took your outer pagination version. Set postgres up locally with your scenario and got your numbers exactly: 10 rows before, 17 with my first attempt, 10 now. Full scan returns 1003 of 1003 distinct and pages no longer overlap.

Test now updates through set_fungible_token_data, changes an nft owner to cover DISTINCT nft_id, asserts set equality against all_ids, and pages across the boundary. You were right about the nfts.

Changelog added.

Offset fixed with try_from. The same cast is in 7 places in that file, so a cap in get_offset_and_items would cover them all. Happy to split it out or widen it.

The containerised suite needs podman and I only have docker, so I checked the SQL directly rather than through the suite.

[erubboli]

tACK 👍

[erubboli]

@oliv3rdrt Merging is blocked
Commits must have verified signatures.

[oliv3rdrt]

@erubboli all signed and verified now. rebased onto master while I was at it.