Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TRA-107] Add parentSubaccountNumber endpoint for historical-pnl #1476

Merged
merged 3 commits into from
May 8, 2024

Conversation

shrenujb
Copy link
Contributor

@shrenujb shrenujb commented May 8, 2024

Changelist

Add parentSubaccountNumber endpoint for historical-pnl

Test Plan

Added 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.

Copy link

linear bot commented May 8, 2024

Comment on lines +107 to +131
SubaccountTable.findAll(
{
id: childSubaccountIds,
},
[QueryableField.ID],
),
PnlTicksTable.findAll(
{
subaccountId: childSubaccountIds,
limit,
createdBeforeOrAtBlockHeight: createdBeforeOrAtHeight
? createdBeforeOrAtHeight.toString()
: undefined,
createdBeforeOrAt,
createdOnOrAfterBlockHeight: createdOnOrAfterHeight
? createdOnOrAfterHeight.toString()
: undefined,
createdOnOrAfter,
},
[QueryableField.LIMIT],
{
...DEFAULT_POSTGRES_OPTIONS,
orderBy: [[QueryableField.BLOCK_HEIGHT, Ordering.DESC]],
},
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you want the to pull both the childSubaccountIds and the parentSubaccountId? You're ignoring the parent now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The childSubccountIds are inclusive. So parentSubaccountNumber 0 will have childSubaccountNumbers 0, 128, 256 , ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the documentation for getChildSubaccountId or getChildSubaccountNums doesn't seem to indicate that, could you update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does?

* Child subaccounts = [128*0+parentSubaccount, 128*1+parentSubaccount ... 128*999+parentSubaccount]

Is there anything more you'd like to have specified?

@@ -207,5 +207,65 @@ describe('pnlTicks-controller#V4', () => {
],
});
});

it('Get /historical-pnl/parentSubaccountNumber', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you create a test to verify that this returns an aggregated pnl of child and parent subaccount?

Copy link
Contributor Author

@shrenujb shrenujb May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is exactly what this test does. It aggregates the pnl for subaccounts 0 and 128 when parentSubaccountNumber 0 is requested

@shrenujb shrenujb merged commit eadfc82 into main May 8, 2024
11 checks passed
@shrenujb shrenujb deleted the tra107 branch May 8, 2024 14:50
@vincentwschau
Copy link
Contributor

@Mergifyio backport release/indexer/v5.x

Copy link
Contributor

mergify bot commented May 9, 2024

backport release/indexer/v5.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 9, 2024
Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
(cherry picked from commit eadfc82)
vincentwschau pushed a commit that referenced this pull request May 9, 2024
…) (#1485)

Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange>
(cherry picked from commit eadfc82)

Co-authored-by: shrenujb <98204323+shrenujb@users.noreply.github.com>
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.

3 participants