Use Skip API key with tx status endpoints #3849
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe changes introduce a new interface, Changes
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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: 2
Outside diff range and nitpick comments (5)
packages/web/pages/api/skip-track-tx.ts (2)
4-6: Nitpick: Improve the code comment.The code comment could be more descriptive and provide additional context. Consider expanding it to explain why the API key is required and how this edge function enables the usage of the credentialed
SkipApiClienton the server.Here's a suggested improvement:
-/** This edge function is necessary to invoke the SkipApiClient on the server - * as a secret API key is required for the client. - */ +/** + * This edge function is necessary to invoke the SkipApiClient on the server + * because a secret API key is required for the client to authenticate with + * the Skip API. By implementing this as an edge function, we can securely + * use the credentialed SkipApiClient on the server without exposing the + * API key to the client. + */
17-19: Consider using a more specific error message for missing query parameters.Instead of a generic "Missing required query parameters" error message, consider providing a more specific message that indicates which query parameter(s) are missing. This can help with debugging and provide more context to the client.
Here's a suggested improvement:
-if (!chainID || !txHash || !env) { - return res.status(400).json({ error: "Missing required query parameters" }); -} +const missingParams = []; +if (!chainID) missingParams.push("chainID"); +if (!txHash) missingParams.push("txHash"); +if (!env) missingParams.push("env"); + +if (missingParams.length > 0) { + return res.status(400).json({ + error: `Missing required query parameter(s): ${missingParams.join(", ")}`, + }); +}packages/web/pages/api/skip-tx-status.ts (2)
4-6: Enhance the documentation.The comment provides a good explanation of the purpose of the edge function. Consider adding more details about the required query parameters and the expected response format to make it more comprehensive.
Apply this diff to enhance the documentation:
-/** This edge function is necessary to invoke the SkipApiClient on the server - * as a secret API key is required for the client. - */ +/** + * This edge function is necessary to invoke the SkipApiClient on the server + * as a secret API key is required for the client. + * + * Required query parameters: + * - chainID: The ID of the chain. + * - txHash: The hash of the transaction. + * - env: The bridge environment (mainnet, testnet, or devnet). + * + * Response format: + * - 200 OK: Returns the transaction status as a JSON object. + * - 400 Bad Request: Returns an error message if required query parameters are missing. + * - 500 Internal Server Error: Returns an error message if an exception occurs during processing. + */
1-32: Consider adding rate limiting and authentication.To enhance security and prevent abuse, consider implementing rate limiting and authentication for this API endpoint. This can help protect against excessive requests and ensure only authorized clients can access the transaction status.
packages/bridge/src/skip/client.ts (1)
Line range hint
1-140: Consider adding unit tests.While the changes look good, it would be beneficial to add unit tests to cover the new
apiKeyproperty and the updated authorization logic. This will help ensure that the API key is being correctly set and used for authentication in the API client.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/bridge/src/skip/client.ts (2 hunks)
- packages/bridge/src/skip/index.ts (1 hunks)
- packages/bridge/src/skip/transfer-status.ts (3 hunks)
- packages/web/pages/api/skip-track-tx.ts (1 hunks)
- packages/web/pages/api/skip-tx-status.ts (1 hunks)
- packages/web/stores/root.ts (1 hunks)
Additional comments not posted (20)
packages/web/pages/api/skip-track-tx.ts (2)
1-32: LGTM!The new API endpoint for tracking transactions using the
SkipApiClientis implemented correctly. It follows best practices for error handling, input validation, and response status codes. The code is clean, readable, and well-documented.
26-31: Verify error handling in the codebase.The error handling for the
trackTransactioncall looks good. However, it's important to ensure that all potential errors thrown by theSkipApiClientare properly caught and handled in the codebase.Run the following script to verify the error handling:
packages/web/pages/api/skip-tx-status.ts (5)
1-2: LGTM!The imports are correctly specified and follow the necessary conventions.
7-15: LGTM!The function signature and parameter typing are correctly defined.
17-19: LGTM!The validation of required query parameters is implemented correctly, and the appropriate error response is returned if any parameter is missing.
23-31: LGTM!The error handling is implemented correctly. The function attempts to retrieve the transaction status and returns the appropriate response based on the result or any encountered errors.
21-21: Verify the API key is securely stored and accessed.Ensure that the API key used to initialize the
SkipApiClientis securely stored (e.g., in environment variables) and not hardcoded in the codebase.Run the following script to verify the secure storage and access of the API key:
Verification successful
API key for SkipApiClient is securely stored and accessed
The verification process confirms that the API key for SkipApiClient is securely handled:
- The API key is not hardcoded in the codebase.
- It is accessed from the environment variable
SKIP_API_KEY.- There's a check to ensure the API key is set before using it.
- SkipApiClient is initialized with an
envparameter, likely containing the environment variables.These practices align with security best practices for handling sensitive information like API keys.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the API key is not hardcoded in the codebase. # Test 1: Search for hardcoded API keys. Expect: No results. rg --type typescript $'SkipApiClient\([^)]*[\'"][A-Za-z0-9]{32,}[\'"]' # Test 2: Verify the API key is accessed from environment variables. # Expect: Usage of `process.env` to access the API key. ast-grep --lang typescript --pattern $'new SkipApiClient($_) { $$$ this.apiKey = process.env.$_ $$$ }'Length of output: 711
Script:
#!/bin/bash # Search for potential hardcoded API keys in SkipApiClient initialization rg --type-add 'ts:*.{ts,tsx}' --type ts 'new SkipApiClient\([^)]*["\'][A-Za-z0-9]{32,}["\']' # Find SkipApiClient initializations ast-grep --lang typescript --pattern 'new SkipApiClient($_)' # Search for environment variable usage related to API keys rg --type-add 'ts:*.{ts,tsx}' --type ts 'process\.env\.[A-Z_]+_API_KEY' # Look for SkipApiClient class definition to understand its structure ast-grep --lang typescript --pattern 'class SkipApiClient { $$$ }'Length of output: 93
Script:
#!/bin/bash # Search for SkipApiClient initializations rg --type-add 'ts:*.{ts,tsx}' --type ts 'new SkipApiClient' # Search for potential API key usage rg --type-add 'ts:*.{ts,tsx}' --type ts 'API_KEY|apiKey' # Find SkipApiClient initializations using ast-grep ast-grep --lang typescript --pattern 'new SkipApiClient($_)'Length of output: 2701
packages/bridge/src/skip/client.ts (2)
17-17: LGTM!The addition of the
apiKeyproperty is a good refactor that encapsulates the API key within the class. This change simplifies the code by reducing the direct dependency on the environment variable throughout the class methods.
126-126: Verify that theapiKeyproperty is being used consistently.The logic for checking the presence of the API key has been updated to reference the new
apiKeyproperty. This is the correct approach.To ensure that the
apiKeyproperty is being used consistently throughout the codebase, run the following script:Verification successful
The
apiKeyproperty is being used consistently as intended.The verification process has confirmed that:
- The
apiKeyproperty is correctly declared usingprocess.env.SKIP_API_KEY.- There is only one instance of direct access to
process.env.SKIP_API_KEY, which is in the property declaration.- The
this.apiKeyis being used as expected in the code (line 126).- There are no other instances of direct access to
process.env.SKIP_API_KEYin the codebase.These findings indicate that the changes have been implemented correctly, and the
apiKeyproperty is being used consistently throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `apiKey` property is being used instead of directly accessing the environment variable. # Test 1: Search for direct access to `process.env.SKIP_API_KEY`. Expect: No occurrences found (except for the property declaration). rg --type typescript $'process\.env\.SKIP_API_KEY' -g '!protected readonly apiKey' # Test 2: Search for usage of the `apiKey` property. Expect: At least one occurrence found. rg --type typescript $'this\.apiKey'Length of output: 191
Script:
#!/bin/bash # Description: Verify that the `apiKey` property is being used instead of directly accessing the environment variable. # Test 1: Search for direct access to `process.env.SKIP_API_KEY`. Expect: Only the property declaration. ast-grep --lang javascript --pattern 'process.env.SKIP_API_KEY' # Test 2: Search for usage of the `apiKey` property. Expect: At least one occurrence found. ast-grep --lang javascript --pattern 'this.apiKey' # Test 3: Search for the property declaration to ensure it exists. ast-grep --lang javascript --pattern 'protected readonly apiKey = process.env.SKIP_API_KEY'Length of output: 208
Script:
#!/bin/bash # Description: Verify that the `apiKey` property is being used instead of directly accessing the environment variable. # Find TypeScript files and search for patterns echo "Searching for direct access to process.env.SKIP_API_KEY:" fd -e ts -e tsx | xargs -I {} ast-grep --lang typescript --pattern 'process.env.SKIP_API_KEY' {} echo "Searching for usage of this.apiKey:" fd -e ts -e tsx | xargs -I {} ast-grep --lang typescript --pattern 'this.apiKey' {} echo "Searching for the apiKey property declaration:" fd -e ts -e tsx | xargs -I {} ast-grep --lang typescript --pattern 'protected readonly apiKey = process.env.SKIP_API_KEY' {} # Fallback to rg if ast-grep doesn't yield results echo "Fallback: Searching with rg for process.env.SKIP_API_KEY:" rg 'process\.env\.SKIP_API_KEY' echo "Fallback: Searching with rg for this.apiKey:" rg 'this\.apiKey'Length of output: 1225
packages/bridge/src/skip/transfer-status.ts (9)
13-14: LGTM!The import statement for
SkipTxStatusResponseis correctly added.
15-20: LGTM!The
Transactiontype is well-defined with the necessary properties.
21-32: LGTM!The
SkipStatusProviderinterface is properly defined with the required methods:
transactionStatus: Retrieves the status of a transaction.trackTransaction: Prompts Skip to track a transaction.This interface allows for better separation of concerns and modularity in the codebase.
43-47: LGTM!The constructor of the
SkipTransferStatusProviderclass is updated to accept an instance ofSkipStatusProvideras a dependency. This change enables dependency injection and improves testability.
63-67: LGTM!The
txobject is correctly constructed with the necessary properties from theGetTransferStatusParams.
69-79: LGTM!The transaction status is now fetched using the
transactionStatusmethod from the injectedSkipStatusProvider. If a "not found" error occurs, it attempts to track the transaction first using thetrackTransactionmethod before retrying the status check. This change improves the error handling and tracking capabilities.
82-85: LGTM!The logic for determining the transfer status based on the
txStatus.stateis correctly implemented. The status is set to:
- "success" when the state is "STATE_COMPLETED_SUCCESS".
- "failed" when the state is "STATE_COMPLETED_ERROR".
- "pending" for all other cases.
Also applies to: 87-88, 90-90
91-94: LGTM!The
idandstatusproperties are correctly returned from the polling function.
Line range hint
1-157: Verify the usage of the newSkipStatusProviderinterface.The changes in this file look good overall. However, it's important to ensure that the new
SkipStatusProviderinterface is properly implemented and used throughout the codebase.Please run the following script to verify the usage of the
SkipStatusProviderinterface:If the tests pass, it confirms that the
SkipStatusProviderinterface is properly integrated into the codebase. If any test fails, please address the inconsistencies before merging this PR.Verification successful
The
SkipStatusProviderinterface is correctly implemented and used.The verification process confirms that the
SkipStatusProviderinterface is properly integrated into the codebase:
- The
SkipTransferStatusProviderclass correctly implements theTransferStatusProviderinterface.- The constructor accepts a
skipStatusProviderparameter of typeSkipStatusProvider.- Both
transactionStatusandtrackTransactionmethods from theSkipStatusProviderinterface are used correctly within the class.No further changes are required for this implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `SkipStatusProvider` interface. # Test 1: Search for the interface declaration. # Expect: The interface should be declared in this file. ast-grep --lang typescript --pattern $'type SkipStatusProvider = { $$$ }' # Test 2: Search for the interface usage in the constructor. # Expect: The constructor should accept an instance of `SkipStatusProvider`. ast-grep --lang typescript --pattern $'constructor($$$, protected readonly skipStatusProvider: SkipStatusProvider)' # Test 3: Search for the usage of `transactionStatus` method. # Expect: The method should be called on the `skipStatusProvider` instance. rg --type typescript -A 5 $'this.skipStatusProvider.transactionStatus' # Test 4: Search for the usage of `trackTransaction` method. # Expect: The method should be called on the `skipStatusProvider` instance. rg --type typescript -A 5 $'this.skipStatusProvider.trackTransaction'Length of output: 1216
Script:
#!/bin/bash # Description: Verify the usage of the `SkipStatusProvider` interface. # Test 1: Search for the constructor usage ast-grep --lang typescript --pattern 'constructor($$$) { $$$ }' # Test 2: Search for the usage of `transactionStatus` method ast-grep --lang typescript --pattern 'this.skipStatusProvider.transactionStatus($$$)' # Test 3: Search for the usage of `trackTransaction` method ast-grep --lang typescript --pattern 'this.skipStatusProvider.trackTransaction($$$)' # Test 4: Search for the class declaration to ensure it's implementing the interface ast-grep --lang typescript --pattern 'export class SkipTransferStatusProvider implements TransferStatusProvider {'Length of output: 10140
packages/web/stores/root.ts (1)
254-265: Verify error handling for failed transaction status fetch.The
transactionStatusmethod correctly checks for non-OK responses and throws an error with theresponseJson.errordetails.However, it's worth verifying what happens to this error at the call-sites of
transactionStatus. Ensure it is caught and handled appropriately, providing a good UX.You can use the following script to check how the errors thrown by
transactionStatusare handled:If there are no
catchortryblocks around the calls totransactionStatus, consider adding them to prevent the errors from propagating and crashing the application.Verification successful
Error handling for
transactionStatusis properly implemented.The verification process has confirmed that while the
transactionStatusmethod itself doesn't include error handling (as per its implementation), the calling code appropriately handles potential errors:
In
packages/bridge/src/skip/transfer-status.ts:
- Errors are caught and checked for a "not found" message.
- If "not found", it attempts to track the transaction and retry.
- Other errors are re-thrown for higher-level handling.
In
packages/web/pages/api/skip-tx-status.ts:
transactionStatusis called within a try-catch block.- Caught errors are properly handled, returning appropriate HTTP 500 responses with error details.
This implementation ensures that errors from
transactionStatusare caught and handled appropriately, providing a good user experience as suggested in the review comment.Scripts executed
The following scripts were executed for the analysis:
Script:
# Check if the error thrown by transactionStatus is caught and handled rg --type typescript $'transactionStatus\(' -A 10 | rg $'catch|try'Length of output: 102
Script:
#!/bin/bash # Find TypeScript files containing 'transactionStatus(' files=$(fd -e ts -e tsx | xargs rg --files-with-matches 'transactionStatus\(') # For each file, search for 'transactionStatus(' and check for nearby error handling for file in $files; do echo "File: $file" rg 'transactionStatus\(' -C 10 "$file" | rg --color never -e 'transactionStatus\(' -e 'try\s*{' -e 'catch' -e '.catch\(' echo "---" doneLength of output: 1529
Script:
#!/bin/bash files=( "packages/bridge/src/skip/client.ts" "packages/bridge/src/skip/transfer-status.ts" "packages/web/pages/api/skip-tx-status.ts" ) for file in "${files[@]}"; do echo "File: $file" echo "---" rg 'transactionStatus\(' -C 10 "$file" echo "---" echo "Error handling patterns:" rg 'try|catch|\.catch\(' -C 5 "$file" echo "==========" doneLength of output: 4427
packages/bridge/src/skip/index.ts (1)
939-939: LGTM!The new export statement re-exports all the exports from the
clientmodule, making them accessible from the current module. This change enhances the module's interface by providing a single entry point for accessing the functionality related to the Skip client.
| // if we get an error that it's not found, prompt skip to track it first | ||
| // then try again | ||
| await this.skipStatusProvider.trackTransaction(tx); | ||
| return this.skipStatusProvider.transactionStatus(tx); |
There was a problem hiding this comment.
Wouldn't this call short-circuit the poll if it fails since this is returning a promise without a catch statement?
There was a problem hiding this comment.
WDYM without a catch statement? I believe what would happen here is it would be transformed into the same promise that is awaited at the initial call above.
There was a problem hiding this comment.
It wouldn’t, since this isn't a recursive call and it's using the dependency-injected method. If we want to share the catch statement, we’d likely need to create an inline function and call it inside the catch method
There was a problem hiding this comment.
I'm not sure I follow, how would this look?
There was a problem hiding this comment.
Actually, I think we could avoid recursion by just adding a catch. It will just call the method on the next poll cycle
| return this.skipStatusProvider.transactionStatus(tx); | |
| return this.skipStatusProvider.transactionStatus(tx).catch(() => {}); |
There was a problem hiding this comment.
I don't think recursion is being used, we're just calling the same given function twice
What is the purpose of the change:
Linear Task
This PR refactors the Skip TransferStatusProvider to dep-inject the tx status and track endpoints so that we can create serverless functions that utilize the credentialed SkipApiClient on the server.
Also, I refactored the trackTransaction POST call to skip to be within a catch clause instead of a try catch statement. This makes it slightly more readable as there's less jumping around when interpreting the logic related to prompting Skip to track transactions before continuing the polling.
Brief Changelog
Testing and Verifying
Tested locally with Skip transfers in the deposit/withdraw flow, verifying that conclusive statuses can be received from Skip.