[SLT-157] feat(rest-api): Refactor for RESTful architectural standardization #3126
Conversation
* Validations * Middleware for error handling
abtestingalpha
requested review from
bigboydiamonds,
trajan0x and
Defi-Moses
as code owners
WalkthroughThe pull request introduces significant updates to the REST API, enhancing its structure and functionality. Key changes include the addition of new controllers for handling bridge transactions and token swaps, improved validation mechanisms, and the introduction of utility functions for token information retrieval. The Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (5)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (22)
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
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
9496812
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cb572b76.sanguine-fe.pages.dev |
| Branch Preview URL: | https://api-restful.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3126 +/- ##
===================================================
+ Coverage 36.66888% 37.97374% +1.30485%
===================================================
Files 443 418 -25
Lines 25643 24222 -1421
Branches 119 82 -37
===================================================
- Hits 9403 9198 -205
+ Misses 15498 14285 -1213
+ Partials 742 739 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range and nitpick comments (10)
packages/rest-api/src/routes/tokenListRoute.ts (1)
7-7: Consider aligning the route path with RESTful conventions.The route path
/may not clearly indicate the resource being accessed. Consider using a more descriptive path like/tokensto align with RESTful conventions and improve the API's readability.Apply this diff to update the route path:
-router.get('/', tokenListController) +router.get('/tokens', tokenListController)packages/rest-api/src/utils/findTokenInfo.ts (2)
1-17: LGTM! Consider optimizing the iteration usingObject.entries.The function correctly searches for the token information based on the provided chain and token symbol. It handles the case when the chain is not found and returns the token address and decimals if a match is found.
To optimize the iteration over the chain data, consider using the
Object.entriesmethod:- for (const tokenAddress in chainData) { - if (chainData[tokenAddress].symbol === tokenSymbol) { - return { - address: tokenAddress, - decimals: chainData[tokenAddress].decimals, - } - } - } + for (const [tokenAddress, tokenInfo] of Object.entries(chainData)) { + if (tokenInfo.symbol === tokenSymbol) { + return { + address: tokenAddress, + decimals: tokenInfo.decimals, + } + } + }
3-3: Add type annotations for the input parameters and return value.To improve the type safety and readability of the function, consider adding type annotations for the input parameters and return value:
-export const findTokenInfo = (chain: string, tokenSymbol: string) => { +export const findTokenInfo = (chain: string, tokenSymbol: string): { address: string; decimals: number } | null => {packages/rest-api/src/controllers/getSynapseTxIdController.ts (1)
20-22: Improve error handling by logging the actual error.Consider logging the actual error that occurred in the catch block for debugging purposes. You can use a logger library or simply log the error to the console.
Here's an example of how you can log the error:
} catch (err) { + console.error('Error in getSynapseTxIdController:', err); res.status(500).json({ error: 'Server error' }); }This will help in identifying and fixing issues more easily.
packages/rest-api/src/routes/bridgeRoute.ts (1)
11-12: Add a meaningful path to the route.The route is missing a path. It is a good practice to add a meaningful path to the route for better readability and maintainability.
Consider adding a path like
/bridgeto the route:-router.get( - '/', +router.get( + '/bridge',packages/rest-api/src/controllers/swapTxInfoController.ts (1)
17-19: Consider providing more specific error messages for missing token info.Instead of returning a generic "Invalid token symbol" error, consider providing separate error messages for missing
fromTokenandtoTokeninformation. This will help users identify which token is causing the issue.-if (!fromTokenInfo || !toTokenInfo) { - return res.status(400).json({ error: 'Invalid token symbol' }) -} +if (!fromTokenInfo) { + return res.status(400).json({ error: 'Invalid from token symbol' }) +} +if (!toTokenInfo) { + return res.status(400).json({ error: 'Invalid to token symbol' }) +}packages/rest-api/src/controllers/bridgeTxInfoController.ts (1)
43-45: Consider improving error handling and logging.While the function catches errors and returns a 500 response, it could benefit from more specific error handling and logging. Consider the following:
- Log the actual error using a logger service for easier debugging.
- If possible, distinguish between different types of errors (e.g., validation errors, Synapse service errors) and return appropriate status codes and messages.
- In case of sensitive information in the error object, make sure to sanitize it before sending it in the response.
packages/rest-api/src/controllers/getDestinationTxController.ts (3)
49-51: Improve error handling.The current error handling could be enhanced in the following ways:
- Provide more specific error messages based on the type of error encountered. This will help in debugging and understanding the root cause of the issue.
- Log the error using a logger utility for easier debugging and monitoring.
Here's an example of how the error handling could be improved:
try { // ... } catch (err) { console.error('Error in getDestinationTxController:', err); if (err instanceof SomeSpecificError) { res.status(500).json({ error: 'Specific error message' }); } else { res.status(500).json({ error: 'Internal server error' }); } }
12-12: Consider moving the GraphQL endpoint URL to a configuration file.The GraphQL endpoint URL is currently hardcoded in the controller function. It would be better to move it to a configuration file for the following reasons:
- It allows for easier modification of the URL if it changes in the future.
- It keeps the controller function focused on the business logic rather than configuration details.
- It enables the possibility of using different URLs for different environments (e.g., development, staging, production).
You could define the URL in a
config.tsfile like this:export const config = { graphqlEndpoint: 'https://explorer.omnirpc.io/graphql', };And then import and use it in the controller function:
import { config } from '../config'; // ... const graphqlEndpoint = config.graphqlEndpoint;
3-52: Add code comments for better documentation.While the code is readable and follows a clear structure, it would be beneficial to add some comments explaining the purpose and expected behavior of each section. This will make it easier for other developers (or your future self) to understand and maintain the code.
Here's an example of how comments could be added:
export const getDestinationTxController = async (req, res) => { // Validate the request const errors = validationResult(req); if (!errors.isEmpty()) { return res.status(400).json({ errors: errors.array() }); } try { // Extract query parameters const { originChainId, txHash } = req.query; // Define the GraphQL endpoint and query const graphqlEndpoint = 'https://explorer.omnirpc.io/graphql'; const graphqlQuery = `...`; // Make the GraphQL request const graphqlResponse = await fetch(graphqlEndpoint, { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify({ query: graphqlQuery }), }); // Parse the GraphQL response const graphqlData = await graphqlResponse.json(); const toInfo = graphqlData.data.bridgeTransactions[0]?.toInfo || null; // Return the appropriate response based on the fetched data if (toInfo === null) { res.json({ status: 'pending' }); } else { res.json({ status: 'completed', toInfo }); } } catch (err) { // Handle errors res.status(500).json({ error: 'Server error' }); } };
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (28)
- packages/rest-api/package.json (3 hunks)
- packages/rest-api/src/app.ts (1 hunks)
- packages/rest-api/src/constants/index.ts (1 hunks)
- packages/rest-api/src/controllers/bridgeController.ts (1 hunks)
- packages/rest-api/src/controllers/bridgeTxInfoController.ts (1 hunks)
- packages/rest-api/src/controllers/getBridgeTxStatusController.ts (1 hunks)
- packages/rest-api/src/controllers/getDestinationTxController.ts (1 hunks)
- packages/rest-api/src/controllers/getSynapseTxIdController.ts (1 hunks)
- packages/rest-api/src/controllers/indexController.ts (1 hunks)
- packages/rest-api/src/controllers/swapController.ts (1 hunks)
- packages/rest-api/src/controllers/swapTxInfoController.ts (1 hunks)
- packages/rest-api/src/controllers/tokensListController.ts (1 hunks)
- packages/rest-api/src/middleware/showFirstValidationError.ts (1 hunks)
- packages/rest-api/src/routes/bridgeRoute.ts (1 hunks)
- packages/rest-api/src/routes/bridgeTxInfoRoute.ts (1 hunks)
- packages/rest-api/src/routes/getBridgeTxStatusRoute.ts (1 hunks)
- packages/rest-api/src/routes/getDestinationTxRoute.ts (1 hunks)
- packages/rest-api/src/routes/getSynapseTxIdRoute.ts (1 hunks)
- packages/rest-api/src/routes/index.ts (1 hunks)
- packages/rest-api/src/routes/indexRoute.ts (1 hunks)
- packages/rest-api/src/routes/swapRoute.ts (1 hunks)
- packages/rest-api/src/routes/swapTxInfoRoute.ts (1 hunks)
- packages/rest-api/src/routes/tokenListRoute.ts (1 hunks)
- packages/rest-api/src/services/synapseService.ts (1 hunks)
- packages/rest-api/src/utils/findTokenInfo.ts (1 hunks)
- packages/rest-api/src/utils/formatBNToString.ts (1 hunks)
- packages/rest-api/src/validations/validateTokens.ts (1 hunks)
- packages/rest-api/tsconfig.json (3 hunks)
Additional comments not posted (39)
packages/rest-api/src/constants/index.ts (2)
1-5: LGTM!The constant array is correctly defined and exported.
1-5: Verify the usage ofVALID_BRIDGE_MODULES.The purpose and usage of this constant is not clear from the provided context. Please ensure that it is being used correctly to validate the bridge module names.
Run the following script to verify the constant usage:
packages/rest-api/src/controllers/tokensListController.ts (2)
3-5: Clarify the route and purpose of this endpoint. Consider adding error handling.It would be helpful to have more information about the route this controller function handles and the purpose of this endpoint. Please consider adding this information as a comment or in the function name itself.
Also, consider adding error handling to this function to catch and handle any errors that may occur while processing the request. This will ensure a better user experience and easier debugging.
1-1: Provide the contents of thetokenListimport for verification.To ensure that the correct data is being returned from this endpoint, please provide the contents of the
tokenListimport from../constants/bridgeable. This will allow for verification of the response data.packages/rest-api/src/routes/indexRoute.ts (1)
1-9: LGTM!The
indexRoute.tsfile is well-structured and follows best practices for setting up an Express router. Key observations:
- The necessary dependencies (
expressandindexController) are imported correctly.- The router is created using
express.Router().- The GET route for the root path
/is mapped to theindexController.- The router is exported as the default export, allowing it to be imported and used in other parts of the application.
This modular approach aligns with the MVC architecture mentioned in the PR objectives, promoting code organization and maintainability.
packages/rest-api/src/routes/tokenListRoute.ts (1)
3-3: Verify the imported controller.Ensure that the
tokenListControlleris correctly implemented and handles the request as expected. Verify that it returns the appropriate response format and status codes.Run the following script to locate the controller implementation:
Verification successful
Update import statement to match the controller file name
The
tokenListControlleris correctly implemented, but there's a minor naming inconsistency. The import statement usestokenListController(singular 'token'), while the actual file name istokensListController.ts(plural 'tokens'). To improve code consistency, consider updating the import statement as follows:import { tokenListController } from '../controllers/tokensListController'This change will make the import statement match the actual file name, enhancing code readability and maintainability.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate the `tokenListController` implementation. # Test: Search for the controller file. Expect: A single match. fd --type file --hidden --no-ignore tokenListControllerLength of output: 57
Script:
#!/bin/bash # Description: Search for tokenListController implementation and similar files # Search for files containing "tokenListController" echo "Files containing 'tokenListController':" rg -l "tokenListController" # List files in the controllers directory echo -e "\nFiles in the controllers directory:" fd . "packages/rest-api/src/controllers" # Check for files with similar names echo -e "\nFiles with similar names:" fd "token.*controller" "packages/rest-api/src/controllers" # Check the content of the tokenListRoute.ts file for any typos echo -e "\nContent of tokenListRoute.ts:" cat "packages/rest-api/src/routes/tokenListRoute.ts"Length of output: 1392
packages/rest-api/src/app.ts (1)
8-9: The changes utilizeexpress.json()middleware and integrate the new routing structure.Using
express.json()middleware is a good practice as it allows parsing JSON request bodies. Mounting the importedroutesat the root path is a common practice in Express applications.Verify that the
routesmodule defines the necessary endpoints and handles the requests correctly by running the following script:Verification successful
Routing structure and request handling are correctly implemented
The verification process confirms that the routing structure mentioned in the original review comment is properly implemented. The
routes/index.tsfile combines all individual route files and mounts them to specific paths. Individual route files, such asbridgeRoute.tsandswapRoute.ts, demonstrate correct usage of Express.js routing, including input validation, error handling, and controller integration.Key points:
- The main router in
routes/index.tsusesapp.use()to mount various routes- Individual route files define specific endpoints with proper request handling
- Input validation and error handling are implemented using express-validator and custom middleware
The changes utilize the new routing structure effectively, and the necessary endpoints are defined and handled correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the endpoints and request handling in the routes module. # Test 1: Check if the routes module defines the necessary endpoints. Expect: The file to contain endpoint definitions. rg --type typescript $'app\.(get|post|put|delete|patch)\(' $(fd --type file --regex '^routes\.(js|ts)$') # Test 2: Check if the routes module handles the requests correctly. Expect: The file to contain request handling logic. rg --type typescript $'(req|request)\.' $(fd --type file --regex '^routes\.(js|ts)$')Length of output: 356
Script:
#!/bin/bash # Description: Examine the contents of the routes file and search for endpoint definitions and request handling. # Display the contents of the routes file cat packages/synapse-interface/constants/routes.ts # Search for endpoint definitions grep -E "(get|post|put|delete|patch)\\(" packages/synapse-interface/constants/routes.ts # Search for request handling grep -E "(req|request)\\." packages/synapse-interface/constants/routes.tsLength of output: 1290
Script:
#!/bin/bash # Description: Search for the actual route implementations and request handling logic # Find potential route files echo "Potential route files:" fd -e ts -e js --type file --search-path packages/rest-api/src | grep -E '(routes?|api)' # Search for Express.js route definitions echo -e "\nExpress.js route definitions:" rg --type typescript "app\.(get|post|put|delete|patch)\(" packages/rest-api/src # Search for request handling echo -e "\nRequest handling:" rg --type typescript "(req|res)\." packages/rest-api/srcLength of output: 1978
Script:
#!/bin/bash # Description: Examine the contents of key route files # Display the contents of the main routes file echo "Contents of routes/index.ts:" cat packages/rest-api/src/routes/index.ts # Display the contents of an individual route file (e.g., bridgeRoute.ts) echo -e "\nContents of routes/bridgeRoute.ts:" cat packages/rest-api/src/routes/bridgeRoute.ts # Display the contents of another individual route file (e.g., swapRoute.ts) echo -e "\nContents of routes/swapRoute.ts:" cat packages/rest-api/src/routes/swapRoute.tsLength of output: 3197
packages/rest-api/src/services/synapseService.ts (1)
1-11: LGTM!The code in
synapseService.tsfollows a clear and logical structure. It properly imports the necessary dependencies, initializes theJsonRpcProviderinstances using the primary RPC URL of each chain, extracts the chain IDs, and initializes a new instance ofSynapseSDKwith the required parameters.The code looks good and I don't see any issues or improvements needed.
packages/rest-api/src/routes/getDestinationTxRoute.ts (1)
1-22: LGTM!The code follows a modular structure and implements input validation for the GET route. The use of express-validator and the
showFirstValidationErrormiddleware ensures that the required parameters are present and of the correct type before invoking the controller.The separation of the route definition from the controller logic promotes a clean and maintainable codebase.
packages/rest-api/src/controllers/getSynapseTxIdController.ts (1)
5-23: LGTM!The
getSynapseTxIdControllerfunction is well-implemented and follows best practices for input validation, error handling, and RESTful response format. The code is clean, readable, and well-structured.packages/rest-api/src/controllers/indexController.ts (1)
4-21: LGTM!The
indexControllerfunction correctly retrieves token and chain information and sends it as a JSON response. The mapping operations to create thetokensWithChainsandavailableChainsarrays are implemented accurately, and the JSON response structure is clear and provides relevant information.packages/rest-api/src/middleware/showFirstValidationError.ts (1)
1-24: LGTM!The
showFirstValidationErrormiddleware function is implemented correctly and follows best practices:
- It uses the
express-validatorlibrary to check for validation errors in the request.- It sends a detailed 400 response with the first validation error details, which is helpful for debugging.
- It correctly calls the
nextfunction if no validation errors are found to pass control to the next middleware.- It uses TypeScript types for the request, response, and next function parameters.
Great job on this middleware function! It will help improve the error handling and input validation of the REST API.
packages/rest-api/src/validations/validateTokens.ts (1)
5-22: LGTM!The
validateTokensfunction is well-structured and follows best practices for input validation using theexpress-validatorlibrary. It checks if the token parameter is a string and exists, and throws an informative error if not. It also uses thefindTokenInfoutility function to retrieve token information, which promotes code reuse and modularity.The function mutates the
res.localsobject to store the token info, which can be accessed by subsequent middleware or route handlers. This is a common and acceptable pattern in Express applications.Overall, the code changes look good and can be approved.
packages/rest-api/tsconfig.json (1)
25-25: LGTM!The
downlevelIterationcompiler option is a valid addition that allows iteration over objects that don't have a[Symbol.iterator]()method.packages/rest-api/src/routes/swapTxInfoRoute.ts (3)
1-8: LGTM!The imports are well-organized, follow a consistent pattern, and maintain a clear separation of concerns. The naming of the imported modules is descriptive and helps in understanding their purpose.
11-26: LGTM!The route definition is clear, follows Express.js conventions, and has comprehensive input validation. The use of custom validation against the
CHAINS_ARRAYand thevalidateTokensutility function ensures data integrity. TheshowFirstValidationErrormiddleware handles validation errors effectively. The separation of the route handler into a separate controller file promotes modularity and maintainability.
1-28: Overall, the code in this file is well-structured, modular, and follows best practices.The route definition is clear, has comprehensive input validation, and maintains separation of concerns. The use of utility functions and middleware promotes code reuse and maintainability. The code is readable and follows a consistent pattern.
Great job on this implementation! The code is of high quality and ready to be merged.
packages/rest-api/src/routes/swapRoute.ts (3)
9-26: Route definition and middleware setup look good!The route is correctly defined using
express.Router(), and the middleware setup is appropriate. The use ofshowFirstValidationErrormiddleware ensures that validation errors are handled properly.
13-23: Input validation rules are comprehensive and effective.The validation rules cover all the necessary fields for a token swap request. The custom validations for
chain,fromToken, andtoTokenensure that only valid and supported values are accepted. This helps prevent invalid requests from reaching the controller.
1-28: Overall, the swapRoute.ts file is well-structured, modular, and follows best practices.The route definition is clean and properly configured to handle token swap requests. The use of middleware for input validation and error handling enhances the robustness and reliability of the API. The input validation rules are comprehensive and ensure data integrity.
Great job on this file! It demonstrates a solid understanding of Express.js routing, middleware, and validation techniques.
packages/rest-api/src/routes/getSynapseTxIdRoute.ts (3)
8-32: LGTM!The route definition and middleware usage are correct and follow best practices:
- The route is defined using
express.Router().- The route path and HTTP method are appropriate for getting a Synapse transaction ID.
- The usage of validation middleware from
express-validatoris appropriate for checking request parameters.- The custom middleware
showFirstValidationErroris used to handle validation errors.- The separation of the route handler into a separate controller file promotes modularity.
12-27: Validation checks are comprehensive and appropriate.The validation checks cover all the required request parameters with appropriate rules:
originChainIdis checked to be numeric and required.bridgeModuleis checked to be a string, one of the valid bridge modules, and required.txHashis checked to be a string and required.The validation errors are properly handled by the
showFirstValidationErrormiddleware.
1-32: The file structure promotes modularity and separation of concerns.The route definition is separated from the route handler, which is encapsulated in a separate controller file
getSynapseTxIdController. This promotes modularity and maintainability.The validation middleware
showFirstValidationErrorand the valid bridge modules constants are imported from separate files, promoting reusability.Overall, the file structure follows the principle of separation of concerns, making the code more modular and maintainable.
packages/rest-api/src/routes/index.ts (1)
1-25: LGTM!The file follows a modular and organized approach for setting up the Express router and defining the API routes. The code is clean, readable, and follows best practices. The route paths are well-defined and follow a consistent naming convention.
Great job on structuring the routes in a maintainable way!
packages/rest-api/src/controllers/swapController.ts (1)
6-30: The swap controller follows the MVC architecture and RESTful standards.The
swapControllerfunction aligns with the PR objectives by:
- Handling the swap request in a controller, following the MVC architecture.
- Performing input validation using express-validator to prevent silent response errors.
- Using utility functions like
parseUnitsandformatUnitsto handle token amounts, improving code readability and maintainability.Overall, the implementation looks good and follows the architectural standardization goals of the PR.
packages/rest-api/package.json (3)
11-11: LGTM!The new
"dev"script is a great addition to improve the development workflow. It will automatically restart the application when changes are made to the source files, saving time and effort during development.
28-28: Excellent choice!Adding the
express-validatordependency is a great step towards improving the application's input validation capabilities. This middleware will help ensure that all inputs are properly checked and sanitized before being processed, reducing the risk of silent response errors and enhancing the overall robustness of the API.
38-38: Good addition!Including the
ts-nodepackage as a dev dependency is necessary to support the new"dev"script that executes the TypeScript source files directly. This will enable a smooth development experience by allowing the application to run without a separate build step.packages/rest-api/src/routes/getBridgeTxStatusRoute.ts (2)
1-8: Imports look good!The imports are well-organized and follow a consistent naming convention. The custom middleware and controller names are descriptive and adhere to the established naming pattern.
9-38: Route definition and validation look solid!The Express route is defined correctly, and the
express-validatormiddleware is used effectively to validate the request parameters. The validation checks ensure that the required parameters are present and have the expected types and values.The use of constants for validation is a good practice as it centralizes the valid values and improves maintainability. The custom error handling middleware ensures consistent handling of validation errors.
Separating the route handler into a separate controller file promotes modularity and separation of concerns, which enhances the overall code structure.
packages/rest-api/src/routes/bridgeTxInfoRoute.ts (2)
11-30: LGTM! The route definition and input validation rules are comprehensive and well-structured.
- The route is correctly defined as a GET request at the root path.
- The input validation rules cover all the necessary fields (
fromChain,toChain,fromToken,toToken,amount, anddestAddress) and ensure their validity.- The custom validation functions for
fromChainandtoChainprovide an additional layer of validation against theCHAINS_ARRAYconstant.- The
validateTokensfunction is a nice abstraction for validating tokens based on the corresponding chain IDs, promoting code reuse and maintainability.
31-32: Excellent use of middleware and controller!
- The
showFirstValidationErrormiddleware is a great way to handle validation errors and send an appropriate response to the client, ensuring a smooth user experience.- Using the
bridgeTxInfoControllerfunction as the final request handler promotes separation of concerns by keeping the route definition focused on input validation and delegating the business logic to the controller.packages/rest-api/src/controllers/swapTxInfoController.ts (2)
1-5: Imports look good.The necessary imports for
express-validator,@ethersproject/units, and theSynapseservice are correctly added.
7-10: Input validation is properly implemented.The function correctly uses
express-validatorto validate the request inputs and returns a 400 response with the validation errors if any are found.packages/rest-api/src/controllers/bridgeController.ts (1)
26-36: LGTM!The response formatting logic looks good. It maps over the quotes and formats the
maxAmountOutandfeeAmountfields using theformatBNToStringutility function with the correct token decimals.packages/rest-api/src/controllers/bridgeTxInfoController.ts (1)
6-46: LGTM!The
bridgeTxInfoControllerfunction is well-structured and follows best practices. It validates the request, retrieves necessary data, interacts with the Synapse service to fetch quotes and transaction info, and returns a comprehensive response. The use ofres.localsfor sharing data andPromise.allfor parallel processing is commendable.packages/rest-api/src/controllers/getDestinationTxController.ts (2)
1-2: Import statements look good.The import statements are correctly used to bring in the required dependencies.
3-52: Controller function structure and flow are well-implemented.The
getDestinationTxControllerfunction follows a clear and logical flow:
- Validate the request using express-validator.
- Make a GraphQL request to fetch bridge transaction details.
- Return the appropriate response based on the fetched data.
- Catch and handle any errors.
The async/await syntax is used effectively for handling asynchronous operations, and the GraphQL query is defined in a readable manner using a template literal.
packages/rest-api/src/controllers/getBridgeTxStatusController.ts (1)
1-62: Implementation looks good, with a few suggestions for improvement.The
getBridgeTxStatusControllerfunction follows the MVC architecture and implements the required functionality as per the PR objectives. It validates the request inputs, retrieves the transaction status from the Synapse service, and fetches additional transaction details using a GraphQL request. The response format aligns with the RESTful standards.Here are a few suggestions for improvement:
- Consider adding more specific error messages and logging the error details for better debugging and troubleshooting.
- Extract the GraphQL request logic into a separate utility function to improve modularity and reusability.
- Evaluate the performance impact of the additional GraphQL request and consider implementing caching mechanisms or rate limiting if necessary.
packages/rest-api/src/controllers/getBridgeTxStatusController.ts
Outdated
Show resolved
Hide resolved
packages/rest-api/src/controllers/getBridgeTxStatusController.ts
Outdated
Show resolved
Hide resolved
abtestingalpha
changed the title
|
All looks good to me, code is significantly cleaner. Some smaller questions I have for the next iteration:
|
Bundle ReportChanges will increase total bundle size by 262.6kB (0.73%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/rest-api/src/controllers/getDestinationTxController.ts (1)
6-72: The controller implementation looks good!The
getDestinationTxControllerfunction is well-structured and handles the retrieval of transaction details effectively. It incorporates input validation, external API interaction, and conditional response handling. The function also formats the transaction value based on the token's decimals, providing a user-friendly representation of the value.Here are a few suggestions for improvement:
- Consider extracting the GraphQL query into a separate file or constant to improve readability and maintainability.
- Consider adding more specific error handling and logging for better debugging and monitoring.
- Consider adding unit tests to ensure the correctness of the controller's behavior under different scenarios.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/rest-api/src/controllers/getBridgeTxStatusController.ts (1 hunks)
- packages/rest-api/src/controllers/getDestinationTxController.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/rest-api/src/controllers/getBridgeTxStatusController.ts
Additional comments not posted (1)
packages/rest-api/src/controllers/getDestinationTxController.ts (1)
1-4: LGTM!The imports are correctly used and necessary for the functionality of the controller.
github-actions
bot
added
the
javascript
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
packages/rest-api/src/controllers/tokenListController.ts (1)
3-5: LGTM, but consider removing the unused parameter.The
tokenListControllerfunction correctly sends thetokenListdata as a JSON response. The async implementation is a good practice for handling API requests.However, the
_reqparameter is unused and can be removed to improve code clarity.Apply this diff to remove the unused parameter:
-export const tokenListController = async (_req, res) => { +export const tokenListController = async (_, res) => { res.json(tokenList) }packages/rest-api/src/tests/tokenListRoute.test.ts (1)
1-25: LGTM! Consider adding more test cases for completeness.The unit test for the
tokenListRouteendpoint is well-structured and verifies the expected behavior by checking the response status code, the number of tokens, and the addresses for specific tokens.To further enhance the test coverage, consider adding the following test cases:
- Test for an empty token list response.
- Test for invalid or missing token addresses.
- Test for error scenarios, such as server errors or timeouts.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (9)
- packages/rest-api/.babelrc (1 hunks)
- packages/rest-api/.eslintrc.js (1 hunks)
- packages/rest-api/jest.config.js (1 hunks)
- packages/rest-api/package.json (2 hunks)
- packages/rest-api/src/controllers/tokenListController.ts (1 hunks)
- packages/rest-api/src/routes/tokenListRoute.ts (1 hunks)
- packages/rest-api/src/tests/indexRoute.test.ts (1 hunks)
- packages/rest-api/src/tests/swapRoute.test.ts (1 hunks)
- packages/rest-api/src/tests/tokenListRoute.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/rest-api/package.json
- packages/rest-api/src/routes/tokenListRoute.ts
Additional context used
Learnings (1)
packages/rest-api/src/tests/tokenListRoute.test.ts (1)
Learnt from: abtestingalpha PR: synapsecns/sanguine#3126 File: packages/rest-api/src/routes/tokenListRoute.ts:7-7 Timestamp: 2024-09-13T20:00:10.635Z Learning: The `tokenListRoute` returns a static token list and does not require input validation.
Additional comments not posted (13)
packages/rest-api/.babelrc (1)
1-3: LGTM!The Babel configuration is correct and follows the standard format. The addition of
@babel/preset-envand@babel/preset-typescriptpresets will enable the use of modern JavaScript and TypeScript in the project, enhancing the development experience and maintainability of the codebase.packages/rest-api/src/controllers/tokenListController.ts (1)
1-1: LGTM!The import statement correctly imports the
tokenListconstant from thebridgeablemodule.packages/rest-api/.eslintrc.js (1)
3-10: LGTM!The addition of the
overridessection to specify a differentprettier/prettierrule forjest.config.jsis a valid and reasonable configuration change. It allows for necessary formatting flexibility in the Jest configuration file while maintaining the project's overall ESLint rules.packages/rest-api/jest.config.js (6)
2-2: LGTM!The
ts-jestpreset is the recommended way to integrate TypeScript with Jest. It simplifies the configuration and allows Jest to transpile TypeScript files out of the box.
3-3: LGTM!Setting the
testEnvironmenttonodeis correct for a Node.js-based REST API project. It ensures that the tests run in a Node.js environment, matching the runtime of the application.
4-4: LGTM!The
rootssetting correctly points to thesrcdirectory, which is a common convention for storing test files. This ensures that Jest will discover and run the tests located in thesrcdirectory.
5-7: LGTM!The
transformsetting correctly usesbabel-jestto process.tsand.tsxfiles. This allows Jest to transpile and run tests written in TypeScript, enabling the use of modern JavaScript features and TypeScript syntax in the tests.
8-8: LGTM!The
moduleFileExtensionssetting correctly includes the necessary file extensions for a TypeScript project. It allows Jest to resolve and import modules with.ts,.tsx,.js,.jsx,.json, and.nodeextensions, providing flexibility in module usage within the tests.
9-9: LGTM!The
moduleDirectoriessetting correctly includes bothnode_modulesandsrcdirectories. This allows Jest to resolve modules from the installed dependencies innode_modulesas well as custom modules or test utilities located in thesrcdirectory, providing flexibility in module resolution.packages/rest-api/src/tests/indexRoute.test.ts (1)
9-18: LGTM!The test is correctly checking the response status and message for the root URL. The use of the
supertestlibrary to make a GET request to the root URL is a good practice. Theexpectstatements are correctly asserting the response status and message.packages/rest-api/src/tests/swapRoute.test.ts (3)
9-21: LGTM!The test is well-structured and covers the happy path scenario for the
/swaproute. It verifies the response status and the presence of essential properties in the response body. The 10-second timeout is suitable for a test that makes a real API call.
23-32: LGTM!The test effectively validates the error handling for a missing
toTokenparameter. It ensures that the API returns a 400 status code and the error message correctly identifies the missing field. The 10-second timeout is suitable for a test that makes a real API call.
34-43: LGTM!The test effectively validates the error handling for a missing
amountparameter. It ensures that the API returns a 400 status code and the error message correctly identifies the missing field. The 10-second timeout is suitable for a test that makes a real API call.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- packages/rest-api/src/tests/bridgeRoute.test.ts (1 hunks)
- packages/rest-api/src/tests/indexRoute.test.ts (1 hunks)
- packages/rest-api/src/tests/swapRoute.test.ts (1 hunks)
- packages/rest-api/src/tests/swapTxInfoRoute.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/rest-api/src/tests/indexRoute.test.ts
- packages/rest-api/src/tests/swapRoute.test.ts
Additional comments not posted (7)
packages/rest-api/src/tests/swapTxInfoRoute.test.ts (4)
10-21: LGTM!This test case is well-structured and covers a valid scenario for swapping tokens. It checks for the correct status code and expected properties in the response body. The timeout of 10 seconds is appropriate for a potentially long-running operation.
23-33: LGTM!This test case is well-structured and covers an invalid scenario with an unsupported chain. It checks for the correct status code and expected error message properties in the response body. The timeout of 10 seconds is consistent with other test cases.
35-43: LGTM!This test case is well-structured and covers an invalid scenario with a missing required parameter. It checks for the correct status code and expected error message property in the response body. The timeout of 10 seconds is consistent with other test cases.
45-53: LGTM!This test case is well-structured and covers an invalid scenario with a missing required parameter. It checks for the correct status code and expected error message property in the response body. The timeout of 10 seconds is consistent with other test cases.
packages/rest-api/src/tests/bridgeRoute.test.ts (3)
10-23: LGTM!The test case covers a valid scenario with specific input parameters and makes appropriate assertions on the response. The timeout of 15000ms seems reasonable for an external API call.
25-38: LGTM!The test case covers an error scenario with an unsupported
fromChainvalue and makes appropriate assertions on the response status and error message. The timeout of 10000ms seems reasonable for an error scenario.
40-50: LGTM!The test case covers an error scenario with an unsupported
toChainvalue and makes appropriate assertions on the response status and error message. The timeout of 10000ms seems reasonable for an error scenario.
Description
Refactors app to support MVC architecture and RESTful responses.
yarn devhot reloadSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation