Skip to content

Persist trade's gas cost - #4743

Merged
jmg-duarte merged 6 commits into
mainfrom
jmgd/persist-trade-gas-cost
Aug 18, 2026
Merged

Persist trade's gas cost#4743
jmg-duarte merged 6 commits into
mainfrom
jmgd/persist-trade-gas-cost

Conversation

@jmg-duarte

@jmg-duarte jmg-duarte commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Description

Turns out that the changes in #4540 (at the time of writing) would add a bit of runtime to the settlement query (which we really don't want), the solution is to pre-compute the required information instead of doing it on demand.

This PR introduces the logic to do so.

Changes

  • Adds the gas_cost (approximate) column to trades
  • Adds an update to attribute the gas_cost to trades

Testing

Added a test

@github-actions

github-actions Bot commented Aug 14, 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.

Caused by:

@jmg-duarte
jmg-duarte force-pushed the jmgd/persist-trade-gas-cost branch from 830b2f5 to b38c40d Compare August 14, 2026 14:15
@jmg-duarte jmg-duarte changed the title Persist trade gas cost Persist trade's gas cost Aug 14, 2026
@jmg-duarte
jmg-duarte marked this pull request as ready for review August 14, 2026 14:19
@jmg-duarte
jmg-duarte requested a review from a team as a code owner August 14, 2026 14:19
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: Persist trade's gas cost

  • Read PR skill & gather context
  • Review DB migrations (V121, V122)
  • Review crates/database/src/trades.rs
  • Review crates/autopilot/src/infra/persistence/mod.rs
  • Review README changes
  • Post inline comments & summary

I reviewed the full diff and traced the surrounding code paths. This is a clean, well-tested, well-documented change — I did not find any blocking issues, so I'm not posting inline comments.

Things I explicitly verified:

  • Refactor is behavior-preserving — the extracted SETTLED_TRADES_CTE produces the exact same settled set that get_trades_for_settlement computed inline before (same previous_settlement lower bound, same inclusive BETWEEN … AND $2).
  • Division-by-zero is impossible — when a settlement settles no trades, the UPDATE … FROM settled matches zero rows, so the SET subquery is never evaluated. Covered by postgres_attribute_gas_cost_without_trades. 👍
  • Two settlements in one block don't overlap — each settlement's trade range is bounded by the previous settlement's log index, so gas is attributed to disjoint trade sets (verified by postgres_attribute_gas_cost).
  • Concurrency is safepost_process_outstanding_settlement_transactions runs up to 10 settlements concurrently, each in its own transaction. Same-block settlements update disjoint rows and only read the already-indexed settlements table for previous_settlement, so there's no write conflict or ordering dependency.
  • Timingattribute_gas_cost relies on the settlement's trades already being present, which is the same guarantee get_trades_for_settlement already depends on (trades + settlement events from a block are indexed atomically before post-processing), so no new NULL-forever risk.
  • Rollout compatibility — V121 adds a nullable column (old pods' inserts default it to NULL), and V122 creates the new covering index before dropping trades_covering; since the new index shares the order_uid key and a superset of INCLUDE columns, old code's covering-index queries keep their index-only scan. No breaking window.
  • Types / depsnumeric(78,0) matches the existing settlements gas columns (V116/V048); const_format is already a dependency and used elsewhere in the crate.

Minor confirmations aligned with the PR description: the CREATE/DROP INDEX CONCURRENTLY in V122 follows the established repo pattern (V102/V103) and the author plans to apply it by hand before merge; the double-attribution-per-tx caveat is acknowledged in code comments and mirrors settlements.gas_used.

LGTM. ✅
jmgd/persist-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.

The new approach looks a lot simpler. 👍
Just a few nits/questions but overall good.

Comment thread crates/database/src/trades.rs Outdated
Comment thread database/sql/V122__trade_gas_cost_covering_index.sql Outdated
Comment thread database/README.md Outdated
Comment thread crates/database/src/trades.rs Outdated
@jmg-duarte
jmg-duarte requested a review from MartinquaXD August 18, 2026 10:42
Comment thread crates/database/src/trades.rs Outdated
@jmg-duarte
jmg-duarte enabled auto-merge August 18, 2026 16:25
@jmg-duarte
jmg-duarte added this pull request to the merge queue Aug 18, 2026
Merged via the queue into main with commit 30bdd90 Aug 18, 2026
22 checks passed
@jmg-duarte
jmg-duarte deleted the jmgd/persist-trade-gas-cost branch August 18, 2026 16:54
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants