[OTE-784] Limit addresses for compliance check to dydx wallets with deposit #2330
Conversation
WalkthroughThe changes introduce a new constant for risk score handling in the compliance context, enhancing error management when addresses are not found on the blockchain. The modifications include updates to the compliance data retrieval logic, specifically addressing how 404 errors are handled and logged. Additionally, the changes expand the exported modules and improve the filtering of compliance data based on wallet addresses. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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 using PR comments)
Other keywords and placeholders
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: 4
Outside diff range and nitpick comments (9)
indexer/services/comlink/src/controllers/api/v4/compliance-controller.ts (1)
92-101: LGTM: New logic to handle invalid addresses. Consider adding logging.The new conditional logic successfully prevents invalid addresses from being upserted into the compliance table, aligning with the PR objectives. This implementation correctly handles both undefined riskScore and the specific NOT_IN_BLOCKCHAIN_RISK_SCORE case.
However, the PR objectives mentioned adding logging functionality to track these operations. Consider adding appropriate logging when an invalid address is encountered.
Here's a suggestion to add logging:
if (response.riskScore === undefined || Number(response.riskScore) === NOT_IN_BLOCKCHAIN_RISK_SCORE) { + logger.info({ + at: 'ComplianceControllerHelper#screen', + message: 'Invalid address encountered', + address, + riskScore: response.riskScore, + }); return { restricted: false, reason: undefined, }; }indexer/packages/compliance/src/clients/elliptic-provider.ts (2)
102-102: LGTM: Improved error handling for non-existent blockchain addressesThe use of
NOT_IN_BLOCKCHAIN_RISK_SCOREfor 404 errors aligns with the PR objective and improves code clarity. This change will help distinguish between addresses not found in the blockchain and those with no rules triggered.Consider adding a log statement before returning
NOT_IN_BLOCKCHAIN_RISK_SCOREto track these occurrences:if ( error?.response?.status === 404 && error?.response?.data?.name === 'NotInBlockchain' ) { stats.increment( `${config.SERVICE_NAME}.get_elliptic_risk_score.status_code`, { status: '404' }, ); + logger.info({ + at: 'EllipticProviderClient#getRiskScore', + message: 'Address not found in blockchain', + address, + }); return NOT_IN_BLOCKCHAIN_RISK_SCORE; }This will provide valuable information for monitoring and debugging purposes.
Action Required: Add Tests for Handling NotInBlockchain Addresses
The new changes introduce handling for addresses not found in the blockchain by using
NOT_IN_BLOCKCHAIN_RISK_SCORE. However, existing tests do not cover this scenario. To ensure the functionality works as intended, please add or update tests to include the following cases:
- Verify that
getRiskScorereturnsNOT_IN_BLOCKCHAIN_RISK_SCOREwhen a 404 error with the nameNotInBlockchainis received.- Ensure that
getComplianceResponsecorrectly handles theNOT_IN_BLOCKCHAIN_RISK_SCOREcase.Analysis chain
Line range hint
29-102: Suggestion: Update or add tests for the new behaviorThe changes improve the handling of addresses not found in the blockchain. To ensure the new behavior is properly tested and maintained, consider updating existing tests or adding new ones to cover the following scenarios:
- Test that
getRiskScorereturnsNOT_IN_BLOCKCHAIN_RISK_SCOREwhen a 404 error withNotInBlockchainname is received.- Verify that
getComplianceResponsecorrectly handles theNOT_IN_BLOCKCHAIN_RISK_SCOREcase.To help identify existing tests that may need updating, you can run the following command:
This will help locate the relevant test files where you should add or update tests for the new behavior.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files related to EllipticProviderClient rg -t ts --glob '**/*.test.ts' 'EllipticProviderClient'Length of output: 573
Script:
#!/bin/bash # Search for usage of NOT_IN_BLOCKCHAIN_RISK_SCORE in test files rg 'NOT_IN_BLOCKCHAIN_RISK_SCORE' indexer/packages/compliance/__tests__/clients/**/*.test.tsLength of output: 193
Script:
#!/bin/bash # Search for usage of NOT_IN_BLOCKCHAIN_RISK_SCORE in specific test files rg 'NOT_IN_BLOCKCHAIN_RISK_SCORE' indexer/packages/compliance/__tests__/clients/elliptic-provider.test.ts indexer/packages/compliance/__tests__/clients/clients.test.tsLength of output: 167
indexer/packages/postgres/__tests__/stores/compliance-data-table.test.ts (1)
144-165: LGTM: New test case is well-structured and aligns with PR objectives.The test case effectively verifies the filtering of compliance data based on the presence of addresses in the wallets table. This aligns well with the PR objective of restricting compliance checks to dYdX wallets with deposits.
Minor suggestion for improved clarity:
Consider adding a comment explaining the significance of
addressInWalletsTable: truein the context of this test. For example:// addressInWalletsTable: true simulates filtering for dYdX wallets with deposits const complianceData: ComplianceDataFromDatabase[] = await ComplianceDataTable.findAll( { addressInWalletsTable: true, }, [], { readReplica: true }, );This would make the purpose of the test more explicit to future readers.
indexer/services/comlink/__tests__/controllers/api/v4/compliance-controller.test.ts (1)
265-291: LGTM: New test case is well-implemented and aligns with PR objectives.The test case effectively verifies that invalid addresses are not upserted into the ComplianceTable and that the API responds correctly. It aligns well with the PR objectives and follows the existing test structure.
A minor suggestion for improvement:
Consider adding an assertion to verify that
complianceProvider.client.getComplianceResponsewas called exactly once with the invalid address. This would ensure that the mocked function is being used as expected. You can add this assertion just before the finalexpectstatement:expect(complianceProvider.client.getComplianceResponse).toHaveBeenCalledTimes(1); expect(complianceProvider.client.getComplianceResponse).toHaveBeenCalledWith(invalidAddress);indexer/packages/postgres/src/types/query-types.ts (1)
295-295: LGTM: Interface updated correctly, consider adding a commentThe addition of
[QueryableField.ADDRESS_IN_WALLETS_TABLE]?: booleanto theComplianceDataQueryConfiginterface is correct and aligns with the PR objective. It allows for filtering compliance data queries based on whether the address exists in the wallets table.Consider adding a brief comment above this line to explain the purpose of this new field, e.g.:
// Filter for addresses that exist in the wallets table (i.e., have a deposit) [QueryableField.ADDRESS_IN_WALLETS_TABLE]?: boolean,This would improve code readability and make the intent clearer for future developers.
indexer/services/roundtable/__tests__/tasks/update-compliance-data.test.ts (1)
682-740: The new test case looks good, but could benefit from a minor improvement.The test case "Only updates old addresses that are in wallets table" effectively verifies that the
updateComplianceDataTaskonly updates addresses present in the wallets table. However, there's a small typo in a variable name that should be corrected.There's a typo in the variable name
updatedCompliancnceDataon lines 703 and 711. It should beupdatedComplianceData. Here's the suggested fix:- const updatedCompliancnceData: ComplianceDataFromDatabase[] = await ComplianceTable.findAll({ + const updatedComplianceData: ComplianceDataFromDatabase[] = await ComplianceTable.findAll({ address: [testConstants.defaultAddress], }, [], {}); const unchangedComplianceData: ComplianceDataFromDatabase[] = await ComplianceTable.findAll({ address: [rogueWallet], }, [], {}); expectUpdatedCompliance( - updatedCompliancnceData[0], + updatedComplianceData[0], { address: testConstants.defaultAddress, blocked: true, riskScore, }, mockProvider.provider, );Overall, the test case is well-structured and effectively verifies the intended behavior:
- It sets up the test data correctly, including a rogue wallet address not in the wallets table.
- It calls the
updateComplianceDataTaskwith the mock provider.- It verifies that only the address in the wallets table is updated, while the rogue wallet remains unchanged.
- It checks the gauge stats to ensure they reflect the correct number of addresses processed.
The test case aligns well with the existing testing patterns in the file and provides good coverage for the specific scenario it's testing.
indexer/packages/postgres/src/stores/compliance-table.ts (2)
38-38: Consider renamingaddressInWalletsTablefor clarity.The parameter name
addressInWalletsTablecould be more descriptive. Consider renaming it tofilterAddressesInWalletsoronlyWalletsWithAddressto better convey its purpose in filtering compliance data based on wallet addresses existing in the wallets table.
76-82: Assess the performance impact of the added inner join.Adding an inner join to the query may affect performance, especially with large datasets. Ensure that appropriate indexes exist on the joined columns (
addressfields in both tables) to optimize query execution.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- indexer/packages/compliance/src/clients/elliptic-provider.ts (2 hunks)
- indexer/packages/compliance/src/index.ts (1 hunks)
- indexer/packages/postgres/tests/stores/compliance-data-table.test.ts (3 hunks)
- indexer/packages/postgres/src/stores/compliance-table.ts (4 hunks)
- indexer/packages/postgres/src/types/query-types.ts (2 hunks)
- indexer/services/comlink/tests/controllers/api/v4/compliance-controller.test.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/compliance-controller.ts (2 hunks)
- indexer/services/roundtable/tests/tasks/update-compliance-data.test.ts (1 hunks)
- indexer/services/roundtable/src/tasks/update-compliance-data.ts (4 hunks)
Additional comments not posted (17)
indexer/packages/compliance/src/index.ts (1)
8-8: New export added for elliptic-provider clientThe addition of
export * from './clients/elliptic-provider';expands the public API of the compliance package. This change aligns with the PR objectives by potentially exposing functionality related to address validation for the comlink compliance process.However, there are a few points to consider:
- Ensure that exposing the entire elliptic-provider module is intentional and necessary.
- Verify that this change doesn't unintentionally expose any sensitive or internal-only functionality.
- Consider if this addition requires updates to documentation or usage instructions for consumers of this package.
To ensure this change doesn't introduce any unintended consequences, please run the following verification script:
This script will help identify any potential issues with exposing the entire elliptic-provider module.
indexer/services/comlink/src/controllers/api/v4/compliance-controller.ts (2)
2-6: LGTM: New import for address validation.The addition of
NOT_IN_BLOCKCHAIN_RISK_SCOREfrom the compliance module aligns with the PR objectives to enhance address validation in the compliance process.
92-101: Verify the impact of these changes on the rest of the codebase.The new logic for handling invalid addresses is a significant change. Let's verify that this doesn't negatively impact other parts of the codebase that might expect all addresses to be present in the compliance table.
Run the following script to check for potential issues:
Verification successful
Compliance Data Handling Verified Successfully
The changes to handle invalid addresses in
compliance-controller.tshave been reviewed. No issues were found that negatively impact other parts of the codebase expecting all addresses to be present in the compliance table.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for places where compliance data is assumed to exist for all addresses # Test 1: Search for direct queries to the ComplianceTable that don't handle the case of missing data echo "Searching for potential issues with ComplianceTable queries:" rg --type typescript "ComplianceTable\.find" -A 5 -B 5 # Test 2: Search for uses of compliance data that might assume it always exists echo "Searching for uses of compliance data:" rg --type typescript "complianceData\." -A 5 -B 5 # Test 3: Search for other parts of the codebase that interact with the compliance screen echo "Searching for other interactions with the compliance screen:" rg --type typescript "compliance.*screen" -A 5 -B 5Length of output: 616
Script:
#!/bin/bash # Description: Search for places where compliance data is assumed to exist for all addresses # Test 1: Search for direct queries to the ComplianceTable that don't handle the case of missing data echo "Searching for potential issues with ComplianceTable queries:" rg "ComplianceTable\.find" --glob "*.ts" --glob "*.tsx" -A 5 -B 5 # Test 2: Search for uses of compliance data that might assume it always exists echo "Searching for uses of compliance data:" rg "complianceData\." --glob "*.ts" --glob "*.tsx" -A 5 -B 5 # Test 3: Search for other parts of the codebase that interact with the compliance screen echo "Searching for other interactions with the compliance screen:" rg "compliance.*screen" --glob "*.ts" --glob "*.tsx" -A 5 -B 5Length of output: 68799
indexer/packages/compliance/src/clients/elliptic-provider.ts (2)
29-29: LGTM: New constant improves code clarityThe addition of
NOT_IN_BLOCKCHAIN_RISK_SCOREenhances code readability and maintainability. It clearly distinguishes between addresses not found in the blockchain and those with no rules triggered.
Line range hint
29-102: Overall assessment: Changes look good with minor suggestionsThe modifications to handle addresses not found in the blockchain are well-implemented and align with the PR objectives. The new constant and its usage improve code clarity and maintainability.
Summary of suggestions:
- Add a log statement when returning
NOT_IN_BLOCKCHAIN_RISK_SCOREfor better monitoring.- Update or add tests to cover the new behavior.
These changes enhance the compliance process by ensuring that only valid dYdX wallets are considered, improving the integrity of compliance checks during roundtable updates.
indexer/packages/postgres/__tests__/stores/compliance-data-table.test.ts (2)
3-3: LGTM: Import changes are appropriate.The new imports for
WalletTableanddefaultWalletare correctly added and necessary for the new test case.Also applies to: 13-13
Line range hint
1-165: Overall assessment: Changes enhance test coverage without disrupting existing tests.The new test case "Successfully filters by onlyDydxAddressWithDeposit" integrates well with the existing test suite. It adds valuable coverage for the new filtering functionality without affecting other test cases. The changes are focused and align with the PR objectives of implementing restrictions on the compliance screen related to address validation for the comlink compliance process.
indexer/services/comlink/__tests__/controllers/api/v4/compliance-controller.test.ts (2)
12-16: LGTM: Import changes are appropriate.The addition of
NOT_IN_BLOCKCHAIN_RISK_SCOREto the imports is necessary for the new test case and is correctly placed with other imports from the same module.
Line range hint
1-291: Overall, the changes effectively implement the PR objectives.The addition of the new test case for invalid addresses complements the existing test suite and aligns well with the PR objectives. It verifies that the compliance screen does not perform an upsert operation when the address provided is not a valid dYdX wallet. The changes are well-contained and don't introduce any unintended side effects to the existing tests.
To ensure comprehensive coverage, let's verify if there are any other places in the codebase where address validation or compliance checks are performed:
This will help us identify if similar changes need to be applied elsewhere in the codebase.
indexer/packages/postgres/src/types/query-types.ts (1)
95-95: LGTM: New enum value added correctlyThe addition of
ADDRESS_IN_WALLETS_TABLEto theQueryableFieldenum is consistent with existing naming conventions and aligns with the PR objective of restricting compliance checks to valid dYdX wallets with deposits.indexer/services/roundtable/__tests__/tasks/update-compliance-data.test.ts (1)
Line range hint
1-740: Overall, the changes in this file look good.The new test case "Only updates old addresses that are in wallets table" is a valuable addition to the test suite. It effectively verifies that the
updateComplianceDataTaskonly updates addresses present in the wallets table, which is an important behavior to test.The test case is well-structured, follows the existing patterns in the file, and provides good coverage for the specific scenario it's testing. With the minor typo correction suggested earlier, this change enhances the overall test coverage and helps ensure the correct behavior of the compliance data update process.
indexer/packages/postgres/src/stores/compliance-table.ts (3)
16-16: ImportWalletModelis correctly added.The import of
WalletModelis necessary for the new inner join operation in the query.
38-38: Ensure all calls tofindAllare updated appropriately.Adding a new parameter
addressInWalletsTableto the exportedfindAllfunction might affect existing usages. Verify that existing calls either pass the new parameter if needed or that the default value maintains existing behavior.You can run the following script to locate all usages of
findAll:#!/bin/bash # Description: Find all usages of `findAll` in the codebase to ensure they handle `addressInWalletsTable`. # Expected result: Review each usage to confirm proper handling of the new parameter. rg --type typescript --fixed-strings '.findAll(' -A 3
76-82:⚠️ Potential issueVerify that the inner join correctly filters wallets with deposits as intended.
The current inner join with
WalletModelfilters compliance data to addresses present in the wallets table. However, the PR objective is to limit compliance checks to dYdX wallets that have a deposit. Ensure that the query accounts for wallets with deposits, not just any wallet addresses.To verify this, check if
WalletModelincludes a field indicating deposits (e.g.,has_deposit) and modify the query accordingly. For example:baseQuery = baseQuery.innerJoin( WalletModel.tableName, `${ComplianceDataModel.tableName}.${ComplianceDataColumns.address}`, '=', `${WalletModel.tableName}.${WalletModel.idColumn}` +).where(`${WalletModel.tableName}.has_deposit`, true);You can run the following script to confirm if
WalletModelhas a deposit indicator:indexer/services/roundtable/src/tasks/update-compliance-data.ts (3)
4-4: Import statement approvedThe import of
NOT_IN_BLOCKCHAIN_RISK_SCOREis necessary and correctly used in the code.
359-363:⚠️ Potential issueTypo in logger 'at' field
The
'at'field in the logger error message should be corrected from'updated-compliance-data#getComplianceData'to'update-compliance-data#getComplianceData'to match the file name and maintain consistency.Suggested change:
- at: 'updated-compliance-data#getComplianceData', + at: 'update-compliance-data#getComplianceData',Run the following script to check for similar typos in the codebase:
#!/bin/bash # Description: Search for instances of 'updated-compliance-data' in logger 'at' fields that should be 'update-compliance-data'. rg --type typescript "at:\s*'updated-compliance-data#getComplianceData'"
154-154: ConfirmaddressInWalletsTableparameter in queryEnsure that
addressInWalletsTableis a valid filter option forComplianceTable.findAlland behaves as expected in the query. This parameter filters addresses that are present in the wallets table, which aligns with the PR objective to limit compliance checks to wallets with deposits.Run the following script to verify that
addressInWalletsTableis a valid filter:
| @@ -92,6 +92,7 @@ export enum QueryableField { | |||
| REFEREE_ADDRESS = 'refereeAddress', | |||
| KEY = 'key', | |||
| TOKEN = 'token', | |||
| ADDRESS_IN_WALLETS_TABLE = 'addressInWalletsTable', | |||
There was a problem hiding this comment.
💡 Codebase verification
Ensure ADDRESS_IN_WALLETS_TABLE is Utilized in Relevant Components
The ADDRESS_IN_WALLETS_TABLE has been added to QueryableField and ComplianceDataQueryConfig. However, it is not currently used in any other components within the codebase.
- Location:
indexer/packages/postgres/src/types/query-types.ts
To fully implement the intended functionality, please integrate ADDRESS_IN_WALLETS_TABLE into the relevant parts of the application where compliance data queries are constructed and executed.
Analysis chain
Verify usage of new query field in related components
The addition of ADDRESS_IN_WALLETS_TABLE to QueryableField and ComplianceDataQueryConfig is well-implemented and aligns with the PR objective. These changes allow for filtering compliance data queries based on whether an address exists in the wallets table (i.e., has a deposit).
To ensure full implementation:
Please run the following script to check if the new query field is being used in relevant components:
This will help verify that the new query field is being utilized in the appropriate places within the codebase, particularly in compliance-related components.
Also applies to: 295-295
Re-verify usage of new query field in related components
The previous script encountered an error due to an unrecognized file type. Please run the following revised script to search for usage of ADDRESS_IN_WALLETS_TABLE in TypeScript and JavaScript files:
Re-verify usage of new query field in related components
The previous script failed because the src/ directory does not exist. Please run the following revised script to search for usage of ADDRESS_IN_WALLETS_TABLE across the entire repository:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ADDRESS_IN_WALLETS_TABLE in compliance-related files
rg --type typescript --type javascript 'ADDRESS_IN_WALLETS_TABLE' src/
Length of output: 105
Script:
#!/bin/bash
# Search for usage of ADDRESS_IN_WALLETS_TABLE in compliance-related files using file extensions
rg 'ADDRESS_IN_WALLETS_TABLE' --glob '*.ts' --glob '*.tsx' --glob '*.js' src/
Length of output: 122
Script:
#!/bin/bash
# Search for usage of ADDRESS_IN_WALLETS_TABLE in TypeScript and JavaScript files across the repository
rg 'ADDRESS_IN_WALLETS_TABLE' --glob '*.ts' --glob '*.tsx' --glob '*.js'
Length of output: 283
| @@ -45,6 +47,7 @@ export async function findAll( | |||
| provider, | |||
| blocked, | |||
| limit, | |||
| addressInWalletsTable, | |||
There was a problem hiding this comment.
Ensure addressInWalletsTable is compatible with QueryConfig in verifyAllRequiredFields.
The verifyAllRequiredFields function casts the object to QueryConfig. Verify that QueryConfig includes addressInWalletsTable to prevent potential type mismatches or runtime errors.
| @@ -34,6 +35,7 @@ | |||
| provider, | |||
| blocked, | |||
| limit, | |||
| addressInWalletsTable, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defaulting addressInWalletsTable to false for backward compatibility.
Adding addressInWalletsTable to the ComplianceDataQueryConfig may impact existing calls to findAll. Setting a default value of false ensures existing functionality remains unaffected if the parameter is not provided.
You can set a default value in the function parameter:
export async function findAll(
{
address,
updatedBeforeOrAt,
provider,
blocked,
limit,
- addressInWalletsTable,
+ addressInWalletsTable = false,
}: ComplianceDataQueryConfig,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addressInWalletsTable, | |
| address, | |
| updatedBeforeOrAt, | |
| provider, | |
| blocked, | |
| limit, | |
| addressInWalletsTable = false, |
| const responses404: | ||
| PromiseFulfilledResult<ComplianceClientResponse>[] = successResponses.filter( | ||
| (result: PromiseSettledResult<ComplianceClientResponse>): | ||
| result is PromiseFulfilledResult<ComplianceClientResponse> => { | ||
| return result.status === 'fulfilled' && result.value.riskScore === NOT_IN_BLOCKCHAIN_RISK_SCORE.toString(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Ensure consistent types in risk score comparison
The comparison between result.value.riskScore and NOT_IN_BLOCKCHAIN_RISK_SCORE.toString() may lead to unexpected results if the types are inconsistent. Consider ensuring that both values are of the same type when comparing.
Suggested change:
- return result.status === 'fulfilled' && result.value.riskScore === NOT_IN_BLOCKCHAIN_RISK_SCORE.toString();
+ return result.status === 'fulfilled' && result.value.riskScore === String(NOT_IN_BLOCKCHAIN_RISK_SCORE);Or, if riskScore should be a number:
- return result.status === 'fulfilled' && result.value.riskScore === NOT_IN_BLOCKCHAIN_RISK_SCORE.toString();
+ return result.status === 'fulfilled' && Number(result.value.riskScore) === NOT_IN_BLOCKCHAIN_RISK_SCORE;Ensure that riskScore and NOT_IN_BLOCKCHAIN_RISK_SCORE are of the same type during the comparison.
Committable suggestion was skipped due to low confidence.
| @@ -70,6 +73,14 @@ export async function findAll( | |||
| baseQuery = baseQuery.where(ComplianceDataColumns.blocked, blocked); | |||
| } | |||
|
|
|||
| if (addressInWalletsTable === true) { | |||
| baseQuery = baseQuery.innerJoin( | |||
There was a problem hiding this comment.
nice I've never seen this done before!
| @@ -318,10 +319,18 @@ async function getComplianceData( | |||
| return result.value; | |||
| }, | |||
| )); | |||
| const responses404: | |||
There was a problem hiding this comment.
nit: best not to include numbers in variable names, notFoundResponses
| PromiseFulfilledResult<ComplianceClientResponse>[] = successResponses.filter( | ||
| (result: PromiseSettledResult<ComplianceClientResponse>): | ||
| result is PromiseFulfilledResult<ComplianceClientResponse> => { | ||
| return result.status === 'fulfilled' && result.value.riskScore === NOT_IN_BLOCKCHAIN_RISK_SCORE.toString(); |
There was a problem hiding this comment.
this logic is kinda messy, up to you, but would it be worthwhile to further populate ComplianceClientResponse? We already support return an undefined riskScore, might be cleaner to add a status field and have that encompass all of the states of blocked, NOT_IN_BLOCKCHAIN, and NO_RULES_TRIGGERED
There was a problem hiding this comment.
Yes i was actually thinking of adding a new field to CompliacenClientResponse to track the states. Didn't like hacking the state by conditioning on riskScore
There was a problem hiding this comment.
Looked further into it. Modifying ComplianceClientResponse has ripple effects into all other providers. We can leave the new field nullable. At minimum the mocks for elliptic provider need to be changed. Will just stick to conditioning on riskScore and add comment explaining.
jerryfan01234
force-pushed
the
compliance-roundtable-address-filter
branch
from
6f8c101
to
ad2777d
Compare
jerryfan01234
changed the title
|
@Mergifyio backport release/indexer/v6.x |
✅ Backports have been created
|
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
Changelist
Restrict comlink compliance screen from upserting when address is not a dydx wallet with deposit
Restrict compliance check during compliance update roundtable to dydx wallet with depoit + add logging
Test Plan
[Describe how this PR was tested (if applicable)]
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
Documentation