Skip to content

Add onchain gas cost to trades and orders APIs - #4540

Open
jmg-duarte wants to merge 16 commits into
mainfrom
jmgd/order-trade-gas-cost
Open

Add onchain gas cost to trades and orders APIs#4540
jmg-duarte wants to merge 16 commits into
mainfrom
jmgd/order-trade-gas-cost

Conversation

@jmg-duarte

@jmg-duarte jmg-duarte commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Description

Exposes gasCost (native token wei) on every endpoint that returns trades or
orders, attributing each settlement's gas cost (settlements.gas_used * settlements.effective_gas_price) to the trades and orders it settled.

Attribution rules:

  • A trade gets an equal share of its settlement's cost:
    gas_used * effective_gas_price / <trades in the settlement>.
  • An order gets the sum over its fills, but only when the cost of every
    fill is known — otherwise it reports no cost rather than an understated one.
  • Settlements without recorded gas (rows predating V116, not backfilled)
    report no gasCost.

Changes

  • gasCost on Trade and OrderMetadata in the API models and OpenAPI spec
    (the field is omitted when the cost is not known)
  • Trades, single-order, user-orders and JIT-order queries compute the
    attribution in SQL, in the same round-trip as the rest of the data
  • The trades query is restructured into a CTE so the settlement/gas join runs
    only on the returned page instead of on every candidate row

How to test

DB tests in crates/database cover the attribution: settlements with and
without 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:
Screenshot 2026-06-19 at 15 10 58

@jmg-duarte
jmg-duarte requested a review from a team as a code owner June 19, 2026 14:11
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable).

  • If creating new tables, update the tables list.
  • When adding a new index, consider using CREATE INDEX CONCURRENTLY for tables involved in the critical execution path.
  • For breaking changes, remember that during rollout k8s starts the new autopilot, runs the Flyway migration, and only then shuts down the old pod. That overlap means the previous version can still be processing requests on the migrated schema, so make it compatible first and ship the breaking DB change in the following release.

Resolved

@github-actions

Copy link
Copy Markdown

Reminder: Please consider backward compatibility when modifying the API specification.
If breaking changes are unavoidable, ensure:

  • You explicitly pointed out breaking changes.
  • You communicate the changes to affected teams (at least Frontend team and SAFE team).
  • You provide proper versioning and migration mechanisms.

Caused by:

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 4m 13s —— View job


PR Review: Add on-chain gas cost to trades and orders APIs

I reviewed the diff against origin/main. The overall approach is solid — sharing the GAS_COST_EXPR between trades and order_gas_costs so they can't drift, and moving the settlement/gas join onto the paginated page CTE (instead of every UNION branch) is a nice performance win. I verified the autopilot stores genuine receipt values (receipt.gas_used, receipt.effective_gas_price), so the "actual on-chain" framing in the OpenAPI is accurate.

I left 3 inline comments:

  1. crates/model/src/order.rs:715 — incorrect doc comment. Says the value is "derived from its quote (gas_amount * gas_price)", but it's actually the on-chain settlement gas cost. The None condition is wrong too.
  2. crates/model/src/trade.rs:34 — same incorrect doc comment, plus the "(e.g. JIT orders)" note is misleading (JIT orders do get a gas cost).
  3. database/sql/V111 — deploy-ordering caveat. The orderbook queries reference the new columns unconditionally; confirm V111 is applied before the new orderbook serves traffic.

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)

  • gas_cost uses skip_serializing_if = "Option::is_none", so the field is omitted when absent, while the OpenAPI marks it nullable: true (implying an explicit null). Consistent with other optional fields here, so likely fine — just flagging the spec/impl nuance for the FE.
  • DB tests for the rewritten trades query (cargo nextest run postgres ...) aren't runnable in this environment — worth confirming they pass, since the query restructure is substantial.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/model/src/order.rs Outdated
Comment thread crates/model/src/trade.rs Outdated
Comment thread crates/model/src/order.rs Outdated
Comment thread crates/model/src/trade.rs Outdated
Comment thread database/sql/V115__add_gas_to_settlements.sql

@MartinquaXD MartinquaXD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread crates/database/src/trades.rs Outdated
Comment thread crates/database/src/trades.rs Outdated
Comment on lines +85 to +86
WHERE s.block_number = page.block_number
AND s.log_index > page.log_index

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🚀

Comment thread crates/orderbook/src/database/orders.rs Outdated
Comment thread crates/orderbook/src/database/orders.rs Outdated
Comment thread crates/orderbook/openapi.yml Outdated
Comment thread crates/orderbook/src/database/orders.rs Outdated
Comment thread crates/database/src/trades.rs Outdated
@github-actions

This comment was marked as outdated.

