[streaming-quote-api] Extract reusable assemble_quote_data (2/3)#4468
Conversation
- inline estimate_all into the trait impl, elide lifetime to match PriceEstimating - assert arrival order in the streaming test so a serial impl would fail - drop em dash in doc comment
squadgazzz
changed the title
squadgazzz
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the quote assembly logic by extracting it into a dedicated function, assemble_quote_data, and adds corresponding unit tests. The review feedback highlights that the function should accept the estimate parameter by value rather than by reference to avoid unnecessary cloning of heap-allocated execution metadata, which improves performance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let quote = assemble_quote_data( | ||
| parameters, | ||
| &trade_estimate, | ||
| effective_gas_price, | ||
| sell_token_price, | ||
| }; | ||
|
|
||
| let quote_kind = quote_kind_from_signing_scheme(¶meters.signing_scheme); | ||
| let quote = QuoteData { | ||
| sell_token: parameters.sell_token, | ||
| buy_token: parameters.buy_token, | ||
| quoted_sell_amount, | ||
| quoted_buy_amount, | ||
| fee_parameters, | ||
| kind: trade_query.kind, | ||
| expiration, | ||
| quote_kind, | ||
| solver: trade_estimate.solver, | ||
| verified: trade_estimate.verified, | ||
| metadata: QuoteMetadataV1 { | ||
| interactions: trade_estimate.execution.interactions, | ||
| pre_interactions: trade_estimate.execution.pre_interactions, | ||
| jit_orders: trade_estimate.execution.jit_orders, | ||
| } | ||
| .into(), | ||
| }; | ||
| ); |
There was a problem hiding this comment.
Pass trade_estimate by value to assemble_quote_data to avoid unnecessary cloning of execution metadata.
let quote = assemble_quote_data(
parameters,
trade_estimate,
effective_gas_price,
sell_token_price,
expiration,
);References
- Prefer passing a value (by cloning at the call site) over passing a reference if the called function requires ownership and would have to clone the value internally anyway.
| execution: Default::default(), | ||
| }; | ||
|
|
||
| let data = assemble_quote_data(¶meters, &estimate, 2, 0.5, expiration); |
There was a problem hiding this comment.
Pass estimate by value in the test.
let data = assemble_quote_data(¶meters, estimate, 2, 0.5, expiration);References
- Prefer passing a value (by cloning at the call site) over passing a reference if the called function requires ownership and would have to clone the value internally anyway.
|
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. |
…ighten error-passthrough test
…oid cloning execution vecs
The update-branch merge with main kept main's plain `primitives::{Address,
U256}` import while the assemble_quote_data tests still reference AlloyU256,
breaking the shared lib-test build.
3 participants
Description
Second of three stacked PRs for the streaming quote API (#4456). Pure refactor, no behavior change. It pulls the "build one quote from one solver estimate" logic out of
compute_quote_datainto a standaloneassemble_quote_data, so the streaming path in the next PR can call it once per emitted solver result. Stacked on #4467.Changes
assemble_quote_datafromOrderQuoter::compute_quote_data. The method still computes the quote expiration itself and now delegates the field assembly to the new function.How to test
Existing tests, plus new unit tests covering the extracted function on both sell and buy sides.
Related issues
Part of #4456