feat(api): don't strike users from /geoblock*, raise 403#3158
feat(api): don't strike users from /geoblock*, raise 403#3158UnbornAztecKing merged 1 commit intomainfrom
Conversation
WalkthroughAdds controller-level restricted-country geoblocking: POSTs with restricted-country headers now return 403 with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller
participant GeoUtils
participant DB
Note over Controller: Geo-aware compliance flow (new behavior)
Client->>Controller: POST /api/v4/compliance/... (headers, body)
Controller->>GeoUtils: isRestrictedCountryHeaders(headers)
alt Restricted country
Controller-->>Client: 403 { INDEXER_GEOBLOCKED_PAYLOAD, code: BlockedCode.GEOBLOCKED }
Note right of Controller: No DB mutation, no metric emission
else Not restricted
Controller->>DB: read existing compliance_status
alt No existing status
Controller->>DB: upsert COMPLIANT
else Existing status
Controller->>DB: upsert per simplified rules
end
Controller-->>Client: 200 ComplianceV2Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e6bebfe to
3035327
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (1)
439-461: CRITICAL: INVALID_SURVEY from non-restricted origin incorrectly transitions to COMPLIANT.The
actionparameter is declared but never used in the non-restricted code path (lines 452-458). This causesINVALID_SURVEYfrom non-restricted origins to incorrectly transition toCOMPLIANTinstead ofCLOSE_ONLY, violating the documented behavior in the PR objectives: "INVALID_SURVEY or a restricted origin transitions to CLOSE_ONLY".Apply this diff to restore action-aware transitions:
if ( complianceStatus[0].status === ComplianceStatus.FIRST_STRIKE || complianceStatus[0].status === ComplianceStatus.FIRST_STRIKE_CLOSE_ONLY || complianceStatus[0].status === ComplianceStatus.BLOCKED || complianceStatus[0].status === ComplianceStatus.COMPLIANT ) { if (restricted) { return ComplianceStatusTable.update({ address, status: ComplianceStatus.CLOSE_ONLY, reason: getGeoComplianceReason(req.headers as GeoOriginHeaders)!, updatedAt, }); - } else { + } + + if (action === ComplianceAction.INVALID_SURVEY) { + return ComplianceStatusTable.update({ + address, + status: ComplianceStatus.CLOSE_ONLY, + updatedAt, + }); + } + - return ComplianceStatusTable.update({ - address, - status: ComplianceStatus.COMPLIANT, - updatedAt, - }); - } + return ComplianceStatusTable.update({ + address, + status: ComplianceStatus.COMPLIANT, + updatedAt, + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between acc045c1f3e7c47b28f53cbc7539701d2332bd1b and a312a10.
📒 Files selected for processing (2)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts(9 hunks)indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (5)
indexer/packages/compliance/src/geoblocking/restrict-countries.ts (1)
isRestrictedCountryHeaders(6-31)indexer/packages/compliance/src/types.ts (1)
GeoOriginHeaders(8-12)indexer/services/comlink/src/lib/helpers.ts (1)
create4xxResponse(159-171)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/services/comlink/src/helpers/compliance/compliance-utils.ts (1)
getGeoComplianceReason(30-46)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (3)
indexer/packages/postgres/src/types/db-model-types.ts (1)
ComplianceStatusFromDatabase(248-254)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/services/comlink/__tests__/helpers/helpers.ts (1)
sendRequest(82-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: test / run_command
- GitHub Check: check-build-bazooka
- GitHub Check: run_command
- GitHub Check: build-and-push-mainnet
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (2)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (1)
196-203: LGTM: Early 403 responses prevent DB writes for restricted origins.The implementation correctly returns 403 with
INDEXER_GEOBLOCKED_PAYLOADbefore any validation or database operations occur. This applies to both/geoblockand/geoblock-keplrendpoints and aligns with the PR objective to prevent DB writes and metric emissions for restricted countries.Also applies to: 249-256
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (1)
471-719: LGTM: Comprehensive test coverage for restricted-country scenarios.The tests thoroughly verify that restricted-country requests receive 403 responses with the geoblocked payload, with no database writes or status changes occurring. Coverage includes whitelisted addresses, various existing compliance statuses, and all action types (CONNECT, INVALID_SURVEY, VALID_SURVEY).
| it.each([ | ||
| ComplianceStatus.BLOCKED, | ||
| ComplianceStatus.FIRST_STRIKE, | ||
| ComplianceStatus.FIRST_STRIKE_CLOSE_ONLY, | ||
| ])('should set status to COMPLIANT for any action from a non-restricted country with existing compliance status not CLOSE_ONLY', async (status: ComplianceStatus) => { | ||
| isRestrictedCountryHeadersSpy.mockReturnValue(false); | ||
|
|
||
| await ComplianceStatusTable.create({ | ||
| address: testConstants.defaultAddress, | ||
| status, | ||
| }); | ||
|
|
||
| const response: any = await sendRequest({ | ||
| type: RequestMethod.POST, | ||
| path: endpoint, | ||
| body, | ||
| expectedStatus: 200, | ||
| }); | ||
| expect(stats.increment).toHaveBeenCalledWith( | ||
| `${config.SERVICE_NAME}.compliance-v2-controller.${endpoint === geoblockEndpoint ? 'geo_block' : 'geo_block_keplr'}.compliance_status_changed.count`, | ||
| { | ||
| newStatus: ComplianceStatus.FIRST_STRIKE, | ||
| }); | ||
|
|
||
| expect(response.body.updatedAt).toBeDefined(); | ||
| expect(response.body.status).toEqual(ComplianceStatus.COMPLIANT); | ||
|
|
||
| const data: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll({}, [], {}); | ||
|
|
||
| expect(data).toHaveLength(1); | ||
| expect(data[0]).toEqual(expect.objectContaining({ | ||
| address: testConstants.defaultAddress, | ||
| status: ComplianceStatus.FIRST_STRIKE, | ||
| reason: ComplianceReason.US_GEO, | ||
| status: ComplianceStatus.COMPLIANT, | ||
| })); | ||
| }); | ||
|
|
||
| expect(response.body.status).toEqual(ComplianceStatus.FIRST_STRIKE); | ||
| expect(response.body.reason).toEqual(ComplianceReason.US_GEO); | ||
| expect(response.body.updatedAt).toBeDefined(); | ||
| it('should set status to COMPLIANT for any action from a non-restricted country with no existing compliance status', async () => { | ||
| isRestrictedCountryHeadersSpy.mockReturnValue(false); | ||
|
|
||
| const response: any = await sendRequest({ | ||
| type: RequestMethod.POST, | ||
| path: endpoint, | ||
| body, | ||
| expectedStatus: 200, | ||
| }); | ||
|
|
||
| const data: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll({}, [], {}); | ||
| expect(data).toHaveLength(1); | ||
| expect(data[0]).toEqual(expect.objectContaining({ | ||
| address: testConstants.defaultAddress, | ||
| status: ComplianceStatus.COMPLIANT, | ||
| })); | ||
|
|
||
| expect(response.body.status).toEqual(ComplianceStatus.COMPLIANT); | ||
| }); |
There was a problem hiding this comment.
Missing test: INVALID_SURVEY from non-restricted origin should transition to CLOSE_ONLY.
The test suite lacks coverage for INVALID_SURVEY action from a non-restricted country. Per the PR objectives, this should transition to CLOSE_ONLY, but the current controller implementation (due to the unused action parameter bug) would incorrectly set it to COMPLIANT. Adding this test would have caught the critical issue.
Do you want me to generate a test case that verifies INVALID_SURVEY from non-restricted origins correctly transitions to CLOSE_ONLY?
🤖 Prompt for AI Agents
In
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
around lines 721 to 770, add a test that posts an INVALID_SURVEY action from a
non-restricted country and asserts the controller persists status CLOSE_ONLY
(both in response.body.status and in the ComplianceStatusTable row), to cover
the missing case where a non-restricted INVALID_SURVEY should transition to
CLOSE_ONLY rather than COMPLIANT; set
isRestrictedCountryHeadersSpy.mockReturnValue(false), send the POST with
body.action = 'INVALID_SURVEY', expect HTTP 200, expect response.body.status ===
ComplianceStatus.CLOSE_ONLY and that ComplianceStatusTable.findAll returns one
entry with address testConstants.defaultAddress and status
ComplianceStatus.CLOSE_ONLY.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (1)
428-446: Critical: INVALID_SURVEY from non-restricted origin incorrectly transitions to COMPLIANT.The
actionparameter is unused. Lines 440–445 unconditionally upgrade non-restricted requests toCOMPLIANT, meaningINVALID_SURVEYfrom a non-restricted country will (incorrectly) becomeCOMPLIANTinstead ofCLOSE_ONLY. This violates the PR objectives ("INVALID_SURVEY … transitions to CLOSE_ONLY") and the function's own documentation (lines 383–384). This is the same issue flagged in the previous review.Apply this diff to respect the
actionparameter:if (restricted) { return ComplianceStatusTable.update({ address, status: ComplianceStatus.CLOSE_ONLY, reason: getGeoComplianceReason(req.headers as GeoOriginHeaders)!, updatedAt, }); - } else { + } + + if (action === ComplianceAction.INVALID_SURVEY) { + return ComplianceStatusTable.update({ + address, + status: ComplianceStatus.CLOSE_ONLY, + updatedAt, + }); + } + return ComplianceStatusTable.update({ address, status: ComplianceStatus.COMPLIANT, updatedAt, }); - }indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (1)
636-714: Missing test: INVALID_SURVEY from non-restricted origin should transition to CLOSE_ONLY.The test suite covers
INVALID_SURVEYandVALID_SURVEYfrom restricted countries (both correctly expect 403), but lacks a test forINVALID_SURVEYfrom a non-restricted country. Per the PR objectives, this should transition toCLOSE_ONLY, but the current controller implementation would incorrectly set it toCOMPLIANT(due to the unusedactionparameter). Adding this test would catch the critical issue.Do you want me to generate a test case that verifies
INVALID_SURVEYfrom non-restricted origins correctly transitions toCLOSE_ONLY?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 98a0cd2 and 35374b7961abba86d5288d4c8b5ebf7a8f2472a3.
📒 Files selected for processing (3)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts(3 hunks)indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts(7 hunks)indexer/services/comlink/src/lib/compliance-and-geo-check.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T22:21:49.129Z
Learnt from: Kefancao
Repo: dydxprotocol/v4-chain PR: 3162
File: indexer/services/comlink/src/lib/apple-helpers.ts:151-164
Timestamp: 2025-10-16T22:21:49.129Z
Learning: In the dydxprotocol/v4-chain codebase, when implementing Apple Sign-In for Turnkey authentication in `indexer/services/comlink/src/lib/apple-helpers.ts`, the ID token obtained from Apple's token endpoint is passed to Turnkey for validation. The email extraction from the ID token using `decodeJwt` (without signature verification) is acceptable because Turnkey handles the actual token validation in the `socialSignin` flow, and the email extraction is just for convenience.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
🧬 Code graph analysis (1)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (5)
indexer/packages/postgres/src/types/db-model-types.ts (1)
ComplianceStatusFromDatabase(252-258)indexer/services/comlink/__tests__/helpers/helpers.ts (1)
sendRequest(82-106)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
stats(377-385)indexer/packages/redis/__tests__/caches/constants.ts (1)
createdAt(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: check-build-auxo
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: test / run_command
- GitHub Check: build-and-push-mainnet
- GitHub Check: lint
- GitHub Check: build-and-push-testnet
- GitHub Check: run_command
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (3)
indexer/services/comlink/src/lib/compliance-and-geo-check.ts (1)
28-30: LGTM! Documentation accurately reflects control flow.The comment reordering clarifies that BLOCKED status is checked before geo-restriction checks, matching the actual code flow below.
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (2)
492-522: LGTM! Test correctly verifies no DB mutations on geoblock.The test asserts that restricted-origin requests short-circuit with 403 and leave the database untouched, with no
compliance_status_changedmetrics emitted. This aligns with the PR objectives.
716-744: LGTM! Test validates upgrade paths from strike statuses.The parameterized test correctly verifies that
FIRST_STRIKEandFIRST_STRIKE_CLOSE_ONLYupgrade toCOMPLIANTfrom non-restricted origins, consistent with the normalization behavior described in the PR objectives.
| if (isRestrictedCountryHeaders(req.headers as GeoOriginHeaders)) { | ||
| return create4xxResponse( | ||
| res, | ||
| INDEXER_GEOBLOCKED_PAYLOAD, | ||
| 403, | ||
| { code: BlockedCode.GEOBLOCKED }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full function to understand the control flow
cd indexer/services/comlink/src/controllers/api/v4
wc -l compliance-v2-controller.tsRepository: dydxprotocol/v4-chain
Length of output: 96
🏁 Script executed:
# Search for the function containing lines 287-294 and understand its structure
# Let's read a broader context around the mentioned lines
sed -n '280,450p' indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts | cat -nRepository: dydxprotocol/v4-chain
Length of output: 6336
🏁 Script executed:
# Search for all calls to upsertComplianceStatus to verify if early return blocks all paths
rg -nP '\bupsertComplianceStatus\s*\(' indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts -C3Repository: dydxprotocol/v4-chain
Length of output: 533
Remove dead code or restructure to handle restricted-origin action transitions.
The early 403 return (lines 287–294) prevents restricted requests from ever reaching upsertComplianceStatus, making the if (restricted) branch at lines 154–160 unreachable. Additionally, the action parameter is never used within upsertComplianceStatus, preventing state transitions like INVALID_SURVEY → CLOSE_ONLY.
If the intent is to immediately block all geo-restricted users with 403, remove the dead if (restricted) branch and its getGeoComplianceReason calls (lines 154–160) for clarity. Otherwise, restructure the early return logic to allow action-based transitions for restricted origins before returning 403 on eligible statuses.
🤖 Prompt for AI Agents
In indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
around lines 287–294, the early 403 return for geo-restricted headers makes the
`if (restricted)` branch at lines 154–160 unreachable and the `action` parameter
never drives state transitions; either (A) remove the unreachable `if
(restricted)` branch and its calls to getGeoComplianceReason (lines ~154–160) so
the 403-only behavior is explicit, or (B) move the geo-restriction check so that
upsertComplianceStatus is called first (passing the `action`) to allow
action-driven transitions for restricted origins, and only after upsert return
403 for statuses that should be blocked; also ensure upsertComplianceStatus
actually consumes the `action` parameter to perform transitions (implement
action handling if missing) and remove any now-unused helper calls accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (1)
720-795: Missing test: INVALID_SURVEY from non-restricted origin should transition to CLOSE_ONLY.Per PR objectives: "Survey behavior:
VALID_SURVEYfrom a non-restricted origin transitions toCOMPLIANT;INVALID_SURVEYor a restricted origin transitions toCLOSE_ONLY."The test suite lacks coverage for INVALID_SURVEY action from a non-restricted country.
🧹 Nitpick comments (3)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (3)
720-748: Misleading test title: claims "any action" but only tests CONNECT.The test description says "any action from a non-restricted country" but the test body uses the default
bodywhich hasaction: ComplianceAction.CONNECT. Consider either:
- Renaming to specify CONNECT action
- Parameterizing the test to cover multiple actions (CONNECT, VALID_SURVEY)
- it.each([ - ComplianceStatus.FIRST_STRIKE, - ComplianceStatus.FIRST_STRIKE_CLOSE_ONLY, - ])('should set status to COMPLIANT for any action from a non-restricted country with existing compliance status FIRST_STRIKE or FIRST_STRIKE_CLOSE_ONLY', async (status: ComplianceStatus) => { + it.each([ + ComplianceStatus.FIRST_STRIKE, + ComplianceStatus.FIRST_STRIKE_CLOSE_ONLY, + ])('should set status to COMPLIANT for CONNECT action from a non-restricted country with existing %s status', async (status: ComplianceStatus) => {
750-775: Misleading test title: claims "any action" but only tests CONNECT.Same issue as the previous test - the title claims "any action" but uses the default body with CONNECT action.
- it('should leave status at BLOCKED for any action from a non-restricted country with existing compliance status BLOCKED', async () => { + it('should leave status at BLOCKED for CONNECT action from a non-restricted country with existing BLOCKED status', async () => {
777-795: Missing test: CLOSE_ONLY from non-restricted country should remain unchanged.Per PR objectives: "Non-restricted origins: ...
BLOCKEDandCLOSE_ONLYremain unchanged."The test suite covers BLOCKED remaining unchanged (lines 750-775) but lacks coverage for CLOSE_ONLY from a non-restricted origin. Consider adding a test to verify CLOSE_ONLY status is not upgraded to COMPLIANT.
it('should leave status at CLOSE_ONLY for CONNECT action from a non-restricted country with existing CLOSE_ONLY status', async () => { isRestrictedCountryHeadersSpy.mockReturnValue(false); await ComplianceStatusTable.create({ address: testConstants.defaultAddress, status: ComplianceStatus.CLOSE_ONLY, }); const response: any = await sendRequest({ type: RequestMethod.POST, path: endpoint, body, expectedStatus: 200, }); expect(response.body.status).toEqual(ComplianceStatus.CLOSE_ONLY); const data: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll({}, [], {}); expect(data).toHaveLength(1); expect(data[0]).toEqual(expect.objectContaining({ address: testConstants.defaultAddress, status: ComplianceStatus.CLOSE_ONLY, })); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 35374b7961abba86d5288d4c8b5ebf7a8f2472a3 and 4ce4692e4eb4e3404d9705651c815024cdefd643.
📒 Files selected for processing (1)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T22:21:49.129Z
Learnt from: Kefancao
Repo: dydxprotocol/v4-chain PR: 3162
File: indexer/services/comlink/src/lib/apple-helpers.ts:151-164
Timestamp: 2025-10-16T22:21:49.129Z
Learning: In the dydxprotocol/v4-chain codebase, when implementing Apple Sign-In for Turnkey authentication in `indexer/services/comlink/src/lib/apple-helpers.ts`, the ID token obtained from Apple's token endpoint is passed to Turnkey for validation. The email extraction from the ID token using `decodeJwt` (without signature verification) is acceptable because Turnkey handles the actual token validation in the `socialSignin` flow, and the email extraction is just for convenience.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
🧬 Code graph analysis (1)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (4)
indexer/packages/postgres/src/types/db-model-types.ts (1)
ComplianceStatusFromDatabase(252-258)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
stats(377-385)indexer/packages/redis/__tests__/caches/constants.ts (1)
createdAt(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: test / run_command
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (5)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (5)
12-12: LGTM!The import additions are correct and necessary for the new geoblock-related assertions.
Also applies to: 22-26
487-494: LGTM!The test correctly verifies that whitelisted addresses bypass geoblocking: no DB mutations and a COMPLIANT response.
496-526: LGTM!Thorough verification of the geoblock response: 403 status, correct error payload with
INDEXER_GEOBLOCKED_PAYLOADandBlockedCode.GEOBLOCKED, no DB mutations, and no stats increments.
528-638: LGTM!Good coverage of existing compliance statuses (COMPLIANT, FIRST_STRIKE, CLOSE_ONLY) under restricted country scenarios. Each test correctly verifies immutability of DB state and proper error response structure.
640-718: LGTM!Survey actions (INVALID_SURVEY, VALID_SURVEY) from restricted countries correctly return 403 with no DB changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (1)
393-411: Unused parameters and missing implementation per docstring.The
actionandwalletparameters are never used. The docstring (Lines 360-391) describes status transitions that aren't implemented:
- FIRST_STRIKE/FIRST_STRIKE_CLOSE_ONLY from non-restricted → COMPLIANT (not implemented)
- VALID_SURVEY → COMPLIANT (not implemented)
- INVALID_SURVEY → CLOSE_ONLY (not implemented)
The current implementation only upserts COMPLIANT for new addresses and returns existing status unchanged otherwise.
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (1)
720-796: Missing test: INVALID_SURVEY from non-restricted origin should transition to CLOSE_ONLY.Per the PR objectives,
INVALID_SURVEYfrom a non-restricted origin should transition toCLOSE_ONLY. However, there is no test covering this scenario. This would help ensure theactionparameter is properly utilized in the controller logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4ce4692e4eb4e3404d9705651c815024cdefd643 and 295232c81b1e66212b20736130fb273860f70fe8.
📒 Files selected for processing (2)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts(3 hunks)indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T22:21:49.129Z
Learnt from: Kefancao
Repo: dydxprotocol/v4-chain PR: 3162
File: indexer/services/comlink/src/lib/apple-helpers.ts:151-164
Timestamp: 2025-10-16T22:21:49.129Z
Learning: In the dydxprotocol/v4-chain codebase, when implementing Apple Sign-In for Turnkey authentication in `indexer/services/comlink/src/lib/apple-helpers.ts`, the ID token obtained from Apple's token endpoint is passed to Turnkey for validation. The email extraction from the ID token using `decodeJwt` (without signature verification) is acceptable because Turnkey handles the actual token validation in the `socialSignin` flow, and the email extraction is just for convenience.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
📚 Learning: 2024-11-22T18:12:04.606Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
Applied to files:
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
🧬 Code graph analysis (2)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (5)
indexer/packages/postgres/src/types/db-model-types.ts (1)
ComplianceStatusFromDatabase(252-258)indexer/services/comlink/__tests__/helpers/helpers.ts (1)
sendRequest(82-106)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
stats(377-385)indexer/packages/redis/__tests__/caches/constants.ts (1)
createdAt(32-32)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (4)
indexer/packages/compliance/src/geoblocking/restrict-countries.ts (1)
isRestrictedCountryHeaders(6-31)indexer/packages/compliance/src/types.ts (1)
GeoOriginHeaders(8-12)indexer/services/comlink/src/lib/helpers.ts (1)
create4xxResponse(157-169)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: test / run_command
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: check-build-bazooka
- GitHub Check: build-and-push-testnet
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (10)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (7)
12-12: LGTM!Import additions for
BlockedCode,RequestMethod,INDEXER_GEOBLOCKED_PAYLOAD,isRestrictedCountryHeaders, andisWhitelistedAddressare correctly aligned with the new geoblock testing requirements.Also applies to: 22-26
487-494: LGTM!Correctly verifies that whitelisted addresses from restricted countries receive
COMPLIANTresponse without any database mutations.
496-526: LGTM!Comprehensive test that verifies the 403 geoblock response, error payload structure, absence of DB mutations, and no stats increments for status changes.
528-638: LGTM!These tests correctly verify that restricted-country requests return 403 with the geoblock payload regardless of existing compliance status (COMPLIANT, FIRST_STRIKE, CLOSE_ONLY), and that no database mutations occur.
640-718: LGTM!Good coverage for
INVALID_SURVEYandVALID_SURVEYactions from restricted countries. Both correctly verify the 403 geoblock response and absence of database mutations.
751-776: LGTM!Correctly verifies that
BLOCKEDstatus remains unchanged for non-restricted country requests, aligning with PR objectives.
778-796: LGTM!Correctly verifies that new addresses from non-restricted origins are upserted as
COMPLIANT.indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3)
10-10: LGTM!Import additions for
INDEXER_GEOBLOCKED_PAYLOADandBlockedCodeare correctly added to support the 403 geoblock response.Also applies to: 42-46
287-294: LGTM!The early exit for restricted-country requests correctly returns a 403 with the geoblocked payload before any database operations, implementing the core PR requirement.
343-358: LGTM!The
handleErrorfunction signature is appropriately typed.
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (3)
496-526: 403 + no-mutation path for restricted CONNECT with no status is well coveredThe test fully captures the new contract for restricted CONNECT with no existing status: 403, standard geoblock error payload, no DB writes, and no
compliance_status_changedmetric emission. This directly enforces the PR’s compliance behavior.If you have a shared constant for the
compliance_status_changedmetric name, consider using it here instead ofstringContaining(...)to make the assertion more robust to future renames.
528-718: Restricted/geoblock*coverage across statuses and actions looks solidThe 403 tests for restricted requests with existing
COMPLIANT,FIRST_STRIKE,CLOSE_ONLY, andFIRST_STRIKE_CLOSE_ONLYstatuses, plusCONNECT,INVALID_SURVEY, andVALID_SURVEYactions, do a good job of asserting:
- No
ComplianceStatusmutations (length, status, andupdatedAtunchanged).- Standardized error payload:
errors[0].msg === INDEXER_GEOBLOCKED_PAYLOAD,errors[0].code === BlockedCode.GEOBLOCKED.- Behavior is identical for both
/geoblockand/geoblock-keplrviadescribe.each.This matches the PR objective of short‑circuiting restricted requests with 403 and eliminating survey-driven progression.
If
ComplianceAction.COMPLIANCE_PROGRESSIONis still a valid action for this route, you might optionally add a similar 403/no-mutation test for it to fully lock down all action variants.
720-795: Non-restricted upsert behavior matches simplified semantics; consider aligning “any action” wording with coverageThese three tests collectively pin down the new non‑restricted behavior:
- Existing statuses
FIRST_STRIKE,FIRST_STRIKE_CLOSE_ONLY, andBLOCKEDare returned unchanged.- No existing status leads to a new
COMPLIANTrow andCOMPLIANTin the response.That’s consistent with the simplified
upsertComplianceStatuslogic described in the PR.The only minor mismatch is naming vs. coverage: the test names say “for any action”, but the request
bodyalways uses the defaultComplianceAction.CONNECT. To avoid confusion and to protect against reintroducing action-dependent behavior, you could either:
- Parameterize over actions, for example:
it.each([ ComplianceAction.CONNECT, ComplianceAction.VALID_SURVEY, ComplianceAction.INVALID_SURVEY, // ComplianceAction.COMPLIANCE_PROGRESSION if still supported ])('...', async (action) => { const response = await sendRequest({ body: { ...body, action }, ... }); // existing assertions });or
- Rename the tests to explicitly mention CONNECT instead of “any action”.
Either way, current assertions themselves are correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 92e5f7af42f4b6f5f79bebaa5a98a3c15dd15665 and 7bf60ce733302c8b852abca862092dfcb659ca4f.
📒 Files selected for processing (1)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-06-19T15:01:43.117Z
Learnt from: vincentwschau
Repo: dydxprotocol/v4-chain PR: 1728
File: indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts:327-394
Timestamp: 2024-06-19T15:01:43.117Z
Learning: User vincentwschau acknowledged the correction suggestion about renaming the function `upsertComplicanceStatus` to `upsertComplianceStatus` with a thumbs-up emoji, indicating agreement or approval.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
📚 Learning: 2025-10-16T22:21:49.129Z
Learnt from: Kefancao
Repo: dydxprotocol/v4-chain PR: 3162
File: indexer/services/comlink/src/lib/apple-helpers.ts:151-164
Timestamp: 2025-10-16T22:21:49.129Z
Learning: In the dydxprotocol/v4-chain codebase, when implementing Apple Sign-In for Turnkey authentication in `indexer/services/comlink/src/lib/apple-helpers.ts`, the ID token obtained from Apple's token endpoint is passed to Turnkey for validation. The email extraction from the ID token using `decodeJwt` (without signature verification) is acceptable because Turnkey handles the actual token validation in the `socialSignin` flow, and the email extraction is just for convenience.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
🧬 Code graph analysis (1)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (4)
indexer/packages/postgres/src/types/db-model-types.ts (1)
ComplianceStatusFromDatabase(252-258)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
stats(377-385)indexer/packages/redis/__tests__/caches/constants.ts (1)
createdAt(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: build-and-push-mainnet
- GitHub Check: build-and-push-testnet
- GitHub Check: run_command
- GitHub Check: lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (2)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (2)
12-26: Imports and compliance helpers are wired correctly
RequestMethod,BlockedCode, and the compliance helpers/constants from@dydxprotocol-indexer/complianceare used consistently throughout the tests (request typing, geoblock payload assertions, header/address checks). This centralizes the geoblock message/code and matches the PR’s intent.
471-494: Whitelisted restricted-country behavior is explicit and statelessThis test clearly codifies that restricted-but-whitelisted requests return
COMPLIANTand do not create/update anyComplianceStatusrows, keeping/geoblock*stateless for whitelisted users. That aligns well with “don’t strike” semantics for exemptions.If product intent was instead to treat whitelisted users as fully non-restricted for status persistence, that would imply creating a
COMPLIANTrow here, so this test effectively documents the chosen behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (1)
359-390: Stale docstring does not reflect simplified implementation.The docstring describes complex state transitions (FIRST_STRIKE_CLOSE_ONLY, VALID_SURVEY/INVALID_SURVEY handling, restricted country logic) that no longer exist in the implementation. The actual function (lines 401-409) simply:
- Upserts COMPLIANT if no status exists
- Returns the existing status unchanged otherwise
This mismatch will confuse future maintainers.
/** - * If the address doesn't exist in the compliance table: - * - if the request is from a restricted country: - * - if the action is CONNECT and no wallet, set the status to BLOCKED - * - if the action is CONNECT and wallet exists, set the status to FIRST_STRIKE_CLOSE_ONLY - * - else if the request is from a non-restricted country: - * - set the status to COMPLIANT - * - * if the address is COMPLIANT: - * - the ONLY action should be CONNECT. VALID_SURVEY/INVALID_SURVEY are no-ops. - * - if the request is from a restricted country: - * - set the status to FIRST_STRIKE_CLOSE_ONLY - * - * if the address is FIRST_STRIKE_CLOSE_ONLY: - * - the ONLY actions should be VALID_SURVEY/INVALID_SURVEY/CONNECT. - * - if the request is from a restricted country: - * - set the status to CLOSE_ONLY - * - else if the request is from a non-restricted country: - * - set the status to COMPLIANT - * - * - if the action is VALID_SURVEY: - * - set the status to COMPLIANT unless the address is CLOSE_ONLY - * - * - if the action is INVALID_SURVEY: - * - set the status to CLOSE_ONLY - * - * if the address is FIRST_STRIKE: - * - if the request is from a restricted country: - * - set the status to CLOSE_ONLY - * - else if the request is from a non-restricted country: - * - set the status to COMPLIANT + * Upserts or returns the compliance status for a given address. + * + * Note: Geo-restricted requests are handled earlier in checkCompliance() and return 403, + * so this function only handles non-restricted requests. + * + * - If no compliance status exists: upsert as COMPLIANT + * - If a compliance status exists: return it unchanged */
🧹 Nitpick comments (1)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (1)
392-410: Unused parameters should be removed for clarity.The parameters
req,action, andwalletare never used in the function body. While this is intentional per the PR objectives (survey-driven transitions were removed), keeping unused parameters creates confusion and suggests incomplete implementation.async function upsertComplianceStatus( - req: express.Request, - action: ComplianceAction, address: string, - wallet: WalletFromDatabase | undefined, complianceStatus: ComplianceStatusFromDatabase[], updatedAt: string, ): Promise<ComplianceStatusFromDatabase | undefined> {Then update the call site at lines 311-318:
const complianceStatusFromDatabase: ComplianceStatusFromDatabase | undefined = await upsertComplianceStatus( - req, - action, address, - wallet, complianceStatus, updatedAt, );And remove the unused
walletquery at line 306 if not needed elsewhere:- const [ - complianceStatus, - wallet, - ]: [ + const complianceStatus: ComplianceStatusFromDatabase[], - WalletFromDatabase | undefined, - ] = await Promise.all([ - ComplianceStatusTable.findAll( + = await ComplianceStatusTable.findAll( { address: [address] }, [], - ), - WalletTable.findById(address), - ]); + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7bf60ce733302c8b852abca862092dfcb659ca4f and 9110a11a83f0d27965f5231905bd730e2cdfe923.
📒 Files selected for processing (3)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts(3 hunks)indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts(7 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/lib/compliance-and-geo-check.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-06-19T15:01:43.117Z
Learnt from: vincentwschau
Repo: dydxprotocol/v4-chain PR: 1728
File: indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts:327-394
Timestamp: 2024-06-19T15:01:43.117Z
Learning: User vincentwschau acknowledged the correction suggestion about renaming the function `upsertComplicanceStatus` to `upsertComplianceStatus` with a thumbs-up emoji, indicating agreement or approval.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
📚 Learning: 2025-10-16T22:21:49.129Z
Learnt from: Kefancao
Repo: dydxprotocol/v4-chain PR: 3162
File: indexer/services/comlink/src/lib/apple-helpers.ts:151-164
Timestamp: 2025-10-16T22:21:49.129Z
Learning: In the dydxprotocol/v4-chain codebase, when implementing Apple Sign-In for Turnkey authentication in `indexer/services/comlink/src/lib/apple-helpers.ts`, the ID token obtained from Apple's token endpoint is passed to Turnkey for validation. The email extraction from the ID token using `decodeJwt` (without signature verification) is acceptable because Turnkey handles the actual token validation in the `socialSignin` flow, and the email extraction is just for convenience.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
📚 Learning: 2024-11-22T18:12:04.606Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
Applied to files:
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
🧬 Code graph analysis (2)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (5)
indexer/packages/postgres/src/types/db-model-types.ts (1)
ComplianceStatusFromDatabase(252-258)indexer/services/comlink/__tests__/helpers/helpers.ts (1)
sendRequest(82-106)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
stats(377-385)indexer/packages/redis/__tests__/caches/constants.ts (1)
createdAt(32-32)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (4)
indexer/packages/compliance/src/geoblocking/restrict-countries.ts (1)
isRestrictedCountryHeaders(6-31)indexer/packages/compliance/src/types.ts (1)
GeoOriginHeaders(8-12)indexer/services/comlink/src/lib/helpers.ts (1)
create4xxResponse(157-169)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: lint
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (7)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (4)
12-26: LGTM! Import changes align with new geoblock behavior.The imports are correctly updated to include
BlockedCodefrom types and geoblock-related exports from@dydxprotocol-indexer/compliance.
496-526: Good test coverage for geoblock scenarios.The test correctly verifies:
- 403 status code
- Error payload structure with
INDEXER_GEOBLOCKED_PAYLOADmessage andBlockedCode.GEOBLOCKEDcode- No DB state mutations
- No stats increment for status changes
This pattern is consistently applied across all geoblock test cases.
720-749: Test name now correctly reflects behavior.The test name "should not mutate status for any action from a non-restricted country with existing compliance status FIRST_STRIKE or FIRST_STRIKE_CLOSE_ONLY" accurately describes the assertions. The parameterized test properly verifies that existing statuses are preserved.
778-796: Test coverage aligns with simplified upsert logic.The test correctly verifies that non-restricted requests with no existing compliance status result in COMPLIANT status. This aligns with the PR's removal of survey-driven transitions.
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3)
10-11: LGTM! Imports correctly added for geoblock response.
286-293: Clean implementation of early geoblock rejection.The early return with 403 correctly prevents DB mutations and returns the standardized error payload. This aligns with the PR objectives.
342-357: No issues with handleError function.The spacing adjustment in the function signature is a minor formatting change with no functional impact.
Compliance Controller /geoblock* endpoint changes: - If a connection is whitelisted by ip or chain address, proceed. - Then, if a connection is restricted, raise a 403 geo-blocking error, but don't change compliance status in the database. - If the connection was not restricted, return the compliance status for the address in the database. - If the address has no current compliance status, query the compliance provider to classify the address and set the compliance status, and then return the new compliance status. - The VALID_SURVEY/INVALID_SURVEY api no longer modifies compliance status.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (2)
319-331: Remove dead metrics code.With the simplified upsertComplianceStatus logic, this metrics increment code is unreachable:
- New addresses → create COMPLIANT → condition
status !== ComplianceStatus.COMPLIANTis false- Existing addresses → return unchanged → condition
complianceStatus[0] !== complianceStatusFromDatabaseis falsePer PR objectives, metrics increments for geoblocked status changes were removed, so this code should be deleted for clarity.
Apply this diff to remove the dead code:
const updatedAt: string = DateTime.utc().toISO(); const complianceStatusFromDatabase: ComplianceStatusFromDatabase | undefined = await upsertComplianceStatus( req, action, address, wallet, complianceStatus, updatedAt, ); - if (complianceStatus.length === 0 || - complianceStatus[0] !== complianceStatusFromDatabase) { - if (complianceStatusFromDatabase !== undefined && - complianceStatusFromDatabase.status !== ComplianceStatus.COMPLIANT - ) { - stats.increment( - `${config.SERVICE_NAME}.${controllerName}.geo_block${forKeplr ? '_keplr' : ''}.compliance_status_changed.count`, - { - newStatus: complianceStatusFromDatabase!.status, - }, - ); - } - } const response = { status: complianceStatusFromDatabase!.status, reason: complianceStatusFromDatabase!.reason, updatedAt: complianceStatusFromDatabase!.updatedAt, };
370-388: Remove unused parameters.The
actionandwalletparameters are never used in the function body. With survey-driven transitions removed (per PR objectives), these parameters are no longer needed.Apply this diff to remove unused parameters:
async function upsertComplianceStatus( req: express.Request, - action: ComplianceAction, address: string, - wallet: WalletFromDatabase | undefined, complianceStatus: ComplianceStatusFromDatabase[], updatedAt: string, ): Promise<ComplianceStatusFromDatabase | undefined> { if (complianceStatus.length === 0) { return ComplianceStatusTable.upsert({ address, status: ComplianceStatus.COMPLIANT, updatedAt, }); } return complianceStatus[0]; }Then update the function call at line 311:
const complianceStatusFromDatabase: ComplianceStatusFromDatabase | undefined = await upsertComplianceStatus( req, - action, address, - wallet, complianceStatus, updatedAt, );
🧹 Nitpick comments (2)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (1)
496-796: Comprehensive geoblock test coverage.The test suite thoroughly validates:
- 403 responses for all restricted-country scenarios
- No DB mutations when geoblocked
- Proper error payload structure (INDEXER_GEOBLOCKED_PAYLOAD + BlockedCode.GEOBLOCKED)
- Status preservation for non-restricted countries
- Whitelisted address bypass
Optional: Consider adding an explicit test for
INVALID_SURVEYorVALID_SURVEYactions from non-restricted countries to document that survey-driven transitions have been removed and these actions now behave identically toCONNECT.it('should set status to COMPLIANT for INVALID_SURVEY action from a non-restricted country with no existing compliance status', async () => { isRestrictedCountryHeadersSpy.mockReturnValue(false); const response: any = await sendRequest({ type: RequestMethod.POST, path: endpoint, body: { ...body, action: ComplianceAction.INVALID_SURVEY, }, expectedStatus: 200, }); const data: ComplianceStatusFromDatabase[] = await ComplianceStatusTable.findAll({}, [], {}); expect(data).toHaveLength(1); expect(data[0]).toEqual(expect.objectContaining({ address: testConstants.defaultAddress, status: ComplianceStatus.COMPLIANT, })); expect(response.body.status).toEqual(ComplianceStatus.COMPLIANT); });indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (1)
359-368: Documentation accurately reflects new behavior.The comments correctly document the simplified upsert logic. Minor clarity improvement: "blocked upstream" might be clearer as "blocked by early geoblock check in checkCompliance" since the blocking happens in the same controller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 9110a11a83f0d27965f5231905bd730e2cdfe923 and 5095f69.
📒 Files selected for processing (3)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts(3 hunks)indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts(7 hunks)indexer/services/comlink/src/lib/compliance-and-geo-check.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- indexer/services/comlink/src/lib/compliance-and-geo-check.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-06-19T15:01:43.117Z
Learnt from: vincentwschau
Repo: dydxprotocol/v4-chain PR: 1728
File: indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts:327-394
Timestamp: 2024-06-19T15:01:43.117Z
Learning: User vincentwschau acknowledged the correction suggestion about renaming the function `upsertComplicanceStatus` to `upsertComplianceStatus` with a thumbs-up emoji, indicating agreement or approval.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
📚 Learning: 2025-10-16T22:21:49.129Z
Learnt from: Kefancao
Repo: dydxprotocol/v4-chain PR: 3162
File: indexer/services/comlink/src/lib/apple-helpers.ts:151-164
Timestamp: 2025-10-16T22:21:49.129Z
Learning: In the dydxprotocol/v4-chain codebase, when implementing Apple Sign-In for Turnkey authentication in `indexer/services/comlink/src/lib/apple-helpers.ts`, the ID token obtained from Apple's token endpoint is passed to Turnkey for validation. The email extraction from the ID token using `decodeJwt` (without signature verification) is acceptable because Turnkey handles the actual token validation in the `socialSignin` flow, and the email extraction is just for convenience.
Applied to files:
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts
📚 Learning: 2024-11-22T18:12:04.606Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
Applied to files:
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
🧬 Code graph analysis (2)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (6)
indexer/packages/postgres/src/types/db-model-types.ts (1)
ComplianceStatusFromDatabase(252-258)indexer/services/comlink/__tests__/helpers/helpers.ts (1)
sendRequest(82-106)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
stats(377-385)indexer/packages/redis/__tests__/caches/constants.ts (1)
createdAt(32-32)v4-proto-rs/src/cosmos.base.node.v1beta1.rs (1)
status(194-214)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (4)
indexer/packages/compliance/src/geoblocking/restrict-countries.ts (1)
isRestrictedCountryHeaders(6-31)indexer/packages/compliance/src/types.ts (1)
GeoOriginHeaders(8-12)indexer/services/comlink/src/lib/helpers.ts (1)
create4xxResponse(157-169)indexer/packages/compliance/src/constants.ts (1)
INDEXER_GEOBLOCKED_PAYLOAD(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (3)
indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (1)
1-28: LGTM!The imports are correctly updated to include the new geoblock-related entities (BlockedCode, INDEXER_GEOBLOCKED_PAYLOAD) and compliance utilities.
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (2)
1-46: LGTM!Imports correctly updated to include INDEXER_GEOBLOCKED_PAYLOAD and BlockedCode for the geoblock implementation.
286-293: LGTM!The early geoblock check correctly enforces immediate 403 responses for restricted geographies, preventing any DB mutations or downstream processing.
|
@Mergifyio backport release/indexer/v9.4.x |
✅ Backports have been createdDetails
|
|
@Mergifyio backport release/indexer/v9.5.x |
✅ Backports have been createdDetails
|
|
@Mergifyio backport release/indexer/v9.4.x-2 |
✅ Backports have been createdDetails
|
|
@Mergifyio backport release/indexer/v9.4.2 |
✅ Backports have been createdDetails
|
Summary
COMPLIANT→FIRST_STRIKE_CLOSE_ONLY→CLOSE_ONLY→BLOCKEDbased on connection attempts and surveys.403 Forbiddenimmediately, regardless of existing compliance status or action type.COMPLIANT; existing non-COMPLIANTstatuses are returned as-is.Details
API behavior changes
CONNECT,VALID_SURVEY,INVALID_SURVEY) from restricted geolocations now return403with error codeBlockedCode.GEOBLOCKEDand payloadINDEXER_GEOBLOCKED_PAYLOAD, before any database lookups or mutations.COMPLIANTstatus created in DB,200returned.200returned (includingBLOCKED,FIRST_STRIKE,FIRST_STRIKE_CLOSE_ONLY,CLOSE_ONLY).Risk & Impact
Breaking change: High impact on compliance enforcement.
FIRST_STRIKE,FIRST_STRIKE_CLOSE_ONLY, andCLOSE_ONLYrecords in the database are preserved but no longer updated by this controller.Testing
compliance-v2-controller.test.ts:403response for all actions (CONNECT,VALID_SURVEY,INVALID_SURVEY) from restricted countries, regardless of pre-existing status (COMPLIANT,FIRST_STRIKE,FIRST_STRIKE_CLOSE_ONLY,CLOSE_ONLY, or none).it.each) verifies non-restricted requests return existingFIRST_STRIKE/FIRST_STRIKE_CLOSE_ONLYstatuses unchanged.BLOCKEDstatus persists for non-restricted requests.COMPLIANT→FIRST_STRIKE_CLOSE_ONLY, survey-based transitions).Reviewer Notes
upsertComplianceStatus()is now effectively a no-op for existing addresses; all mutation logic removed.compliance_status_changedmetrics are no longer emitted for geoblocked requests (verified in tests).