refactor quoting interfaces to work with full competition - #4714
refactor quoting interfaces to work with full competition#4714MartinquaXD wants to merge 3 commits into
Conversation
|
Claude finished @MartinquaXD's task in 3m 29s —— View job PR Review: refactor quoting interfaces to work with full competitionI reviewed the diff against I verified the two trickiest pieces:
Two minor points posted as inline comments (both non-blocking):
The DB-side Nothing blocking from my side. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
851e5a7 to
23d1393
Compare
4007722 to
558df31
Compare
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
LGTM! The separation using to_final_quote makes sense to me.
| pub auction_id: Option<AuctionId>, | ||
| } | ||
|
|
||
| /// Collection of data that describes the full quote comptition (request, |
There was a problem hiding this comment.
| /// Collection of data that describes the full quote comptition (request, | |
| /// Collection of data that describes the full quote competition (request, |
AryanGodara
left a comment
AryanGodara
left a comment
There was a problem hiding this comment.
again, just some questions/doubts, Otherwise gg 👌🏼
| let row = create_quote_row(data)?; | ||
| let row = create_quote_row(&data)?; | ||
| let id = database::quotes::save(&mut ex, &row).await?; | ||
| // TODO populate `competition_auctions`, `proposed_solutions`, |
There was a problem hiding this comment.
you're already working on this PR right? 👀 (confirming that it's not meant to stay as a TODO for long)
| verified: data.verified, | ||
| metadata: data.metadata.try_into()?, | ||
| auction_id: None, | ||
| auction_id: competition.metadata.auction_id, |
There was a problem hiding this comment.
this is a change to how DB is filled. But yeah, techincally still an interface change, that's in TODO
| // Validate the fee doesn't exceed the requested input by deriving the | ||
| // final quote once. Callers can re-derive it later via | ||
| // `QuoteCompetition::to_final_quote`. | ||
| let _ = competition.to_final_quote(¶meters)?; |
There was a problem hiding this comment.
this function builds the final quote just to surface the fee error and drops it; each caller rebuilds it + sets the id. Could it hand back the assembled Quote to kill the double-compute? (Or is this it trivial to create)
| metadata: quote.data.metadata.try_into()?, | ||
| auction_id: None, | ||
| metadata: quote.data.metadata.clone().try_into()?, | ||
| auction_id: quote.data.auction_id, |
There was a problem hiding this comment.
This propagates auction_id into order_quotes for the onchain orders' path, but fast-path orders come in through the regular API/DB route, (ie, orderbook/database/orders.rs:209), which still sets this to None. And since model::OrderQuote has no auction_id field, it's stuck at None. So the orders that actually get an auction_id won't persist it here. Is wiring that through OrderQuote + the regular path the next PR? (i think this is in line with this comment on #4712)
3 participants
Description
In order to actually store a full quote competition in the DB as if it were a regular auction we need to update the quoting related types and interfaces. Actually writing the competition data to the DB will happen in a follow up PR.
Changes
QuoteCompetitionwhich contains all quotes and only returns the new quote id. If a full quote + id is needed the caller can assemble it themselvesQuoteCompetition- will become relevant in the follow up PRHow to test
only an interface change - all tests should still pass
Related Issues
BE-56