Skip to content

Conversation

@pp346
Copy link
Contributor

@pp346 pp346 commented Jan 20, 2026

Changelist

revert the total realized pnl change back to old formula. this is backwards compatible since old formula columns are untouched and logic there is unchanged

Test Plan

tested on testnet

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

Release Notes

  • Refactor

    • Modified how realized PnL is calculated for perpetual positions, now computed dynamically from price movements and funding costs rather than retrieved from stored values.
  • Chores

    • Database schema updates to support the refactored calculation approach.

✏️ Tip: You can customize this high-level summary in your review settings.

@pp346 pp346 force-pushed the feat/revert-total-realized-pnl branch from e51f423 to b9a59f0 Compare January 20, 2026 16:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 20, 2026

📝 Walkthrough

Walkthrough

The PR removes the totalRealizedPnl field from perpetual position tracking across the codebase. Changes include dropping the column via database migration, removing the field from type definitions and models, eliminating the SQL function that applied fill-related realized effects, and updating realized PnL calculation logic to compute dynamically rather than store as a persisted value.

Changes

Cohort / File(s) Summary
Type & Model Definitions
indexer/packages/postgres/src/types/perpetual-position-types.ts, indexer/packages/postgres/src/types/db-model-types.ts, indexer/packages/postgres/src/models/perpetual-position-model.ts
Removed totalRealizedPnl optional field from PerpetualPositionCreateObject, PerpetualPositionUpdateObject, PerpetualPositionFromDatabase, and the corresponding model class property and JSON schema mappings.
Database Migration
indexer/packages/postgres/src/db/migrations/migration_files/20260120113311_revert_total_realized_pnl.ts
New migration that drops totalRealizedPnl column from perpetual_positions table in the up function; down function restores it as a nullable decimal.
SQL Scripts & Functions
indexer/services/ender/src/scripts/helpers/dydx_apply_fill_realized_effects.sql, indexer/services/ender/src/scripts/helpers/dydx_order_fill_handler_per_order.sql, indexer/services/ender/src/helpers/postgres/postgres-functions.ts
Removed dydx_apply_fill_realized_effects PL/pgSQL function (50 lines) that previously updated realized PnL on fills; removed the function from the HELPER_SCRIPTS registry; removed the PERFORM call to this function from dydx_order_fill_handler_per_order.
Realized PnL Calculation
indexer/services/comlink/src/request-helpers/request-transformer.ts
Changed realized PnL calculation from using stored position.totalRealizedPnl to computing it dynamically based on price difference (entry vs. exit by side) times closed amount, plus net funding (settled + unsettled).
Test Updates
indexer/packages/postgres/__tests__/helpers/constants.ts, indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts, indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts
Removed totalRealizedPnl field from test object initializers and helper functions; deleted test case validating realized PnL updates on reduce-only order closes; removed totalRealizedPnl from expectPerpetualPosition function signature and related assertion blocks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • dydxprotocol/v4-chain#3168 — Directly inverse relationship; that PR added the totalRealizedPnl column, helper SQL function, and snapshot fields that this PR removes.

Suggested reviewers

  • tqin7
  • UnbornAztecKing

Poem

🐰 A field once stored, now computed with care,
Realized PnL floats through the air,
No more SQL hoarding what math can derive,
Dynamic calculations keep logic alive! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: revert total realized pnl' clearly and specifically describes the main change—reverting the total realized PnL implementation to an old formula.
Description check ✅ Passed The description includes all required sections: Changelist explaining the revert, Test Plan confirming testnet testing, and completed Author/Reviewer Checklist with appropriate labels.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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

Labels

Development

Successfully merging this pull request may close these issues.

2 participants