[OTE-755] Wrap wallet total volume roundtable queries in transaction #2309
Conversation
…dtable-affiliate-info
WalkthroughThe changes primarily involve refactoring the wallet and affiliate info logic in the indexer package. Key modifications include the removal of specific test cases, updates to method signatures, and enhancements to transaction handling. The documentation for functions has been clarified, and the logic for updating total volume has been streamlined. Overall, the updates focus on improving clarity, efficiency, and error handling across the codebase. Changes
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Files skipped from review as they are similar to previous changes (3)
Additional comments not posted (10)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (4)
indexer/packages/postgres/src/stores/affiliate-info-table.ts (1)
110-110: Consider adding a type for thetxIdparameter.To improve type safety, consider adding a type for the
txIdparameter. For example:type TransactionId = number; export async function updateInfo( windowStartTs: string, windowEndTs: string, txId: TransactionId | undefined = undefined, ) : Promise<void> { // ... }indexer/services/roundtable/src/tasks/update-wallet-total-volume.ts (1)
19-20: Typo in comment: "affilitate" should be "affiliate"There's a typo in the comment on line 20. The word "affilitate" should be corrected to "affiliate".
Apply this diff to fix the typo:
- // cache and affilitate info table are in sync. + // cache and affiliate info table are in sync.indexer/packages/postgres/src/stores/wallet-table.ts (1)
Line range hint
133-163: Parameterize the SQL query to prevent SQL injectionUsing string interpolation to insert
windowStartTsandwindowEndTsdirectly into the SQL query can lead to SQL injection vulnerabilities if these values are not properly sanitized. It's recommended to parameterize the query to enhance security.Apply this diff to parameterize the query:
const query = ` WITH fills_total AS ( -- Step 1: Calculate total volume for each subaccountId SELECT "subaccountId", SUM("price" * "size") AS "totalVolume" FROM fills - WHERE "createdAt" > '${windowStartTs}' AND "createdAt" <= '${windowEndTs}' + WHERE "createdAt" > ? AND "createdAt" <= ? GROUP BY "subaccountId" ), subaccount_volume AS ( -- Step 2: Merge with subaccounts table to get the address SELECT s."address", f."totalVolume" FROM fills_total f JOIN subaccounts s ON f."subaccountId" = s."id" ), address_volume AS ( -- Step 3: Group by address and sum the totalVolume SELECT "address", SUM("totalVolume") AS "totalVolume" FROM subaccount_volume GROUP BY "address" ) -- Step 4: Left join the result with the wallets table and update the total volume UPDATE wallets SET "totalVolume" = COALESCE(wallets."totalVolume", 0) + av."totalVolume" FROM address_volume av WHERE wallets."address" = av."address"; `; - return transaction - ? knexPrimary.raw(query).transacting(transaction) - : knexPrimary.raw(query); + const bindings = [windowStartTs, windowEndTs]; + return transaction + ? knexPrimary.raw(query, bindings).transacting(transaction) + : knexPrimary.raw(query, bindings);indexer/services/roundtable/__tests__/tasks/update-wallet-total-volume.test.ts (1)
178-180: Clarify the number of backfill iterations for better understandingIn the comment, it mentions running the backfill task seven times, but it's not immediately clear why seven iterations are necessary. Consider explaining how this number relates to the date range or calculating it programmatically to enhance clarity.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- indexer/packages/postgres/tests/stores/wallet-table.test.ts (5 hunks)
- indexer/packages/postgres/src/stores/affiliate-info-table.ts (1 hunks)
- indexer/packages/postgres/src/stores/wallet-table.ts (3 hunks)
- indexer/services/roundtable/tests/tasks/update-wallet-total-volume.test.ts (5 hunks)
- indexer/services/roundtable/src/tasks/update-wallet-total-volume.ts (2 hunks)
Additional comments not posted (6)
indexer/packages/postgres/__tests__/stores/wallet-table.test.ts (4)
14-15: LGTM!The imports are valid and used appropriately in the test file.
100-102: LGTM!The variable renaming improves code clarity. The logic to retrieve the wallet from the database is correct.
114-115: LGTM!The variable renaming improves code clarity. The logic to retrieve the wallet from the database is correct.
Line range hint
132-172: LGTM!The refactoring of the
populateWalletSubaccountFillfunction improves code efficiency and readability:
- Using
Promise.allfor concurrent execution of fill creation is a good optimization.- Simplifying the subaccount IDs to use single variables instead of an array enhances code clarity.
The logic and syntax of the function are correct.
indexer/packages/postgres/src/stores/affiliate-info-table.ts (1)
110-110: Verify the function signature change in the codebase.The change to the
updateInfofunction signature looks good. It simplifies the function by directly accepting thetxIdparameter instead of an options object.However, ensure that all function calls to
updateInfohave been updated to match the new signature.Run the following script to verify the function usage:
Verification successful
Function signature change verified successfully
The
updateInfofunction signature change has been successfully verified across the codebase. All calls toupdateInfoare using the new signature correctly:
- In
indexer/services/roundtable/src/tasks/update-affiliate-info.ts, the function is called with all three parameters, includingtxId.- In the test file
indexer/packages/postgres/__tests__/stores/affiliate-info-table.test.ts, multiple calls are made with only two parameters (windowStartTs and windowEndTs), omitting the optionaltxIdparameter, which is valid according to the new signature.The function definition in
indexer/packages/postgres/src/stores/affiliate-info-table.tsmatches the expected new signature:export async function updateInfo( windowStartTs: string, windowEndTs: string, txId: number | undefined = undefined ) : Promise<void>Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `updateInfo` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'updateInfo'Length of output: 73
Script:
#!/bin/bash # Description: Verify all function calls to `updateInfo` match the new signature. # Search for the function usage without specifying file type echo "Searching for updateInfo function calls:" rg -A 5 'updateInfo\s*\(' # Use ast-grep to search for function calls to updateInfo echo "Using ast-grep to search for updateInfo function calls:" ast-grep --lang typescript --pattern 'updateInfo($_)'Length of output: 4825
indexer/services/roundtable/__tests__/tasks/update-wallet-total-volume.test.ts (1)
163-163: Simplify DateTime comparison by removing redundant conversionsSince
backfillTimeis already aDateTimeobject, you can compare it directly withcurrentDtwithout converting to an ISO string and back:- while (backfillTime !== undefined && DateTime.fromISO(backfillTime.toISO()) < currentDt) { + while (backfillTime !== undefined && backfillTime < currentDt) {This simplifies the code and improves readability.
Likely invalid or redundant comment.
indexer/services/roundtable/__tests__/tasks/update-wallet-total-volume.test.ts
Show resolved
Hide resolved
jerryfan01234
force-pushed
the
affiliates-roundtable-total-volume
branch
from
4d574a2 to
4db9ad7
Compare
| ...defaultWallet, | ||
| totalVolume: '105', // 103 + 2 | ||
| })); | ||
| }); | ||
|
|
||
| it('Successfully upserts persistent cache', async () => { |
There was a problem hiding this comment.
why delete this test?
There was a problem hiding this comment.
The query to update total volume was modified to not update the persistent cache. The roundtable task calls persistent cache upsert within the same transaction as total volume update.
This pattern is what the affiliate info uses.
There was a problem hiding this comment.
Persistent cache updating is checked in the roundtable task unit tests.
Changelist
remove persistent cache update from wallet total volume update query
wrap wallet total volume roundtable queries in transaction
styling changes
Test Plan
[Describe how this PR was tested (if applicable)]
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
Bug Fixes
New Features
Refactor
updateInfofunction documentation.Tests