Add pagination and expanded data set, piece, provider, and payment queries - #913
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 5b0d94c | Commit Preview URL Branch Preview URL |
Aug 18 2026, 02:59 PM |
SgtPooki
left a comment
There was a problem hiding this comment.
in general lgtm.. huge change, but everything seems legit.. a few callouts that aren't necessarily blocking but could make some things better.
| }) | ||
|
|
||
| // after | ||
| const page = await getActivePiecesByCursor(client, { |
There was a problem hiding this comment.
the name here feels weird, when we're not passing a cursor. the rename seems unnecessary. can we leave it as getActivePieces ?
There was a problem hiding this comment.
I’d prefer to keep this name. Synapse Core actions intentionally match contract function names, and the underlying endpoint is getActivePiecesByCursor. It also avoids conflating this endpoint with the older offset-based getActivePieces contract method.
|
|
||
| `findPieceIdsByCid`, `getPieces`, and `getPiecesWithMetadata` also return pages and accept `cursor` rather than `startPieceId` or `offset` at the action level. | ||
|
|
||
| Raw `*Call` helpers remain ABI-oriented: provide their required contract-facing `offset` or `startPieceId` and `limit` fields explicitly when constructing multicalls. |
There was a problem hiding this comment.
and these raw call methods are just in synapse-core, yes? I think it would be nice to normalize synapse-sdk being the "polished" surface and synapse-core being the lego bricks.
There was a problem hiding this comment.
Yes, these raw ABI-oriented helpers are part of @filoz/synapse-core. The higher-level SDK remains the polished surface and either exposes normalized pages or fully traversed arrays depending on the method. I’ll make that package boundary explicit in the migration guide.
| } | ||
| ``` | ||
|
|
||
| Use `paginate()` when you want to traverse every page. It follows `nextCursor`, yields individual items, and rejects a repeated or non-advancing cursor. |
There was a problem hiding this comment.
is it possible to break out of a paginate inner loop?
There was a problem hiding this comment.
Yes. paginate() is an async generator, so a normal break exits the loop and closes the generator; it won’t fetch another page.
| const hasMore = BigInt(data.length) > limit | ||
| return { | ||
| items: Array.from(hasMore ? data.slice(0, -1) : data), | ||
| ...(hasMore ? { nextCursor: cursor + limit } : {}), | ||
| } |
There was a problem hiding this comment.
this is duplicated in three places, and findPieceIdsByCid, getApprovedProviderIds, and getProvidersByProductType are missing limit+1 boundary testing, which means any individual implementation can drift.
Can we pull this out into a shared helper and then test that shared helper so we are sure each implementation is doing what it's supposed to?
| const [pdpProviders, approvedProviders] = await Promise.all([ | ||
| Array.fromAsync( | ||
| paginate(({ cursor }) => | ||
| getPDPProviders(client, { onlyActive: true, cursor, contractAddress: options.contractAddress }) | ||
| ) | ||
| ), | ||
| Array.fromAsync(paginate(({ cursor }) => getApprovedProviderIds(client, { cursor }))), | ||
| ]) | ||
| return pdpProviders.filter((provider) => approvedProviders.includes(provider.id)) |
There was a problem hiding this comment.
suggestion (non-blocking): getApprovedPDPProviders is only exercised indirectly, via fetch-provider-selection-input.test.ts, and only against single-page responses.. the basic preset never returns limit+1 items or hasMore: true, so paginate()'s multi-page path never runs against a real action anywhere in synapse-core (pagination.test.ts uses an in-memory stub).
The old get-client-data-sets.test.ts covered real multi-call traversal (150 items across 100+50 RPC calls) and was deleted with the old API, so multi-page coverage is net lower after this PR.
Can we add a direct test here with a mock returning 2+ pages? That covers getApprovedPDPProviders itself (including the approved-ids intersection) and restores the traversal coverage in one shot.
| ], | ||
| }, | ||
| ], | ||
| hasMore: false, |
There was a problem hiding this comment.
this getProvidersByProductType mock ignores offset/limit and hardcodes hasMore:false.. we could extend this so we can write tests to help catch any cursor handling regression
| limit: 100n, | ||
| }) | ||
|
|
||
| for await (const piece of paginate(({ cursor }) => |
There was a problem hiding this comment.
| for await (const piece of paginate(({ cursor }) => | |
| // iterate | |
| for await (const piece of paginate(({ cursor }) => |
currently reads like it's part of // after
| import { readPdpDataSetInfo } from './get-pdp-data-set.ts' | ||
| import type { PdpDataSet } from './types.ts' | ||
| const ENRICHMENT_BATCH_SIZE = 20 | ||
| const DATA_SET_CALL_COUNT = 4 |
There was a problem hiding this comment.
I think this one deserves a comment, it's going to be very easy to get this out of syncwith the number of calls
rvagg
left a comment
There was a problem hiding this comment.
well I don't hate it, I think that's what you wanted to hear?
needs a ! in a commit somewhere to bump major I guess
also utils/sp-tool.js will be broken after this, may as well fix it now, also core-concepts/storage-providers.mdx could do with a look, it has getApprovedProviderIds calls
2a7f325 to
5b0d94c
Compare
Summary
Pagination examples
Read one bounded page and pass the returned cursor back unchanged:
Iterate over every item with the shared
paginate()generator:Accumulate all items when a complete array is required:
Cursors are opaque continuation values. Each action uses a bounded default when
limitis omitted, and rejectslimit: 0n.Testing