Skip to content

fix(router): populate wallet card_network in V1 payments list response#13119

Open
XyneSpaces wants to merge 1 commit into
mainfrom
fix/wallet-card-network-payment-list
Open

fix(router): populate wallet card_network in V1 payments list response#13119
XyneSpaces wants to merge 1 commit into
mainfrom
fix/wallet-card-network-payment-list

Conversation

@XyneSpaces

Copy link
Copy Markdown

Type of Change

  • Bugfix

Description

The V1 payments list API (GET /payments/list, consumed by the Control Center transaction list) returned card_network as null for wallet transactions (Google Pay / Apple Pay / Samsung Pay), while the payments retrieve API (GET /payments/{id}) returned it correctly. Plain card payments returned card_network in both. The divergence was backend-only — the Control Center already reads card_network from the list payload, the value was simply null.

Root cause: both responses build payment_method_data from the same stored column (payment_attempt.payment_method_data, persisted as AdditionalPaymentData). Retrieve parses it as AdditionalPaymentData then converts via PaymentMethodDataResponse::from(...), which maps the wallet's apple_pay/google_pay data into WalletAdditionalDataForCard.card_network. The V1 list ForeignFrom<(PaymentIntent, PaymentAttempt)> for PaymentsResponse instead deserialized the raw column directly into PaymentMethodDataResponseWithBilling, skipping that conversion. Card field names coincide so survived; the wallet variant's differing serde shape dropped the data, yielding null.

Fix: build payment_method_data in the list impl the same way retrieve does — parse via check_and_get_payment_method_data_based_on_encryption_strategy() as AdditionalPaymentData and convert through PaymentMethodDataResponse::from.

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

Closes #13118

How did you test it?

cargo check -p router (default v1 features) — green. Wallet payment list entries now carry payment_method_data.wallet.<wallet>.card_network, matching the retrieve response for the same payment. Card list entries unchanged.

Checklist

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

The V1 /payments/list builder deserialized the stored payment_method_data
(AdditionalPaymentData) directly into PaymentMethodDataResponseWithBilling,
skipping the AdditionalPaymentData -> PaymentMethodDataResponse conversion the
retrieve flow uses. Card field names coincide so survived; the wallet variant's
differing serde shape dropped the underlying card_network, returning null in the
list while retrieve returned it. Build payment_method_data via
check_and_get_payment_method_data_based_on_encryption_strategy() +
PaymentMethodDataResponse::from, mirroring retrieve.

Closes #13118
@XyneSpaces XyneSpaces requested review from a team as code owners June 30, 2026 19:30
@semanticdiff-com

semanticdiff-com Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  crates/router/src/core/payments/transformers.rs  25% smaller
  SPEC.md Unsupported file format

@XyneSpaces

Copy link
Copy Markdown
Author

💡 Minor observability gap in error handling

The new code at line 4644 handles serde_json::Value::Null silently without logging:

serde_json::Value::Null => None,

While returning None is correct for list API resilience, consider adding a debug/trace log for observability:

serde_json::Value::Null => {
    router_env::logger::debug!("No payment method data for payment_id={}", pa.payment_id);
    None
}

This would help debug cases where payment method data is unexpectedly null. The parse failure path is properly logged at error level.

@XyneSpaces

Copy link
Copy Markdown
Author

💡 Error logging for parse failures uses inconsistent pattern

In transformers.rs, when AdditionalPaymentData parsing fails:

Err(e) => {
    router_env::logger::error!(...);
    None
}

The error is logged but not attached with context. Consider attaching the actual error:

router_env::logger::error!(
    payment_attempt_id = ?pa.attempt_id,
    error = ?e,
    "Failed to parse AdditionalPaymentData"
);

This helps correlate parse failures with specific payment attempts during debugging.

@XyneSpaces

Copy link
Copy Markdown
Author

💡 Error handling uses logger::error! without error_stack propagation, losing diagnostic context that the rest of the codebase typically preserves.

Consider using error_stack chaining via change_context/attach_printable for parse failures, or document the intentional deviation if the error should be silently swallowed for list response resilience.

@XyneSpaces

Copy link
Copy Markdown
Author

📝 Review Summary

The changes in this PR to fix card_network for wallet payments in the list API look well-structured.

✅ Positive observations:

  1. Correct approach: Using AdditionalPaymentDataPaymentMethodDataResponse conversion path matches the retrieve flow pattern
  2. Error handling: Proper error logging when parsing fails instead of silent failure
  3. Documentation: Clear SPEC.md explaining the root cause and acceptance criteria

💡 Minor suggestion:

Consider adding a metric or counter for failed AdditionalPaymentData parsing to enable monitoring of potential data quality issues in production.


This is an automated review. Overall this looks good to merge after standard CI checks.

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.

bug(payments): wallet card_network is null in V1 /payments/list but present in /payments/{id}

1 participant