-
Notifications
You must be signed in to change notification settings - Fork 89
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
Store surplus capturing jit order owners in database #2796
Store surplus capturing jit order owners in database #2796
Conversation
f0efe65
to
a06ee36
Compare
crates/autopilot/src/run_loop.rs
Outdated
@@ -249,6 +249,11 @@ impl RunLoop { | |||
settlement | |||
}) | |||
.collect(), | |||
surplus_capturing_jit_order_owners: self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to contain the cow amm addresses which makes this PR a whole lot bigger. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MartinquaXD oh, definitely! but why a lot bigger? we have the indexer merged, it should be as simple as extending it with the registry no? (the same as we do when sending the addresses to the driver)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoehorning it in is of course simple. But refactoring the code such that it looks alright is more work than that.
When I looked into it I ended up moving the cow amm addresses into the domain::Auction
which meant it had to be populated in the SolvableOrdersCache
(not too unreasonable) but that then caused problems in the shadow
autopilot which would require these values in the auction returned from the orderbook. And this in turn means the orderbook and what we store in the DB also needs to be adjusted.
All of this is nothing complicated but still way more than a couple of lines PR that it is currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoehorning it in is of course simple. But refactoring the code such that it looks alright is more work than that.
I had in mind something for this which I already implemented but discarded some weeks ago. I will rescue it. No worries I don't plan to shoehorn :)
5b6ada0
to
71dd6e8
Compare
ffc3bd2
to
3503156
Compare
Folow up PR: #2799 |
crates/autopilot/src/domain/surplus_capturing_jit_order_owners.rs
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered creating a separate db table for this data? Similar to how we created auction_orders: #2745
We would like to avoid adding more stuff to SolverCompetition
, but rather carefully extract away data we need into separate strong typed tables and shrink SolverCompetition
as much as possible.
I've added the table in a separate PR to speed up things a little bit (and as it will be needed for a new autopilot surplus calculation in domain): #2810 |
# Description A prerequisite for #2796. In linked PR actual usage of this code will be implemented. Related to comment #2796 (review) Implements saving of cow amm owners to database so that the autopilot can identify JIT orders as cow amm orders. ## How to test Unit test
3503156
to
a8a4a4e
Compare
4ef0fcc
to
07a036f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but I think the surplus capturing order fetching should be part of the auction creation logic (as it directly depends on chain state it should all happen on the same block as other auction creation logic).
crates/autopilot/src/run_loop.rs
Outdated
@@ -270,6 +272,23 @@ impl RunLoop { | |||
return; | |||
} | |||
|
|||
let surplus_capturing_jit_order_owners = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refeching all jit order owners here sounds wrong. What if a new AMM was deployed after the auction was dispatched to solvers? Then it should not be saved to disk.
Aren't those part of the Auction
struct (they should IMO). As such we should just persist this field instead of fetching things again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just wanted to point this out early to avoid spending time polishing something that may receive fundamental feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it makes a lot of sense, thanks a lot for pointing it out! we were aware of it, and @sunce86 was preparing that part 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that we should fetch everything we need inside SolvableOrdersCache and store it into Auction
.
Once in Runloop fetch the owners from Auction
only.
23a7da9
to
3c5df56
Compare
@@ -528,7 +528,7 @@ components: | |||
happen, for example, if the `validTo` is too high, or no valid quote was | |||
found or generated. | |||
type: string | |||
enum: [QuoteNotFound, ValidToTooFarInFuture, PreValidationError] | |||
enum: [ QuoteNotFound, ValidToTooFarInFuture, PreValidationError ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These spaces is the standard formatting for openapi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Biased but looks good.
Removing the owner configuration (in a separate PR) seems more straight forward now.
7cb707e
to
ed260e9
Compare
Description
We need to store for each auction which jit order owners were considered to be surplus capturing for a given auction in order for the external circuit breaker to check the score properly.
Changes
surplus_capturing_jit_order_owners
tablesurplus_capturing_jit_order_owners
of the auctionsurplus_capturing_jit_order_owners
How to test
Related Issues
Fixes #2793