Skip to content

Reuse serialized JSON auction orders - #4686

Draft
jmg-duarte wants to merge 1 commit into
mainfrom
jmgd/shared-orders
Draft

Reuse serialized JSON auction orders#4686
jmg-duarte wants to merge 1 commit into
mainfrom
jmgd/shared-orders

Conversation

@jmg-duarte

Copy link
Copy Markdown
Contributor

Description

Re-use serialized JSON orders for the /solve request, DB insert and S3 upload.

We can't re-use the full serialized auction, but we can re-use the orders array which gets quite big, cutting a full serialization section from the whole.

Changes

  • Auction becomes "CutAuction" — the old structure + JSON serialized orders
  • RawAuctionData becomes generic to keep deserialization the same
    • RawValue defers serialization until later

How to test

Tested in base staging, prod and mainnet prod

Base Prod:
image

Mainnet Prod:
image

@jmg-duarte
jmg-duarte requested a review from a team as a code owner July 30, 2026 14:25
@jmg-duarte jmg-duarte changed the title Reuse serialized JSON orders Reuse serialized JSON auction orders Jul 30, 2026
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 4m 3s —— View job


PR Review: Reuse serialized JSON orders

  • Read PR diff and understand changes
  • Review serialization/deserialization changes (both /solve and archive paths)
  • Verify all callers updated & no stray serializers
  • Post inline comments for issues found

Verdict: LGTM. Clean, well-documented, and well-tested refactor. No correctness issues found.

What I checked

  • RawValue splicing is correct. OrdersJson::serialize delegates straight to the inner RawValue, preserving serde_json's verbatim-inclusion marker, and both consumers (archive_auctionserde_json::to_vec, /solveserde_json::to_vec) serialize with serde_json, so the bytes are spliced verbatim. The orders_json_is_spliced_verbatim test nails this down. ✅
  • Both sides render the same DTO. Archive and /solve both come from dto::order::from_domain, and orders_json is built from the exact RawAuctionData.orders that later move into domain::Auction.orders, so sharing the bytes is safe. ✅
  • Generic RawAuctionData<O = Vec<Order>> + Deserialize. The serde derive bounds land on the impl, so RawAuctionData<OrdersJson> simply has no Deserialize — correct, and the read path (RawAuctionData<Vec<Order>>) round-trips (covered by archived_auction_round_trips). All callers of from_domain/archive_auction/Request::new (incl. shadow.rs) were updated. ✅
  • Panics. expect on serialization matches prior behavior (Request::new already expected); order DTOs are always JSON-serializable. ✅

Notes

  • One minor, optional memory note posted inline on run_loop.rs:362single_run retains the multi-MB orders_json for the whole run even though it's dead after fetch_solutions.
  • The orders.iter().map(from_domain).collect() conversion still runs on the async worker thread (not the blocking pool), but this matches prior behavior and is explicitly documented in OrdersJson::new, so not flagging.


// Collect valid solutions from all drivers
let solutions = self.fetch_solutions(&auction).await;
let solutions = self.fetch_solutions(&cut).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor (optional): orders_json is only needed by fetch_solutions, but single_run holds the whole cut: CutAuction — including the multi-MB serialized order buffer — until the function returns, i.e. across post_processing().await (DB writes) and settlement kickoff, even though the bytes are dead after this line.

Not a leak (it's bounded and freed at the end of each run), but since the whole point of this PR is shaving auction-sized allocations, you could drop it as soon as the request is built — e.g. pass orders_json into fetch_solutions by value instead of &cut, so the Arc is released when the solve requests are done. Feel free to ignore if the extra copy for the sub-second post-processing window isn't worth the churn.

@MartinquaXD MartinquaXD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a bit worried that this complexity and fragility is not worth the relatively modest speed ups it brings.

Comment on lines +113 to +118
let written = from_domain(&auction, OrdersJson::new(&auction.orders).await);
let json = serde_json::to_string(&written).unwrap();

let read: RawAuctionData = serde_json::from_str(&json).unwrap();
assert_eq!(read.block, 42);
assert!(read.orders.is_empty());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 comments on this test:

  1. why are the orders empty? Wouldn't it make more sense to have at least 1 order in ther?
  2. this shows in a nutshell that from_domain() is broken at the moment. Nothing guarantees that the OrdersJson you pass in actually matches the RawAuctionData::orders. This makes this code quite fragile IMO. Especially with JSON streaming and auction diffing coming up I feel like this will age super poorly. We'll keep all the complexity with very little upside since we can serialize JSON faster than the network can transfer it anyway.

@jmg-duarte

Copy link
Copy Markdown
Contributor Author

Moving to draft, probably will be closed/archived but will keep it around so the idea isn't forgotten

@jmg-duarte
jmg-duarte marked this pull request as draft August 3, 2026 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants