feat(bindings): type notification payloads per notification kind - #255
feat(bindings): type notification payloads per notification kind#255kartojal wants to merge 5 commits into
Conversation
brunson-bot
left a comment
There was a problem hiding this comment.
Reviewed at 22f6113. All CI green.
I checked the wire contract against the producers rather than the PR description, and the core of it holds up:
NotificationType1–10 matchesFENotificationEventTypeexactly (clob-v2 packages/fe-notifications/pkg/model/notification.go:14-24).ownerreally is the API key — the read query aliases'{ API KEY }' AS "owner"for every kind, including the two market kinds whose storedowneris''(clob-v2 packages/fe-notifications/pkg/repository/constants.go).ApiKeySchemais correct.- Order payload:
marketandcondition_idare both assignedord.Market,sideserializes as"BUY"|"SELL",typeasGTC|FOK|GTD|FAK, and the display fields have noomitempty(they are""when the market lookup misses). So theoptional()+''→undefinedhandling ontrade_id/transaction_hash/typematches the producer (clob-v2 packages/ledger/pkg/app/util.go:49-160,common/pkg/ledger/model/trade_notification.go). - Payout/redeem payloads match
RewardPayout/YieldPayout/AutoRedeemed/ComboAutoRedeemedfield-for-field, including exactly which threeAutoRedeemedfields areomitempty(notifications packages/common/pkg/model/notification.go). minimum_order_size/minimum_tick_sizeare quoted decimals upstream, soDecimalishSchemais right.minoron both packages is consistent with how #280 shipped a comparable public-type change.
Findings below, none blocking.
[issue] One unrecognized kind now discards the entire notifications list — packages/bindings/src/clob/notifications.ts:499
Before this PR the schema was type: z.number() + payload: z.unknown(), so any kind parsed. Now a discriminant outside 1–10 fails the item, and I verified on zod 4.4.3 that one failing item fails the whole z.array — fetchNotifications (packages/client/src/actions/account.ts:362) throws UnexpectedResponseError and the consumer loses all of them, not just the new one. The read query returns up to 75 rows across all ten kinds in a single array, so the blast radius is the entire feed. Upstream is explicitly a growing proto enum ("Values must stay in sync with clob-grpc's FENotificationEventType enum", plus an UNSPECIFIED = 0), and it has gone 1 → 10 already, so kind 11 is a matter of when.
Worth noting the obvious fix is the wrong one: adding a trailing { type: z.number().int(), payload: z.unknown() } union member destroys the narrowing this PR exists to provide. Verified with tsc 5.9.3 — with an open type: number member in the union, if (n.type === NotificationType.ORDER_FILL) no longer excludes it and n.payload collapses to unknown (TS2322). Tolerating at the array level instead keeps Notification precise, e.g.:
export const NotificationsResponseSchema = z
.array(z.unknown())
.transform((items) =>
items.flatMap((item) => {
const parsed = NotificationSchema.safeParse(item);
return parsed.success ? [parsed.data] : [];
}),
);That trades a silent drop for the feed staying up, which is a real tradeoff — your call on which way it should go, but I don't think all-or-nothing on a polled feed is the right default.
[issue] maker_base_fee / taker_base_fee are nullable on the wire — notifications.ts:194, :207
Both are *big.Int with no omitempty in the producing struct, so a nil serializes as null. That is not theoretical: the producer's own encoding test asserts the exact market payload and it contains "maker_base_fee":null,"taker_base_fee":null (notifications packages/common/pkg/model/notification_test.go:92). The happy path fills them from the markets service, where they're int64 (clob-v2 packages/markets/pkg/model/market.go:31-32), so I'd call this a robustness gap rather than a live break — but combined with the finding above, one null fee field takes down the whole list. .nullish() on both is the cheap fix. Everything else nullable in that struct (accepting_order_timestamp, end_date_iso, game_start_time, rewards.rates, tags) is already handled correctly.
[issue] Throwing brand transforms on this path bypass the documented error contract — notifications.ts:369 (and :261/:263, :278/:279, :347/:350, :374/:375)
ComboConditionIdSchema → toComboConditionId (shared.ts:157) throws a raw TypeError on every reject path; EvmAddressSchema/TxHashSchema → expectEvmAddress/expectTxHash → invariant → InvariantError. A throwing .transform() escapes safeParse (re-verified on zod 4.4.3), so it escapes parseResponse's UnexpectedResponseError.fromZodError mapping and crosses fetchNotifications as an error outside FetchNotificationsError — AGENTS.md rules InvariantError out of public unions. None of these ran on this path before, since payload was unknown.
The EvmAddressSchema/TxHashSchema half is a pre-existing package-wide shape, so I'd only ask for the combo one here: ConditionIdSchema (shared.ts:312) is the in-repo answer — .refine(...) plus a non-throwing cast, which surfaces as a Zod issue.
[issue] ChildCommentNotificationProfileSchema re-declares CommentProfileSchema — notifications.ts:291
gamma/comment.ts:18-34 already models this object, with the same proxyWallet → wallet rename, plus bio, profileImage, profileImageOptimized and positions. subscriptions/rtds.ts reuses it for the same producer's comment objects, so there's precedent. Every field is nullish() and z.object strips extras, so importing CommentProfileSchema is a drop-in and consumers get one profile type across comments and comment notifications instead of two that differ by four fields.
[nit] CtfConditionIdSchema is deprecated — notifications.ts:7, :68, :184, :341
It's an alias for ConditionIdSchema and picked up @deprecated in e138474 ("restore CTF condition ID aliases"), which is in this branch's merge from main (shared.ts:320-321). Identical runtime behavior, so purely cosmetic — but new code shouldn't land on the deprecated name.
[nit] NotificationTypeSchema has no consumers — notifications.ts:41
The union builds its discriminant with z.literal(type) (:403), so this export is unused. Compare PerpsNotificationTypeSchema (perps/notifications.ts:25), which is consumed at :285. Drop it or use it. (The schema itself is fine — I confirmed on zod 4.4.3 that z.enum() over a numeric TS enum rejects the reverse-mapped string keys and accepts the numbers.)
[nit] Changeset describes the type change but not the runtime one — .changeset/dev-459-typed-notification-payloads.md
Every notification used to parse regardless of kind or payload contents. After this PR the response is rejected when either doesn't match. That's the part an integrator upgrading needs to know about; worth a sentence.
Types
Notificationper DEV-459:NotificationTypeenum (all 10 wire kinds), brandedApiKeyowner, and a discriminated union tying eachpayloadshape to its notification kind. Payload shapes verified against the producing services.Linear: DEV-459
Note
Medium Risk
Stricter Zod validation and a changed public
Notificationshape can break consumers or reject previously accepted payloads at runtime.Overview
Types CLOB account notifications so
Notificationis a discriminated union on a newNotificationTypeenum instead oftype: numberwithpayload: unknown.Moves schemas into
notifications.tsand defines typed payloads for all 10 kinds (order lifecycle, market lifecycle, rewards/yield, comments, auto-redeem). Payloads normalize snake_case wire fields to camelCase, andowneris now the brandedApiKey.Re-exports
NotificationTypefrom@polymarket/clientand adds parse coverage for the new shapes.Reviewed by Cursor Bugbot for commit 22f6113. Bugbot is set up for automated code reviews on this repo. Configure here.