feat(core): [Retry] MIT Retries #8628
Conversation
|
Review changes with Changed Files
|
ShankarSinghC
left a comment
There was a problem hiding this comment.
Can you please add some test cases and also provide flow solution documentation ?
03ad3f1 to
1287b2f
Compare
| types::RouterData<F, FData, types::PaymentsResponseData>: Feature<F, FData>, | ||
| dyn api::Connector: services::api::ConnectorIntegration<F, FData, types::PaymentsResponseData>, | ||
| { | ||
| use hyperswitch_domain_models::ext_traits::OptionExt; |
| pub struct ConnectorRoutingData { | ||
| pub connector_data: ConnectorData, | ||
| pub network: Option<common_enums::CardNetwork>, | ||
| pub action_type: Option<ActionType>, |
There was a problem hiding this comment.
please add a comment that this is used for mandates currently. since action_type is not self-explanatory.
There was a problem hiding this comment.
this can be mandate_type or some other similar name.
Are we not modifying this?
| payment_data.get_payment_intent().setup_future_usage, | ||
| payment_data.get_payment_intent().off_session, | ||
| ) { | ||
| (Some(storage_enums::FutureUsage::OffSession), _) | (_, Some(true)) => { |
There was a problem hiding this comment.
if setup future usage is off-session - then it is considered as CIT, then we dont want to go through this snippet.
transaction is considered as Merchant Initiated transaction only when off-session is set to true which mean customer is not present.
crates/router/src/core/payments.rs
Outdated
|
|
||
| if is_mandate_flow { | ||
| if let Ok(connector_mandate_details) = connector_mandate_details { | ||
| let action_type = ActionType::ConnectorMandate(connector_mandate_details.to_owned()); |
There was a problem hiding this comment.
can we use extend() to minimize this nested id-else?
crates/router/src/core/payments.rs
Outdated
| if is_mandate_flow { | ||
| action_types.extend( | ||
| connector_mandate_details | ||
| .as_ref() | ||
| .ok() | ||
| .map(|details| ActionType::ConnectorMandate(details.to_owned())), | ||
| ); | ||
| } | ||
|
|
||
| // Collect network token with network transaction ID action types | ||
| if let IsNtWithNtiFlow::NtWithNtiSupported(network_transaction_id) = | ||
| is_network_token_with_ntid_flow | ||
| { | ||
| let connector_supports_both = ntid_supported_connectors.contains(&connector.connector_name) | ||
| && network_tokenization_supported_connectors.contains(&connector.connector_name); | ||
|
|
||
| if connector_supports_both { | ||
| action_types.extend( | ||
| network_tokenization::do_status_check_for_network_token(state, payment_method_info) | ||
| .await | ||
| { | ||
| Some(ActionType::NetworkTokenWithNetworkTransactionId( | ||
| NTWithNTIRef { | ||
| token_exp_month, | ||
| token_exp_year, | ||
| network_transaction_id, | ||
| }, | ||
| )) | ||
| } else { | ||
| None | ||
| } | ||
| .ok() | ||
| .map(|(token_exp_month, token_exp_year)| { | ||
| ActionType::NetworkTokenWithNetworkTransactionId(NTWithNTIRef { | ||
| token_exp_month, | ||
| token_exp_year, | ||
| network_transaction_id, | ||
| }) | ||
| }), | ||
| ); |
There was a problem hiding this comment.
instead of these many if-else, can we use builder pattern for the code optimization?
There was a problem hiding this comment.
refactored these with match statement with guards
83c54a2 to
df9f0ab
Compare
crates/router/src/core/payments.rs
Outdated
| if is_mandate_flow { | ||
| action_types.extend( | ||
| connector_mandate_details | ||
| .as_ref() | ||
| .ok() | ||
| .map(|details| ActionType::ConnectorMandate(details.to_owned())), | ||
| ); | ||
| } | ||
|
|
||
| let is_nt_with_ntid_supported_connector = ntid_supported_connectors | ||
| .contains(&connector.connector_name) | ||
| && network_tokenization_supported_connectors.contains(&connector.connector_name); | ||
|
|
||
| // Collect network token with network transaction ID action types | ||
| match is_network_token_with_ntid_flow { | ||
| IsNtWithNtiFlow::NtWithNtiSupported(network_transaction_id) | ||
| if is_nt_with_ntid_supported_connector => | ||
| { | ||
| action_types.extend( | ||
| network_tokenization::do_status_check_for_network_token(state, payment_method_info) |
There was a problem hiding this comment.
instead of if-else(s), use builder pattern for it
There was a problem hiding this comment.
for example -
let action_types = ActionTypesBuilder::new()
.with_mandate_flow(is_mandate_flow, connector_mandate_details)
.with_network_tokenization(is_network_token_with_ntid_flow, is_nt_with_ntid_supported_connector)
.build();
```
ShankarSinghC
left a comment
There was a problem hiding this comment.
Clippy is failing on this pr, please check
crates/router/src/core/payments.rs
Outdated
| NetworkTokenWithNetworkTransactionId(NTWithNTIRef), | ||
| CardWithNetworkTransactionId(String), // Network Transaction Id | ||
| #[cfg(feature = "v1")] | ||
| ConnectorMandate(Option<diesel_models::PaymentsMandateReference>), |
There was a problem hiding this comment.
Why is PaymentsMandateReference optional in ConnectorMandate ?
| pub struct ConnectorRoutingData { | ||
| pub connector_data: ConnectorData, | ||
| pub network: Option<common_enums::CardNetwork>, | ||
| pub action_type: Option<ActionType>, // action_type is used for mandates currently |
There was a problem hiding this comment.
If required add the comment above the filed
| pub action_type: Option<ActionType>, // action_type is used for mandates currently | |
| // action_type is used for mandates currently | |
| pub action_type: Option<ActionType>, |
| let current_connector_routing_data = | ||
| super::get_connector_data(&mut connector_routing_data)?; |
There was a problem hiding this comment.
The connector data is already fetched in the above code right ?
It is stored in the connector variable.
There was a problem hiding this comment.
connector_data is strored in the connector variable not connector_routing _data
|
|
||
| let is_mandate_flow = connector_mandate_details | ||
| .as_ref() | ||
| .ok() |
There was a problem hiding this comment.
If there is a parsing error should we log it here ?
crates/router/src/core/payments.rs
Outdated
| .as_ref() | ||
| .ok() | ||
| .zip(merchant_connector_id) | ||
| .and_then(|(details, merchant_id)| details.clone().map(|d| d.contains_key(merchant_id))) |
There was a problem hiding this comment.
The naming here is misleading, is it merchant id or merchant connector id ?
crates/router/src/core/payments.rs
Outdated
| let connector_mandate_details = &payment_method_info | ||
| .connector_mandate_details | ||
| .clone() | ||
| .map(|details| { | ||
| details | ||
| .parse_value::<diesel_models::PaymentsMandateReference>("connector_mandate_details") | ||
| }) | ||
| .transpose(); |
There was a problem hiding this comment.
I think we can use get_common_mandate_reference function here.
|
|
||
| let is_mandate_flow = connector_mandate_details | ||
| .as_ref() | ||
| .ok() |
There was a problem hiding this comment.
You can do flatten here to avoid details.clone().map(|d| d.contains_key(merchant_id))
| (connector_routing_data.connector_data, routing_decision) | ||
| }; | ||
|
|
||
| if payment_data |
There was a problem hiding this comment.
Correct me if I am wrong
If the code is reaching this place that means the payment has failed and it can be retried, which our auto retry. In such cases aren't we suppose to chose a different connector each time ?
For example
Retryable Connectors = [Adyen(Connector Mandate), Adyen(CardWithNetworkTransactionId), Cybersource(NetworkTokenWithNetworkTransactionId), Cybersource(CardWithNetworkTransactionId), Stripe(CardWithNetworkTransactionId)]
If the first payment happened -> Adyen(Connector Mandate) and failed
And the GSM record says retry then it would because of connector error, right ?
If yes, in such cases we can not use get_connector_data as it just gives the next element from the list. In this case it is Adyen(CardWithNetworkTransactionId). But as it is connector error we should retry with Cybersource(NetworkTokenWithNetworkTransactionId).
There was a problem hiding this comment.
If the connector mandate id is wrong for that connector, we should proceed with the payment with the other two action types right? (network token + nti or card+nti)
cc: @prasunna09
There was a problem hiding this comment.
For 5xx type of errors we can have do_default mapping in gsm to stop retrying. Other than that we are just retrying with the next item in the retryable connector list in this PR. The logic to change the current connector and proceed with the next connector, and other incremental changes if any will be done in the next following PR.
crates/router/src/core/payments.rs
Outdated
| #[cfg(feature = "v2")] | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn decide_connector_for_normal_or_recurring_payment<F: Clone, D>( | ||
| state: &SessionState, | ||
| payment_data: &mut D, | ||
| routing_data: &mut storage::RoutingData, | ||
| connectors: Vec<api::ConnectorData>, | ||
| is_connector_agnostic_mit_enabled: Option<bool>, | ||
| payment_method_info: &domain::PaymentMethod, | ||
| ) -> RouterResult<ConnectorCallType> | ||
| where | ||
| D: OperationSessionGetters<F> + OperationSessionSetters<F> + Send + Sync + Clone, | ||
| { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Why is this required ?
Are we calling decide_connector_for_normal_or_recurring_payment some where other than v2 ?
crates/router/src/core/payments.rs
Outdated
| let connector_details = connector_mandate_details | ||
| .clone() | ||
| .get_required_value("connector_mandate_details") |
There was a problem hiding this comment.
As mentioned in the enum definition, the connector_mandate_details inside ActionType::ConnectorMandate is currently an Option, but its usage here assumes it is always present. To avoid runtime errors and improve clarity, can we make PaymentsMandateReference mandatory in the enum variant ?
f183fdb to
34ec09c
Compare
crates/router/src/core/payments.rs
Outdated
| { | ||
| let mandate_reference_id = match action_type { | ||
| Some(ActionType::NetworkTokenWithNetworkTransactionId(nt_data)) => { | ||
| logger::info!("using network_tokenization with network_transaction_id for MIT flow"); |
There was a problem hiding this comment.
Nit
| logger::info!("using network_tokenization with network_transaction_id for MIT flow"); | |
| logger::info!("using network token with network_transaction_id for MIT flow"); |
crates/router/src/core/payments.rs
Outdated
| D: OperationSessionGetters<F> + OperationSessionSetters<F> + Send + Sync + Clone, | ||
| { | ||
| let mandate_reference_id = match action_type { | ||
| Some(ActionType::NetworkTokenWithNetworkTransactionId(nt_data)) => { |
There was a problem hiding this comment.
Nit
| Some(ActionType::NetworkTokenWithNetworkTransactionId(nt_data)) => { | |
| Some(ActionType::NetworkTokenWithNetworkTransactionId(network_token_data)) => { |
| Some(payments_api::MandateReferenceId::NetworkTokenWithNTI( | ||
| payments_api::NetworkTokenWithNTIRef { | ||
| network_transaction_id: nt_data.network_transaction_id.to_string(), | ||
| token_exp_month: nt_data.token_exp_month, | ||
| token_exp_year: nt_data.token_exp_year, | ||
| }, | ||
| )) |
There was a problem hiding this comment.
This can be an from conversation on network_token_data
crates/router/src/core/payments.rs
Outdated
| hyperswitch_domain_models::router_data::RecurringMandatePaymentData { | ||
| payment_method_type: mandate_reference_record.payment_method_type, | ||
| original_payment_authorized_amount: mandate_reference_record | ||
| .original_payment_authorized_amount, | ||
| original_payment_authorized_currency: mandate_reference_record | ||
| .original_payment_authorized_currency, | ||
| mandate_metadata: mandate_reference_record.mandate_metadata.clone(), | ||
| }, |
There was a problem hiding this comment.
This can be from conversation
| is_nt_with_ntid_supported_connector: bool, | ||
| payment_method_info: &domain::PaymentMethod, | ||
| ) -> Self { | ||
| match is_network_token_with_ntid_flow { |
There was a problem hiding this comment.
As we are taking some action only in case of NtWithNtiSupported and not returning anything from the match block, this can be made as if let
Ex:
if let IsNtWithNtiFlow::NtWithNtiSupported(network_transaction_id) = is_network_token_with_ntid_flow
There was a problem hiding this comment.
I have used match cases with guards here, if I do this with if let case, then there will be nesting of ifs, I guess that is less readable.
| payment_method_info, | ||
| ) | ||
| .await | ||
| .ok() |
There was a problem hiding this comment.
@prasunna09 should we log the error that occurred during do_status_check_for_network_token ?
There was a problem hiding this comment.
we can, but we have done attach_printable for every response/error returns.
ig its better to log before we do .ok()
| pub connector_mandate_request_reference_id: Option<String>, | ||
| } | ||
|
|
||
| impl From<&PaymentsMandateReferenceRecord> for crate::router_data::RecurringMandatePaymentData { |
There was a problem hiding this comment.
| impl From<&PaymentsMandateReferenceRecord> for crate::router_data::RecurringMandatePaymentData { | |
| impl From<&PaymentsMandateReferenceRecord> for router_data::RecurringMandatePaymentData { |
crates/router/src/core/payments.rs
Outdated
| #[cfg(feature = "v1")] | ||
| impl Default for ActionTypesBuilder { | ||
| fn default() -> Self { | ||
| Self::new() |
a06125e to
995f968
Compare
2553552
2553552 to
15431c0
Compare
…ee-ds * 'main' of github.com:juspay/hyperswitch: feat(webhooks): Provide outgoing webhook support for revenue recovery (#9294) feat(connector): Add Peachpayments Template Code (#9363) feat(connector): [Paysafe] Implement card 3ds flow (#9305) feat(router): Add Connector changes for 3ds (v2) (#9117) feat(connector): [ADYEN] Add support to ideal Mandate Webhook (#9347) refactor(core): accept manual retry from profile (#9302) fix(nuvei): nuvei 3ds fix + psync fix (#9279) fix(connector): [checkout] Add US Support for Apple Pay and Google Pay + Enhanced Checkout Response Data (#9356) fix(router): adding connector_customer_id for external vault proxy (#9263) feat(core): Add first_name and last_name as Secret<String> Types. (#9326) feat(injector): injector request formation changes (#9306) fix(revenue-recovery): Update Redis TTL for customer locks after token selection (#9282) chore(version): 2025.09.11.0 refactor(connector): [Paysafe] fix wasm (#9349) refactor(connector): rename RevenueRecoveryRecordBack as InvoiceRecordBack (#9321) feat(connector): [checkout] add support for MOTO payments (#9327) feat(connector): enhance ACI connector with comprehensive 3DS support - DRAFT (#8986) feat(core): [Retry] MIT Retries (#8628)
Type of Change
Description
Added action type in ConnectorRoutingData (NT with ntid, Card with ntid, Connector mandate).
We are storing all the instances of connector routing data with different action types.
During retry it will pick the top element in the list and will try to do payment with the mandate_reference_id that will be fetched from action type.
Solution doc: link
Additional Changes
Motivation and Context
How did you test it?
Change the value of
connector_mandate_idto some wrong value in db.GSM error:
Response (attempt_count = 2):
Payments Retrieve Response:
Checklist
cargo +nightly fmt --allcargo clippy