Skip to content
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

server/{auth,db}: add order_status route #687

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

itswisdomagain
Copy link
Member

Similar in implementation to the match_status route, the order_status route
accepts a slice of struct{Base,Quote,OrderID} and returns the status and
fill amts for the requested user-submitted orders that are found in db.

Will put up a follow up with spec changes for the new order_status route
and the updated connect route.

@chappjc
Copy link
Member

chappjc commented Sep 17, 2020

Will get on this today 100%, just a little delayed with a different fix. I'm note sure we care too much about order_status and match_status being limited to the owning user though.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review, will return to this shortly. We may need to consider queries for cancel orders too. Alternatively, client could check the status of the targeted order instead and assume the cancel order that they are interested in was the one that matched it.

dex/msgjson/types.go Outdated Show resolved Hide resolved
server/db/driver/pg/orders.go Show resolved Hide resolved
server/db/driver/pg/orders.go Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Sep 18, 2020

auth bits look good. Just need json tags, docs, and a decision about cancel orders.

@chappjc
Copy link
Member

chappjc commented Sep 18, 2020

I think we can move forward without cancel order status support at least initially. Discussion in #687

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. If you weren't planning on following up immediately with a spec update, please open an issue.

server/auth/auth.go Outdated Show resolved Hide resolved
server/db/interface.go Outdated Show resolved Hide resolved
server/db/driver/pg/orders.go Outdated Show resolved Hide resolved
@itswisdomagain
Copy link
Member Author

If you weren't planning on following up immediately with a spec update, please open an issue.

Plan is to put that up after this gets merged, while #677 is reviewed. That way, I can include this new route and the update to the connect response.

Similar in implementation to the match_status route, the order_status route
accepts a slice of `struct{Base,Quote,OrderID}` and returns the status and
fill amts for the requested **user-submitted** orders that are found in db.

Will follow up with spec changes.
server/db/driver/pg/orders_online_test.go Outdated Show resolved Hide resolved
server/db/driver/pg/orders_online_test.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, although I kinda question the need for a map in pg.orderStatuses. Eager to get moving on including active orders in the 'connect' map, and that work will provide opportunity to tweak this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants