New logic to compute scores of buy order trades#3340
Conversation
|
The new score looks correct. The reason it is extremely small is the tiny amount being traded and the extremely low native price of the buy token. Specifically: which implies that 1 unit of the buy token is worth Since the buy amount is 8050667745 atoms of the buy token, according to the native price, the buy amount is "equal" to So the score indeed should be bounded by this number. We can now directly compute it and verify it is correct. The original score is From here, we see that and so we get |
There was a problem hiding this comment.
I think the logic is correct.
We still have the equivalent autopilot code that should be updated as well: https://github.com/cowprotocol/services/blob/main/crates/autopilot/src/domain/settlement/trade/math.rs
Note that the fn score() in the autopilot is not used for anything particular yet, but other functions are. I tried to keep the code 1:1 to the driver version (with regards to function names, signatures etc) as much as I could, but your refactor in this PR diverges from that. I'd let you decide how important (keeping the code 1:1) is for further maintaining the code.
| assert_eq!(old_score.0, 19731899115084598u128.into()); | ||
|
|
||
| let new_score = trade.score(&native_prices).unwrap(); | ||
| // TODO: double check, this seems super low... |
There was a problem hiding this comment.
Probably due to the new score being bounded by the volume of the trade. The actual volume of the order was low.
| // the actual score of the solution was 19729793891650888 | ||
| // (~0.02 ETH) but our result is slightly different | ||
| // because we ignored fees. | ||
| assert_eq!(old_score.0, 19731899115084598u128.into()); |
There was a problem hiding this comment.
This PR refactors the old scoring code (although it should be equivalent to the previous one). I'm a bit concerned that all the unit tests now use the new scoring so this is the only test we have of the old scoring.
The old scoring will still be in place until the cutover, as a suggestion we could make the unit tests assert both old and new scoring to be on the safe side.
| /// Time at which the buy order scoring change should go into | ||
| /// effect (based on `deadline` in `/solve` request). | ||
| buy_order_scoring_change_cutover: chrono::DateTime<chrono::Utc>, |
There was a problem hiding this comment.
Is it a required param now that requires an infra PR?
There was a problem hiding this comment.
It was until now. The official CIP defines this date so I could use this as the default value now.
| settle_queue: mpsc::Sender<SettleRequest>, | ||
| /// If the auction deadline is later than this timestamp the score | ||
| /// for buy orders will be computed based on the latest rules. | ||
| // TODO: remove after the cutover happened |
There was a problem hiding this comment.
Maybe we should create an issue for this, so we don't forget.
| #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, From, Into)] | ||
| pub struct SurplusTokenAmount(pub U256); |
There was a problem hiding this comment.
nit: why to have from/into if we have access to the internal data (pub)?
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
6 participants
Description
Implements the necessary changes for this upcoming CIP.
I also created #3343 as a follow up PR for the same logic but on the autopilot side (eventually we should have 1 code shared across autopilot and driver).
Changes
U256with a fractionalf64intoU256Extdeadlineof a/solverequests is past the cutover dateHow to test
Added a unit test based on the order that sparked the CIP. (@harisang confirmed that the new values are correct)
Updated our e2e tests to use the new scores everywhere and verified that the new values are reasonable