@github-actions github-actions Bot added the stale label Jun 30, 2026
@jmg-duarte jmg-duarte removed the stale label Jul 2, 2026
@jmg-duarte
jmg-duarte marked this pull request as draft July 9, 2026 10:32
@github-actions

Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the stale label Jul 17, 2026
@linear-code

linear-code Bot commented Jul 22, 2026

Copy link
Copy Markdown

BE-33

@jmg-duarte jmg-duarte removed the stale label Jul 23, 2026

@jmg-duarte jmg-duarte left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying out commenting the PR myself so i can have claude address these

Comment thread crates/autopilot/src/infra/persistence/mod.rs Outdated
Comment thread crates/database/src/trades.rs Outdated
Comment thread crates/database/src/trades.rs Outdated
Comment thread crates/database/src/trades.rs Outdated
Comment thread crates/database/src/trades.rs Outdated
Comment thread crates/database/src/trades.rs
Comment thread crates/database/src/trades.rs Outdated
@jmg-duarte
jmg-duarte marked this pull request as ready for review July 23, 2026 16:53
@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 6m 2s —— View job


PR Review: Add on-chain gas cost to trades and orders APIs

I re-reviewed the current state of the branch against origin/main. This is in good shape — the doc-comment issues flagged earlier are now correctly fixed, and the SQL attribution logic is sound.

Verified correct:

  • The gas-split SQL is self-consistent. GAS_COST_EXPR's divisor (trades between the previous settlement and this one, same block) exactly matches the trade→settlement attribution in the LATERAL join, and it correctly handles multiple settlements in the same block / different txs (confirmed against the new postgres_gas_cost_attribution test — 500/500 and 1500/1500 splits, plus the un-recorded-gas → None case). This addresses @squadgazzz's request for a DB test.
  • gas_cost now propagates consistently across all order endpoints: single_order/many_orders via full_order_with_quote_into_model_order, and user_orders/orders_for_tx via the orderbook-local full_order_into_model_order (which also delegates and sets it). The shared db_order_conversions::full_order_into_model_order correctly leaves it None for the autopilot's solvable-orders path (which doesn't select the column, defaulted via #[sqlx(default)]). So the earlier "null on /account/{owner}/orders" concern is resolved.
  • Moving the settlement/gas join onto the paginated page CTE (instead of every UNION branch) is a genuine win — gas is computed only for returned rows. FULL_ORDER_WITH_QUOTE stays off the solvable-orders hot path.

Findings (2 inline):

  1. database/sql/V115 — the deploy-ordering risk is still live since this PR ships the migration and the unconditional query changes together. The orderbook reads s.gas_used before the autopilot-run migration is guaranteed applied. Please confirm the rollout ordering (or split into two releases).
  2. jit_orders.rs get_by_tx — the per-tx endpoint reports the order's lifetime gas cost (summed across all fills), which can over-count in a per-transaction view. Consistent with the field definition, flagging as a nuance for the FE.

Known limitations (equal-split can be inaccurate for mixed-complexity batches; a tx_hash column on trades would simplify this) are already acknowledged in-thread, so I won't re-litigate them. The self-notes on doc-comment conciseness/formatting are yours to tidy.

  • Read pr-review skill & gather context
  • Review the diff (git diff origin/main...HEAD)
  • Verify open review comments are addressed
  • Post inline comments for findings
  • Post summary

Comment thread database/sql/V115__add_gas_to_settlements.sql
Comment thread crates/database/src/jit_orders.rs
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

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 and others added 2 commits August 4, 2026 14:52
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>
@jmg-duarte
jmg-duarte force-pushed the jmgd/order-trade-gas-cost branch from 70a833c to 0063759 Compare August 4, 2026 14:00
@jmg-duarte
jmg-duarte marked this pull request as draft August 4, 2026 14:00
@jmg-duarte
jmg-duarte changed the base branch from main to jmgd/settlement-gas-tracking August 4, 2026 14:01
Base automatically changed from jmgd/settlement-gas-tracking to main August 5, 2026 10:41
MartinquaXD pushed a commit to kirsanium/services that referenced this pull request Aug 10, 2026
# 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>
@jmg-duarte

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 4m 6s —— View job


PR Review: Add on-chain gas cost to trades and orders APIs

Re-reviewed the current branch against origin/main.

The main long-running concern is now resolved. The V116 migration (gas_used / effective_gas_price on settlements) and the autopilot write path (update_settlement_solver_and_gas) are already merged to main — neither appears in this PR's diff. So the deploy-ordering risk flagged across earlier reviews (migration shipping together with the unconditional query reads) no longer applies: this PR only adds the read side (GAS_COST_EXPR / ORDER_GAS_COST queries, model fields, OpenAPI) on top of columns that already exist in the production schema. 👍

