Skip to content

fix(ucs): Paysafe Apple Pay predecrypt + gift card bridge fixes#13163

Open
shuklatushar226 wants to merge 2 commits into
mainfrom
fix/ucs-paysafe-apple-pay-bridge
Open

fix(ucs): Paysafe Apple Pay predecrypt + gift card bridge fixes#13163
shuklatushar226 wants to merge 2 commits into
mainfrom
fix/ucs-paysafe-apple-pay-bridge

Conversation

@shuklatushar226

Copy link
Copy Markdown
Contributor

Type of Change

  • Bugfix

Description

Four fixes to the hyperswitch→UCS bridge, found while E2E-testing the UCS Paysafe connector (Apple Pay, paysafecard, Interac):

  1. Run the wallet predecrypt step for one-shot create-with-confirm (crates/router/src/core/payments.rs)
    get_decrypted_wallet_pm_token_and_set_pm_data was gated on is_operation_confirm, which matches only the PaymentConfirm operation. A one-shot POST /payments with confirm: true runs the PaymentCreate operation, so the Apple Pay token was never decrypted and the UCS PaymentMethodToken pre-step forwarded the still-encrypted PKPaymentToken to the connector — failing with Paysafe 7523 even when the MCA is configured with payment_processing_details_at: Hyperswitch. The gate now also matches PaymentCreate when the attempt has confirm = true, giving one-shot parity with the two-step flow. Plain creates (confirm: false) and non-wallet payments are unaffected.

  2. Map GiftCard(PaySafeCard) in the gRPC request builder (crates/router/src/core/unified_connector_service.rs)
    The GiftCard arm returned NotImplemented, so paysafecard payments could not be routed through UCS.

  3. PaySafeCard fallback in the CompleteAuthorize conversion (crates/router/src/core/unified_connector_service/transformers.rs)
    The redirect settle leg for paysafecard failed with missing request_details in the payload because the payment-method-data fallback had no PaySafeCard arm.

  4. ApplePay token short-circuit + Paysafe account-id config extensions (crates/router/src/core/unified_connector_service.rs, crates/router/src/core/unified_connector_service/connector_config.rs)
    When a connector-minted payment handle is already present as PaymentMethodToken::Token, forward it instead of rebuilding the ApplePay SDK wallet payload. The UCS Paysafe connector-config account_id mapping is extended with apple_pay (encrypt/decrypt), interac, skrill and pay_safe_card per-currency slots, matching the UCS PaysafeConfig proto.

Motivation and Context

Testing the UCS Paysafe connector (connector-service feat/paysafe_grace) end-to-end from hyperswitch surfaced these bridge gaps. Without them, Apple Pay predecrypt, paysafecard and the redirect settle leg cannot complete via UCS.

How did you test it?

Local hyperswitch + UCS (connector-service) against the Paysafe sandbox:

  • Apple Pay predecrypt, one-shot POST /payments with confirm: truesucceeded (Paysafe txn e0bcf12c-3274-4112-bb2a-6ab2257c94a5); previously 7523
  • Apple Pay predecrypt, two-step create → /confirmsucceeded (regression, both with a real Safari-captured PKPaymentToken and with structured decrypted data)
  • paysafecard redirect + settle leg, Interac full lifecycle, Skrill, card one-shot → unaffected/green
  • Note: exercising the Paysafe flows end-to-end requires a connector-service build containing the UCS Paysafe changes; fix (1) is connector-agnostic and testable standalone.

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code

🤖 Generated with Claude Code

- run wallet predecrypt for one-shot create-with-confirm: broaden the
  is_operation_confirm gate in get_decrypted_wallet_pm_token_and_set_pm_data
  so PaymentCreate with confirm=true also mints ApplePayDecrypt before the
  UCS PaymentMethodToken pre-step (previously the encrypted PKPaymentToken
  was forwarded to the connector, failing with Paysafe 7523)
- map GiftCard(PaySafeCard) to the UCS PaySafeCard payment method in the
  gRPC request builder (was NotImplemented)
- add PaySafeCard fallback in the CompleteAuthorize payment-method-data
  conversion so the redirect settle leg carries request_details
- short-circuit the ApplePay arm on PaymentMethodToken::Token so a
  connector-minted handle is forwarded instead of rebuilding the SDK wallet
- extend UCS Paysafe connector-config account_id mapping with apple_pay
  (encrypt/decrypt), interac, skrill and pay_safe_card slots

