-
Notifications
You must be signed in to change notification settings - Fork 96
feat(backend): additional webhook events for outgoing payments #3651
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
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs
|
await knex.schema.alterTable( | ||
'outgoingPaymentCardDetails', | ||
function (table) { | ||
table.string('requestId').nullable() |
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.
Let's make this non-nullable
import { LiquidityAccount } from '../../../accounting/service' | ||
import { Asset } from '../../../asset/model' | ||
import { ConnectorAccount } from '../../../payment-method/ilp/connector/core/rafiki' | ||
import { OutgoingPaymentInitiationReason } from './types' |
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, but we can keep OutgoingPaymentInitiationReason
here
if (initiationReason === IncomingPaymentInitiationReason.Card) { | ||
recipients = recipients.concat(buildPosRecipient()) | ||
} |
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.
These new outgoing payment events should be sent only to card service, and not POS service (outgoing payments -> card service, incoming payments -> POS Service).
I think we should also make this function take in an object instead of a list of params for readability
// So default the amountSent and balance to 0 for outgoing payments still in the funding or cancelled states | ||
const isZeroAmountSent = | ||
payment.state === OutgoingPaymentState.Funding || | ||
type === OutgoingPaymentEventType.PaymentCancelled |
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.
type === OutgoingPaymentEventType.PaymentCancelled | |
payment.state === OutgoingPaymentState.Cancelled |
packages/backend/src/asset/model.ts
Outdated
): Promise<Asset> { | ||
if (this.liquidityThreshold !== null) { | ||
if (balance <= this.liquidityThreshold) { | ||
const { finalizeWebhookRecipients } = await import('../webhook/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.
Why do we need the dynamic import here?
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 it because IncomingPaymentInitiationReason
was causing a cycle in the static imports after I moved it to model.ts
. (I guess this is why it was in a different file before). So in order to be able to properly use this enum inside webhook service and still have the type in model.ts I had to break the cycle by adding this dynamic import. We can of course keep it in a separate file, but as you suggested here for the outgoing payment equivalent enum, it is cleaner this way. Also, we can hardcode the enum variant (e.g. "CARD") here instead of using the type, but I think that might create confusion on the long run.
payment.walletAddressId | ||
) | ||
|
||
if (payment.initiatedBy === OutgoingPaymentInitiationReason.Card) { |
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.
I'm debating whether we keep this conditional, or still store the webhookEvent
for cancelled & funded, but just don't send it to anyone if it was done for a non-card payment. Thoughts?
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.
My first thought is that it is confusing to store webhook events that will not be sent, especially if doing so by calling sendWebhookEvent
here. Is there any reason we would need to store these events (e.g. audit, proofs)? If yes, then I think we should find a way to store this information separately, without touching webhooks logic.
Cancelled = 'CANCELLED' | ||
} | ||
|
||
export enum OutgoingPaymentDepositType { |
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.
Technically, neither the cancelled nor the funded events are "deposit" or "funded" type. We can either check whether the OutgoingPaymentWithdrawType
or OutgoingPaymentDepositType
are used anywhere and if they are, just make a third category for the cancelled or funded.
return await query.getPage(pagination, sortOrder) | ||
} | ||
|
||
type FinalizeRecipientsOptions = { |
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 do you think about having sendToPosService
and sendToCardService
parameters, and just let the caller decide when those are true? Then this function doesn't need to know about anything payments related
Changes proposed in this pull request
This PR adds new webhook events for outgoing payments.
In order not to have the POS service waiting until the incoming payment expires, we have to reply to the POS service request with the ASEs decision on the outgoing payment. This is done by having backend send new webhooks to the card service:
outgoing_payment.funded
andoutgoing_payment.cancelled
.Context
Closes RAF-1156
Checklist
fixes #number
user-docs
label (if necessary)