Verified correct:

  • All FullOrder-producing queries account for the new column — SELECT/jit_orders::SELECT append ORDER_GAS_COST, and the two hand-written paths (solvable_orders/OPEN_ORDERS and open_orders_by_time_or_uids) both select NULL AS gas_cost. No missing-column runtime failures, and the solvable-orders hot path stays free of the gas subquery.
  • The attribution SQL is self-consistent: SETTLEMENT_JOIN picks the first settlement after each trade in the same block, and the divisor counts exactly the trades between the previous settlement and this one — correctly handling multiple settlements in the same block. The LATERAL LIMIT 1 LEFT JOIN doesn't multiply rows, and dropping the settlement columns from the UNION'd page CTE doesn't change dedup (a trade is uniquely keyed by (block_number, log_index)).
  • ORDER_GAS_COST's CASE WHEN COUNT(*) = COUNT(settlement.gas_cost) correctly returns NULL for a partial sum, and NULL (not 0) for a never-filled order — both pinned by the new postgres_* tests, which are thorough (uneven splits, cross-block isolation, per-tx lifetime-sum semantics).
  • The doc-comment mismatches from the first round are fixed; model docs and OpenAPI now consistently describe an estimated per-fill share that is None/omitted unless every fill's cost is known.

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: gas_cost uses skip_serializing_if = "Option::is_none", so the JSON key is omitted when absent, whereas the OpenAPI marks it nullable: true (implying an explicit null). Harmless for JS consumers and consistent with the sibling optional fields, so I'd leave it — just flagging for anyone generating a typed client.

Acknowledged limitations (equal-split inaccuracy for mixed-complexity batches; double-counting if one tx calls settle() twice — guarded by the autopilot comment; no tx_hash column on trades; per-tx endpoint reporting order-lifetime cost) are all discussed in earlier threads, so I won't re-litigate them.

  • Read pr-review skill & gather context
  • Review the diff (git diff origin/main...HEAD)
  • Verify prior review comments are addressed
  • Confirm deploy-ordering concern (V116 already on main)
  • Post summary
    jmgd/order-trade-gas-cost

@jmg-duarte
jmg-duarte marked this pull request as ready for review August 11, 2026 12:34
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 1m 59s —— View job


PR Review: Add on-chain gas cost to trades and orders APIs

Re-reviewed the current branch against origin/main.

Verified correct:

  • No missing-column failures. Every FullOrder-producing query accounts for gas_cost: orders::SELECT and jit_orders::SELECT append ORDER_GAS_COST; the two hand-written paths (solvable_orders/OPEN_ORDERS at orders.rs:838 and open_orders_by_time_or_uids at orders.rs:967) both select NULL AS gas_cost; user_orders (order_history.rs) inherits it via the two shared SELECT consts. OrderWithQuote is a distinct struct and correctly untouched. The solvable-orders hot path stays free of the gas subquery.
  • Attribution SQL is self-consistent. SETTLEMENT_JOIN picks the first settlement after each trade in the same block, and the divisor counts exactly the trades between the previous settlement and this one — so multiple settlements in the same block / different txs split correctly. ORDER_GAS_COST's CASE WHEN COUNT(*) = COUNT(settlement.gas_cost) returns NULL on a partial sum and NULL (not 0) for a never-filled order. The LATERAL … LIMIT 1 LEFT JOIN doesn't multiply rows, and dropping the settlement columns from the UNION'd page CTE doesn't change dedup (a trade is keyed by (block_number, log_index)).
  • Tests are thorough — uneven splits (2300/3 pinned to whole wei so big_decimal_to_u256 doesn't reject it), cross-block isolation, the partial-sum → None case, full_orders_in_tx shadowed-trades-join not multiplying rows and still reporting lifetime cost, and single-vs-batch agreement.
  • Doc comments (model + OpenAPI) now consistently describe an estimated per-fill share that is None/omitted unless every fill's cost is known — the round-one mismatch is fixed.

Deploy-ordering concern resolved: the V116 migration and the autopilot write path (update_settlement_solver_and_gas) are already on main — not in this diff. This PR only adds the read side on top of columns that already exist in production, so the "migration ships with unconditional reads" risk no longer applies.

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; gas_cost omitted-vs-null under skip_serializing_if; per-tx endpoint reporting order-lifetime cost; no tx_hash on trades) are discussed in earlier threads, so I won't re-litigate them.

  • Read pr-review skill & gather context
  • Review the diff (git diff origin/main...HEAD)
  • Verify prior review comments are addressed
  • Confirm deploy-ordering (V116 already on main) & column coverage
  • Post summary
    jmgd/order-trade-gas-cost

@MartinquaXD MartinquaXD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +2472 to +2473
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread crates/database/src/trades.rs
Comment thread crates/database/src/trades.rs
")",
" SELECT t.*, settlement.tx_hash, settlement.auction_id, settlement.gas_cost",
" FROM page t",
SETTLEMENT_JOIN,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants