[OTE-544][OTE-545] Add rountable task for leaderboard table #1965
Conversation
WalkthroughThis update significantly enhances the system's capabilities by introducing new constants, functions, and testing improvements related to subaccounts, PnL ticks, and leaderboard management. Key features include the addition of vault-related entities, new asynchronous functions for ranking, and refined error handling for leaderboard updates. Overall, these modifications aim to bolster data handling, expand testing coverage, and improve maintainability to efficiently support financial analytics and user interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PnlTicksService
participant Database
participant LeaderboardService
participant Redis
User->>PnlTicksService: Create PnL Ticks
PnlTicksService->>Database: Store PnL Ticks
PnlTicksService->>LeaderboardService: Update Leaderboard PnL
LeaderboardService->>Database: Fetch Ranked PnL Ticks
Database->>LeaderboardService: Return Ranked PnL Data
LeaderboardService->>Database: Insert Leaderboard Data
Database->>LeaderboardService: Confirm Insert
LeaderboardService->>Redis: Update Cache
Redis-->>LeaderboardService: Confirm Cache Update
LeaderboardService->>User: Return Updated Leaderboard
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
affanv14
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (3 hunks)
- indexer/packages/postgres/src/constants.ts (1 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (4 hunks)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts (2 hunks)
- indexer/services/roundtable/src/config.ts (1 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (2 hunks)
Additional comments not posted (18)
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (3)
101-122: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
updateLeaderboardPnlTableare correctly integrated.Verification successful
The function
updateLeaderboardPnlTableis correctly integrated within the filecreate-pnl-ticks.tsand is not called from any other files.
indexer/services/roundtable/src/tasks/create-pnl-ticks.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `updateLeaderboardPnlTable` are correctly integrated. # Test: Search for the function usage. Expect: Proper integration of function calls. rg --type ts -A 5 $'updateLeaderboardPnlTable'Length of output: 847
124-136: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
insertLeaderboardPnlObjectsare correctly integrated.Verification successful
LGTM! But verify the function usage in the codebase.
The function
insertLeaderboardPnlObjectsis defined and used within the same file (indexer/services/roundtable/src/tasks/create-pnl-ticks.ts). Ensure that the function call is correctly integrated and that the surrounding code properly handles its behavior.
- File:
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts- Function Call:
await insertLeaderboardPnlObjects(leaderboardPnlObjects);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `insertLeaderboardPnlObjects` are correctly integrated. # Test: Search for the function usage. Expect: Proper integration of function calls. rg --type ts -A 5 $'insertLeaderboardPnlObjects'Length of output: 1054
98-99: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
runTaskare correctly integrated.Verification successful
The
runTaskfunction fromcreate-pnl-ticks.tsis correctly integrated in the codebase.
- The function is imported and used in the test file
create-pnl-ticks.test.ts.- The function is also imported in the main index file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `runTask` are correctly integrated. # Test: Search for the function usage. Expect: Proper integration of function calls. rg --type ts -A 5 $'runTask'Length of output: 30962
Script:
#!/bin/bash # Description: Verify the specific usage of `runTask` from `create-pnl-ticks.ts` are correctly integrated. # Test: Search for the import of `runTask` from `create-pnl-ticks.ts`. Expect: Proper integration of function calls. rg --type ts "import runTask from '../../src/tasks/create-pnl-ticks';" -A 5Length of output: 83
Script:
#!/bin/bash # Description: Verify any references to `create-pnl-ticks` to ensure it is correctly integrated. # Test: Search for any references to `create-pnl-ticks`. Expect: Proper integration of function calls. rg --type ts 'create-pnl-ticks' -A 5Length of output: 3799
indexer/packages/postgres/src/constants.ts (1)
129-135: LGTM! But verify the enum usage in the codebase.The code changes are approved.
However, ensure that the
LEADERBOARD_TIMESPANenum is used consistently throughout the codebase.Verification successful
Enum usage is consistent and correct.
The
LEADERBOARD_TIMESPANenum is used consistently across the codebase in the following files:
indexer/packages/postgres/src/stores/pnl-ticks-table.tsindexer/services/roundtable/src/tasks/create-pnl-ticks.tsNo issues found with the enum usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `LEADERBOARD_TIMESPAN` enum in the codebase. # Test: Search for the enum usage. Expect: Consistent usage of the enum. rg --type ts -A 5 $'LEADERBOARD_TIMESPAN'Length of output: 2921
indexer/services/roundtable/src/config.ts (1)
129-129: LGTM! But verify the constant usage in the codebase.The code changes are approved.
However, ensure that the
LEADERBOARD_PNL_MAX_ROWS_PER_UPSERTconstant is used consistently throughout the codebase.Verification successful
LGTM! But verify the constant usage in the codebase.
The code changes are approved.
However, ensure that the
LEADERBOARD_PNL_MAX_ROWS_PER_UPSERTconstant is used consistently throughout the codebase. It is defined inindexer/services/roundtable/src/config.tsand used inindexer/services/roundtable/src/tasks/create-pnl-ticks.ts.
- indexer/services/roundtable/src/config.ts: Definition of the constant.
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts: Usage of the constant.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `LEADERBOARD_PNL_MAX_ROWS_PER_UPSERT` constant in the codebase. # Test: Search for the constant usage. Expect: Consistent usage of the constant. rg --type ts -A 5 $'LEADERBOARD_PNL_MAX_ROWS_PER_UPSERT'Length of output: 1199
indexer/packages/postgres/src/stores/pnl-ticks-table.ts (4)
249-263: LGTM! But verify the function usage in the codebase.The function correctly converts timespan strings to SQL interval strings and handles invalid inputs by throwing an error.
However, ensure that all calls to
convertTimespanToSQLare correctly handled.Verification successful
Function usage verified.
The
convertTimespanToSQLfunction is correctly utilized in the fileindexer/packages/postgres/src/stores/pnl-ticks-table.ts.
- File:
indexer/packages/postgres/src/stores/pnl-ticks-table.ts- Usage: Line 249
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `convertTimespanToSQL` are correctly handled. # Test: Search for the function usage. Expect: Only occurances of the new function signature. rg --type js -A 5 $'convertTimespanToSQL'Length of output: 40
Script:
#!/bin/bash # Description: Verify all function calls to `convertTimespanToSQL` are correctly handled. # Test: Search for the function usage across all file types. rg -A 5 'convertTimespanToSQL'Length of output: 1201
387-438: LGTM! But verify the SQL query construction.The function correctly constructs a SQL query using CTEs to calculate and rank the latest PnL ticks.
However, ensure that the SQL query is correctly constructed and optimized.
Verification successful
The SQL query construction in
getLatestRankedPnlTicksis correctly implemented.The query uses CTEs appropriately and follows correct SQL syntax to calculate and rank the latest PnL ticks.
- The query correctly partitions and orders the PnL ticks.
- It selects the latest PnL for each subaccount.
- It aggregates the results by address.
- It ranks the addresses by total PnL.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the SQL query construction in `getLatestRankedPnlTicks`. # Test: Search for the SQL query construction. Expect: Correct usage of CTEs and SQL syntax. rg --type sql -A 5 $'WITH latest_pnl_ranked'Length of output: 45
Script:
#!/bin/bash # Description: Verify the SQL query construction in `getLatestRankedPnlTicks`. # Test: Search for the SQL query construction in TypeScript files. Expect: Correct usage of CTEs and SQL syntax. rg --type ts -A 5 $'WITH latest_pnl_ranked'Length of output: 538
Script:
#!/bin/bash # Description: Extract the entire SQL query from `getLatestRankedPnlTicks` for review. # Test: Extract the SQL query from the TypeScript file. rg --type ts -A 50 $'WITH latest_pnl_ranked' indexer/packages/postgres/src/stores/pnl-ticks-table.tsLength of output: 1362
298-384: LGTM! But verify the SQL query construction.The function correctly constructs a SQL query using CTEs to calculate and rank PnL ticks over a specified timespan.
However, ensure that the SQL query is correctly constructed and optimized.
Verification successful
The SQL query construction in the
getRankedPnlTicksForTimeSpanfunction is correctly implemented.The query uses multiple CTEs to gather and aggregate data before ranking the results, ensuring optimized and accurate data retrieval.
- The use of
ROW_NUMBER()for ranking within CTEs is appropriate.- The final aggregation and ranking steps are correctly structured.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the SQL query construction in `getRankedPnlTicksForTimeSpan`. # Test: Search for the SQL query construction. Expect: Correct usage of CTEs and SQL syntax. rg --type sql -A 5 $'WITH latest_subaccount_pnl_x_days_ago_ranked'Length of output: 67
Script:
#!/bin/bash # Description: Verify the SQL query construction in `getRankedPnlTicksForTimeSpan`. # Test: Search for the SQL query construction within TypeScript files. Expect: Correct usage of CTEs and SQL syntax. rg --type ts -A 5 $'WITH latest_subaccount_pnl_x_days_ago_ranked'Length of output: 642
Script:
#!/bin/bash # Description: Extract the full SQL query construction in `getRankedPnlTicksForTimeSpan` for review. # Extract lines containing the SQL query construction within the TypeScript file. rg --type ts -A 100 'WITH latest_subaccount_pnl_x_days_ago_ranked' indexer/packages/postgres/src/stores/pnl-ticks-table.tsLength of output: 3219
289-296: LGTM! But verify the function usage in the codebase.The function correctly delegates the task based on the timespan value.
However, ensure that all calls to
getRankedPnlTicksare correctly handled.Verification successful
Let's refine our search to include more file types and ensure we capture all instances of the function usage.
Let's correct the syntax for adding new file types and re-run the search command to verify the function usage.
Verification successful!
The function
getRankedPnlTicksis correctly handled in the codebase. It is used in the following locations:
indexer/services/roundtable/src/tasks/create-pnl-ticks.tsindexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.tsThese usages indicate that the function is actively used and tested.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getRankedPnlTicks` are correctly handled. # Test: Search for the function usage. Expect: Only occurances of the new function signature. rg --type js -A 5 $'getRankedPnlTicks'Length of output: 37
Script:
#!/bin/bash # Description: Verify all function calls to `getRankedPnlTicks` are correctly handled. # Test: Search for the function usage in all relevant file types. rg --type-add 'ts' --type-add 'tsx' --type-add 'js' --type-add 'jsx' -A 5 'getRankedPnlTicks'Length of output: 145
Script:
#!/bin/bash # Description: Verify all function calls to `getRankedPnlTicks` are correctly handled. # Test: Search for the function usage in all relevant file types. rg --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' --type-add 'js:*.js' --type-add 'jsx:*.jsx' -A 5 'getRankedPnlTicks'Length of output: 5242
indexer/services/roundtable/__tests__/tasks/create-pnl-ticks.test.ts (1)
98-119: LGTM! But verify the assertions.The test case correctly validates the functionality of populating the leaderboard with ranked PnL ticks.
However, ensure that the assertions are correctly validating the expected behavior.
indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.ts (6)
297-317: LGTM! But verify the assertions.The test case correctly validates the retrieval of ranked PnL ticks for the "ALL_TIME" timespan.
However, ensure that the assertions are correctly validating the expected behavior.
Verification successful
Assertions are correctly validating the expected behavior.
The test case for
Get all time ranked pnl tickscorrectly sets up the data, invokes thegetRankedPnlTicksfunction, and validates the expected output.
- The length of the
mostRecentarray is correctly asserted to be 2.- The objects within the
mostRecentarray are correctly asserted to match the expected structure and values.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the assertions in the test case `Get all time ranked pnl ticks`. # Test: Search for the assertions. Expect: Correct validation of expected behavior. rg --type js -A 5 $'expect(mostRecent.length).toEqual(2)'Length of output: 58
Script:
#!/bin/bash # Description: Verify the assertions in the test case `Get all time ranked pnl ticks`. # Test: Search for the assertions in the specified test file. Expect: Correct validation of expected behavior. rg --type js -A 10 'expect(mostRecent.length).toEqual(2)' indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.tsLength of output: 126
Script:
#!/bin/bash # Description: Verify the presence of the test case `Get all time ranked pnl ticks` and its assertions. # Test: Search for the entire test case block and include more context lines to capture the assertions. rg --type js -A 30 "it('Get all time ranked pnl ticks'" indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.tsLength of output: 218
Script:
#!/bin/bash # Description: Verify the presence of the test case `Get all time ranked pnl ticks` and its assertions. # Test: Search for the entire test case block and include more context lines to capture the assertions. rg --type js -A 30 "it\('Get all time ranked pnl ticks'" indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.tsLength of output: 1075
414-516: LGTM! But verify the data setup.The function correctly sets up ranked PnL ticks data for the test cases.
However, ensure that the data setup is correctly providing the necessary context for the tests.
Verification successful
LGTM! The data setup in
setupRankedPnlTicksDatais correctly providing the necessary context for the tests.The function sets up ranked PnL ticks data, and the assertions in the test file validate this data thoroughly.
- The function
setupRankedPnlTicksDatais defined and used multiple times within the test file.- The test file contains numerous assertions validating the data set up by this function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the data setup in the function `setupRankedPnlTicksData`. # Test: Search for the data setup. Expect: Correct creation of block and PnL tick data. rg --type js -A 5 $'await PnlTicksTable.createMany'Length of output: 52
Script:
#!/bin/bash # Description: Verify the definition and usage of the function `setupRankedPnlTicksData`. # Search for the definition of the function `setupRankedPnlTicksData`. rg --type js 'function setupRankedPnlTicksData' # Search for the usage of the function `setupRankedPnlTicksData` within the test file. rg --type js 'setupRankedPnlTicksData' indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.tsLength of output: 381
Script:
#!/bin/bash # Description: Verify the validation of the data set up by the function `setupRankedPnlTicksData`. # Search for assertions or validations related to the data set up by `setupRankedPnlTicksData`. rg --type js 'expect' indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.tsLength of output: 2394
343-364: LGTM! But verify the assertions.The test case correctly validates the retrieval of ranked PnL ticks for the "THIRTY_DAYS" timespan.
However, ensure that the assertions are correctly validating the expected behavior.
389-410: LGTM! But verify the assertions.The test case correctly validates the retrieval of ranked PnL ticks for the "ONE_DAY" timespan.
However, ensure that the assertions are correctly validating the expected behavior.
320-341: LGTM! But verify the assertions.The test case correctly validates the retrieval of ranked PnL ticks for the "ONE_YEAR" timespan.
However, ensure that the assertions are correctly validating the expected behavior.
Verification successful
LGTM! But verify the assertions.
The test case correctly validates the retrieval of ranked PnL ticks for the "ONE_YEAR" timespan.
- The length of the
mostRecentarray is correctly asserted to be 2.- The properties of the elements in the
mostRecentarray are correctly validated.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the assertions in the test case `Get one year ranked pnl ticks with missing pnl for one subaccount`. # Test: Search for the assertions. Expect: Correct validation of expected behavior. rg --type js -A 5 $'expect(mostRecent.length).toEqual(2)'Length of output: 58
Script:
#!/bin/bash # Description: Verify the assertions in the test case `Get one year ranked pnl ticks with missing pnl for one subaccount`. # Test: Search for the assertions with more context lines to ensure accurate matching. rg --type js -A 10 'expect(mostRecent.length).toEqual(2)'Length of output: 59
Script:
#!/bin/bash # Description: Locate the file `pnl-ticks-table.test.ts` and inspect its contents for the test case. # Locate the file file_path=$(fd 'pnl-ticks-table.test.ts') # If the file is found, display its contents for verification if [ -n "$file_path" ]; then cat "$file_path" else echo "File not found" fiLength of output: 14878
366-387: LGTM! But verify the assertions.The test case correctly validates the retrieval of ranked PnL ticks for the "SEVEN_DAYS" timespan.
However, ensure that the assertions are correctly validating the expected behavior.
Verification successful
The test case
Get seven days ranked pnl ticksis present and the assertions are correctly validating the expected behavior.
- The test case sets up ranked PnL ticks data.
- It invokes the
getRankedPnlTicksfunction with the "SEVEN_DAYS" timespan.- The assertions check that the length of the result is 2 and that the objects match the expected values.
Verified assertions:
expect(mostRecent.length).toEqual(2);expect(mostRecent[0]).toEqual(expect.objectContaining({ address: defaultAddress, pnl: '20', currentEquity: '1100', timeSpan: 'SEVEN_DAYS', rank: '1' }));expect(mostRecent[1]).toEqual(expect.objectContaining({ address: defaultAddress2, pnl: '-20', currentEquity: '200', timeSpan: 'SEVEN_DAYS', rank: '2' }));Location:
indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the assertions in the test case `Get seven days ranked pnl ticks`. # Test: Search for the assertions. Expect: Correct validation of expected behavior. rg --type js -A 5 $'expect(mostRecent.length).toEqual(2)'Length of output: 58
Script:
#!/bin/bash # Description: Locate the test case `Get seven days ranked pnl ticks` and inspect the assertions within it. # Step 1: Locate the test case by its name. rg --type js -A 20 "it('Get seven days ranked pnl ticks'" # Step 2: Verify the assertions within the located test case. rg --type js -A 20 "expect(mostRecent.length).toEqual(2)"Length of output: 215
Script:
#!/bin/bash # Description: Locate the test case `Get seven days ranked pnl ticks` and inspect the assertions within it. # Step 1: Locate the test case by its name with properly escaped single quotes. rg --type js -A 20 "it(\"Get seven days ranked pnl ticks\")" # Step 2: Verify the assertions within the located test case. rg --type js -A 20 "expect(mostRecent.length).toEqual(2)"Length of output: 120
Script:
#!/bin/bash # Description: Locate the test file `pnl-ticks-table.test.ts` and inspect its contents for the test case and assertions. # Step 1: Locate the test file `pnl-ticks-table.test.ts`. test_file=$(fd pnl-ticks-table.test.ts) # Step 2: Inspect the contents of the located test file for the test case and assertions. if [[ -n "$test_file" ]]; then echo "Test file found: $test_file" echo "Inspecting test case 'Get seven days ranked pnl ticks' and its assertions..." rg --type js -A 20 "it(\"Get seven days ranked pnl ticks\")" "$test_file" rg --type js -A 20 "expect(mostRecent.length).toEqual(2)" "$test_file" else echo "Test file not found." fiLength of output: 787
Script:
#!/bin/bash # Description: Print the contents of the test file `pnl-ticks-table.test.ts` for manual inspection. # Step 1: Locate the test file `pnl-ticks-table.test.ts`. test_file=$(fd pnl-ticks-table.test.ts) # Step 2: Print the contents of the located test file for manual inspection. if [[ -n "$test_file" ]]; then echo "Test file found: $test_file" echo "Printing contents of the test file..." cat "$test_file" else echo "Test file not found." fiLength of output: 15138
indexer/packages/postgres/__tests__/helpers/constants.ts (2)
94-99: LGTM!The constant
defaultSubaccountWithAlternateAddressis well-defined and follows the structure of other subaccount constants in the file.
129-132: LGTM!The constant
defaultSubaccountIdWithAlternateAddressis well-defined and follows the structure of other subaccount ID constants in the file.
affanv14
changed the title
affanv14
force-pushed
the
affan/leaderboard-task
branch
from
e532108 to
7c2b5ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- indexer/packages/postgres/tests/helpers/constants.ts (5 hunks)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (3 hunks)
- indexer/packages/postgres/src/lib/helpers.ts (1 hunks)
- indexer/packages/postgres/src/lib/vault-addresses.json (1 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (4 hunks)
- indexer/packages/postgres/src/types/leaderboard-pnl-types.ts (1 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (2 hunks)
- protocol/scripts/vault/get_vault.go (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
Additional comments not posted (5)
indexer/packages/postgres/src/lib/helpers.ts (2)
7-9: LGTM!The function remains unchanged and is correctly implemented.
12-14: LGTM! Verify the JSON file's existence and format.The function correctly retrieves vault addresses from the JSON file.
Ensure that
vault-addresses.jsonis correctly formatted and accessible.Verification successful
LGTM! The
vault-addresses.jsonfile exists and is in a valid JSON format.
- The function
getVaultAddressescorrectly retrieves vault addresses from the JSON file.- The
vault-addresses.jsonfile is correctly formatted and accessible.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and format of `vault-addresses.json`. # Test: Check if the JSON file exists and is valid. if [ -f "indexer/packages/postgres/src/lib/vault-addresses.json" ]; then echo "File exists." jq empty indexer/packages/postgres/src/lib/vault-addresses.json && echo "Valid JSON format." || echo "Invalid JSON format." else echo "File does not exist." fiLength of output: 215
indexer/packages/postgres/src/types/leaderboard-pnl-types.ts (1)
19-25: LGTM!The enum
LeaderboardPnlTimeSpanis well-defined and enhances functionality by providing granular control over time periods.protocol/scripts/vault/get_vault.go (1)
Line range hint
24-52:
LGTM! Verify the loop processing and output.The changes streamline the function by removing user input dependencies and expanding the functionality to handle multiple vaults in a single execution.
Ensure that the loop processes vault numbers correctly and the output is as expected.
Verification successful
Verified: Loop processing and output logic in
get_vault.goare correctly implemented.The
ast-grepresults confirm the correct usage ofvaultcli.GetVaultTypeFromString,sdk.GetConfig, andvaulttypes.VaultIdwithin the loop. The loop processes vault numbers correctly, and the output logic is in place.
vaultcli.GetVaultTypeFromStringis used to convert the vault type string toVaultType.sdk.GetConfigis used to get the configuration.vaulttypes.VaultIdis used to create the vault ID with the correct type and number.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the loop processing and output in `get_vault.go`. # Test: Check the output for the range of vault numbers. go run protocol/scripts/vault/get_vault.go | head -n 10Length of output: 127
Script:
#!/bin/bash # Verify the usage of vaultcli.GetVaultTypeFromString ast-grep --lang go --pattern 'vaultcli.GetVaultTypeFromString($_)' # Verify the usage of sdk.GetConfig ast-grep --lang go --pattern 'sdk.GetConfig()' # Verify the usage of vaulttypes.VaultId ast-grep --lang go --pattern 'vaulttypes.VaultId { $$$ }'Length of output: 2716
indexer/packages/postgres/src/lib/vault-addresses.json (1)
1-1002: Ensure uniqueness and consistency of identifiers.The JSON array contains 1001 unique identifiers. Verify that all identifiers are unique and consistently formatted.
Verification successful
Identifiers are unique and consistently formatted.
The JSON array contains 1001 unique identifiers, and no duplicates were found. The identifiers are consistently formatted.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the uniqueness and consistency of identifiers in the JSON array. # Test: Check for duplicate identifiers. Expect: No duplicates. jq -r '.[]' indexer/packages/postgres/src/lib/vault-addresses.json | sort | uniq -dLength of output: 85
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- indexer/packages/postgres/tests/helpers/constants.ts (5 hunks)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (3 hunks)
- indexer/packages/postgres/src/lib/helpers.ts (1 hunks)
- indexer/packages/postgres/src/lib/vault-addresses.json (1 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (4 hunks)
- indexer/packages/postgres/src/types/leaderboard-pnl-types.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts (2 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (2 hunks)
- protocol/scripts/vault/get_vault.go (3 hunks)
Files skipped from review as they are similar to previous changes (8)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/src/lib/helpers.ts
- indexer/packages/postgres/src/lib/vault-addresses.json
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts
- indexer/packages/postgres/src/types/leaderboard-pnl-types.ts
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
- protocol/scripts/vault/get_vault.go
Additional comments not posted (12)
indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.ts (12)
3-3: Import addition approved.The import statement for
LeaderboardPnlCreateObjectis correctly added and necessary for the new test cases.
12-13: Import additions approved.The import statements for
WalletTableandSubaccountTableare necessary for creating wallet and subaccount records in thebeforeEachhook.
15-27: Import additions approved.The constants related to default addresses, blocks, pnl ticks, subaccounts, and wallets are necessary for setting up test data and validating the new test cases.
35-36: Changes tobeforeEachhook approved.The
beforeEachhook now includes calls to create wallet and subaccount records, ensuring the test context is correctly set up before each test case.
291-297: Changes to test case approved.The test case
createMany PnlTicks, find most recent pnl ticks for each accounthas been expanded to include assertions for the new leaderboardRankedData, ensuring the correctness of the ranking logic.
300-322: New test case approved.The new test case
Get all time ranked pnl ticksvalidates the retrieval of ranked PnL ticks for the "ALL_TIME" duration, ensuring the returned data matches the expected structure and values.
324-346: New test case approved.The new test case
Get one year ranked pnl ticks with missing pnl for one subaccountvalidates the retrieval of ranked PnL ticks for the "ONE_YEAR" duration, ensuring the returned data matches the expected structure and values, even with missing PnL data for one subaccount.
348-370: New test case approved.The new test case
Get thirty days ranked pnl ticksvalidates the retrieval of ranked PnL ticks for the "THIRTY_DAYS" duration, ensuring the returned data matches the expected structure and values.
372-394: New test case approved.The new test case
Get seven days ranked pnl ticksvalidates the retrieval of ranked PnL ticks for the "SEVEN_DAYS" duration, ensuring the returned data matches the expected structure and values.
396-418: New test case approved.The new test case
Get one day ranked pnl ticksvalidates the retrieval of ranked PnL ticks for the "ONE_DAY" duration, ensuring the returned data matches the expected structure and values.
420-440: New test case approved.The new test case
Ensure that vault addresses are not included in the leaderboardvalidates that vault addresses are excluded from the leaderboard, ensuring the returned data does not include these addresses.
444-545: New function approved.The new function
setupRankedPnlTicksDataensures that the database is correctly seeded with realistic data, which is crucial for validating the accuracy of the ranking logic.
| rows: LeaderboardPnlCreateObject[] | ||
| } = await knexReadReplica.getConnection().raw( | ||
| ` | ||
| WITH latest_subaccount_pnl_x_days_ago_ranked AS ( |
There was a problem hiding this comment.
could you test the runtime of these sql commands in internal mainnet with explain analyze? I'm concerned with how much time they will take
There was a problem hiding this comment.
WindowAgg (cost=367986.26..367989.76 rows=200 width=148) (actual time=76309.323..76318.837 rows=16277 loops=1)
-> Sort (cost=367986.26..367986.76 rows=200 width=108) (actual time=76309.312..76311.359 rows=16277 loops=1)
Sort Key: aggregated_results."totalPnl" DESC
Sort Method: quicksort Memory: 2673kB
-> Subquery Scan on aggregated_results (cost=367956.65..367978.62 rows=200 width=108) (actual time=76284.526..76303.321 rows=16277 loops=1)
-> GroupAggregate (cost=367956.65..367976.62 rows=200 width=108) (actual time=76284.526..76301.609 rows=16277 loops=1)
Group Key: latest_pnl.address
-> Sort (cost=367956.65..367960.89 rows=1697 width=57) (actual time=76284.512..76286.680 rows=18127 loops=1)
Sort Key: latest_pnl.address
Sort Method: quicksort Memory: 3135kB
-> Subquery Scan on latest_pnl (cost=367840.16..367865.61 rows=1697 width=57) (actual time=76139.205..76229.244 rows=18127 loops=1)
-> Unique (cost=367840.16..367848.64 rows=1697 width=97) (actual time=76139.203..76226.909 rows=18127 loops=1)
-> Sort (cost=367840.16..367844.40 rows=1697 width=97) (actual time=76139.202..76195.461 rows=325653 loops=1)
Sort Key: a."subaccountId", a."blockHeight" DESC
Sort Method: external merge Disk: 29928kB
-> Nested Loop (cost=0.57..367749.13 rows=1697 width=97) (actual time=11.067..75743.401 rows=325653 loops=1)
-> Seq Scan on subaccounts b (cost=0.00..674.17 rows=92 width=60) (actual time=0.010..11.217 rows=18128 loops=1)
Filter: (("subaccountNumber" % 128) = 0)
Rows Removed by Filter: 217
-> Index Scan using pnl_ticks_subaccountid_createdat_index on pnl_ticks a (cost=0.57..3989.71 rows=24 width=37) (actual time=4.152..4.174 rows=18 loops=18128)
Index Cond: ("subaccountId" = b.id)
Filter: (("createdAt")::date = CURRENT_DATE)
Rows Removed by Filter: 3651
There was a problem hiding this comment.
WindowAgg (cost=780380.06..780383.56 rows=200 width=148) (actual time=266511.173..266520.779 rows=16277 loops=1)
-> Sort (cost=780380.06..780380.56 rows=200 width=108) (actual time=266511.165..266512.302 rows=16277 loops=1)
Sort Key: aggregated_results."totalPnl" DESC
Sort Method: quicksort Memory: 2673kB
-> Subquery Scan on aggregated_results (cost=780346.20..780372.41 rows=200 width=108) (actual time=266486.377..266506.644 rows=16277 loops=1)
-> GroupAggregate (cost=780346.20..780370.41 rows=200 width=108) (actual time=266486.376..266504.933 rows=16277 loops=1)
Group Key: b.address
-> Sort (cost=780346.20..780350.44 rows=1697 width=64) (actual time=266486.362..266488.777 rows=18127 loops=1)
Sort Key: b.address
Sort Method: quicksort Memory: 3135kB
-> Merge Left Join (cost=779466.17..780255.17 rows=1697 width=64) (actual time=253355.362..266426.130 rows=18127 loops=1)
Merge Cond: (a."subaccountId" = a_1."subaccountId")
-> Unique (cost=367840.17..367848.66 rows=1697 width=97) (actual time=75718.199..75806.954 rows=18127 loops=1)
-> Sort (cost=367840.17..367844.42 rows=1697 width=97) (actual time=75718.198..75779.857 rows=325653 loops=1)
Sort Key: a."subaccountId", a."blockHeight" DESC
Sort Method: external merge Disk: 35032kB
-> Nested Loop (cost=0.57..367749.14 rows=1697 width=97) (actual time=10.950..75394.181 rows=325653 loops=1)
-> Seq Scan on subaccounts b (cost=0.00..674.19 rows=92 width=60) (actual time=0.010..11.709 rows=18129 loops=1)
Filter: (("subaccountNumber" % 128) = 0)
Rows Removed by Filter: 217
-> Index Scan using pnl_ticks_subaccountid_createdat_index on pnl_ticks a (cost=0.57..3989.71 rows=24 width=37) (actual time=4.133..4.155 rows=18 loops=18129)
Index Cond: ("subaccountId" = b.id)
Filter: (("createdAt")::date = CURRENT_DATE)
Rows Removed by Filter: 3651
-> Unique (cost=411625.99..412191.78 rows=14124 width=547) (actual time=177637.157..190608.495 rows=17529 loops=1)
-> Sort (cost=411625.99..411908.88 rows=113157 width=547) (actual time=177637.155..186055.812 rows=63619443 loops=1)
Sort Key: a_1."subaccountId", a_1."blockHeight" DESC
Sort Method: external merge Disk: 2987792kB
-> Nested Loop (cost=0.57..370299.61 rows=113157 width=547) (actual time=0.032..97505.319 rows=63622388 loops=1)
-> Seq Scan on subaccounts b_1 (cost=0.00..674.19 rows=92 width=16) (actual time=0.012..14.078 rows=18129 lo
ops=1)
Filter: (("subaccountNumber" % 128) = 0)
Rows Removed by Filter: 217
-> Index Scan using pnl_ticks_subaccountid_createdat_index on pnl_ticks a_1 (cost=0.57..4001.69 rows=1598 wi
dth=31) (actual time=0.013..4.902 rows=3509 loops=18129)
Index Cond: ("subaccountId" = b_1.id)
Filter: (("createdAt")::date <= (CURRENT_DATE - '7 days'::interval))
Rows Removed by Filter: 159
Planning Time: 0.601 ms
Execution Time: 267091.460 ms
There was a problem hiding this comment.
first one is all time, second one is for 7 day timspan
There was a problem hiding this comment.
oddly testnet has more data than internal mainnet
There was a problem hiding this comment.
why doesnt the first one have an execution or planning time? I think what we really need is just how long each command takes, and you can use explain analyze to optimize it. In this case 263s seems on the long side, and im concerned that it could cause the read replica to lag
| LEFT JOIN | ||
| subaccounts b ON a."subaccountId" = b."id" | ||
| WHERE | ||
| a."createdAt"::date <= (CURRENT_DATE - INTERVAL '${intervalSqlString}') |
There was a problem hiding this comment.
If we're selecting a subaccount that was created 3 days ago and we're getting the 7 days ago ranked, it seems like we wouldn't get a result for them. I think we should sync with Yi on if that's the product outcome we want
There was a problem hiding this comment.
We do get a results for them if you look below a."totalPnl" - COALESCE(b."totalPnl", 0) as "pnlDifference", would just give latest pnl(which is for the last 3 days)
| "address", | ||
| "totalPnl" as "pnl", | ||
| 'ALL_TIME' as "timeSpan", | ||
| "currentEquity", |
There was a problem hiding this comment.
thinking about this a little more, if we want to calculate pnl over this period of time by percentage isn't it:
(ending equity - starting equity - net transfers)/(starting equity + netTransfers/2)
Let's check one more time with Yi that we don't want percentage PNL
There was a problem hiding this comment.
Why netTransfers/2?
old way of doing this was Percent PNL = (Dollar PNL) / max(500, Starting Equity + Deposits During Period)
There was a problem hiding this comment.
Added linear ticket for this one https://linear.app/dydx/project/social-mvp-e029a2c846e4/issues. Since I need to change tables etc
There was a problem hiding this comment.
we can stick to this formula if thats what we've used historically. I do think we should use net deposits rather than deposits during period though
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- indexer/packages/postgres/tests/helpers/constants.ts (5 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (3 hunks)
- indexer/packages/redis/src/caches/leaderboard-processed-cache.ts (1 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/create-leaderboard.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (3 hunks)
- indexer/services/roundtable/src/index.ts (3 hunks)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts (1 hunks)
Additional comments not posted (33)
indexer/packages/redis/src/caches/leaderboard-processed-cache.ts (4)
8-10: LGTM!The
getKeyfunction is correctly implemented to generate a cache key based on the provided period.
12-17: LGTM!The
getProcessedTimefunction is correctly implemented to retrieve the processed time from the cache.
19-25: LGTM!The
setProcessedTimefunction is correctly implemented to set the processed time in the cache.
1-7: LGTM!The imports and constant declaration are correctly implemented.
indexer/services/roundtable/src/tasks/create-leaderboard.ts (3)
15-21: LGTM!The
generateLeaderboardTaskFromTimespanfunction is correctly implemented to generate a leaderboard task based on the provided timespan.
77-84: LGTM!The
insertLeaderboardPnlObjectsfunction is correctly implemented to insert leaderboard PnL objects into the database.
1-14: LGTM!The imports and constant declaration are correctly implemented.
indexer/services/roundtable/__tests__/tasks/create-leaderboard.test.ts (9)
1-13: LGTM! Import statements are appropriate.The imported modules and dependencies are necessary for the tests.
14-18: LGTM! Import statements are appropriate.The imported modules and dependencies are necessary for the tests.
20-24: LGTM! Describe block is well-structured.The describe block sets up the test suite for the
create-leaderboardtask.
26-31: LGTM! beforeEach block is well-structured.The beforeEach block seeds data and sets up the necessary tables for the tests.
33-36: LGTM! afterAll block is well-structured.The afterAll block tears down the test environment and resets mocks.
38-42: LGTM! afterEach block is well-structured.The afterEach block clears data and resets mocks after each test.
44-66: LGTM! Test case is well-structured.The test case verifies the functionality of populating the leaderboard with ranked PnL ticks.
68-82: LGTM! Test case is well-structured.The test case verifies that the leaderboard is not updated if the last processed PnL time is less than the cached leaderboard time.
85-116: LGTM! Helper function is well-structured.The helper function sets up the necessary data for the tests.
indexer/services/roundtable/src/index.ts (5)
3-3: LGTM! Import statement is appropriate.The import statement includes the new
LeaderboardPnlTimeSpanalong with existing imports.
205-213: LGTM! Conditional block for all-time leaderboard PnL task is well-structured.The conditional block checks if the all-time leaderboard PnL task is enabled and starts the loop if true.
214-222: LGTM! Conditional block for daily leaderboard PnL task is well-structured.The conditional block checks if the daily leaderboard PnL task is enabled and starts the loop if true.
223-231: LGTM! Conditional block for weekly leaderboard PnL task is well-structured.The conditional block checks if the weekly leaderboard PnL task is enabled and starts the loop if true.
232-249: LGTM! Conditional blocks for monthly and yearly leaderboard PnL tasks are well-structured.The conditional blocks check if the monthly and yearly leaderboard PnL tasks are enabled and start the loops if true.
indexer/services/roundtable/src/config.ts (3)
55-59: LGTM! Boolean flags for leaderboard PnL tracking are well-structured.The new boolean flags enable leaderboard PnL tracking over different timeframes.
113-127: LGTM! Interval settings for leaderboard PnL tracking are well-structured.The new interval settings configure the update intervals for each leaderboard PnL timeframe.
149-149: LGTM! Configuration option for maximum rows per upsert operation is well-structured.The new configuration option sets the maximum number of rows per upsert operation for leaderboard PnL.
indexer/packages/postgres/src/stores/pnl-ticks-table.ts (3)
327-394: Verify the SQL query and check for potential performance issues.The function
getRankedPnlTicksForTimeSpanconstructs a complex SQL query using CTEs. Ensure the correctness and performance of the SQL query.
409-450: Verify the SQL query and check for potential performance issues.The function
getAllTimeRankedPnlTicksconstructs a SQL query to calculate and rank PnL for the current day. Ensure the correctness and performance of the SQL query.
277-284: Ensure the helper function and called function are correctly implemented.The function
getRankedPnlTicksrelies onconvertTimespanToSQLandgetRankedPnlTicksForTimeSpan. Verify their correctness.Verification successful
Ensure the helper function and called function are correctly implemented.
The functions
convertTimespanToSQLandgetRankedPnlTicksForTimeSpanare correctly implemented and used within theindexer/packages/postgres/src/stores/pnl-ticks-table.tsfile. The helper functionconvertTimespanToSQLconverts a time span to a SQL-compatible string, andgetRankedPnlTicksForTimeSpanperforms a database query using this converted string.
convertTimespanToSQLimplementation atindexer/packages/postgres/src/stores/pnl-ticks-table.tslines 277-284.getRankedPnlTicksForTimeSpanimplementation atindexer/packages/postgres/src/stores/pnl-ticks-table.tslines 285-294.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `convertTimespanToSQL` and `getRankedPnlTicksForTimeSpan`. # Test: Search for the function implementations. Expect: Correct implementation of both functions. rg --type=typescript 'function convertTimespanToSQL' -A 10 rg --type=typescript 'async function getRankedPnlTicksForTimeSpan' -A 10Length of output: 205
Script:
#!/bin/bash # Description: Verify the implementation of `convertTimespanToSQL` and `getRankedPnlTicksForTimeSpan`. # Test: Search for the function implementations. Expect: Correct implementation of both functions. rg 'function convertTimespanToSQL' -A 10 rg 'async function getRankedPnlTicksForTimeSpan' -A 10Length of output: 2271
indexer/packages/postgres/__tests__/helpers/constants.ts (6)
96-101: Verify the object structure and ensure the values are correct.The constant
defaultSubaccountWithAlternateAddressrepresents a subaccount with an alternate address. Verify the object structure and ensure the values are correct.Verification successful
Verified: The object structure and values are correct.
The constant
defaultSubaccountWithAlternateAddresshas the correct structure and values:
address: defaultAddress2subaccountNumber: 0updatedAt: createdDateTime.toISO()updatedAtHeight: createdHeightScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure and values of `defaultSubaccountWithAlternateAddress`. # Test: Search for the object definition. Expect: Correct structure and values. rg --type=typescript 'defaultSubaccountWithAlternateAddress'Length of output: 95
Script:
#!/bin/bash # Description: Verify the structure and values of `defaultSubaccountWithAlternateAddress`. # Test: Search for the object definition. Expect: Correct structure and values. rg --type ts 'defaultSubaccountWithAlternateAddress'Length of output: 709
Script:
#!/bin/bash # Extract the definition of `defaultSubaccountWithAlternateAddress` from the constants file. rg --type ts --context 5 'defaultSubaccountWithAlternateAddress' indexer/packages/postgres/__tests__/helpers/constants.tsLength of output: 779
71-71: Ensure the address is correctly generated and documented.The constant
vaultAddressrepresents a vault address. Verify the address generation and ensure proper documentation.Verification successful
The address is correctly generated and documented.
The constant
vaultAddressrepresents a BTC vault address and is used in multiple configurations. The address generation is linked to theget_vault.goscript, which is documented within the script itself. Additionally, the address is documented in the shell scripts.
indexer/packages/postgres/__tests__/helpers/constants.tsprotocol/testing/testnet-local/local.shprotocol/testing/testnet-staging/staging.shprotocol/testing/testnet-dev/dev.shScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the generation and documentation of `vaultAddress`. # Test: Search for the address generation script. Expect: Proper documentation of the address. rg --type=go 'get_vault.go'Length of output: 140
Script:
#!/bin/bash # Description: Retrieve the contents of the script responsible for generating the vault address. # Fetch the content of the script to verify the address generation and documentation. cat protocol/scripts/vault/get_vault.goLength of output: 1351
Script:
#!/bin/bash # Description: Search for the specific `vaultAddress` value within the repository to find references or documentation. # Search for the specific vault address value in the repository. rg 'dydx1c0m5x87llaunl5sgv3q5vd7j5uha26d2q2r2q0'Length of output: 591
Script:
#!/bin/bash # Description: Retrieve the contents of the files where the specific `vaultAddress` value appears to find references or documentation. # Retrieve content from the JSON file. cat indexer/packages/postgres/src/lib/vault-addresses.json # Retrieve content from the shell scripts. cat protocol/testing/testnet-local/local.sh cat protocol/testing/testnet-staging/staging.sh cat protocol/testing/testnet-dev/dev.shLength of output: 81309
103-108: Verify the object structure and ensure the values are correct.The constant
vaultSubaccountrepresents a vault subaccount. Verify the object structure and ensure the values are correct.Verification successful
The structure and values of
vaultSubaccountare correct.
addressis set tovaultAddress, which is a valid string.subaccountNumberis set to0, which is a valid number.updatedAtis set to the ISO string representation of the current UTC date-time.updatedAtHeightis set tocreatedHeight, which is a valid string.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure and values of `vaultSubaccount`. # Test: Search for the object definition. Expect: Correct structure and values. rg --type=typescript 'vaultSubaccount'Length of output: 73
Script:
#!/bin/bash # Description: Verify the structure and values of `vaultSubaccount`. # Test: Search for the object definition. Expect: Correct structure and values. rg --type ts 'vaultSubaccount'Length of output: 755
Script:
#!/bin/bash # Extract lines around the definition of `vaultSubaccount` to verify its structure and values. rg --type ts 'export const vaultSubaccount: SubaccountCreateObject' -A 5 indexer/packages/postgres/__tests__/helpers/constants.tsLength of output: 311
Script:
#!/bin/bash # Search for the definitions of `vaultAddress`, `createdDateTime`, and `createdHeight` in the same file to verify their values. rg --type ts 'vaultAddress|createdDateTime|createdHeight' indexer/packages/postgres/__tests__/helpers/constants.tsLength of output: 2284
167-170: Verify the object structure and ensure the values are correct.The constant
vaultWalletrepresents a vault wallet. Verify the object structure and ensure the values are correct.Verification successful
Verified: The constant
vaultWalletis correctly defined and used.The structure and values of the
vaultWalletobject are accurate, and its usage context does not indicate any issues.
vaultWalletdefinition:export const vaultWallet: WalletCreateObject = { address: vaultAddress, totalTradingRewards: denomToHumanReadableConversion(0), };Import statement:
vaultWallet,Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure and values of `vaultWallet`. # Test: Search for the object definition. Expect: Correct structure and values. rg --type=typescript 'vaultWallet'Length of output: 69
Script:
#!/bin/bash # Description: Verify the structure and values of `vaultWallet`. # Find all TypeScript files and search for the `vaultWallet` definition. fd -e ts -x rg 'vaultWallet' {}Length of output: 140
Script:
#!/bin/bash # Description: Extract the full context around the `vaultWallet` definition and its usages. # Find the file containing the `vaultWallet` definition and display the surrounding lines for context. fd -e ts -x rg 'export const vaultWallet' -A 5 -B 5 # Display the surrounding lines for the other occurrences of `vaultWallet` to understand its usage context. fd -e ts -x rg 'vaultWallet,' -A 5 -B 5 fd -e ts -x rg 'await WalletTable.create(vaultWallet)' -A 5 -B 5Length of output: 811
138-141: Verify the ID generation.The constant
defaultSubaccountIdWithAlternateAddressrepresents a subaccount ID with an alternate address. Verify the ID generation.
151-154: Verify the ID generation.The constant
vaultSubaccountIdrepresents a vault subaccount ID. Verify the ID generation.
indexer/packages/redis/src/index.ts
Outdated
| @@ -11,6 +11,7 @@ export * as LatestAccountPnlTicksCache from './caches/latest-account-pnl-ticks-c | |||
| export * as CanceledOrdersCache from './caches/canceled-orders-cache'; | |||
| export * as StatefulOrderUpdatesCache from './caches/stateful-order-updates-cache'; | |||
| export * as StateFilledQuantumsCache from './caches/state-filled-quantums-cache'; | |||
| export * as LeadeboardPnlProcessedCache from './caches/leaderboard-processed-cache'; | |||
There was a problem hiding this comment.
Typographical error in export name.
The export name LeadeboardPnlProcessedCache contains a typo. It should be LeaderboardPnlProcessedCache.
- export * as LeadeboardPnlProcessedCache from './caches/leaderboard-processed-cache';
+ export * as LeaderboardPnlProcessedCache from './caches/leaderboard-processed-cache';Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export * as LeadeboardPnlProcessedCache from './caches/leaderboard-processed-cache'; | |
| export * as LeaderboardPnlProcessedCache from './caches/leaderboard-processed-cache'; |
| async function updateLeaderboardPnlTable(timespan: LeaderboardPnlTimeSpan) { | ||
| const leaderboardPnlObjects: LeaderboardPnlCreateObject[] = []; | ||
|
|
||
| const lastProcessedTime: string | null = await LeadeboardPnlProcessedCache.getProcessedTime( | ||
| timespan, redisClient); | ||
| const lastProcessedPnlTime: string = ( | ||
| await PnlTicksTable.findLatestProcessedBlocktimeAndCount()).maxBlockTime; | ||
| if (lastProcessedTime && Date.parse(lastProcessedTime) >= Date.parse(lastProcessedPnlTime)) { | ||
| logger.info({ | ||
| at: 'create-leaderboard#runTask', | ||
| message: 'Skipping run because last current pnl ticks have been processed leaderboard', | ||
| pnlTickLatestBlocktime: lastProcessedPnlTime, | ||
| latestBlockTime: lastProcessedTime, | ||
| threshold: config.PNL_TICK_UPDATE_INTERVAL_MS, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| leaderboardPnlObjects.push(...await PnlTicksTable.getRankedPnlTicks(timespan)); | ||
| } catch (error) { | ||
| logger.error({ | ||
| at: 'create-leaderboard#runTask', | ||
| message: `Error when getting ranked pnl ticks for timespan${timespan.toString()}`, | ||
| error, | ||
| timespan, | ||
| }); | ||
| return; | ||
| } | ||
| const txId: number = await Transaction.start(); | ||
| try { | ||
| await insertLeaderboardPnlObjects(leaderboardPnlObjects, txId); | ||
| await Transaction.commit(txId); | ||
| } catch (error) { | ||
| logger.error({ | ||
| at: 'create-leaderboard#runTask', | ||
| message: 'Error when inserting leaderboard pnl objects', | ||
| error, | ||
| }); | ||
| await Transaction.rollback(txId); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await LeadeboardPnlProcessedCache.setProcessedTime(timespan, lastProcessedPnlTime, redisClient); | ||
| } catch (error) { | ||
| logger.error({ | ||
| at: 'create-leaderboard#runTask', | ||
| message: 'Error when setting processed time', | ||
| error, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Typographical error in log message.
There is a typo in the log message at line 33. It should be "Skipping run because the current pnl ticks have been processed for the leaderboard".
- message: 'Skipping run because last current pnl ticks have been processed leaderboard',
+ message: 'Skipping run because the current pnl ticks have been processed for the leaderboard',Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function updateLeaderboardPnlTable(timespan: LeaderboardPnlTimeSpan) { | |
| const leaderboardPnlObjects: LeaderboardPnlCreateObject[] = []; | |
| const lastProcessedTime: string | null = await LeadeboardPnlProcessedCache.getProcessedTime( | |
| timespan, redisClient); | |
| const lastProcessedPnlTime: string = ( | |
| await PnlTicksTable.findLatestProcessedBlocktimeAndCount()).maxBlockTime; | |
| if (lastProcessedTime && Date.parse(lastProcessedTime) >= Date.parse(lastProcessedPnlTime)) { | |
| logger.info({ | |
| at: 'create-leaderboard#runTask', | |
| message: 'Skipping run because last current pnl ticks have been processed leaderboard', | |
| pnlTickLatestBlocktime: lastProcessedPnlTime, | |
| latestBlockTime: lastProcessedTime, | |
| threshold: config.PNL_TICK_UPDATE_INTERVAL_MS, | |
| }); | |
| return; | |
| } | |
| try { | |
| leaderboardPnlObjects.push(...await PnlTicksTable.getRankedPnlTicks(timespan)); | |
| } catch (error) { | |
| logger.error({ | |
| at: 'create-leaderboard#runTask', | |
| message: `Error when getting ranked pnl ticks for timespan${timespan.toString()}`, | |
| error, | |
| timespan, | |
| }); | |
| return; | |
| } | |
| const txId: number = await Transaction.start(); | |
| try { | |
| await insertLeaderboardPnlObjects(leaderboardPnlObjects, txId); | |
| await Transaction.commit(txId); | |
| } catch (error) { | |
| logger.error({ | |
| at: 'create-leaderboard#runTask', | |
| message: 'Error when inserting leaderboard pnl objects', | |
| error, | |
| }); | |
| await Transaction.rollback(txId); | |
| return; | |
| } | |
| try { | |
| await LeadeboardPnlProcessedCache.setProcessedTime(timespan, lastProcessedPnlTime, redisClient); | |
| } catch (error) { | |
| logger.error({ | |
| at: 'create-leaderboard#runTask', | |
| message: 'Error when setting processed time', | |
| error, | |
| }); | |
| } | |
| } | |
| logger.info({ | |
| at: 'create-leaderboard#runTask', | |
| message: 'Skipping run because the current pnl ticks have been processed for the leaderboard', | |
| pnlTickLatestBlocktime: lastProcessedPnlTime, | |
| latestBlockTime: lastProcessedTime, | |
| threshold: config.PNL_TICK_UPDATE_INTERVAL_MS, | |
| }); |
affanv14
force-pushed
the
affan/leaderboard-task
branch
from
a06a3b1 to
fd07dca
Compare
affanv14
force-pushed
the
affan/leaderboard-task
branch
from
fd07dca to
0b2edf4
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
indexer/packages/postgres/src/stores/pnl-ticks-table.ts (1)
277-284: LGTM! Consider adding a docstring for clarity.The function
getRankedPnlTicksis straightforward and delegates the work to other functions based on thetimeSpanparameter. Adding a docstring would improve readability./** * Retrieves ranked PnL ticks based on the specified time span. * Delegates to `getAllTimeRankedPnlTicks` for 'ALL_TIME' span. * * @param {string} timeSpan - The time span for which to retrieve ranked PnL ticks. * @returns {Promise<LeaderboardPnlCreateObject[]>} - A promise that resolves to an array of leaderboard PnL objects. */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- indexer/packages/postgres/tests/helpers/constants.ts (5 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (3 hunks)
- indexer/packages/redis/src/caches/leaderboard-processed-cache.ts (1 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/create-leaderboard.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (3 hunks)
- indexer/services/roundtable/src/index.ts (3 hunks)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/redis/src/caches/leaderboard-processed-cache.ts
- indexer/packages/redis/src/index.ts
- indexer/services/roundtable/tests/tasks/create-leaderboard.test.ts
- indexer/services/roundtable/src/config.ts
- indexer/services/roundtable/src/index.ts
- indexer/services/roundtable/src/tasks/create-leaderboard.ts
Additional comments not posted (2)
indexer/packages/postgres/src/stores/pnl-ticks-table.ts (2)
409-450: LGTM! Consider adding detailed docstrings for clarity.The function
getAllTimeRankedPnlTicksis well-constructed. Adding detailed docstrings would improve readability and maintainability./** * Constructs a query to calculate and rank the Profit and Loss (PnL) and current equity of * subaccounts for the current day. This query is divided into 3 main parts: * 1. latest_pnl: This selects the most recent PnL tick for each Parent subaccount * and associated child subaccounts. It filters subaccounts based on the current date. * Additionally, it excludes any addresses that are vault addresses. * 2. aggregated_results: This CTE aggregates the results from latest_pnl by address. * It sums up the total PnL and current equity for each address. * 3. The final SELECT statement calculates a rank for each address based on the total PnL in * descending order along with associated fields. * * @returns {Promise<LeaderboardPnlCreateObject[]>} - A promise that resolves to an array of leaderboard PnL objects. */
327-394: LGTM! Consider adding detailed docstrings for clarity.The function
getRankedPnlTicksForTimeSpanis complex and involves multiple steps. Adding detailed docstrings would improve readability and maintainability./** * Constructs a complex SQL query to calculate the PnL difference and current equity * of subaccounts over a specified time span, ranking them by their PnL. * * This has 5 main parts: * 1. latest_subaccount_pnl_x_days_ago: Identifies the most recent PnL tick for each subaccount * before the specified time span. It filters out subaccounts which are not parent * subaccounts or associated child subaccounts. It also excludes any addresses * that are vault addresses. * 2. latest_pnl: Finds the latest PnL tick for each subaccount as of the current date, * applying the same filters as latest_subaccount_pnl_x_days_ago. * 3. subaccount_pnl_difference: Calculates the difference in PnL between the * current date and the start of the specified time span for each subaccount. * 4. aggregated_results: Aggregates the PnL differences and current equity for * all subaccounts, grouping by address. * 5. The final SELECT statement then ranks the addresses based on their total PnL * in descending order, providing a snapshot of subaccount performance over the * specified time span. * * @param {string} timeSpan - The time span for which to retrieve ranked PnL ticks. * @returns {Promise<LeaderboardPnlCreateObject[]>} - A promise that resolves to an array of leaderboard PnL objects. */Verification successful
LGTM! Consider adding detailed docstrings for clarity.
The function
getRankedPnlTicksForTimeSpanis complex and involves multiple steps. Adding detailed docstrings would improve readability and maintainability. The verification script confirmed that the required tables and columns exist, indicating that the SQL query structure and logic are correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the performance of the SQL query in `getRankedPnlTicksForTimeSpan`. # Test: Run EXPLAIN ANALYZE on the SQL query. Expect: Acceptable performance metrics. echo " WITH latest_subaccount_pnl_x_days_ago AS ( SELECT DISTINCT ON (a.\"subaccountId\") a.\"subaccountId\", a.\"totalPnl\", b.\"address\" FROM pnl_ticks a LEFT JOIN subaccounts b ON a.\"subaccountId\" = b.\"id\" WHERE a.\"createdAt\"::date <= (CURRENT_DATE - INTERVAL '7 days') AND (b.\"subaccountNumber\" % 128) = 0 AND b.\"address\" NOT IN ('vaultAddress1', 'vaultAddress2') ORDER BY a.\"subaccountId\", a.\"blockHeight\" DESC ), latest_pnl as ( SELECT DISTINCT ON (a.\"subaccountId\") \"subaccountId\", \"totalPnl\", \"equity\" as \"currentEquity\", \"address\" FROM pnl_ticks a left join subaccounts b ON a.\"subaccountId\"=b.\"id\" WHERE \"createdAt\"::date = CURRENT_DATE AND (b.\"subaccountNumber\" % 128) = 0 AND b.\"address\" NOT IN ('vaultAddress1', 'vaultAddress2') ORDER BY a.\"subaccountId\", \"blockHeight\" DESC ), subaccount_pnl_difference as( SELECT a.\"address\", a.\"totalPnl\" - COALESCE(b.\"totalPnl\", 0) as \"pnlDifference\", a.\"currentEquity\" as \"currentEquity\" FROM latest_pnl a left join latest_subaccount_pnl_x_days_ago b ON a.\"subaccountId\"=b.\"subaccountId\" ), aggregated_results as( SELECT \"address\", sum(subaccount_pnl_difference.\"pnlDifference\") as \"totalPnl\", sum(subaccount_pnl_difference.\"currentEquity\") as \"currentEquity\" FROM subaccount_pnl_difference GROUP BY address ) SELECT \"address\", \"totalPnl\" as \"pnl\", '7 days' as \"timeSpan\", \"currentEquity\", ROW_NUMBER() over (order by aggregated_results.\"totalPnl\" desc) as rank FROM aggregated_results; " | psql -d your_database -c "EXPLAIN ANALYZE"Length of output: 1741
Script:
#!/bin/bash # Description: Verify the correctness of the SQL query structure and logic in `getRankedPnlTicksForTimeSpan`. # Check if the required tables and columns exist in the codebase. # This script will search for the definitions of the tables and columns used in the SQL query. # Search for the definition of the `pnl_ticks` table. echo "Searching for the definition of the 'pnl_ticks' table..." rg -A 5 'CREATE TABLE.*pnl_ticks' . # Search for the definition of the `subaccounts` table. echo "Searching for the definition of the 'subaccounts' table..." rg -A 5 'CREATE TABLE.*subaccounts' . # Check if the required columns exist in the `pnl_ticks` table. echo "Checking for required columns in the 'pnl_ticks' table..." rg -A 5 'pnl_ticks' | grep -E 'subaccountId|totalPnl|createdAt|blockHeight' # Check if the required columns exist in the `subaccounts` table. echo "Checking for required columns in the 'subaccounts' table..." rg -A 5 'subaccounts' | grep -E 'id|address|subaccountNumber'Length of output: 209673
Comments failed to post (1)
indexer/packages/postgres/src/stores/pnl-ticks-table.ts
286-301: LGTM! Consider using a map for better maintainability.
The function
convertTimespanToSQLis correct but using a map instead of a switch-case structure would make it easier to maintain.function convertTimespanToSQL(timeSpan: string): string { const timeSpanMap: { [key in LeaderboardPnlTimeSpan]: string } = { [LeaderboardPnlTimeSpan.ONE_DAY]: '1 days', [LeaderboardPnlTimeSpan.SEVEN_DAYS]: '7 days', [LeaderboardPnlTimeSpan.THIRTY_DAYS]: '30 days', [LeaderboardPnlTimeSpan.ONE_YEAR]: '365 days', }; const interval = timeSpanMap[LeaderboardPnlTimeSpan[timeSpan as keyof typeof LeaderboardPnlTimeSpan]]; if (!interval) { throw new Error(`Invalid time span: ${timeSpan}`); } return interval; }
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- indexer/packages/postgres/tests/helpers/constants.ts (5 hunks)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (3 hunks)
- indexer/packages/postgres/src/lib/helpers.ts (1 hunks)
- indexer/packages/postgres/src/lib/vault-addresses.json (1 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (3 hunks)
- indexer/packages/postgres/src/types/leaderboard-pnl-types.ts (1 hunks)
- indexer/packages/redis/src/caches/leaderboard-processed-cache.ts (1 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/create-leaderboard.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (3 hunks)
- indexer/services/roundtable/src/index.ts (3 hunks)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts
- indexer/packages/redis/src/caches/leaderboard-processed-cache.ts
- indexer/packages/redis/src/index.ts
- indexer/services/roundtable/tests/tasks/create-leaderboard.test.ts
- indexer/services/roundtable/src/config.ts
- indexer/services/roundtable/src/index.ts
- indexer/services/roundtable/src/tasks/create-leaderboard.ts
Additional comments not posted (13)
indexer/packages/postgres/src/lib/helpers.ts (2)
5-5: LGTM! Import statement forvaultAddressesis correct.The import statement for
vaultAddressesfrom the JSON file is correctly added.
12-14: LGTM! FunctiongetVaultAddressesis correctly implemented.The function correctly returns the imported
vaultAddresses.indexer/packages/postgres/src/types/leaderboard-pnl-types.ts (1)
19-25: LGTM! EnumerationLeaderboardPnlTimeSpanis correctly defined.The enumeration includes five distinct time span options:
ONE_DAY,SEVEN_DAYS,THIRTY_DAYS,ONE_YEAR, andALL_TIME.indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.ts (9)
3-3: LGTM! Import statement forLeaderboardPnlCreateObjectis correct.The import statement for
LeaderboardPnlCreateObjectis correctly added.
291-297: LGTM! Variable rename frommostRecenttoleaderboardRankedDatais correct.The variable rename accurately reflects its new purpose in the context of leaderboard data retrieval.
300-322: LGTM! Test case forALL_TIMEranked PnL ticks is correctly implemented.The test case correctly retrieves and asserts properties of the
leaderboardRankedDatafor theALL_TIMEtime span.
324-346: LGTM! Test case forONE_YEARranked PnL ticks is correctly implemented.The test case correctly retrieves and asserts properties of the
leaderboardRankedDatafor theONE_YEARtime span.
348-370: LGTM! Test case forTHIRTY_DAYSranked PnL ticks is correctly implemented.The test case correctly retrieves and asserts properties of the
leaderboardRankedDatafor theTHIRTY_DAYStime span.
372-394: LGTM! Test case forSEVEN_DAYSranked PnL ticks is correctly implemented.The test case correctly retrieves and asserts properties of the
leaderboardRankedDatafor theSEVEN_DAYStime span.
396-418: LGTM! Test case forONE_DAYranked PnL ticks is correctly implemented.The test case correctly retrieves and asserts properties of the
leaderboardRankedDatafor theONE_DAYtime span.
420-440: LGTM! Test case to ensure vault addresses are excluded from the leaderboard is correctly implemented.The test case correctly asserts that vault addresses are not included in the
leaderboardRankedData.
444-546: LGTM! Helper functionsetupRankedPnlTicksDatais correctly implemented.The function correctly sets up the necessary test data for the new test cases.
indexer/packages/postgres/src/lib/vault-addresses.json (1)
1-1002: LGTM! The JSON structure and content are well-formed.The JSON array is correctly formatted, and all addresses follow a consistent pattern. No syntax errors or inconsistencies were found.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/roundtable/tests/tasks/create-leaderboard.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/tests/tasks/create-leaderboard.test.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts
| @@ -67,6 +67,8 @@ export const dydxChain: string = 'dydx'; | |||
| export const defaultAddress: string = 'dydx1n88uc38xhjgxzw9nwre4ep2c8ga4fjxc565lnf'; | |||
| export const defaultAddress2: string = 'dydx1n88uc38xhjgxzw9nwre4ep2c8ga4fjxc575lnf'; | |||
| export const blockedAddress: string = 'dydx1f9k5qldwmqrnwy8hcgp4fw6heuvszt35egvtx2'; | |||
| // Vault address was generated using script protocol/scripts/vault/get_vault.go | |||
| export const vaultAddress: string = 'dydx1c0m5x87llaunl5sgv3q5vd7j5uha26d2q2r2q0'; | |||
There was a problem hiding this comment.
nit: also add that this is for vault id 0
| })); | ||
| }); | ||
|
|
||
| it('Get thirty days ranked pnl ticks', async () => { |
There was a problem hiding this comment.
is it possible to do an it.each for these tests?
| }); | ||
|
|
||
| it('Get all time ranked pnl ticks', async () => { |
There was a problem hiding this comment.
Could you also add a test case for if you're trying to get a month of data and there is < 1 month worth of data?
There was a problem hiding this comment.
there is already a test case to get a year of data and there is < 1 year worth of data. Should translate to month the same way
| const lastProcessedTime: string | null = await LeaderboardPnlProcessedCache.getProcessedTime( | ||
| timespan, redisClient); | ||
| const lastProcessedPnlTime: string = ( | ||
| await PnlTicksTable.findLatestProcessedBlocktimeAndCount()).maxBlockTime; | ||
| if (lastProcessedTime && Date.parse(lastProcessedTime) >= Date.parse(lastProcessedPnlTime)) { | ||
| logger.info({ | ||
| at: 'create-leaderboard#runTask', | ||
| message: 'Skipping run because the current pnl ticks have been processed for the leaderboard', | ||
| pnlTickLatestBlocktime: lastProcessedPnlTime, | ||
| latestBlockTime: lastProcessedTime, | ||
| threshold: config.PNL_TICK_UPDATE_INTERVAL_MS, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the indexer is brought into an unhealthy state, the redis is cleared and the postgres is reset from an old state. In this case lastProcessedTime will be null, because LeaderboardPnlProcessedCache will be empty. How will you recover from this state? Please document this too
There was a problem hiding this comment.
if lastProcessedTime is null, we run the the queries and update leaderboard table. So thats fine
for the case where we have an old state, the leaderboard too will process using an old state from the pnl ticks table - but will update to the latest state as soon as the pnl ticks table is updated to the latest state.
There was a problem hiding this comment.
I'll update comments
| const chunkedLeaderboardPnlObjects = _.chunk( | ||
| leaderboardPnlObjects, config.LEADERBOARD_PNL_MAX_ROWS_PER_UPSERT); |
There was a problem hiding this comment.
| const chunkedLeaderboardPnlObjects = _.chunk( | |
| leaderboardPnlObjects, config.LEADERBOARD_PNL_MAX_ROWS_PER_UPSERT); | |
| const chunkedLeaderboardPnlObjects = _.chunk( | |
| leaderboardPnlObjects, | |
| config.LEADERBOARD_PNL_MAX_ROWS_PER_UPSERT, | |
| ); |
nit: a little prettier to have each argument on separate lines, this is the general preference across the entire codebase
| message: 'Error when setting processed time', | ||
| error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
nit: Instead of having 3 try catches in the same function which is messy, it's cleaner if each of these live in its own function
| } | ||
|
|
||
| try { | ||
| leaderboardPnlObjects.push(...await PnlTicksTable.getRankedPnlTicks(timespan)); |
There was a problem hiding this comment.
I wonder if we want a read lock here just to ensure data consistency
There was a problem hiding this comment.
Thought about this a bit - the queries take quite a bit of time and I wouldnt wanna hold a read lock for these queries.
| export default function generateLeaderboardTaskFromTimespan( | ||
| timespan: LeaderboardPnlTimeSpan, | ||
| ): () => Promise<void> { | ||
| return async () => { | ||
| await updateLeaderboardPnlTable(timespan); | ||
| }; | ||
| } | ||
|
|
||
| async function updateLeaderboardPnlTable(timespan: LeaderboardPnlTimeSpan) { |
There was a problem hiding this comment.
nit: codebase standard is to name the task being run runTask and have the generator be similar in someway: https://github.com/dydxprotocol/v4-chain/blob/cl_elliptic_stats/indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts#L68
You can see in all other tasks the exported function is named runTask
| export default function generateLeaderboardTaskFromTimespan( | ||
| timespan: LeaderboardPnlTimeSpan, | ||
| ): () => Promise<void> { | ||
| return async () => { | ||
| await updateLeaderboardPnlTable(timespan); | ||
| }; | ||
| } | ||
|
|
||
| async function updateLeaderboardPnlTable(timespan: LeaderboardPnlTimeSpan) { |
There was a problem hiding this comment.
Could you include some high level documentation on the steps that this task goes through? I.e:
1. decide if the task should be run based on x criteria. If cache is empty then y
2. Read pnl ticks
3. upload leaderboard pnl
4. update cache
Something a little better written out than ^^
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- indexer/packages/postgres/tests/helpers/constants.ts (5 hunks)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (3 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (3 hunks)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts
- indexer/services/roundtable/src/tasks/create-leaderboard.ts
| pnlTickLatestBlocktime: lastProcessedPnlTime, | ||
| latestBlockTime: lastProcessedTime, | ||
| threshold: config.PNL_TICK_UPDATE_INTERVAL_MS, | ||
| }); |
There was a problem hiding this comment.
nit: in the logs and errors can you also include timespan?
| // Check if the last processed time is greater than or equal to the latest block time. | ||
| // In cases where indexer is from previous state, the last processed time may be null. | ||
| // In that case update the leaderboard. | ||
| if (lastProcessedTime && Date.parse(lastProcessedTime) >= Date.parse(lastProcessedPnlTime)) { |
There was a problem hiding this comment.
is lastProcessedTime is null, what does this return? Date.parse(lastProcessedTime)
There was a problem hiding this comment.
if lastProcessedTime is null, it shouldnt even evaluate the second part of the && - Date.parse(lastProcessedTime) should never be reached
| [], | ||
| ); | ||
| expect(leaderboardResults.length).toEqual(0); | ||
| }); |
There was a problem hiding this comment.
Please add testcase for if redis cache is deleted
There was a problem hiding this comment.
redis cache is empty for all test cases except 'leaderboard not updated if last processed pnl time < cached leaderboard time' where I explicitly set it
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/tasks/create-leaderboard.ts
Changelist
Test Plan
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes