-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(core): Create Payout Webhook Flow #4696
Conversation
pub fn is_payout_event(event_code: &WebhookEventCode) -> bool { | ||
matches!( | ||
event_code, | ||
WebhookEventCode::PayoutThirdparty | WebhookEventCode::PayoutDecline |
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.
Are we not handling PAYOUT_REVERSED and PAYOUT_EXPIRE?
ref - https://docs.adyen.com/online-payments/online-payouts/payout-webhook/
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.
Added support for them
Self::PayoutDetails(payout_payload) => Some(OutgoingWebhookEventContent::Payout { | ||
payout_id: payout_payload.payout_id.clone(), | ||
content: masking::masked_serialize(&payout_payload) | ||
.unwrap_or(serde_json::json!({"error":"failed to serialize"})), |
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 what we send to merchants if deserialization fails?
crates/router/src/types.rs
Outdated
@@ -420,7 +420,7 @@ impl PayoutIndividualDetailsExt for api_models::payouts::PayoutIndividualDetails | |||
#[derive(Clone, Debug, Default)] | |||
pub struct PayoutsResponseData { | |||
pub status: Option<storage_enums::PayoutStatus>, | |||
pub connector_payout_id: String, | |||
pub connector_payout_id: Option<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.
TODO for later: make it non optional, and add new struct for connector integration if this field is not consumed in other flows
@@ -1107,6 +1108,10 @@ pub enum EventType { | |||
DisputeLost, | |||
MandateActive, | |||
MandateRevoked, | |||
PayoutSuccess, |
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.
Possible to add PayoutCreated
? Trigger - when payout is created at connector's end.
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.
shouldn't we add feature flags here as well?
) | ||
.await | ||
} | ||
MerchantStorageScheme::RedisKv => { |
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.
@dracarys18 can you review the reverse lookup bit?
@@ -68,6 +68,7 @@ pub enum EventObjectType { | |||
RefundDetails, | |||
DisputeDetails, | |||
MandateDetails, | |||
PayoutDetails, |
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.
feature flag?
@@ -301,7 +301,7 @@ impl<F> TryFrom<types::PayoutsResponseRouterData<F, EbanxFulfillResponse>> | |||
Ok(Self { | |||
response: Ok(types::PayoutsResponseData { | |||
status: Some(storage_enums::PayoutStatus::from(item.response.status)), | |||
connector_payout_id: item.data.request.get_transfer_id()?, | |||
connector_payout_id: item.data.request.get_transfer_id().ok(), |
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 the error in this case?
crates/router/src/core/payouts.rs
Outdated
@@ -1298,7 +1298,7 @@ pub async fn complete_create_payout( | |||
let db = &*state.store; | |||
let payout_attempt = &payout_data.payout_attempt; | |||
let updated_payout_attempt = storage::PayoutAttemptUpdate::StatusUpdate { | |||
connector_payout_id: "".to_string(), | |||
connector_payout_id: 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.
can we map it to payout_data.payout_attempt.connector_payout_id
instead to hardcoding to None
crates/router/src/core/payouts.rs
Outdated
@@ -1248,7 +1248,7 @@ pub async fn check_payout_eligibility( | |||
Err(err) => { | |||
let status = storage_enums::PayoutStatus::Failed; | |||
let updated_payout_attempt = storage::PayoutAttemptUpdate::StatusUpdate { | |||
connector_payout_id: String::default(), | |||
connector_payout_id: 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.
+1
crates/router/src/core/payouts.rs
Outdated
@@ -1440,7 +1440,7 @@ pub async fn create_payout( | |||
Err(err) => { | |||
let status = storage_enums::PayoutStatus::Failed; | |||
let updated_payout_attempt = storage::PayoutAttemptUpdate::StatusUpdate { | |||
connector_payout_id: String::default(), | |||
connector_payout_id: 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.
+1
crates/router/src/core/payouts.rs
Outdated
@@ -1563,7 +1563,7 @@ pub async fn create_recipient_disburse_account( | |||
} | |||
Err(err) => { | |||
let updated_payout_attempt = storage::PayoutAttemptUpdate::StatusUpdate { | |||
connector_payout_id: String::default(), | |||
connector_payout_id: 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.
+1
crates/router/src/core/payouts.rs
Outdated
@@ -1659,7 +1659,7 @@ pub async fn cancel_payout( | |||
Err(err) => { | |||
let status = storage_enums::PayoutStatus::Failed; | |||
let updated_payout_attempt = storage::PayoutAttemptUpdate::StatusUpdate { | |||
connector_payout_id: String::default(), | |||
connector_payout_id: 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.
+1
crates/router/src/core/payouts.rs
Outdated
@@ -1799,7 +1799,7 @@ pub async fn fulfill_payout( | |||
Err(err) => { | |||
let status = storage_enums::PayoutStatus::Failed; | |||
let updated_payout_attempt = storage::PayoutAttemptUpdate::StatusUpdate { | |||
connector_payout_id: String::default(), | |||
connector_payout_id: 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.
+1
crates/router/src/core/webhooks.rs
Outdated
business_profile, | ||
&key_store, | ||
outgoing_event_type, | ||
enums::EventClass::Refunds, |
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 should not be enums::EventClass::Refunds
but event_type
storage_enums::PayoutStatus::Failed => Some(storage_enums::EventType::PayoutFailed), | ||
storage_enums::PayoutStatus::Cancelled => { | ||
Some(storage_enums::EventType::PayoutCancelled) | ||
} |
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 remove curly braces!
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 done by formatter bro, everytime I remove it the formatter puts it back
} | ||
storage_enums::PayoutStatus::Initiated => { | ||
Some(storage_enums::EventType::PayoutInitiated) | ||
} |
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.
+1
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 with this
|
||
#[inline] | ||
#[instrument(skip_all)] | ||
async fn add_connector_payout_id_to_reverse_lookup<T: DatabaseStore>( |
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.
get these Kv changes reviewed by @dracarys18
@@ -292,6 +292,7 @@ impl ForeignTryFrom<(bool, AdyenWebhookStatus)> for storage_enums::AttemptStatus | |||
AdyenWebhookStatus::CancelFailed => Ok(Self::VoidFailed), | |||
AdyenWebhookStatus::Captured => Ok(Self::Charged), | |||
AdyenWebhookStatus::CaptureFailed => Ok(Self::CaptureFailed), | |||
AdyenWebhookStatus::Reversed => Ok(Self::AutoRefunded), |
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.
What does Reversed mean here, Is it a Voided payment? AutoRefunded occurs when we initiate a refund from Hyperswitch without merchant initiating it
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.
changed it, now it will through error for reversed in payment webhooks
crates/router/src/core/webhooks.rs
Outdated
metrics::INCOMING_PAYOUT_WEBHOOK_METRIC.add(&metrics::CONTEXT, 1, &[]); | ||
if source_verified { | ||
let db = &*state.store; | ||
//find refund by connector refund id |
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 add the relevant comment here
if source_verified { | ||
let db = &*state.store; | ||
//find refund by connector refund id | ||
let payout_attempt = match webhook_details.object_reference_id { |
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 please move this to a fn?
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.
will refactor this in next pr
@@ -0,0 +1,2 @@ | |||
-- Your SQL goes here | |||
ALTER TABLE payout_attempt ALTER COLUMN connector_payout_id DROP NOT NULL; |
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 add a query here to turn '' string to NULL
) -> StorageResult<Self> { | ||
generics::generic_find_one::<<Self as HasTable>::Table, _, _>( | ||
conn, | ||
dsl::merchant_id |
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 please create an index for merchant_id, connector_payout_id
.change_context(errors::ApiErrorResponse::WebhookProcessingFailure) | ||
.attach_printable("failed payout status mapping from event type")?, | ||
error_message: None, | ||
error_code: 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.
We need to capture error_code, error_message at connector level and update them here
status: updated_payout_attempt.status, | ||
}) | ||
} else { | ||
metrics::INCOMING_PAYOUT_WEBHOOK_SIGNATURE_FAILURE_METRIC.add(&metrics::CONTEXT, 1, &[]); |
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 capture merchant_id / connector here
Please handle the payout connectors in |
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 address the comments by @sai-harsha-vardhan , other than that LGTM!
Type of Change
Description
Create core flow for processing payout webhooks and send respective outgoing webhook
Additional Changes
Motivation and Context
How did you test it?
Tested through Postman:
Craete an MCA(Adyen Payout):
Update the MCA with merchant_secret:
Update the Merchant Account with the outgoing webhook url:
Set up the url at Connector's Dashboard:
Create a payout through Adyen:
The webhook should be received at your defined endpoint with event type as
payout_initiated
Checklist
cargo +nightly fmt --all
cargo clippy