-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat(gRPC): build gRPC client interface to initiate communication with recovery-decider service #8178
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
Conversation
…tion_monitoring_feilds
logger::debug!( | ||
payment_intent_id = ?payment_intent.get_id(), | ||
attempt_id = ?payment_attempt.get_id(), | ||
message = "payment_attempt.payment_method_data is 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.
Nit: You don't have to explicitly pass the message
field.
logger::debug!( | |
payment_intent_id = ?payment_intent.get_id(), | |
attempt_id = ?payment_attempt.get_id(), | |
message = "payment_attempt.payment_method_data is None" | |
); | |
logger::debug!( | |
payment_intent_id = ?payment_intent.get_id(), | |
attempt_id = ?payment_attempt.get_id(), | |
"payment_attempt.payment_method_data is None" | |
); |
|
||
let card_network_str = billing_connector_payment_method_details | ||
.and_then(|details| match details { | ||
BillingConnectorPaymentMethodDetails::Card(card_info) => card_info.card_network.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.
Nit: Can't we avoid the clone here? I think you should be able to, by deriving Copy
on the common_enums::enums::CardNetwork
enum.
BillingConnectorPaymentMethodDetails::Card(card_info) => card_info.card_issuer.clone(), | ||
})?; | ||
|
||
let card_funding_str = payment_intent |
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.
Card funding? Where are we obtaining this information from?
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 basically card type that we are fetching from payment intent's PaymentRevenueRecoveryMetadata
-> payment_method_subtype
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 the "funding" in the variable name?
fn from(internal_request: InternalDeciderRequest) -> Self { | ||
Self { | ||
first_error_message: internal_request.first_error_message, | ||
billing_state: internal_request.billing_state.peek().to_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.
@Aprabhat19 @srujanchikke Ideally you folks should consider using an equivalent of Secret
on the gRPC server side as well, similar to how the connector-service folks are doing...
config/config.example.toml
Outdated
[billing_connectors_invoice_sync] | ||
billing_connectors_which_requires_invoice_sync_call = "recurly" # List of billing connectors which has invoice sync api call | ||
|
||
[revenue_recovery] | ||
monitoring_threshold_in_seconds = 2592000 # 30*24*60*60 secs , threshold for monitoring the retry system | ||
retry_algorithm_type = "cascading" # type of retry algorithm | ||
monitoring_threshold_in_seconds = 60 # 30*24*60*60 secs , threshold for monitoring the retry system |
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.
The number and the comment don't match?
config/config.example.toml
Outdated
[billing_connectors_invoice_sync] | ||
billing_connectors_which_requires_invoice_sync_call = "recurly" # List of billing connectors which has invoice sync api call | ||
|
||
[revenue_recovery] | ||
monitoring_threshold_in_seconds = 2592000 # 30*24*60*60 secs , threshold for monitoring the retry system | ||
retry_algorithm_type = "cascading" # type of retry algorithm | ||
monitoring_threshold_in_seconds = 60 # 30*24*60*60 secs , threshold for monitoring the retry system |
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.
The number (60) and the comment don't match?
); | ||
|
||
logger::info!("Recovery Decider gRPC client successfully initialized"); | ||
// Some(Box::new(client) as Box<dyn RecoveryDeciderClientInterface>) |
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 commented code.
@@ -97,7 +99,7 @@ impl RevenueRecoveryPaymentsAttemptStatus { | |||
payment_intent: &PaymentIntent, | |||
process_tracker: storage::ProcessTracker, | |||
revenue_recovery_payment_data: &storage::revenue_recovery::RevenueRecoveryPaymentData, | |||
payment_attempt: payment_attempt::PaymentAttempt, | |||
payment_attempt: PaymentAttempt, |
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.
Nit: Can't we accept a reference here? Do you necessarily need an owned value?
process: &storage::ProcessTracker, | ||
payment_attempt: payment_attempt::PaymentAttempt, | ||
payment_attempt: PaymentAttempt, |
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.
Same here, can't we accept a reference?
|
||
#[derive(Debug, serde::Deserialize, Clone)] | ||
pub struct RecoveryTimestamp { | ||
pub initial_timestamp_in_hours: i64, |
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 do we need an i64
, can't we use something like u16
or u32
? This is anyway a duration, we don't need these to be negative.
} | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug, serde::Deserialize, Clone, Default)] | ||
pub struct RevenueRecoverySettings { | ||
pub monitoring_threshold_in_seconds: i64, |
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.
Same here, we don't need an i64
?
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.
Core changes LGTM
…ordea-sepa * 'main' of github.com:juspay/hyperswitch: fix(router): [worldpayvantiv] dispute validations and statuses (#8862) chore(version): 2025.08.07.0 feat(connector): [WORLDPAYVANTIV] Populate Network Decline Error Code & Message (#8856) feat(router): add support for partial authorization (#8833) feat(gRPC): build gRPC client interface to initiate communication with recovery-decider service (#8178) fix(connector): [CYBERSOURCE] fix response field for netcetera authentication response (#8850) chore(events): making events nanosecond level precision (#8759)
Type of Change
Description
I have implemented a gRPC client that facilitates communication with the recovery-decider gRPC service. The client should be capable of initializing and managing the connection to the service, sending requests, and handling responses in accordance with the defined service contract (protobuf definitions).
Additional Changes
Motivation and Context
How did you test it?
Decider gRPC server:-

Decider gRPC client(HS):-

Predictor gRPC server:-

Checklist
cargo +nightly fmt --all
cargo clippy
Summary by CodeRabbit
New Features
Improvements
Configuration
Bug Fixes