Move settlements merging into SingleOrderSolver #434
Conversation
| @@ -1,4 +1,5 @@ | |||
| use crate::{ | |||
| driver::solver_settlements::merge_settlements, | |||
There was a problem hiding this comment.
Should we move this function to something like mod single_order_solver::merge?
Fine where it is as well.
There was a problem hiding this comment.
The single order solver currently doesn't live in its own directory. If I'm not mistaken we would have to either create that directory structure (making git blame less effective) or let the merge file float around in the /solver directory.
There was a problem hiding this comment.
I'm fine either way, just thought I should ask (because where the code was used moved drastically, I thought it might make sense to move the code). No strong opinions though.
There was a problem hiding this comment.
It definitely is a valid point.
I believe when we are eventually happy with the new driver we probably should move around a bunch of code from the old driver and solver crates so maybe that would be a good time to do it.
I will merge for now and keep that in mind.
Codecov Report
@@ Coverage Diff @@
## main #434 +/- ##
==========================================
+ Coverage 63.58% 63.97% +0.38%
==========================================
Files 228 228
Lines 44572 44266 -306
==========================================
- Hits 28343 28317 -26
+ Misses 16229 15949 -280 |
Merging multiple single order settlements in the driver is a bit awkward. Instead it should be the responsibility of the
SingleOrderSolverbecause those solvers are the only ones that need merged settlements anyway.To make it a bit nicer to create
SingleOrderSolvers generically (and more inline with what we do for price estimators) I also made it use dynamic dispatch instead of static dispatch.Initially discussed here
Test Plan
Only moved logic around => CI
updated test asserting the number of generated settlements