Skip to content
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

Merged
merged 15 commits into from
Jul 17, 2024

Conversation

m-lord-renkse
Copy link
Contributor

@m-lord-renkse m-lord-renkse commented Jul 2, 2024

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

  • Store in the surplus capturing jit order owner addresses into the surplus_capturing_jit_order_owners table
  • Extend the auction in orderbook to return the surplus_capturing_jit_order_owners of the auction
  • Extend the auction in the database (autopilot) to include the surplus_capturing_jit_order_owners

How to test

  1. Regression tests

Related Issues

Fixes #2793

@m-lord-renkse m-lord-renkse requested a review from a team as a code owner July 2, 2024 13:52
@m-lord-renkse m-lord-renkse force-pushed the 2793/store-surplus-capturing-owners-in-competition branch from f0efe65 to a06ee36 Compare July 2, 2024 14:05
@@ -249,6 +249,11 @@ impl RunLoop {
settlement
})
.collect(),
surplus_capturing_jit_order_owners: self
Copy link
Contributor

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. 😅

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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 :)

@m-lord-renkse m-lord-renkse requested a review from MartinquaXD July 3, 2024 08:42
@m-lord-renkse m-lord-renkse marked this pull request as draft July 3, 2024 10:20
@m-lord-renkse m-lord-renkse changed the title Store surplus capturing jit order owners in solver_competitions table Store surplus capturing jit order owners in solver_competitions table + remove protocol_fee_exempt_addresses Jul 3, 2024
@m-lord-renkse m-lord-renkse force-pushed the 2793/store-surplus-capturing-owners-in-competition branch 3 times, most recently from 5b6ada0 to 71dd6e8 Compare July 3, 2024 13:42
@m-lord-renkse m-lord-renkse changed the title Store surplus capturing jit order owners in solver_competitions table + remove protocol_fee_exempt_addresses (Proposal) Store surplus capturing jit order owners in solver_competitions table + remove protocol_fee_exempt_addresses Jul 3, 2024
@m-lord-renkse m-lord-renkse force-pushed the 2793/store-surplus-capturing-owners-in-competition branch 2 times, most recently from ffc3bd2 to 3503156 Compare July 3, 2024 14:47
@m-lord-renkse m-lord-renkse changed the title (Proposal) Store surplus capturing jit order owners in solver_competitions table + remove protocol_fee_exempt_addresses Store surplus capturing jit order owners in solver_competitions table Jul 3, 2024
@m-lord-renkse
Copy link
Contributor Author

Folow up PR: #2799

@m-lord-renkse m-lord-renkse marked this pull request as ready for review July 3, 2024 14:55
Copy link

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.

@github-actions github-actions bot added the stale label Jul 11, 2024
Copy link
Contributor

@sunce86 sunce86 left a 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.

@sunce86
Copy link
Contributor

sunce86 commented Jul 12, 2024

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
Let's merge this and then we can adjust this PR to save owners to auction_cow_amms table instead of to solver competition table.

sunce86 added a commit that referenced this pull request Jul 15, 2024

Verified

This commit was signed with the committer’s verified signature.
achingbrain Alex Potsides
# 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
@m-lord-renkse m-lord-renkse marked this pull request as draft July 15, 2024 17:16
@m-lord-renkse m-lord-renkse force-pushed the 2793/store-surplus-capturing-owners-in-competition branch from 3503156 to a8a4a4e Compare July 15, 2024 17:24
@sunce86 sunce86 changed the title Store surplus capturing jit order owners in solver_competitions table Store surplus capturing jit order owners in database Jul 16, 2024
@m-lord-renkse m-lord-renkse force-pushed the 2793/store-surplus-capturing-owners-in-competition branch from 4ef0fcc to 07a036f Compare July 16, 2024 08:11
Copy link
Contributor

@fleupold fleupold left a 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).

@@ -270,6 +272,23 @@ impl RunLoop {
return;
}

let surplus_capturing_jit_order_owners =
Copy link
Contributor

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.

Copy link
Contributor Author

@m-lord-renkse m-lord-renkse Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fleupold the PR is in a draft state, @sunce86 and I are still working on it 🙏

Copy link
Contributor

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.

Copy link
Contributor Author

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 🙏

Copy link
Contributor

@sunce86 sunce86 left a 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.

crates/autopilot/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/auction/mod.rs Outdated Show resolved Hide resolved
crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2793/store-surplus-capturing-owners-in-competition branch from 23a7da9 to 3c5df56 Compare July 16, 2024 09:21
@sunce86 sunce86 marked this pull request as ready for review July 16, 2024 09:58
@@ -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 ]
Copy link
Contributor Author

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.

Copy link
Contributor

@sunce86 sunce86 left a 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.

crates/autopilot/src/database/competition.rs Outdated Show resolved Hide resolved
crates/autopilot/src/run.rs Outdated Show resolved Hide resolved
crates/autopilot/src/solvable_orders.rs Outdated Show resolved Hide resolved
crates/autopilot/src/solvable_orders.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2793/store-surplus-capturing-owners-in-competition branch from 7cb707e to ed260e9 Compare July 17, 2024 08:26
crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
crates/autopilot/src/shadow.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse enabled auto-merge (squash) July 17, 2024 10:19
@m-lord-renkse m-lord-renkse merged commit 4c19299 into main Jul 17, 2024
10 checks passed
@m-lord-renkse m-lord-renkse deleted the 2793/store-surplus-capturing-owners-in-competition branch July 17, 2024 10:20
@github-actions github-actions bot locked and limited conversation to collaborators Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: Store surplus capturing jit order owners in solver_competitions table
5 participants