Skip to content

fix(bridge): align BridgeService withdrawal return shapes with GraphQL (ENG-352) - #396

Closed
patoo0x wants to merge 1 commit into
tmp/bridge-rebase-pr-readyfrom
eng-352/svc-return-shapes
Closed

fix(bridge): align BridgeService withdrawal return shapes with GraphQL (ENG-352)#396
patoo0x wants to merge 1 commit into
tmp/bridge-rebase-pr-readyfrom
eng-352/svc-return-shapes

Conversation

@patoo0x

@patoo0x patoo0x commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

ENG-352 — NEW-1: Bridge service return shapes match GraphQL types (svc half)

Closes the service half of the GraphQL contract work. ENG-351/ENG-353 (PR #393) fixed the GraphQL-side types and error mapping but did not touch src/services/bridge/, so the service-shape audit ENG-352 was scoped to was never actually completed.

Why this matters

The bridgeInitiateWithdrawal mutation and bridgeWithdrawals query resolvers return the BridgeService result directly — there is no resolver-level field mapping. That makes the service the source of truth for the public BridgeWithdrawal GraphQL type, which requires transferId, amount, currency, state, createdAt (all NonNull) and exposes an optional failureReason.

Findings & fixes

  • createdAt missing on the initiate path (live bug). InitiateWithdrawalResult omitted createdAt. A client running bridgeInitiateWithdrawal(...) { withdrawal { createdAt } } hit a non-null violation because the field resolved to undefined against GT.NonNull(GT.String). Now populated from transfer.created_at.
  • failureReason not surfaced. Added optional failureReason to both InitiateWithdrawalResult and WithdrawalResult to match the nullable GraphQL field; getWithdrawals now maps the persisted row's failureReason.
  • Tautological filter in getWithdrawals. w.bridgeTransferId !== null || w.bridgeTransferId !== undefined is always true, letting pending rows with no bridgeTransferId leak through and violate the NonNull transferId field. Corrected to &&.

Tests

New test/flash/unit/services/bridge/return-shapes.spec.ts (5 tests, all passing) asserts both withdrawal paths:

  • return every NonNull field the BridgeWithdrawal type requires (incl. createdAt),
  • carry failureReason through getWithdrawals,
  • drop rows without a transferId,
  • do not leak the raw Bridge transfer shape (id/status).

Scope / hygiene

  • Only 2 files touched: src/services/bridge/index.ts + the new spec.
  • No whitespace/Prettier/lint-only churn (pre-existing Prettier debt in getKycStatus left untouched to keep the diff scoped).
  • tsc --noEmit clean for both changed files.

Linear: ENG-352

…L (ENG-352)

The bridgeInitiateWithdrawal/bridgeWithdrawals resolvers return the
BridgeService result directly with no resolver-level mapping, so the
service is the source of truth for the public BridgeWithdrawal contract.

Audit findings (ENG-352 svc half):
- InitiateWithdrawalResult was missing `createdAt`, which is NonNull on
  the GraphQL BridgeWithdrawal type. A client selecting `createdAt` on the
  mutation payload hit a non-null violation (undefined). Now populated
  from `transfer.created_at`.
- Added optional `failureReason` to both InitiateWithdrawalResult and
  WithdrawalResult to match the nullable GraphQL field; getWithdrawals now
  surfaces the persisted row's failureReason.
- getWithdrawals filtered with `!== null || !== undefined` (always true),
  letting pending rows with no bridgeTransferId leak through and violate
  the NonNull `transferId` field. Corrected to `&&`.

Adds focused unit tests asserting both withdrawal paths return every
NonNull field required by the BridgeWithdrawal GraphQL type and do not
leak the raw Bridge transfer shape.
@linear

linear Bot commented Jun 7, 2026

Copy link
Copy Markdown

ENG-352

@patoo0x

patoo0x commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

⏸️ Hold for merge — depends on #387.

PR #387 (request-withdrawal-before-initiation, split withdrawal into request/confirm/cancel) restructures the same withdrawal paths in src/services/bridge/index.ts and rewrites the BridgeWithdrawal GraphQL type. It's the larger structural change, so this PR should rebase on top of #387 rather than merge first.

What changes after #387 lands:

Plan: merge #387 first, then rebase this PR and re-scope it to (1) the getWithdrawals filter fix and (2) the transferId/stateid/status contract alignment, with the contract test updated to the new field names. Not merging this until #387 is in.

Copy link
Copy Markdown
Contributor

Closing this as superseded by #387.

After #387 merged, the public BridgeWithdrawal contract is no longer the old transferId/state shape this PR was built around. The merged #387 code now exposes and tests the final service shape:

  • initiateWithdrawal returns every non-null GraphQL field (id, amount, currency, status, createdAt) plus bridgeTransferId
  • getWithdrawals returns id/status, not legacy transferId/state
  • failureReason is preserved
  • rows without bridgeTransferId are excluded so non-null id/status never resolve as undefined

A fresh comparison of this branch against the updated base shows the stale diff would now conflict with and partially reverse #387 (32 files changed, 337 insertions, 1933 deletions), especially in src/services/bridge/index.ts and test/flash/unit/services/bridge/return-shapes.spec.ts.

Verified on the updated base:

TEST="test/flash/unit/services/bridge/return-shapes.spec.ts test/flash/unit/graphql/public/root/query/bridge-withdrawals.spec.ts test/flash/unit/graphql/public/types/object/bridge-contract.spec.ts" yarn test:unit

Result: 3 suites passed, 7 tests passed.

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.

3 participants