fix(clob): add side field to MakerOrder struct#14
Open
jclmnop wants to merge 1 commit intoPolymarket:mainfrom
Open
fix(clob): add side field to MakerOrder struct#14jclmnop wants to merge 1 commit intoPolymarket:mainfrom
side field to MakerOrder struct#14jclmnop wants to merge 1 commit intoPolymarket:mainfrom
Conversation
Each trade event contains a list of maker orders. These orders can have different `side` values to the taker order. The maker orders in the websocket event have a `side` field, but it's been missed in the clob client so it's not deserialized. Fixed by adding the field to the `MakerOrder` struct
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Each trade event contains a list of maker orders. These orders can have different
sidevalues (BUY/SELL) that aren't just the opposite of the taker order.The maker orders in the websocket event have a
sidefield, but it's been missed in the clob client so it's not deserialized.If using trade events to calculate changes in your own positions, it's helpful to be able to tell whether your maker order in the trade event was a BUY or SELL order.
Fixed by adding the field to the
MakerOrderstructBeen using this fix in the v1 sdk for a while now with no issues. I'm assuming it will be the same for the v2 client. Forgot to ever open a PR in the old repo because it's such a tiny fix.
By this point I must have deserialised tens of thousands of trade events, and never had any errors (or
Side::Unknown(_)variants).Note
Low Risk
Low risk: adds a missing
sidefield to WebSocket trade maker-order deserialization and updates the corresponding test fixture; no behavior changes beyond exposing additional parsed data.Overview
Trade WebSocket parsing now includes the maker order’s
sideby adding aside: Sidefield toMakerOrderinsrc/clob/ws/types/response.rs.The WebSocket integration test payload in
tests/websocket.rsis updated to include"side": "BUY"inmaker_orders, ensuring deserialization coverage for the new field.Reviewed by Cursor Bugbot for commit 184372c. Bugbot is set up for automated code reviews on this repo. Configure here.