Skip to content

fix: make params optional on compareMarketPrices and fetchRelatedMarkets#581

Closed
realfishsam wants to merge 1 commit into
mainfrom
fix/448-router-params-optional
Closed

fix: make params optional on compareMarketPrices and fetchRelatedMarkets#581
realfishsam wants to merge 1 commit into
mainfrom
fix/448-router-params-optional

Conversation

@realfishsam
Copy link
Copy Markdown
Contributor

Fixes #448

@realfishsam
Copy link
Copy Markdown
Contributor Author

PR Review: FAIL

What This Does

Makes the params argument optional on compareMarketPrices, fetchRelatedMarkets, and fetchHedges in both BaseExchange.ts (abstract declarations) and Router.ts (concrete implementations), allowing callers to invoke these methods without arguments for "browse all" mode.

Blast Radius

  • Core: BaseExchange.ts (3 method signatures), Router.ts (3 implementations)
  • OpenAPI spec: openapi.yaml is AST-generated from BaseExchange.ts -- signature changes require regeneration
  • SDKs: Generated TS and Python clients depend on the OpenAPI spec

Findings

  1. CRITICAL: OpenAPI spec not regenerated. BaseExchange.ts is changed but openapi.yaml is not updated. Per CLAUDE.md and the .github/workflows/openapi-check.yml CI check, this will fail CI. The PR must run npm run generate:openapi --workspace=pmxt-core and include the updated openapi.yaml and method-verbs.json.

  2. Note on compareMarketPrices OpenAPI behavior. The current OpenAPI spec for compareMarketPrices uses a POST with required: ["args"] in the request body. Making params optional in TS does not automatically make the args array optional in the spec -- the regeneration step should handle this, but it needs verification.

  3. Router.ts uses params.market without null-guarding for the = {} default. When params defaults to {}, params.market is undefined, so the if (params.market && !params.marketId) guard works correctly. No issue here.

  4. Good immutable patterns. Router.ts already uses params = { ...params, ... } spread for reassignment, consistent with project conventions.

PMXT Pipeline Check

  • Field propagation: N/A
  • OpenAPI sync: ISSUE (BaseExchange.ts changed, openapi.yaml not regenerated)
  • Type safety: OK (optional params with = {} defaults are sound)

Semver Impact

minor -- relaxes required argument to optional, backward-compatible

Risk

  • CI will reject this PR until openapi.yaml is regenerated. After regeneration, SDKs should be regenerated too to reflect the optional params.

@realfishsam realfishsam deleted the fix/448-router-params-optional branch May 24, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compareMarketPrices and fetchRelatedMarkets take non-optional params while sibling router methods take optional params

1 participant