Add more_info_url to transaction lookup response#158
Conversation
|
@Lynndabel Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request extends the transaction lookup API by introducing an optional Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the /deposit endpoint to conditionally include a more_info_url in the response, which is only present when server.interactiveDomain is configured. The changes also include an updated test case to assert the presence of this new URL. Feedback suggests refactoring the response data construction for improved type safety and maintainability by avoiding any and extracting URL components. Additionally, a new test case is recommended to cover the scenario where server.interactiveDomain is not configured, ensuring more_info_url is correctly omitted.
| const serverConfig = this.config.get('server'); | ||
| const responseData: any = { | ||
| id: transaction.id, | ||
| kind: transaction.kind, | ||
| status: transaction.status, | ||
| amount: transaction.amount, | ||
| asset_code: transaction.assetCode, | ||
| account: transaction.account, | ||
| interactive_url: `${this.config.get('server').interactiveDomain ?? 'http://localhost:3000'}/deposit/${transaction.id}`, | ||
| interactive_url: `${serverConfig.interactiveDomain ?? 'http://localhost:3000'}/deposit/${transaction.id}`, | ||
| created_at: transaction.createdAt, | ||
| updated_at: transaction.updatedAt, | ||
| }); | ||
| }; | ||
|
|
||
| // Add more_info_url only when interactive domain is configured | ||
| if (serverConfig.interactiveDomain) { | ||
| responseData.more_info_url = `${serverConfig.interactiveDomain}/deposit/${transaction.id}`; | ||
| } |
There was a problem hiding this comment.
To improve maintainability and type safety, I suggest a small refactor here:
- Avoid using
anyforresponseData.Record<string, unknown>is a more type-safe alternative that aligns with thesendJsonfunction's signature. - Extract the URL components into constants to avoid repetition and make the code DRY (Don't Repeat Yourself). This makes it easier to update the URL structure in the future.
| const serverConfig = this.config.get('server'); | |
| const responseData: any = { | |
| id: transaction.id, | |
| kind: transaction.kind, | |
| status: transaction.status, | |
| amount: transaction.amount, | |
| asset_code: transaction.assetCode, | |
| account: transaction.account, | |
| interactive_url: `${this.config.get('server').interactiveDomain ?? 'http://localhost:3000'}/deposit/${transaction.id}`, | |
| interactive_url: `${serverConfig.interactiveDomain ?? 'http://localhost:3000'}/deposit/${transaction.id}`, | |
| created_at: transaction.createdAt, | |
| updated_at: transaction.updatedAt, | |
| }); | |
| }; | |
| // Add more_info_url only when interactive domain is configured | |
| if (serverConfig.interactiveDomain) { | |
| responseData.more_info_url = `${serverConfig.interactiveDomain}/deposit/${transaction.id}`; | |
| } | |
| const serverConfig = this.config.get('server'); | |
| const interactiveDomain = serverConfig.interactiveDomain; | |
| const depositUrlPath = `/deposit/${transaction.id}`; | |
| const responseData: Record<string, unknown> = { | |
| id: transaction.id, | |
| kind: transaction.kind, | |
| status: transaction.status, | |
| amount: transaction.amount, | |
| asset_code: transaction.assetCode, | |
| account: transaction.account, | |
| interactive_url: `${interactiveDomain ?? 'http://localhost:3000'}${depositUrlPath}`, | |
| created_at: transaction.createdAt, | |
| updated_at: transaction.updatedAt, | |
| }; | |
| // Add more_info_url only when interactive domain is configured | |
| if (interactiveDomain) { | |
| responseData.more_info_url = `${interactiveDomain}${depositUrlPath}`; | |
| } |
| expect(response.body.more_info_url).toBe( | ||
| `https://anchor.example.com/deposit/${transactionId}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This is a good test for the happy path. However, the test suite is missing coverage for the case where server.interactiveDomain is not configured. According to the logic, more_info_url should be omitted in that scenario. Please consider adding a new test case to verify this behavior and ensure robustness.
|
@Lynndabel Please fix CI |
|
@0xNgoo approve my workflow so i can see the error please |
|
Please fix ci |
2 participants
Adds more_info_url to successful GET /transactions/:id lookup responses when server.interactiveDomain is configured. The URL points to the same interactive deposit status page the SDK already builds (e.g. https://anchor.example.com/deposit/:id). When no interactive domain is configured, more_info_url is omitted from the response.
Closes #92
How to test?
Automated
Run:
bash
bun test tests/mvp-express.integration.test.ts
Run full suite:
bash
bun test
Run lint:
bash
bun run lint
Manual verification (optional)
Start your app that mounts the router with server.interactiveDomain set.
Create an interactive deposit, then lookup the transaction:
GET /transactions/:id
Confirm the JSON includes:
interactive_url
more_info_url (same /deposit/:id page, based on interactiveDomain)
Repeat with interactiveDomain unset and confirm more_info_url is not present.
Checklist
My code follows the code style of this project.
I have added tests for my changes.
I have updated the documentation accordingly. (not needed for this narrow change)
I have run bun run test and bun run lint locally.
Issue Reference
Closes #92