Fix unsettled funding sign #854

dydxwill · 2023-12-07T17:53:48Z

Changelist

Fix unsettled funding sign

Since the sign was changed in the shared getUnsettledFunding function, updated minus -> plus in roundtable pnl calculation task. Pnl unit tests are still passing.

Test Plan

Unit tests

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.

coderabbitai · 2023-12-07T17:53:56Z

Walkthrough

The overall changes involve adjustments to the financial calculations related to funding and profit and loss (PnL) across various functions and test cases. The logic for computing values such as maintenance margin, unsettled funding, net funding, realized PnL, and adjusted size has been updated, with operations like addition and subtraction being swapped in several instances. These modifications suggest a correction or refinement in the way financial metrics are calculated within the system.

Changes

File Path	Change Summary
`.../postgres/__tests__/db/helpers.test.ts` `.../comlink/__tests__/controllers/api/v4/addresses-controller.test.ts` `.../comlink/__tests__/controllers/api/v4/perpetual-positions-controller.test.ts` `.../comlink/__tests__/lib/helpers.test.ts`	Updated test cases to reflect changes in financial calculation logic, including negated expected values and modified parameters.
`.../postgres/src/db/helpers.ts` `.../services/roundtable/src/helpers/pnl-ticks-helper.ts` `.../services/comlink/src/lib/helpers.ts`	Adjusted financial calculation logic in functions, including changes to the computation of maintenance margin, unsettled funding, equity, and adjusted size.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 23a6709 and f0b1129.

Files selected for processing (3)

indexer/packages/postgres/tests/db/helpers.test.ts (3 hunks)
indexer/packages/postgres/src/db/helpers.ts (2 hunks)
indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (1 hunks)

Files skipped from review due to trivial changes (1)

indexer/packages/postgres/tests/db/helpers.test.ts

Additional comments: 3

