-
Notifications
You must be signed in to change notification settings - Fork 1
[Mirror] Add price improvement protocol fee #92
Conversation
- in batch rewards query - in order rewards query (there was some divergence there)
Can you elaborate on what was the issue that was fixed here? |
@@ -110,12 +142,25 @@ order_protocol_fee_prices AS ( | |||
opf.auction_id, | |||
opf.protocol_fee, | |||
opf.protocol_fee_token, | |||
ap.price / pow(10, 18) as protocol_fee_native_price | |||
CASE |
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.
Are these used anywhere? Or did you add them for completeness purposes?
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.
They are not used anywhere. This was mostly to make it more or less a copy paste from the solver-rewards repo. Unfortunately there is some deviation anyways (e.g. the solver-rewards query does not keep order_uid
as a column till the end.
Generally I would be in favor of making the queries as close as possible. So we should imho rather add used information to solver-rewards than remove unused info here.
ELSE opf.protocol_fee | ||
END AS network_fee_correction, | ||
opf.sell_token as network_fee_token, | ||
ap_surplus.price / pow(10, 18) as surplus_token_price, |
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.
would prefer to add the native
term in the column name, e.g., surplus_token_native_price
so that there is no ambiguity about what price is this.
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.
If you think this is useful we should add it to the solver-rewards repo and copy the change here after merging over there.
I added a PR in solver-rewards on changing details of those SQL queries: cowprotocol/solver-rewards#354 |
Seems i merged it although some test was failing. Sorry, misread what was failing and thought it was for another reason. PR #93 addresses this. |
This PR mirrors the changes from cowprotocol/solver-rewards#353.
Besides introducing price improvement fees, mirroring the changes from solver-rewards fixed some issues in order rewards.