Make automatic gateway routing deterministic and safe#121
Merged
Conversation
📝 WalkthroughWalkthroughPaymentService now validates automatic gateway routing by normalized currency and payment method, honors compatible configured defaults, rejects unsupported routes before provider calls, and rejects unknown transaction prefixes. AddPayBridge validates gateway configuration, with tests and documentation covering these rules. ChangesGateway configuration validation
Automatic gateway routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PaymentService
participant ConfiguredGateways
participant PaymentGateway
PaymentService->>ConfiguredGateways: inspect normalized currency and payment method
ConfiguredGateways-->>PaymentService: return compatible configured gateway
PaymentService->>PaymentGateway: invoke selected provider
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
PayBridge.SDK.Test/Unit/PaymentServiceRoutingTests.cs (1)
16-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a compatible but unregistered default.
Configure
Checkoutas the USD default while registering onlyStripe, then verify Stripe is selected. This directly protects the “default must be registered” contract.Proposed test
+ [Fact] + public async Task Unregistered_compatible_default_is_skipped() + { + var stripe = Gateway(PaymentGatewayType.Stripe); + var service = CreateService( + new PaymentGatewayConfig { DefaultGateway = PaymentGatewayType.Checkout }, + stripe.Object); + + await service.CreatePaymentAsync(Request("USD")); + + stripe.Verify( + item => item.CreatePaymentAsync(It.IsAny<PaymentRequest>()), + Times.Once); + }🤖 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 `@PayBridge.SDK.Test/Unit/PaymentServiceRoutingTests.cs` around lines 16 - 59, Extend Automatic_routing tests with a case configuring Checkout as the USD default while registering only the Stripe gateway. Invoke CreatePaymentAsync with a USD request, then verify Stripe.CreatePaymentAsync is called once and no unregistered Checkout gateway is selected.
🤖 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.
Nitpick comments:
In `@PayBridge.SDK.Test/Unit/PaymentServiceRoutingTests.cs`:
- Around line 16-59: Extend Automatic_routing tests with a case configuring
Checkout as the USD default while registering only the Stripe gateway. Invoke
CreatePaymentAsync with a USD request, then verify Stripe.CreatePaymentAsync is
called once and no unregistered Checkout gateway is selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc00fd1f-2f17-4bae-a491-9cc94c362178
📒 Files selected for processing (3)
PayBridge.SDK.Test/Unit/PaymentServiceRoutingTests.csPayBridge.SDK/Services/PaymentService.csdocs/automatic-gateway-routing.md
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens PayBridge’s automatic gateway routing to be deterministic and capability-aware, preventing “fallback to first registered gateway” behavior and making selection observable.
Changes:
- Updates
PaymentServicerouting to honor a compatible, registeredDefaultGateway, otherwise select deterministically from a currency’s compatibility list and fail fast when no compatible gateway is configured. - Adds unit tests covering default gateway behavior, DI registration-order determinism, single-gateway bypass removal, unknown currency, crypto, and JPY routing.
- Documents the automatic routing rules and current currency groupings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| PayBridge.SDK/Services/PaymentService.cs | Refactors automatic gateway selection to be deterministic, validate compatibility, log selection reasons, and fail fast instead of arbitrary fallback. |
| PayBridge.SDK.Test/Unit/PaymentServiceRoutingTests.cs | Adds routing-focused unit tests to lock in deterministic behavior and pre-provider-call failures for unsupported routes. |
| docs/automatic-gateway-routing.md | Documents routing rules, currency groups, DefaultGateway behavior, and explicit rejection of unsupported routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
438
to
442
| if (request.PaymentMethodType == PaymentMethodType.Crypto) | ||
| { | ||
| return _gateways.Keys.First(); | ||
| throw new PaymentGatewayException( | ||
| "No configured gateway supports payment method Crypto."); | ||
| } |
Comment on lines
451
to
+455
| // Select based on currency | ||
| switch (request.Currency?.ToUpper()) | ||
| { | ||
| case "NGN": | ||
| return ChooseAvailableGateway(PaymentGatewayType.Monnify, PaymentGatewayType.Squad, PaymentGatewayType.Korapay, PaymentGatewayType.Interswitch, PaymentGatewayType.Remita, PaymentGatewayType.Opay, PaymentGatewayType.Paystack, PaymentGatewayType.Flutterwave); | ||
|
|
||
| case "KES": | ||
| case "GHS": | ||
| case "UGX": | ||
| case "TZS": | ||
| case "ZAR": | ||
| case "RWF": | ||
| case "ZMW": | ||
| case "CDF": | ||
| case "XOF": | ||
| case "XAF": | ||
| case "MWK": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
|
|
||
| case "BWP": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.DpoGroup); | ||
|
|
||
| case "BHD": | ||
| return ChooseAvailableGateway(PaymentGatewayType.BenefitPay); | ||
|
|
||
| case "KWD": | ||
| return ChooseAvailableGateway(PaymentGatewayType.Knet); | ||
|
|
||
| case "USD": | ||
| case "EUR": | ||
| case "GBP": | ||
| default: | ||
| return ChooseAvailableGateway(PaymentGatewayType.Stripe, PaymentGatewayType.Checkout); | ||
| } | ||
| var compatibleGateways = request.Currency.ToUpperInvariant() switch | ||
| { | ||
| "NGN" => new[] { PaymentGatewayType.Monnify, PaymentGatewayType.Squad, PaymentGatewayType.Korapay, PaymentGatewayType.Interswitch, PaymentGatewayType.Remita, PaymentGatewayType.Opay, PaymentGatewayType.Paystack, PaymentGatewayType.Flutterwave }, | ||
| "KES" or "GHS" or "UGX" or "TZS" or "ZAR" or "RWF" or "ZMW" or |
Comment on lines
+501
to
+503
| throw new PaymentGatewayException( | ||
| $"No configured gateway supports {request.Currency.ToUpperInvariant()} " + | ||
| $"with payment method {request.PaymentMethodType}."); |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@PayBridge.SDK/Services/PaymentService.cs`:
- Around line 48-54: Update ValidateRequest after NormalizeCurrency to reject a
currency that is null, empty, or whitespace-only, before payment-method routing
or provider selection continues. Preserve the normalized value for valid
currencies and raise the existing request-validation exception used by
PaymentService.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dc3cf5f-ed50-4fab-b4f0-5c9932390f24
📒 Files selected for processing (5)
PayBridge.SDK.Test/Unit/IServiceCollectionExtensionsTests.csPayBridge.SDK.Test/Unit/PaymentServiceRoutingTests.csPayBridge.SDK/Externsions/IServiceCollectionExtensions.csPayBridge.SDK/Services/PaymentService.csdocs/automatic-gateway-routing.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/automatic-gateway-routing.md
Comment on lines
+48
to
+54
| request.Currency = NormalizeCurrency(request.Currency); | ||
|
|
||
| if (request.PaymentMethodType == PaymentMethodType.Crypto) | ||
| { | ||
| throw new PaymentGatewayException( | ||
| "No configured gateway supports payment method Crypto."); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject currency that becomes empty after normalization.
ValidateRequest accepts whitespace-only currency, then Line 48 converts it to "". Explicit gateway routing bypasses SelectBestGateway, so the malformed request reaches the provider.
Proposed fix
request.Currency = NormalizeCurrency(request.Currency);
+if (request.Currency.Length == 0)
+{
+ throw new ArgumentException("Currency is required", nameof(request));
+}📝 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
| request.Currency = NormalizeCurrency(request.Currency); | |
| if (request.PaymentMethodType == PaymentMethodType.Crypto) | |
| { | |
| throw new PaymentGatewayException( | |
| "No configured gateway supports payment method Crypto."); | |
| } | |
| request.Currency = NormalizeCurrency(request.Currency); | |
| if (request.Currency.Length == 0) | |
| { | |
| throw new ArgumentException("Currency is required", nameof(request)); | |
| } | |
| if (request.PaymentMethodType == PaymentMethodType.Crypto) | |
| { | |
| throw new PaymentGatewayException( | |
| "No configured gateway supports payment method Crypto."); | |
| } |
🤖 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 `@PayBridge.SDK/Services/PaymentService.cs` around lines 48 - 54, Update
ValidateRequest after NormalizeCurrency to reject a currency that is null,
empty, or whitespace-only, before payment-method routing or provider selection
continues. Preserve the normalized value for valid currencies and raise the
existing request-validation exception used by PaymentService.
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
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
DefaultGatewayonly when it is registered and compatibleRoot cause
Automatic routing ignored
DefaultGateway, treated every unknown currency as a Stripe/Checkout currency, and fell back to the first DI-registered gateway when no preferred provider was available. This could invoke a provider that could not process the request.Validation
Part of #65
Related to #90
Summary by CodeRabbit