[OTE-130] implement new geo-blocking strategy for socks #1049
Conversation
WalkthroughThe changes involve removing geoblocking features from the WebSocket service of the indexer. This includes the deletion of imports and code related to country restrictions, specifically the 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- indexer/services/socks/tests/websocket/index.test.ts (4 hunks)
- indexer/services/socks/src/helpers/header-utils.ts (1 hunks)
- indexer/services/socks/src/websocket/index.ts (4 hunks)
Additional comments: 4
indexer/services/socks/src/helpers/header-utils.ts (1)
- 5-8: The implementation of
getCountryfunction is correct and aligns with the PR's objective of handling country information without imposing restrictions. It properly uses TypeScript's type assertion to ensure the request headers include the expectedcf-ipcountryproperty. This approach is both efficient and maintainable.indexer/services/socks/__tests__/websocket/index.test.ts (1)
- 17-17: The introduction of
COUNTRY_HEADER_KEYfrom@dydxprotocol-indexer/compliancein the test file is appropriate and aligns with the PR's objectives of handling country information differently after the removal of geoblocking functionality. This change ensures that the tests accurately reflect the updated logic in the main codebase.indexer/services/socks/src/websocket/index.ts (2)
- 8-8: The import of
getCountryfromheader-utilsis correctly implemented and aligns with the PR's objective of handling country information without imposing restrictions. This change is necessary for the WebSocket service to adapt to the removal of geoblocking functionality.- 106-106: The usage of
getCountrywithin theonConnectionmethod to extract the country code from incoming requests is appropriate and aligns with the PR's objectives. This approach ensures that the WebSocket service can identify country information without enforcing access restrictions, which is consistent with the removal of geoblocking functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/socks/tests/lib/subscriptions.test.ts (8 hunks)
- indexer/services/socks/src/lib/subscription.ts (5 hunks)
Additional comments: 2
indexer/services/socks/src/lib/subscription.ts (2)
- 132-132: The removal of geoblocking logic, including the elimination of country-based checks, is consistent with the PR's objectives. However, ensure that the removal of this logic does not inadvertently affect other functionalities, especially error handling and rate limiting. It's also important to verify that the new approach to handling country information, as indicated by the introduction of
header-utils.tsin the PR summary, is adequately integrated and tested within the subscription logic.- 576-576: With the removal of geoblocking functionality, it's essential to ensure that the new method for handling country information (
getCountryfromheader-utils.ts) is properly integrated into the subscription process, especially for functionalities that might still require country information for non-restrictive purposes. This integration should be seamless and maintain the service's overall functionality and performance.
| import { axiosRequest } from '../../src/lib/axios'; | ||
| import { AxiosSafeServerError, makeAxiosSafeServerError } from '@dydxprotocol-indexer/base'; | ||
| import { BlockedError } from '../../src/lib/errors'; | ||
| import { isRestrictedCountry } from '@dydxprotocol-indexer/compliance'; | ||
|
|
||
| jest.mock('ws'); | ||
| jest.mock('../../src/helpers/wss'); |
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 [1-576]
The removal of geoblocking functionality, including references to restrictedCountry and nonRestrictedCountry, aligns with the PR's objectives. However, it's crucial to ensure that the removal of these tests does not reduce the coverage of other important functionalities within the Subscriptions class. Consider adding or enhancing tests to cover any new logic introduced or existing logic that may now behave differently without geoblocking.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/socks/tests/lib/subscriptions.test.ts (9 hunks)
- indexer/services/socks/src/lib/subscription.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/socks/tests/lib/subscriptions.test.ts
- indexer/services/socks/src/lib/subscription.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/socks/tests/lib/subscriptions.test.ts (14 hunks)
- indexer/services/socks/src/lib/subscription.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/socks/tests/lib/subscriptions.test.ts
- indexer/services/socks/src/lib/subscription.ts
dydxwill
changed the title
dydxwill
changed the title
dydxwill
changed the title
Changelist
remove geoblocking from socks endpoints:
add the country header from the connection request to the request sent to comlink to enable blocking subscriptions to v4_subaccount
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.