feat(rest-api): /destinationTx returns status of refunded when RFQ transaction has been refunded [SLT-350]
#3307
Conversation
WalkthroughThe changes introduce new constants and utility functions to enhance the handling of bridge transactions within the application. Two constants, Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3307 +/- ##
===================================================
- Coverage 44.38046% 35.14833% -9.23213%
===================================================
Files 137 77 -60
Lines 5383 2663 -2720
Branches 356 82 -274
===================================================
- Hits 2389 936 -1453
+ Misses 2979 1722 -1257
+ Partials 15 5 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
3b5a684
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d27505d6.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rest-api-refund-status.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (13)
packages/rest-api/src/constants/enums.ts (1)
1-7: LGTM! Consider adding documentation and explicit values.The
BridgeStatusenum is well-structured and aligns with the PR objectives. It correctly includes the newREFUNDEDstatus as mentioned in the PR title.To improve maintainability and clarity, consider the following suggestions:
Add JSDoc comments to describe what each status represents. This will help other developers understand the lifecycle of a bridge transaction.
Consider assigning explicit values to each enum member. While the current auto-incrementing behavior works, explicit values can provide more control and stability, especially if the order or number of statuses might change in the future.
Here's an example of how you could implement these suggestions:
/** * Represents the various states of a bridge transaction. */ export enum BridgeStatus { /** Initial state or unknown status */ NULL = 0, /** Transaction has been requested but not yet processed */ REQUESTED = 1, /** Relayer has proved the transaction */ RELAYER_PROVED = 2, /** Relayer has claimed the transaction */ RELAYER_CLAIMED = 3, /** Transaction has been refunded */ REFUNDED = 4, }packages/rest-api/src/utils/fetchBridgeTransaction.ts (1)
1-4: Consider adding a return type for improved type safety.The function signature looks good, but adding an explicit return type would enhance type safety and make the function's contract clearer.
Consider updating the function signature as follows:
export const fetchBridgeTransaction = async ( originChainId: number | string, txHash: string ): Promise<any> => { // ... function body ... }Replace
anywith a more specific type that matches the expected structure of the GraphQL response for even better type safety.packages/rest-api/src/tests/destinationTxRoute.test.ts (4)
19-19: LGTM! Consider adding a test description.The new assertion for checking the 'completed' status enhances the test coverage and aligns with the PR objective. Good job!
Consider adding a description to the test case to clarify its purpose:
- it('should return destination transaction info for valid input', async () => { + it('should return destination transaction info with completed status for valid input', async () => {
33-56: Excellent addition! Consider using constants for consistency.The new test case for refunded transactions is well-structured and covers the expected behavior. It aligns perfectly with the PR objective.
For consistency and maintainability, consider using constants for the status values:
+ const COMPLETED_STATUS = 'completed'; + const REFUNDED_STATUS = 'refunded'; it('should return a refunded response for refunded transaction', async () => { // ... (existing code) expect(response.status).toBe(200) expect(response.body).toHaveProperty('status') - expect(response.body.status).toBe('refunded') + expect(response.body.status).toBe(REFUNDED_STATUS) // ... (rest of the test) }, 10000)This approach would make it easier to update status values across all tests if needed in the future.
58-67: Great addition! Consider increasing the timeout for consistency.The new test case for non-existent transaction hashes is a valuable addition, ensuring proper error handling for edge cases.
For consistency with other tests in this file, consider increasing the timeout to 10000ms:
- }, 10000) + }, 10000)This change ensures that all tests have the same timeout, which can help prevent potential issues with slower test environments.
Line range hint
1-67: Overall, excellent additions to the test suite!The new test cases for refunded transactions and non-existent transaction hashes, along with the enhanced assertion for completed transactions, significantly improve the test coverage. These changes align well with the PR objectives and contribute to a more robust API.
If you'd like assistance in implementing any of the suggested improvements or adding more test cases, please let me know. Great job on enhancing the test suite!
packages/rest-api/src/constants/abis/fastBridgeRouter.json (3)
69-160: Consider adding inline documentation for thebridgefunction.The
bridgefunction is complex with multiple parameters and nested structures. To improve readability and maintainability, consider adding inline comments or natspec documentation explaining the purpose of each parameter and the overall function logic.
30-68: Clarify the usage ofrawParamsin theadapterSwapfunction.The
adapterSwapfunction uses arawParamsparameter of typebytes. It would be helpful to add a comment explaining the expected format and content of this parameter for better understanding and proper usage.
347-386: LGTM: Custom error definitions are comprehensive and gas-efficient.The custom error definitions cover various failure scenarios specific to the contract's functionality. Using custom errors is a gas-efficient way to handle exceptions in Solidity contracts. The error names are descriptive, which is good for understanding the reason for reverts.
Consider adding brief comments for each error to provide more context on when they are thrown, especially for less obvious cases like
TokenAddressMismatchorTokensIdentical.packages/rest-api/src/constants/abis/fastBridge.json (1)
807-850: Custom errors are well-defined and cover various failure scenarios.The ABI includes a comprehensive set of custom errors, which is a gas-efficient way to provide informative error messages. These errors cover various scenarios including access control issues, parameter validation, and invalid state transitions.
Positive aspects:
- Use of custom errors instead of revert strings for gas efficiency.
- Errors cover a wide range of potential issues, aiding in debugging and providing clear feedback.
- Some errors include parameters (e.g.,
AccessControlUnauthorizedAccount), offering more context about the error condition.Suggestion for improvement:
Consider adding more specific errors for bridge-related operations. For example, you could add errors likeInsufficientBridgeLiquidityorBridgeRequestExpiredto provide more precise information about why a bridge operation might fail.Here's an example of how you could add more specific bridge-related errors:
+ { + "type": "error", + "name": "InsufficientBridgeLiquidity", + "inputs": [ + { "name": "requested", "type": "uint256", "internalType": "uint256" }, + { "name": "available", "type": "uint256", "internalType": "uint256" } + ] + }, + { + "type": "error", + "name": "BridgeRequestExpired", + "inputs": [ + { "name": "expirationTime", "type": "uint256", "internalType": "uint256" }, + { "name": "currentTime", "type": "uint256", "internalType": "uint256" } + ] + }, { "type": "error", "name": "ZeroAddress", "inputs": [] } ]These additional errors would provide more specific information about why a bridge operation might fail, improving the debugging process and user experience.
packages/rest-api/src/utils/getBridgeStatus.ts (1)
33-33: Validatekappainput formatWhile you check if
kappastarts with '0x', it may be beneficial to validate thatkappais a valid hexadecimal string of the expected length to prevent errors downstream.packages/rest-api/src/controllers/destinationTxController.ts (2)
28-32: Simplify Payload by Omitting Null FieldsIncluding
fromInfoandtoInfoasnullin the payload may not be necessary and could clutter the response. Consider omitting these fields when they arenullto simplify the payload.Apply this diff to remove
fromInfoandtoInfowhen they arenull:payload = { status: 'not found', - fromInfo, - toInfo, }
15-15: Limit Scope ofpayloadVariableSince
payloadis only used within thetryblock, declaring it inside the block limits its scope and improves code readability.Apply this diff to move
payloadinside thetryblock:- let payload = {} try { + let payload = {}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- packages/rest-api/src/constants/abis/fastBridge.json (1 hunks)
- packages/rest-api/src/constants/abis/fastBridgeRouter.json (1 hunks)
- packages/rest-api/src/constants/enums.ts (1 hunks)
- packages/rest-api/src/constants/index.ts (1 hunks)
- packages/rest-api/src/controllers/destinationTxController.ts (1 hunks)
- packages/rest-api/src/serializers/serializeBridgeInfo.ts (1 hunks)
- packages/rest-api/src/tests/destinationTxRoute.test.ts (2 hunks)
- packages/rest-api/src/utils/fetchBridgeTransaction.ts (1 hunks)
- packages/rest-api/src/utils/getBridgeStatus.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
packages/rest-api/src/serializers/serializeBridgeInfo.ts (2)
1-4: LGTM: Imports are relevant and concise.The imported libraries and utility functions are appropriate for the task at hand. Good job keeping the imports minimal and focused.
1-19: Overall assessment: Good implementation with room for improvement.The
serializeBridgeInfofunction effectively processes and formats bridge transaction information. However, consider implementing the suggested improvements to enhance type safety, error handling, and code clarity. These changes will make the code more robust and maintainable in the long run.packages/rest-api/src/constants/index.ts (1)
31-41: Verify the correctness ofFAST_BRIDGE_ROUTER_ADDRESS_MAPThe new constant
FAST_BRIDGE_ROUTER_ADDRESS_MAPhas been added, but there are a couple of points that need attention:
All addresses in the map are identical ('0x00cD000000003f7F682BE4813200893d4e690000'). Please verify if this is intentional or if unique addresses should be assigned for each chain.
Some chain IDs present in
SUPPORTED_SWAP_CHAIN_IDSare missing from this new map (e.g., AURORA, AVALANCHE, FANTOM). Consider if these should be included or if there's a reason for their exclusion.To help verify the correctness of the addresses, you can run the following script:
packages/rest-api/src/constants/abis/fastBridgeRouter.json (2)
1-12: LGTM: Constructor definition is appropriate.The constructor is well-defined with a single
owner_parameter of typeaddress. This is a good practice for setting up the initial owner of the contract.
302-346: LGTM: Event definitions are well-structured.The event definitions for
FastBridgeSet,OwnershipTransferred, andSwapQuoterSetare appropriate for logging important state changes. The use of indexed parameters inOwnershipTransferredis a good practice for efficient event filtering.packages/rest-api/src/constants/abis/fastBridge.json (4)
1-8: Constructor looks good.The constructor is correctly defined with an
_ownerparameter, which is a common pattern for setting up initial ownership of the contract. Thenonpayablestate mutability is appropriate for a constructor.
9-15: View functions are well-defined and comprehensive.The ABI includes a wide range of view functions that provide read-only access to important contract state and configuration. These functions cover roles (e.g.,
DEFAULT_ADMIN_ROLE,GOVERNOR_ROLE), constants (e.g.,DISPUTE_PERIOD,FEE_BPS), and state readers (e.g.,bridgeProofs,bridgeStatuses). The use ofviewstate mutability is correct, ensuring these functions don't modify the contract state.Particularly noteworthy is the
getBridgeTransactionfunction, which provides a comprehensive view of a bridge transaction's details. This function will be crucial for off-chain services to retrieve and verify transaction information.Also applies to: 16-22, 23-29, 30-36, 37-43, 44-50, 51-57, 58-64, 65-71, 72-78, 124-132, 133-139, 140-152, 153-166, 167-173, 184-190, 204-272, 273-281, 282-291, 292-300, 311-320, 321-327, 328-334, 335-341, 416-424
1-851: Overall, the FastBridge ABI is well-designed and comprehensive.This ABI provides a complete interface for interacting with the FastBridge contract, covering all necessary aspects including the constructor, view and state-changing functions, events, and custom errors.
Key strengths:
- Comprehensive set of functions for both core bridge operations and administrative tasks.
- Well-structured events that provide a detailed audit trail of contract activities.
- Use of custom errors for gas-efficient and informative error handling.
- Proper use of state mutability specifiers (
view,payable,nonpayable).Recommendations:
- Ensure that access control is properly implemented in the contract code, especially for administrative functions.
- Consider adding more specific bridge-related errors as suggested earlier.
- Thoroughly test all functions, especially those involving cross-chain operations, to ensure they behave as expected in various scenarios.
This ABI forms a solid foundation for a cross-chain bridge contract. When implementing the contract, pay close attention to security best practices, especially around access control and handling of user funds.
435-471: Events are well-defined and provide a comprehensive audit trail.The ABI includes a robust set of events that cover all major operations and state changes in the contract. These events are crucial for off-chain services to track and react to on-chain activities.
Key points:
- Events cover bridge operations (e.g.,
BridgeRequested,BridgeRelayed,BridgeDepositClaimed) and administrative actions (e.g.,RoleGranted,FeeRateUpdated).- Proper use of indexing for key parameters (e.g.,
transactionId,sender,relayer) allows for efficient event filtering.- Events include all relevant information about each action, providing a detailed audit trail.
These events will be invaluable for monitoring the bridge's activities, reconciling cross-chain transactions, and maintaining an off-chain record of administrative changes. Ensure that these events are emitted at the appropriate points in the contract's functions.
To verify the correct emission of events, you can run the following script:
#!/bin/bash # Check for event emissions in the contract implementation ast-grep --lang solidity --pattern 'function $FUNC_NAME($_) $_ { $$$ emit $EVENT_NAME($$$); $$$ }'This script will help identify functions that emit events, ensuring that all important actions are properly logged.
Also applies to: 472-502, 503-521, 522-546, 547-607, 608-668, 669-687, 688-706, 707-731, 732-756, 757-781, 782-806
packages/rest-api/src/controllers/destinationTxController.ts (1)
68-71: Verify That Logged Payload Does Not Contain Sensitive InformationWhen logging
payload, ensure it does not include any sensitive user information that should not be recorded.Run the following script to check for sensitive data in the payload:
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/rest-api/src/routes/destinationTxRoute.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/rest-api/src/routes/destinationTxRoute.ts (1)
37-39: Update All References tostatusType ChangeThe
statusproperty's type has been changed frombooleantostring. Ensure that all parts of the documentation and codebase that referencestatusare updated accordingly to reflect this change. This is important so that consumers of the API are aware of the new possible string values forstatus.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (4)
packages/rest-api/src/types/index.d.ts (2)
37-41: LGTM! Consider adding JSDoc comments.The
BridgeTransactioninterface is well-structured and clearly represents a cross-chain transaction. The use offromInfoandtoInfoof typeBridgeTransactionInfoprovides a clear separation of source and destination information.Consider adding JSDoc comments to explain the purpose of each property, especially
kappa. This would enhance code readability and maintainability. For example:export interface BridgeTransaction { /** Unique identifier for the bridge transaction */ kappa: string; /** Information about the source of the transaction */ fromInfo: BridgeTransactionInfo; /** Information about the destination of the transaction */ toInfo: BridgeTransactionInfo; }
43-53: LGTM! Consider adding optional properties and JSDoc comments.The
BridgeTransactionInfointerface provides comprehensive information about a transaction on a specific chain. The properties cover essential details such as chain ID, addresses, transaction hash, values, and timing information.Consider the following suggestions to enhance the interface:
Add JSDoc comments to explain the purpose of each property, especially for less obvious ones like
USDValue.Consider making some properties optional with the
?operator, as they might not always be available immediately (e.g.,blockNumberorformattedTimemight not be known until the transaction is confirmed).Here's an example of how you could implement these suggestions:
export interface BridgeTransactionInfo { /** ID of the blockchain where the transaction occurred */ chainID: number; /** Address involved in the transaction (sender or recipient) */ address: string; /** Transaction hash */ txnHash: string; /** Transaction amount in the native token */ value: string; /** Transaction amount converted to USD */ USDValue: string; /** Symbol of the token involved in the transaction */ tokenSymbol: string; /** Contract address of the token */ tokenAddress: string; /** Block number where the transaction was included (optional) */ blockNumber?: number; /** Formatted timestamp of the transaction (optional) */ formattedTime?: string; }These changes would improve code documentation and allow for more flexible use of the interface.
packages/rest-api/src/utils/fetchBridgeTransaction.ts (1)
1-6: Add explicit return type to the function signature.For improved type safety and code readability, consider adding an explicit return type to the
fetchBridgeTransactionfunction. This will make the function's contract clearer and help prevent potential type-related bugs.Here's the suggested change:
export const fetchBridgeTransaction = async ( originChainId: number | string, txHash: string -) => { +): Promise<BridgeTransaction | null> => {This change clearly indicates that the function returns a Promise that resolves to either a
BridgeTransactionobject ornull.packages/rest-api/src/routes/destinationTxRoute.ts (1)
137-151: LGTM with suggestion: 404 response addedThe addition of the 404 response schema aligns with the PR objectives and correctly uses
nullable: trueforfromInfoandtoInfo. This addresses the previous comment about the correct way to indicate nullable properties.To improve consistency and reduce duplication, consider referencing the existing schemas for
fromInfoandtoInfo:* properties: * status: * type: string * fromInfo: -* type: object +* $ref: '#/components/schemas/FromInfo' * nullable: true * toInfo: -* type: object +* $ref: '#/components/schemas/ToInfo' * nullable: trueThis assumes that
FromInfoandToInfoare defined in your components schemas. If not, you should define them there.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (25)
- packages/rest-api/src/controllers/bridgeController.ts (0 hunks)
- packages/rest-api/src/controllers/bridgeLimitsController.ts (0 hunks)
- packages/rest-api/src/controllers/bridgeTxInfoController.ts (0 hunks)
- packages/rest-api/src/controllers/bridgeTxStatusController.ts (0 hunks)
- packages/rest-api/src/controllers/destinationTokensController.ts (0 hunks)
- packages/rest-api/src/controllers/destinationTxController.ts (1 hunks)
- packages/rest-api/src/controllers/indexController.ts (0 hunks)
- packages/rest-api/src/controllers/swapController.ts (0 hunks)
- packages/rest-api/src/controllers/swapTxInfoController.ts (0 hunks)
- packages/rest-api/src/controllers/synapseTxIdController.ts (0 hunks)
- packages/rest-api/src/routes/bridgeLimitsRoute.ts (0 hunks)
- packages/rest-api/src/routes/bridgeRoute.ts (0 hunks)
- packages/rest-api/src/routes/bridgeTxInfoRoute.ts (0 hunks)
- packages/rest-api/src/routes/bridgeTxStatusRoute.ts (0 hunks)
- packages/rest-api/src/routes/destinationTokensRoute.ts (0 hunks)
- packages/rest-api/src/routes/destinationTxRoute.ts (3 hunks)
- packages/rest-api/src/routes/indexRoute.ts (0 hunks)
- packages/rest-api/src/routes/swapRoute.ts (0 hunks)
- packages/rest-api/src/routes/swapTxInfoRoute.ts (0 hunks)
- packages/rest-api/src/routes/synapseTxIdRoute.ts (0 hunks)
- packages/rest-api/src/routes/tokenListRoute.ts (0 hunks)
- packages/rest-api/src/serializers/serializeBridgeInfo.ts (1 hunks)
- packages/rest-api/src/types/index.d.ts (1 hunks)
- packages/rest-api/src/utils/fetchBridgeTransaction.ts (1 hunks)
- packages/rest-api/src/utils/getBridgeStatus.ts (1 hunks)
💤 Files with no reviewable changes (19)
- packages/rest-api/src/controllers/bridgeController.ts
- packages/rest-api/src/controllers/bridgeLimitsController.ts
- packages/rest-api/src/controllers/bridgeTxInfoController.ts
- packages/rest-api/src/controllers/bridgeTxStatusController.ts
- packages/rest-api/src/controllers/destinationTokensController.ts
- packages/rest-api/src/controllers/indexController.ts
- packages/rest-api/src/controllers/swapController.ts
- packages/rest-api/src/controllers/swapTxInfoController.ts
- packages/rest-api/src/controllers/synapseTxIdController.ts
- packages/rest-api/src/routes/bridgeLimitsRoute.ts
- packages/rest-api/src/routes/bridgeRoute.ts
- packages/rest-api/src/routes/bridgeTxInfoRoute.ts
- packages/rest-api/src/routes/bridgeTxStatusRoute.ts
- packages/rest-api/src/routes/destinationTokensRoute.ts
- packages/rest-api/src/routes/indexRoute.ts
- packages/rest-api/src/routes/swapRoute.ts
- packages/rest-api/src/routes/swapTxInfoRoute.ts
- packages/rest-api/src/routes/synapseTxIdRoute.ts
- packages/rest-api/src/routes/tokenListRoute.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rest-api/src/serializers/serializeBridgeInfo.ts
🧰 Additional context used
🪛 Biome
packages/rest-api/src/utils/fetchBridgeTransaction.ts
[error] 53-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (14)
packages/rest-api/src/types/index.d.ts (1)
Line range hint
1-53: Verify the impact of removed BridgeableToken constants.The AI summary mentioned the removal of several
BridgeableTokenconstants (e.g., AGEUR, AVAX, BTCB, etc.). While these removals are not visible in the provided code snippet, it's important to verify that this change doesn't negatively impact other parts of the codebase.Please run the following script to check for any remaining references to the removed constants:
If any references are found, please ensure they are updated or removed as necessary.
packages/rest-api/src/utils/getBridgeStatus.ts (3)
1-8: LGTM: Imports and cache initialization look good.The necessary dependencies are imported, and the provider cache is initialized using a Map. This is a good optimization to avoid creating new providers for each call.
17-22: LGTM: Effective provider caching implementation.The caching logic for the JsonRpcProvider is well-implemented. This addresses the performance concern by reusing provider instances, which is especially beneficial if the
getBridgeStatusfunction is called frequently.
34-38: LGTM: Fast bridge contract initialization is correct.The initialization of the fast bridge contract looks good, using the retrieved
fastBridgeAddressand the appropriate ABI.packages/rest-api/src/controllers/destinationTxController.ts (6)
4-7: LGTM: New imports enhance modularityThe addition of these imports indicates a good separation of concerns. Utility functions and constants are now imported from separate modules, which should improve the overall structure and maintainability of the codebase.
15-16: LGTM: Clear payload initializationInitializing the
payloadvariable as an empty object at the beginning of the function is a good practice. It provides a clear starting point for the response and ensures that the variable is always defined, even if an error occurs early in the function execution.
24-36: LGTM: Proper handling of 'not found' scenarioThe addition of this block improves the API's responsiveness by explicitly handling cases where no bridge transaction is found. Returning a 404 status with a clear "not found" payload is appropriate. The logging before the response is also a good practice for debugging and monitoring purposes.
38-54: LGTM: Clear handling of pending and refunded statesThis block effectively differentiates between 'refunded' and 'pending' states based on the bridge status. The use of the
serializeBridgeInfofunction ensures that thefromInfois consistently formatted in the response. The logic is clear and easy to follow.
56-63: LGTM: Proper handling of completed stateThis block correctly identifies and handles the 'completed' state when both
fromInfoandtoInfoare present. The use ofserializeBridgeInfofor both pieces of information ensures a consistent format in the response payload.
76-80: LGTM: Improved error handlingThis change addresses the security concern raised in a previous review comment about exposing internal error messages to clients. The generic error message is now appropriate for client-facing responses, improving the security of the API.
packages/rest-api/src/routes/destinationTxRoute.ts (4)
37-37: LGTM: Status type change enhances response flexibilityChanging the
statustype frombooleantostringallows for more detailed status information, which aligns with the PR objective of returning arefundedstatus when applicable.
39-65: LGTM: ComprehensivefromInfoobject addedThe new
fromInfoobject provides detailed information about the transaction's origin, enhancing the API's usefulness. All properties are well-defined with appropriate types and descriptions.This addition also resolves the previous comment about the missing
blockNumberproperty.
84-86: LGTM:blockNumberadded totoInfoThe addition of the
blockNumberproperty to thetoInfoobject enhances consistency with thefromInfoobject and provides valuable information about the transaction on the destination chain.
94-112: LGTM: Example response updated to match schema changesThe example response has been correctly updated to reflect all the schema changes, including the string
statusand the new properties in bothfromInfoandtoInfoobjects. This provides a clear illustration of the API response structure.This update also resolves the previous comment about consistency between the schema and example for the
blockNumberproperty.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
packages/rest-api/src/utils/constructBridgeTransactionsQuery.ts (1)
1-46: Enhance documentation and add input validation.The overall structure of the file is clean and focused. Consider the following improvements:
- Add JSDoc comments to document the function's purpose, parameters, and return value.
- Implement input validation for required parameters, especially
useMv.Here's an example of how you could implement these suggestions:
/** * Constructs a GraphQL query string for retrieving bridge transaction data. * * @param {Object} params - The parameters for the query. * @param {boolean} params.useMv - Indicates whether to use a specific mode. * @param {(string|number|null)} [params.originChainId] - The identifier for the originating blockchain. * @param {string|null} [params.txnHash] - The transaction hash. * @param {string|null} [params.kappa] - The kappa value. * @returns {string} The constructed GraphQL query string. * @throws {Error} If required parameters are missing or invalid. */ export const constructBridgeTransactionsQuery = ({ useMv, originChainId, txnHash, kappa, }: { useMv: boolean originChainId?: string | number | null txnHash?: string | null kappa?: string | null }) => { if (typeof useMv !== 'boolean') { throw new Error('useMv must be a boolean'); } // ... rest of the function implementation };These additions will improve the function's documentation and robustness.
packages/rest-api/src/utils/fetchBridgeTransaction.ts (1)
1-13: LGTM! Consider adding input validation.The function signature and imports look good. The use of optional parameters provides flexibility. However, consider adding input validation to ensure at least one of the parameters is provided and they are of the correct type.
Here's a suggestion for input validation:
if (!originChainId && !txnHash && !kappa) { throw new Error('At least one of originChainId, txnHash, or kappa must be provided'); }packages/rest-api/src/controllers/destinationTxController.ts (2)
25-39: LGTM: Improved transaction data handling with a minor suggestion.The destructuring with default values and the explicit check for non-existent transactions are excellent improvements. Returning a 404 status for non-existent transactions is appropriate.
Consider adding error logging for the 404 case:
logger.info(`Successful destinationTxController response`, { query: req.query, payload, }) + logger.warn(`Transaction not found`, { + query: req.query, + }) return res.status(404).json(payload)This will help in monitoring and debugging potential issues with missing transactions.
41-66: LGTM: Improved status determination logic with a suggestion.The status determination logic is clear and covers important scenarios, including the new 'refunded' status. The use of
getBridgeStatusfunction enhances the accuracy of status reporting.Consider handling the case where only
toInfois present:} else { payload = { status: 'pending', fromInfo: serializeBridgeInfo(fromInfo), toInfo, } } + } else if (!fromInfo && toInfo) { + payload = { + status: 'received', + fromInfo, + toInfo: serializeBridgeInfo(toInfo), + } } if (fromInfo && toInfo) {This addition would provide a more complete handling of all possible scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- packages/rest-api/src/constants/index.ts (2 hunks)
- packages/rest-api/src/controllers/bridgeTxStatusController.ts (2 hunks)
- packages/rest-api/src/controllers/destinationTxController.ts (1 hunks)
- packages/rest-api/src/utils/constructBridgeTransactionsQuery.ts (1 hunks)
- packages/rest-api/src/utils/fetchBridgeTransaction.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/rest-api/src/utils/fetchBridgeTransaction.ts
[error] 29-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
packages/rest-api/src/utils/constructBridgeTransactionsQuery.ts (1)
1-11: Well-structured function declaration with clear parameter typing.The function declaration uses modern JavaScript practices with TypeScript typing. The use of object destructuring for parameters and clear type definitions enhances code readability and maintainability.
packages/rest-api/src/controllers/destinationTxController.ts (5)
4-6: LGTM: New imports are well-organized and necessary.The new imports (
getBridgeStatus,BridgeStatus, andfetchBridgeTransaction) are correctly added and align with the refactored functionality of the controller.
15-16: LGTM: Clear payload initialization.Initializing the
payloadvariable as an empty object outside the try-catch block is a good practice. It provides a clear starting point for payload construction and ensures the variable is accessible throughout the function scope.
20-24: LGTM: Improved abstraction for fetching bridge transaction.The use of
fetchBridgeTransactionfunction improves code readability and maintainability by abstracting the GraphQL query logic. The object destructuring in the function call enhances clarity.
67-72: LGTM: Proper logging and response handling.The logging of successful responses with query and payload information is valuable for monitoring and debugging. The JSON response is correctly sent.
79-83: LGTM: Improved error handling for better security.Removing
err.messagefrom the client response is an excellent security improvement. It prevents potential leakage of sensitive information while still maintaining detailed error logging internally.packages/rest-api/src/controllers/bridgeTxStatusController.ts (3)
7-7: Import statement added correctlyThe addition of
fetchBridgeTransactionimport enhances code modularity by leveraging utility functions.
29-31: Refactored fetch logic improves maintainabilityConstructing the
paramsobject and utilizingfetchBridgeTransaction(params)centralizes the transaction fetching logic. This refactoring promotes code reuse and simplifies future maintenance.
33-33: Destructuring 'toInfo' with default value ensures robustnessBy destructuring
toInfofrombridgeTxnwith a default ofnull, the code safely handles cases wherebridgeTxnmight be undefined or lackingtoInfo, preventing potential runtime errors.
refundedstatus alongside response when RFQ transaction has been refunded404when neitherfromInfoortoInfoare founddestinationTxcontroller intoutilsandserializers500response as they exist in loggingSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests