ON HOLD: Calculate score as per "rank by surplus" CIP #2406
Conversation
|
I think it would be good to pick a block as we get close to activating rank by surplus, and communicate this block to solvers as well. Should be the "estimated" first block of the new accounting period. |
| let mut surplus = vec![]; | ||
| for trade in trades.iter() { | ||
| match trade.score(&prices, weth) { | ||
| Ok(score) => surplus.push(score), |
There was a problem hiding this comment.
As is the surplus vector will contain multiple entries for the same token and 0 surplus entries for JIT orders.
Should we aggregate those surpluses in the same token and filter out 0 amounts to remove noise or are the individual entries specifically needed for some bookkeeping?
There was a problem hiding this comment.
Even if we aggregate them at the solution level, I have to do it again at the settlement level, since score is calculated for settlement and settlement can have multiple solutions. So i figured I just do it once at the settlement level and keep vector here for potential debugging purposes.
| /// | ||
| /// The surplus is defined as the difference between the executed price and | ||
| /// the order limit price. | ||
| pub fn surplus(&self, prices: ClearingPrices) -> Result<eth::Asset, Error> { |
There was a problem hiding this comment.
| pub fn surplus(&self, prices: ClearingPrices) -> Result<eth::Asset, Error> { | |
| pub fn surplus(&self, prices: &ClearingPrices) -> Result<eth::Asset, Error> { |
Seems like we could only take a reference everywhere where we currently require ownership.
Also unrelated to this PR it's a bit awkward that the ClearingPrices are strictly related to a Trade but this is not enforce on a type level. Also the struct is pub in the crate so I think it's probably possible to skrew up and use ClearingPrices for a trade it's not related to.
Just wanted to bring it up in case we want to refactor the ClearingPrices at some point.
There was a problem hiding this comment.
The new score (actually surplus) is calculated on the raw solution received from solver, so protocol fee is not taken into consideration, as required by CIP.
I think it would be nice to compute the score based on the surplus and the fees that are being taken as this is what will eventually be needed in the autopilot (where we only observe the executions after fees).
With this design decision we would not be able to allow solvers to deal with protocol fees themselves (e.g. market maker might want to quote slightly worse prices and keep the fees as inventory rather than having the settlement contract keep them for a week).
But if this make the implementation much harder I guess this approach is more pragmatic.
I'm however not very happy with the architectural decision of creating a new public CIP28Score struct (I find both the name as well as the score duplication very unappealing from a code readability perspective). I don't think we need to extend the domain model with two different flavours of scores.
Can't we just make the score that already exist whatever it needs to be semantically given the current configuration?
| @@ -277,6 +305,13 @@ pub enum SolverScore { | |||
| Solver(eth::U256), | |||
| RiskAdjusted(f64), | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone)] | |||
| pub struct SolverScoreCIP38 { | |||
There was a problem hiding this comment.
Why does this struct need to be public?
There was a problem hiding this comment.
It's referenced in the boundary
| @@ -215,6 +215,31 @@ impl Settlement { | |||
| .into() | |||
| } | |||
|
|
|||
| /// CIP38 score denominated in the native token (ETH) | |||
| pub fn score_cip38( | |||
There was a problem hiding this comment.
Can we not keep the existing score method (instead of bloating the domain's interface) and have either a config value or some field on the auction inform us whether we are supposed to compute the surplus based on solver provided scores/revert risk or based on surplus?
There was a problem hiding this comment.
I renamed current score computation to be "old_score" while the cip38 version will be called just "score".
I did it this way (added another struct) to avoid handling a nightmare of dependencies for the SolverScore enum, and it's soo much easier to do a follow up cleanup because we would just delete the old code (and not modify the existing code and do another iteration of testing), so creating a small temporary mess is justified in this case IMO.
| let score_cip38 = SolverScoreCIP38 { | ||
| surplus: { | ||
| let mut surplus = HashMap::new(); | ||
| for trade in trades.iter() { | ||
| match trade.surplus(&prices, weth) { | ||
| Ok(eth::Asset { token, amount }) => { | ||
| surplus | ||
| .entry(token) | ||
| .or_insert(eth::TokenAmount(eth::U256::zero())) | ||
| .0 += amount.0; | ||
| } | ||
| Err(_err) => { | ||
| // todo CIP38 enable | ||
| // return Err(SolutionError::Scoring(err)) | ||
| } | ||
| } | ||
| } | ||
| surplus | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Why do we need to create the SolverScoreCIP38 struct here? What extra information that is necessary does it add? Can't we deduct it later from trades, prices and weth? And what benefit do we have from storing the list of surpluses rather than normalizing it directly into a single native token value?
There was a problem hiding this comment.
Why do we need to create the SolverScoreCIP38 struct here?
It needs to be created before the solution.with_protocol_fees() is called, because we want our score to be based on the surplus before protocol fees are taken.
And what benefit do we have from storing the list of surpluses rather than normalizing it directly into a single native token value
The function is also called for /quote where external prices vector is not available. See PR description ☝️
There was a problem hiding this comment.
Added comment explaining this.
There was a problem hiding this comment.
It needs to be created before the solution.with_protocol_fees() is called, because we want our score to be based on the surplus before protocol fees are taken.
🙈 In my opinion these kind of implicit timing assumptions make the code extremely hard to reason about and hacky. In this case we should maybe introduce a new type SettlementWithFees to make it clear that some operations aren't supported any longer?
The function is also called for /quote where external prices vector is not available. See PR description ☝️
Honestly, I'm really unhappy on how this PR changes the driver codebase which we have spent so much ressources on to get to where we are. We now have two score fields ("old score" is a really bad name imo) implicit conditional logic for quote vs solve codepaths (we secretly expect the exact same auction that we use when calling Solution:new to have native eth prices when used in to_native_score) and are introducing assumptions on the order on how things are called. This is too hacky and reminds me of the convoluted logic in the solver crate which we are trying to kill.
If computing the score before we call with_protocol_fees is really necessary (ie we cannot compute it based on the fee information we have available, which is something that has to happen at some point anyways) then I would suggest to extend the existing SolverScore to also have a Surplus variant and use some field on the auction to decide in domain::Solution whether to use the solver provided score or compute our own.
As for the quote path, I think we should either invent a native price or be fine with not computing a score in case it is absent.
There was a problem hiding this comment.
Moved PR to on hold. Will try to calculate the surplus based on the final solution with applied protocol fees which should remove the duplicate score and most of the confusion.
There was a problem hiding this comment.
Is this how you preferred to calculate the score: https://github.com/cowprotocol/services/pull/2421/files#diff-4c8f610bdbea68810a9395f71782c8a9fb5a6931e9e814a2f5f9b33bdc847e2eR101-R128
There was a problem hiding this comment.
Another thing I could try is to encode the Settlement into EncodedSettlement as part of the scoring function and implement a completely new protocol fee and surplus calculation that uses EncodedSettlement only, which should guarantee that the driver and autopilot end up with identical values?
sunce86
changed the title
|
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. |
|
Closing in favor #2448 |
Description
Calculates the new score (in the driver) as defined in CIP(38 probably).
The new score (actually surplus) is calculated on the raw solution received from solver, so protocol fee is not taken into consideration, as required by CIP. We want to rank solutions based on the raw quality they provide, while protocol fee is considered irrelevant if it is taken or not, or in which amount.
The score represents a list of surpluses, defined as
eth::Asset. That means that, for a settlement containing multiple orders, we can have surpluses in different tokens representing the score.For
/quote, there will be only one token so the surpluses of different settlements can be directly compared.For
/solve, external prices will be used to convert the different tokens into native token and summarized, so that the comparison could be made.The PR change is made in a way that the new code is implemented, while not being actively used for ranking. I thought about how to implement the actual rule switch, and for now, since we still control all the drivers, we can just have a final PR which will do a syncronized switch on both autopilot and driver (and also implement tests, or adjust existing). Still not sure if this is a good approuch, since accounting also might need to be aware from which block number the switch is made (cc @harisang maybe if you already know from the top of your head).
Opened an issue to discuss if needed: #2407
How to test
Existing tests.
Also tested locally by tweaking the existing e2e tests.