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.

Sign up for GitHub

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

Jump to bottom

Store auction with liquidity #2663

Merged
merged 5 commits into from
Apr 30, 2024
Merged

Store auction with liquidity #2663

merged 5 commits into from
Apr 30, 2024

Conversation

m-lord-renkse
Copy link
Contributor

Description

Store auction instances from one of the solvers which receives liquidity, similarly to how it was done for the legacy format. Auction instances from the autopilot are not enough since they do not contain liquidity.

How to test

  1. Regression tests
  2. It can be tested in staging by configuring quasidomo and checking the S3 bucket

Fixes #2633

@m-lord-renkse m-lord-renkse requested a review from a team as a code owner April 25, 2024 13:29
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch 2 times, most recently from 7ec8d62 to 8f3d94a Compare April 25, 2024 13:31
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch from 8f3d94a to 3ea4aa9 Compare April 25, 2024 13:33
m-lord-renkse
m-lord-renkse commented Apr 25, 2024
View reviewed changes
crates/autopilot/src/infra/persistence/dto/order.rs
@@ -258,7 +258,6 @@ impl From<boundary::EcdsaSignature> for domain::auction::order::EcdsaSignature {
}
}

#[serde_as]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't look like this is needed.

@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch from 3ea4aa9 to 19c0e03 Compare April 25, 2024 13:36
fleupold
fleupold reviewed Apr 25, 2024
View reviewed changes
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.

Looks pretty good to me. I would consider serialising the solver-dto object instead of creating a new one (as I believe this is what will be the main use case of this feature)

crates/driver/src/infra/persistence/dto.rs Outdated Show resolved Hide resolved
crates/driver/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/infra/solver/mod.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch 2 times, most recently from 275a33e to bbc33dc Compare April 26, 2024 11:00
@m-lord-renkse m-lord-renkse requested a review from fleupold April 26, 2024 11:24
fleupold
fleupold requested changes Apr 29, 2024
View reviewed changes
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.

Domain objects are supposed to be pure logic structs and should not implement serialisation to be passed around (that's for infra dto representations of those objects)

crates/driver/src/domain/competition/auction.rs Outdated Show resolved Hide resolved
MartinquaXD
MartinquaXD reviewed Apr 29, 2024
View reviewed changes
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.

I agree that we should not (de)serialize domain structs and instead should store the stringified dto structs.

crates/driver/src/util/bytes.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse marked this pull request as draft April 29, 2024 12:16
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch 5 times, most recently from c2f4478 to 4bbd73c Compare April 29, 2024 13:25
@m-lord-renkse m-lord-renkse marked this pull request as ready for review April 29, 2024 13:31
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch 2 times, most recently from 7cd6c9d to e4cfeff Compare April 29, 2024 16:32
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch from e4cfeff to 70fd46d Compare April 29, 2024 16:37
MartinquaXD
MartinquaXD reviewed Apr 30, 2024
View reviewed changes
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.

Almost there. Mostly nits and comments regarding DDD.

crates/driver/src/infra/persistence/mod.rs Outdated
Comment on lines 52 to 54
let Some(id) = auction_id else {
return;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Deciding that auctions without ids (i.e. quote requests) should not be uploaded seems like a domain decision to me. If archive_auction were to take a non-optional Id this would naturally be enforced by the type system in the domain logic.

Also there should be a comment somewhere explaining why we only store auctions which have an id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I'll move it to the domain logic.
I believe (I may be wrong) that auctions without id are only the ones generated by fake_auction function 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe (I may be wrong) that auctions without id are only the ones generated by fake_auction function

Yes, that's right. Those are the quote requests which don't really make sense to store.

crates/driver/src/infra/config/file/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/infra/config/file/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch from df7b257 to 0d00368 Compare April 30, 2024 07:54
MartinquaXD
MartinquaXD approved these changes Apr 30, 2024
View reviewed changes
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.

LGTM 👍

crates/driver/src/infra/solver/mod.rs Outdated Show resolved Hide resolved
squadgazzz
squadgazzz approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

LGTM

crates/driver/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2633/store-auction-with-liquidity branch from 0d00368 to 5a7f087 Compare April 30, 2024 08:20
@m-lord-renkse m-lord-renkse enabled auto-merge (squash) April 30, 2024 08:21
fleupold
fleupold approved these changes Apr 30, 2024
View reviewed changes
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.

LGTM

@m-lord-renkse m-lord-renkse merged commit a0e67c8 into main Apr 30, 2024
10 checks passed
@m-lord-renkse m-lord-renkse deleted the 2633/store-auction-with-liquidity branch April 30, 2024 09:06
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@squadgazzz squadgazzz squadgazzz approved these changes

@MartinquaXD MartinquaXD MartinquaXD approved these changes

@fleupold fleupold fleupold approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Store auction instances including liquidity
4 participants
@m-lord-renkse @fleupold @MartinquaXD @squadgazzz