Add onchain gas cost to trades and orders APIs - #4540
Conversation
|
Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable).
Resolved |
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
|
Claude finished @jmg-duarte's task in 4m 13s —— View job PR Review: Add on-chain gas cost to trades and orders APIsI reviewed the diff against I left 3 inline comments:
The doc-comment mismatches (1 & 2) read like leftovers from an earlier quote-based design — worth fixing since they directly contradict the OpenAPI and the implementation. Minor (non-blocking)
|
There was a problem hiding this comment.
Code Review
This pull request implements tracking and persistence of actual on-chain gas costs for settlements, trades, and orders, including database migrations, query updates, and OpenAPI documentation. The reviewer feedback highlights that the docstrings for the gas_cost fields in both the OrderMetadata and Trade models are incorrect and misleading, as they describe the value as an estimated cost derived from quotes rather than the actual on-chain gas cost.
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.
MartinquaXD
left a comment
There was a problem hiding this comment.
Main concern is the error prone settlement <> trade association via log indices and the fact that this estimate can be very much off. Was that discussed with the solver / frontend team?
| WHERE s.block_number = page.block_number | ||
| AND s.log_index > page.log_index |
There was a problem hiding this comment.
I think deciding to not store the tx_hash in the trades table was a blunder back in the day which leads to this ugly code again and again. Also unless I'm missing something this code does not handle trades from multiple settlements in the same block correctly.
WDYT about introducing the tx_hash column on the trades table to simply join on that? Since we can already get the association using the log index this data can even be fully backfilled using a DB migration alone.
There was a problem hiding this comment.
For what it's worth. Same-block different-tx case actually works fine imo. the LATERAL JOIN picks the right settlement per trade and the inner subquery counts the right divisor, I did a dry run for multiple settles within a block case.
The case that does mis-attribute is one tx calling settle() multiple times.
Not sure if this is applicable for now. But in theory, each Settlement event row ends up with the full receipt's gas_used (find_settlement_trace_and_callers only returns the first settle frame in the trace), so per-event splitting sums to 2x the real cost across the tx's trades.
No solver does this afaik, but tx_hash on trades sidesteps the question entirely. So +1 on the direction 🚀
This comment was marked as outdated.
This comment was marked as outdated.
|
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. |
jmg-duarte
left a comment
There was a problem hiding this comment.
trying out commenting the PR myself so i can have claude address these
|
Claude finished @jmg-duarte's task in 6m 2s —— View job PR Review: Add on-chain gas cost to trades and orders APIsI re-reviewed the current state of the branch against Verified correct:
Findings (2 inline):
Known limitations (equal-split can be inaccurate for mixed-complexity batches; a
|
|
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. |
The settlement observer already reads `gas_used` and `effective_gas_price` from the transaction receipt to build its metrics. Store them on the `settlements` row (columns added in V116) so that consumers can attribute a real on-chain gas cost to settlements. Only settlements observed after this is deployed get values, historical rows stay `NULL`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Expose the settlement gas cost attributed to each fill as `gasCost` (native token wei) on the trades and order-detail endpoints: - a trade gets its settlement's `gas_used * effective_gas_price` split equally across the trades settled in the same transaction - an order gets the sum over all of its fills `null` when the underlying settlement has no recorded gas, i.e. settlements observed before V116 was deployed. Both values are computed in SQL so they come back in the same round-trip as the rest of the order/trade data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
70a833c to
0063759
Compare
# Description cowprotocol#4540 requires that the migration is done first to avoid issues when upping it in K8s This PR adds the settlement gas info columns to the settlement table. # Changes * Adds `gas_used` column to `settlement` table * Adds `effective_gas_price` to `settlement` table --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
@claude review |
|
Claude finished @jmg-duarte's task in 4m 6s —— View job PR Review: Add on-chain gas cost to trades and orders APIsRe-reviewed the current branch against The main long-running concern is now resolved. The V116 migration ( Verified correct:
No new blocking findings — I'm not posting inline comments because I didn't find a change-worthy defect. One minor nuance already noted in-thread and worth keeping in mind: Acknowledged limitations (equal-split inaccuracy for mixed-complexity batches; double-counting if one tx calls
|
|
Claude finished @jmg-duarte's task in 1m 59s —— View job PR Review: Add on-chain gas cost to trades and orders APIsRe-reviewed the current branch against Verified correct:
Deploy-ordering concern resolved: the V116 migration and the autopilot write path ( No new blocking findings — not posting inline comments since I didn't find a change-worthy defect. Already-acknowledged nuances (equal-split inaccuracy for mixed-complexity batches;
|
MartinquaXD
left a comment
There was a problem hiding this comment.
Just wanted to point out one more edge case. AFAICS this treats all trades equally when they are not.
Some solvers provide liquidity using JIT orders. So if you have a settlement with 1 user order and 1 JIT order that is just there for market making the cost of the user order would be half as much as it should be.
That being said I think getting this detail correct is not really worth is as we'd have to involve 2 more tables to figure out if an order should actually split the gas or not.
Before approving this I'd like you to compare the runtime of a the old and the new trades query. We wouldn't want to cause a regression here.
| // order_a fills again, alone in a settlement of 300 gas, so it takes all | ||
| // 3000 of that one on top of its first 500. |
There was a problem hiding this comment.
You are probably aware but pointing it out just in case:
This poses an interesting question for long lived partially fillable orders. While we are able to compute the total ETH cost over all fills it will still be hard to get a reasonable human readable cost out of this because the price of ETH can fluctuate. Or is the plan to just leave this denominated in ETH in the explorer?
There was a problem hiding this comment.
Or is the plan to just leave this denominated in ETH in the explorer?
That's actually still being debated
| pub gas_cost: Option<BigDecimal>, | ||
| } | ||
|
|
||
| /// Join resolving, for each row of a `trades t` alias, the settlement that |
There was a problem hiding this comment.
If this is supposed to be spliced into other queries wouldn't it make more sense to directly refer to trades instead of relying on all parent queries using the same alias for the trades table?
| ")", | ||
| " SELECT t.*, settlement.tx_hash, settlement.auction_id, settlement.gas_cost", | ||
| " FROM page t", | ||
| SETTLEMENT_JOIN, |
There was a problem hiding this comment.
I'm a bit worried about the change to this query. Making this fast on all networks was not trivial and it's unclear to me how this would affect its performance.
Description
Exposes
gasCost(native token wei) on every endpoint that returns trades ororders, attributing each settlement's gas cost (
settlements.gas_used * settlements.effective_gas_price) to the trades and orders it settled.Attribution rules:
gas_used * effective_gas_price / <trades in the settlement>.fill is known — otherwise it reports no cost rather than an understated one.
report no
gasCost.Changes
gasCostonTradeandOrderMetadatain the API models and OpenAPI spec(the field is omitted when the cost is not known)
attribution in SQL, in the same round-trip as the rest of the data
only on the returned page instead of on every candidate row
How to test
DB tests in
crates/databasecover the attribution: settlements with andwithout recorded gas, multiple settlements in one block, and orders with a
mix of known and unknown fill costs.
Sepolia staging is currently suspended with this code there, please do a trade and review the UI using this link
https://jmgd-ucp.explorer-dev-dxz.pages.dev/
Under costs and fees you should see something like:
