Skip to content

feat(client): expose trading approval state (DEV-564) - #288

Open
kartojal wants to merge 13 commits into
mainfrom
feature/dev-564-ts-sdk-expose-read-only-trading-approvals-state
Open

feat(client): expose trading approval state (DEV-564)#288
kartojal wants to merge 13 commits into
mainfrom
feature/dev-564-ts-sdk-expose-read-only-trading-approvals-state

Conversation

@kartojal

@kartojal kartojal commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add signer-free trading approval state reads for public clients
  • add authenticated-wallet convenience reads for secure clients
  • reuse the canonical missing-approval resolver in setupTradingApprovals
  • add read-only integration coverage and a client package changeset

Closes #286
Linear: DEV-564

Testing

  • pnpm lint
  • pnpm typecheck
  • client unit suite: 295 passed
  • approvals integration suite: 2 passed, 5 credential-dependent skipped

Note

Medium Risk
Touches trading approval resolution shared with setup workflows and changes EVM address validation behavior across bindings consumers. The new API itself is read-only eth_call based.

Overview
Adds fetchTradingApprovalsState, a read-only API that reports whether a wallet is fully approved for trading and which ERC-20/ERC-1155 approvals are still missing—without a signer or transactions.

Public clients can inspect any wallet; secure clients default to the authenticated wallet. The existing setup workflow now reuses the same resolver, and malformed approval-check RPC results surface as UnexpectedResponseError. Related approval requirement types are exported.

Also updates EvmAddressSchema so bad addresses fail Zod validation instead of throwing InvariantError.

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

@kartojal
kartojal marked this pull request as ready for review August 10, 2026 16:01

@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.

Read the diff plus the surrounding decorator / action / rpc.ts layers. No blocker — CI is green and the read path itself is correct: ethCallBatch already guarantees a same-length HexString[] with every element validated (rpc.ts:150-186), so the offset slicing and as HexString casts are sound, and wrapping the ABI decode in UnexpectedResponseError is a genuine improvement to the pre-existing prepareTradingApprovals path.

Four comments, ranked. The first two are public-API shape and are cheapest to fix before this ships; the third is a pre-existing hazard that this PR newly exposes on a fresh entry point.

): Promise<TradingApprovalsState>;
};

