-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat(subscribeorders.ts): SubscribeAddedOrders should have existing
true by default
#730
Conversation
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.
Skimmed through the code, looks like
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.
api.md fine now. Rest needs to be reviewed & approved by either @sangaman or @moshababo
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.
#728 is not about adding a limit to the order subscription calls. Also, the way this is implemented it will return an arbitrary set of orders, which does not seem useful at all.
@sangaman How should I decide which orders should be returned? |
existing
true by default
I just don't think we should be returning a subset of orders, and that's not the problem highlighted in the issue, so I think we should stick to returning all orders up front when |
@ImmanuelSegol Will you be able to fix this typo soon? I think it makes sense to merge this before the next release and obviously the change is very minor, but if you're busy I can handle it. |
Closes #728