backend/feat: stripe payout with transfer v2#1282
Open
roman-bodavskiy wants to merge 2 commits into
Open
Conversation
WalkthroughThis PR introduces Stripe transfer support to the payout system by decomposing payout order creation into two steps: fund transfer and external payout. It refactors the refund flow to accept structured request types, makes transfer amount and status observable in payout responses, and replaces PayoutStatus with ExternalPayoutStatus. ChangesTransfer-Based Payout and Refund Flow Refactoring
Sequence Diagram(s)sequenceDiagram
participant Client
participant PayoutInterface
participant StripeService
participant StripeTransfer
participant StripeExternalPayout
Client->>PayoutInterface: createPayoutOrder(config, CreatePayoutOrderReq)
PayoutInterface->>PayoutInterface: validate mConnectedAccountId
PayoutInterface->>StripeService: createTransfer(StripeConfig, CreateTransferReq)
StripeService->>StripeTransfer: Stripe.createTransfer
StripeTransfer-->>StripeService: TransferObject
StripeService-->>PayoutInterface: CreateTransferResp
PayoutInterface->>StripeService: createExternalPayout(config, CreateExternalPayoutReq)
StripeService->>StripeExternalPayout: Stripe.createPayout
StripeExternalPayout-->>StripeService: ExternalPayoutResp
StripeService-->>PayoutInterface: CreateExternalPayoutResp
PayoutInterface->>PayoutInterface: merge transfer + external payout
PayoutInterface-->>Client: CreatePayoutOrderResp
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
roman-bodavskiy
force-pushed
the
backend/feat/stripe-payout-with-transfer-v2
branch
from
c6c25a8 to
bb9baf6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/mobility-core/src/Kernel/External/Payment/Stripe/Types/Refund.hs (1)
58-58: 💤 Low valueConsider documenting why currency is now Text instead of Currency.
The
currencyfield was changed from the strongly-typedCurrencyenum toText, which reduces type safety at the JSON deserialization boundary. While the parsing now happens at the interface layer (line 807 inInterface/Stripe.hs), this allows potentially invalid currency values to be accepted during JSON decode.Consider adding a comment explaining this design decision, or verify whether the Stripe API returns currency values that don't map cleanly to the internal
Currencytype.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/mobility-core/src/Kernel/External/Payment/Stripe/Types/Refund.hs` at line 58, Add an explanatory comment above the currency :: Text field in Refund.hs that documents why the type was relaxed from the strong Currency enum to Text (e.g., because Stripe can return values not representable by the internal Currency type or to defer validation to the interface layer), and reference where the parsing/validation now occurs (Interface/Stripe.hs around the parse/validation logic at line ~807); alternatively, if the intent is to preserve type safety, validate and map the Text to Currency during JSON decoding or call out that validation is intentionally performed later in Interface/Stripe.hs.lib/mobility-core/src/Kernel/External/Payment/Interface/Stripe.hs (2)
343-345: 💤 Low valueClarify the default behavior for transferPaymentToConnectedAccount.
The conditional logic at line 343-345:
(transfer_data, application_fee_amount) = case config.transferPaymentToConnectedAccount of Just False -> (Nothing, Nothing) _ -> (Just $ Stripe.TransferData {destination = driverAccountId}, Just $ eurToCents applicationFeeAmount)The
_pattern means that bothNothingandJust Truewill set transfer data. Consider making the default explicit for clarity, or document this behavior if it's intentional.♻️ Suggested clarification
-(transfer_data, application_fee_amount) = case config.transferPaymentToConnectedAccount of - Just False -> (Nothing, Nothing) - _ -> (Just $ Stripe.TransferData {destination = driverAccountId}, Just $ eurToCents applicationFeeAmount) -- True is default +(transfer_data, application_fee_amount) = case fromMaybe True config.transferPaymentToConnectedAccount of + False -> (Nothing, Nothing) + True -> (Just $ Stripe.TransferData {destination = driverAccountId}, Just $ eurToCents applicationFeeAmount)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/mobility-core/src/Kernel/External/Payment/Interface/Stripe.hs` around lines 343 - 345, The pattern matching for transfer_data and application_fee_amount currently treats Nothing and Just True the same via `_`, making the default behavior implicit; update the case on config.transferPaymentToConnectedAccount to explicitly handle Just True and Nothing separately (e.g., add a `Just True -> ...` branch and a `Nothing -> ...` branch) or add a clarifying comment above the case to state the intended default, referencing the symbols transfer_data, application_fee_amount, config.transferPaymentToConnectedAccount, driverAccountId and eurToCents so the behavior is explicit and easy to find.
339-339: ⚖️ Poor tradeoffUse consistent currency conversion functions across all payment operations.
While the function implementations are identical (
usdToCentsandeurToCentsboth performround $ money * 100), the inconsistent usage patterns are problematic for code clarity:
- Lines 339, 345: Platform charge uses
eurToCentsonamountInUsd- Lines 372, 371: Connected account charge uses
usdToCentsonamountInUsd- Line 747: Refund uses
eurToCentsonamountInUsdThis suggests either (1) the actual currencies differ by operation but variable names are misleading, or (2) the function names don't reflect actual usage. Either way, standardize on a single function name across all similar operations to reduce confusion and maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/mobility-core/src/Kernel/External/Payment/Interface/Stripe.hs` at line 339, The code mixes eurToCents and usdToCents while operating on a value named amountInUsd; standardize to a single conversion function (prefer usdToCents if amounts are USD) by replacing all uses of eurToCents with usdToCents in the platform charge, refund, and any other payment flows that accept amountInUsd (e.g., the spots around the platform charge at the let amountInCents = eurToCents amountInUsd, the connected account charge spots using usdToCents, and the refund logic), or if some flows truly use EUR rename the amount/local variables to reflect EUR and keep eurToCents—ensure the chosen function (usdToCents) is the sole converter for USD flows and update callers and tests accordingly.lib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs (1)
40-41: Track webhook and status-field implementation.The TODO and comment note missing webhook handlers (
transfer.created,transfer.reversed,transfer.updated) and the absence of astatusfield inTransferObject. Consider opening an issue to track this work.Do you want me to open a new issue to track the webhook and status-field implementation?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs` around lines 40 - 41, Open a new tracking issue for implementing Stripe transfer webhooks and adding a status field to TransferObject referenced in lib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs; include the TODO text as the issue description, list the specific events to handle (transfer.created, transfer.updated, transfer.reversed), describe that current Transfer API returns an error instead of an object on failure so status is missing, specify required tasks (add status field to TransferObject, implement webhook handlers, add tests and update docs), and tag with backend/payment and priority labels so it can be scheduled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/Payout/Interface/Stripe.hs`:
- Around line 93-121: The createTransfer implementation misnames and mishandles
currency: buildCreateTransferReq uses amountInUsd and eurToCents regardless of
the CreateTransferReq.currency, and usdToCents/eurToCents are identical so
currency semantics are lost; update createTransfer (buildCreateTransferReq and
CreateTransferReq usage) to convert amounts based on the actual currency field
(not always EUR), either by replacing usdToCents/eurToCents with a single
currency-aware converter (e.g., amountToMinorUnits(currency, amount)) or by
switching on currency to call the correct conversion; also rename amountInUsd to
a neutral name (e.g., amount) if the request can be multiple currencies and
mirror the same currency-aware fix in createExternalPayout to keep behavior
consistent.
In `@lib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs`:
- Around line 29-38: The ToForm instance for TransferReq currently omits the
metadata field; update the toForm (ToForm TransferReq) implementation to include
metadata when present by adding an entry to the catMaybes list that encodes the
Metadata into the form map (e.g. produce the appropriate key/value pairs for
each metadata entry and insert them into the HM.fromList used by Form). Locate
the toForm function for TransferReq and, when metadata :: Maybe Metadata is
Just, expand Metadata into the same form representation (using your existing
helpers such as toQueryParam or the pattern used for other nested form fields)
and append those key/value pairs so metadata is sent to Stripe.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/Payment/Interface/Stripe.hs`:
- Around line 343-345: The pattern matching for transfer_data and
application_fee_amount currently treats Nothing and Just True the same via `_`,
making the default behavior implicit; update the case on
config.transferPaymentToConnectedAccount to explicitly handle Just True and
Nothing separately (e.g., add a `Just True -> ...` branch and a `Nothing -> ...`
branch) or add a clarifying comment above the case to state the intended
default, referencing the symbols transfer_data, application_fee_amount,
config.transferPaymentToConnectedAccount, driverAccountId and eurToCents so the
behavior is explicit and easy to find.
- Line 339: The code mixes eurToCents and usdToCents while operating on a value
named amountInUsd; standardize to a single conversion function (prefer
usdToCents if amounts are USD) by replacing all uses of eurToCents with
usdToCents in the platform charge, refund, and any other payment flows that
accept amountInUsd (e.g., the spots around the platform charge at the let
amountInCents = eurToCents amountInUsd, the connected account charge spots using
usdToCents, and the refund logic), or if some flows truly use EUR rename the
amount/local variables to reflect EUR and keep eurToCents—ensure the chosen
function (usdToCents) is the sole converter for USD flows and update callers and
tests accordingly.
In `@lib/mobility-core/src/Kernel/External/Payment/Stripe/Types/Refund.hs`:
- Line 58: Add an explanatory comment above the currency :: Text field in
Refund.hs that documents why the type was relaxed from the strong Currency enum
to Text (e.g., because Stripe can return values not representable by the
internal Currency type or to defer validation to the interface layer), and
reference where the parsing/validation now occurs (Interface/Stripe.hs around
the parse/validation logic at line ~807); alternatively, if the intent is to
preserve type safety, validate and map the Text to Currency during JSON decoding
or call out that validation is intentionally performed later in
Interface/Stripe.hs.
In `@lib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs`:
- Around line 40-41: Open a new tracking issue for implementing Stripe transfer
webhooks and adding a status field to TransferObject referenced in
lib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs; include
the TODO text as the issue description, list the specific events to handle
(transfer.created, transfer.updated, transfer.reversed), describe that current
Transfer API returns an error instead of an object on failure so status is
missing, specify required tasks (add status field to TransferObject, implement
webhook handlers, add tests and update docs), and tag with backend/payment and
priority labels so it can be scheduled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab7b75e5-aa81-4388-a149-588be858ec6f
📒 Files selected for processing (16)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/Payment/Interface.hslib/mobility-core/src/Kernel/External/Payment/Interface/Stripe.hslib/mobility-core/src/Kernel/External/Payment/Interface/Types.hslib/mobility-core/src/Kernel/External/Payment/Stripe/Config.hslib/mobility-core/src/Kernel/External/Payment/Stripe/Types/PaymentIntent.hslib/mobility-core/src/Kernel/External/Payment/Stripe/Types/Refund.hslib/mobility-core/src/Kernel/External/Payout/Interface.hslib/mobility-core/src/Kernel/External/Payout/Interface/Juspay.hslib/mobility-core/src/Kernel/External/Payout/Interface/Stripe.hslib/mobility-core/src/Kernel/External/Payout/Interface/Types.hslib/mobility-core/src/Kernel/External/Payout/Stripe/Config.hslib/mobility-core/src/Kernel/External/Payout/Stripe/Flow.hslib/mobility-core/src/Kernel/External/Payout/Stripe/Types.hslib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Payout.hslib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs
Comment on lines
+93
to
+121
| createTransfer :: | ||
| forall m r. | ||
| ( Metrics.CoreMetrics m, | ||
| EncFlow m r, | ||
| HasRequestId r, | ||
| MonadReader r m | ||
| ) => | ||
| StripeConfig -> | ||
| CreateTransferReq -> | ||
| m CreateTransferResp | ||
| createTransfer config req = do | ||
| let url = config.url | ||
| apiKey <- decrypt config.apiKey | ||
| transferReq <- buildCreateTransferReq req | ||
| let senderAccountId = case req.senderAccountId of | ||
| TransferConnectedAccount accountId -> Just accountId | ||
| TransferPlatformAccount -> Nothing | ||
| mkCreateTransferResp <$> Stripe.createTransfer url apiKey senderAccountId transferReq | ||
| where | ||
| buildCreateTransferReq :: CreateTransferReq -> m Stripe.TransferReq | ||
| buildCreateTransferReq CreateTransferReq {amount = amountInUsd, ..} = do | ||
| let amountInCents = eurToCents amountInUsd | ||
| destination <- case destinationAccount of | ||
| TransferConnectedAccount accountId -> pure accountId | ||
| TransferPlatformAccount -> config.platformAccountId & fromMaybeM (InternalError "STRIPE_PLATFORM_ACCOUNT_ID_NOT_FOUND") | ||
| pure Stripe.TransferReq {amount = amountInCents, metadata = Nothing, currency = T.toLower $ show currency, ..} | ||
|
|
||
| mkCreateTransferResp :: Stripe.TransferObject -> CreateTransferResp | ||
| mkCreateTransferResp Stripe.TransferObject {..} = CreateTransferResp {transferId = id, transferStatus = TRANSFERRED} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check currency field usage and conversion patterns in payout/transfer code
# Search for currency conversion functions and their usage context
rg -n -C3 'usdToCents|eurToCents|centsToUsd' --type=hs -g '*Payout*' -g '*Payment*'
# Find where currency field from request is used
rg -n -C3 'currency.*=' --type=hs -g '*Payout/Interface*'Repository: nammayatri/shared-kernel
Length of output: 217
🏁 Script executed:
cat -n lib/mobility-core/src/Kernel/External/Payout/Interface/Stripe.hs | head -150Repository: nammayatri/shared-kernel
Length of output: 5726
🏁 Script executed:
rg -n 'createExternalPayout|usdToCents|eurToCents' --type=hsRepository: nammayatri/shared-kernel
Length of output: 2804
🏁 Script executed:
fd -e hs | xargs grep -l 'createExternalPayout' 2>/dev/null | head -5Repository: nammayatri/shared-kernel
Length of output: 191
🏁 Script executed:
rg -n 'data CreateTransferReq|data CreatePayoutOrderReq' --type=hs -A 10Repository: nammayatri/shared-kernel
Length of output: 3293
🏁 Script executed:
rg -n 'type.*CreateTransferReq\|newtype.*CreateTransferReq' --type=hs -A 5Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
fd -e hs | xargs grep -l 'CreateTransferReq' | head -3Repository: nammayatri/shared-kernel
Length of output: 255
🏁 Script executed:
cat -n lib/mobility-core/src/Kernel/External/Payout/Interface/Types.hs | sed -n '56,75p; 144,155p'Repository: nammayatri/shared-kernel
Length of output: 1331
🏁 Script executed:
rg -n 'Currency' --type=hs lib/mobility-core/src/Kernel/External/Payout/Interface/Types.hs | head -10Repository: nammayatri/shared-kernel
Length of output: 127
🏁 Script executed:
rg -n 'buildCreateTransferReq\|CreateTransferReq' --type=hs lib/mobility-core/src/Kernel/External/Payout/ -B 2 -A 8Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
rg -n 'data Currency|newtype Currency' --type=hs -A 15Repository: nammayatri/shared-kernel
Length of output: 1768
🏁 Script executed:
rg -n 'createTransfer\|TransferReq' --type=hs lib/mobility-core/src/Kernel/External/Payout/ -B 3 -A 6Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
rg -n 'Stripe.createTransfer' --type=hs -B 5 -A 5Repository: nammayatri/shared-kernel
Length of output: 3378
🏁 Script executed:
cat -n lib/mobility-core/src/Kernel/External/Payment/Interface/Stripe.hs | sed -n '550,560p'Repository: nammayatri/shared-kernel
Length of output: 472
Address inconsistent currency conversion and misleading parameter naming in createTransfer.
Both createExternalPayout and createTransfer have a currency field in their request types that supports INR, USD, and EUR, but both ignore it when converting amounts to cents:
createExternalPayoutusesusdToCents(line 39)createTransferuseseurToCents(line 114) despite the parameter being namedamountInUsd
Additionally, usdToCents and eurToCents have identical implementations (round $ money * 100), making the semantic distinction meaningless. While this works for USD and EUR (both use 2 decimal places), it will be incorrect if INR is actually used in production (INR uses 0 decimal places).
Recommend either:
- Use a single conversion function for consistency, or
- Make the conversion function respect the actual
currencyfield from the request, or - If the currency is always USD/EUR, remove the currency field or document why it's ignored
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/External/Payout/Interface/Stripe.hs` around
lines 93 - 121, The createTransfer implementation misnames and mishandles
currency: buildCreateTransferReq uses amountInUsd and eurToCents regardless of
the CreateTransferReq.currency, and usdToCents/eurToCents are identical so
currency semantics are lost; update createTransfer (buildCreateTransferReq and
CreateTransferReq usage) to convert amounts based on the actual currency field
(not always EUR), either by replacing usdToCents/eurToCents with a single
currency-aware converter (e.g., amountToMinorUnits(currency, amount)) or by
switching on currency to call the correct conversion; also rename amountInUsd to
a neutral name (e.g., amount) if the request can be multiple currencies and
mirror the same currency-aware fix in createExternalPayout to keep behavior
consistent.
Comment on lines
+29
to
+38
| instance ToForm TransferReq where | ||
| toForm TransferReq {..} = | ||
| Form $ | ||
| HM.fromList $ | ||
| catMaybes | ||
| [ Just . ("amount",) . pure $ toQueryParam amount, | ||
| Just . ("currency",) . pure $ toQueryParam currency, | ||
| Just . ("destination",) . pure $ toQueryParam destination, | ||
| ("description",) . pure . toQueryParam <$> description | ||
| ] |
There was a problem hiding this comment.
Include metadata field in the ToForm encoding.
The TransferReq record includes a metadata :: Maybe Metadata field (Line 24), but the ToForm instance does not encode it into the form data. If metadata should be sent to Stripe, add it to the catMaybes list; otherwise, remove the field from the record.
🔧 Proposed fix to include metadata in form encoding
instance ToForm TransferReq where
toForm TransferReq {..} =
Form $
HM.fromList $
catMaybes
[ Just . ("amount",) . pure $ toQueryParam amount,
Just . ("currency",) . pure $ toQueryParam currency,
Just . ("destination",) . pure $ toQueryParam destination,
+ ("metadata",) . pure . toQueryParam <$> metadata,
("description",) . pure . toQueryParam <$> description
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| instance ToForm TransferReq where | |
| toForm TransferReq {..} = | |
| Form $ | |
| HM.fromList $ | |
| catMaybes | |
| [ Just . ("amount",) . pure $ toQueryParam amount, | |
| Just . ("currency",) . pure $ toQueryParam currency, | |
| Just . ("destination",) . pure $ toQueryParam destination, | |
| ("description",) . pure . toQueryParam <$> description | |
| ] | |
| instance ToForm TransferReq where | |
| toForm TransferReq {..} = | |
| Form $ | |
| HM.fromList $ | |
| catMaybes | |
| [ Just . ("amount",) . pure $ toQueryParam amount, | |
| Just . ("currency",) . pure $ toQueryParam currency, | |
| Just . ("destination",) . pure $ toQueryParam destination, | |
| ("metadata",) . pure . toQueryParam <$> metadata, | |
| ("description",) . pure . toQueryParam <$> description | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/External/Payout/Stripe/Types/Transfer.hs` around
lines 29 - 38, The ToForm instance for TransferReq currently omits the metadata
field; update the toForm (ToForm TransferReq) implementation to include metadata
when present by adding an entry to the catMaybes list that encodes the Metadata
into the form map (e.g. produce the appropriate key/value pairs for each
metadata entry and insert them into the HM.fromList used by Form). Locate the
toForm function for TransferReq and, when metadata :: Maybe Metadata is Just,
expand Metadata into the same form representation (using your existing helpers
such as toQueryParam or the pattern used for other nested form fields) and
append those key/value pairs so metadata is sent to Stripe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
New Features
Refactor