-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(core): [Network Tokenization] pre network tokenization #6873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
276d588
to
5c13cc8
Compare
a5845b1
to
3f9c0ba
Compare
16e0f52
to
dcc8625
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also when constructing router data, if pre-tokenization enabled, we need to pass network token data as payment method data. Could you point me to that change?
crates/router/src/core/payments.rs
Outdated
if is_pre_tokenization_enabled && is_nt_supported_connector_available { | ||
let pre_tokenization_response = tokenization::pre_payment_tokenization( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this validation for pre-network tokenization move to a a function?
crates/router/src/core/payments.rs
Outdated
.await?; | ||
let pm_data = payment_data.get_payment_method_data(); | ||
match pre_tokenization_response { | ||
(Some(token_response), Some(_token_ref)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we ignoring this token_ref
? this should be set in payment method info right?
crates/router/src/core/payments.rs
Outdated
let filtered_nt_supported_connectors = | ||
get_filtered_nt_supported_connectors(&state, [connector.clone()].to_vec()); | ||
|
||
let is_nt_supported_connector_available = | ||
filtered_nt_supported_connectors.first().is_some(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do no filter out. check only if the connector decided by routing is in the network tokenization supported connectors. this is because routing should always be prioritized over any feature. please remove this.
crates/router/src/core/payments.rs
Outdated
match pre_tokenization_response { | ||
(Some(token_response), Some(_token_ref)) => { | ||
let token_data = domain::NetworkTokenData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is same logic, can you write this to a common function?
4c8d6a4
to
7563104
Compare
crates/api_models/src/admin.rs
Outdated
|
||
/// Indicates if network tokenization before first payment is enabled or not | ||
#[serde(default)] | ||
pub is_tokenize_before_payment_enabled: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_tokenize_before_payment_enabled
is bit ambiguous when 'before payment' is mentioned in variable name.
is_pre_network_tokenization_enabled
can be used ig. We can add clear doc comment
crates/router/src/core/payments.rs
Outdated
|
||
let is_nt_supported_connector = ntid_supported_connectors.contains(&connector_data.connector_name); | ||
|
||
println!("{:?}<<>>10",is_nt_supported_connector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the loggers.
crates/router/src/core/payments.rs
Outdated
println!("{:?}<<>>10",is_nt_supported_connector); | ||
|
||
if is_pre_tokenization_enabled && is_nt_supported_connector { | ||
println!("{:?}<<>>14",is_nt_supported_connector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here ^
crates/router/src/core/payments.rs
Outdated
if is_pre_tokenization_enabled && is_nt_supported_connector { | ||
println!("{:?}<<>>14",is_nt_supported_connector); | ||
save_card_and_network_token_data(state, &mut payment_data).await; | ||
println!("{:?}<<>>15",payment_data.get_payment_method_data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this too^
crates/router/src/core/payments.rs
Outdated
network_token_data: token_data.clone(), | ||
ref_id: token_ref | ||
}; | ||
println!("{:?}<<>>17", network_token_data_for_vault); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove logger here^
crates/router/src/core/payments.rs
Outdated
#[derive(Default, Clone, serde::Serialize, Debug)] | ||
pub struct NetworkTokenDataForVault { | ||
pub network_token_data: hyperswitch_domain_models::payment_method_data::NetworkTokenData, | ||
pub ref_id: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is network_token_req_ref id right?
Can you rename it?
crates/router/src/core/payments.rs
Outdated
|
||
if is_pre_tokenization_enabled && is_nt_supported_connector { | ||
println!("{:?}<<>>14",is_nt_supported_connector); | ||
save_card_and_network_token_data(state, &mut payment_data).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you modify save_card_and_network_token_data
fn name? because, the fn is trying to pre tokenize and update payment data.payment method data. but not saving card flow.
crates/router/src/core/payments.rs
Outdated
} | ||
|
||
#[derive(Default, Clone, serde::Serialize, Debug)] | ||
pub struct CardDataForVault { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this struct has never been used. if this is not needed, can you remove this?
crates/router/src/core/payments.rs
Outdated
.await.ok(); | ||
match pre_tokenization_response { | ||
Some((Some(token_response), Some(token_ref))) => { | ||
let token_data = domain::NetworkTokenData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be moved to a From implementation?
Some(token_response), | ||
network_token_requestor_ref_id.clone(), | ||
)), | ||
_ => Ok((None, None)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incase fetch cryptogram fails we need to delete the network token generated right?
you return Ok((None, None)) in this failure case.
in save_card_and_network_token_data
fn, it is written that if pre_payment_tokenization
is None, then vault operation is set to SaveCardData(api::Card). but we need to return network token requestor ref id right? so that we can use this id to delete network token at token requestor in post update tracker
d3d4ecf
to
90da476
Compare
4b62178
to
6bb59b2
Compare
a699724
to
9311c2c
Compare
e5f5590
to
17f5577
Compare
@@ -106,6 +107,7 @@ pub struct ProfileNew { | |||
pub is_click_to_pay_enabled: bool, | |||
pub authentication_product_ids: | |||
Option<common_types::payments::AuthenticationConnectorAccountMap>, | |||
pub is_pre_network_tokenization_enabled: Option<bool>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub is_pre_network_tokenization_enabled: Option<bool>, | |
pub is_pre_network_tokenization_enabled: bool, |
crates/router/src/core/payments.rs
Outdated
} | ||
|
||
#[cfg(feature = "v1")] | ||
impl From<network_tokenization::TokenResponse> for domain::NetworkTokenData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to payments/transformers
crates/router/src/core/payments.rs
Outdated
} | ||
|
||
#[derive(Clone, serde::Serialize, Debug)] | ||
pub enum PaymentMethodDataAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move these to domain models?
#[cfg(feature = "v1")] | ||
pub async fn pre_payment_tokenization( | ||
state: &SessionState, | ||
customer_id: Option<id_type::CustomerId>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
customer_id: Option<id_type::CustomerId>, | |
customer_id: id_type::CustomerId, |
crates/router/src/core/payments.rs
Outdated
{ | ||
let customer_id = payment_data.get_payment_intent().customer_id.clone(); | ||
let payment_method_data = payment_data.get_payment_method_data(); | ||
let pre_tokenization_response = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can call pre_payment_tokenization after checking for PaymentMethodData::Card
Some(token_response), | ||
network_token_requestor_ref_id.clone(), | ||
)), | ||
_ => Ok((None, network_token_requestor_ref_id.clone())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid default cases, and you can log the errors here
) { | ||
let _ = record_operation_time( | ||
async { | ||
network_tokenization::delete_network_token_from_tokenization_service( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we tokio::spawn these deletions?
payments::PaymentMethodDataAction::SaveCardAndNetworkTokenData( | ||
save_card_and_network_token_data, | ||
) => { | ||
let card_data = api::CardDetail { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have from impls for these
@@ -559,6 +576,19 @@ where | |||
|
|||
let connector_data = get_connector_data(&mut connectors)?; | |||
|
|||
if is_pre_network_tokenization_enabled( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoudn't we move this to call_connector_service? What if the next connector is eligible for network tokenization and not the first one?
a56539e
to
f9bfc8a
Compare
1964bb5
to
ba62a4d
Compare
3eebfe7
to
fb31fea
Compare
ca1d682
to
fb31fea
Compare
fb31fea
to
b10f6c1
Compare
dda6dac
to
a5035b5
Compare
5a6ebb8
to
a5035b5
Compare
@@ -2423,6 +2434,9 @@ pub struct ProfileResponse { | |||
//Merchant country for the profile | |||
#[schema(value_type = Option<CountryAlpha2>, example = "US")] | |||
pub merchant_business_country: Option<api_enums::CountryAlpha2>, | |||
|
|||
/// Indicates if pre network tokenization is enabled or not | |||
pub is_pre_network_tokenization_enabled: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this feature available in v2 as well?
If not, this can be removed.
@@ -541,6 +550,7 @@ impl ProfileUpdateInternal { | |||
merchant_business_country, | |||
revenue_recovery_retry_algorithm_type, | |||
revenue_recovery_retry_algorithm_data, | |||
is_pre_network_tokenization_enabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not used in v2, it can be removed.
@@ -221,6 +221,7 @@ diesel::table! { | |||
merchant_business_country -> Nullable<CountryAlpha2>, | |||
#[max_length = 64] | |||
id -> Varchar, | |||
is_pre_network_tokenization_enabled -> Bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed if not used
@@ -210,41 +215,163 @@ where | |||
.await?; | |||
((res, dc, None), None) | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move the logic in else part to a different function.
Type of Change
Description
Introduced a new field in business_profile (is_pre_network_tokenization_enabled).
Based on that field the card will be tokenized before first payment.
If the payment get succeeded the card and network token data will be stored in the locker afterwards.
Additional Changes
Motivation and Context
How did you test it?
Checklist
cargo +nightly fmt --all
cargo clippy