Move settlement ranking logic into separate component #432
Conversation
|
|
||
| /// Appends a merged settlement if multiple simple settlements canlegally be merged into a | ||
| /// single more efficient settlement. | ||
| fn append_merged_settlement( |
There was a problem hiding this comment.
One refactor that I think would be useful here, would be to push settlement merging to the SingleOrderSolver wrapper type (i.e. the type that turns an impl SingleOrderSolver into an impl Solver).
The reason I suggest this is that, eventually, with the solver+driver co-location, each solver+driver will present a single settlement (so would have to have already shuffled and merged single order trades and simulated and ranked them to pick the best one).
There was a problem hiding this comment.
Great suggestion!
Making the merge part of the settlement ranking didn't feel right anyway.
There was a problem hiding this comment.
This also allows our trait Solver to return a single settlement instead of a Vec<Settlement> and push merging logic only to single order solvers.
There was a problem hiding this comment.
Refactoring the merge into the SingleOrderSolver would blow up this PR. I will do that refactor first and then rebase the SettlemenRanker on top of that.
This also allows our trait Solver to return a single settlement instead of a Vec and push merging logic only to single order solvers.
I'm a bit hesitant about this one. Although I would love simplifying the interface if we only return the merged settlement a single flaky single-order-settlement could result in the solver not competing (if it causes the merged settlement to revert for some reason).
There was a problem hiding this comment.
Refactoring the merge into the SingleOrderSolver would blow up this PR. I will do that refactor first and then rebase the SettlemenRanker on top of that.
Oh, I assumed it would be a separate PR. Sorry, could have been clearer on my initial comment.
There was a problem hiding this comment.
flaky single-order-settlement could result in the solver not competing
Shouldn't it be doing its simulations for the merging? I assumed that it would simulate the various proposed merged settlements and pick the best one from those.
There was a problem hiding this comment.
Shouldn't it be doing its simulations for the merging?
Currently the simulation happens only after some invariants of the settlements have been checked and we added the merged settlement.
I assumed that it would simulate the various proposed merged settlements and pick the best one from those.
The current merging logic generates at most 1 additional merged settlement and determines if a merge is possible by checking settlement invariants of the merged settlement.
There was a problem hiding this comment.
could result in the solver not competing
We should still do it. Currently single order solvers gain an unfair advantage compared to other solvers because the driver does the merging for them. It should be something for solvers to decide how big they want to make their settlements.
There was a problem hiding this comment.
We should still do it.
So, I took a look at the other PR and agree that we don't necessarily need to change the trait Solver to return a single Settlement for now.
Once we finish co-locating Driver+Solver, we will automatically get this (since each Driver+Solver combo will only get a single settlement to propose). I think we can wait to refactor some of these abstractions as we finish the colocation logic instead of trying to do it all now.
That being said, I do agree with @vkgnosis and think we should still do it at some point. The merge_settlements will be a no-op for all Driver+HTTPSolver instances we have, so it seems like logic that is specific to SingleOrderSolver implementations, and should make its way there eventually.
MartinquaXD
force-pushed
the
settlement-rater
branch
from
7a4185b
to
00038aa
Compare
MartinquaXD
changed the base branch from
main
to
merge-settlements-in-single-order-solver
MartinquaXD
force-pushed
the
settlement-rater
branch
from
00038aa
to
99a6dcf
Compare
Codecov Report
@@ Coverage Diff @@
## main #432 +/- ##
==========================================
- Coverage 63.40% 63.37% -0.04%
==========================================
Files 230 231 +1
Lines 44717 44746 +29
==========================================
+ Hits 28355 28358 +3
- Misses 16362 16388 +26 |
Related to step 5 of #337
The process of verifying settlements, determining viability (by simulating them) and finding a winner is important to get right and will have to happen at multiple places (current driver,
CommitRevealSolver, reveal stage of new driver).To reduce the risk of introducing errors I moved the existing logic for that into the new
SettlementRankerand made the current driver use it.One thing which I'm not certain of yet is how to deal with the metrics in the new component. For now I will keep using the
SolverMetrics(to keep changes minimal and not disrupt grafana) but eventually this should probably be refactored to use the macro based approach.A follow up PR will replace the current logic for that in the
CommitRevealSolverand introduce the required CLI arguments.Test Plan
No logic changes so hopefully CI and careful reviews should be enough. 🤞