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
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);
|
AryanGodara
left a comment
There was a problem hiding this comment.
lgtm 👌🏼. verified read-side coverage + tested locally. And queried the mainnet read replica to verify no orders were mis-stored as 0
MartinquaXD
left a comment
There was a problem hiding this comment.
Change looks alright.
Could you please also extend one of the existing e2e tests to verify that the gas cost actually gets populated properly? Doesn't have to be a dedicated e2e test but having an assertion somewhere that the API actually reports that field as populated somewhere would be good.
Currently the postgres tests verify that the queries perform as one would expect if the correct DB inserts are stringed together but there is no e2e tests that actually verifies the prod code actually calls the DB function correctly.
| pub auction_id: Option<AuctionId>, | ||
| /// Share of the settlement's gas cost in native token wei, as attributed | ||
| /// by [`attribute_gas_cost`]: `NULL` until then, forever for settlements | ||
| /// observed before the column existed. `0` for a liquidity-only JIT order. |
There was a problem hiding this comment.
nit: this wording is super weird
NULLuntil then, forever for settlements
/// observed before the column existed.
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).