indexer/packages/postgres/src/db/helpers.ts (2)
58-64: The calculation in getUnsettledFunding still performs a subtraction, which contradicts the PR objective of reversing the sign from minus to plus. This needs to be corrected to align with the intended logic change.
-    lastUpdateFundingIndexMap[position.perpetualId].minus(
-      latestFundingIndexMap[position.perpetualId],
+    latestFundingIndexMap[position.perpetualId].minus(
+      lastUpdateFundingIndexMap[position.perpetualId],
40-49: The updated comments in the getUnsettledFunding function provide clear guidance on the behavior of unsettled funding based on the direction of the funding index change, which aligns with the PR objective.
indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (1)

361-364: The change in the calculateEquity function to add totalUnsettledFundingPayment instead of subtracting it aligns with the PR objective to correct the sign used in the calculation of unsettled funding within the PnL calculation task. This is a significant change in the logic of the financial calculations and should be thoroughly tested to ensure accuracy.

vincentwschau · 2023-12-07T18:02:31Z

This function will need to be fixed as well.

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f0b1129 and a169b3a.

Files selected for processing (4)

indexer/services/comlink/tests/controllers/api/v4/addresses-controller.test.ts (2 hunks)
indexer/services/comlink/tests/controllers/api/v4/perpetual-positions-controller.test.ts (2 hunks)
indexer/services/comlink/tests/lib/helpers.test.ts (6 hunks)
indexer/services/comlink/src/lib/helpers.ts (1 hunks)

Additional comments: 9

indexer/services/comlink/__tests__/controllers/api/v4/addresses-controller.test.ts (2)

89-100: The changes in the test values for netFunding and realizedPnl correctly reflect the updated logic for unsettled funding calculation, as per the PR's objective. The comments in the code provide clarity on the calculation and its expected outcome, which is helpful for understanding the changes.

246-257: The changes in the test values for netFunding and realizedPnl in this second hunk are consistent with the changes in the first hunk and align with the PR's objective to correct the sign in the calculation of unsettled funding. The comments in the code are consistent and provide a clear explanation of the expected values based on the updated logic.

indexer/services/comlink/__tests__/controllers/api/v4/perpetual-positions-controller.test.ts (2)

75-86: The test case for a long position reflects the updated logic for the calculation of net funding and realized Pnl. The comments explain the calculation steps, which is helpful for maintainability. The calculation seems to be consistent with the PR's objective of correcting the sign in the calculation of unsettled funding.

126-137: The test case for a short position also reflects the updated logic for the calculation of net funding and realized Pnl. The comments and calculations are consistent with the changes described in the PR's objective. It's important to ensure that the actual implementation of getUnsettledFunding in the source code matches these test expectations.

indexer/services/comlink/__tests__/lib/helpers.test.ts (4)

415-421: The test cases for adjustUSDCAssetPosition have been updated, but the summary does not mention any changes to this function. Please ensure that the changes in these test cases are consistent with the actual changes made to the adjustUSDCAssetPosition function in the source code.

478-483: The test cases for adjustUSDCAssetPosition have been updated, but the summary does not mention any changes to this function. Please ensure that the changes in these test cases are consistent with the actual changes made to the adjustUSDCAssetPosition function in the source code.

543-549: The test cases for adjustUSDCAssetPosition have been updated, but the summary does not mention any changes to this function. Please ensure that the changes in these test cases are consistent with the actual changes made to the adjustUSDCAssetPosition function in the source code.

593-598: The test cases for adjustUSDCAssetPosition have been updated, but the summary does not mention any changes to this function. Please ensure that the changes in these test cases are consistent with the actual changes made to the adjustUSDCAssetPosition function in the source code.

indexer/services/comlink/src/lib/helpers.ts (1)

456-462: The changes in the adjustUSDCAssetPosition function correctly reflect the PR's objective to correct the sign used in the calculation of unsettled funding. The addition of unsettledFunding to signedUsdcPositionSize on line 459 is consistent with the PR description and the summary provided.

coderabbitai · 2023-12-07T18:43:41Z

indexer/services/comlink/__tests__/lib/helpers.test.ts

      );

      expect(unsettledFunding).toEqual(
-        Big(perpetualPosition.size).times('100').plus(
-          Big(perpetualPosition2.size).times('1000'),
+        Big(perpetualPosition.size).times('-100').plus(
+          Big(perpetualPosition2.size).times('-1000'),
        ),
      );


The test case for getTotalUnsettledFunding seems to be expecting negative values for unsettled funding, which contradicts the PR objective of changing the sign from minus to plus in the getUnsettledFunding function. Please verify that the expected values in this test case align with the updated logic in the getUnsettledFunding function.

* [CLOB-976] Push indexer ECR images to mainnet. (#766) * Fix unsettled funding sign (#854) --------- Co-authored-by: vincentwschau <[email protected]>

fix unsettled funding sign

f0b1129

github-actions bot added the indexer label Dec 7, 2023

coderabbitai bot reviewed Dec 7, 2023

View reviewed changes

fix

a169b3a

coderabbitai bot reviewed Dec 7, 2023

View reviewed changes

vincentwschau approved these changes Dec 7, 2023

View reviewed changes

dydxwill merged commit 501491f into main Dec 7, 2023
11 checks passed

dydxwill deleted the wl/fix_unsettled_funding_sign branch December 7, 2023 19:11

dydxwill added a commit that referenced this pull request Dec 8, 2023

Fix unsettled funding sign (#854)

69bc3d6

dydxwill added a commit that referenced this pull request Dec 8, 2023

Cherry-pick changes for indexer v1.0.3 (#862)

594c6e9

* [CLOB-976] Push indexer ECR images to mainnet. (#766) * Fix unsettled funding sign (#854) --------- Co-authored-by: vincentwschau <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unsettled funding sign #854

Fix unsettled funding sign #854

dydxwill commented Dec 7, 2023 •

edited

Loading

coderabbitai bot commented Dec 7, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

vincentwschau commented Dec 7, 2023

coderabbitai bot left a comment

coderabbitai bot Dec 7, 2023

Fix unsettled funding sign #854

Fix unsettled funding sign #854

Conversation

dydxwill commented Dec 7, 2023 • edited Loading

Changelist

Test Plan

Author/Reviewer Checklist

coderabbitai bot commented Dec 7, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

vincentwschau commented Dec 7, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Dec 7, 2023

Choose a reason for hiding this comment

dydxwill commented Dec 7, 2023 •

edited

Loading

coderabbitai bot commented Dec 7, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)