Do not overwrite existing CLOSE_ONLY/BLOCKED compliance statuses #1926
Conversation
WalkthroughThe changes primarily involve enhancing the handling of compliance status data within the system. The updates include modifying import entities, altering the logic for filtering compliance statuses, and enabling the handling of multiple compliance statuses in queries. Additionally, a new test case was added to verify the updated compliance status behavior. These modifications aim to improve the efficiency and accuracy of compliance status updates and queries. Changes
Sequence Diagram(s)sequenceDiagram
participant Task as update-compliance-data.ts
participant DB as ComplianceStatusTable
participant Status as ComplianceStatusFromDatabase
Task->>DB: Fetch compliance statuses
DB-->>Task: Return statuses as array
Task->>Status: Process and filter statuses
Status-->>Task: Return filtered statuses
Task->>DB: Upsert filtered compliance statuses
DB-->>Task: Confirmation of upsert
Poem
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 (
|
![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
Outside diff range, codebase verification and nitpick comments (1)
indexer/services/roundtable/src/tasks/update-compliance-data.ts (1)
Line range hint
186-212: Approve changes but consider performance implications.The introduction of
closeOnlyStatusesandcloseOnlyAddressesto filter compliance status updates aligns with the PR objectives. However, consider the performance impact of these operations, especially with large datasets.Consider implementing more efficient querying or caching mechanisms if performance issues arise.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/roundtable/src/tasks/update-compliance-data.ts (4 hunks)
Additional comments not posted (2)
indexer/services/roundtable/src/tasks/update-compliance-data.ts (2)
9-12: Verify correct integration of new imports.New entities
ComplianceStatusFromDatabaseandComplianceStatusUpsertObjecthave been introduced, andComplianceTablehas been reordered. Ensure that these changes are integrated correctly throughout the codebase.Verification successful
Verified correct integration of new imports.
The new entities
ComplianceStatusFromDatabase,ComplianceStatusUpsertObject, andComplianceTableare used correctly across the codebase, including in test files and other modules. No issues were found related to their integration.
ComplianceStatusFromDatabaseandComplianceStatusUpsertObjectare used in multiple files, includingcompliance-status-table.test.tsandupdate-compliance-data.ts.ComplianceTableis used in files likeupdate-compliance-data.tsandcompliance-controller.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of new imports in the codebase. # Test: Search for the usage of new imports. Expect: Correct usage patterns. rg --type typescript -A 5 $'ComplianceStatusFromDatabase|ComplianceStatusUpsertObject|ComplianceTable'Length of output: 138
Script:
#!/bin/bash # Description: Verify integration of new imports in the codebase. # Test: Search for the usage of new imports. Expect: Correct usage patterns. rg --type ts -A 5 $'ComplianceStatusFromDatabase|ComplianceStatusUpsertObject|ComplianceTable'Length of output: 64906
283-289: Verify and enhance error handling.The error handling in
getComplianceDatafunction logs failed compliance data retrievals. Verify that the error details provided are sufficient for troubleshooting and consider enhancing the error logging to include more contextual information if necessary.Verification successful
Error handling in
getComplianceDatafunction is sufficient.The error logging in the
getComplianceDatafunction includes detailed contextual information, making it adequate for troubleshooting.
indexer/services/roundtable/src/tasks/update-compliance-data.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error logging details. # Test: Search for error logging patterns. Expect: Sufficient contextual information in logs. rg --type typescript -A 5 $'logger.error'Length of output: 75
Script:
#!/bin/bash # Description: Verify error logging details. # Test: Search for error logging patterns. Expect: Sufficient contextual information in logs. rg --glob '*.ts' -A 5 'logger.error'Length of output: 80631
dydxwill
changed the title
| const closeOnlyStatuses: ComplianceStatusFromDatabase[] = await | ||
| ComplianceStatusTable.findAll( | ||
| { | ||
| address: addressesToQuery, | ||
| status: [ComplianceStatus.CLOSE_ONLY, ComplianceStatus.BLOCKED], | ||
| }, | ||
| [], | ||
| ); | ||
| const closeOnlyAddresses: string[] = _.chain(closeOnlyStatuses) | ||
| .map(ComplianceDataColumns.address) | ||
| .uniq() | ||
| .value(); | ||
|
|
There was a problem hiding this comment.
Is it possible to exclude these addresses earlier than here? There's already been a query made to the compliance provider by this filter and the results for these addresses will then be un-used by still charged for.
| }, | ||
| [], | ||
| ); | ||
| const closeOnlyAddresses: string[] = _.chain(closeOnlyAndBlockedStatuses) |
There was a problem hiding this comment.
nit: closeOnlyAndBlockedAddresses
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- indexer/packages/postgres/tests/stores/compliance-status-table.test.ts (1 hunks)
- indexer/packages/postgres/src/stores/compliance-status-table.ts (1 hunks)
- indexer/packages/postgres/src/types/query-types.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/update-compliance-data.test.ts (1 hunks)
- indexer/services/roundtable/src/tasks/perform-compliance-status-transitions.ts (1 hunks)
- indexer/services/roundtable/src/tasks/update-compliance-data.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/tasks/update-compliance-data.ts
Additional comments not posted (5)
indexer/services/roundtable/src/tasks/perform-compliance-status-transitions.ts (1)
24-24: Updated status query to handle arrays.The change to accept an array for the
statusfield instead of a single value is a significant improvement for flexibility in querying. This allows the function to handle multiple statuses at once, which is crucial for robust compliance management.indexer/packages/postgres/src/stores/compliance-status-table.ts (1)
62-62: Use ofwhereInfor status filtering.The modification to use
whereInfor thestatusfield supports the handling of multiple statuses in queries, enhancing the flexibility and robustness of the database operations. This is a positive change in line with the PR's objectives.indexer/packages/postgres/__tests__/stores/compliance-status-table.test.ts (1)
101-101: Updated test to reflect new array input for status.The test modification to handle an array for the
statusfield is necessary to ensure that the new functionality is correctly tested. This change ensures that the tests align with the updated querying capabilities.indexer/packages/postgres/src/types/query-types.ts (1)
286-286: Updated interface to handle array of statuses.The change to allow an array of strings for the
STATUSfield in theComplianceStatusQueryConfiginterface is a necessary update to support the new querying capabilities. This ensures that the type definitions are consistent with the new functionality.indexer/services/roundtable/__tests__/tasks/update-compliance-data.test.ts (1)
252-286: Test Case Review: Ensuring CLOSE_ONLY positions are not updatedThis test case is crucial as it directly addresses the core functionality introduced in this PR. It specifically tests that an existing CLOSE_ONLY compliance status is not overwritten even when new compliance data is available.
- Correctness: The test correctly sets up the scenario by creating a compliance status with
CLOSE_ONLYand then running theupdateComplianceDataTask. After the task, it checks that the status remains unchanged.- Completeness: The test appears to cover all necessary assertions to ensure the status is not updated.
- Readability: The test is well-structured and easy to understand, with clear setup, action, and assertion phases.
Overall, this test case effectively validates the new behavior introduced in the PR. It's a good practice to include such targeted tests to ensure critical functionality is not regressed by future changes.
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/update-compliance-data.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/tasks/update-compliance-data.ts
|
@Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
(cherry picked from commit a68a31e)
Changelist
Do not overwrite existing CLOSE_ONLY/BLOCKED compliance statuses
Test Plan
Unit tested
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
Improvements