Skip to content

Persist trade's gas cost - #4743

Open
jmg-duarte wants to merge 1 commit into
mainfrom
jmgd/persist-trade-gas-cost
Open

Persist trade's gas cost#4743
jmg-duarte wants to merge 1 commit 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
  • Replaces the existing covering index with one that includes the gas_cost (note that they can't have the same name at creation, hence the new _with_gas_cost
    • As we've learned in the past, we need to add the index by hand, so once the PR is approved AND before merging, I'll apply the migration changes by hand (add column + replace index) and then we can merge

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

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.

1 participant