Skip to content

feat(client): add account funding workflows - #291

Open
kartojal wants to merge 10 commits into
mainfrom
feature/dev-2-add-bridge-support
Open

feat(client): add account funding workflows#291
kartojal wants to merge 10 commits into
mainfrom
feature/dev-2-add-bridge-support

Conversation

@kartojal

@kartojal kartojal commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Adds typed Bridge funding workflows, cursor-paginated status, and integration coverage for DEV-2.


Note

Medium Risk
New deposit/withdraw and fund-movement APIs touch real money flows; mistakes in address/quote/status handling could mislead integrators, though changes are mostly additive SDK wiring with validation and tests.

Overview
Adds typed Bridge account-funding support in @polymarket/bindings (@polymarket/bindings/bridge) and wires it through @polymarket/client against https://bridge.polymarket.com.

New SDK surface covers deposit/withdrawal address creation (EVM/SVM/BTC/Tron, with tvmtron normalization and optional builder attribution), supported assets, quotes, and cursor-paginated transfer status (including tolerant handling when nextCursor is missing during rollout and forward-compatible unknown statuses).

Public clients must pass an explicit wallet for address creation; secure clients bind deposit/withdraw flows to client.account.wallet. Integration tests exercise read paths and an opt-in metered pUSD round-trip on Polygon production.

Reviewed by Cursor Bugbot for commit 1e08e84. Bugbot is set up for automated code reviews on this repo. Configure here.

@kartojal
kartojal marked this pull request as ready for review August 13, 2026 17:20
@kartojal
kartojal marked this pull request as draft August 13, 2026 17:27

@brunson-bot brunson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff plus surrounding context (environments.ts, pagination.ts, params.ts, ServiceClient.ts, the listComboActivity cursor precedent, and shared.ts). All CI green. Structure is good — schemas land in bindings, the client only owns user input, list*/fetch*/create* naming and the PageSizeSchema.max(n).default(n) shape match existing actions, error unions and @throws are complete, and the public/secure split is enforced at the type level with funding.test-d.ts. Three [issue]s, one [nit], none blocking.


[issue] packages/client/src/environments.ts:305preproduction silently resolves to the production bridge.

preproduction is a forkEnvironmentConfig over production that overrides clob, data, gamma, and relayer. It does not override bridge, and forkRestEndpoint (:264-273) spreads the base when the fork is absent — so preproduction.bridge.rest === 'https://bridge.polymarket.com'.

Every other service in that config points at a preprod host; bridge is now the only one that falls back to production, and it's the money-movement one. createDepositAddresses / createWithdrawalAddresses from a preproduction client would mint live production funding addresses.

Impact today is bounded — preproduction has zero consumers in the repo and is @internal. But #284 just made environments forkable for integration runs via POLYMARKET_INTEGRATION_ENVIRONMENT_CONFIG, so this is the moment it becomes reachable. Either point it at a preprod bridge host or leave an explicit comment that production is intentional.

[issue] packages/client/src/actions/funding.ts:273 / packages/client/src/decorators/funding.ts:68 — the documented pageSize contract isn't what production does.

Both TSDoc blocks state "Page size must be between 1 and 100 and defaults to 50", and :243 validates it client-side. But per this PR's own notes, production doesn't paginate yet: funding.ts:138 in bindings says the nextCursor field is omitted during rollout, and tests/integration/funding.test.ts:68 says "The legacy proxy ignores limit and returns one terminal page."

So today a caller passing pageSize: 1 gets the entire collection back in a single page, with hasMore: false. The doc reads as a guarantee, not a hint.

The same gap makes the continuation path untestable: in the integration test the if (page.hasMore) branch at :70-73 never executes, so cursor threading, the limit/cursor param names, and the nextCursor round-trip have no coverage from either side (integration can't reach it, and AGENTS.md rules out mocking the response). That's acceptable given the rollout, but worth being explicit about — the param naming follows the listComboActivity precedent (actions/activity.ts:345-351), which is the main reason I'd expect it to be right rather than anything the tests prove.

Suggest softening the TSDoc to say the page size is a request hint honored once server-side pagination is enabled, and noting that responses are currently returned as a single terminal page.

[issue] packages/bindings/src/bridge/funding.ts:118quoteId is public surface nothing can consume, and the quote's non-binding nature is undocumented.

quoteId appears exactly once in the repo: the schema field. No action accepts it, and createWithdrawalAddresses takes a destination, not a quote — so there is no way to bind the quoted rate to the transfer that follows.

That's presumably the upstream design, but for a money-movement API the docs should say it. fetchFundingQuote's TSDoc (actions/funding.ts:194) says "estimated", which covers the numbers, but not the fact that estFeeBreakdown.minReceived / maxSlippage are advisory and the executed transfer may differ. The PR's own metered test treats the quote as a pre-flight guard and then hopes (tests/integration/funding.test.ts:175-183) — that's the right usage, and it's exactly what an integrator won't infer from the current docs.

Either drop quoteId from the exposed type until something accepts it, or document what it's for.

[nit] packages/client/src/actions/funding.ts:38-41 and packages/bindings/src/bridge/funding.ts:30-33 — the same regex-then-pipe workaround, twice.

Both wrap EvmAddressSchema in z.string().regex(/^0x[0-9a-fA-F]{40}$/).pipe(...). The reason is real: EvmAddressSchema is z.string().transform(toEvmAddress) (shared.ts:334) → expectEvmAddress → an InvariantError that escapes safeParse, so without the wrapper a malformed address surfaces as an invariant failure instead of UserInputError / UnexpectedResponseError. These are the only two .pipe(EvmAddressSchema) sites in the repo.

#288 (open) changes EvmAddressSchema itself to a .refine-then-cast with a byte-identical predicate, which makes both wrappers redundant. Worth a note so whichever lands second cleans them up rather than leaving two layers of the same check.


Checked and not raised: the KnownFundingTransactionStatus | (string & {}) open union plus z.string().transform(v => v) matches PerpsWithdrawalStatus (bindings/src/perps/common.ts:239,312) exactly; the tvmtron transform and the nextCursor optional-to-null transform both behave as the bindings tests assert; the secure decorator's {...request, wallet} ordering can't be overridden by a JS caller and strictObject rejects strays; request types not being re-exported by name from client/src/index.ts matches every other decorator (only value exports are re-exported).

@kartojal
kartojal marked this pull request as ready for review August 14, 2026 08:33

@brunson-bot brunson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5f7f921..1e08e84 (3 commits) only.

Addressed since the last round: quoteId now carries TSDoc scoping it to log/support correlation (bridge/funding.ts:206), which answers my "dead public surface" [issue] — it no longer reads as something you can bind to a subsequent transfer. The metered reorder is a genuine improvement: the precondition is now checkable (fetchBalanceAllowance on collateral is the same wallet transferErc20 spends from), and an unsafe refreshed quote after the irreversible leg throws loudly instead of skipping.

Still open from earlier rounds: preproduction has no bridge override (environments.ts:304-310), the pageSize "1–100, defaults to 50" contract vs. a non-paginating production, and the regex().pipe(EvmAddressSchema) duplication (#288 is still open).


[issue] packages/client/tests/integration/funding.test.ts:18 — the deposit leg is pinned exactly to the live minimum, and it now runs after the irreversible withdrawal. GET https://bridge.polymarket.com/supported-assets right now reports minCheckoutUsd: 2 for both Polygon USDC and pUSD (229 assets, checked live). So the withdrawal leg has 5% headroom (2.1 vs 2) while the deposit leg has none (2.0 vs 2). The pre-flight guard at :144 reads the minimums once, before anything moves; the post-withdrawal guard at :227 only re-checks quote outputs. Bump the minimum — or apply it to a USD-converted value with USDC a hair under peg — and fetchFundingQuote at :216 rejects with the wallet already holding loose USDC. Headroom is the fix, not another check: raising both constants (e.g. 2_300_000n / 2_200_000n) restores the withdrawal leg's margin on the deposit side too.

[issue] :283-287 — the round trip no longer asserts its own outcome. Dropping toTokenAddress is justified by the routing-token comment, but nothing replaced it: the matcher now keys on the source leg plus createdTimeMs, and the test ends at waitForFundingTransfer. Both legs can report COMPLETED with funds landing in something other than pUSD and the test still passes. The balance read at :150 is already there — reusing it after the deposit (final collateral ≥ starting balance minus the quoted fee band) asserts the user-visible outcome rather than the bridge's own status record, and is strictly stronger than the destination check that was removed.

[nit] bridge/funding.ts:62,75,97,116,135,183,208,239satisfies z.ZodType<T> ties the schema in one direction only. Verified on tsc 5.9.3 + zod 4.4.3: it catches a dropped field, a changed field type, required→optional, and widening — but a schema that gains a field passes silently, so a field added to the schema and forgotten in the hand-written type is invisible to consumers with no compile error. Currently all eight pairs match, and the direction it does catch is the one that matters (a type promising more than the schema delivers), so this is optional. If you want the complete tie, #251 landed a mutual-assignability marker for exactly this.

[nit] packages/client/src/index.ts:3-13 — the bridge line is the only one of seven bindings subpaths using an explicit type list. clob, data, gamma, perps, relayer and the root all use export type *, so a future bridge type reaches consumers automatically everywhere except here. Deliberate, or worth matching the siblings?


Verified, not findings:
actions/funding.ts:58 — the /^\d+$/ chain-ID regex is safe for every currently supported chain: all 13 distinct chainId values across the 229 live assets are decimal, including the non-EVM ones (Solana 1151111081099710, Bitcoin 8253038, Tron 728126428).
:305-331 — the retry can't double-spend. batch would revert is raised by simulateBatch in relayer-v2 (packages/api/pkg/depositwallet/orchestrator.go:276-277), an eth_estimateGas that runs before reservations and submission, so a 400 carrying that message means nothing was broadcast. The comment's "pre-submission" claim holds.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 89ca1af. Configure here.

Comment thread packages/client/tests/integration/funding.test.ts

@brunson-bot brunson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 3 — reviewed 1e08e84..89ca1af (1 commit) plus a live check of the Bridge API and the CLOB balance-checker source. Cursor's open finding is confirmed (replied in-thread), and two of my own earlier findings need correcting.


Correcting two of my own findings.

Round 2, "reuse the balance read" — my advice was wrong, and it produced the assertion Cursor is flagging. I called reusing fetchBalanceAllowance after the deposit "strictly stronger" without checking what that endpoint reads. It is cache-first: GET /balance-allowanceRepositoryServiceImpl.GetBalanceAndAllowance (clob, packages/balance-checker/pkg/repository/repository_service_impl.go:77-115) reads Redis via getBalanceAndAllowances and only falls through to an on-chain fetchBalanceAndAllowance when the cached balance is absent. setBalance (:890-906) writes with TTL 0 — no expiry. GET /balance-allowance/update (:140-158) is the one that unconditionally re-reads chain state.

So the read at funding.test.ts:152 populates a never-expiring cache entry before anything moves, and the read at :264 can return that same value fifteen minutes later. Substituting into the assertion: starting >= starting - 2.3 + 2.15, i.e. starting >= starting - 0.15a fully stale read passes vacuously. The repo's own convention agrees: actions/orders/trade.ts:273 calls updateBalanceAllowance immediately after handle.wait(), while fetchBalanceAllowance is used only for pre-checks (actions/orders/allowance.ts:22).

Fix is updateBalanceAllowance(client, { assetType: AssetType.COLLATERAL }) for the final read. Worth pairing with a short poll: the cache is also refreshed by chain events, but only for watched makers (repository_service_event_impl.go:41,58 both gate on isMakerAddressWatched) and asynchronously, so a bridge COMPLETED can lead the balance-checker's log pipeline by a beat.

Round 1, pageSize "the documented contract isn't what production does" — void. I took the bindings comment at its word instead of hitting the endpoint. Production paginates today: GET /status/0x2356…C053?limit=1 returns a nextCursor; walking it yields 6 transactions over 6 pages; limit=101 returns HTTP 400. So PageSizeSchema.max(100).default(50) matches the server's cap exactly, limit/cursor are the right param names, and the if (page.hasMore) branch at :78-81 does execute. Dropping the rollout wording in this commit is correct.


Resolved since the last round: metered amounts now carry headroom on both legs (2_300_000n / 2_200_000n), the environment guard added environment.name !== 'production' on top of the existing bridge.rest / chainId / collateralToken checks, and preproduction's production bridge is now explicit and commented (environments.ts:305-310) — that closes my round-1 [issue].

Checked, not raised: /supported-assets (229 assets) and /quote (same-chain and cross-chain) both parse clean against the new schemas with zero field violations; fileParallelism: false in vitest.config.ts means no other integration file can move collateral concurrently, so the startingCollateralBalance baseline isn't racy; float * 10 ** decimals in minimumReturnedCollateral can be one base unit low, which only loosens a >= bound.

Validation steps
  1. Verified PR #291 is OPEN at head 89ca1af, worktree git rev-parse HEAD matches
  2. Read .brunson/review.md and applied it plus AGENTS.md (linked doc 1 of 2; skipped docs/sdk-direction.md — no public-API reshaping in this commit)
  3. Read the full diff (16 files) and git show 89ca1af for the delta since round 2
  4. Read surrounding context: pagination.ts, ServiceClient.ts, actions/params.ts, response.ts, clients.ts, environments.ts, actions/account.ts, decorators/perps.ts, tests/integration/fixtures.ts, vitest.config.ts
  5. Confirmed the builderCode / secureClientWithDepositWallet / runMeteredTests / skip fixtures all exist in fixtures.ts
  6. Confirmed all three tsconfig.build.json files carry no paths, so the root tsconfig.json alias is the correct and only place for the new subpath
  7. Live-probed https://bridge.polymarket.com: /supported-assets, /quote same-chain and cross-chain, /status with limit=1|100|101|500, and a full cursor walk of the test address
  8. Validated all 229 live assets and both quote responses field-by-field against FundingAssetSchema / FundingQuoteSchema
  9. Traced GET /balance-allowance vs /balance-allowance/update through the clob balance-checker repository layer to settle the staleness question
  10. Confirmed paginate cannot loop forever here — hasMore is true exactly when nextCursor is defined
  11. Confirmed CI is green (Verify / Run, Tests / Run, Changeset, CodeQL)

Slack discussion

Comment thread packages/client/src/actions/funding.ts
Comment thread packages/client/tests/integration/funding.test.ts Outdated

@brunson-bot brunson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 4 — reviewed 89ca1af..391b595: a merge from main plus one commit. All three open items are addressed, and I verified the fixes rather than taking them on faith. One [nit] on the new allowlist.

Addressed.

  • recipientAddress validation — the superRefine gated on KnownEvmFundingChainId is the right shape, and the allowlist is exactly correct today. I re-pulled /supported-assets (229 assets, 13 distinct chain IDs) and diffed it against the enum: all ten EVM chains are present (1, 10, 56, 137, 143, 999, 4663, 8453, 42161, 57073) and the three non-EVM ones are correctly left out (Solana 1151111081099710, Bitcoin 8253038, Tron 728126428). The TSDoc on FundingDestination.recipientAddress and the regression test cover the documentation half of the finding too.
  • Stale CLOB balance (Cursor's finding + my bad round-2 advice) — both reads now go through updateBalanceAllowance, and waitForCollateralBalance (:381-399) polls it for up to 60s. That covers the async gap I flagged: the cache is only event-refreshed for watched makers and lags the bridge's COMPLETED. Returning the last-read balance on timeout so the expect still reports the real number, rather than throwing a bare timeout, is a nicer failure than what I suggested.
  • Duplicated regex().pipe(EvmAddressSchema) (round-1 [nit]) — both wrappers are gone, and the merge picked up #288, so this is behaviour-preserving rather than a loosening: EvmAddressSchema is now .refine(isHexString(value) && value.length === 42, 'Expected an EVM address') (bindings/src/shared.ts:335-341), and isHexString is /^0x[a-fA-F0-9]+$/ (types/src/hex.ts:36-38), which composes to exactly the /^0x[0-9a-fA-F]{40}$/ the wrappers enforced — mixed-case checksummed addresses still pass. BuilderCodeSchema did not gain a refine in that merge, so keeping BuilderCodeInputSchema's wrapper is correct and matches rfq.ts:893-895.

Merge check. Funding wiring survived intact: bridge in PublicContext (clients.ts:83), the @internal getter (:152), both client constructors (:326, :564), decorator registration and barrel re-export, and the nine type exports in index.ts. .changeset/dev-2-bridge-support.md is untouched and none of the six changesets the merge brought in collide with it. No unused imports left behind in either file the commit touched.

Worth knowing rather than fixing: the new rejects malformed EVM withdrawal recipients test is pure input validation with no network, but it lives in the client-integration vitest project, which Tests / Run doesn't include — so the one test guarding the new money-path rule only runs in the separate integration workflow. AGENTS.md's "never create client fixtures solely to satisfy an action signature" and "don't add production exports solely to test a private schema" both push it there, so I don't think there's a better home under the current rules; just flagging that required CI isn't covering it.

Validation steps
  1. Verified PR is OPEN at head 391b595
  2. git log/git diff over 89ca1af..391b595 — isolated the two branch commits from the merge-from-main noise
  3. Read 391b595 in full (3 files) and diffed 89ca1af..bb967c5 on bindings/src/shared.ts to see what the merge changed under the funding code
  4. Re-pulled live /supported-assets and diffed all 13 chain IDs against the new KnownEvmFundingChainId enum
  5. Read isHexString (types/src/hex.ts:36) and the new EvmAddressSchema to confirm the wrapper removal is behaviour-preserving
  6. Traced the error message the new test asserts through parseUserInputUserInputError.fromZodErrorformatInputZodErrorformatIssues/formatPath (validation.ts:19-145)
  7. Confirmed BuilderCodeSchema is still a bare transform, so its wrapper is still load-bearing
  8. Checked post-merge funding wiring in clients.ts, decorators/index.ts, index.ts, and both funding files for unused imports
  9. Listed .changeset/ at head for filename collisions after the merge
  10. CI: Tests / Run, Changeset, CodeQL, dependency-review green; Verify / Run still running at review time

Slack discussion

Comment thread packages/client/src/actions/funding.ts

@brunson-bot brunson-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 5 — reviewed 391b595..f3dea39 (1 commit, 2 files, +7/-1). Both remaining [nit]s addressed; nothing new. No open findings from me on this PR.

  • Enum drift comment — the wording is better than my suggestion: stating that unknown chains intentionally skip EVM recipient validation for forward compatibility makes the fail-open explicit, which was the actual gap. Resolving that thread.
  • Timeout budget20 * 60_00030 * 60_000 gives ~11 min of slack over the 19 min of bounded inner waits (8 + 8 + 2 + 1), so the helpers' own timeout errors, which name the address and the leg, are now what trips first rather than a bare vitest abort mid-round-trip. Resolving that thread too.

Re-checked that nothing else moved: the enum members are unchanged (still an exact match for the ten EVM chain IDs in live /supported-assets), KnownEvmFundingChainId stays module-private, and the diff touches no runtime behaviour beyond the test timeout.

Cursor Bugbot is green on this commit; the rest of CI was still running at review time — worth a glance at Verify / Run before merging, though a TSDoc block and a numeric literal are unlikely to move it.

Validation steps
  1. Verified PR is OPEN at head f3dea39
  2. Read the full 391b595..f3dea39 diff (2 files)
  3. Re-read the enum in context at the new head to confirm no member changed alongside the comment
  4. Recomputed the timeout headroom against the three inner wait loops plus waitForCollateralBalance
  5. Confirmed both previously-open threads are genuinely addressed, then resolved them
  6. CI at review time: Cursor Bugbot and dependency-review green, remaining jobs still running

Slack discussion

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.

2 participants