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

Add quoteId parameter to orders #231

Merged
merged 1 commit into from
May 30, 2022
Merged

Add quoteId parameter to orders #231

merged 1 commit into from
May 30, 2022

Conversation

ahhda
Copy link
Contributor

@ahhda ahhda commented May 27, 2022

Partially fulfills #170

This PR adds the ability to set the quoteId parameter while creating Orders. This way the order can be linked to a quote and enable providing more metadata when analyzing order slippage.

Currently, it doesn't involve interaction with the Database i.e. Storing or retrieving the quoteId. This will be a part of a separate PR.

Also api/v1/quote now also returns an ID. Currently, implemented as a counter which gets reset on orderbook restarts.

Screenshot

Screenshot 2022-05-27 at 7 21 57 PM

Test Plan

  • Use the custom-order-ui to place a new order and add the Quote ID parameter
    Screenshot 2022-05-27 at 7 08 24 PM

  • Review the logs to see quoteId visible
    Screenshot 2022-05-27 at 7 09 02 PM

Release notes

Users can now pass an optional parameter quoteId while creating orders for easier debugging.

@ahhda ahhda requested a review from a team as a code owner May 27, 2022 13:40
@nlordell
Copy link
Contributor

Also might be worth adding to the PR description that api/v1/quote now also returns an ID (that is internally just implemented as a counter so gets reset on orderbook restarts).

@@ -0,0 +1,126 @@
use crate::{
Copy link
Contributor

Choose a reason for hiding this comment

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

It is worth nothing that this code moved from the orderbook to the model crate as its part of the "API models" (along with others like order creation payload type) that we want to keep in a shared crate.

Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Great PR description; love it!

I'm just a little worried to allow a user of our API to provide the ID. I guess probably only the cowswap UI will use that functionality for now but could other integrators cause troubles if they simply provide the wrong ID (knowingly or by accident)?
Could we maybe recreate the specifics of a quote via the FeeParameters? AFAIK while validating an order (before we add it to the orderbook) we look for a FeeParameters entry which seems reasonable for that particular order. Could that association be used for the same purpose as the new ID without relying on integrators to supply the correct ID?

Sorry for voicing these concerns so late. They should rather have been part of the issue. This PR implements what the issue requires so if my concerns are not valid this PR is good to go imo.

@nlordell
Copy link
Contributor

Sorry for voicing these concerns so late. They should rather have been part of the issue. This PR implements what the issue requires so if my concerns are not valid this PR is good to go imo.

These are super valid concerns - and definitely the plan. These should be solved once we tackle:

Currently, it doesn't involve interaction with the Database i.e. Storing or retrieving the quoteId. This will be a part of a separate PR.

I will add more context to the issue to make this more explicit.

@alfetopito
Copy link
Contributor

Do we expect the id to remain an integer or will at a later point become a string/base16/whatever later?

Just to understand if I should make the json schema validation for the metadata more strict or not

https://github.com/cowprotocol/cow-sdk/blob/47527e56747e0eda78e6dfd20fd02a4355afb3f4/src/schemas/appData.schema.json#L84-L88=

@nlordell
Copy link
Contributor

Do we expect the id to remain an integer

I think so. Maybe it should be unknown to make it explicit that its an opaque ID whose exact semantics are undefined.

@alfetopito
Copy link
Contributor

Do we expect the id to remain an integer

I think so. Maybe it should be unknown to make it explicit that its an opaque ID whose exact semantics are undefined.

Ok, I'll keep it as a generic string field for the JSON schema then since unknown would apply only to TS AFAIAA

@nlordell
Copy link
Contributor

I'll keep it as a generic string field for the JSON schema

I think its an integer at the moment FWIW.

@ahhda ahhda merged commit 6581af3 into main May 30, 2022
@ahhda ahhda deleted the add-quoteid branch May 30, 2022 14:41
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants