[OTE-625] Keplr geoblock new endpoint #2117
Conversation
WalkthroughThe recent changes enhance the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (3)
- indexer/services/comlink/tests/controllers/api/v4/compliance-v2-controller.test.ts (15 hunks)
- indexer/services/comlink/package.json (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3 hunks)
Additional comments not posted (27)
indexer/services/comlink/package.json (1)
35-35: Dependency Addition Approved.The addition of
@keplr-wallet/cosmosversion^0.12.122is appropriate for integrating Keplr wallet support.indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (7)
38-38: New Import Approved.The import of
verifyADR36Aminofrom@keplr-wallet/cosmosis necessary for Keplr wallet signature verification.
217-231: Refactored Geoblock Endpoint Logic Approved.The refactoring of the
/geoblockendpoint to usevalidateSignatureandcheckCompliancefunctions improves code readability and maintainability. Error handling is appropriately managed.
235-266: New Geoblock-Keplr Endpoint Logic Approved.The introduction of the
/geoblock-keplrendpoint withvalidateSignatureKeplrenhances functionality for Keplr wallet users. The logic is clear and consistent with existing patterns.
277-332: Signature Validation Function Approved.The
validateSignaturefunction correctly implements signature validation using Secp256k1. The logic for address and timestamp validation is sound.
334-356: Keplr Signature Validation Function Approved.The
validateSignatureKeplrfunction appropriately leveragesverifyADR36Aminofor Keplr wallet signature verification. The implementation is straightforward and efficient.
358-413: Compliance Check Function Approved.The
checkCompliancefunction effectively handles compliance status checks and updates. The use of database queries and response formatting is well-structured.
415-430: Error Handling Function Approved.The
handleErrorfunction centralizes error logging and response creation, improving consistency across endpoints.indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (19)
21-21: Mocking of Keplr Wallet Function Approved.The mock setup for
verifyADR36Aminois correctly implemented, allowing for controlled testing of Keplr wallet interactions.
337-338: New Endpoint Constants Approved.The addition of
geoblockEndpointandgeoblockKeplerEndpointconstants improves test organization and readability.
339-353: Request Body Constants Approved.The definition of
geoblockBodyandgeoblockKeplrBodystandardizes request structures, enhancing test clarity and maintainability.
359-374: Parameterized Test Structure Approved.The use of
describe.eachfor testing both geoblock endpoints is efficient and improves test coverage.
388-398: Test for Non-dYdX Address Approved.The test correctly verifies that a non-dYdX address results in a 400 response for the
/geoblockendpoint.
402-412: Test for Invalid Timestamp Approved.The test accurately checks for a 400 response when the timestamp is invalid for the
/geoblockendpoint.
416-425: Test for Invalid Signature Approved.The test ensures that an invalid signature results in a 400 response for the
/geoblockendpoint.
429-438: Test for Incorrect Address Approved.The test verifies that an incorrect address results in a 400 response for the
/geoblockendpoint.
440-449: Test for Failed Keplr Validation Approved.The test correctly checks for a 400 response when Keplr signature validation fails for the
/geoblock-keplrendpoint.
Line range hint
455-459: Test for Valid Request Approved.The test confirms that a valid request is processed correctly, returning a 200 response with a COMPLIANT status.
Line range hint
474-487: Test for Whitelisted Address Approved.The test ensures that a whitelisted address from a restricted country returns a COMPLIANT status.
Line range hint
493-508: Test for BLOCKED Status Approved.The test verifies that a CONNECT action from a restricted country with no existing compliance status results in a BLOCKED status.
Line range hint
522-537: Test for FIRST_STRIKE_CLOSE_ONLY Status Approved.The test confirms that a CONNECT action from a restricted country with a wallet results in a FIRST_STRIKE_CLOSE_ONLY status.
Line range hint
552-561: Test for COMPLIANT Status Approved.The test ensures that any action from a non-restricted country results in a COMPLIANT status.
Line range hint
577-592: Test for Status Update to FIRST_STRIKE_CLOSE_ONLY Approved.The test verifies that a CONNECT action from a restricted country updates the status to FIRST_STRIKE_CLOSE_ONLY.
Line range hint
609-624: Test for Status Update to CLOSE_ONLY Approved.The test confirms that a CONNECT action from a restricted country updates the status to CLOSE_ONLY.
Line range hint
648-663: Test for Existing CLOSE_ONLY Status Approved.The test ensures that a CONNECT action from a restricted country with an existing CLOSE_ONLY status does not change the status.
Line range hint
674-689: Test for INVALID_SURVEY Action Approved.The test verifies that an INVALID_SURVEY action updates the status to CLOSE_ONLY.
Line range hint
710-725: Test for VALID_SURVEY Action Approved.The test confirms that a VALID_SURVEY action updates the status to FIRST_STRIKE.
jerryfan01234
force-pushed
the
keplr-geoblock-new-endpoint
branch
from
21dacb1
to
f09d00c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (3)
- indexer/services/comlink/tests/controllers/api/v4/compliance-v2-controller.test.ts (14 hunks)
- indexer/services/comlink/package.json (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/comlink/tests/controllers/api/v4/compliance-v2-controller.test.ts
- indexer/services/comlink/package.json
Additional comments not posted (7)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (7)
15-15: Import statement approved.The import of
verifyADR36Aminofrom@keplr-wallet/cosmosis necessary for the new Keplr signature verification functionality.
279-334: Signature validation logic approved.The
validateSignaturefunction encapsulates the signature validation logic effectively, improving code organization and reusability.
336-358: Keplr signature validation logic approved.The
validateSignatureKeplrfunction simplifies Keplr signature verification usingverifyADR36Amino, integrating well with the existing logic.
360-415: Compliance check logic approved.The
checkCompliancefunction effectively handles compliance status retrieval and response formatting, ensuring accurate compliance checks.
417-432: Error handling logic approved.The
handleErrorfunction centralizes error handling, improving maintainability and consistency in error reporting.
217-231: Refactored/geoblockendpoint logic approved.The refactoring of the
/geoblockendpoint improves the control flow by separating validation and compliance logic, enhancing readability and maintainability.
235-270: Refactored/geoblock-keplrendpoint logic approved.The refactoring of the
/geoblock-keplrendpoint improves the control flow by separating validation and compliance logic, enhancing readability and maintainability.
jerryfan01234
force-pushed
the
keplr-geoblock-new-endpoint
branch
from
f09d00c
to
738e43e
Compare
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
Outdated
Show resolved
Hide resolved
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
Show resolved
Hide resolved
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
Show resolved
Hide resolved
| `${config.SERVICE_NAME}.${controllerName}.geo_block.compliance_status_changed.count`, | ||
| { | ||
| newStatus: complianceStatusFromDatabase!.status, | ||
| }, |
There was a problem hiding this comment.
nit: could you also add if this was a regular or a Keplr signature in teh stats?
| @@ -5,6 +5,7 @@ | |||
| "outDir": "build", | |||
| "experimentalDecorators": true, | |||
| "emitDecoratorMetadata": true, | |||
| "skipLibCheck": true | |||
There was a problem hiding this comment.
Turns out @keplr-wallet/cosmos will throw ts errors, unless "skipLibCheck": true.
There was a problem hiding this comment.
During compile, @keplr-wallet/cosmos throws many type errors. The simplest resolution is to "skipLibCheck": true, which will skip type checking for dependency code that we do not reference.
https://www.typescriptlang.org/tsconfig/#skipLibCheck
Skip type checking of declaration files.
This can save time during compilation at the expense of type-system accuracy. For example, two libraries could define two copies of the same type in an inconsistent way. Rather than doing a full check of all d.ts files, TypeScript will type check the code you specifically refer to in your app’s source code.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (4)
- indexer/services/comlink/tests/controllers/api/v4/compliance-v2-controller.test.ts (14 hunks)
- indexer/services/comlink/package.json (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3 hunks)
- indexer/services/comlink/tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/services/comlink/package.json
- indexer/services/comlink/tsconfig.json
Files skipped from review as they are similar to previous changes (1)
- indexer/services/comlink/tests/controllers/api/v4/compliance-v2-controller.test.ts
Additional comments not posted (4)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (4)
349-373: LGTM! But verify the function usage in the codebase.The
validateSignatureKeplrfunction is straightforward and leverages theverifyADR36Aminomethod effectively.However, ensure that all function calls to
validateSignatureKeplrare correctly implemented.Run the following script to verify the function usage:
Verification successful
Function Usage Verified
The
validateSignatureKeplrfunction is used correctly within the same file where it is defined. The function call includes all necessary parameters and handles the response appropriately.
- File:
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts- Usage: The function is called with the expected parameters, and its return value is checked for a failed validation response.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `validateSignatureKeplr`. # Test: Search for the function usage. Expect: Correct implementation in all occurrences. rg --type typescript -A 5 $'validateSignatureKeplr'Length of output: 85
Script:
#!/bin/bash # Description: Verify all function calls to `validateSignatureKeplr`. # Test: Search for the function usage in TypeScript files. rg --glob '*.ts' -A 5 $'validateSignatureKeplr'Length of output: 1312
286-341: LGTM! But verify the function usage in the codebase.The
validateSignaturefunction is well-structured and handles errors appropriately.However, ensure that all function calls to
validateSignatureare correctly implemented.Run the following script to verify the function usage:
Verification successful
Function usage verified successfully.
The
validateSignaturefunction is correctly implemented and used within the same file. It is called with the necessary parameters, and the response is handled appropriately.
- File:
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `validateSignature`. # Test: Search for the function usage. Expect: Correct implementation in all occurrences. rg --type typescript -A 5 $'validateSignature'Length of output: 80
Script:
#!/bin/bash # Description: Verify all function calls to `validateSignature`. # Test: Search for the function usage. Expect: Correct implementation in all occurrences. rg --type ts -A 5 $'validateSignature'Length of output: 2615
436-451: LGTM! But verify the function usage in the codebase.The
handleErrorfunction centralizes error handling and logging effectively.However, ensure that all function calls to
handleErrorare correctly implemented.Run the following script to verify the function usage:
Verification successful
Function Usage Verified
The
handleErrorfunction is correctly implemented and consistently used within thecompliance-v2-controller.tsfile.
- The function is called with appropriate parameters matching its definition.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `handleError`. # Test: Search for the function usage. Expect: Correct implementation in all occurrences. rg --type typescript -A 5 $'handleError'Length of output: 74
Script:
#!/bin/bash # Description: Verify all function calls to `handleError`. # Test: Search for the function usage. Expect: Correct implementation in all occurrences. rg --type ts -A 5 $'handleError'Length of output: 2070
375-434: LGTM! But verify the function usage in the codebase.The
checkCompliancefunction is well-implemented and maintains consistency in error handling.However, ensure that all function calls to
checkComplianceare correctly implemented.Run the following script to verify the function usage:
Verification successful
Function Usage Verified
The
checkCompliancefunction is used correctly in the codebase. The function calls are implemented with the expected parameters, and error handling is consistent with the rest of the code.
- The function is called with appropriate arguments and error handling in place.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `checkCompliance`. # Test: Search for the function usage. Expect: Correct implementation in all occurrences. rg --type typescript -A 5 $'checkCompliance'Length of output: 78
Script:
#!/bin/bash # Description: Verify all function calls to `checkCompliance`. # Find all TypeScript files and search for the function usage within them. fd --extension ts --exec rg -A 5 'checkCompliance' {}Length of output: 755
jerryfan01234
force-pushed
the
keplr-geoblock-new-endpoint
branch
from
0cfef79
to
c730764
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (2)
279-285: Correct the return type in the docstring.The return type in the docstring should match the function signature.
* @returns {Promise<express.Response | undefined>} Returns undefined if validation
343-347: Correct the return type in the docstring.The return type in the docstring should match the function signature.
* @returns {express.Response | undefined} Returns undefined if validation
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/services/comlink/tests/controllers/api/v4/compliance-v2-controller.test.ts (14 hunks)
- indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3 hunks)
Additional comments not posted (6)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (2)
375-434: LGTM!The
checkCompliancefunction is well-structured and handles different compliance scenarios effectively.
436-451: LGTM!The
handleErrorfunction centralizes error handling, enhancing maintainability and consistency.indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (4)
43-46: LGTM!The test setup and mocks are appropriately set up to control the behavior of external dependencies during testing.
Line range hint
87-271: LGTM!The test cases for GET requests are comprehensive and ensure that different compliance scenarios are handled correctly.
Line range hint
273-335: LGTM!The test cases for POST /setStatus effectively validate the behavior of the
/setStatusendpoint.
Line range hint
336-717: LGTM!The test cases for POST /geoblock and /geoblock-keplr are well-structured and ensure thorough testing of both endpoints.
jerryfan01234
force-pushed
the
keplr-geoblock-new-endpoint
branch
from
c730764
to
a7cff5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (2)
286-341: Add documentation for successful validation.The function lacks documentation on the behavior when validation is successful. Consider adding this information for clarity.
* @returns {Promise<express.Response | undefined>} Returns undefined if validation * is successful. Returns an HTTP response with an error message if validation fails. + * If validation is successful, the function allows further processing without interruption. */
349-373: Add documentation for successful validation.The function lacks documentation on the behavior when validation is successful. Consider adding this information for clarity.
* @returns {Promise<express.Response | undefined>} Returns undefined if validation * is successful. Returns an HTTP response with an error message if validation fails. + * If validation is successful, the function allows further processing without interruption. */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/services/comlink/tests/controllers/api/v4/compliance-v2-controller.test.ts (15 hunks)
- indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3 hunks)
Additional comments not posted (9)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (5)
15-15: Import statement approved.The import of
verifyADR36Aminois necessary for Keplr-specific signature validation.
375-434: Function implementation approved.The
checkCompliancefunction is well-structured and effectively manages compliance checks and updates.
436-451: Function implementation approved.The
handleErrorfunction is well-implemented, providing consistent error handling across endpoints.
217-233: Endpoint implementation approved.The
/geoblockendpoint is correctly structured with appropriate validation and error handling.
Line range hint
235-273: Endpoint implementation approved.The
/geoblock-keplrendpoint is correctly structured with appropriate validation and error handling.indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts (4)
21-21: Import statement approved.The import of
verifyADR36Aminois necessary for mocking Keplr-specific signature validation in tests.
43-46: Mock setup approved.The mock setup for
verifyADR36Aminois appropriate for testing Keplr-specific scenarios.
336-352: Constants definition approved.The new constants enhance readability and maintainability of the test code.
Line range hint
358-732: Parameterized tests approved.The use of
describe.eachfor testing both geoblock endpoints is efficient and comprehensive.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (3 hunks)
Additional comments not posted (5)
indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts (5)
15-15: Import statement is appropriate.The import of
verifyADR36Aminofrom@keplr-wallet/cosmosis necessary for Keplr signature validation.
349-373: FunctionvalidateSignatureKeplris effectively implemented.The function uses
verifyADR36Aminoto validate Keplr signatures, ensuring streamlined verification.
375-434: FunctioncheckComplianceis comprehensive.The function effectively handles compliance checks and updates, ensuring accurate status management.
436-451: FunctionhandleErrorcentralizes error handling effectively.The function logs errors and returns standardized responses, enhancing maintainability.
286-341: FunctionvalidateSignatureis well-structured.The function performs comprehensive validation checks, including address format and signature verification.
However, ensure that the removal of timestamp checks in other parts of the codebase aligns with the intended functionality.
Run the following script to verify the usage of timestamp checks:
Verification successful
Timestamp checks in
validateSignatureare consistent with the codebase.The search results show that timestamps are actively used across various modules and tests, indicating that the timestamp check in the
validateSignaturefunction aligns with the codebase's functionality. No evidence of removal of similar checks was found.
- The timestamp check in
validateSignatureremains relevant and consistent.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of timestamp checks across the codebase. # Test: Search for timestamp usage. Expect: Consistent handling of timestamp checks. rg --type ts 'timestamp'Length of output: 26900
jerryfan01234
changed the title
|
@Mergifyio backport release/indexer/v6.x |
✅ Backports have been created
|
(cherry picked from commit a814b33) # Conflicts: # indexer/services/comlink/__tests__/controllers/api/v4/compliance-v2-controller.test.ts # indexer/services/comlink/src/controllers/api/v4/compliance-v2-controller.ts
Co-authored-by: jerryfan01234 <[email protected]> Co-authored-by: Jerry Fan <[email protected]>
Changelist
Added new geoblock endpoint powered by keplr.
Refactored existing geoblock endpoint to minimize duplicated code with new endpoint.
Modified existing unit tests to include additional tests for new endpoint
Test Plan
Deployed to internal mainnet and endpoint was tested via FE
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores