[TASK-6263] feat: improve requests usability #157
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce a new environment variable for the Peanut API in the Changes
Possibly related PRs
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (3)
.env.example (1)
20-20: LGTM with suggestions for improvementThe addition of the
PEANUT_API_URLvariable is appropriate and aligns with the PR objectives. However, consider the following suggestions:
- Instead of hardcoding the staging URL, consider using a placeholder value (e.g.,
export PEANUT_API_URL="") to allow for environment-specific configurations.- Add a comment above this line to explain the purpose of this URL and its usage in the application.
Here's a suggested modification:
+# URL for the Peanut API (e.g., https://api.staging.peanut.to for staging) -export PEANUT_API_URL="https://api.staging.peanut.to" +export PEANUT_API_URL=""src/request.ts (1)
181-183: Consider improving error handling for missing API keys.While the changes look good, I noticed that the
createRequestLinkandgetRequestLinkDetailsfunctions also use anAPIKeyparameter. To improve robustness, consider adding explicit error handling for cases where the API key is required but not provided.Here's a suggestion for
getRequestLinkDetails(apply similar logic to other functions):export async function getRequestLinkDetails({ link, APIKey, apiUrl = 'https://api.peanut.to/', }: IGetRequestLinkDetailsProps): Promise<IGetRequestLinkDetailsResponse> { if (!APIKey) { throw new Error('API key is required for this operation'); } // ... rest of the function }This change would provide clearer feedback when the API key is missing, improving the developer experience.
test/basic/RequestLinkXChain.test.ts (1)
9-19: Enhance Error Logging in theretryFunctionThe
retryfunction is a useful utility for handling intermittent failures. To improve debugging, consider adding more informative error messages or logging within the retry mechanism. This will help identify the cause of failures when all retry attempts have been exhausted.You might modify the catch block to log the error:
try { await assertion() return } catch (err) { if (i === times - 1) throw err + console.warn(`Assertion failed on attempt ${i + 1}:`, err) await new Promise((resolve) => setTimeout(resolve, interval)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .env.example (1 hunks)
- src/request.ts (2 hunks)
- test/basic/RequestLinkXChain.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/request.ts (3)
181-183: LGTM: Function updated to use API key.The
prepareXchainRequestFulfillmentTransactionfunction has been correctly updated to accept and utilize theAPIKey. The changes are consistent with the interface update and implement the PR objective.
Line range hint
1-383: Summary: Changes align with PR objectives, minor improvement suggested.The modifications to
src/request.tssuccessfully implement the PR objective of updating theprepareXchainRequestFulfillmentTransactionfunction to accept and utilize an API key. The changes are well-implemented and maintain consistency across the file.A minor suggestion for improvement has been made regarding error handling for missing API keys, which would enhance the robustness of the SDK.
Overall, the changes look good and are ready for merging after addressing the suggested improvement.
49-49: LGTM: Interface update for API key.The addition of the optional
APIKeyproperty to theIPrepareXchainRequestFulfillmentTransactionPropsinterface is a good change. It aligns with the PR objective and maintains backward compatibility.To ensure consistency, let's verify if this interface is used elsewhere in the codebase:
✅ Verification successful
Verification Complete: APIKey Addition Approved.
The addition of the optional
APIKeyproperty to theIPrepareXchainRequestFulfillmentTransactionPropsinterface is safe and does not impact existing implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uses of IPrepareXchainRequestFulfillmentTransactionProps rg --type typescript "IPrepareXchainRequestFulfillmentTransactionProps"Length of output: 106
Script:
#!/bin/bash # Search for uses of IPrepareXchainRequestFulfillmentTransactionProps with correct file type rg --type ts "IPrepareXchainRequestFulfillmentTransactionProps"Length of output: 386
test/basic/RequestLinkXChain.test.ts (2)
115-126: Handle Missing API Key and API URL in Function CallIn the call to
prepareXchainRequestFulfillmentTransaction, theAPIKeyandapiUrlare provided. Ensure that the underlying function properly handles cases where these parameters might beundefinedor invalid, to prevent unexpected errors.Verify that the function gracefully handles missing or incorrect API credentials.
If needed, I can help review the
prepareXchainRequestFulfillmentTransactionfunction to ensure robust error handling. Let me know if you'd like me to assist with this.
88-88: Ensure Unique Sender and Recipient WalletsThe sender and recipient wallets are derived from environment variables. To accurately test the transfer functionality, confirm that these wallets are not the same. Using the same wallet for both roles could mask issues in the transfer logic.
Run the following script to verify that the sender and recipient addresses are different:
This script checks that
TEST_WALLET_PRIVATE_KEYandTEST_WALLET_PRIVATE_KEY2correspond to different addresses.
jjramirezn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (16)
test/unit/util.test.ts (1)
5-12: Comprehensive test cases with room for expansionThe test cases cover a good range of scenarios for URL normalization, including trailing slashes, double slashes, and leading slashes. This is a solid foundation for testing the
normalizePathfunction.Consider adding a few more test cases to further improve coverage:
- A URL with query parameters (e.g., 'https://example.com/foo?bar=baz')
- A URL with a hash fragment (e.g., 'https://example.com/foo#section1')
- A URL with both query parameters and a hash fragment
- A URL with unusual characters that might need encoding
test/unit/request.test.ts (4)
14-35: LGTM: Well-structured test forcreateRequestLinkThe test case for
createRequestLinkis well-implemented and verifies the correct URL construction when the API URL is not provided.Consider adding additional test cases to cover:
- A scenario where the API URL is provided.
- Error handling for failed API responses.
- Validation of the request payload sent to the API.
37-63: LGTM: Comprehensive test forgetRequestLinkDetailsThe test case for
getRequestLinkDetailsis well-implemented, providing a thorough mock response and verifying the correct URL construction using the link UUID.Consider the following improvements:
- Add a test case for when the API URL is provided.
- Include error handling tests for failed API responses or invalid UUIDs.
- Verify that the function correctly parses and returns the mock response data.
65-86: LGTM: Good test coverage forsubmitRequestLinkFulfillmentThe test case for
submitRequestLinkFulfillmentcorrectly verifies the API URL construction and basic functionality.Consider enhancing the test suite with:
- A test case for when the API URL is provided.
- Error handling tests for failed API responses or invalid input data.
- Verification of the request payload sent to the API, ensuring all required fields are included.
- A more comprehensive mock response to test proper handling of additional returned data.
1-87: Good test coverage, with room for improvementThe test file provides good coverage for the main functions of the request module, focusing on URL construction when the API URL is not provided. The use of Jest's mocking capabilities is effective and the tests are well-structured.
To further enhance the test suite, consider:
- Adding test cases for scenarios where the API URL is provided.
- Implementing error handling tests for each function.
- Including edge case tests (e.g., invalid inputs, network errors).
- Verifying the request payloads sent to the API in each test.
- Expanding mock responses to cover more fields and scenarios.
- Adding tests for any untested parameters or options in the functions.
These additions would provide more comprehensive coverage and align well with the PR objective of updating and improving the request tests.
package.json (1)
11-11: Good consolidation of tests, but consider renaming the script.The modification to the
test:basicscript to include both unit and basic tests is a smart move for consolidation. It aligns well with the PR objective of reducing code duplication in tests.However, the script name
test:basicmight now be misleading since it includes unit tests as well. Consider renaming this script to something more descriptive, such astest:allortest:unit-and-basic.Here's a suggested change:
- "test:basic": "npm run pkg-move && jest 'test/(unit|basic)' --silent --coverage", + "test:unit-and-basic": "npm run pkg-move && jest 'test/(unit|basic)' --silent --coverage",This change would make the script name more accurately reflect its functionality.
test/basic/RequestLinkXChain.test.ts (3)
9-19: LGTM! Consider adding type annotations for improved readability.The
retryfunction is well-implemented and provides a useful mechanism for retrying asynchronous assertions. It correctly handles multiple attempts and throws the last error if all attempts fail.Consider adding type annotations to improve readability:
const retry = async ( assertion: () => Promise<void>, { times = 3, interval = 100 }: { times?: number; interval?: number } = {} ): Promise<void> => { // ... function body ... }
22-160: Great refactoring! Consider extracting test data for improved maintainability.The use of
it.eachfor parameterized testing is an excellent improvement. It reduces code duplication and makes it easy to add new test scenarios. The consolidated test logic covers various token types and cross-chain scenarios effectively.Consider extracting the test data into a separate constant or file. This would further improve maintainability and make it easier to manage test scenarios:
const TEST_SCENARIOS = [ { amount: '0.1', sourceToken: { /* ... */ }, destinationToken: { /* ... */ }, }, // ... other scenarios ... ]; it.each(TEST_SCENARIOS)( '$sourceToken.name to $destinationToken.name', async ({ amount, sourceToken, destinationToken }) => { // ... test logic ... } );
Line range hint
162-254: Good addition of a new test case. Consider refactoring to reduce duplication.The new test case for fetching link details when not provided is a valuable addition to the test suite. It covers an important scenario that wasn't tested before.
To reduce code duplication, consider extracting the common logic (especially the balance check) into a separate function that can be reused across test cases. For example:
async function verifyBalanceChange( destinationToken: TokenInfo, recipientAddress: string, amount: string ) { const initialBalance = await peanut.getTokenBalance({ /* ... */ }); // ... perform transaction ... const numDigits = Math.floor(Math.log10(1 / Number(amount))) + 1; const expectedBalance = Number(initialBalance) + Number(amount); await retry( async () => { const finalBalance = await peanut.getTokenBalance({ /* ... */ }); expect(Number(finalBalance)).toBeCloseTo(expectedBalance, numDigits); }, { times: 15, interval: 2000 } ); } // Use in both test cases await verifyBalanceChange(destinationToken, recipientAddress, amount);This refactoring would make the tests more maintainable and reduce the risk of inconsistencies between test cases.
src/request.ts (4)
39-59: LGTM: Refactored IPrepareXchainRequestFulfillmentTransactionPropsThe transformation of
IPrepareXchainRequestFulfillmentTransactionPropsinto a union type enhances the flexibility of theprepareXchainRequestFulfillmentTransactionfunction. This change allows for more versatile input handling, aligning well with the PR objectives of updating and refactoring the request tests.Consider adding JSDoc comments to explain the purpose of each union type option, which would improve code readability and maintainability.
Line range hint
137-143: LGTM with suggestion: Updated createRequestLink functionThe changes to
createRequestLinkfunction, including the use ofnormalizePathand the addition of theAPIKeyheader, align well with the PR objectives and previous interface updates.Consider adding a check for the presence of
APIKeybefore using it in the header to avoid potential runtime errors:- 'api-key': APIKey!, + 'api-key': APIKey || '',This change would provide better error handling if the
APIKeyis not provided.
Line range hint
164-169: LGTM with suggestion: Updated getRequestLinkDetails functionThe changes to
getRequestLinkDetailsfunction, including the use ofnormalizePathand the addition of theAPIKeyheader, are consistent with the updates tocreateRequestLinkand align well with the PR objectives.Similar to the suggestion for
createRequestLink, consider adding a check for the presence ofAPIKeybefore using it in the header:- 'api-key': APIKey!, + 'api-key': APIKey || '',This change would provide better error handling if the
APIKeyis not provided.
180-193: LGTM with suggestion: Refactored prepareXchainRequestFulfillmentTransaction functionThe refactoring of
prepareXchainRequestFulfillmentTransactionfunction aligns well with the updatedIPrepareXchainRequestFulfillmentTransactionPropstype. The changes improve the function's flexibility and maintainability, consistent with the PR objectives.To improve code readability, consider using a type guard function to determine which structure is being used:
function isLinkProps(props: IPrepareXchainRequestFulfillmentTransactionProps): props is { link: string; apiUrl?: string; APIKey?: string } { return 'link' in props; } // Then in the function: if (isLinkProps(props)) { const { link, apiUrl = 'https://api.peanut.to/', APIKey } = props; linkDetails = await getRequestLinkDetails({ link, apiUrl, APIKey }); } else { linkDetails = props.linkDetails; }This approach would make the code more type-safe and easier to understand.
Also applies to: 199-199
src/consts/interfaces.consts.ts (2)
Line range hint
170-174: LGTM! Consider using more specific types for numerical values.The renaming and addition of new properties in the
IPrepareXchainRequestFulfillmentTransactionResponseinterface improve its clarity and provide more detailed information about cross-chain transactions. This is a positive change that enhances the SDK's functionality.Consider using more specific types for
feeEstimationandestimatedFromAmount. While strings offer flexibility, usingBigNumberfrom ethers.js might provide better precision and type safety for financial calculations. For example:import { BigNumber } from 'ethers' export interface IPrepareXchainRequestFulfillmentTransactionResponse { unsignedTxs: IPeanutUnsignedTransaction[] feeEstimation: BigNumber estimatedFromAmount: BigNumber }This change would ensure type consistency with other parts of the SDK and prevent potential issues with string-to-number conversions.
Line range hint
570-579: LGTM! Consider adding documentation for the new error codes.The addition of the
EGenericErrorCodesenum and its integration into theallErrorEnumstype andSDKStatusclass is a great improvement to the error handling system. This change will allow for more specific and informative error reporting, which is crucial for debugging and user experience.To further enhance the usability of these new error codes, consider adding brief documentation comments for each error code in the
EGenericErrorCodesenum. This will help developers understand when and why each error might occur. For example:export enum EGenericErrorCodes { /** A general error that doesn't fit into other categories */ GENERIC_ERROR, /** The specified blockchain is not supported by the SDK */ ERROR_UNSUPPORTED_CHAIN, /** The provided name exceeds the maximum allowed length */ ERROR_NAME_TOO_LONG, /** The input contains prohibited symbols */ ERROR_PROHIBITED_SYMBOL, /** Failed to retrieve the token balance */ ERROR_GETTING_TOKENBALANCE, /** The specified token is not supported by the SDK */ ERROR_UNSUPPORTED_TOKEN, // Add more generic error codes here if needed }These comments will appear in IDE tooltips, making it easier for developers to understand and use the error codes correctly.
Also applies to: 583-583, 590-638
src/util.ts (1)
757-757: Consider adding a brief documentation comment for thenormalizePathfunction.To improve code readability and maintainability, it would be helpful to add a brief JSDoc comment explaining the purpose and usage of the
normalizePathfunction. This will make it easier for other developers to understand and use the function correctly.Here's a suggested comment you could add above the function:
/** * Normalizes a URL or path by replacing multiple consecutive slashes with a single slash. * @param url - The URL or path to normalize * @returns The normalized URL or path as a string */ export function normalizePath(url: string): string { // ... (existing implementation) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- package.json (1 hunks)
- src/consts/interfaces.consts.ts (1 hunks)
- src/request.ts (6 hunks)
- src/util.ts (1 hunks)
- test/basic/RequestLinkXChain.test.ts (2 hunks)
- test/unit/request.test.ts (1 hunks)
- test/unit/util.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/basic/RequestLinkXChain.test.ts (1)
Learnt from: jjramirezn PR: peanutprotocol/peanut-sdk#157 File: test/basic/RequestLinkXChain.test.ts:143-145 Timestamp: 2024-10-16T16:53:52.383Z Learning: In `test/basic/RequestLinkXChain.test.ts`, the calculation of `numDigits` using `Math.floor(Math.log10(1 / Number(amount))) + 1` is intentional and determines how close the numbers must be to pass the test.
🔇 Additional comments (15)
test/unit/util.test.ts (3)
1-4: Well-structured test suiteThe import statement and overall structure of the test suite are well-organized and follow best practices for Jest testing.
13-15: Correct test assertionThe test assertion
expect(normalizePath(url)).toBe(normalizedUrl)is correctly implemented and follows Jest's best practices for comparing expected and actual values.
1-17: Overall excellent test implementationThis test file for the
normalizePathfunction is well-implemented, with a clear structure, comprehensive test cases, and correct assertions. It provides a solid foundation for ensuring the reliability of thenormalizePathfunction.The use of
it.eachfor multiple test cases is an efficient approach, making the tests easy to read and maintain. The variety of URL scenarios covered demonstrates thorough consideration of different use cases.While the current test cases are good, consider adding a few more edge cases as suggested earlier to further enhance the test coverage.
test/unit/request.test.ts (1)
1-12: LGTM: Proper test setup and mockingThe import statement and mock setup are well-implemented. The use of
beforeEachandafterEachhooks ensures proper isolation between tests and cleanup of mocks.package.json (2)
10-10: Excellent addition of a dedicated unit test script!The new
test:unitscript is a valuable addition to the project's testing infrastructure. It allows for running unit tests separately, which can speed up the development process and make it easier to focus on specific test types.The use of the
--silentand--coverageflags is also commendable, as it provides a cleaner output and generates useful code coverage information.
10-11: Overall improvement to testing infrastructureThese changes to the testing scripts in
package.jsonrepresent a significant improvement to the project's testing infrastructure. They align well with the PR objectives of updating request tests and eliminating code duplication.The addition of a dedicated unit test script and the consolidation of unit and basic tests provide more flexibility and efficiency in the testing process. These improvements will likely lead to better code quality and easier maintenance in the long run.
Great job on enhancing the testing capabilities of the project!
test/basic/RequestLinkXChain.test.ts (2)
141-157: Effective balance check implementation with retry mechanism.The balance check implementation using the
retryfunction improves the test's reliability by accommodating potential delays in transaction finalization. The calculation ofnumDigitsis intentional and determines the precision for the balance comparison.As per the previous discussion, the current retry parameters (15 attempts, 2000ms interval) allowing for up to 30 seconds of retries are considered sufficient.
Line range hint
1-254: Overall, excellent improvements to the test suite.The refactoring and additions to this test file have significantly enhanced its maintainability, reliability, and coverage. The introduction of the
retryfunction, use of parameterized testing withit.each, and the new test case for fetching link details are all valuable improvements.While there are some opportunities for further refactoring to reduce duplication and improve organization, the current state of the file is a substantial step forward. Great work on improving the test suite!
src/request.ts (4)
5-5: LGTM: Addition of APIKey to ICreateRequestLinkPropsThe addition of the optional
APIKeyproperty to theICreateRequestLinkPropsinterface enhances the flexibility of API authentication. This change aligns well with the PR objectives of updating the request tests to use an API key.Also applies to: 22-22
Line range hint
24-28: LGTM: Addition of APIKey to IGetRequestLinkDetailsPropsThe addition of the optional
APIKeyproperty to theIGetRequestLinkDetailsPropsinterface is consistent with the previous interface update and the PR objectives. This change enhances the flexibility of API authentication for getting request link details.
394-394: LGTM: Updated submitRequestLinkFulfillment functionThe update to
submitRequestLinkFulfillmentfunction, incorporating the use ofnormalizePath, ensures consistent URL formatting. This change is in line with the updates made to other functions in the file.
Line range hint
1-426: Overall assessment: Well-structured and consistent changesThe changes in this file are well-aligned with the PR objectives. The refactoring of interfaces and functions improves code flexibility and maintainability. The consistent use of
normalizePathand the addition ofAPIKeyhandling enhance the robustness of the API interactions.A few minor suggestions have been made to further improve error handling and code readability. These suggestions are not critical and can be addressed in a follow-up PR if desired.
src/consts/interfaces.consts.ts (1)
Line range hint
1-638: Overall, these changes significantly improve the SDK's functionality and error handling.The modifications to the
IPrepareXchainRequestFulfillmentTransactionResponseinterface and the introduction of theEGenericErrorCodesenum represent substantial improvements to the Peanut SDK. These changes enhance cross-chain transaction handling and provide more detailed error reporting, which will benefit both developers using the SDK and end-users of applications built with it.The renaming of the interface from "Props" to "Response" clarifies its purpose, and the additional properties provide valuable transaction details. The new generic error codes allow for more precise error handling across various scenarios.
These updates demonstrate a commitment to improving the SDK's usability, maintainability, and robustness. Great work on these enhancements!
src/util.ts (2)
757-766: LGTM: NewnormalizePathfunction added to handle URL path normalization.The
normalizePathfunction is a useful addition that normalizes URL paths by replacing multiple consecutive slashes with a single slash. It handles both full URLs and path-only strings, making it versatile for various use cases within the SDK.Some observations:
- The function correctly uses a try-catch block to handle both full URLs and path-only strings.
- The regular expression
/\/+/gefficiently replaces multiple slashes with a single slash.- The error handling assumes any failure in URL creation means the input is a path-only string, which is a reasonable approach.
757-766: Overall assessment: Approved with minor suggestions for improvement.The addition of the
normalizePathfunction is a valuable enhancement to the utility file. It provides a standardized way to handle URL and path normalization, which can improve consistency across the SDK. The implementation is sound and doesn't introduce any breaking changes.To further improve this addition:
- Consider adding a brief documentation comment as suggested earlier.
- Enhance the error handling to make the function more robust.
- Review the broader codebase to identify potential uses for this new function and ensure consistent URL/path handling.
These minor improvements will make the
normalizePathfunction even more valuable and easier to maintain in the long run.
Deployment notesThis was tested with peanut-ui and related flows (creating and paying request link) and it works, a PR will be created on ui to use the new linkDetails param for prepare xchain when this is deployed. The changes are: MINOR ChangesFixes
Changes in Testing
|
Hugo0
changed the title
Code changes
Test changes