Add onchain gas cost to trades and orders APIs - #4540
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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?
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
This comment was marked as resolved.
This comment was marked as resolved.
|
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. |
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.
11a3ad9 to
7b3c37a
Compare
7b3c37a to
7ef256b
Compare
|
Due to the new signing process I had to do quite a broken rebase, as such I just squashed the whole PR Apologies dear colleagues, complain to devops 😂 |
|
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. |
486bbc7 to
8e6bae8
Compare
|
All contributors have signed the CLA ✍️ ✅ |
|
Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable).
Resolved |
8e6bae8 to
486bbc7
Compare
|
Claude finished @jmg-duarte's task in 3m 9s —— View job PR Review: Add on-chain gas cost to trades and orders APIsRe-reviewed the current (squashed) branch against Verified correct:
No new blocking findings, so I'm not posting inline comments. Already-acknowledged nuances I won't re-litigate: equal-split ignores per-trade complexity and JIT-liquidity dilution (@MartinquaXD);
|
Description
Exposes
gasCost(native token wei) on every trades and orders endpoint.The value is read from
trades.gas_cost(V121, onmain), written by the autopilot when it post-processes a settlement:gas_used * effective_gas_pricesplit equally between that settlement's user trades; liquidity-only JIT trades get0. A trade reports its share. An order reports the sum over its fills, omitted unless every fill's share is known. The field is absent until a settlement is attributed (shortly after indexing) and for settlements observed before V121 (no backfill).Known limitations (see thread): the equal split ignores per-trade complexity, and
/transactions/{tx}/ordersreports an order's lifetime total.Changes
gasCostonTradeandOrderMetaData(models, OpenAPI); key omitted when unknown.tradesquery:t.gas_costadded to the select list, shape unchanged.orders::SELECT/jit_orders::SELECT: all-or-nothing sum subquery overtrades. Solvable-orders queries selectNULL, so the autopilot hot path is unchanged.U256conversion fails on non-integer or negative values instead of reporting them as unknown.Verification
Old (
main) vs new (this branch) query on the mainnet read replica. Each comparison isto_jsonb(old) EXCEPT ALL to_jsonb(new) - 'gas_cost'in both directions; every case returned 0 rows each way.user_orderstradestradestrades: old vs new, owner 0x89b537d4…, limit 1000, offset 0user_orders: old vs new, owner 0x89b537d4…, limit 1000, offset 0Timing, warm cache:
single_full_order_with_quote, order with 405 fillsuser_orders, limit 1000, 5 heavy ownerstrades, owner with 150k fills, limit 1000Cold reads did not increase (the
tradesheap is resident). Possible follow-up: fold the fourtradessubqueries inorders::SELECTinto one aggregate.How to test
Run the DB tests (needs
docker compose up -d):New tests:
postgres_trades_report_attributed_gas_cost(two settlements in one block, one attributed),postgres_order_gas_cost_across_fills(sum across fills, unknown once a fill is unattributed,Nonefor an unfilled order),postgres_user_orders_correctness(both union arms, liquidity-only JIT order reports0).