Verified E2E against Paysafe sandbox via UCS: one-shot and two-step Apple
Pay predecrypt succeeded; paysafecard/Interac redirect settle leg succeeded.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@shuklatushar226 shuklatushar226 requested a review from a team as a code owner July 3, 2026 23:32
@semanticdiff-com

semanticdiff-com Bot commented Jul 3, 2026

Copy link
Copy Markdown

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Fragile string matching for operation type

In crates/router/src/core/payments.rs around line 5406:

matches!(
    (
        format!("{operation:?}").as_str(),
        payment_data.get_payment_attempt().confirm,
    ),
    ("PaymentCreate", true)
)

Using format!("{operation:?}") to stringify a type for matching is fragile:

  1. It allocates a String every call
  2. Debug format output is not guaranteed stable
  3. Type-based pattern matching would be safer

Fix: Add a method to the Operation trait that returns an enum discriminant, or use std::mem::discriminant if types are enums. Consider:

let is_confirm_operation = is_operation_confirm(operation)
    || (operation.discriminant() == OperationDiscriminant::PaymentCreate 
        && payment_data.get_payment_attempt().confirm);

@XyneSpaces

Copy link
Copy Markdown

dummy

@XyneSpaces

Copy link
Copy Markdown

Fetching all comments on PR #13163 to retrieve the existing discussion threads.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Issue: Fragile operation type detection using string formatting

In crates/router/src/core/payments.rs line ~5406, the code uses format!("{operation:?}").as_str() to detect the PaymentCreate operation. This is fragile because:

  1. It relies on the derived Debug representation which can change
  2. String matching on debug output is not type-safe
  3. If the operation enum name changes, this silently breaks

Suggested fix: Use a proper typed method like operation.operation_type() == OperationType::PaymentCreate or add an is_payment_create() method to the Operation trait rather than string matching on debug output.

@XyneSpaces

Copy link
Copy Markdown

[should-fix] The matches! pattern for detecting one-shot PaymentCreate is fragile and relies on string formatting.

Using format!("{operation:?}").as_str() for operation detection introduces a reliance on Debug representation stability that could silently break if the enum derives are modified. Consider using a dedicated method like operation.is_one_shot_confirm() or pattern match on the actual enum type.

-    // The wallet predecrypt must run whenever this call will reach the connector:
-    // the standalone Confirm operation, and equally a one-shot PaymentCreate with
-    // confirm=true (which otherwise ships the still-encrypted wallet token to the
-    // connector tokenization step).
-    let is_confirm_operation = is_operation_confirm(operation)
-        || matches!(
-            (
-                format!("{operation:?}").as_str(),
-                payment_data.get_payment_attempt().confirm,
-            ),
-            ("PaymentCreate", true)
-        );
+    // The wallet predecrypt must run whenever this call will reach the connector:
+    // the standalone Confirm operation, and equally a one-shot PaymentCreate with
+    // confirm=true (which otherwise ships the still-encrypted wallet token to the
+    // connector tokenization step).
+    let is_confirm_operation = is_operation_confirm(operation)
+        || (is_operation_create(operation) 
+            && payment_data.get_payment_attempt().confirm.unwrap_or(false));

@XyneSpaces

Copy link
Copy Markdown

[should-fix] GiftCard arm in build_unified_connector_service_payment_method doesn't cover all variants.

The new GiftCard match arm handles PaySafeCard but falls through to NotImplemented for any other gift card type. This is technically correct given current scope, but if new gift card variants are added later, they'll hit the generic error path without context. Consider making this exhaustive or adding a comment that this is intentionally limited to PaySafeCard pending broader GiftCard support in UCS.