const SecureGetTradingApprovalsStateRequestSchema = z.object({

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.

[issue] This is the only decorator in the package that imports zod or calls parseUserInput (checked all of src/decorators/). account.ts already solves the exact same "secure client defaults the address to its own account" problem without a second schema — DefaultAccountWallet<TRequest> for the type (account.ts:50-58) plus a three-line withAccountWallet() for the runtime default (account.ts:510-518), used by eight secure methods at account.ts:533-547.

Two concrete costs of doing it with a schema instead:

  • z.object strips unknown keys, so the override forwards only { wallet }. The moment GetTradingApprovalsStateRequest grows a second field (a block tag, an include, anything), it will work on the public client and be silently dropped on the secure one — no type error, no runtime error, just ignored input. The spread in withAccountWallet has no such failure mode.
  • The @defaultValue TSDoc you wrote inside the z.object doesn't survive z.input<...>, so SecureGetTradingApprovalsStateRequest's wallet reaches consumers undocumented. DefaultAccountWallet puts that doc on a hand-written member, where it does survive.

DefaultAccountWallet is keyed on user so it isn't directly reusable here, but a sibling keyed on wallet (or generalising it over the key) gets you the same contract, one parse instead of two, and leaves the input contract owned by the action.

* @throws {@link GetTradingApprovalsStateError}
* Thrown on failure.
*/
export async function getTradingApprovalsState(

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.

[issue] get* isn't a prefix this package uses. packages/client/src/actions/ exports 47 fetch* and 31 list* actions, and this is the only get*. AGENTS.md ties the prefix to SDK behaviour — fetch* means "returns a direct item or direct collection, with no SDK pagination abstraction" — which is exactly what this is. get* reads like a local/synchronous accessor, and this is a batched eth_call round trip.

fetchTradingApprovalsState costs nothing today and is a breaking rename once this is published.

missing: TradingApprovalRequirements;
};

export type GetTradingApprovalsStateError =

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.

[issue] UserInputError doesn't actually cover a malformed address. EvmAddressSchema is z.string().transform(toEvmAddress) (bindings/src/shared.ts:312), and toEvmAddressexpectEvmAddressinvariant(...), which throws InvariantError (types/src/refinements.ts:62-68). A throwing .transform() escapes safeParse as a raw exception rather than becoming a Zod issue, so parseUserInput never sees a failed result. Verified on zod 4.4.3:

const S = z.object({ wallet: z.string().transform(v => { throw new InvariantError('Expected an EVM address') }) });
S.safeParse({ wallet: 'nope' })  // → THROWS InvariantError out of safeParse
S.safeParse({ wallet: null })    // → { success: false }  ✔

So { wallet: 'not-an-address' } throws InvariantError — the assertion-style error AGENTS.md explicitly says must not appear in a public union — while { wallet: null } correctly throws UserInputError.

This is pre-existing and package-wide (every EvmAddressSchema input has it; cf. the markets.test.ts:52 comment about a raw TypeError aborting a page), so not a blocker on this PR. Flagging it because this is a brand-new public entry point whose documented @throws and whose only validation test both assert UserInputError.

const fetchSpy = vi.spyOn(globalThis, 'fetch');

try {
for (const request of [null, { wallet: null }]) {

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.

[issue] Both vectors here (null, { wallet: null }) fail at z.string() — the branch that works. Adding { wallet: 'not-an-address' } to the loop exercises the branch that doesn't (see the comment on GetTradingApprovalsStateError).

Separately, the secure default is untested: nothing calls secureClient.getTradingApprovalsState() with no argument and asserts it resolved against client.account.wallet. That default is the only behaviour this decorator override adds over the action, and it's also the behaviour that would silently regress if the request type ever grows a field (see the wallet.ts comment). The public-client test is good — the eth_call-only assertion is the right way to prove "no signer, no writes".

"@polymarket/client": minor
---

Add `getTradingApprovalsState` for reading a wallet's missing trading approvals without a signer or transaction workflow.

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.

[nit] The changeset covers the new action but not the two other consumer-visible changes: decode failures on the existing prepareTradingApprovals / setupTradingApprovals path now surface as UnexpectedResponseError instead of a raw ox decode error (a strict improvement — it makes the already-documented PrepareTradingApprovalsError union true rather than aspirational), and Erc20TradingApproval / Erc1155TradingApproval / TradingApprovalRequirements are now public exported types.

Also, while you're in there: TradingApprovalsState's own members are undocumented, and missing.erc20[].amount is the required allowance (MAX_UINT256), not a shortfall — worth one line each so missing doesn't read as "the amount you're short by".

@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 2 potential issues.

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 021a542. Configure here.

Comment thread packages/bindings/src/shared.ts
Comment thread packages/client/src/decorators/wallet.ts Outdated
@brunson-bot

Copy link
Copy Markdown
Contributor

Round 2 — all three round-1 findings addressed. get*fetch* throughout (action, both decorator surfaces, error guard, changeset, tests); the decorator-owned zod schema is gone in favour of a DefaultWallet<T> type + runtime spread mirroring withAccountWallet in decorators/account.ts; and EvmAddressSchema now refines before transforming, so a malformed address surfaces as UserInputError instead of InvariantError.

Two things to look at before merge.

[issue] The secure decorator's ?? default changes the invalid-input contract, and the PR's own test asserts the old one.

decorators/wallet.ts:349-355:

fetchTradingApprovalsState: (
  request: SecureFetchTradingApprovalsStateRequest = {},
) =>
  fetchTradingApprovalsState(client, {
    ...request,
    wallet: request.wallet ?? client.account.wallet,
  }),

Two of the three vectors in tests/integration/approvals.test.ts:86-88 no longer behave as asserted:

  • { wallet: null }?? is nullish, so null falls through to client.account.wallet and the read succeeds against a different wallet rather than rejecting. The old code ran this through EvmAddressSchema.optional(), where null is not undefined and so failed parse → UserInputError.
  • null — the parameter default only applies to undefined, so request.wallet dereferences null and throws a synchronous TypeError out of a non-async arrow. expect(...).rejects never sees a promise; the test throws at argument evaluation.

Neither is caught by CI: pnpm test is test:types && test:bindings && test:client, so the client-integration project this file belongs to doesn't run on PRs. The null-request deref exists in withAccountWallet too, so if you'd rather standardise on nullish-defaults, the call is "update the test vectors" — but a raw TypeError crossing the action boundary isn't in FetchTradingApprovalsStateError, which AGENTS.md's error-union rule covers.

[issue] packages/bindings changed but has no changeset.

.changeset/dev-564-trading-approvals-state.md is @polymarket/client: minor only, while 6c401f2 edits packages/bindings/src/shared.ts. updateInternalDependencies propagates a bindings bump down to dependents, not a client bump up to bindings — so changeset version leaves bindings at 0.5.0 (already published) and it never gets republished. tsup externalises @polymarket/bindings (it's in dependencies, no noExternal), so the published client resolves the released 0.5.0 and consumers get the old z.string().transform(toEvmAddress) — the InvariantError this PR set out to fix. The Changeset job only asserts the set is non-empty, so it stays green. Adding a @polymarket/bindings: patch entry fixes it; the fix touches all 57 EvmAddressSchema call sites, so it's worth its own changelog line anyway.

Nothing else. The refine is exactly expectEvmAddress's predicate (isHexString(value) && value.length === 42), so the accepted set is unchanged and only the error type moves — no behavioural risk on the ~50 response-parsing call sites. DefaultWallet also avoids the TRequest['user'] indexed access that the DefaultAccountWallet precedent uses, which is the right direction.

@brunson-bot

Copy link
Copy Markdown
Contributor

Round 3 — both round-2 findings are closed, and the merge from main is clean. No blockers.

Invalid-input contract. decorators/wallet.ts:349-368 now guards non-object requests and switches ?? for an explicit undefined check, so all three vectors asserted at tests/integration/approvals.test.ts:86-90 behave: null and [] fall through to the action (which is async, so .rejects gets a promise) and fail z.object as an invalid type; { wallet: null } is forwarded verbatim and fails EvmAddressSchema. The Array.isArray branch is doing real work rather than being defensive breadth — without it [].wallet is undefined, so an array would silently read the authenticated wallet.

Bindings changeset. .changeset/dev-564-evm-address-validation.md is the missing piece; bindings will now republish with the schema change instead of consumers getting the old 0.5.0 transform through tsup's externalization.

Address validation. Dropping expectEvmAddress from the transform (shared.ts:528) leaves toEvmAddress as a pure brand cast, with the refine at :335-338 owning validation. Predicate is byte-identical to the old invariant, so the accepted set is unchanged, and this is worth more than the new entry point: EvmAddressSchema parses ~60 fields, mostly upstream responses, and a malformed address there used to throw a raw InvariantError straight out of safeParse, bypassing parseResponse's UnexpectedResponseError.fromZodError mapping. Those now surface as UnexpectedResponseError. The new shape also matches ConditionIdSchema, which main moved to the same refine-then-cast form in #280 — consistent with where the repo is heading.

[nit] The defaulting logic is inline where the sibling pattern is a helper. decorators/account.ts:510-518 factors the same concern into withAccountWallet, used by eight methods at :533-547. Those eight still have exactly the two behaviors fixed here — request.user ?? client.account.wallet swallows an explicit null, and a null request derefs synchronously out of a non-async arrow. Lifting this into a withDefaultWallet(client, request) next to DefaultWallet<T> would keep the decorator body a one-liner and give a follow-up one place to harden listPositions, listActivity, and the rest. Not something to fix for eight methods in this PR, but the inline form makes the divergence permanent.

[nit] Two expectEvmAddress-in-transform sites are left in the client. actions/perps.ts:952 (PerpsCredentialsSchema.proxy) and :984 (RevokePerpsCredentialsRequestSchema) are reached through parseUserInput at :1185 and :1209, so a malformed proxy still throws InvariantError rather than UserInputError — the same class this PR just closed in bindings. Worth a follow-up rather than widening scope here.

Every code check is green. The one red check is Brunson PR Review, which is my own trigger webhook timing out, not a signal about this branch.

Merge verification

c0d27de has one genuine conflict, in packages/bindings/src/shared.test.tsmain added the condition-ID describe block in the same position this PR added the EvmAddressSchema block. Resolved correctly: diffing the merge result against each parent shows both blocks present and unmodified. shared.ts and approvals.test.ts auto-merged with disjoint hunks and both sides intact, so no repeat of the clean-auto-merge-broken-file case from #274.

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.

Public API to read trading-approvals state without triggering the signing workflow

2 participants