fix: add missing network value in conversion requests #1567
Conversation
WalkthroughThe changes modify the Changes
Suggested Reviewers
✨ Finishing Touches
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 (
|
github-actions
bot
requested review from
alexandre-abrioux,
kevindavee,
leoslr and
yomarion
![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
🧹 Nitpick comments (1)
packages/request-client.js/src/api/request-network.ts (1)
612-615: Add JSDoc comments for the new feeBalance structure.While the implementation is correct, adding documentation would help explain the purpose and structure of the new feeBalance object.
+ /** Fee balance tracking + * @property {Array} events - List of fee-related events + * @property {string} balance - Current fee balance as string + */ feeBalance: { events: [], balance: '0', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/request-client.js/src/api/request-network.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/request-client.js/src/api/request-network.ts (1)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1386
File: packages/request-client.js/src/api/request-network.ts:74-145
Timestamp: 2024-11-12T14:52:38.580Z
Learning: The data passed to the `preparePaymentRequest` method in the `RequestNetwork` class is reliably formatted by the transaction manager, negating the need for additional error handling for data formatting within this method.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
packages/request-client.js/src/api/request-network.ts (3)
608-612: LGTM! Network value correctly added to extension values.The implementation properly adds the network value from extension parameters, which addresses the PR objective of fixing missing network value in conversion requests.
631-631: LGTM! Currency value correctly updated.The currency value is now properly sourced from
requestData.parameters.currency.value, which aligns with the changes described in the AI summary.
626-630: Verify escrow events usage and add documentation.The new balance structure includes escrowEvents, but its usage should be verified across the codebase.
Consider adding documentation:
+ /** Request balance information + * @property {string} balance - Current balance as string + * @property {Array} events - List of balance-related events + * @property {Array} escrowEvents - List of escrow-related events + */ balance: { balance: '0', events: [], escrowEvents: [], },✅ Verification successful
✓ escrowEvents usage verified - properly integrated across the codebase
The escrowEvents array is an integral part of the payment detection system, with proper typing, test coverage, and consistent usage across packages. While additional documentation would be helpful, it's not critical given the comprehensive implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for escrow-related functionality to verify the usage of escrowEvents echo "Searching for escrow-related code..." rg -l "escrow" --type ts # Search specifically for escrowEvents usage echo "Searching for escrowEvents usage..." ast-grep --pattern 'escrowEvents'Length of output: 3818
|
Do we need to update any unit tests or add a new one to verify the fields? |
aimensahnoun
deleted the
1566-request-sdk-inmemory-request-are-missing-the-network-field-in-the-values-of-extensions
branch
Resolves #1566
Issue:
InMemory requests data was missing some fields necessary for conversion payments.
Fix:
Add network to extension values.
Summary by CodeRabbit