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

Add auction database functions to orderbook and autopilot #436

Merged
merged 2 commits into from
Aug 10, 2022
Merged

Conversation

vkgnosis
Copy link
Contributor

They will be used soon in another PR that makes the autopilot store auctions in the database and the orderbook read them from the database.

Test Plan

CI, actual db logic is already tested in database crate

@vkgnosis vkgnosis requested a review from a team as a code owner August 10, 2022 10:48
}

impl Postgres {
pub async fn solvable_orders(&self, min_valid_to: u32) -> Result<SolvableOrders> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is copied from the orderbook crate where it can be removed when the autopilot takes over.

Comment on lines +79 to +80
fn full_order_into_model_order(order: database::orders::FullOrder) -> Result<Order> {
let status = OrderStatus::Open;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from the orderbook crate. I'm planning on eventually changing the Order type that is used in the autopilot and in the auction endpoint to contain only information relevant to solvers so that we don't need this anymore.


let data = serde_json::to_value(&auction)?;
let mut ex = self.0.begin().await?;
database::auction::delete_all_auctions(&mut ex).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's okay to delete all the auctions here because they will end up in the settlement competition table eventually, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also found the fact that we can delete all auctions surprising.

I know that we store he settlement competition information in the database, and it includes order UIDs that were included in the auction - but maybe it does make sense to also include this additional data (because things like available balance, executed amounts, etc. get lost).

Copy link
Contributor Author

@vkgnosis vkgnosis Aug 10, 2022

Choose a reason for hiding this comment

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

At the moment this table just facilitates communicating the auction from the autopilot to the orderbook and giving a persistently increasing id to the autopilot. The settlement competition table still exists and the way it works isn't changed in this PR. That means there is no reason to persist these auctions.

Eventually I could see us combining the tables in some way but I'm not sure about the details yet.

Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Just a question about deleting the auctions.

@cowprotocol cowprotocol deleted a comment from codecov-commenter Aug 10, 2022
Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

Code looks good.

Just one question about the "lifetime" of the data in the auctions table.

@vkgnosis
Copy link
Contributor Author

Just one question about the "lifetime" of the data in the auctions table.

I don't see this question unless you mean Martin's question.

@vkgnosis vkgnosis enabled auto-merge (rebase) August 10, 2022 12:48
@codecov-commenter
Copy link

Codecov Report

Merging #436 (5a7abc4) into main (ab979c6) will decrease coverage by 0.19%.
The diff coverage is 0.00%.

❗ Current head 5a7abc4 differs from pull request most recent head c8363bf. Consider uploading reports for the commit c8363bf to get more accurate results

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
- Coverage   63.59%   63.40%   -0.20%     
==========================================
  Files         228      230       +2     
  Lines       44586    44703     +117     
==========================================
- Hits        28355    28343      -12     
- Misses      16231    16360     +129     

@vkgnosis vkgnosis merged commit de9dac2 into main Aug 10, 2022
@vkgnosis vkgnosis deleted the db-auction branch August 10, 2022 12:52
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2022
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.

4 participants