fix: return funds to original sources when voiding holds #46
Conversation
WalkthroughThis pull request updates how transaction data is structured across several components and tests. The changes primarily transition the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Manager
participant Ledger
participant ScriptBuilder
Client->>Manager: Request to void hold
Manager->>Ledger: List transactions for hold
Ledger-->>Manager: Return transaction list
alt Exactly one transaction found
Manager->>ScriptBuilder: BuildCancelHoldScript(postings)
ScriptBuilder-->>Manager: Return cancel hold script
Manager->>Ledger: Submit void hold transaction
Ledger-->>Manager: Confirmation
Manager-->>Client: Success response
else Transaction count != 1
Manager-->>Client: Error ("expected 1 transaction")
end
Suggested reviewers
Poem
✨ 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 (
|
![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)
pkg/scripts.go (1)
54-59: Add validation for postings parameter.Consider adding validation to ensure postings is not empty to prevent potential runtime errors.
func BuildCancelHoldScript(asset string, postings ...shared.V2Posting) string { + if len(postings) == 0 { + panic("postings cannot be empty") + } return renderTemplate(CancelHoldScript, map[string]any{ "Asset": asset, "Postings": postings, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (10)
pkg/api/handler_holds_confirm_test.go(3 hunks)pkg/api/handler_holds_void_test.go(1 hunks)pkg/api/handler_transactions_list_test.go(2 hunks)pkg/api/handler_wallets_credit_test.go(5 hunks)pkg/api/handler_wallets_debit_test.go(9 hunks)pkg/manager.go(7 hunks)pkg/numscript/cancel-hold.num(1 hunks)pkg/scripts.go(2 hunks)pkg/transaction.go(1 hunks)test/e2e/debit_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (22)
pkg/numscript/cancel-hold.num (1)
7-12: LGTM! Improved hold cancellation logic.The new implementation correctly returns funds to their original sources by:
- Iterating over postings to handle multiple sources
- Using max operation to ensure correct amount is returned
- Safely handling any remaining funds
pkg/transaction.go (2)
9-12: LGTM! Simplified transaction structure.The change to
V2Transactionprovides a cleaner structure while maintaining all necessary functionality.
14-25: LGTM! MarshalJSON updated correctly.The MarshalJSON method has been properly updated to use V2Transaction while preserving the ledger field.
pkg/api/handler_holds_void_test.go (2)
48-69: LGTM! Comprehensive test coverage for transaction listing.The mock implementation correctly provides the original transaction data needed for returning funds to their sources.
70-94: LGTM! Thorough validation of hold cancellation.The test verifies that funds are correctly returned to both secondary and main balance accounts with proper amounts and assets.
pkg/api/handler_transactions_list_test.go (1)
66-67: LGTM! Clean type simplification.The change from
V2ExpandedTransactiontoV2Transactionmaintains all necessary fields while simplifying the data structure.Also applies to: 89-90
pkg/api/handler_wallets_credit_test.go (1)
45-48: LGTM! Consistent simplification of transaction data structure.The changes streamline the
Varsmap by using a simpler string format for amounts (e.g., "USD 100") across all test cases, making the code more maintainable and consistent.Also applies to: 72-75, 95-98, 114-117, 142-145
pkg/api/handler_holds_confirm_test.go (1)
47-51: LGTM! Consistent hold transaction data structure.The changes maintain the same simplified string format for amounts in hold confirmation tests, ensuring consistency across the codebase.
Also applies to: 99-103, 231-236
test/e2e/debit_test.go (1)
51-126: Great addition of test coverage for secondary balance operations!The new test cases effectively verify that:
- Funds can be debited from a secondary balance using holds
- Voiding a hold correctly returns funds to the original secondary balance
- Balance amounts are accurately tracked before and after voiding holds
This directly supports the PR objective of fixing fund returns to original sources when voiding holds.
pkg/api/handler_wallets_debit_test.go (8)
58-61: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
77-80: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
97-100: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
116-119: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
152-155: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
174-177: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
193-196: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
233-236: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
pkg/manager.go (5)
166-169: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
207-211: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
284-287: LGTM! The amount format change is consistent.The simplified string format for the amount variable improves readability and reduces complexity.
405-409: LGTM! Simplified transaction data structure.The change from
shared.V2ExpandedTransactiontoshared.V2Transactionaligns with the overall effort to simplify data structures.
232-241:❓ Verification inconclusive
Improved error handling and fund return logic.
The changes enhance the robustness of the
VoidHoldmethod by:
- Adding validation to ensure exactly one transaction is retrieved.
- Iterating over postings to return funds to their original sources.
These changes align with the PR objective and improve error handling.
Run the following script to verify the impact of these changes:
Also applies to: 255-255
🏁 Script executed:
#!/bin/bash # Description: Verify the usage of postings in the cancel hold script. # Test: Search for the BuildCancelHoldScript function to understand how postings are used. ast-grep --pattern $'func BuildCancelHoldScript($_, $$$) { $$$ }'Length of output: 68
Please verify further the BuildCancelHoldScript usage.
The ast-grep command produced no output, which is inconclusive—it may indicate that the expected function pattern isn’t present or is defined differently. I recommend running an additional script (e.g., using ripgrep) to search for "BuildCancelHoldScript" across the codebase for further confirmation of its usage in the cancel hold logic.
- Verify that the BuildCancelHoldScript function exists and that its implementation correctly handles postings.
- Confirm that the changes in error handling and iteration over postings align with the intended functionality in the cancel hold process.
No description provided.