Skip to content

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

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

Add onchain gas cost to trades and orders APIs#4540
jmg-duarte wants to merge 7 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 trades and orders endpoint.

The value is read from trades.gas_cost (V121, on main), written by the autopilot when it post-processes a settlement: gas_used * effective_gas_price split equally between that settlement's user trades; liquidity-only JIT trades get 0. 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}/orders reports an order's lifetime total.

Changes

  • gasCost on Trade and OrderMetaData (models, OpenAPI); key omitted when unknown.
  • trades query: t.gas_cost added to the select list, shape unchanged.
  • orders::SELECT / jit_orders::SELECT: all-or-nothing sum subquery over trades. Solvable-orders queries select NULL, so the autopilot hot path is unchanged.
  • U256 conversion 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 is to_jsonb(old) EXCEPT ALL to_jsonb(new) - 'gas_cost' in both directions; every case returned 0 rows each way.

Query Parameters Rows Old only New only
user_orders 5 heavy owners, limit 1000 514-1000 0 0
trades owner with 150k fills, limit 1000, offsets 0 and 5000 1000 0 0
trades order with 405 fills; no filter, limit 1000 405 / 1000 0 0
trades: old vs new, owner 0x89b537d4…, limit 1000, offset 0
WITH old_query AS (
(
SELECT
    t.block_number,
    t.log_index,
    t.order_uid,
    t.buy_amount,
    t.sell_amount,
    t.sell_amount - t.fee_amount as sell_amount_before_fees,
    o.owner,
    o.buy_token,
    o.sell_token,
    settlement.tx_hash,
    settlement.auction_id FROM trades t
LEFT OUTER JOIN LATERAL (
    SELECT tx_hash, auction_id FROM settlements s
    WHERE s.block_number = t.block_number
    AND   s.log_index > t.log_index
    ORDER BY s.log_index ASC
    LIMIT 1
) AS settlement ON true
JOIN orders o ON o.uid = t.order_uid
WHERE ('\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea IS NULL OR substring(t.order_uid, 33, 20) = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea) AND (NULL IS NULL OR t.order_uid = NULL)
ORDER BY t.block_number DESC, t.log_index DESC
LIMIT 1000 + 0)
UNION (
SELECT
    t.block_number,
    t.log_index,
    t.order_uid,
    t.buy_amount,
    t.sell_amount,
    t.sell_amount - t.fee_amount as sell_amount_before_fees,
    o.owner,
    o.buy_token,
    o.sell_token,
    settlement.tx_hash,
    settlement.auction_id FROM trades t
LEFT OUTER JOIN LATERAL (
    SELECT tx_hash, auction_id FROM settlements s
    WHERE s.block_number = t.block_number
    AND   s.log_index > t.log_index
    ORDER BY s.log_index ASC
    LIMIT 1
) AS settlement ON true
JOIN orders o ON o.uid = t.order_uid
JOIN onchain_placed_orders onchain_o ON onchain_o.uid = t.order_uid
WHERE ('\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea IS NULL OR onchain_o.sender = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea) AND (NULL IS NULL OR t.order_uid = NULL)
ORDER BY t.block_number DESC, t.log_index DESC
LIMIT 1000 + 0)
UNION ( WITH jit AS MATERIALIZED (   SELECT uid, owner, buy_token, sell_token   FROM jit_orders   WHERE ('\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea IS NULL OR owner = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea)   AND (NULL IS NULL OR uid = NULL))
SELECT
    t.block_number,
    t.log_index,
    t.order_uid,
    t.buy_amount,
    t.sell_amount,
    t.sell_amount - t.fee_amount as sell_amount_before_fees,
    o.owner,
    o.buy_token,
    o.sell_token,
    settlement.tx_hash,
    settlement.auction_id FROM jit o
JOIN trades t ON o.uid = t.order_uid
LEFT OUTER JOIN LATERAL (
    SELECT tx_hash, auction_id FROM settlements s
    WHERE s.block_number = t.block_number
    AND   s.log_index > t.log_index
    ORDER BY s.log_index ASC
    LIMIT 1
) AS settlement ON true
ORDER BY t.block_number DESC, t.log_index DESC
LIMIT 1000 + 0)
ORDER BY block_number DESC, log_index DESC
LIMIT 1000 OFFSET 0
), new_query AS (
(
SELECT
    t.block_number,
    t.log_index,
    t.order_uid,
    t.buy_amount,
    t.sell_amount,
    t.sell_amount - t.fee_amount as sell_amount_before_fees,
    o.owner,
    o.buy_token,
    o.sell_token,
    t.gas_cost,
    settlement.tx_hash,
    settlement.auction_id FROM trades t
LEFT OUTER JOIN LATERAL (
    SELECT tx_hash, auction_id FROM settlements s
    WHERE s.block_number = t.block_number
    AND   s.log_index > t.log_index
    ORDER BY s.log_index ASC
    LIMIT 1
) AS settlement ON true
JOIN orders o ON o.uid = t.order_uid
WHERE ('\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea IS NULL OR substring(t.order_uid, 33, 20) = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea) AND (NULL IS NULL OR t.order_uid = NULL)
ORDER BY t.block_number DESC, t.log_index DESC
LIMIT 1000 + 0)
UNION (
SELECT
    t.block_number,
    t.log_index,
    t.order_uid,
    t.buy_amount,
    t.sell_amount,
    t.sell_amount - t.fee_amount as sell_amount_before_fees,
    o.owner,
    o.buy_token,
    o.sell_token,
    t.gas_cost,
    settlement.tx_hash,
    settlement.auction_id FROM trades t
LEFT OUTER JOIN LATERAL (
    SELECT tx_hash, auction_id FROM settlements s
    WHERE s.block_number = t.block_number
    AND   s.log_index > t.log_index
    ORDER BY s.log_index ASC
    LIMIT 1
) AS settlement ON true
JOIN orders o ON o.uid = t.order_uid
JOIN onchain_placed_orders onchain_o ON onchain_o.uid = t.order_uid
WHERE ('\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea IS NULL OR onchain_o.sender = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea) AND (NULL IS NULL OR t.order_uid = NULL)
ORDER BY t.block_number DESC, t.log_index DESC
LIMIT 1000 + 0)
UNION ( WITH jit AS MATERIALIZED (   SELECT uid, owner, buy_token, sell_token   FROM jit_orders   WHERE ('\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea IS NULL OR owner = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea)   AND (NULL IS NULL OR uid = NULL))
SELECT
    t.block_number,
    t.log_index,
    t.order_uid,
    t.buy_amount,
    t.sell_amount,
    t.sell_amount - t.fee_amount as sell_amount_before_fees,
    o.owner,
    o.buy_token,
    o.sell_token,
    t.gas_cost,
    settlement.tx_hash,
    settlement.auction_id FROM jit o
JOIN trades t ON o.uid = t.order_uid
LEFT OUTER JOIN LATERAL (
    SELECT tx_hash, auction_id FROM settlements s
    WHERE s.block_number = t.block_number
    AND   s.log_index > t.log_index
    ORDER BY s.log_index ASC
    LIMIT 1
) AS settlement ON true
ORDER BY t.block_number DESC, t.log_index DESC
LIMIT 1000 + 0)
ORDER BY block_number DESC, log_index DESC
LIMIT 1000 OFFSET 0
)
SELECT 'old only' AS side, count(*) FROM
  (SELECT to_jsonb(o) FROM old_query o EXCEPT ALL SELECT to_jsonb(n) - 'gas_cost' FROM new_query n) d
UNION ALL
SELECT 'new only', count(*) FROM
  (SELECT to_jsonb(n) - 'gas_cost' FROM new_query n EXCEPT ALL SELECT to_jsonb(o) FROM old_query o) d;
user_orders: old vs new, owner 0x89b537d4…, limit 1000, offset 0
WITH old_query AS (
WITH page_uids AS ( SELECT uid, min(creation_timestamp) as creation_timestamp FROM ( (  SELECT o.uid, o.creation_timestamp  FROM orders o  WHERE o.owner = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea  ORDER BY creation_timestamp DESC  LIMIT 1000 + 0 ) UNION ALL (  SELECT o.uid, o.creation_timestamp  FROM onchain_placed_orders opo  JOIN orders o ON opo.uid = o.uid  WHERE opo.sender = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea AND o.owner != '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea  ORDER BY creation_timestamp DESC  LIMIT 1000 + 0 ) UNION ALL (  SELECT jit_o.uid, jit_o.creation_timestamp  FROM jit_orders jit_o  WHERE jit_o.owner = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea  ORDER BY creation_timestamp DESC  LIMIT 1000 + 0 ) ) combined
GROUP BY uid
ORDER BY creation_timestamp DESC
LIMIT 1000 OFFSET 0)  (  SELECT 
o.uid, o.owner, o.creation_timestamp, o.sell_token, o.buy_token, o.sell_amount, o.buy_amount,
o.valid_to, o.valid_from, o.app_data, o.fee_amount, o.kind, o.partially_fillable, o.signature,
o.receiver, o.signing_scheme, o.settlement_contract, o.sell_token_balance, o.buy_token_balance,
o.class,
(SELECT COALESCE(SUM(t.buy_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_buy,
(SELECT COALESCE(SUM(t.sell_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_sell,
(SELECT COALESCE(SUM(t.fee_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_fee,
(o.cancellation_timestamp IS NOT NULL OR
    (SELECT COUNT(*) FROM invalidations WHERE invalidations.order_uid = o.uid) > 0 OR
    (SELECT COUNT(*) FROM onchain_order_invalidations onchain_c where onchain_c.uid = o.uid limit 1) > 0
) AS invalidated,
(o.signing_scheme = 'presign' AND COALESCE((
    SELECT (NOT p.signed) as unsigned
    FROM presignature_events p
    WHERE o.uid = p.order_uid
    ORDER BY p.block_number DESC, p.log_index DESC
    LIMIT 1
), true)) AS presignature_pending,
array(Select (p.target, p.value, p.data) from interactions p where p.order_uid = o.uid and p.execution = 'pre' order by p.index) as pre_interactions,
array(Select (p.target, p.value, p.data) from interactions p where p.order_uid = o.uid and p.execution = 'post' order by p.index) as post_interactions,
(SELECT (tx_hash, eth_o.valid_to) from ethflow_orders eth_o
    left join ethflow_refunds on ethflow_refunds.order_uid=eth_o.uid
    where eth_o.uid = o.uid limit 1) as ethflow_data,
(SELECT onchain_o.sender from onchain_placed_orders onchain_o where onchain_o.uid = o.uid limit 1) as onchain_user,
(SELECT onchain_o.placement_error from onchain_placed_orders onchain_o where onchain_o.uid = o.uid limit 1) as onchain_placement_error,
COALESCE((SELECT SUM(executed_fee) FROM order_execution oe WHERE oe.order_uid = o.uid), 0) as executed_fee,
COALESCE((SELECT executed_fee_token FROM order_execution oe WHERE oe.order_uid = o.uid LIMIT 1), o.sell_token) as executed_fee_token, -- TODO surplus token
(SELECT full_app_data FROM app_data ad WHERE o.app_data = ad.contract_app_data LIMIT 1) as full_app_data
  FROM orders o 
WHERE o.uid IN (SELECT uid FROM page_uids) )
UNION ALL (  SELECT 
o.uid, o.owner, o.creation_timestamp, o.sell_token, o.buy_token, o.sell_amount, o.buy_amount,
o.valid_to, NULL AS valid_from, o.app_data, o.fee_amount, o.kind, o.partially_fillable, o.signature,
o.receiver, o.signing_scheme, '\x9008d19f58aabd9ed0d60971565aa8510560ab41'::bytea AS settlement_contract, o.sell_token_balance, o.buy_token_balance,
'liquidity'::OrderClass AS class,
(SELECT COALESCE(SUM(t.buy_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_buy,
(SELECT COALESCE(SUM(t.sell_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_sell,
(SELECT COALESCE(SUM(t.fee_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_fee,
FALSE AS invalidated,
FALSE AS presignature_pending,
ARRAY[]::record[] AS pre_interactions,
ARRAY[]::record[] AS post_interactions,
NULL AS ethflow_data,
NULL AS onchain_user,
NULL AS onchain_placement_error,
COALESCE((SELECT SUM(executed_fee) FROM order_execution oe WHERE oe.order_uid = o.uid), 0) as executed_fee,
COALESCE((SELECT executed_fee_token FROM order_execution oe WHERE oe.order_uid = o.uid LIMIT 1), o.sell_token) as executed_fee_token, -- TODO surplus token
NULL AS full_app_data
  FROM jit_orders o 
WHERE o.uid IN (SELECT uid FROM page_uids)    AND NOT EXISTS (SELECT 1 FROM orders ord WHERE o.uid = ord.uid) )
ORDER BY creation_timestamp DESC
), new_query AS (
WITH page_uids AS ( SELECT uid, min(creation_timestamp) as creation_timestamp FROM ( (  SELECT o.uid, o.creation_timestamp  FROM orders o  WHERE o.owner = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea  ORDER BY creation_timestamp DESC  LIMIT 1000 + 0 ) UNION ALL (  SELECT o.uid, o.creation_timestamp  FROM onchain_placed_orders opo  JOIN orders o ON opo.uid = o.uid  WHERE opo.sender = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea AND o.owner != '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea  ORDER BY creation_timestamp DESC  LIMIT 1000 + 0 ) UNION ALL (  SELECT jit_o.uid, jit_o.creation_timestamp  FROM jit_orders jit_o  WHERE jit_o.owner = '\x89b537d4e0de035303dc1bdae18394f7a6c15c36'::bytea  ORDER BY creation_timestamp DESC  LIMIT 1000 + 0 ) ) combined
GROUP BY uid
ORDER BY creation_timestamp DESC
LIMIT 1000 OFFSET 0)  (  SELECT 
o.uid, o.owner, o.creation_timestamp, o.sell_token, o.buy_token, o.sell_amount, o.buy_amount,
o.valid_to, o.valid_from, o.app_data, o.fee_amount, o.kind, o.partially_fillable, o.signature,
o.receiver, o.signing_scheme, o.settlement_contract, o.sell_token_balance, o.buy_token_balance,
o.class,
(SELECT COALESCE(SUM(t.buy_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_buy,
(SELECT COALESCE(SUM(t.sell_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_sell,
(SELECT COALESCE(SUM(t.fee_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_fee,
(o.cancellation_timestamp IS NOT NULL OR
    (SELECT COUNT(*) FROM invalidations WHERE invalidations.order_uid = o.uid) > 0 OR
    (SELECT COUNT(*) FROM onchain_order_invalidations onchain_c where onchain_c.uid = o.uid limit 1) > 0
) AS invalidated,
(o.signing_scheme = 'presign' AND COALESCE((
    SELECT (NOT p.signed) as unsigned
    FROM presignature_events p
    WHERE o.uid = p.order_uid
    ORDER BY p.block_number DESC, p.log_index DESC
    LIMIT 1
), true)) AS presignature_pending,
array(Select (p.target, p.value, p.data) from interactions p where p.order_uid = o.uid and p.execution = 'pre' order by p.index) as pre_interactions,
array(Select (p.target, p.value, p.data) from interactions p where p.order_uid = o.uid and p.execution = 'post' order by p.index) as post_interactions,
(SELECT (tx_hash, eth_o.valid_to) from ethflow_orders eth_o
    left join ethflow_refunds on ethflow_refunds.order_uid=eth_o.uid
    where eth_o.uid = o.uid limit 1) as ethflow_data,
(SELECT onchain_o.sender from onchain_placed_orders onchain_o where onchain_o.uid = o.uid limit 1) as onchain_user,
(SELECT onchain_o.placement_error from onchain_placed_orders onchain_o where onchain_o.uid = o.uid limit 1) as onchain_placement_error,
COALESCE((SELECT SUM(executed_fee) FROM order_execution oe WHERE oe.order_uid = o.uid), 0) as executed_fee,
COALESCE((SELECT executed_fee_token FROM order_execution oe WHERE oe.order_uid = o.uid LIMIT 1), o.sell_token) as executed_fee_token, -- TODO surplus token
(SELECT full_app_data FROM app_data ad WHERE o.app_data = ad.contract_app_data LIMIT 1) as full_app_data,
(SELECT CASE WHEN COUNT(*) = COUNT(t.gas_cost) THEN SUM(t.gas_cost) END FROM trades t WHERE t.order_uid = o.uid) as gas_cost
  FROM orders o 
WHERE o.uid IN (SELECT uid FROM page_uids) )
UNION ALL (  SELECT 
o.uid, o.owner, o.creation_timestamp, o.sell_token, o.buy_token, o.sell_amount, o.buy_amount,
o.valid_to, NULL AS valid_from, o.app_data, o.fee_amount, o.kind, o.partially_fillable, o.signature,
o.receiver, o.signing_scheme, '\x9008d19f58aabd9ed0d60971565aa8510560ab41'::bytea AS settlement_contract, o.sell_token_balance, o.buy_token_balance,
'liquidity'::OrderClass AS class,
(SELECT COALESCE(SUM(t.buy_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_buy,
(SELECT COALESCE(SUM(t.sell_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_sell,
(SELECT COALESCE(SUM(t.fee_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_fee,
FALSE AS invalidated,
FALSE AS presignature_pending,
ARRAY[]::record[] AS pre_interactions,
ARRAY[]::record[] AS post_interactions,
NULL AS ethflow_data,
NULL AS onchain_user,
NULL AS onchain_placement_error,
COALESCE((SELECT SUM(executed_fee) FROM order_execution oe WHERE oe.order_uid = o.uid), 0) as executed_fee,
COALESCE((SELECT executed_fee_token FROM order_execution oe WHERE oe.order_uid = o.uid LIMIT 1), o.sell_token) as executed_fee_token, -- TODO surplus token
NULL AS full_app_data,
(SELECT CASE WHEN COUNT(*) = COUNT(t.gas_cost) THEN SUM(t.gas_cost) END FROM trades t WHERE t.order_uid = o.uid) as gas_cost
  FROM jit_orders o 
WHERE o.uid IN (SELECT uid FROM page_uids)    AND NOT EXISTS (SELECT 1 FROM orders ord WHERE o.uid = ord.uid) )
ORDER BY creation_timestamp DESC
)
SELECT 'old only' AS side, count(*) FROM
  (SELECT to_jsonb(o) FROM old_query o EXCEPT ALL SELECT to_jsonb(n) - 'gas_cost' FROM new_query n) d
UNION ALL
SELECT 'new only', count(*) FROM
  (SELECT to_jsonb(n) - 'gas_cost' FROM new_query n EXCEPT ALL SELECT to_jsonb(o) FROM old_query o) d;

Timing, warm cache:

Query Old New
single_full_order_with_quote, order with 405 fills 1.0 ms 1.2 ms
user_orders, limit 1000, 5 heavy owners 30-51 ms 35-56 ms (+5-17 %)
trades, owner with 150k fills, limit 1000 12.4 ms 11.6 ms (same plan)

Cold reads did not increase (the trades heap is resident). Possible follow-up: fold the four trades subqueries in orders::SELECT into one aggregate.

How to test

Run the DB tests (needs docker compose up -d):

cargo nextest run postgres -p orderbook -p database -p autopilot --test-threads 1 --run-ignored ignored-only

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, None for an unfilled order), postgres_user_orders_correctness (both union arms, liquidity-only JIT order reports 0).

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

This comment was marked as resolved.

@github-actions

This comment was marked as resolved.

@claude

This comment was marked as resolved.

@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 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 Outdated
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

This comment was marked as resolved.

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.

@github-actions github-actions Bot added the stale label Aug 1, 2026
@jmg-duarte jmg-duarte removed the stale label Aug 3, 2026
@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 thread crates/database/src/orders.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
@jmg-duarte
jmg-duarte force-pushed the jmgd/order-trade-gas-cost branch from 11a3ad9 to 7b3c37a Compare August 21, 2026 15:59
@jmg-duarte
jmg-duarte force-pushed the jmgd/order-trade-gas-cost branch from 7b3c37a to 7ef256b Compare August 21, 2026 16:03
@jmg-duarte

Copy link
Copy Markdown
Contributor Author

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 😂

@jmg-duarte
jmg-duarte marked this pull request as draft August 21, 2026 18:03
@github-actions

github-actions Bot commented Sep 2, 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.

@github-actions github-actions Bot added the stale label Sep 2, 2026
@jmg-duarte jmg-duarte removed the stale label Sep 3, 2026
@MartinquaXD
MartinquaXD force-pushed the jmgd/order-trade-gas-cost branch from 486bbc7 to 8e6bae8 Compare September 3, 2026 14:10
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@github-actions

github-actions Bot commented Sep 3, 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

@jmg-duarte
jmg-duarte force-pushed the jmgd/order-trade-gas-cost branch from 8e6bae8 to 486bbc7 Compare September 4, 2026 14:42
@jmg-duarte
jmg-duarte marked this pull request as ready for review September 4, 2026 16:00
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 3m 9s —— View job


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

Re-reviewed the current (squashed) branch against origin/main. This is the simplified write-at-attribution-time design (trades.gas_cost written by the autopilot's attribute_gas_cost), and it holds together well.

Verified correct:

  • The trades read query is back to its original shapeSETTLEMENT_JOIN is unchanged and only t.gas_cost (a plain column read) is added to the select list. This retires @MartinquaXD's performance concern about the earlier subquery-heavy restructure: there is no longer any per-row gas subquery on the trades path.
  • No missing-column failures. Every FullOrder-producing query accounts for gas_cost: orders::SELECT and jit_orders::SELECT add the all-or-nothing subquery; the two hand-written hot paths (solvable_orders/OPEN_ORDERS and open_orders_by_time_or_uids) select NULL::numeric AS gas_cost; full_orders_in_tx and user_orders inherit it via the shared SELECT. TradesQueryRow/FullOrder match by column name so select ordering is irrelevant. The solvable-orders hot path stays free of the gas subquery.
  • All-or-nothing sum is right. CASE WHEN COUNT(*) = COUNT(t.gas_cost) THEN SUM(t.gas_cost) END returns NULL on a partial sum, NULL for a never-filled order (empty SUM), and Some(0) for a liquidity-only JIT order whose fills are all attributed 0 — all pinned by the new tests (postgres_order_gas_cost_across_fills, postgres_trades_report_attributed_gas_cost, postgres_user_orders_correctness).
  • attribute_gas_cost divisor is safegas_paying only ever divides for user trades; liquidity-only rows short-circuit to 0 via the CASE, so no divide-by-zero even for an all-JIT settlement.
  • Conversions fail loud. big_decimal_to_u256 now surfaces a non-integer/negative value as an error rather than silently reporting None; convert_trade_gas_cost covers the negative case.
  • Model/OpenAPI docs now consistently describe the attributed on-chain share (not a quote-derived estimate) — the round-one doc-comment mismatch is fixed. ..Default::default() construction sites (remaining_amounts.rs, etc.) are unaffected since OrderMetadata/Trade derive Default.

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); /transactions/{tx}/orders reports an order's lifetime total, not the tx-scoped share; gasCost is omitted (via skip_serializing_if) rather than serialized as explicit null under the nullable-implying OpenAPI schema — harmless and consistent with sibling optional fields; the deploy-ordering risk is moot since the V116/V121 migration and the autopilot write path are already on main (this PR is read-side only). The self-flagged follow-up (folding the four trades subqueries in orders::SELECT into one aggregate) is a reasonable later optimization, not a blocker.

  • Read pr-review skill & gather context
  • Review the diff (git diff origin/main...HEAD)
  • Verify prior review comments are addressed
  • Confirm column coverage across all FullOrder/TradesQueryRow queries
  • Post summary
    jmgd/order-trade-gas-cost

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