RFQ Indexer: adding search by origin tx #3277
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: |
89f6ac4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://567918d9.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-search-by-txhash.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3277 +/- ##
===================================================
+ Coverage 30.65718% 31.73701% +1.07983%
===================================================
Files 447 427 -20
Lines 29794 28503 -1291
Branches 82 82
===================================================
- Hits 9134 9046 -88
+ Misses 19809 18609 -1200
+ Partials 851 848 -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: 0
🧹 Outside diff range and nitpick comments (1)
packages/rfq-indexer/api/src/routes/transactionIdRoute.ts (1)
11-11: LGTM! The changes align with the PR objectives.The updates to the OpenAPI documentation accurately reflect the new functionality of searching by either transaction ID or origin transaction hash. This enhances the clarity of the API documentation for users.
Consider adding a brief explanation of the difference between a transaction ID and an origin transaction hash in the description. This could help API users understand when to use each identifier.
* name: transactionId * required: true * schema: * type: string -* description: The unique identifier of the transaction or the origin transaction hash +* description: The unique identifier of the transaction or the origin transaction hash. Use transaction ID for internal transactions and origin transaction hash for cross-chain transactions.Also applies to: 19-19
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rfq-indexer/api/src/controllers/transactionIdController.ts (1 hunks)
- packages/rfq-indexer/api/src/routes/transactionIdRoute.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/rfq-indexer/api/src/controllers/transactionIdController.ts (1)
13-18: Enhance function clarity and robustnessThe implementation of the new search capability by both
transactionIdandtransactionHashis a good addition that aligns with the PR objectives. However, there are several points to consider for improving clarity, robustness, and maintainability:
Consider renaming the function and parameter to reflect the dual functionality. For example:
export const getTransactionByIdOrHash = async (req: Request, res: Response) => { const { transactionIdOrHash } = req.params // ... }Add input validation to ensure the provided
transactionIdOrHashis in the correct format for either a transaction ID or a transaction hash. This will help prevent potential errors and improve the function's robustness.Be aware of potential performance implications. Ensure that appropriate indexes are in place on both
transactionIdandtransactionHashcolumns to maintain query efficiency.Update the OpenAPI documentation for this endpoint to clearly indicate that it now accepts either a transaction ID or a transaction hash.
Consider adding a comment in the code explaining the dual functionality for better maintainability.
To verify the impact on query performance and ensure proper indexing, you can run the following script:
This script will help identify if proper indexes are in place and provide insights into the query execution plan.
There was a problem hiding this comment.
looks great - i like the approach where its the same single input param & just checks both. I think this could even be expanded to just search for that hash anywhere -- transactionId, deposit, relay, proof, claim, dispute.
Another edge case catch is that unlike transactionId, a single transaction hash can technically repeat on multiple bridges. (EG: a single transaction can have any number of bridge deposits --- or relays, proofs, etc)
Not sure what current expected behavior of this endpoint would be in that case, but its probably best if it returns all of the matches, even if they are multiple bridges.
Its such an edge case tho right now -- I think this is good as-is & we can always do more later!
Adding search by origin txn hash and not just transactionID.
Summary by CodeRabbit
/transaction-id/{transactionId}endpoint.