fix(payments): fix missing delay when scheduling in the future #226
Conversation
WalkthroughThe pull request introduces a mechanism to handle delayed payment initiations by adding a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/api/services/payment_initiations_retry.go (1)
6-6: Optional improvement on import grouping.
The explicit import of "time" is acceptable, and there's no group mismatch at the moment. Just ensure it remains consistent with the project's overall import conventions.internal/api/services/payment_initiations_approve.go (1)
42-47: Ensure no negative scheduling intervals.
The code checks if pi.ScheduledAt is after now, so negative intervals are unlikely. However, consider logging or handling edge cases if the clock changes or pi.ScheduledAt is unexpectedly set in the past.internal/api/services/payment_initiations_create.go (1)
32-37: Validate large or negative scheduled intervals.
This logic is clear for computing startDelay when scheduled in the future. As with the other files, you might want to log or handle unexpected edge cases if the time difference is negative or extremely large.internal/connectors/engine/engine.go (2)
485-485: Consider validating large or negative delay.
In your engine’s CreateTransfer implementation, consider logging or rejecting extreme values if the calculated delay is unexpectedly large or negative due to a data or time discrepancy.
604-604: CreatePayout admissions check.
Similarly, before scheduling a payout, consider verifying StartDelay boundaries to avoid accidental indefinite scheduling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/api/services/payment_initiations_approve.go(2 hunks)internal/api/services/payment_initiations_create.go(2 hunks)internal/api/services/payment_initiations_retry.go(2 hunks)internal/connectors/engine/engine.go(5 hunks)
🔇 Additional comments (13)
internal/api/services/payment_initiations_retry.go (3)
44-49: Consistency in attempts counting.
Here, attempts is incremented by one. Confirm that the logic for isReversed and the existing getAttemps function still yields the correct attempt count. Mismatching attempt counts may lead to confusing references in tasks or logs.
50-55: Aligning payout logic with transfer logic.
The new addition of startDelay for payouts is consistent with transfers. Ensure consistent usage and avoid missing checks for negative or unexpectedly large startDelay.
42-43: Potential negative delays check.
You're initializing startDelay to zero. Before setting the final value, consider validating that the derived delay is not negative if future logic (or dynamic time adjustments) modifies pi.ScheduledAt.
Would you like to verify all usage of ScheduledAt across the codebase to ensure negative intervals are never passed to StartDelay?
internal/api/services/payment_initiations_approve.go (3)
6-6: No direct concerns with the import.
The import of "time" is consistent with your usage for startDelay. No changes needed here.
50-50: Separate attempt count from delay.
Currently, you’re using a hardcoded attempt of 1. Confirm if strictly using 1 is intended or if you plan to track attempts similarly to PaymentInitiationsRetry for clarity and consistency.
56-56: Consistent usage of startDelay.
As with transfers, passing the same startDelay for payouts is consistent. Ensure the rest of the code still properly logs or surfaces the delay if debugging is necessary.
internal/api/services/payment_initiations_create.go (3)
5-5: Import usage is consistent.
Importing "time" is valid for the delayed logic. No specific suggestions here.
40-40: Maintain consistent attempts.
Here, attempts is hard-coded to 1. Confirm if you plan on tracking the attempt count or if the single attempt is always intended for newly created payment initiations.
46-46: Similar usage for payouts.
The same reasoning for maintaining consistent attempts applies to CreatePayout. Otherwise, logic is fine.
internal/connectors/engine/engine.go (4)
44-44: Method signature updated: verify usage across codebase.
The new parameter list includes startDelay. Ensure all call sites now pass the correct parameters (particularly attempt vs. startDelay).
48-48: Method signature refactor for payouts.
Similarly, confirm that all references to CreatePayout across the codebase, including test files if any, match this new signature.
512-512: Proper usage of StartDelay in client.StartWorkflowOptions.
Looks correct. Validate that the rest of the workflow logic accounts for the delay (e.g., ensuring deadlines or timeouts can handle the wait).
631-631: Consistency across workflow calls.
Again, usage of StartDelay is consistent. Ensure logs capture the delay so operators can diagnose issues if payments are not proceeding on time.
Fixes ENG-1573