Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds currency validation for UPI transactions to reject gateways that don't support non-INR currencies. The implementation checks the supportedCurrencies field from Gateway Payment Method (GPM) configurations to filter out incompatible gateways.
Key Changes
- Added currency check that filters UPI gateways based on supported currencies for non-INR transactions
- Introduced
filter_upi_gateways_for_currencyhelper function to validate gateway currency support against MGAs - Modified
filterGatewaysForUpito apply currency filtering before setting final gateway list
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Convert currency strings to Currency enum and check if txn_currency is supported | ||
| currency_list.iter().any(|curr_str| { | ||
| Currency::text_to_curr(curr_str) | ||
| .map(|curr| curr == txn_currency) | ||
| .unwrap_or(false) | ||
| }) |
There was a problem hiding this comment.
The currency parsing logic here duplicates the existing canAcceptCurrency function (lines 556-568), but uses text_to_curr differently. The existing canAcceptCurrency uses .ok() to handle the Result, while this code uses .map() and .unwrap_or(false). This inconsistency could lead to different error handling behavior. Consider reusing the existing canAcceptCurrency function to maintain consistency and reduce code duplication.
| // Convert currency strings to Currency enum and check if txn_currency is supported | |
| currency_list.iter().any(|curr_str| { | |
| Currency::text_to_curr(curr_str) | |
| .map(|curr| curr == txn_currency) | |
| .unwrap_or(false) | |
| }) | |
| // Use the existing canAcceptCurrency function for consistency | |
| canAcceptCurrency(¤cy_list, txn_currency) |
|
|
||
| //Convert upi_only_gateways to <HashSet<_>> | ||
| let upi_only_gateways_hashset = upi_only_gateways.into_iter().collect::<HashSet<_>>(); | ||
| let upi_only_gateways_hashset = upi_only_gateways.clone().into_iter().collect::<HashSet<_>>(); |
There was a problem hiding this comment.
The .clone() added here is unnecessary. The original code consumed upi_only_gateways by converting it into an iterator and then collecting into a HashSet. However, upi_only_gateways is now used again at line 3121. Instead of cloning the entire vector here, consider collecting into the HashSet first, then cloning from the HashSet when needed, or restructure to avoid the need for cloning altogether by reordering operations.
| async fn filter_upi_gateways_for_currency( | ||
| this: &mut DeciderFlow<'_>, | ||
| upi_gateways: Vec<String>, | ||
| all_upi_gateways: Vec<String>, | ||
| txn_currency: Currency, | ||
| txn_card_info: &TxnCardInfo, | ||
| ) -> Vec<String> { | ||
| // Get payment method entry from database | ||
| let pm_entry = ETP::get_by_name(txn_card_info.paymentMethod.clone()).await; | ||
|
|
||
| match pm_entry { | ||
| Some(pm) => { | ||
| // Get all MGAs for the merchant | ||
| let mgas = Utils::get_mgas(this).unwrap_or_default(); | ||
|
|
||
| // Filter MGAs to only those supporting UPI gateways | ||
| let upi_gateway_set: HashSet<String> = all_upi_gateways.iter().cloned().collect(); | ||
| let upi_mgas: Vec<_> = mgas | ||
| .into_iter() | ||
| .filter(|mga| upi_gateway_set.contains(&mga.gateway)) | ||
| .collect(); | ||
|
|
||
| // Filter gateways based on supported currencies | ||
| let currency_supported_gws: Vec<String> = upi_gateways | ||
| .iter() | ||
| .filter(|gw| { | ||
| // Find the MGA for this gateway | ||
| if let Some(mga) = upi_mgas.iter().find(|mga| &mga.gateway == *gw) { | ||
| // Check if the MGA has supported currencies configured | ||
| if let Some(ref supported_currencies_str) = mga.supportedCurrencies { | ||
| // Parse the supported currencies JSON array | ||
| match serde_json::from_str::<Vec<String>>(supported_currencies_str) { | ||
| Ok(currency_list) => { | ||
| // Convert currency strings to Currency enum and check if txn_currency is supported | ||
| currency_list.iter().any(|curr_str| { | ||
| Currency::text_to_curr(curr_str) | ||
| .map(|curr| curr == txn_currency) | ||
| .unwrap_or(false) | ||
| }) | ||
| } | ||
| Err(_) => false, | ||
| } | ||
| } else { | ||
| false | ||
| } | ||
| } else { | ||
| false | ||
| } | ||
| }) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| // Log if gateways were filtered out | ||
| if currency_supported_gws.len() < upi_gateways.len() { | ||
| let rejected_gateways: Vec<String> = upi_gateways | ||
| .iter() | ||
| .filter(|gw| !currency_supported_gws.contains(gw)) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| logger::info!( | ||
| tag = "filterGatewaysForUpi", | ||
| action = "filterGatewaysForUpi", | ||
| "Filtered out gateways for non-INR UPI transaction. Currency: {:?}, Remaining gateways: {:?}, Rejected gateways: {:?}", | ||
| txn_currency, | ||
| currency_supported_gws, | ||
| rejected_gateways | ||
| ); | ||
| } | ||
|
|
||
| currency_supported_gws | ||
| } | ||
| None => { | ||
| logger::info!( | ||
| tag = "filterGatewaysForUpi", | ||
| action = "filterGatewaysForUpi", | ||
| "Payment method not found for {}, rejecting all gateways for non-INR UPI transaction", | ||
| txn_card_info.paymentMethod | ||
| ); | ||
| Vec::new() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function lacks documentation. Consider adding a docstring that explains the purpose, parameters, return value, and the currency filtering logic, especially noting that it returns an empty vector when payment method is not found or when gateways don't support the transaction currency.
| Err(_) => false, | ||
| } | ||
| } else { | ||
| false |
There was a problem hiding this comment.
When supportedCurrencies is None for a gateway, the code returns false and filters out that gateway. This differs from the behavior in the existing currencyFilter function (line 542-548) which defaults to allowing INR when supportedCurrencies is not set. This inconsistency means gateways without configured currencies will be rejected for non-INR transactions even if they might support INR by default. Consider aligning this behavior with the existing currency filtering logic.
| false | |
| // If supportedCurrencies is not set, allow INR by default (for consistency with currencyFilter) | |
| txn_currency == Currency::INR |
To add a check for supportedCurrencies from GPM so that txns with non supported currencies gets rejected