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

Exclude orders with low cost coverage #200

Closed
wants to merge 2 commits into from
Closed

Conversation

josojo
Copy link
Contributor

@josojo josojo commented May 16, 2022

Hey guys

This is just a very quick handover of the current state for the task to exclude low cost coverage orders from the solvable order endpoint since I am on holiday.

I think the PR is in a reasonable state, hopefully it just needs some smoothening and polishing. It would be great if it would get a review and if then someone could actually push this PR over the finish line.

I left one todo in the code, but I am not sure it's even necessary. I think it would give a minor performance improvement that is bought with more complexity during order fetching.

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #200 (51ab720) into main (b028a87) will decrease coverage by 0.18%.
The diff coverage is 87.07%.

@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
- Coverage   64.75%   64.56%   -0.19%     
==========================================
  Files         190      189       -1     
  Lines       39033    38926     -107     
==========================================
- Hits        25275    25134     -141     
- Misses      13758    13792      +34     

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.

I would be willing to push the PR over the finish line but it feels like I'm not getting the big picture. Silently ignoring orders only because they became inconvenient for us seems shady.
Should we maybe disallow very long order validity times instead if we are afraid of bad gas cost coverage (only for orders where we want to collect a fee)?
Or publicly announce that we might ignore valid orders in certain cases?
Or maybe cancel low gas cost coverage orders with a reasonable error message such that users know what's going on and can decide on their own whether they want to submit another order with a higher gas price?

(order.clone(), fees)
});
let orders_with_fee_parameters: Vec<(Order, FeeParameters)> =
futures::future::join_all(orders_with_fee_parameters).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nicer to only make a single request to the database.
To do that the function fetching the data from the database would have to be adapted slightly:

async fn fees_of_orders(&self, uid: &Vec<OrderUid>) -> Result<HashMap<OrderUid, FeeParameters>>

Copy link
Contributor

@MartinquaXD MartinquaXD May 16, 2022

Choose a reason for hiding this comment

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

Mmhh, looks like implementing this is not trivial at the moment: sqlx faq
I tried to follow the example which led to following error which suggests this approach is probably not want we want.

"DATABASE_URL" must be set to use query macros

I believe the QueryBuilder should also enable us to fetch multiple fees per query but that seems very verbose. Will keep it in the back of my head.
Edit: Looks like this also requires us to update from sqlx 0.5.11 to 0.5.13 🙄

@MartinquaXD
Copy link
Contributor

Since I had some concerns about the this PR I did some research on how much of a difference this would make.
I used the fee analysis tool and made some changes specifically for this analysis.

While doing this investigation I stumbled across a problem with how we store fee infomation for liquidity orders. Currently all fee information entries for liquidity orders say that the order does not use any gas and it also does not record the gas price at the time of creation. This was an easy way to implement 0 fees for liquidity orders but decreases our accuracy of gas cost analyses. The analyzed time spans did not include any liquidity orders which would have been filtered out by the new PR so it wouldn't have skewed the results.

I analyzed 2 time spans:

  1. --from 14684745 --to 14694745 (Apr-30-2022 09:17:39 AM +UTC - May-01-2022 10:59:21 PM +UTC)
    This is the time around this order which sparked following discussion and resulted in this PR.

Total orders: 1359
Expensive orders: 16 / 0.12%
Total settlements: 928
Settlements including expensive orders: 7 / 0.07%
Total gas spent during that time: ~31 ETH
Total gas of expensive orders: 0.53 ETH / 1.7%
Excessive gas spent on expensive orders (exceeding 2X price): 0.34 ETH / 1.1%
analysis_specific_order.txt

  1. --from 14751599 --to 14764133 (May-11-2022 12:08:49 AM +UTC - May-13-2022 12:09:38 AM +UTC)
    This is the time span where LUNA/UST crashed which was volatile for a much longer period of time

Total orders: 1521
Expensive orders: 16 / 1.05%
Total settlements: 960
Settlements including expensive orders: 10 / 1.04%
Total gas spent during that time: ~97
Total gas of expensive orders: 1.6 ETH / 1.6%
Excessive gas spent on expensive orders (exceeding 2X price): 0.42 ETH / 0.4%
analysis_luna_ust.txt

To get the total ETH spent in a time span I used the etherbalance graph.
You can check out how volatile the gas prices were in those time spans here.

So to conclude: I still have concerns about the ethics of this PR given the fact that it wouldn't even significantly reduce our gas costs. Note that those 2 analyses were done for very volatile times and even those didn't show very promising results.

nlordell pushed a commit that referenced this pull request May 24, 2022
Supersedes #200

This PR adds new restrictions for orders, they have a maximum valid `validTo` value. The default is for 3 hours in the future (the same maximum as we have in the CowSwap FE).

This new restriction does not apply to liquidity orders (since they don't have the same issue with gas price quotes being WAY off). We also don't apply the restriction to pre-sign orders, since they are, arguably much riskier to abuse (because of the required transaction).

### Test Plan

Added test cases testing new conditions for the order validity restrictions.
@MartinquaXD
Copy link
Contributor

Superseded by #224.

@MartinquaXD MartinquaXD closed this Jun 2, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2022
@josojo josojo deleted the order-protection branch January 31, 2023 11:15
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.

3 participants