feat: add new getTwapOrders method - #928
Open
kernelwhisperer wants to merge 8 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a public programmatic-orders module for paginated TWAP parent and part-order retrieval, with Valibot validation, GraphQL response decoding, normalized API errors, exported TWAP types, tests, and README documentation. ChangesProgrammatic TWAP history
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProgrammaticOrderApi
participant GraphqlClient
participant GraphQLAPI
Caller->>ProgrammaticOrderApi: getTwapOrders(params, options)
ProgrammaticOrderApi->>ProgrammaticOrderApi: validate inputs and apply pagination defaults
ProgrammaticOrderApi->>GraphqlClient: queryPage(TWAP_ORDERS_QUERY)
GraphqlClient->>GraphQLAPI: POST query with chainId, resolvedOwner, limit, offset, direction
GraphQLAPI-->>GraphqlClient: TWAP parent page
GraphqlClient-->>ProgrammaticOrderApi: decoded items and totalCount
ProgrammaticOrderApi-->>Caller: QueryPage<TwapOrder>
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
📦 GitHub Packages PublishedLast updated: Jul 27, 2026, 03:16:03 PM UTC The following packages have been published to GitHub Packages with pre-release version
InstallationThese packages require authentication to install from GitHub Packages. First, create a # Create .npmrc file in your project root
echo "@cowprotocol:registry=https://npm.pkg.github.com" > .npmrc
echo "//npm.pkg.github.com/:_authToken=YOUR_GITHUB_TOKEN" >> .npmrcTo get your GitHub token:
Then install any of the packages above, either by exact version (i.e. # Yarn
yarn add npm:@cowprotocol/cow-sdk@pr-928
# pnpm
pnpm install npm:@cowprotocol/cow-sdk@pr-928
# NPM
npm install npm:@cowprotocol/cow-sdk@pr-928Update to the latest version (only if you used the tag)Every commit will publish a new package. To upgrade to the latest version, run: # Yarn
yarn upgrade @cowprotocol/cow-sdk
# pnpm
pnpm update @cowprotocol/cow-sdk
# NPM
npm update @cowprotocol/cow-sdkView PackagesYou can view the published packages at: https://github.com/cowprotocol/cow-sdk/packages |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
kernelwhisperer
marked this pull request as ready for review
July 22, 2026 08:10
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/composable/src/programmaticOrders/graphql.ts`:
- Around line 37-50: Update GraphqlClient.query to enforce a request timeout by
creating or accepting an AbortSignal and passing it to fetch, including the
signal through pagination and getTwapOrders call paths. Catch aborts caused by
timeout and map them to GraphqlClientError with clear timeout context, while
preserving existing handling for other request failures; add a test that
verifies a stalled fetch rejects within the configured timeout.
In `@packages/composable/tests/ProgrammaticOrderApi.spec.ts`:
- Around line 9-28: Make the ProgrammaticOrderApi tests hermetic by mocking
deterministic GraphQL responses for both EOA and Safe getTwapOrders cases
instead of using the live default API. Update snapshots to reflect the fixed
fixtures, and move any live API coverage into a separately opt-in smoke test so
the default suite is independent of network access and mutable order history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1b9982a-ba61-41e8-9d23-3a58e81bdd7c
⛔ Files ignored due to path filters (2)
packages/composable/tests/__snapshots__/ProgrammaticOrderApi.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/composable/README.mdpackages/composable/package.jsonpackages/composable/src/index.tspackages/composable/src/programmaticOrders/ProgrammaticOrderApi.tspackages/composable/src/programmaticOrders/graphql.tspackages/composable/src/programmaticOrders/index.tspackages/composable/src/programmaticOrders/schemas.tspackages/composable/src/programmaticOrders/twap-queries.tspackages/composable/src/programmaticOrders/twap-schemas.tspackages/composable/src/programmaticOrders/twap-types.tspackages/composable/src/programmaticOrders/types.tspackages/composable/tests/GraphqlClient.spec.tspackages/composable/tests/ProgrammaticOrderApi.spec.ts
Comment on lines
+37
to
+50
| async query(query: string, variables: Record<string, unknown>): Promise<unknown> { | ||
| let response: Response | ||
|
|
||
| try { | ||
| response = await fetch(this.url, { | ||
| method: 'POST', | ||
| headers: { | ||
| Accept: 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ query, variables }), | ||
| }) | ||
| } catch (cause) { | ||
| throw new GraphqlClientError('GraphQL request failed', { cause }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols first.
ast-grep outline packages/composable/src/programmaticOrders/graphql.ts --view expanded
# Inspect the file with line numbers.
cat -n packages/composable/src/programmaticOrders/graphql.ts | sed -n '1,220p'
# Find callers and any timeout/AbortSignal patterns in the repo.
rg -n "AbortSignal|AbortController|timeout\(|fetch\(" packages/composable packages -g '!**/dist/**' -g '!**/build/**'
# Check package/runtime targets.
git ls-files 'packages/**/package.json' 'package.json' 'tsconfig*.json' 'packages/**/tsconfig*.json' | xargs -r rg -n '"(engines|target|lib|types|type)":|"typescript"'Repository: cowprotocol/cow-sdk
Length of output: 11871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the root and composable package/runtime targets.
cat -n package.json | sed -n '1,120p'
printf '\n--- composable package ---\n'
cat -n packages/composable/package.json | sed -n '1,120p'
printf '\n--- composable tsconfig ---\n'
cat -n packages/composable/tsconfig.json | sed -n '1,120p'
# Look for any request context/abort signal plumbing in composable.
rg -n "signal:|AbortSignal|AbortController|timeout\(|contextOverride|timeoutMs|request.*timeout" packages/composable packages/order-book packages/contracts-ts -g '!**/dist/**' -g '!**/build/**'Repository: cowprotocol/cow-sdk
Length of output: 11335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find GraphqlClient usage and the public TWAP surface.
rg -n "new GraphqlClient|GraphqlClient\\b|getTwapOrders|paginate\\(" packages/composable -g '!**/dist/**' -g '!**/build/**'
# Inspect the TWAP-related entrypoints.
ast-grep outline packages/composable/src --view expanded
rg -n "TWAP|twap|programmaticOrders|graphql" packages/composable/src -g '!**/dist/**' -g '!**/build/**'
# Inspect the composable package exports for a public API surface that might carry context/options.
cat -n packages/composable/src/index.ts | sed -n '1,220p'Repository: cowprotocol/cow-sdk
Length of output: 29697
Bound the GraphQL request in packages/composable/src/programmaticOrders/graphql.ts:41. A stalled upstream can leave query(), pagination, and getTwapOrders() pending indefinitely; thread an AbortSignal/timeout through GraphqlClient, map expiry to GraphqlClientError, and add a stalled-fetch test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/composable/src/programmaticOrders/graphql.ts` around lines 37 - 50,
Update GraphqlClient.query to enforce a request timeout by creating or accepting
an AbortSignal and passing it to fetch, including the signal through pagination
and getTwapOrders call paths. Catch aborts caused by timeout and map them to
GraphqlClientError with clear timeout context, while preserving existing
handling for other request failures; add a test that verifies a stalled fetch
rejects within the configured timeout.
Comment on lines
+9
to
+28
| it('lists all known EOA TWAPs from the live programmatic orders API', async () => { | ||
| const progress: TwapOrder[] = [] | ||
| const orders = await new ProgrammaticOrderApi().getTwapOrders({ | ||
| resolvedOwner: EOA, | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| onProgress: (order) => progress.push(order), | ||
| }) | ||
|
|
||
| expect(progress).toEqual(orders) | ||
| expect(orders).toMatchSnapshot() | ||
| }, 30_000) | ||
|
|
||
| it('lists all known Safe TWAPs from the live programmatic orders API', async () => { | ||
| const orders = await new ProgrammaticOrderApi().getTwapOrders({ | ||
| resolvedOwner: SAFE, | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| }) | ||
|
|
||
| expect(orders).toMatchSnapshot() | ||
| }, 30_000) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make these tests hermetic.
They call the live default API and snapshot mutable order history, so new orders, executions, or endpoint/network failures can break CI without a code regression. Mock fixed GraphQL responses for the default suite; keep any live smoke test opt-in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/composable/tests/ProgrammaticOrderApi.spec.ts` around lines 9 - 28,
Make the ProgrammaticOrderApi tests hermetic by mocking deterministic GraphQL
responses for both EOA and Safe getTwapOrders cases instead of using the live
default API. Update snapshots to reflect the fixed fixtures, and move any live
API coverage into a separately opt-in smoke test so the default suite is
independent of network access and mutable order history.
Danziger
reviewed
Jul 22, 2026
| "@cowprotocol/sdk-order-signing": "workspace:*", | ||
| "@openzeppelin/merkle-tree": "^1.0.8" | ||
| "@openzeppelin/merkle-tree": "^1.0.8", | ||
| "valibot": "1.4.2" |
Contributor
There was a problem hiding this comment.
Regarding valibot vs zod (4), no preference, but I think it would make sense to use the same one in both projects so that we have a single dependency. Maybe even declare it as a peer dependency here.
Danziger
reviewed
Jul 22, 2026
| readonly name = 'GraphqlClientError' | ||
| } | ||
|
|
||
| export class GraphqlClient { |
Contributor
There was a problem hiding this comment.
How come you need a custom GraphQL client instad of using a lib like Apollo or similar?
Contributor
Author
There was a problem hiding this comment.
Mostly to keep the bundle small and the npm supply chain attack vector small.
This is the only place where we use graphql in the sdk, it felt a bit overkill to pull in Apollo, but I'm not against exploring it.
Danziger
reviewed
Jul 22, 2026
| const parents = await getTwapParents(this.graphql, resolvedOwner, chainId) | ||
|
|
||
| for (const parent of parents) { | ||
| const partOrders = await getTwapPartOrders(this.graphql, chainId, parent.eventId) |
Contributor
There was a problem hiding this comment.
I guess this is here for the initial version, but it will be removed later once the indexer includes this data in parent orders (as discussed in the sync) (?)
Contributor
There was a problem hiding this comment.
In any case, maybe better to use Promise.all? Even though on that case maybe onProgress doesn't make much sense. Can you elaborate on this?
Contributor
Author
There was a problem hiding this comment.
Fixed in 0989f13 (refactor: decouple parts from twap order)
Danziger
reviewed
Jul 22, 2026
| for (const record of records) { | ||
| const { chainId: recordChainId, parentEventId: recordParentEventId, ...partOrder } = record | ||
|
|
||
| if (recordChainId !== chainId || recordParentEventId !== parentEventId) { |
Contributor
There was a problem hiding this comment.
Why do we need this? Also, if you are going to throw, there's no need for push (if the extra chainId and parentEventId are not an issue):
const records = awat client.paginate(...)
if (records.find(...)) throw new Error(...)
return records
Contributor
Author
There was a problem hiding this comment.
Removed in 0989f13 (refactor: decouple parts from twap order)
Danziger
reviewed
Jul 22, 2026
|
|
||
| Each result preserves both its creation `eventId` and ComposableCoW `hash`. `schedule.numberOfParts` is the number of scheduled parts. `partOrders` contains the corresponding part orders, so its length may be smaller. `executedAmounts` sums those part orders. The schedule's `effectiveStartTime` is the creation block timestamp when the on-chain `t0` was zero. Pass `apiUrl` to the constructor to use another programmatic orders API deployment. | ||
|
|
||
| Pass `onProgress` to receive each fully assembled TWAP order after all its available part orders are fetched. The promise returns the complete result. If a later page fails, the promise rejects and previously emitted orders remain provisional. |
Contributor
There was a problem hiding this comment.
Based on this, should we...:
- Implement auto-retry?
- Implement a
getTwapOrder(singular), as it looks like one use case foronProgressmight be to get TWAP orders when looking for a specific one (so we don't care if fetching other ones fail).
Contributor
Author
There was a problem hiding this comment.
I don't think this is applicable anymore in 0989f13 (refactor: decouple parts from twap order). We're doing polling so auto-retry adds little value.
The use case of onProgress was not to get 1 twap order, but to start hydrating the ui with partial results rather thank a bulk update at the end.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/composable/src/programmaticOrders/graphql.ts (1)
42-56: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo timeout/cancellation on the GraphQL
fetchcall.A stalled or slow upstream response leaves
query(),queryPage(), and bothProgrammaticOrderApimethods pending indefinitely. Thread anAbortSignal/timeout throughGraphqlClientand map expiry toGraphqlClientError.🔧 Suggested approach
- async query(query: string, variables: Record<string, unknown>): Promise<unknown> { + async query(query: string, variables: Record<string, unknown>, timeoutMs = 30_000): Promise<unknown> { let response: Response + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), timeoutMs) try { response = await fetch(this.url, { method: 'POST', headers: { Accept: 'application/json', 'Content-Type': 'application/json', }, body: JSON.stringify({ query, variables }), + signal: controller.signal, }) } catch (cause) { - throw new GraphqlClientError('GraphQL request failed', { cause }) + throw new GraphqlClientError( + controller.signal.aborted ? 'GraphQL request timed out' : 'GraphQL request failed', + { cause }, + ) + } finally { + clearTimeout(timeout) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/composable/src/programmaticOrders/graphql.ts` around lines 42 - 56, Update GraphqlClient.query to support request cancellation via an AbortSignal and a bounded timeout, passing the signal into fetch so query(), queryPage(), and both ProgrammaticOrderApi methods cannot remain pending indefinitely. Distinguish timeout/abort failures and map them to GraphqlClientError while preserving the existing handling for other fetch failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/composable/tests/ProgrammaticOrderApi.spec.ts`:
- Around line 172-212: Make the live-network tests in the ProgrammaticOrderApi
test suite hermetic by replacing direct default-API calls with a deterministic
mock or fixture-backed response. Update the EOA and Safe TWAP parent tests and
the part-order test so snapshots and the executed-fee parent selection use
stable controlled data, while preserving assertions for pagination and
part-order behavior.
- Around line 88-131: Update the query assertion in the “requests parent-only
TWAPs by creation” test to detect nested part-order item selection despite the
existing discreteOrders(limit: 1) syntax. Assert against the actual rendered
nested-field pattern, while preserving the response-level assertion that
page.items[0] has no partOrders.
---
Duplicate comments:
In `@packages/composable/src/programmaticOrders/graphql.ts`:
- Around line 42-56: Update GraphqlClient.query to support request cancellation
via an AbortSignal and a bounded timeout, passing the signal into fetch so
query(), queryPage(), and both ProgrammaticOrderApi methods cannot remain
pending indefinitely. Distinguish timeout/abort failures and map them to
GraphqlClientError while preserving the existing handling for other fetch
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2fb917e-7c88-4c37-b44b-4aa80adf0543
⛔ Files ignored due to path filters (1)
packages/composable/tests/__snapshots__/ProgrammaticOrderApi.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
packages/composable/README.mdpackages/composable/src/programmaticOrders/ProgrammaticOrderApi.tspackages/composable/src/programmaticOrders/graphql.tspackages/composable/src/programmaticOrders/index.tspackages/composable/src/programmaticOrders/schemas.tspackages/composable/src/programmaticOrders/twap-queries.tspackages/composable/src/programmaticOrders/twap-schemas.tspackages/composable/src/programmaticOrders/twap-types.tspackages/composable/src/programmaticOrders/types.tspackages/composable/src/programmaticOrders/validation.tspackages/composable/tests/ProgrammaticOrderApi.spec.tspackages/composable/tests/graphql.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/composable/src/programmaticOrders/index.ts
- packages/composable/src/programmaticOrders/twap-schemas.ts
- packages/composable/src/programmaticOrders/schemas.ts
- packages/composable/README.md
Comment on lines
+88
to
+131
| it('requests parent-only TWAPs by creation and preserves the returned order', async () => { | ||
| const fetchMock = jest.spyOn(globalThis, 'fetch').mockResolvedValueOnce( | ||
| new Response( | ||
| JSON.stringify({ | ||
| data: { | ||
| twapOrders: { | ||
| items: [twapParent('newer-event', 200), twapParent('older-event', 100)], | ||
| totalCount: 2, | ||
| }, | ||
| }, | ||
| }), | ||
| { headers: { 'Content-Type': 'application/json' } }, | ||
| ), | ||
| ) | ||
|
|
||
| const page = await new ProgrammaticOrderApi({ apiUrl: 'https://example.com' }).getTwapOrders( | ||
| { | ||
| resolvedOwner: EOA, | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| }, | ||
| { direction: 'asc', limit: 2, offset: 1 }, | ||
| ) | ||
| const request = JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body)) as { | ||
| query: string | ||
| variables: Record<string, unknown> | ||
| } | ||
|
|
||
| expect(request.query).toContain('orderBy: "eventId"') | ||
| expect(request.query).toContain('orderDirection: $direction') | ||
| expect(request.query).not.toContain('discreteOrders {\n') | ||
| expect(request.variables).toEqual({ | ||
| resolvedOwner: EOA.toLowerCase(), | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| offset: 1, | ||
| limit: 2, | ||
| direction: 'asc', | ||
| }) | ||
| expect(page.totalCount).toBe(2) | ||
| expect(page.items.map(({ eventId, createdAt }) => ({ eventId, createdAt }))).toEqual([ | ||
| { eventId: 'newer-event', createdAt: 200 }, | ||
| { eventId: 'older-event', createdAt: 100 }, | ||
| ]) | ||
| expect(page.items[0]).not.toHaveProperty('partOrders') | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tautological assertion doesn't verify the "no nested part-order items" claim.
request.query never contains the literal 'discreteOrders {\n' because the query always renders discreteOrders(limit: 1) {. This assertion passes even if the parent query started selecting nested part-order item fields.
🔧 Suggested fix
- expect(request.query).not.toContain('discreteOrders {\n')
+ expect(request.query).not.toContain('discreteOrders(limit: 1) {\n orderUid')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('requests parent-only TWAPs by creation and preserves the returned order', async () => { | |
| const fetchMock = jest.spyOn(globalThis, 'fetch').mockResolvedValueOnce( | |
| new Response( | |
| JSON.stringify({ | |
| data: { | |
| twapOrders: { | |
| items: [twapParent('newer-event', 200), twapParent('older-event', 100)], | |
| totalCount: 2, | |
| }, | |
| }, | |
| }), | |
| { headers: { 'Content-Type': 'application/json' } }, | |
| ), | |
| ) | |
| const page = await new ProgrammaticOrderApi({ apiUrl: 'https://example.com' }).getTwapOrders( | |
| { | |
| resolvedOwner: EOA, | |
| chainId: SupportedChainId.GNOSIS_CHAIN, | |
| }, | |
| { direction: 'asc', limit: 2, offset: 1 }, | |
| ) | |
| const request = JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body)) as { | |
| query: string | |
| variables: Record<string, unknown> | |
| } | |
| expect(request.query).toContain('orderBy: "eventId"') | |
| expect(request.query).toContain('orderDirection: $direction') | |
| expect(request.query).not.toContain('discreteOrders {\n') | |
| expect(request.variables).toEqual({ | |
| resolvedOwner: EOA.toLowerCase(), | |
| chainId: SupportedChainId.GNOSIS_CHAIN, | |
| offset: 1, | |
| limit: 2, | |
| direction: 'asc', | |
| }) | |
| expect(page.totalCount).toBe(2) | |
| expect(page.items.map(({ eventId, createdAt }) => ({ eventId, createdAt }))).toEqual([ | |
| { eventId: 'newer-event', createdAt: 200 }, | |
| { eventId: 'older-event', createdAt: 100 }, | |
| ]) | |
| expect(page.items[0]).not.toHaveProperty('partOrders') | |
| }) | |
| it('requests parent-only TWAPs by creation and preserves the returned order', async () => { | |
| const fetchMock = jest.spyOn(globalThis, 'fetch').mockResolvedValueOnce( | |
| new Response( | |
| JSON.stringify({ | |
| data: { | |
| twapOrders: { | |
| items: [twapParent('newer-event', 200), twapParent('older-event', 100)], | |
| totalCount: 2, | |
| }, | |
| }, | |
| }), | |
| { headers: { 'Content-Type': 'application/json' } }, | |
| ), | |
| ) | |
| const page = await new ProgrammaticOrderApi({ apiUrl: 'https://example.com' }).getTwapOrders( | |
| { | |
| resolvedOwner: EOA, | |
| chainId: SupportedChainId.GNOSIS_CHAIN, | |
| }, | |
| { direction: 'asc', limit: 2, offset: 1 }, | |
| ) | |
| const request = JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body)) as { | |
| query: string | |
| variables: Record<string, unknown> | |
| } | |
| expect(request.query).toContain('orderBy: "eventId"') | |
| expect(request.query).toContain('orderDirection: $direction') | |
| expect(request.query).not.toContain('discreteOrders(limit: 1) {\n orderUid') | |
| expect(request.variables).toEqual({ | |
| resolvedOwner: EOA.toLowerCase(), | |
| chainId: SupportedChainId.GNOSIS_CHAIN, | |
| offset: 1, | |
| limit: 2, | |
| direction: 'asc', | |
| }) | |
| expect(page.totalCount).toBe(2) | |
| expect(page.items.map(({ eventId, createdAt }) => ({ eventId, createdAt }))).toEqual([ | |
| { eventId: 'newer-event', createdAt: 200 }, | |
| { eventId: 'older-event', createdAt: 100 }, | |
| ]) | |
| expect(page.items[0]).not.toHaveProperty('partOrders') | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/composable/tests/ProgrammaticOrderApi.spec.ts` around lines 88 -
131, Update the query assertion in the “requests parent-only TWAPs by creation”
test to detect nested part-order item selection despite the existing
discreteOrders(limit: 1) syntax. Assert against the actual rendered nested-field
pattern, while preserving the response-level assertion that page.items[0] has no
partOrders.
Comment on lines
+172
to
+212
| it('lists the latest EOA TWAP parents from the live programmatic orders API', async () => { | ||
| const page = await new ProgrammaticOrderApi().getTwapOrders({ | ||
| resolvedOwner: EOA, | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| }) | ||
|
|
||
| expect(page.items).toMatchSnapshot() | ||
| }, 30_000) | ||
|
|
||
| it('lists the latest Safe TWAP parents from the live programmatic orders API', async () => { | ||
| const page = await new ProgrammaticOrderApi().getTwapOrders({ | ||
| resolvedOwner: SAFE, | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| }) | ||
|
|
||
| expect(page.items).toMatchSnapshot() | ||
| }, 30_000) | ||
|
|
||
| it('lists one page of EOA TWAP part orders from the live programmatic orders API', async () => { | ||
| const api = new ProgrammaticOrderApi() | ||
| const parentsPage = await api.getTwapOrders({ | ||
| resolvedOwner: EOA, | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| }) | ||
| const parent = parentsPage.items.find(({ executedAmounts }) => executedAmounts.executedFeeAmount > 0n) | ||
|
|
||
| expect(parent).toBeDefined() | ||
|
|
||
| const page = await api.getTwapPartOrders( | ||
| { | ||
| eventId: String(parent?.eventId), | ||
| chainId: SupportedChainId.GNOSIS_CHAIN, | ||
| }, | ||
| { offset: 0, limit: 10 }, | ||
| ) | ||
|
|
||
| expect(page.totalCount).toBe(parent?.partOrdersCount) | ||
| expect(page.items.some(({ executedFeeAmount }) => executedFeeAmount !== null)).toBe(true) | ||
| expect(page).toMatchSnapshot() | ||
| }, 30_000) | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Live-network tests remain non-hermetic.
These tests call the live default programmatic orders API and depend on mutable on-chain order history (snapshot comparisons, and finding an order with executedFeeAmount > 0n). Network issues, indexer downtime, or the underlying data changing can break CI without any code regression — the same concern raised previously and still present after the refactor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/composable/tests/ProgrammaticOrderApi.spec.ts` around lines 172 -
212, Make the live-network tests in the ProgrammaticOrderApi test suite hermetic
by replacing direct default-API calls with a deterministic mock or
fixture-backed response. Update the EOA and Safe TWAP parent tests and the
part-order test so snapshots and the executed-fee parent selection use stable
controlled data, while preserving assertions for pagination and part-order
behavior.
kernelwhisperer
force-pushed
the
daniel/fe-272-list-twap-orders-for-eoa
branch
from
July 27, 2026 15:13
ff7f4c7 to
f62dc6e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/composable/src/programmaticOrders/graphql.ts (1)
42-56: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFetch has no timeout/AbortSignal.
A stalled upstream leaves
query(),queryPage(), and callers likegetTwapOrders()pending indefinitely. This was flagged in a prior review round on this same method and remains unaddressed.🔧 Proposed fix: bound the request with an AbortSignal timeout
async query(query: string, variables: Record<string, unknown>): Promise<unknown> { let response: Response + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), DEFAULT_TIMEOUT_MS) try { response = await fetch(this.url, { method: 'POST', headers: { Accept: 'application/json', 'Content-Type': 'application/json', }, body: JSON.stringify({ query, variables }), + signal: controller.signal, }) } catch (cause) { + if (controller.signal.aborted) throw new GraphqlClientError('GraphQL request timed out', { cause }) throw new GraphqlClientError('GraphQL request failed', { cause }) + } finally { + clearTimeout(timeout) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/composable/src/programmaticOrders/graphql.ts` around lines 42 - 56, Update the GraphQLClient query method’s fetch request to use an AbortSignal timeout, ensuring stalled upstream requests are aborted within the configured timeout and continue to be wrapped as GraphqlClientError. Preserve the existing request payload and error-handling behavior.
🧹 Nitpick comments (1)
packages/composable/src/programmaticOrders/graphql.ts (1)
76-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInvalid-envelope/page errors discard validation details.
result.issuesis dropped when throwing'Invalid GraphQL response'/`Invalid GraphQL page: ${operation.page}`, making it hard to diagnose schema mismatches from production errors.♻️ Proposed fix: include a compact issue summary
if (!result.success) { - throw new GraphqlClientError('Invalid GraphQL response') + throw new GraphqlClientError('Invalid GraphQL response', { cause: result.issues }) }if (!result.success) { - throw new GraphqlClientError(`Invalid GraphQL page: ${operation.page}`) + throw new GraphqlClientError(`Invalid GraphQL page: ${operation.page}`, { cause: result.issues }) }Also applies to: 108-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/composable/src/programmaticOrders/graphql.ts` around lines 76 - 80, Update the error handling after safeParse in the GraphQL response and page validation paths to include a compact summary derived from result.issues in the thrown GraphqlClientError messages. Preserve the existing invalid-envelope and invalid-page context, including the operation.page value, while retaining the current success flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/composable/src/programmaticOrders/graphql.ts`:
- Around line 42-56: Update the GraphQLClient query method’s fetch request to
use an AbortSignal timeout, ensuring stalled upstream requests are aborted
within the configured timeout and continue to be wrapped as GraphqlClientError.
Preserve the existing request payload and error-handling behavior.
---
Nitpick comments:
In `@packages/composable/src/programmaticOrders/graphql.ts`:
- Around line 76-80: Update the error handling after safeParse in the GraphQL
response and page validation paths to include a compact summary derived from
result.issues in the thrown GraphqlClientError messages. Preserve the existing
invalid-envelope and invalid-page context, including the operation.page value,
while retaining the current success flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d6dec8e-0195-46f3-b466-d263b177ef1e
⛔ Files ignored due to path filters (2)
packages/composable/tests/__snapshots__/ProgrammaticOrderApi.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/composable/README.mdpackages/composable/package.jsonpackages/composable/src/index.tspackages/composable/src/programmaticOrders/ProgrammaticOrderApi.tspackages/composable/src/programmaticOrders/graphql.tspackages/composable/src/programmaticOrders/index.tspackages/composable/src/programmaticOrders/schemas.tspackages/composable/src/programmaticOrders/twap-queries.tspackages/composable/src/programmaticOrders/twap-schemas.tspackages/composable/src/programmaticOrders/twap-types.tspackages/composable/src/programmaticOrders/types.tspackages/composable/src/programmaticOrders/validation.tspackages/composable/tests/ProgrammaticOrderApi.spec.tspackages/composable/tests/graphql.spec.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/composable/src/index.ts
- packages/composable/src/programmaticOrders/validation.ts
- packages/composable/src/programmaticOrders/twap-queries.ts
- packages/composable/src/programmaticOrders/index.ts
- packages/composable/src/programmaticOrders/types.ts
- packages/composable/tests/graphql.spec.ts
- packages/composable/src/programmaticOrders/ProgrammaticOrderApi.ts
- packages/composable/src/programmaticOrders/twap-types.ts
- packages/composable/src/programmaticOrders/twap-schemas.ts
- packages/composable/tests/ProgrammaticOrderApi.spec.ts
- packages/composable/README.md
- packages/composable/src/programmaticOrders/schemas.ts
shoom3301
approved these changes
Jul 27, 2026
shoom3301
left a comment
shoom3301
left a comment
Contributor
There was a problem hiding this comment.
Semantically all good. I haven't checked the queries too deep.
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
3 participants
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.
Part of FE-272
Add a read-only
ProgrammaticOrderApito@cowprotocol/sdk-composablefor listing TWAP orders by EOA or Safe. Parents and part orders are fetched separately.Changes
getTwapOrders({ resolvedOwner, chainId }, options?).getTwapPartOrders({ eventId, chainId }, options?).QueryOptions:offset,limit, anddirection.QueryPage<T>withitemsandtotalCount.owner,resolvedOwner,eventId, andhash.bigint.ProgrammaticOrderApiError.Summary by CodeRabbit