feat(core): accept merchant_connector_details in Payments and Psync flow #8199
Conversation
swangi-kumari
changed the title
WalkthroughThis update introduces a new abstraction, Changes
Sequence Diagram(s)Payment Status Retrieval with Gateway Credentials (New POST Flow)sequenceDiagram
participant Client
participant Router (payments_status_with_gateway_creds)
participant Core (payments_core)
participant Domain Logic
participant Store/DB
Client->>Router: POST /v2/payments/{id} with PaymentsRetrieveRequestWithMerchantConnectorDetails
Router->>Core: Call payments_core with request (includes merchant_connector_details)
Core->>Domain Logic: get_connector_from_request (extract connector from merchant_connector_details)
Domain Logic->>Core: Return ConnectorData
Core->>Store/DB: Fetch payment data as needed
Core->>Router: Return payment status response
Router->>Client: Respond with payment status
Unified Handling of Merchant Connector Account DetailssequenceDiagram
participant PaymentFlow
participant AccountTypeDetails (enum)
participant DB
participant DirectCreds
PaymentFlow->>AccountTypeDetails: Needs connector details
alt Variant: MerchantConnectorAccount
AccountTypeDetails->>DB: Fetch details by account ID
DB-->>AccountTypeDetails: Return account details
else Variant: MerchantConnectorDetails
AccountTypeDetails->>DirectCreds: Use provided credentials
end
AccountTypeDetails-->>PaymentFlow: Return connector details
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (3)
crates/router/src/core/payments/flows/psync_flow.rs (1)
91-99:⚠️ Potential issueParameter type mismatch will fail to compile
get_merchant_recipient_datastill takes
&_helpers::MerchantConnectorAccountType, while the precedingconstruct_router_data
method (and the global type migration) now use
&domain::MerchantConnectorAccountTypeDetails.These two method signatures must be consistent with the trait definition;
otherwise the impl will not compile under--all-features.- _merchant_connector_account: &helpers::MerchantConnectorAccountType, + _merchant_connector_account: &domain::MerchantConnectorAccountTypeDetails,crates/router/src/routes/payments.rs (1)
955-3055: 🛠️ Refactor suggestionSignificant code duplication and complex authentication logic detected.
This new handler duplicates substantial logic from the existing
payment_statushandler. The core payment processing logic (lines 3021-3048) is nearly identical, and the authentication selection logic is complex and inconsistent with patterns used elsewhere in the file.Refactor to reduce duplication:
Extract the common payment status logic into a shared helper function:
+async fn handle_payment_status_core<T>( + state: app::SessionState, + req_state: ReqState, + auth: auth::AuthenticationData, + global_payment_id: common_utils::id_type::GlobalPaymentId, + request: payment_types::PaymentsRetrieveRequest, + header_payload: HeaderPayload, +) -> errors::RouterResponse<api_models::payments::PaymentsResponse> { + let operation = payments::operations::PaymentGet; + let merchant_context = domain::MerchantContext::NormalMerchant(Box::new( + domain::Context(auth.merchant_account, auth.key_store), + )); + Box::pin(payments::payments_core::< + api_types::PSync, + api_models::payments::PaymentsResponse, + _, + _, + _, + PaymentStatusData<api_types::PSync>, + >( + state, + req_state, + merchant_context, + auth.profile, + operation, + request, + global_payment_id, + payments::CallConnectorAction::Trigger, + header_payload, + )) + .await +}Simplify authentication logic:
Consider extracting the authentication selection logic into a helper function to ensure consistency across handlers and reduce complexity.
crates/router/src/core/payments/operations/payment_confirm_intent.rs (1)
262-273: 🛠️ Refactor suggestion
merchant_connector_detailscloned but never validated
merchant_connector_detailsis copied intoPaymentConfirmData, yet no validation (connector name present, credential formats, mutually-exclusive fields) happens here. Invalid JSON will propagate until hitting deeper layers with less helpful error messages.Consider validating the struct right after extraction to surface deterministic, user-friendly errors.
♻️ Duplicate comments (1)
crates/router/src/core/payment_methods/vault.rs (1)
1737-1747: Duplicate boxed-account allocationThe deletion path repeats the boxing pattern discussed above; refactor alongside retrieval to keep memory profile lean.
🧹 Nitpick comments (21)
crates/router/src/core/payments/flows/session_update_flow.rs (1)
60-60: Implement the v2 construct_router_data function.The
todo!()placeholder indicates that the v2 implementation for session update flow is incomplete. This needs to be implemented before the v2 feature can be used in production.Do you want me to help implement this function or create an issue to track this work?
crates/router/src/core/payments/flows/complete_authorize_flow.rs (1)
73-73: Complete the v2 implementation for complete authorize flow.The function remains unimplemented with a
todo!()placeholder. This needs to be completed to enable v2 complete authorize functionality.Would you like me to help implement this function based on the v1 implementation pattern?
crates/router/src/core/payments/flows/cancel_flow.rs (1)
29-29: Implement the cancel flow v2 functionality.The
todo!()placeholder indicates incomplete implementation. The cancel flow needs to be implemented for v2 to support merchant connector details.Do you need assistance implementing the v2 cancel flow logic?
crates/router/src/core/payments/flows/approve_flow.rs (1)
30-30: Complete the approve flow v2 implementation.The function body contains only a
todo!()placeholder. This needs to be implemented to enable v2 approve flow functionality with merchant connector details.Would you like me to help create a tracking issue for implementing this function?
crates/router/src/core/fraud_check/flows/record_return.rs (1)
41-41: Implement fraud check record return v2 functionality.The
todo!()placeholder indicates that the v2 implementation for fraud check record return is incomplete. Note that this function requires bothv2andcustomer_v2feature flags.Do you want me to help implement this function or track it as technical debt?
crates/router/src/core/payments/operations/payment_get.rs (1)
5-6: Unused importpaymeThe new
use hyperswitch_connectors::connectors::payme;line is not referenced anywhere
in this module, which will triggerunused_importswarnings (and possibly a build
failure if#![deny(warnings)]is enabled).-#[cfg(feature = "v2")] -use hyperswitch_connectors::connectors::payme; +// remove – not usedcrates/router/src/routes/payments.rs (1)
2995-3014: Authentication logic is overly complex and inconsistent with other handlers.The conditional authentication selection logic here is more complex than similar handlers in this file. Most other handlers use simpler patterns like direct
auth::auth_type()calls or straightforward conditional logic.Consider simplifying to match the pattern used in other handlers:
- let auth_type = if state.conf.merchant_id_auth.merchant_id_auth_enabled { - &auth::MerchantIdAuth - } else { - match env::which() { - env::Env::Production => &auth::V2ApiKeyAuth { - is_connected_allowed: false, - is_platform_allowed: false, - }, - _ => auth::auth_type( - &auth::V2ApiKeyAuth { - is_connected_allowed: false, - is_platform_allowed: false, - }, - &auth::JWTAuth { - permission: Permission::ProfilePaymentWrite, - }, - req.headers(), - ), - } - }; + let auth_type = if state.conf.merchant_id_auth.merchant_id_auth_enabled { + &auth::MerchantIdAuth + } else { + auth::auth_type( + &auth::V2ApiKeyAuth { + is_connected_allowed: false, + is_platform_allowed: false, + }, + &auth::JWTAuth { + permission: Permission::ProfilePaymentWrite, + }, + req.headers(), + ) + };This removes the environment-specific logic and makes the authentication selection more predictable.
crates/router/src/core/payments/customers.rs (1)
163-182: Unnecessaryasyncmarker inflates call-stack
update_connector_customer_in_customersis declaredasyncbut performs no.awaitinside either match arm; the whole body is fully synchronous. Compiling this asasynccauses an extra state-machine to be generated for every call and forces callers to.awaitneedlessly.-#[instrument] -pub async fn update_connector_customer_in_customers( +#\[instrument] +pub fn update_connector_customer_in_customers(Switching to a plain function will reduce allocations and simplify the call-graph.
crates/router/src/core/payments/operations/payment_confirm_intent.rs (1)
492-506: Duplicate “get_connector_from_request” implementation – factor into shared helperThis block is identical to the implementation in
payment_get.rs. To avoid future drift, pull it into a small helper (e.g.helpers::set_connector_from_request) and call it from both places.crates/router/src/core/payment_methods/vault.rs (3)
1390-1391: PassMerchantConnectorAccountTypeDetailsby reference to avoid movesThe function now takes
merchant_connector_accountby value but subsequently borrows it multiple times; this forces an unnecessary move (and Box allocation upstream). Accepting&MerchantConnectorAccountTypeDetailskeeps the call-site simpler and zero-copy:- merchant_connector_account: domain::MerchantConnectorAccountTypeDetails, + merchant_connector_account: &domain::MerchantConnectorAccountTypeDetails,You’ll then drop the leading
&when passing it further down.
1566-1576: RedundantBoxallocationWrapping the fetched MCA in
Box::new(...)just to fit the enum variant inflates the heap and complicates ownership:domain::MerchantConnectorAccountTypeDetails::MerchantConnectorAccount( Box::new(account), )If the enum does not strictly need a boxed value, make the variant hold the struct directly; otherwise obtain a boxed account once at the helper and reuse it.
1634-1635: Same by-value issue as retrieve – please switch to reference
delete_payment_method_data_from_vault_externalmoved to by-value as well; same reasoning as earlier applies here.crates/router/src/core/payments/helpers.rs (1)
1605-1626: Consider taking a non-optional parameter if always requiredThe function takes
Option<api_models::payments::MerchantConnectorDetails>but immediately returns an error if it'sNone. If this parameter is always required, consider changing the signature to take a non-optional parameter to make the API clearer.#[cfg(feature = "v2")] pub async fn get_connector_data_from_request( state: &SessionState, - req: Option<api_models::payments::MerchantConnectorDetails>, + req: api_models::payments::MerchantConnectorDetails, ) -> CustomResult<api::ConnectorData, errors::ApiErrorResponse> { - let connector = req - .as_ref() - .map(|connector_details| connector_details.connector_name.to_string()) - .ok_or(errors::ApiErrorResponse::MissingRequiredField { - field_name: "merchant_connector_details", - })?; + let connector = req.connector_name.to_string(); let connector_data: api::ConnectorData = api::ConnectorData::get_connector_by_name( &state.conf.connectors, &connector, api::GetToken::Connector, None, ) .change_context(errors::ApiErrorResponse::InternalServerError) .attach_printable("Invalid connector name received")?; Ok(connector_data) }crates/hyperswitch_domain_models/src/merchant_connector_account.rs (1)
145-161: Consider consolidating duplicate ID retrieval logic.Both
get_idandget_mca_idmethods implement similar logic for theMerchantConnectorAccountvariant. Theget_idmethod directly accesses theidfield whileget_mca_idcalls theget_id()method.Consider having
get_iddelegate toget_mca_idfor consistency:pub fn get_id(&self) -> Option<id_type::MerchantConnectorAccountId> { - match self { - Self::MerchantConnectorAccount(merchant_connector_account) => { - Some(merchant_connector_account.id.clone()) - } - Self::MerchantConnectorDetails(_) => None, - } + self.get_mca_id() }crates/api_models/src/payments.rs (1)
279-287:❓ Verification inconclusive
Consider security implications of accepting connector credentials in API requests
The
MerchantConnectorDetailsstruct accepts connector credentials directly in API requests. While the credentials are wrapped inpii::SecretSerdeValue, ensure that:
- These credentials are properly validated and authenticated
- Audit logs appropriately handle these sensitive fields
- The credentials are not persisted in request logs
Run the following script to check how these credentials are handled in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify security handling of merchant_connector_creds # Check validation of merchant_connector_details echo "=== Checking validation of merchant_connector_details ===" rg -A 10 "merchant_connector_details" --type rust | grep -E "(validate|verify|check|auth)" # Check if credentials are logged echo -e "\n=== Checking if credentials might be logged ===" rg -A 5 "(log|trace|debug|info|warn|error).*merchant_connector" --type rust # Check how pii::SecretSerdeValue is handled echo -e "\n=== Checking SecretSerdeValue handling ===" rg -A 5 "SecretSerdeValue" --type rust | head -50Length of output: 13696
Double-check security handling of MerchantConnectorDetails
Our search didn’t reveal any explicit validation or logging redaction for the
merchant_connector_credsfield wrapped inpii::SecretSerdeValue. Please verify that:
- Credentials are validated/authenticated against the connector before use (e.g. in
payment_complete_authorizeincrates/router/src/core/payments/operations/payment_complete_authorize.rs).- All logs (error, warn, info, debug, trace) redact or omit the raw
SecretSerdeValue—no credentials should end up in request logs.- Audit trails record when credentials are used without ever persisting or exposing the secret.
- The mapping from
merchant_connector_credstocreds_identifiercorrectly handles and discards the raw secret after validation.crates/router/src/core/payments/vault_session.rs (3)
79-87: Unnecessary DB round-trip whenexternal_vault_sourceisNone
helpers::get_merchant_connector_account_v2is invoked even whenexternal_vault_sourceisNone.
If the vault profile doesn’t configure a dedicated connector, the extra query returns the default MCA anyway – costing an extra DB hit on every request.if is_external_vault_sdk_enabled { - let external_vault_source = profile - .external_vault_connector_details - .as_ref() - .map(|details| &details.vault_connector_id); - - let merchant_connector_account = - domain::MerchantConnectorAccountTypeDetails::MerchantConnectorAccount(Box::new( - helpers::get_merchant_connector_account_v2( - state, - merchant_context.get_merchant_key_store(), - external_vault_source, - ) - .await?, - )); + let Some(vault_connector_id) = profile + .external_vault_connector_details + .as_ref() + .map(|d| &d.vault_connector_id) + else { + // No dedicated vault connector configured – nothing to populate. + return Ok(()); + }; + + let merchant_connector_account = domain::MerchantConnectorAccountTypeDetails::MerchantConnectorAccount( + Box::new( + helpers::get_merchant_connector_account_v2( + state, + merchant_context.get_merchant_key_store(), + Some(vault_connector_id), + ) + .await?, + ), + );This short-circuit avoids an extra query when a vault connector isn’t explicitly configured.
223-237: More explicit error response would aid client debuggingAll parsing failures of connector auth collapse into
InternalServerError.
Consider distinguishing between “invalid credentials” (4xx) and “unexpected parsing failure” (5xx) to help API consumers act on configuration errors quicker.Example:
-.map_err(|err| { - err.change_context(errors::ApiErrorResponse::InternalServerError) +.map_err(|err| { + err.change_context(errors::ApiErrorResponse::InvalidDataFormat) // 422
288-299: Minor: avoid cloning large enum instances
merchant_connector_account_typeis passed as&…throughout, but converted into owned values in deep calls (e.g.,get_mca_id,construct_vault_router_data).
ImplementingCopy/Clone-free helpers that return borrowed data (Option<&str>) would save heap allocations for every vault call.crates/router/src/core/payments.rs (3)
325-399: Struct-destructuring is brittle – add..to future-proof the pattern
operations::GetTrackerResponseis destructured without...
The moment a new field is appended to the struct, this line will break compilation.-let operations::GetTrackerResponse { mut payment_data } = get_tracker_response; +let operations::GetTrackerResponse { mut payment_data, .. } = get_tracker_response;While you’re here, consider renaming the boxed variable (
operation) to avoid shadowing the parameter — that would improve readability.
3586-3595: Repeated MCA retrieval – extract into a helperThe block that wraps the MCA in
MerchantConnectorAccountTypeDetails::MerchantConnectorAccountappears verbatim in several places. Extracting it into a small helper (e.g.build_mca_details(state, merchant_context, id).await?) would reduce duplication and potential drift.
4358-4373: Duplicate MCA retrieval – candidate for DRYing upSame retrieval logic as in the single-connector path. Centralising this into a helper would keep the multiconnector and single-connector flows in sync automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
crates/api_models/src/payments.rs(8 hunks)crates/common_utils/src/id_type/merchant.rs(2 hunks)crates/hyperswitch_domain_models/src/customer.rs(2 hunks)crates/hyperswitch_domain_models/src/merchant_connector_account.rs(1 hunks)crates/hyperswitch_domain_models/src/payments.rs(4 hunks)crates/hyperswitch_domain_models/src/payments/payment_attempt.rs(2 hunks)crates/hyperswitch_domain_models/src/revenue_recovery.rs(1 hunks)crates/openapi/src/openapi_v2.rs(1 hunks)crates/router/src/core/fraud_check/flows/checkout_flow.rs(1 hunks)crates/router/src/core/fraud_check/flows/record_return.rs(1 hunks)crates/router/src/core/fraud_check/flows/sale_flow.rs(1 hunks)crates/router/src/core/fraud_check/flows/transaction_flow.rs(1 hunks)crates/router/src/core/payment_methods.rs(3 hunks)crates/router/src/core/payment_methods/vault.rs(4 hunks)crates/router/src/core/payments.rs(19 hunks)crates/router/src/core/payments/customers.rs(2 hunks)crates/router/src/core/payments/flows.rs(1 hunks)crates/router/src/core/payments/flows/approve_flow.rs(1 hunks)crates/router/src/core/payments/flows/authorize_flow.rs(1 hunks)crates/router/src/core/payments/flows/cancel_flow.rs(1 hunks)crates/router/src/core/payments/flows/capture_flow.rs(1 hunks)crates/router/src/core/payments/flows/complete_authorize_flow.rs(1 hunks)crates/router/src/core/payments/flows/incremental_authorization_flow.rs(1 hunks)crates/router/src/core/payments/flows/post_session_tokens_flow.rs(1 hunks)crates/router/src/core/payments/flows/psync_flow.rs(1 hunks)crates/router/src/core/payments/flows/reject_flow.rs(1 hunks)crates/router/src/core/payments/flows/session_flow.rs(1 hunks)crates/router/src/core/payments/flows/session_update_flow.rs(1 hunks)crates/router/src/core/payments/flows/setup_mandate_flow.rs(1 hunks)crates/router/src/core/payments/flows/update_metadata_flow.rs(1 hunks)crates/router/src/core/payments/helpers.rs(3 hunks)crates/router/src/core/payments/operations.rs(1 hunks)crates/router/src/core/payments/operations/payment_confirm_intent.rs(7 hunks)crates/router/src/core/payments/operations/payment_get.rs(3 hunks)crates/router/src/core/payments/operations/proxy_payments_intent.rs(2 hunks)crates/router/src/core/payments/transformers.rs(12 hunks)crates/router/src/core/payments/vault_session.rs(8 hunks)crates/router/src/core/payouts/helpers.rs(1 hunks)crates/router/src/core/revenue_recovery/api.rs(1 hunks)crates/router/src/core/utils.rs(1 hunks)crates/router/src/core/webhooks/incoming_v2.rs(2 hunks)crates/router/src/core/webhooks/recovery_incoming.rs(1 hunks)crates/router/src/routes/app.rs(1 hunks)crates/router/src/routes/payments.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
crates/router/src/routes/app.rs (2)
crates/router/src/routes/payments.rs (18)
payments(147-153)payments(218-224)payments(344-350)payments(401-408)payments(483-490)payments(560-567)payments(683-690)payments(743-750)payments(876-883)payments(940-947)payments(1006-1013)payments(1067-1074)payments(1346-1353)payments(1397-1404)payments(1766-1773)payments(1837-1844)payment_status(2868-2953)payments_status_with_gateway_creds(2957-3054)crates/openapi/src/routes/payments.rs (1)
payment_status(783-783)
crates/common_utils/src/id_type/merchant.rs (2)
crates/common_utils/src/id_type/payment.rs (1)
from_str(93-96)crates/common_utils/src/id_type/profile.rs (1)
from_str(29-32)
crates/router/src/core/payments/operations.rs (2)
crates/router/src/core/payments/operations/payment_confirm_intent.rs (1)
get_connector_from_request(492-506)crates/router/src/core/payments/operations/payment_get.rs (1)
get_connector_from_request(349-366)
crates/router/src/core/payments/operations/payment_confirm_intent.rs (3)
crates/router/src/core/payments/operations.rs (1)
get_connector_from_request(295-303)crates/router/src/core/payments/operations/payment_get.rs (1)
get_connector_from_request(349-366)crates/router/src/core/payments/helpers.rs (1)
get_connector_data_from_request(1606-1625)
crates/hyperswitch_domain_models/src/merchant_connector_account.rs (2)
crates/router/src/core/payments/helpers.rs (6)
get_connector_account_details(3935-3940)is_disabled(3949-3956)get_metadata(3928-3933)get_mca_id(3971-3976)get_connector_name(3979-3984)get_connector_name(3987-3992)crates/diesel_models/src/merchant_connector_account.rs (2)
get_id(62-64)get_id(108-110)
crates/router/src/routes/payments.rs (4)
crates/api_models/src/payments.rs (5)
default(1907-1909)default(4292-4294)new(790-801)new(1793-1807)new(1846-1851)crates/router/src/core/payments.rs (9)
state(6673-6679)payments_core(1709-1771)payments_core(2154-2251)payments_core(2333-2340)payments_core(2353-2360)payments_core(2578-2585)payments_core(2745-2745)payments_core(3081-3088)payments_core(3120-3120)crates/router/src/services/api.rs (1)
server_wrap(732-922)crates/router/src/services/authentication.rs (2)
auth_type(4393-4404)new(1907-1909)
🔇 Additional comments (74)
crates/router/src/core/revenue_recovery/api.rs (1)
35-35: LGTM: Consistent addition of merchant_connector_details field.The explicit initialization to
Nonemaintains backward compatibility while supporting the new merchant connector details feature in the API. This is appropriate for revenue recovery flows that rely on existing database configurations.crates/hyperswitch_domain_models/src/revenue_recovery.rs (1)
214-214: LGTM: Consistent field addition in domain model conversion.The addition of
merchant_connector_details: Nonein the conversion fromRevenueRecoveryInvoiceDatatoPaymentsCreateIntentRequestmaintains compatibility with the updated API structure while preserving existing behavior.crates/router/src/core/webhooks/recovery_incoming.rs (1)
490-490: LGTM: Consistent field initialization in webhook processing.The addition of
merchant_connector_details: Noneto thePaymentsRetrieveRequestconstruction maintains existing webhook processing behavior while supporting the updated API structure.crates/router/src/core/payments/operations/proxy_payments_intent.rs (2)
264-264: LGTM: Consistent field addition to PaymentConfirmData.The initialization of
merchant_connector_details: Nonefollows the established pattern and maintains existing proxy payment behavior while supporting the new API structure.
365-373: LGTM: Appropriate handling of optional merchant_connector_id.The wrapping of
merchant_connector_idinSome()reflects the type change to optional as part of the newMerchantConnectorAccountTypeDetailsabstraction. The error handling logic remains consistent while properly handling the optional nature of the field.crates/router/src/core/payments/flows/incremental_authorization_flow.rs (1)
56-61: Flag incomplete implementation.The parameter type change to
domain::MerchantConnectorAccountTypeDetailsaligns with the new abstraction, but thetodo!()implementation indicates incomplete functionality.Please confirm whether this incremental authorization flow is currently used in production, and if so, provide a timeline for completing the implementation. Incomplete implementations in production code paths could lead to runtime panics.
crates/common_utils/src/id_type/merchant.rs (2)
6-6: LGTM: Import addition is appropriate.The addition of
str::FromStrimport supports the new trait implementation.
41-48: LGTM: Consistent implementation following established patterns.The
FromStrimplementation follows the exact same pattern used by other ID types in the codebase (PaymentId,ProfileId), usingCow::Ownedand delegating to the existingtry_fromlogic. This enables string-based parsing of merchant identifiers, which aligns with the PR objective of supporting merchant connector details in payment flows.crates/router/src/core/payments/flows.rs (1)
78-78: LGTM: Type update aligns with merchant connector abstraction refactor.The parameter type change from
MerchantConnectorAccountTypetoMerchantConnectorAccountTypeDetailsis consistent with the broader refactoring effort to unify handling of merchant connector account information, supporting both database and direct credential flows as mentioned in the PR objectives.crates/router/src/routes/app.rs (1)
663-667: LGTM! Clean implementation of dual HTTP method support.The routing configuration correctly adds POST method support alongside the existing GET method for payment status retrieval. This follows RESTful design patterns where:
- GET method uses query parameters for simple status checks
- POST method accepts JSON payload with merchant connector details
The implementation aligns with the PR objective of supporting
merchant_connector_detailsin the Payments and Psync flow.crates/router/src/core/webhooks/incoming_v2.rs (1)
477-477: LGTM - Correct field initialization for webhook flows.The initialization of
merchant_connector_details: Noneis appropriate for webhook handling, as incoming webhooks don't typically carry merchant connector credentials. The changes correctly integrate with the new field addition to payment request and response structures.Also applies to: 682-682
crates/router/src/core/fraud_check/flows/sale_flow.rs (1)
34-34: Parameter type update aligns with merchant connector abstraction.The change from
&domain::MerchantConnectorAccountto&domain::MerchantConnectorAccountTypeDetailsis consistent with the broader refactoring to use the new enum abstraction that supports both database-backed accounts and direct merchant connector details.crates/router/src/core/payments/flows/authorize_flow.rs (1)
43-43: Parameter type update supports enhanced connector account handling.The migration to
&domain::MerchantConnectorAccountTypeDetailsenables the authorize flow to handle both traditional database-backed merchant connector accounts and direct merchant connector details from API requests, supporting the new payment flow flexibility.crates/router/src/core/fraud_check/flows/transaction_flow.rs (1)
39-39: Consistent parameter type migration for fraud check flows.The update to
&domain::MerchantConnectorAccountTypeDetailsmaintains consistency with the broader refactoring effort to support the new merchant connector account abstraction across all payment and fraud check flows.crates/router/src/core/payments/flows/reject_flow.rs (1)
52-52: Parameter type update completes flow consistency.The change to
&domain::MerchantConnectorAccountTypeDetailsensures the reject flow follows the same parameter type pattern as other payment flows, supporting the unified merchant connector account abstraction.crates/openapi/src/openapi_v2.rs (1)
424-426: LGTM: OpenAPI schema additions align with PR objectives.The addition of these three new payment-related models to the OpenAPI schema correctly exposes the new merchant connector details functionality. This ensures proper API documentation for the enhanced payment flows.
crates/router/src/core/payments/flows/update_metadata_flow.rs (1)
55-55: Parameter type update aligns with merchant connector details refactor.The change from
&helpers::MerchantConnectorAccountTypeto&domain::MerchantConnectorAccountTypeDetailsis consistent with the broader refactor to support the new merchant connector details abstraction. This signature change prepares the v2 flow for the new enum type.crates/router/src/core/payments/flows/post_session_tokens_flow.rs (1)
56-56: Parameter type update consistent with refactor pattern.This parameter type change from
&helpers::MerchantConnectorAccountTypeto&domain::MerchantConnectorAccountTypeDetailsfollows the same pattern as other flow implementations, ensuring consistency across the v2 payment flows.crates/router/src/core/payments/operations.rs (1)
294-303: New method supports v2 connector data extraction from requests.The addition of
get_connector_from_requestmethod to theDomaintrait enables v2 payment flows to extract connector data directly from requests containingmerchant_connector_details. The default implementation provides clear error messaging for unimplemented operations.Based on the relevant code snippets, concrete implementations already exist in specific operations like
payment_confirm_intent.rsandpayment_get.rsthat properly extract connector data and update payment attempts.crates/router/src/core/payments/flows/capture_flow.rs (1)
69-69: Parameter type change aligns with merchant connector details abstraction.The update from
&domain::MerchantConnectorAccountto&domain::MerchantConnectorAccountTypeDetailsin the v2 capture flow implementation is consistent with the broader refactor to support the new merchant connector details enum abstraction. This enables the capture flow to work with both traditional merchant connector accounts and new merchant connector credentials.crates/router/src/core/payments/flows/setup_mandate_flow.rs (1)
79-79:✅ Verification successful
LGTM! Parameter type correctly updated for v2 implementation.
The change from
&domain::MerchantConnectorAccountto&domain::MerchantConnectorAccountTypeDetailscorrectly aligns with the new abstraction that supports both database-backed merchant connector accounts and direct merchant connector credentials from API requests.Let me verify that this change is consistent with other payment flow files that were part of this refactor:
🏁 Script executed:
#!/bin/bash # Description: Verify parameter type consistency across payment flow files # Expected: All v2 construct_router_data methods should use MerchantConnectorAccountTypeDetails echo "Checking parameter types in construct_router_data methods across payment flows..." fd ".*_flow\.rs$" crates/router/src/core/payments/flows/ --exec grep -l "construct_router_data" {} \; | while read file; do echo "=== $file ===" rg -A 10 "async fn construct_router_data" "$file" | rg "merchant_connector_account:" doneLength of output: 6112
Consistent v2
construct_router_dataParameters ConfirmedAll payment-flow implementations under the v2 feature flag now uniformly accept
&domain::MerchantConnectorAccountTypeDetailsfor themerchant_connector_accountparameter. No discrepancies were found across*_flow.rsfiles.crates/hyperswitch_domain_models/src/payments/payment_attempt.rs (2)
1756-1756: LGTM: Field optionality correctly implements flexible connector handling.Making
merchant_connector_idoptional in theConfirmIntentvariant aligns with the PR objective to support both database-backed merchant connector accounts and direct credential flows. This change enables the newMerchantConnectorAccountTypeDetailsabstraction.
2551-2551: LGTM: Conversion logic correctly handles optional field.The direct assignment of
merchant_connector_idis appropriate since the field is now optional in the source enum variant. This eliminates the need to wrap an already-optional value inSome().crates/router/src/core/payments/flows/session_update_flow.rs (1)
56-56: Type change aligns with the merchant connector refactor.The parameter type change from
&domain::MerchantConnectorAccountto&domain::MerchantConnectorAccountTypeDetailsis consistent with the broader refactor to support both database-backed and API-provided merchant connector credentials.crates/router/src/core/payments/flows/complete_authorize_flow.rs (1)
63-63: Type change is consistent with the refactor pattern.The parameter type update to
&domain::MerchantConnectorAccountTypeDetailsfollows the same pattern as other payment flows and supports the new merchant connector details functionality.crates/router/src/core/payments/flows/cancel_flow.rs (1)
25-25: Parameter type update follows established pattern.The change to
&domain::MerchantConnectorAccountTypeDetailsis consistent with the merchant connector account refactor across all payment flows.crates/router/src/core/payments/flows/approve_flow.rs (1)
26-26: Type signature update aligns with refactor goals.The parameter type change to
&domain::MerchantConnectorAccountTypeDetailsmaintains consistency with the merchant connector account handling refactor.crates/router/src/core/fraud_check/flows/record_return.rs (1)
36-36: Fraud check flow updated consistently with payment flows.The parameter type change to
&domain::MerchantConnectorAccountTypeDetailsextends the merchant connector account refactor to fraud check flows, maintaining consistency across the codebase.crates/hyperswitch_domain_models/src/customer.rs (2)
23-24: LGTM: Clean import addition for the new enum type.The conditional import is properly scoped under the appropriate feature flags.
104-118: LGTM: Correct implementation of enum pattern matching.The method properly handles both variants of
MerchantConnectorAccountTypeDetails:
- For
MerchantConnectorAccount: Maintains existing logic to lookup connector customer ID- For
MerchantConnectorDetails: ReturnsNone, which is appropriate since direct credentials don't have stored connector customer mappingsThe pattern matching is exhaustive and follows Rust best practices.
crates/router/src/core/payments/flows/session_flow.rs (1)
43-43: LGTM: Signature update aligns with enum refactor.The parameter type change from
&helpers::MerchantConnectorAccountTypeto&domain::MerchantConnectorAccountTypeDetailsis consistent with the broader refactor to support both database-backed accounts and direct merchant connector credentials.crates/router/src/core/payouts/helpers.rs (1)
1036-1036: LGTM! Parameter type change aligns with the refactoring objectives.The parameter type change from
id_type::MerchantConnectorAccountIdtodomain::MerchantConnectorAccountTypeDetailscorrectly supports the new feature for accepting dynamic merchant connector credentials while maintaining the same function logic. This change is appropriately scoped to the v2 + customer_v2 features and is consistent with the broader refactoring described in the PR objectives.crates/router/src/core/utils.rs (1)
2043-2054: API type migration looks correct but double-check the return type ofget_connector_account_details()You switched from the legacy parse-then-convert path to directly using the value returned by
get_connector_account_details(), assuming it is already atypes::ConnectorAuthType.If that helper still returns a
serde_json::Value(or a raw string) in any variant, the implicit
conversion you dropped will break at runtime. Please confirm the signature:fn get_connector_account_details(&self) -> CustomResult<types::ConnectorAuthType /* ? */>If the function still returns a generic
Value, you need to restore theparse_value::<ConnectorAuthType>step.crates/router/src/core/payments/operations/payment_get.rs (1)
238-248: Good addition: persistingmerchant_connector_detailsinPaymentStatusDataPersisting the cloned
merchant_connector_detailsensures downstream flows
(e.g. PSync, Vault) can retrieve connector data without re-parsing the request.
Nice catch.crates/hyperswitch_domain_models/src/payments.rs (5)
5-5: LGTM: Import addition is correctly used.The new import for
MerchantConnectorDetailsis properly utilized in the struct field definitions below.
45-47: LGTM: Import updates align with the refactor.The addition of
MerchantConnectorAccountTypeDetailsimport is consistent with the broader codebase refactor to support the new enum abstraction.
860-860: LGTM: Consistent field addition for payment confirmation flow.The
merchant_connector_detailsfield addition toPaymentConfirmDataaligns with the PR objective to support direct credential passing in payment flows.
868-880: LGTM: Well-implemented polymorphic logic for connector customer ID retrieval.The updated method correctly handles both enum variants:
- For stored accounts (
MerchantConnectorAccount), it maintains existing logic to retrieve connector customer ID from customer or feature metadata- For direct credentials (
MerchantConnectorDetails), it appropriately returnsNonesince there wouldn't be a pre-existing connector customer IDThis implementation maintains backward compatibility while supporting the new direct credential flow.
913-913: LGTM: Consistent field addition for payment status flow.The
merchant_connector_detailsfield addition toPaymentStatusDatais consistent with the same field added toPaymentConfirmData, ensuring uniform support across both payment and psync flows as intended.crates/router/src/core/fraud_check/flows/checkout_flow.rs (1)
38-38: LGTM: Parameter type update aligns with codebase refactor.The parameter type change from
&domain::MerchantConnectorAccountto&domain::MerchantConnectorAccountTypeDetailsis consistent with the broader refactor to use the new enum abstraction. Since this function is marked astodo!(), this change appropriately prepares the signature for future implementation.crates/router/src/core/payment_methods.rs (3)
1878-1878: LGTM! Type parameter update aligns with unified merchant connector account handling.This change correctly updates the function parameter to use the new
MerchantConnectorAccountTypeDetailsabstraction, which is part of the broader refactoring to unify merchant connector account handling across the codebase.
1981-1991: LGTM! Correct implementation of the new merchant connector account abstraction.The change properly:
- Uses the new
get_merchant_connector_account_v2helper function- Wraps the result in
MerchantConnectorAccountTypeDetails::MerchantConnectorAccount(Box::new(...))enum variant- Maintains the same error handling and context attachment
This correctly implements the unified merchant connector account handling pattern.
2952-2952: LGTM! Appropriate field addition for zero-auth payments.Setting
merchant_connector_details: Nonefor zero-auth payments is correct since these payments are typically used for payment method validation rather than actual processing, and specific merchant connector details may not be required in this context.crates/router/src/routes/payments.rs (1)
871-890:✅ Verification successful
Verify field mapping completeness in payload transformation.
The transformation from
PaymentsStatusRequesttoPaymentsRetrieveRequestonly maps specific fields (force_sync,expand_attempts,param,all_keys_required) while using..Default::default()for others. Ensure that no critical fields are being inadvertently omitted in this conversion.
🏁 Script executed:
#!/bin/bash # Description: Compare field definitions between PaymentsStatusRequest and PaymentsRetrieveRequest # to ensure no important fields are missing from the mapping # Search for PaymentsStatusRequest field definitions echo "=== PaymentsStatusRequest fields ===" ast-grep --pattern 'struct PaymentsStatusRequest { $$$ }' echo -e "\n=== PaymentsRetrieveRequest fields ===" ast-grep --pattern 'struct PaymentsRetrieveRequest { $$$ }' echo -e "\n=== Check for merchant_connector_details usage ===" rg -A 3 -B 3 "merchant_connector_details" --type rustLength of output: 54971
🏁 Script executed:
#!/bin/bash # Locate the struct definitions to compare their fields echo "=== PaymentsStatusRequest definition ===" rg -n "struct PaymentsStatusRequest" -A 20 crates/api_models/src/payments.rs echo -e "\n=== PaymentsRetrieveRequest definition ===" rg -n "struct PaymentsRetrieveRequest" -A 20 crates/api_models/src/payments.rsLength of output: 4955
🏁 Script executed:
#!/bin/bash # Extract the mapping of PaymentsStatusRequest into the internal request echo "=== Mapping PaymentsStatusRequest to internal payload ===" rg -n "PaymentsStatusRequest" -C 10 crates/router/src/routes/payments.rs # Also inspect where PaymentsRetrieveRequest is constructed for the status handler echo -e "\n=== Construction of PaymentsRetrieveRequest in status handler ===" rg -n "PaymentsRetrieveRequest" -C 10 crates/router/src/routes/payments.rsLength of output: 6424
Field mapping is correct and intentional.
The
payment_statushandler usesPaymentsStatusRequest, which only definesforce_sync,expand_attempts,param, andall_keys_required. All other fields (includingmerchant_connector_details) correctly default toNone. The newpayments_status_with_gateway_credsendpoint explicitly handlesmerchant_connector_detailsvia its own request type. No changes needed.crates/router/src/core/payments/customers.rs (1)
107-131: Risk of silently skipping customer-creation for credential-based connectionsFor the
MerchantConnectorDetailsvariant the function hard-codes(false, None), i.e. it never calls the connector even if that connector appears inconnector_customer.connector_list.
If any connector still mandates a customer object while being driven by runtime credentials, this branch will suppress the create-customer call and you’ll hit connector-side validation errors at run-time.Please double-check business requirements; if customer-creation may still be required with direct credentials, consider keeping the existing logic and falling back only when the connector explicitly states it is unnecessary.
crates/router/src/core/payments/operations/payment_confirm_intent.rs (1)
152-154: Inconsistent identifier type betweenget_trackersand callers
payment_idswitched to&id_type::GlobalPaymentId. Verify every caller (handlers, tests, cron jobs) now passes aGlobalPaymentId; older code paths that still send a plainPaymentIdwill fail to compile.crates/router/src/core/payments/helpers.rs (1)
7344-7363:❓ Verification inconclusive
Verify the impact of removing Redis caching and credential lookup
The
get_merchant_connector_account_v2function has been significantly simplified by removing:
- Redis caching functionality
- Credential identifier-based lookup
merchant_idandcreds_identifierparametersThis is a breaking change that could impact performance if the Redis cache was providing significant benefits. Please ensure that:
- This simplification is intentional
- Any code that depended on the removed functionality has been updated
- The performance impact has been evaluated
🏁 Script executed:
#!/bin/bash # Check for any remaining usages of the old function signature with merchant_id or creds_identifier rg -A 5 "get_merchant_connector_account_v2.*merchant_id|get_merchant_connector_account_v2.*creds_identifier" # Check for any code that might be expecting the Redis caching behavior rg -A 10 "get_merchant_connector_account_v2" | grep -i "redis\|cache"Length of output: 180
Verify removal of Redis caching and credential‐lookup
Our search across the repo found no remaining calls toget_merchant_connector_account_v2using the oldmerchant_id/creds_identifiersignature, nor any references to Redis or “cache” around its call sites. It appears all invocations have been updated to the new, simplified version.Please confirm that:
- Removing Redis caching and the credential-identifier lookup is intentional.
- Any code that depended on the old behavior has been updated or deprecated.
- You’ve evaluated the performance impact of dropping the cache (e.g., via benchmarks or metrics).
crates/hyperswitch_domain_models/src/merchant_connector_account.rs (6)
96-101: LGTM: Well-designed enum abstraction.The enum provides a clean polymorphic interface over two different representations of merchant connector account details. Using
Box<MerchantConnectorAccount>is good for performance, avoiding expensive copying of large structs.
105-125: LGTM: Consistent connector account details parsing.The method properly handles both variants with consistent error handling using
parse_value("ConnectorAuthType"). The use ofpeek()for theMerchantConnectorAccountvariant is appropriate for v2 implementation.
127-134: LGTM: Appropriate disabled status handling.The logic correctly returns the actual disabled status for
MerchantConnectorAccountand defaults tofalseforMerchantConnectorDetails, which is reasonable since dynamically provided credentials shouldn't be considered disabled.
136-143: LGTM: Metadata handling is appropriate.Correctly returns metadata for
MerchantConnectorAccountandNoneforMerchantConnectorDetails, which is expected since dynamically provided connector details don't include metadata.
163-170: LGTM: Connector name retrieval is properly implemented.The method appropriately returns the connector name only for the
MerchantConnectorAccountvariant, which makes sense sinceMerchantConnectorDetailsdoesn't contain this information.
172-180: LGTM: Inner database access method is well-designed.This method provides safe access to the underlying
MerchantConnectorAccountwhen available, returningNonefor theMerchantConnectorDetailsvariant. This is useful for operations that specifically require database-backed account information.crates/router/src/core/payments/transformers.rs (5)
186-186: LGTM: Parameter type change is consistent across all functions.The parameter type change from
&domain::MerchantConnectorAccountto&domain::MerchantConnectorAccountTypeDetailsis consistently applied across all the payment router data construction functions. This aligns with the PR objective of introducing the new abstraction to support both database and direct credential flows.Also applies to: 412-412, 575-575, 699-699, 881-881
226-235: Correct conditional webhook URL generation logic.The webhook URL generation is properly conditioned based on the enum variant:
- Generates webhook URL only for
MerchantConnectorAccount(database entities)- Returns
NoneforMerchantConnectorDetails(ephemeral request data)This is the correct behavior since webhook URLs are only relevant for persisted connector accounts, not for direct connector credentials passed in API requests.
Also applies to: 922-931
906-906: Method signature update aligns with new enum type.The call to
customer.get_connector_customer_id(merchant_connector_account)now passes the entireMerchantConnectorAccountTypeDetailsenum instead of just an ID. This suggests the method signature was properly updated to handle both enum variants and extract the appropriate identifier based on the variant type.
1848-1848: Simplified merchant_connector_id handling is appropriate.The removal of error handling around
.get_required_value()and direct cloning ofmerchant_connector_idsuggests this field is now properly optional at the model level. This simplification aligns with making the field optional to support scenarios where merchant connector details are provided directly in the request rather than referenced by ID.Also applies to: 1903-1903
1914-1914: Addition of merchant_reference_id in responses supports PR objectives.The inclusion of
merchant_reference_idin thePaymentsResponsealigns with the PR objective of including this field in payment responses. This provides better traceability and supports the merchant's own reference system for payments.Also applies to: 2009-2009
crates/api_models/src/payments.rs (1)
5796-5802: LGTM! Good addition for payment idempotencyThe
merchant_reference_idfield is well-documented with appropriate schema constraints (min/max length of 30) and a clear example.crates/router/src/core/payments/vault_session.rs (3)
136-174: Parameter type updated – please cascade unit-test coverageThe signature now accepts
&MerchantConnectorAccountTypeDetails, broadening the surface to handle both DB-backed and request-supplied credentials.
Current unit tests only cover the DB MCA variant. Add tests exercising the newMerchantConnectorDetailspath (e.g., missing MCA ID, auth parsing) to prevent regressions.
331-332: Security footnote – runtime token selection
get_mca_id()returnsNonefor request-supplied credentials. Ensure the downstreamget_connector_by_namegracefully falls back to global connector tokens and never picks up an unrelated MCA’s token set.
193-198:❌ Incorrect review comment
Possible error silently ignored from
update_connector_customer_in_customersThe result of
update_connector_customer_in_customersis assigned without?.
If the helper returnsResult<…>, the error would be swallowed and wrapped inOk(customer_update), changing the function’s return type and masking failures.-let customer_update = customers::update_connector_customer_in_customers( +let customer_update = customers::update_connector_customer_in_customers( merchant_connector_account_type, customer.as_ref(), connector_customer_id.clone(), -) -.await; +) +.await?;Please confirm the helper’s signature and propagate errors if applicable.
🏁 Script executed:
#!/bin/bash # Verify the actual return type of the helper. rg -n "fn update_connector_customer_in_customers" -A3 -B1Length of output: 959
🏁 Script executed:
#!/bin/bash # Display the update_connector_customer_in_customers function signatures with return types sed -n '130,200p' crates/router/src/core/payments/customers.rsLength of output: 2336
Helper returns
Option, notResult
Theupdate_connector_customer_in_customersfunction’s signature ispub async fn update_connector_customer_in_customers(...) -> Option<storage::CustomerUpdate>Since it returns an
Optionrather than aResult, there is no error to propagate with?. The existing.awaitcall correctly yields anOption<…>, and no change is needed here.Likely an incorrect or invalid review comment.
crates/router/src/core/payments.rs (12)
1960-1964: Addition looks good
merchant_connector_detailsis now explicitly initialised – good defensive default.
1983-1987: LGTMConsistent initialisation of the new field.
1993-1997: LGTMSame as above – consistent defaulting.
2210-2241:Opmust beClone– confirm trait bounds
operation.clone()is invoked twice, but the generic bounds ofOpin this scope are not visible here.
IfOp: Cloneis not already enforced in the function signature, the code will fail to compile.Please verify that the
payments_coresignature (or a higher-level bound) includes:Op: Clone + /* existing bounds */
2873-2877: Looks goodNew field set to
None; keeps behaviour unchanged for the legacy flow.
4008-4014: Runtime failure possible –merchant_connector_detailsis mandatory
get_merchant_connector_details()is unwrapped withok_or_else, but earlier implementations of the trait returntodo!()orNone.
Unless every concreteDused in this path provides the field, the request will crash with a 500.Verify all implementers or provide a graceful error at validation time.
4865-4865: Signature update looks correctParameter updated to the enum wrapper – keeps the call-site consistent.
4886-4898: Confirmget_id()is implemented for the enum
merchant_connector_account.get_id()is invoked.
Ensure the newMerchantConnectorAccountTypeDetailsenum provides this method for all variants, or compilation will fail.If not, add:
impl MerchantConnectorAccountTypeDetails { pub fn get_id(&self) -> &str { /* match &self { … } */ } }
4918-4922: LGTMPassing the enum variant downstream keeps the customer update logic isolated.
9394-9398: Implementation looks correctReturns the cloned details – matches the trait contract.
9675-9679: Implementation looks correctSame as above for the status flow – good.
9927-9929: Setter fix is correctDirectly assigning to
payment_attempt.connectorprevents the earlier panic.
|
Payment-method changes looks good! |
| /// These are the query params that are sent in case of redirect response. | ||
| /// These can be ingested by the connector to take necessary actions. | ||
| pub param: Option<String>, |
There was a problem hiding this comment.
Nit: Can we come up with a better name for this field maybe?
| match env::which() { | ||
| env::Env::Production => &auth::V2ApiKeyAuth { | ||
| is_connected_allowed: false, | ||
| is_platform_allowed: false, | ||
| }, | ||
| _ => auth::auth_type( | ||
| &auth::V2ApiKeyAuth { | ||
| is_connected_allowed: false, | ||
| is_platform_allowed: false, | ||
| }, | ||
| &auth::JWTAuth { | ||
| permission: Permission::ProfilePaymentWrite, | ||
| }, | ||
| req.headers(), | ||
| ), | ||
| } |
There was a problem hiding this comment.
Nit: Do we need JWT auth here at all?
hyperswitch-bot
bot
added
the
M-api-contract-changes
Type of Change
Description
payments_status_with_gateway_credsAdditional Changes
Motivation and Context
How did you test it?
Response
Complete the payment using redirection url. use
successtest@iataupi for testingResponse
Checklist
cargo +nightly fmt --allcargo clippySummary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor