-
Notifications
You must be signed in to change notification settings - Fork 112
Feat/extend delta orders filtering #215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/deltaPrice_struct_change
Are you sure you want to change the base?
Feat/extend delta orders filtering #215
Conversation
size-limit report 📦
|
* Filter by any known DeltaAuctionStatus and some custom statuses: | ||
* - **INSUFFICIENT_BALANCE** — returned as SUSPENDED from API | ||
* - **INSUFFICIENT_ALLOWANCE** — returned as SUSPENDED from API | ||
* - **INVALIDATED** — returned as FAILED from API | ||
* - **ACTIVE** — All orders with NOT_STARTED, RUNNING, EXECUTING or SUSPENDED statuses. | ||
* - **INACTIVE** — All orders with EXECUTED, FAILED, EXPIRED, CANCELLED or INVALIDATED statuses. | ||
*/ | ||
status?: DeltaOrderFilterByStatus[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like SDK accepting all statuses even those that are never returned because they are internal to API.
It's very useful to query for SUSPENDED and API will return all Orders with INSUFFICIENT_*
On the other hand I do want to keep the option to query for very specific statuses, so maybe your approach with a comprehensive jsdoc comment is the best.
chainId?: number; // @TODO currently not working | ||
/** @description Filter by type. MARKET, LIMIT, or ALL. Default is ALL */ | ||
type?: 'MARKET' | 'LIMIT' | 'ALL'; | ||
chainId?: number[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chainIds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not always require an array? Would be cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chainIds?
I'd like to keep it the same as API does. chainId=
, status=
Why not always require an array? Would be cleaner
Maybe, but why would I pass an array with a single value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass as array, because it's simpler than a variable type. We have it in a bunch of places: include/exclude methods, dexs, bridges -- off the top of my head
55956cf
to
d3e5555
Compare
const chainIdString = options.chainId | ||
? options.chainId.join(',') | ||
: undefined; | ||
const statusString = options.status ? options.status.join(',') : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can probably do options.chainId?.join()
getDeltaOrders:
chainId
andstatus
filterstype=ALL
filter option