[streaming-quote-api] Extract reusable assemble_quote_data (2/3)#4468
[streaming-quote-api] Extract reusable assemble_quote_data (2/3)#4468squadgazzz wants to merge 2 commits into
Conversation
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.
| fn assemble_quote_data( | ||
| parameters: &QuoteParameters, | ||
| estimate: &price_estimation::Estimate, | ||
| effective_gas_price: u128, | ||
| sell_token_price: f64, | ||
| expiration: DateTime<Utc>, | ||
| ) -> QuoteData { | ||
| let (quoted_sell_amount, quoted_buy_amount, kind) = match ¶meters.side { | ||
| OrderQuoteSide::Sell { | ||
| sell_amount: SellAmount::BeforeFee { value: sell_amount }, | ||
| } | ||
| | OrderQuoteSide::Sell { | ||
| sell_amount: SellAmount::AfterFee { value: sell_amount }, | ||
| } => (sell_amount.get(), estimate.out_amount, OrderKind::Sell), | ||
| OrderQuoteSide::Buy { | ||
| buy_amount_after_fee: buy_amount, | ||
| } => (estimate.out_amount, buy_amount.get(), OrderKind::Buy), | ||
| }; | ||
|
|
||
| let fee_parameters = FeeParameters { | ||
| gas_amount: estimate.gas as f64, | ||
| gas_price: effective_gas_price as f64, | ||
| sell_token_price, | ||
| }; | ||
|
|
||
| QuoteData { | ||
| sell_token: parameters.sell_token, | ||
| buy_token: parameters.buy_token, | ||
| quoted_sell_amount, | ||
| quoted_buy_amount, | ||
| fee_parameters, | ||
| kind, | ||
| expiration, | ||
| quote_kind: quote_kind_from_signing_scheme(¶meters.signing_scheme), | ||
| solver: estimate.solver, | ||
| verified: estimate.verified, | ||
| metadata: QuoteMetadataV1 { | ||
| interactions: estimate.execution.interactions.clone(), | ||
| pre_interactions: estimate.execution.pre_interactions.clone(), | ||
| jit_orders: estimate.execution.jit_orders.clone(), | ||
| } | ||
| .into(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Change assemble_quote_data to accept estimate by value. This avoids cloning interactions, pre_interactions, and jit_orders vectors, which are heap-allocated and can be moved directly.
fn assemble_quote_data(
parameters: &QuoteParameters,
estimate: price_estimation::Estimate,
effective_gas_price: u128,
sell_token_price: f64,
expiration: DateTime<Utc>,
) -> QuoteData {
let (quoted_sell_amount, quoted_buy_amount, kind) = match ¶meters.side {
OrderQuoteSide::Sell {
sell_amount: SellAmount::BeforeFee { value: sell_amount },
}
| OrderQuoteSide::Sell {
sell_amount: SellAmount::AfterFee { value: sell_amount },
} => (sell_amount.get(), estimate.out_amount, OrderKind::Sell),
OrderQuoteSide::Buy {
buy_amount_after_fee: buy_amount,
} => (estimate.out_amount, buy_amount.get(), OrderKind::Buy),
};
let fee_parameters = FeeParameters {
gas_amount: estimate.gas as f64,
gas_price: effective_gas_price as f64,
sell_token_price,
};
QuoteData {
sell_token: parameters.sell_token,
buy_token: parameters.buy_token,
quoted_sell_amount,
quoted_buy_amount,
fee_parameters,
kind,
expiration,
quote_kind: quote_kind_from_signing_scheme(¶meters.signing_scheme),
solver: estimate.solver,
verified: estimate.verified,
metadata: QuoteMetadataV1 {
interactions: estimate.execution.interactions,
pre_interactions: estimate.execution.pre_interactions,
jit_orders: estimate.execution.jit_orders,
}
.into(),
}
}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.
| execution: Default::default(), | ||
| }; | ||
|
|
||
| let data = assemble_quote_data(¶meters, &estimate, 2, 0.5, expiration); |
There was a problem hiding this comment.
| sell_token: Address::repeat_byte(1), | ||
| buy_token: Address::repeat_byte(2), | ||
| side: OrderQuoteSide::Sell { | ||
| sell_amount: SellAmount::AfterFee { |
There was a problem hiding this comment.
For optimal test coverage there should also be case for SellAmount::BeforeFee, no?
But since is just moving code into a separate function that gets executed in basically every e2e I'd also be fine with not having any additional tests.
AFAICS the only difference is that the order kind now gets decided by the match statement instead of reading it from the trade query.
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