Add schema checks for addresses in url endpoints #2692
Conversation
Signed-off-by: Shrenuj Bansal <[email protected]>
WalkthroughThe pull request introduces enhancements to address validation and testing within the dYdX indexer services. Key changes include updating the Changes
Suggested Labels
Poem
Finishing Touches
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: 0
🧹 Nitpick comments (1)
indexer/services/comlink/src/lib/validation/schemas.ts (1)
291-296: Consider removing the debug log statement.The implementation is correct, but the console.log statement should be removed or replaced with proper debug logging.
export function isValidAddress(address: string): boolean { // An address is valid if it starts with `dydx1` and is Bech32 format. const ret: boolean = address.startsWith('dydx1') && (verifyIsBech32(address) === undefined); - console.log('is valid address: ', ret); return ret; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
indexer/packages/postgres/__tests__/helpers/constants.ts(1 hunks)indexer/services/comlink/__tests__/controllers/api/v4/addresses-controller.test.ts(5 hunks)indexer/services/comlink/package.json(1 hunks)indexer/services/comlink/src/lib/validation/schemas.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- 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: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: check-build-auxo
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (7)
indexer/services/comlink/src/lib/validation/schemas.ts (2)
281-289: LGTM! Clean implementation of Bech32 validation.The
verifyIsBech32helper function provides a clear and focused way to validate Bech32 addresses using the bech32 library.
18-21: LGTM! Consistent validation across schemas.The address validation is consistently applied across different schemas with clear error messages.
Also applies to: 36-39, 54-57
indexer/packages/postgres/__tests__/helpers/constants.ts (1)
81-81: LGTM! Updated test constant to use a valid dydx address.The
defaultWalletAddressnow uses a valid Bech32 address that matches the new validation requirements.indexer/services/comlink/__tests__/controllers/api/v4/addresses-controller.test.ts (3)
215-225: LGTM! Updated test to use a valid address for 404 case.The test now uses
defaultAddress3(a valid address) to properly test the 404 case when a subaccount is not found.
409-419: LGTM! Improved error handling for invalid addresses.The test now correctly expects a 400 status code with detailed error information for invalid addresses.
739-742: LGTM! Consistent error handling in token registration.The error response format for invalid addresses is consistent with other endpoints.
indexer/services/comlink/package.json (1)
38-38: LGTM! Added required bech32 dependency.The bech32 library is properly added with a pinned version for address validation.
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
indexer/services/comlink/src/lib/validation/schemas.ts (1)
291-294: Consider optimizing the validation order.The implementation is correct, but consider checking the prefix first before attempting Bech32 decoding to fail fast and avoid unnecessary computation.
- return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined); + return address.startsWith('dydx1') ? verifyIsBech32(address) === undefined : false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
indexer/services/comlink/__tests__/controllers/api/v4/addresses-controller.test.ts(4 hunks)indexer/services/comlink/src/controllers/api/v4/compliance-controller.ts(1 hunks)indexer/services/comlink/src/lib/validation/schemas.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- indexer/services/comlink/src/controllers/api/v4/compliance-controller.ts
⏰ Context from checks skipped due to timeout of 90000ms (13)
- 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: 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-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: check-build-auxo
- GitHub Check: test / run_command
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (4)
indexer/services/comlink/src/lib/validation/schemas.ts (3)
18-21: LGTM! Consistent validation across schemas.The validation schemas have been updated to use the new address validation function with consistent error messages.
Also applies to: 36-39, 54-57
281-289: LGTM! Clean implementation of Bech32 validation.The function provides a clean wrapper around the bech32 library's decode function with proper error handling.
296-299: 🛠️ Refactor suggestionReview the address validation strategy.
The current implementation has several concerns:
- The function name
isValidAddresssuggests complete address validation, but it only performs basic character checks.- It doesn't utilize the Bech32 validation or dYdX address prefix check.
- The 90-character limit seems arbitrary and might need documentation.
Consider either:
- Using
isValidDydxAddressfor complete validation, or- Renaming this function to better reflect its purpose (e.g.,
isValidAddressFormat).indexer/services/comlink/__tests__/controllers/api/v4/addresses-controller.test.ts (1)
215-223: LGTM! Improved test consistency.Good refactoring to use the
defaultAddress3constant instead of the hardcodedinvalidAddressstring. This improves maintainability and consistency across tests.Also applies to: 560-570
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
indexer/services/comlink/src/lib/validation/schemas.ts (1)
294-297: Optimize isValidAddress implementation.Consider these improvements:
- Add input validation
- Precompile the regex pattern for better performance
+const ADDRESS_PATTERN = /^[a-zA-Z0-9]*$/; + export function isValidAddress(address: string): boolean { + if (!address || typeof address !== 'string') { + return false; + } // Address is valid if its under 90 characters and alphanumeric - return address.length <= 90 && /^[a-zA-Z0-9]*$/.test(address); + return address.length <= 90 && ADDRESS_PATTERN.test(address); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
indexer/services/comlink/__tests__/controllers/api/v4/historical-pnl-controller.test.ts(1 hunks)indexer/services/comlink/__tests__/controllers/api/v4/transfers-controller.test.ts(2 hunks)indexer/services/comlink/src/lib/validation/schemas.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- indexer/services/comlink/tests/controllers/api/v4/transfers-controller.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- 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: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: check-build-auxo
- GitHub Check: test / run_command
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (2)
indexer/services/comlink/__tests__/controllers/api/v4/historical-pnl-controller.test.ts (1)
267-267: Consider using a standardized invalid address constant.The test currently uses a hardcoded invalid address string. For consistency across tests and to align with the bech32 schema validation changes, consider using a shared constant from
testConstantsfor invalid addresses.Let's verify if there's a standardized invalid address constant already defined:
Also applies to: 274-274
indexer/services/comlink/src/lib/validation/schemas.ts (1)
7-7: LGTM: Import of bech32 decoder.The addition of the bech32 decoder import aligns with the PR objective of implementing address validation.
| custom: { | ||
| options: isValidAddress, | ||
| }, | ||
| errorMessage: 'address must be a valid dydx address', |
There was a problem hiding this comment.
Inconsistency between error message and validation logic.
The error message states "address must be a valid dydx address", but isValidAddress only checks for alphanumeric characters and length ≤ 90. Consider either:
- Using
isValidDydxAddressinstead ofisValidAddressto match the error message, or - Updating the error message to accurately reflect the current validation criteria
Example fix:
address: {
in: ['params', 'query'],
isString: true,
custom: {
- options: isValidAddress,
+ options: isValidDydxAddress,
},
errorMessage: 'address must be a valid dydx address',
},Or:
address: {
in: ['params', 'query'],
isString: true,
custom: {
options: isValidAddress,
},
- errorMessage: 'address must be a valid dydx address',
+ errorMessage: 'address must be alphanumeric and not exceed 90 characters',
},Also applies to: 34-37, 52-55
💡 Codebase verification
Review comment is correct - schemas should use stricter validation
The codebase confirms that while isValidDydxAddress is properly implemented to enforce bech32 validation (requiring 'dydx1' prefix and valid bech32 format), the schemas are still using the more lenient isValidAddress function that only checks for length and alphanumeric characters.
- The validation is used across multiple controllers including:
- addresses-controller.ts
- transfers-controller.ts
- orders-controller.ts
- fills-controller.ts
- perpetual-positions-controller.ts
Switching to isValidDydxAddress would properly enforce the PR's objective of adding bech32 schema checks.
🔗 Analysis chain
Align implementation with PR objectives.
The PR's objective is to add bech32 schema checks for addresses, but the current implementation uses a more lenient validation (isValidAddress) in the schemas instead of the bech32-specific validation (isValidDydxAddress). This doesn't fully achieve the PR's goal of ensuring addresses are valid dydx addresses.
Consider using isValidDydxAddress in all schemas to enforce proper bech32 validation as intended by the PR.
Let's verify the impact of this change:
Also applies to: 34-37, 52-55, 289-292
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all files that might be affected by this change
echo "Files potentially using these schemas:"
rg -l "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" --type ts
echo -e "\nEndpoints using these schemas:"
rg -A 2 "\.use\((Check(Subaccount|ParentSubaccount|Address)Schema)" --type ts
Length of output: 1108
Script:
#!/bin/bash
# Find implementation of validation functions
echo "=== Validation function implementations ==="
ast-grep --pattern 'function isValidAddress'
ast-grep --pattern 'function isValidDydxAddress'
echo -e "\n=== Usage in controllers ==="
# Look for schema usage in controllers with context
rg "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" "indexer/services/comlink/src/controllers/api/v4/*.ts" -B 1 -A 1
echo -e "\n=== Test coverage ==="
# Check test coverage for these validations
rg "isValidAddress|isValidDydxAddress" "indexer/services/comlink/__tests__/" -A 2
Length of output: 642
Script:
#!/bin/bash
# Find validation function implementations with broader patterns
echo "=== Validation implementations ==="
rg "isValidAddress|isValidDydxAddress" "indexer/services/comlink/src/" -A 2
echo -e "\n=== Exports and imports ==="
rg "export.*isValid|import.*isValid" "indexer/services/comlink/src/" -A 1
echo -e "\n=== Schema usage in controllers ==="
# Fixed controller path
rg "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" "indexer/services/comlink/src/controllers/" -B 1 -A 1
echo -e "\n=== Schema definitions ==="
rg "CheckSubaccountSchema|CheckParentSubaccountSchema|CheckAddressSchema" "indexer/services/comlink/src/lib/validation/" -B 1 -A 3
Length of output: 13609
| export function isValidDydxAddress(address: string): boolean { | ||
| // An address is valid if it starts with `dydx1` and is Bech32 format. | ||
| return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to isValidDydxAddress.
The function should validate its input and handle edge cases.
export function isValidDydxAddress(address: string): boolean {
+ if (!address || typeof address !== 'string') {
+ return false;
+ }
// An address is valid if it starts with `dydx1` and is Bech32 format.
return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined);
}📝 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.
| export function isValidDydxAddress(address: string): boolean { | |
| // An address is valid if it starts with `dydx1` and is Bech32 format. | |
| return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined); | |
| } | |
| export function isValidDydxAddress(address: string): boolean { | |
| if (!address || typeof address !== 'string') { | |
| return false; | |
| } | |
| // An address is valid if it starts with `dydx1` and is Bech32 format. | |
| return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined); | |
| } |
| function verifyIsBech32(address: string): Error | undefined { | ||
| try { | ||
| decode(address); | ||
| } catch (error) { | ||
| return error; | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to verifyIsBech32.
The function should validate its input before attempting to decode.
function verifyIsBech32(address: string): Error | undefined {
+ if (!address || typeof address !== 'string') {
+ return new Error('Invalid input: address must be a non-empty string');
+ }
try {
decode(address);
} catch (error) {
- return error;
+ return error instanceof Error ? error : new Error('Invalid bech32 format');
}
return undefined;
}📝 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.
| function verifyIsBech32(address: string): Error | undefined { | |
| try { | |
| decode(address); | |
| } catch (error) { | |
| return error; | |
| } | |
| return undefined; | |
| } | |
| function verifyIsBech32(address: string): Error | undefined { | |
| if (!address || typeof address !== 'string') { | |
| return new Error('Invalid input: address must be a non-empty string'); | |
| } | |
| try { | |
| decode(address); | |
| } catch (error) { | |
| return error instanceof Error ? error : new Error('Invalid bech32 format'); | |
| } | |
| return undefined; | |
| } |
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
indexer/services/comlink/src/lib/validation/schemas.ts (3)
16-19:⚠️ Potential issueSchema validation doesn't enforce bech32 format.
The schemas use
isValidAddresswhich only performs basic alphanumeric validation, but the error messages suggest bech32 validation. This doesn't fulfill the PR's objective of adding bech32 schema checks for addresses.Apply this change to all three schema definitions:
custom: { - options: isValidAddress, + options: isValidDydxAddress, },Also applies to: 34-37, 52-55
279-287: 🛠️ Refactor suggestionEnhance error handling in verifyIsBech32.
The function needs input validation and proper error handling to be more robust.
Apply this improvement:
function verifyIsBech32(address: string): Error | undefined { + if (!address || typeof address !== 'string') { + return new Error('Invalid input: address must be a non-empty string'); + } try { decode(address); } catch (error) { - return error; + return error instanceof Error ? error : new Error('Invalid bech32 format'); } return undefined; }
289-292: 🛠️ Refactor suggestionAdd input validation to isValidDydxAddress.
The function should validate its input before performing checks.
Apply this improvement:
export function isValidDydxAddress(address: string): boolean { + if (!address || typeof address !== 'string') { + return false; + } // An address is valid if it starts with `dydx1` and is Bech32 format. return address.startsWith('dydx1') && (verifyIsBech32(address) === undefined); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
indexer/services/comlink/__tests__/lib/validation/schemas.test.ts(1 hunks)indexer/services/comlink/src/lib/validation/schemas.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- 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: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: check-build-auxo
- GitHub Check: test / run_command
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
indexer/services/comlink/src/lib/validation/schemas.ts (1)
294-297: 🛠️ Refactor suggestionConsider deprecating isValidAddress in favor of isValidDydxAddress.
- The lenient validation in
isValidAddressdoesn't align with the PR's objective of enforcing bech32 validation.- The function lacks input validation.
If keeping this function, add input validation:
export function isValidAddress(address: string): boolean { + if (!address || typeof address !== 'string') { + return false; + } // Address is valid if its under 90 characters and alphanumeric return address.length <= 90 && /^[a-zA-Z0-9]*$/.test(address); }However, consider deprecating this function and using
isValidDydxAddressconsistently throughout the codebase to enforce bech32 validation.Likely invalid or redundant comment.
| [ | ||
| 'missingaddress', | ||
| { subaccountNumber: defaultSubaccountNumber }, | ||
| 'address', | ||
| 'address must be a valid dydx address', | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for address validation.
The test's error message suggests bech32 validation ("must be a valid dydx address"), but it only tests for a missing address. Additional test cases are needed to verify bech32 validation once the schemas are updated to use isValidDydxAddress.
Add these test cases:
[
'invalid bech32 format',
{ address: 'invalid123', subaccountNumber: defaultSubaccountNumber },
'address',
'address must be a valid dydx address',
],
[
'wrong prefix',
{ address: 'cosmos1abc...', subaccountNumber: defaultSubaccountNumber },
'address',
'address must be a valid dydx address',
],
[
'valid dydx address',
{ address: 'dydx1...', subaccountNumber: defaultSubaccountNumber },
undefined,
undefined,
],
shrenujb
changed the title
Changelist
Add additional schema checks to ensure the addresses in url endpoints are actual dydx addresses
Test Plan
Existing modified tests
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
Release Notes
New Features
Dependencies
bech32library for address encoding and decodingBug Fixes
Chores