[OTE-132] new compliance logic for comlink address-related endpoints #1048
Conversation
WalkthroughThe updates focus on enhancing compliance checks within a financial application's communication layer, specifically targeting country restrictions based on user addresses. These changes involve integrating compliance-related entities, modifying functions to asynchronously retrieve and handle compliance statuses, and updating test cases to support these modifications. The modifications ensure that the application adheres to regulatory requirements by dynamically checking the user's location and making the necessary adjustments in both the implementation and testing environments. Changes
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/comlink/tests/lib/restrict-countries.test.ts (5 hunks)
- indexer/services/comlink/src/lib/restrict-countries.ts (2 hunks)
Additional comments: 2
indexer/services/comlink/__tests__/lib/restrict-countries.test.ts (2)
- 2-2: Ensure that
testConstantsis used in the test cases for consistency and maintainability.- 7-7: The import of
matchedDatais correctly added to support mocking in the test cases.
| @@ -2,17 +2,24 @@ import { | |||
| CountryHeaders, | |||
| isRestrictedCountryHeaders, | |||
| INDEXER_GEOBLOCKED_PAYLOAD, | |||
| INDEXER_COMPLIANCE_BLOCKED_PAYLOAD, | |||
There was a problem hiding this comment.
Consider removing unused imports if INDEXER_GEOBLOCKED_PAYLOAD is not utilized in this file.
| return next(); | ||
| } | ||
|
|
||
| const { | ||
| address, | ||
| }: { | ||
| address: string, | ||
| } = matchedData(req) as AddressRequest; | ||
| console.log('address', address); | ||
| const updatedStatus: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll( | ||
| { address: [address] }, | ||
| [], | ||
| { readReplica: true }, | ||
| ); | ||
| if (updatedStatus.length > 0) { | ||
| if (updatedStatus[0].status === ComplianceStatus.CLOSE_ONLY) { | ||
| return next(); | ||
| } else if (updatedStatus[0].status === ComplianceStatus.BLOCKED) { | ||
| return create4xxResponse( | ||
| res, | ||
| INDEXER_COMPLIANCE_BLOCKED_PAYLOAD, | ||
| 403, | ||
| { code: BlockedCode.COMPLIANCE_BLOCKED }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (isRestrictedCountryHeaders(req.headers as CountryHeaders)) { | ||
| return create4xxResponse( | ||
| res, |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [22-57]
- Logging with
console.logon line 39 should be removed or replaced with a more sophisticated logging mechanism that can be toggled for production environments. - Ensure that the
ComplianceStatusTable.findAllmethod on lines 40-44 properly handles errors, such as through a try-catch block, to avoid unhandled promise rejections. - The use of
readReplica: trueon line 43 suggests read-only operations which is good for performance but ensure that the read replica is up-to-date to avoid stale data issues.
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('does not reject requests from non-restricted countries', () => { | ||
| it('does not reject requests from non-restricted countries', async () => { | ||
| // non-restricted country in header | ||
| req.headers = nonRestrictedHeaders; | ||
| isRestrictedCountrySpy.mockReturnValueOnce(false); | ||
| matchedDataSpy.mockReturnValue({ address: testConstants.defaultAddress }); | ||
|
|
||
| rejectRestrictedCountries(req, res, next); | ||
| await rejectRestrictedCountries(req, res, next); | ||
| expect(res.status).not.toHaveBeenCalled(); | ||
| expect(next).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('rejects request from restricted countries with a 403', () => { | ||
| it('rejects request from restricted countries with a 403', async () => { | ||
| // restricted ipcountry | ||
| req.headers = restrictedHeaders; | ||
| isRestrictedCountrySpy.mockReturnValueOnce(true); | ||
| matchedDataSpy.mockReturnValue({ address: testConstants.defaultAddress }); | ||
|
|
||
| rejectRestrictedCountries(req, res, next); | ||
| await rejectRestrictedCountries(req, res, next); | ||
| expect(res.status).toHaveBeenCalledWith(403); | ||
| expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ | ||
| errors: expect.arrayContaining([ |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [24-95]
- The initialization of
matchedDataSpyand its usage in test cases is appropriate for mocking the return values ofmatchedData. - Using
awaitforrejectRestrictedCountriescalls correctly handles the asynchronous nature of the function. - Ensure that all possible paths and edge cases in
rejectRestrictedCountriesare covered by tests, including error handling scenarios and compliance status variations.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
indexer/services/comlink/package.jsonis excluded by:!**/*.json
Files selected for processing (12)
- indexer/services/comlink/tests/helpers/helpers.ts (2 hunks)
- indexer/services/comlink/tests/lib/compliance-and-geo-check.test.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (3 hunks)
- indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (3 hunks)
- indexer/services/comlink/src/controllers/api/v4/historical-block-trading-rewards-controller.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/historical-trading-reward-aggregations-controller.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/perpetual-positions-controller.ts (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (2 hunks)
- indexer/services/comlink/src/lib/compliance-and-geo-check.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/comlink/src/controllers/api/v4/historical-block-trading-rewards-controller.ts
Additional comments: 14
indexer/services/comlink/src/lib/compliance-and-geo-check.ts (3)
- 42-42: The destructuring assignment for
addressfrommatchedData(req)assumesaddresswill always be present in the request. Ensure upstream validation or checks guarantee this, otherwise, handle potential undefined cases.- 43-46: The use of
findAllwith{ address: [address] }as a filter is correct, but ensure that theaddressvariable is sanitized and validated before this point to prevent any potential SQL injection or query manipulation.- 61-61: The call to
isRestrictedCountryHeadersdirectly usesreq.headerswithout any explicit validation or sanitization of the headers. Ensure that the headers used byisRestrictedCountryHeadersare validated or sanitized upstream.indexer/services/comlink/__tests__/helpers/helpers.ts (1)
- 29-29: The addition of the
headersparameter with a default value of an empty object is a good practice for flexibility in specifying request headers in tests.indexer/services/comlink/src/controllers/api/v4/historical-trading-reward-aggregations-controller.ts (1)
- 18-23: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of
rejectRestrictedCountriesmiddleware from the route configuration is aligned with the PR's objective to enhance compliance logic. Ensure thatcomplianceAndGeoCheckmiddleware adequately covers the intended compliance checks.indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts (1)
- 20-20: Replacing
complianceCheckwithcomplianceAndGeoCheckmiddleware is consistent with the PR's objective. Verify thatcomplianceAndGeoCheckprovides the necessary compliance validations for the historical PnL endpoint.indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (1)
- 26-26: Replacing
complianceCheckwithcomplianceAndGeoCheckmiddleware aligns with the PR's objective. Ensure thatcomplianceAndGeoCheckadequately covers compliance checks for the transfers endpoint.indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (1)
- 24-24: Replacing
complianceCheckwithcomplianceAndGeoCheckmiddleware is consistent with the PR's objective. Confirm thatcomplianceAndGeoCheckprovides the necessary compliance validations for the fills endpoint.indexer/services/comlink/src/controllers/api/v4/asset-positions-controller.ts (1)
- 28-28: Replacing
complianceCheckwithcomplianceAndGeoCheckmiddleware aligns with the PR's objective. Verify thatcomplianceAndGeoCheckadequately covers compliance checks for the asset positions endpoint.indexer/services/comlink/src/controllers/api/v4/perpetual-positions-controller.ts (1)
- 32-32: Replacing
complianceCheckwithcomplianceAndGeoCheckmiddleware is consistent with the PR's objective. Ensure thatcomplianceAndGeoCheckprovides the necessary compliance validations for the perpetual positions endpoint.indexer/services/comlink/__tests__/lib/compliance-and-geo-check.test.ts (1)
- 1-1: The test suite for
complianceAndGeoCheckmiddleware is comprehensive, covering various scenarios. Ensure that all new logic paths introduced bycomplianceAndGeoCheckare adequately tested, including edge cases.indexer/services/comlink/src/controllers/api/v4/addresses-controller.ts (1)
- 40-40: Replacing
complianceCheckwithcomplianceAndGeoCheckmiddleware in theAddressesControllerclass for address and subaccount validation routes aligns with the PR's objective. Confirm thatcomplianceAndGeoCheckadequately covers compliance checks for these routes.indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (2)
- 31-31: Ensure
complianceAndGeoCheckis correctly implemented and tested, given its critical role in compliance and geographical checks.- 225-225: Confirm that
complianceAndGeoCheckmiddleware's integration does not introduce any unintended side effects, especially in terms of performance or error handling.
| @@ -224,7 +222,7 @@ router.get( | |||
| query('goodTilBlock').if(query('goodTilBlockTime').exists()).isEmpty() | |||
| .withMessage('Cannot provide both goodTilBlock and goodTilBlockTime'), | |||
| handleValidationErrors, | |||
| complianceCheck, | |||
| complianceAndGeoCheck, | |||
There was a problem hiding this comment.
Recommend adding extensive tests for complianceAndGeoCheck to cover various compliance scenarios, including edge cases and error handling.
Would you like assistance in generating these tests?
| if (Object.keys(headers).length) { | ||
| await req.set(headers); | ||
| } |
There was a problem hiding this comment.
Using await with req.set(headers) is unnecessary since set does not return a promise. Remove await to avoid confusion.
- await req.set(headers);
+ req.set(headers);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.
| if (Object.keys(headers).length) { | |
| await req.set(headers); | |
| } | |
| if (Object.keys(headers).length) { | |
| req.set(headers); | |
| } |
There was a problem hiding this comment.
nit: this seems legit
There was a problem hiding this comment.
@Christopher-Li: Great, I'm glad you find the suggestion helpful! If you haven't already, please go ahead and apply the change. If there's anything else you need, feel free to ask.
There was a problem hiding this comment.
it returns a Promise.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
dydxwill
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/comlink/tests/lib/compliance-and-geo-check.test.ts (1 hunks)
- indexer/services/comlink/src/lib/compliance-and-geo-check.ts (1 hunks)
Additional comments: 3
indexer/services/comlink/__tests__/lib/compliance-and-geo-check.test.ts (3)
- 24-25: Mocking of
@dydxprotocol-indexer/complianceis done correctly. Ensure that all necessary functions from this package are mocked if they are used in the tests.- 70-90: The setup and teardown methods are correctly implemented using
beforeAll,beforeEach,afterAll, andafterEachhooks. Ensure thatdbHelpers.migrate()anddbHelpers.teardown()are correctly implemented to handle database schema migrations and cleanup.Verification successful
Given the lack of output from the initial scripts, it's necessary to broaden the search to ensure we're not missing any relevant information. Let's attempt a more general search for the usage or definition of
dbHelpers.migrateanddbHelpers.teardownwithout specifying method parameters.
The search results confirm the widespread use of
dbHelpers.migrate()anddbHelpers.teardown()across various test files, indicating their correct implementation for handling database schema migrations and cleanup as part of the testing setup and teardown process. This aligns with the review comment's verification requirement, suggesting that the setup and teardown methods are correctly implemented.* 92-238: The test cases are well-structured and cover various scenarios, including no address in request, address not in database, address not blocked, and address blocked. However, ensure that the mock return values and expected statuses are correctly set up for each test case to accurately reflect the behavior of the `complianceAndGeoCheck` middleware.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of dbHelpers.migrate() and dbHelpers.teardown() ast-grep --lang javascript --pattern $'dbHelpers.migrate($_)' ast-grep --lang javascript --pattern $'dbHelpers.teardown($_)'Length of output: 126
Script:
#!/bin/bash # Broaden the search for dbHelpers.migrate and dbHelpers.teardown rg "dbHelpers\.migrate" rg "dbHelpers\.teardown"Length of output: 14306
| import { | ||
| CountryHeaders, | ||
| isRestrictedCountryHeaders, | ||
| INDEXER_GEOBLOCKED_PAYLOAD, | ||
| INDEXER_COMPLIANCE_BLOCKED_PAYLOAD, | ||
| } from '@dydxprotocol-indexer/compliance'; | ||
| import { | ||
| ComplianceStatus, | ||
| ComplianceStatusFromDatabase, | ||
| ComplianceStatusTable, | ||
| } from '@dydxprotocol-indexer/postgres'; | ||
| import express from 'express'; | ||
| import { matchedData } from 'express-validator'; | ||
|
|
||
| import { AddressRequest, BlockedCode } from '../types'; | ||
| import { create4xxResponse } from './helpers'; | ||
| import { getIpAddr, isIndexerIp } from './utils'; | ||
|
|
There was a problem hiding this comment.
Consider grouping imports from the same package together to improve readability.
- import {
- CountryHeaders,
- isRestrictedCountryHeaders,
- INDEXER_GEOBLOCKED_PAYLOAD,
- INDEXER_COMPLIANCE_BLOCKED_PAYLOAD,
- } from '@dydxprotocol-indexer/compliance';
- import {
- ComplianceStatus,
- ComplianceStatusFromDatabase,
- ComplianceStatusTable,
- } from '@dydxprotocol-indexer/postgres';
+ import { CountryHeaders, isRestrictedCountryHeaders, INDEXER_GEOBLOCKED_PAYLOAD, INDEXER_COMPLIANCE_BLOCKED_PAYLOAD } from '@dydxprotocol-indexer/compliance';
+ import { ComplianceStatus, ComplianceStatusFromDatabase, ComplianceStatusTable } from '@dydxprotocol-indexer/postgres';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.
| import { | |
| CountryHeaders, | |
| isRestrictedCountryHeaders, | |
| INDEXER_GEOBLOCKED_PAYLOAD, | |
| INDEXER_COMPLIANCE_BLOCKED_PAYLOAD, | |
| } from '@dydxprotocol-indexer/compliance'; | |
| import { | |
| ComplianceStatus, | |
| ComplianceStatusFromDatabase, | |
| ComplianceStatusTable, | |
| } from '@dydxprotocol-indexer/postgres'; | |
| import express from 'express'; | |
| import { matchedData } from 'express-validator'; | |
| import { AddressRequest, BlockedCode } from '../types'; | |
| import { create4xxResponse } from './helpers'; | |
| import { getIpAddr, isIndexerIp } from './utils'; | |
| import { CountryHeaders, isRestrictedCountryHeaders, INDEXER_GEOBLOCKED_PAYLOAD, INDEXER_COMPLIANCE_BLOCKED_PAYLOAD } from '@dydxprotocol-indexer/compliance'; | |
| import { ComplianceStatus, ComplianceStatusFromDatabase, ComplianceStatusTable } from '@dydxprotocol-indexer/postgres'; | |
| import express from 'express'; | |
| import { matchedData } from 'express-validator'; | |
| import { AddressRequest, BlockedCode } from '../types'; | |
| import { create4xxResponse } from './helpers'; | |
| import { getIpAddr, isIndexerIp } from './utils'; | |
| const { address }: AddressRequest = matchedData(req) as AddressRequest; | ||
| if (address !== undefined) { | ||
| const updatedStatus: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll( | ||
| { address: [address] }, | ||
| [], | ||
| { readReplica: true }, | ||
| ); |
There was a problem hiding this comment.
Ensure proper error handling for the asynchronous call to ComplianceStatusTable.findAll. Consider wrapping it in a try-catch block to handle potential database connection issues or query failures.
+ try {
const updatedStatus: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll(
{ address: [address] },
[],
{ readReplica: true },
);
+ } catch (error) {
+ return create4xxResponse(res, 'Error fetching compliance status', 500);
+ }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.
| const { address }: AddressRequest = matchedData(req) as AddressRequest; | |
| if (address !== undefined) { | |
| const updatedStatus: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll( | |
| { address: [address] }, | |
| [], | |
| { readReplica: true }, | |
| ); | |
| const { address }: AddressRequest = matchedData(req) as AddressRequest; | |
| if (address !== undefined) { | |
| try { | |
| const updatedStatus: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll( | |
| { address: [address] }, | |
| [], | |
| { readReplica: true }, | |
| ); | |
| } catch (error) { | |
| return create4xxResponse(res, 'Error fetching compliance status', 500); | |
| } |
| if (isRestrictedCountryHeaders(req.headers as CountryHeaders)) { | ||
| return create4xxResponse( | ||
| res, | ||
| INDEXER_GEOBLOCKED_PAYLOAD, | ||
| 403, | ||
| { code: BlockedCode.GEOBLOCKED }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The use of isRestrictedCountryHeaders directly without validation of headers could lead to unexpected behavior if headers are malformed or missing. Consider adding validation for the expected headers before this check.
+ if (!req.headers || typeof req.headers['cf-ipcountry'] !== 'string') {
+ return create4xxResponse(res, 'Missing or invalid country header', 400);
+ }
if (isRestrictedCountryHeaders(req.headers as CountryHeaders)) {
return create4xxResponse(
res,
INDEXER_GEOBLOCKED_PAYLOAD,
403,
{ code: BlockedCode.GEOBLOCKED },
);
}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.
| if (isRestrictedCountryHeaders(req.headers as CountryHeaders)) { | |
| return create4xxResponse( | |
| res, | |
| INDEXER_GEOBLOCKED_PAYLOAD, | |
| 403, | |
| { code: BlockedCode.GEOBLOCKED }, | |
| ); | |
| } | |
| if (!req.headers || typeof req.headers['cf-ipcountry'] !== 'string') { | |
| return create4xxResponse(res, 'Missing or invalid country header', 400); | |
| } | |
| if (isRestrictedCountryHeaders(req.headers as CountryHeaders)) { | |
| return create4xxResponse( | |
| res, | |
| INDEXER_GEOBLOCKED_PAYLOAD, | |
| 403, | |
| { code: BlockedCode.GEOBLOCKED }, | |
| ); | |
| } |
Changelist
new compliance logic for comlink address-related endpoints
IF the address is in the compliance_status table and has the status CLOSE_ONLY, return data for the endpoint ELSE
IF the address has compliance_status of BLOCKED block access to the endpoint (return 403) ELSE
IF the origin country is restricted geography, block access to the endpoint (return 403) ELSE
return data for the endpoint
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.