Conversation
…to financial-gateway Task 2: Add platform_fee_minor_units field to DispatchPaymentResponse proto. Wire it through from Stripe adapter to gRPC response and Starlark client (previously hardcoded to 0). Task 3: Add PaymentRefundedEvent and PaymentDisputedEvent proto messages. Map REFUNDED and DISPUTED webhook statuses to domain events published to their respective Kafka topics. Topics already existed in topics.go. Task 4: Implement CancelPayment RPC with testable PaymentIntentCanceller and PaymentIntentResolver interfaces. Handles idempotency for already-cancelled payments. Replaces the Unimplemented stub.
Contributor
📝 WalkthroughWalkthroughAdds platform_fee_minor_units to dispatch responses, introduces PaymentRefundedEvent and PaymentDisputedEvent protos, maps Stripe REFUNDED/DISPUTED webhooks to domain events, implements PaymentIntent cancellation (interfaces, CancelPayment, error mapping) in the Stripe adapter and wires cancellation through the gRPC service with tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as FinancialGatewayService
participant ClientFactory
participant Adapter as StripeAdapter
participant Resolver as PaymentIntentResolver
participant Canceller as PaymentIntentCanceller
participant Stripe
Client->>Service: CancelPayment(paymentOrderId, reason)
Service->>ClientFactory: create tenant-scoped Stripe client
ClientFactory-->>Service: Stripe client (AccountID)
Service->>Service: inject Stripe account into ctx
Service->>Adapter: CancelPayment(ctx, paymentOrderId, reason)
Adapter->>Resolver: FindByPaymentOrderID(paymentOrderId)
Resolver-->>Adapter: paymentIntentId
Adapter->>Canceller: Cancel(ctx, paymentIntentId, params)
Canceller->>Stripe: Cancel PaymentIntent
Stripe-->>Canceller: PaymentIntent (status)
Canceller-->>Adapter: PaymentIntent
Adapter->>Adapter: map status -> DispatchStatus
Adapter-->>Service: CancelResult
Service->>Service: mapCancelError or success
Service-->>Client: CancelPaymentResponse(status, reason)
sequenceDiagram
participant StripeWebhook
participant Handler as WebhookHandler
participant TopicMapper
participant Outbox
participant DomainEvent
StripeWebhook->>Handler: webhook payload (REFUNDED / DISPUTED)
Handler->>Handler: parse & mapToDomainEvent(status)
alt REFUNDED
Handler->>DomainEvent: create PaymentRefundedEvent
else DISPUTED
Handler->>DomainEvent: create PaymentDisputedEvent
end
Handler->>TopicMapper: determine topic
Handler->>Outbox: publish event to topic
Outbox-->>Handler: acknowledgment
Handler-->>StripeWebhook: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Claude Code ReviewCommit: SummarySolid implementation of three related financial-gateway features: platform fee wiring, refund/dispute webhook events, and CancelPayment RPC. The code follows established patterns well — event mapping mirrors existing SETTLED/REJECTED handlers, the adapter/service layering is clean, and error handling is thorough. Previous review findings (overly broad 400 handling and discarded cancel result) have been fully addressed. Risk Assessment
Findings
Bot Review NotesCodeRabbit thread 1 ( CodeRabbit thread 2 ( CodeRabbit thread 3 ( Previously Flagged
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…g result Address review feedback: - Only treat "status of canceled" Stripe 400 as idempotent success. Succeeded/captured payments now correctly return an error. - Log the CancelResult in grpc_service instead of discarding it. - Add test for already-succeeded cancel attempt returning error.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/financial-gateway/adapters/http/webhook_handler.go (1)
191-194:⚠️ Potential issue | 🟡 MinorStale comment examples after this change.
The comment cites REFUNDED and DISPUTED as examples of events without domain mappings, but this PR adds mappings for both. Consider updating the examples to reflect current behavior.
📝 Suggested fix
if errors.As(err, &noMapping) { - // Events with no domain event mapping (e.g., REFUNDED, DISPUTED) are - // acknowledged to prevent Stripe retries. Full handling is out of scope here. + // Events with no domain event mapping are acknowledged to prevent + // Stripe retries. h.writeSuccess(w, "event acknowledged")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/financial-gateway/adapters/http/webhook_handler.go` around lines 191 - 194, Update the stale comment above the early-ack path in webhook_handler.go (the block that calls h.writeSuccess(w, "event acknowledged")) to remove or replace the examples REFUNDED and DISPUTED since this PR added domain mappings for them; edit the comment in the function handling Stripe events (the code path that returns after h.writeSuccess) to either list correct unmapped event examples or simply state "events with no domain event mapping" without specific examples so the comment stays accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/proto/meridian/financial_gateway_events/v1/events.proto`:
- Around line 121-125: The new event field provider_reference_id in this proto
is being repurposed to mean a Stripe Charge ID but is already used as the Stripe
PaymentIntent ID in PaymentCapturedEvent and exposed upstream via
DispatchPaymentResponse, which will break consumers; update the proto so
provider_reference_id continues to carry the PaymentIntent ID (same semantic as
PaymentCapturedEvent and DispatchPaymentResponse) or, if you need to record the
Charge ID, add a new distinct field like charge_id or provider_charge_id instead
of reusing provider_reference_id; ensure the change is applied for all affected
messages (including the other occurrence mentioned) so consumers can reliably
correlate lifecycle events.
In `@services/financial-gateway/service/grpc_service.go`:
- Around line 214-228: The cancel error mapper mapCancelError currently returns
codes.Internal for unknown errors; instead delegate to the existing
upstream/provider mapping by returning the result of mapStripeError(err) in the
default branch (so cancel-path network/provider/dispatch failures become
codes.Unavailable like mapStripeError does) while preserving the existing
specific cases (stripeadapter.ErrCancelNotConfigured,
stripeadapter.ErrPaymentIntentNotFound, stripeadapter.ErrMissingStripeAccount,
context.Canceled, context.DeadlineExceeded).
---
Outside diff comments:
In `@services/financial-gateway/adapters/http/webhook_handler.go`:
- Around line 191-194: Update the stale comment above the early-ack path in
webhook_handler.go (the block that calls h.writeSuccess(w, "event
acknowledged")) to remove or replace the examples REFUNDED and DISPUTED since
this PR added domain mappings for them; edit the comment in the function
handling Stripe events (the code path that returns after h.writeSuccess) to
either list correct unmapped event examples or simply state "events with no
domain event mapping" without specific examples so the comment stays accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3bab05f9-65ff-435e-b2f6-7c174ac5a503
📒 Files selected for processing (10)
api/proto/meridian/financial_gateway/v1/financial_gateway.protoapi/proto/meridian/financial_gateway_events/v1/events.protoservices/financial-gateway/adapters/http/webhook_handler.goservices/financial-gateway/adapters/http/webhook_handler_test.goservices/financial-gateway/adapters/stripe/payment_intent_adapter.goservices/financial-gateway/adapters/stripe/payment_intent_adapter_test.goservices/financial-gateway/client/starlark.goservices/financial-gateway/e2e/e2e_test.goservices/financial-gateway/service/grpc_service.goservices/financial-gateway/service/grpc_service_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
services/financial-gateway/service/grpc_service.go (1)
219-235:⚠️ Potential issue | 🟠 MajorTransient cancel-provider failures still reported as
Internal.The past review comment about this issue remains valid. The default branch returns
codes.Internal, but provider failures (network issues, Stripe API errors) should returncodes.Unavailableto matchmapStripeError's behavior and enable proper client retry logic.
Internalsignals a server bug, whileUnavailableindicates temporary provider issues that may succeed on retry.🔧 Proposed fix
func mapCancelError(err error) error { switch { case errors.Is(err, stripeadapter.ErrCancelNotConfigured): return status.Error(codes.Unimplemented, "cancel support not configured") case errors.Is(err, stripeadapter.ErrPaymentIntentNotFound): return status.Error(codes.NotFound, "payment intent not found for payment order") case errors.Is(err, stripeadapter.ErrMissingStripeAccount): return status.Error(codes.FailedPrecondition, "stripe connected account not configured") case errors.Is(err, context.Canceled): return status.Error(codes.Canceled, "request canceled") case errors.Is(err, context.DeadlineExceeded): return status.Error(codes.DeadlineExceeded, "request deadline exceeded") default: - return status.Error(codes.Internal, "failed to cancel payment") + return status.Error(codes.Unavailable, "stripe cancel failed") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/financial-gateway/service/grpc_service.go` around lines 219 - 235, The mapCancelError function currently maps unknown/provider failures to codes.Internal; change its default branch to return codes.Unavailable (matching mapStripeError behavior) so transient provider/network/Stripe API errors are reported as Unavailable for proper client retries; update the default return in mapCancelError to use status.Error(codes.Unavailable, "failed to cancel payment") and ensure any tests or callers that expect Internal are adjusted accordingly.
🧹 Nitpick comments (1)
services/financial-gateway/adapters/stripe/payment_intent_adapter.go (1)
318-322: Consider: Thereasonparameter is only used as a presence check.The method accepts a
reasonstring but discards its value, always using"requested_by_customer". This might be intentional since Stripe only accepts specific enum values forCancellationReason, but it could confuse callers who expect their reason to be forwarded.Consider either:
- Documenting this behavior in the method's doc comment
- Mapping the input
reasonto valid Stripe cancellation reasons- Changing the parameter to a boolean or enum if only presence matters
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/financial-gateway/adapters/stripe/payment_intent_adapter.go` around lines 318 - 322, The function currently ignores the value of the reason parameter and always sets params.CancellationReason to "requested_by_customer"; update the implementation (the block that creates params := &stripego.PaymentIntentCancelParams{} and sets params.CancellationReason) to either (A) validate and map the incoming reason string to a permitted Stripe cancellation enum and assign that mapped value via params.CancellationReason = stripego.String(mappedReason), returning an error for unknown values, or (B) change the function signature to accept a boolean/typed enum instead of a free-form string and use that to set CancellationReason; if you opt not to change behavior, add a clear doc comment on the function that the reason parameter is only a presence flag and that "requested_by_customer" is always used. Ensure you also keep params.SetStripeAccount(accountID) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/financial-gateway/adapters/stripe/payment_intent_adapter.go`:
- Around line 337-346: Replace the fragile string match on stripeErr.Msg with a
check on stripeErr.Code (or fetch the PaymentIntent to inspect its Status) in
the cancellation handling code: where the adapter currently does
strings.Contains(stripeErr.Msg, "status of canceled"), use stripeErr.Code ==
"<appropriate_stripe_code_for_already_canceled>" (or call the Stripe API to
retrieve the PaymentIntent status and confirm it's already canceled) and then
return the same CancelResult with ProviderReference piID and Status
DispatchStatus_DISPATCH_STATUS_FAILED; update references to stripeErr, piID,
paymentOrderID and the CancelResult return path in the cancellation handling
branch accordingly.
---
Duplicate comments:
In `@services/financial-gateway/service/grpc_service.go`:
- Around line 219-235: The mapCancelError function currently maps
unknown/provider failures to codes.Internal; change its default branch to return
codes.Unavailable (matching mapStripeError behavior) so transient
provider/network/Stripe API errors are reported as Unavailable for proper client
retries; update the default return in mapCancelError to use
status.Error(codes.Unavailable, "failed to cancel payment") and ensure any tests
or callers that expect Internal are adjusted accordingly.
---
Nitpick comments:
In `@services/financial-gateway/adapters/stripe/payment_intent_adapter.go`:
- Around line 318-322: The function currently ignores the value of the reason
parameter and always sets params.CancellationReason to "requested_by_customer";
update the implementation (the block that creates params :=
&stripego.PaymentIntentCancelParams{} and sets params.CancellationReason) to
either (A) validate and map the incoming reason string to a permitted Stripe
cancellation enum and assign that mapped value via params.CancellationReason =
stripego.String(mappedReason), returning an error for unknown values, or (B)
change the function signature to accept a boolean/typed enum instead of a
free-form string and use that to set CancellationReason; if you opt not to
change behavior, add a clear doc comment on the function that the reason
parameter is only a presence flag and that "requested_by_customer" is always
used. Ensure you also keep params.SetStripeAccount(accountID) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c09f4885-2e8c-486f-8aa2-b5bd13dfb4ed
📒 Files selected for processing (4)
services/financial-gateway/adapters/http/webhook_handler.goservices/financial-gateway/adapters/stripe/payment_intent_adapter.goservices/financial-gateway/adapters/stripe/payment_intent_adapter_test.goservices/financial-gateway/service/grpc_service.go
- Use ErrorCodePaymentIntentUnexpectedState for cancel idempotency check - Map default cancel errors to codes.Unavailable instead of Internal - Add ErrInvalidRequest mapping for non-cancellable payment states - Fix AlreadyCancelled test mock to include proper error code
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/financial-gateway/adapters/stripe/payment_intent_adapter.go (1)
333-349:⚠️ Potential issue | 🟠 MajorAvoid message-fragment matching for idempotent cancel detection.
On Line 337, using
strings.Contains(stripeErr.Msg, "status of canceled")is brittle. If Stripe message text changes, already-cancelled requests can incorrectly fall through toErrInvalidRequest(mapped toFailedPreconditioninservices/financial-gateway/service/grpc_service.goLine 228), breaking idempotent behavior.Proposed fix (prefer structured Stripe fields over message text)
- if errors.As(err, &stripeErr) && stripeErr.Code == stripego.ErrorCodePaymentIntentUnexpectedState { + if errors.As(err, &stripeErr) && stripeErr.Code == stripego.ErrorCodePaymentIntentUnexpectedState { // Stripe returns payment_intent_unexpected_state when the PI // is in a non-cancellable state. Check if it's already canceled // (idempotent success) vs a truly non-cancellable state (e.g., succeeded). - if strings.Contains(stripeErr.Msg, "status of canceled") { + if stripeErr.PaymentIntent != nil && stripeErr.PaymentIntent.Status == stripego.PaymentIntentStatusCanceled { a.logger.Info("stripe payment intent already cancelled", "payment_order_id", paymentOrderID, "payment_intent_id", piID, ) return CancelResult{ ProviderReference: piID, Status: financialgatewayv1.DispatchStatus_DISPATCH_STATUS_FAILED, }, nil } // Non-cancellable state (e.g., succeeded) — return as invalid request return CancelResult{}, fmt.Errorf("payment intent %s cannot be cancelled: %w", piID, ErrInvalidRequest) }For stripe-go v82, when canceling a PaymentIntent fails with code `payment_intent_unexpected_state`, is `stripe.Error.PaymentIntent.Status` reliably populated with the current status (e.g., `canceled`)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/financial-gateway/adapters/stripe/payment_intent_adapter.go` around lines 333 - 349, The code currently detects an already-cancelled PaymentIntent by matching text in stripeErr.Msg (strings.Contains(stripeErr.Msg, "status of canceled")), which is brittle; instead check the structured Stripe error payload: when handling the payment_intent_unexpected_state branch, verify stripeErr.PaymentIntent is non-nil and compare stripeErr.PaymentIntent.Status to the canonical canceled status (e.g., stripego.PaymentIntentStatusCanceled or the literal "canceled"), then treat that as idempotent success by returning the CancelResult (ProviderReference piID, Status DISPATCH_STATUS_FAILED) as before; otherwise fall back to returning ErrInvalidRequest. Update any imports/types as needed and keep the surrounding logic in the same function handling the stripeErr variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@services/financial-gateway/adapters/stripe/payment_intent_adapter.go`:
- Around line 333-349: The code currently detects an already-cancelled
PaymentIntent by matching text in stripeErr.Msg (strings.Contains(stripeErr.Msg,
"status of canceled")), which is brittle; instead check the structured Stripe
error payload: when handling the payment_intent_unexpected_state branch, verify
stripeErr.PaymentIntent is non-nil and compare stripeErr.PaymentIntent.Status to
the canonical canceled status (e.g., stripego.PaymentIntentStatusCanceled or the
literal "canceled"), then treat that as idempotent success by returning the
CancelResult (ProviderReference piID, Status DISPATCH_STATUS_FAILED) as before;
otherwise fall back to returning ErrInvalidRequest. Update any imports/types as
needed and keep the surrounding logic in the same function handling the
stripeErr variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 056b7d94-3cc5-411f-9f14-b22a50608d9f
📒 Files selected for processing (3)
services/financial-gateway/adapters/stripe/payment_intent_adapter.goservices/financial-gateway/adapters/stripe/payment_intent_adapter_test.goservices/financial-gateway/service/grpc_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/financial-gateway/adapters/stripe/payment_intent_adapter_test.go
bjcoombs
dismissed stale reviews from claude[bot], coderabbitai[bot], and coderabbitai[bot]
Stale bot review — verified findings addressed
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.
Summary
platform_fee_minor_unitsfield toDispatchPaymentResponseproto. Wire through from Stripe adapter result to gRPC response and Starlark client (previously hardcoded toint64(0)).PaymentRefundedEventandPaymentDisputedEventproto messages. MapREFUNDEDandDISPUTEDwebhook statuses inwebhook_handler.goto domain events published to existing Kafka topics (financial-gateway.payment-refunded.v1,financial-gateway.payment-disputed.v1).CancelPaymentRPC with testablePaymentIntentCancellerandPaymentIntentResolverinterfaces. Handles idempotency (already-cancelled payments succeed). Replaces theUnimplementedstub.Changes Made
api/proto/.../financial_gateway.proto: Addedplatform_fee_minor_unitsfield (tag 7) toDispatchPaymentResponseapi/proto/.../events.proto: AddedPaymentRefundedEventandPaymentDisputedEventmessagesservices/financial-gateway/adapters/stripe/payment_intent_adapter.go: AddedPaymentIntentCanceller,PaymentIntentResolverinterfaces;CancelResultstruct;CancelPaymentmethodservices/financial-gateway/adapters/http/webhook_handler.go: Map REFUNDED/DISPUTED statuses to new domain events; updatedtopicToEventTypeservices/financial-gateway/service/grpc_service.go: ImplementedCancelPaymentRPC with tenant resolution and error mappingservices/financial-gateway/client/starlark.go: Readplatform_fee_minor_unitsfrom response instead of hardcoding 0Test plan
go build ./...passes