feat(synapse-core): support paginated client dataset reads from FWSS#698
feat(synapse-core): support paginated client dataset reads from FWSS#698Chaitu-Tatipamula wants to merge 2 commits intoFilOzone:masterfrom
Conversation
|
We can ignore the "add issues and PRs" CI check. That will get fixed in #700 |
2f5b70a to
956e6da
Compare
956e6da to
d14c1a1
Compare
There was a problem hiding this comment.
Pull request overview
Adds synapse-core support for FWSS’s new paginated client dataset read APIs by introducing new warm-storage actions (IDs + length) and extending the existing getClientDataSets action to optionally use the paginated overload, along with ABI/mocks/test updates.
Changes:
- Extend
getClientDataSetsto accept optionaloffset/limitand select the correct contract overload. - Add new actions
getClientDataSetsLengthandgetClientDataSetsIdsfor count + paginated ID enumeration. - Regenerate ABI and update JSON-RPC mocks/presets and tests to cover the new actions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/synapse-core/wagmi.config.ts | Bumps FWSS ABI source ref to pick up the new contract overloads/helpers. |
| packages/synapse-core/test/get-client-data-sets.test.ts | Adds call-shape tests for the new paginated args selection. |
| packages/synapse-core/test/get-client-data-sets-length.test.ts | New tests for the dataset-count action (call + mocked RPC). |
| packages/synapse-core/test/get-client-data-sets-ids.test.ts | New tests for the paginated dataset-IDs action (call + mocked RPC). |
| packages/synapse-core/src/warm-storage/index.ts | Exports the new warm-storage actions. |
| packages/synapse-core/src/warm-storage/get-client-data-sets.ts | Adds optional offset/limit and overload selection logic for getClientDataSets. |
| packages/synapse-core/src/warm-storage/get-client-data-sets-length.ts | Introduces getClientDataSetsLength action and call builder. |
| packages/synapse-core/src/warm-storage/get-client-data-sets-ids.ts | Introduces getClientDataSetsIds action and call builder for clientDataSets. |
| packages/synapse-core/src/mocks/jsonrpc/warm-storage.ts | Adds mock handler support for getClientDataSetsLength. |
| packages/synapse-core/src/mocks/jsonrpc/index.ts | Extends mock presets with getClientDataSetsLength return value. |
| packages/synapse-core/src/abis/generated.ts | Regenerates ABI including new overloads/helpers needed by the actions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Starting index (0-based). Use 0 to start from beginning. Defaults to fetching all (unpaginated). */ | ||
| offset?: bigint | ||
| /** Maximum number of data sets to return. Use 0 to get all remaining. Defaults to fetching all (unpaginated). */ |
There was a problem hiding this comment.
The JSDoc for offset/limit says they default to “fetching all (unpaginated)”, but the implementation uses the unpaginated overload only when both params are omitted; providing either param (even 0n) switches to the paginated overload with 0n defaults. Please clarify the docs to match the actual behavior (e.g., “If omitted, uses legacy unpaginated overload; if provided, uses paginated overload with default 0n values”).
| /** Starting index (0-based). Use 0 to start from beginning. Defaults to fetching all (unpaginated). */ | |
| offset?: bigint | |
| /** Maximum number of data sets to return. Use 0 to get all remaining. Defaults to fetching all (unpaginated). */ | |
| /** | |
| * Starting index (0-based). Use 0 to start from the beginning. | |
| * | |
| * If both `offset` and `limit` are omitted, the legacy unpaginated | |
| * overload is used and all data sets are fetched. | |
| * | |
| * If either `offset` or `limit` is provided, the paginated overload is | |
| * used and any omitted `offset` defaults to `0n`. | |
| */ | |
| offset?: bigint | |
| /** | |
| * Maximum number of data sets to return. Use 0 to get all remaining. | |
| * | |
| * If both `offset` and `limit` are omitted, the legacy unpaginated | |
| * overload is used and all data sets are fetched. | |
| * | |
| * If either `offset` or `limit` is provided, the paginated overload is | |
| * used and any omitted `limit` defaults to `0n`. | |
| */ |
There was a problem hiding this comment.
yeah copilot is right, i just dont like the suggestion with words like legacy and overload.
| const base = { | ||
| abi: chain.contracts.fwssView.abi, | ||
| address: options.contractAddress ?? chain.contracts.fwssView.address, | ||
| functionName: 'getClientDataSets', | ||
| } as const | ||
|
|
||
| if (options.offset != null || options.limit != null) { | ||
| return { | ||
| ...base, | ||
| args: [options.address, options.offset ?? 0n, options.limit ?? 0n], | ||
| } as getClientDataSetsCall.OutputType | ||
| } |
There was a problem hiding this comment.
In the paginated branch, the return value is forced with as getClientDataSetsCall.OutputType, which can mask ABI/args mismatches (especially with overloads). Prefer returning an object that satisfies getClientDataSetsCall.OutputType (or otherwise typing base/args so both branches are checked) to keep compile-time verification.
There was a problem hiding this comment.
can args be just [options.address, options.offset ?? 0n, options.limit ?? 0n] ?
| it('should use paginated args when offset is provided', () => { | ||
| const call = getClientDataSetsCall({ | ||
| chain: calibration, | ||
| address: ADDRESSES.client1, | ||
| offset: 10n, | ||
| limit: 50n, | ||
| }) | ||
|
|
||
| assert.equal(call.functionName, 'getClientDataSets') | ||
| assert.deepEqual(call.args, [ADDRESSES.client1, 10n, 50n]) | ||
| }) | ||
|
|
||
| it('should use unpaginated args when no offset/limit provided', () => { | ||
| const call = getClientDataSetsCall({ | ||
| chain: calibration, | ||
| address: ADDRESSES.client1, | ||
| }) | ||
|
|
||
| assert.equal(call.functionName, 'getClientDataSets') | ||
| assert.deepEqual(call.args, [ADDRESSES.client1]) | ||
| }) |
There was a problem hiding this comment.
There’s mocked-RPC coverage for the legacy unpaginated getClientDataSets call, but no test that exercises the new offset/limit path end-to-end against the MSW JSON-RPC handler. Please add a mocked RPC test that calls getClientDataSets(client, { address, offset, limit }) to ensure the ABI overload resolution + mock handler decoding/encoding work for the paginated overload.
|
|
||
| // GIT_REF can be one of: '<branch name>', '<commit>' or 'tags/<tag>' | ||
| const GIT_REF = '44ef7165f7db93e5491a27ce14460143ff9a7d13' | ||
| const GIT_REF = 'd08214e1b3d200e0bc80f0d4f2e5ea3e1e4d603e' |
There was a problem hiding this comment.
can you change it to 'ed85348ebad54196b5bfefc5cb0dbe7e8bfd6f7c' should have what you need here
| const base = { | ||
| abi: chain.contracts.fwssView.abi, | ||
| address: options.contractAddress ?? chain.contracts.fwssView.address, | ||
| functionName: 'getClientDataSets', | ||
| } as const | ||
|
|
||
| if (options.offset != null || options.limit != null) { | ||
| return { | ||
| ...base, | ||
| args: [options.address, options.offset ?? 0n, options.limit ?? 0n], | ||
| } as getClientDataSetsCall.OutputType | ||
| } |
There was a problem hiding this comment.
can args be just [options.address, options.offset ?? 0n, options.limit ?? 0n] ?
| /** Starting index (0-based). Use 0 to start from beginning. Defaults to fetching all (unpaginated). */ | ||
| offset?: bigint | ||
| /** Maximum number of data sets to return. Use 0 to get all remaining. Defaults to fetching all (unpaginated). */ |
There was a problem hiding this comment.
yeah copilot is right, i just dont like the suggestion with words like legacy and overload.
…s, update GIT_REF, add paginated RPC test
d112a7e to
ed66393
Compare
Summary
Adds synapse-core actions for the paginated client dataset query overloads introduced in filecoin-services#444, addressing filecoin-pin#362. This is in support of #691
New actions
getClientDataSetsLength- returns total count of datasets for a client (getClientDataSetsLength(address))getClientDataSetsIds- returns paginated dataset IDs (clientDataSets(address, offset, limit))Updated actions
getClientDataSets- added optionaloffset/limitparams. defaults to unpaginated.Other changes
getClientDataSetsLengthall new pagination params default to 0n (fetch all), so no breaking changes are introduced.