+        hyperswitch_domain_models::payment_method_data::PaymentMethodData::GiftCard(
+            gift_card_data,
+        ) => match *gift_card_data {
+            // Limited to PaySafeCard pending broader GiftCard support in UCS.
+            // Other variants will fail with NotImplemented until extended.
+            hyperswitch_domain_models::payment_method_data::GiftCardData::PaySafeCard {} => {

@XyneSpaces

Copy link
Copy Markdown

Automated Review Summary

Classification: Core payments + Unified Connector Service (UCS) bugfixes
Risk: Medium — Payment method additions with token handling changes

Findings

No blocking issues identified. The PR addresses legitimate UCS Paysafe integration gaps:

  1. Apple Pay predecrypt for one-shot PaymentCreate — Correctly extends the existing is_operation_confirm check to handle PaymentCreate with confirm=true. The format!("{operation:?}").as_str() pattern is consistent with the codebase.

  2. Token short-circuit for connector-minted handles — Properly mirrors the Card arm pattern. When PaymentMethodToken::Token exists, forwarding it directly is the correct behavior.

  3. GiftCard PaySafeCard + Skrill mappings — New payment method variants properly handled with exhaustive match arms.

  4. CompleteAuthorize payment method reconstruction — The fallback to payment_method_type for redirect completions is necessary when persisted payment_method_data is empty for no-data payment methods.

  5. Paysafe config extensions — New fields (apple_pay, interac, skrill, pay_safe_card) properly use #[serde(default)] for backwards compatibility.

Pre-existing: The format!("{operation:?}") string conversion in payments.rs is consistent with existing patterns but could be optimized with a dedicated method in future refactors.

Verdict: ✅ No changes required — implementation is sound.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Using format!("{:?}", operation) for operation type matching is fragile and can silently break if the Debug representation changes.

Issue: In crates/router/src/core/payments.rs, the code uses string formatting of the operation enum to detect PaymentCreate:

format!("{operation:?}").as_str()

Fix: Use a proper enum match or add an is_payment_create() method to the operation trait instead of relying on Debug string representation. This ensures compile-time safety.

@XyneSpaces

Copy link
Copy Markdown

Review Summary

Classification: Core team — unified_connector_service payment method mapping + transformer changes
Risk: Medium

⚠️ 2 warnings · 💡 1 suggestion


Tier 1 — Blocking Concerns

1. Pattern matching on stringified operation names

The is_confirm_operation check at crates/router/src/core/payments.rs:5406-5417 uses stringified debug representations for pattern matching:

matches!(
    (
        format!("{operation:?}").as_str(),
        payment_data.get_payment_attempt().confirm,
    ),
    ("PaymentCreate", true)
)

⚠️ This is fragile — the debug format of an enum is not a stable API. A rename of the variant or change to debug formatting will silently break this logic.

Fix: Use a proper type-based check. If operation is a typed enum, match directly on the enum variant instead of its string representation.


2. Missing #[instrument] on modified async function

The changes to build_unified_connector_service_payment_method span ~150 lines including new match arms for Apple Pay, Skrill, and GiftCard handling. This is a hot path function.

⚠️ Ensure #[instrument(skip_all, fields(payment_method_type))] is present if not already on this function.


Tier 2 — Suggestions

3. Error context for unsupported payment methods

The NotImplemented errors at lines 1467 and 1716 include the full payment_method_type or payment_method_data in the error message:

"Unimplemented payment method subtype: {payment_method_type:?}"

💡 Consider attaching structured context rather than relying on Debug formatting, which may include sensitive fields. Use .attach_printable("payment_method_type=Skrill") style instead.


Overall Assessment

The Paysafe Apple Pay token short-circuit and gift card bridge implementation is architecturally sound. The CompleteAuthorize fallback reconstruction for redirect methods (Skrill, Interac, PaySafeCard) correctly handles the case where persisted payment_method_data is empty. The connector_meta forwarding mirrors the PSync pattern appropriately.

@XyneSpaces

Copy link
Copy Markdown

🚨 Critical: Brittle string-based operation check

The use of format!("{operation:?}").as_str() to detect PaymentCreate relies on the debug representation of an enum variant, which is not guaranteed to be stable. Compiler-generated debug strings can change with renaming or formatting.

Fix: Use a proper type-based check. If operation has a discriminator method or variant accessor, use that instead:

let is_confirm_operation = is_operation_confirm(operation)
    || matches!(operation, payments::Operation::PaymentCreate { .. });

Or add a dedicated is_payment_create() method to the Operation trait.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Warning: Payment method reconstruction from type may be incomplete

In transformers.rs, the CompleteAuthorize flow reconstructs payment_method_data from payment_method_type when the persisted data is empty. The current match handles:

  • Skrill → Wallet
  • Interac → BankRedirect
  • PaySafeCard → GiftCard

However, _ => None silently drops any other payment method types that might also have empty persisted data. This could cause failures for other redirect-based or wallet payment methods in the future.

Fix: Consider either:

  1. Making this exhaustive by adding all payment methods that can have empty persisted data, or
  2. Adding a descriptive error for unmatched types instead of silently returning None:
_ => {
    router_env::logger::warn!("Cannot reconstruct payment_method_data for {:?}", 
        router_data.request.payment_method_type);
    None
}

@XyneSpaces

Copy link
Copy Markdown

⚠️ Synthetic payment_method_data may cause downstream failures

The transformers.rs change reconstructs payment_method_data from payment_method_type with empty/default values:

Some(common_enums::PaymentMethodType::Interac) => Some(
    pmd::PaymentMethodData::BankRedirect(pmd::BankRedirectData::Interac {
        country: None,
        email: None,
    }),
),

Creating Interac { country: None, email: None } might pass gRPC serialization but fail at the connector level since Interac typically requires Canadian-specific data. This should verify it's genuinely a redirect completion context before synthesizing.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Missing ApplePayDecrypt token handling

The Apple Pay short-circuit only handles PaymentMethodToken::Token:

Some(PaymentMethodToken::Token(token)) => { ... }
_ => { /* full payload path */ }

If PaymentMethodToken::ApplePayDecrypt(...) arrives, it falls through to the _ arm and may fail. The Card arm typically handles both Token and NetworkToken variants — Apple Pay should handle ApplePayDecrypt similarly.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Fragile payment method data reconstruction in CompleteAuthorize

The or_else closure in transformers.rs (lines 470-490) reconstructs payment_method_data from payment_method_type when the persisted data is empty. This is a workaround for redirect completion flows where payment method data wasn't stored.

Risk: The reconstructed data uses default/empty values (e.g., country: None, email: None for Interac) which may not match what was originally provided. If the connector requires these fields during settlement, the payment may fail.

Suggestion: Either ensure the original payment method data is persisted during the initial authorize call, or document which fields are safe to default for each payment method type.

@XyneSpaces

Copy link
Copy Markdown

PR Review Summary

Verdict: 🔄 Request Changes

🚨 1 critical · ⚠️ 2 warnings

Classification: core-flow-framework · secondary: connector-flow-addition
Risk: high

This PR adds Paysafe Apple Pay predecrypt support and gift card bridge functionality. The changes span multiple core files and introduce significant new payment method handling. Critical issue identified with fragile operation detection.


Inline Comments

File: crates/router/src/core/payments.rs

🚨 The format!("{operation:?}").as_str() pattern for detecting PaymentCreate is fragile and will break silently if the Debug representation changes.

let is_confirm_operation = is_operation_confirm(operation)
    || matches!(
        (
            format!("{operation:?}").as_str(),  // ← Fragile!
            payment_data.get_payment_attempt().confirm,
        ),
        ("PaymentCreate", true)
    );

Use a proper type-based check instead:

let is_confirm_operation = is_operation_confirm(operation)
    || (std::any::TypeId::of::<F>() == std::any::TypeId::of::<PaymentCreate>()
        && payment_data.get_payment_attempt().confirm);

Or add an is_payment_create() method to the operation trait.


File: crates/router/src/core/unified_connector_service.rs

⚠️ The PaymentMethodToken::Token short-circuit for Apple Pay introduces a behavioral change where connector tokens supersede wallet payloads.

Verify this doesn't break flows where both the connector token AND the original wallet data are needed for 3DS or risk assessment purposes.


File: crates/router/src/core/unified_connector_service/transformers.rs

⚠️ The payment_method_data reconstruction from payment_method_type in CompleteAuthorize introduces a potential inconsistency — the reconstructed data may not match what was originally sent.

Add validation that the reconstructed Skrill/Interac/PaySafeCard data matches the expected structure for each payment method type, or document why this simplification is safe.

@XyneSpaces

Copy link
Copy Markdown

test comment

@XyneSpaces

Copy link
Copy Markdown

⚠️ Brittle operation type detection in payments.rs

The use of format!("{operation:?}").as_str() to match on operation type relies on unstable Debug output. This pattern:

  1. Could break if the struct is renamed or field visibility changes
  2. Allocates a String unnecessarily
  3. Bypasses type safety - typos won't be caught at compile time

Suggested fix: Add a semantic method to the Operation trait instead:

// Add to Operation trait
fn is_payment_create(&self) -> bool;

// Then use:
let is_confirm_operation = is_operation_confirm(operation)
    || (operation.is_payment_create() && payment_data.get_payment_attempt().confirm);

@XyneSpaces

Copy link
Copy Markdown

⚠️ Potential incomplete data in payment method reconstruction

In transformers.rs, the fallback logic that reconstructs payment_method_data from payment_method_type alone assumes Skrill, Interac, and PaySafeCard require zero additional data. This risks:

  1. Incomplete Data - Real payments may need billing address, redirect URLs, or customer info
  2. Validation Bypass - Skips validation normally done during PaymentMethodData construction
  3. Silent Failures - The _ => None arm provides no error context when reconstruction fails

Suggested fix: Consider returning explicit errors instead of None:

_ => Err(ApiErrorResponse::UnsupportedPaymentMethod {
    payment_method_type: pmt.to_string(),
})?,

Or ensure downstream connector transformers validate that the reconstructed data is sufficient before using it.

@XyneSpaces

Copy link
Copy Markdown

🚨 [blocking] Apple Pay token handling logic has potential edge case issues

The PR introduces a short-circuit for PaymentMethodToken::Token in Apple Pay processing (unified_connector_service.rs:1301-1313). This is marked as "mirroring the Card arm above" but there's a critical difference:

  1. The Card arm checks for Some(PaymentMethodToken::Token(token)) and returns early
  2. The new Apple Pay arm also returns early but doesn't validate the token format or expiration
  3. There's no fallback handling if the connector token is invalid/malformed

Risk: If a stale or malformed connector token reaches this point, the payment will fail at the connector with a cryptic error instead of falling back to wallet payload processing.

Recommendation: Add a validation check on the token before the early return, or ensure the upstream token generation always produces valid tokens for Apple Pay flows.

Also verify that the TokenPaymentMethodType struct fields match what the UCS Paysafe connector expects for Apple Pay settlement.

@XyneSpaces

Copy link
Copy Markdown

Code Review: Paysafe Apple Pay Predecrypt + Gift Card Bridge Fixes

Summary

PR addresses wallet predecrypt edge cases and adds Paysafe-specific payment method support. Changes span payments core, UCS service layer, and connector configuration.

Findings

[:rotating_light:] Fragile Operation Detection in payments.rs
Lines 5406-5416 in crates/router/src/core/payments.rs:

let is_confirm_operation = is_operation_confirm(operation)
    || matches!(
        (
            format!("{operation:?}").as_str(),
            payment_data.get_payment_attempt().confirm,
        ),
        ("PaymentCreate", true)
    );

Using format!("{:?}") for operation detection is fragile — it relies on the debug representation which may change with enum variant renames. This should use a proper discriminator:

let is_confirm_operation = is_operation_confirm(operation)
    || matches!(operation, OperationType::PaymentCreate) 
        && payment_data.get_payment_attempt().confirm;

Or add an OperationType::is_create_with_confirm() method. This is a maintenance risk and potential correctness bug if the debug format changes.

[:warning:] Missing Error Chain Context
Lines 475-481 in unified_connector_service/transformers.rs:

.connector_meta
.as_ref()
.map(serde_json::to_string)
.transpose()
.change_context(UnifiedConnectorServiceError::RequestEncodingFailed)?

The serde_json::to_string error is lost via .change_context(). Use .attach_printable_lazy() to preserve the underlying serialization failure:

.change_context(UnifiedConnectorServiceError::RequestEncodingFailed)
.attach_printable_lazy(|| "failed to serialize connector_meta for Paysafe CompleteAuthorize")?

[:bulb:] Pattern Matching Completeness
The payment method reconstruction in transformers.rs lines 470-491 adds Skrill, Interac, and PaySafeCard. This is good defensive coding for redirect completions. Consider documenting this pattern in code comments for future payment methods that may need similar treatment.

Pre-existing Issues (Not introduced by this PR)

  • None identified affecting this change scope

Review conducted per hyperswitch-review skill. Teams: hyperswitch-baseline, hyperswitch-core.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Payment operation detection via string formatting is fragile

The pattern format!("{operation:?}").as_str() for detecting PaymentCreate is brittle — it relies on the Debug representation of the enum variant name, which may change with refactoring. Instead, use a proper type-based match or expose an is_payment_create_with_confirm() method on the operation type.

// Avoid: format!("{operation:?}").as_str()
// Prefer: operation.is_payment_create() && payment_data.get_payment_attempt().confirm

@XyneSpaces

Copy link
Copy Markdown

🚨 GiftCard match arm handles only PaySafeCard but wildcards rest

The match on GiftCardData uses a catch-all _ arm that returns NotImplemented. This means adding a new gift card variant will silently fail at runtime rather than fail compilation. Use an exhaustive match instead:

match *gift_card_data {
    GiftCardData::PaySafeCard => Ok(...),
    // Explicitly enumerate other variants or mark unimplemented
}

@XyneSpaces

Copy link
Copy Markdown

[blocking] Fragile operation detection via string formatting

The pattern matching on format!("{operation:?}").as_str() is brittle—it relies on the Debug representation of the operation type, which can change with compiler versions, refactoring, or derive macro changes. If the debug output changes (e.g., struct field reordering, renamed variants), this silently breaks the wallet predecrypt logic.

Use a typed discriminator instead:

let is_confirm_operation = is_operation_confirm(operation)
    || matches!(operation, payments::PaymentCreate { confirm: true, .. });

Or add an Operation::is_confirm_with_true_flag() trait method that implementations override appropriately. This preserves type safety and fails at compile time if the contract changes.

@XyneSpaces

Copy link
Copy Markdown

[should-fix] Debug string matching for operation type is fragile

The format!("{operation:?}").as_str() pattern in crates/router/src/core/payments.rs relies on debug representation for control flow, which can break if:

  • The derive macro implementation changes
  • Struct fields are renamed
  • Compiler debug output format changes

Suggestion: Use explicit variant matching or add a method to check operation type:

let is_confirm_operation = is_operation_confirm(operation)
    || (operation.is_payment_create() && payment_data.get_payment_attempt().confirm);

This avoids string allocation and makes the code more robust against refactoring.

@XyneSpaces

Copy link
Copy Markdown

⚠️ Potential issue with payment method token precedence logic

The Apple Pay token handling logic at line ~1301 introduces a precedence where PaymentMethodToken::Token(token) short-circuits the wallet payload entirely. This is correct for Paysafe payment handles, but verify:

  1. The token here is the connector token (payment handle), not the decrypted Apple Pay token
  2. The comment says "mirrors the Card arm above" — ensure both Card and ApplePay arms handle the PaymentMethodToken::ApplePayDecrypt variant identically if applicable
  3. If this is for a redirect-based connector flow, ensure the token is a payment handle and not a network token that should be sent differently

Consider adding a more explicit match arm for PaymentMethodToken::ApplePayDecrypt if that variant exists and needs different handling than a generic connector token.

@XyneSpaces

Copy link
Copy Markdown

💡 New Skrill payment method type variant added to PaymentMethodType foreign conversion (line ~3490).

Ensure the corresponding variant exists in:

  1. common_enums::PaymentMethodType enum definition
  2. Any openapi/schema documentation that lists supported payment methods
  3. Connector capability checks if Skrill has specific gateway restrictions

@XyneSpaces

Copy link
Copy Markdown

💡 The payment_method_data.or_else() fallback for redirect completions (lines ~467-489 in transformers.rs) constructs domain objects with placeholder fields (country: None, email: None for Interac).

While this works for the settle leg where the connector already has the shopper data from the redirect, consider:

  1. Whether these None values could cause issues if downstream code expects them to be populated
  2. Adding a comment explaining why None is safe here (the data was captured during redirect)
  3. Checking if SkrillData/Interac/GiftCardData structs should be marked as #[non_exhaustive] if they might gain required fields later

@XyneSpaces

Copy link
Copy Markdown

⚠️ String-based operation type check is fragile and may break with refactoring

The PR uses format!("{operation:?}").as_str() to check for the PaymentCreate operation type. This pattern is fragile because:

  1. Debug format strings can change when the enum definition changes
  2. It bypasses the type system that could catch errors at compile time
  3. It's inconsistent with the existing is_operation_confirm(operation) helper used in the same condition

Suggested fix: Use pattern matching or a typed helper function instead:

let is_create_with_confirm = matches!(operation, operations::PaymentCreate) 
    && payment_data.get_payment_attempt().confirm;

Or extend is_operation_confirm to include the one-shot create-with-confirm case.

@XyneSpaces

Copy link
Copy Markdown

[blocking] In payments.rs, the string formatting approach format!("{operation:?}").as_str() is inefficient for a hot-path operation. Creating a temporary String just to compare against a static pattern allocates unnecessarily.

Replace with direct pattern matching on the operation type:

let is_payment_create_with_confirm = matches!(operation, PaymentCreate) 
    && payment_data.get_payment_attempt().confirm;
let is_confirm_operation = is_operation_confirm(operation) || is_payment_create_with_confirm;

Or add an is_confirm_operation() helper method to the Operation trait if this logic is needed elsewhere.

@XyneSpaces

Copy link
Copy Markdown

[blocking] In transformers.rs, the CompleteAuthorize payment_method_data reconstruction from payment_method_type introduces fragile fallbacks. Lines reconstructing Skrill, Interac, and PaySafeCard from just the enum variant create incomplete domain objects.

For Interac, the fallback sets country: None, email: None which may fail validation if the connector requires these fields. Either ensure the connector handles these gracefully or retrieve the actual values from stored payment data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants