-
Notifications
You must be signed in to change notification settings - Fork 90
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
docs(backend): add webhooks openapi spec #1210
Conversation
To review, go to https://editor-next.swagger.io/ and paste resolved schema (or resolve it yourself first if you don't trust me 😉)
|
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 planning on incorporating this in the code?
I'm thinking at least add .toSatisfyApiSpec
checks to webhook tests.
Maybe even get the MAP using our openapi
package to validate requests.
Which tests are you referring to exactly?
I can create an issue for that. |
Yeah, the webhook tests that I forgot we're skipping... rafiki/packages/backend/src/webhook/service.test.ts Lines 96 to 117 in 8955d7e
|
format: date-time | ||
statusCode: | ||
type: integer | ||
withdrawal: |
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 we necessarily need withdrawal
? it's not being used in the MAP webhooks handler, and looking at the webhook publishers, the only information that isn't already on the data
is the assetId
: something that we could just include in the data
object as well IMO
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 think it's only used by the outgoing payment event.
@wilsonianb can you explain its use?
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.
withdrawal
isn't included in the request, just id
, type
, and data
rafiki/packages/backend/src/webhook/service.ts
Lines 87 to 105 in 80071d3
const body = { | |
id: event.id, | |
type: event.type, | |
data: event.data | |
} | |
if (deps.config.signatureSecret) { | |
requestHeaders['Rafiki-Signature'] = generateWebhookSignature( | |
body, | |
deps.config.signatureSecret, | |
deps.config.signatureVersion | |
) | |
} | |
await axios.post(deps.config.webhookUrl, body, { | |
timeout: deps.config.webhookTimeout, | |
headers: requestHeaders, | |
validateStatus: (status) => status === 200 | |
}) |
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 was looking at this: https://github.com/interledger/rafiki/blob/main/packages/backend/src/webhook/model.ts#L8
Is it just id
, type
, and data
for all of them?
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.
yeah, I think the webhook service code I linked 👆 is the only place we're sending webhook requests
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 about here? I think we have two different sendWebhookEvent
s (which we should clean up)
rafiki/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Lines 148 to 190 in b9bce58
const handleCompleted = async ( | |
deps: ServiceDependencies, | |
payment: OutgoingPayment | |
): Promise<void> => { | |
await payment.$query(deps.knex).patch({ | |
state: OutgoingPaymentState.Completed | |
}) | |
await sendWebhookEvent(deps, payment, PaymentEventType.PaymentCompleted) | |
} | |
export const sendWebhookEvent = async ( | |
deps: ServiceDependencies, | |
payment: OutgoingPayment, | |
type: PaymentEventType | |
): Promise<void> => { | |
// Tigerbeetle accounts are only created as the OutgoingPayment is funded. | |
// So default the amountSent and balance to 0 for outgoing payments still in the funding state | |
const amountSent = | |
payment.state === OutgoingPaymentState.Funding | |
? BigInt(0) | |
: await deps.accountingService.getTotalSent(payment.id) | |
const balance = | |
payment.state === OutgoingPaymentState.Funding | |
? BigInt(0) | |
: await deps.accountingService.getBalance(payment.id) | |
if (amountSent === undefined || balance === undefined) { | |
throw LifecycleError.MissingBalance | |
} | |
const withdrawal = balance | |
? { | |
accountId: payment.id, | |
assetId: payment.assetId, | |
amount: balance | |
} | |
: undefined | |
await PaymentEvent.query(deps.knex).insertAndFetch({ | |
type, | |
data: payment.toData({ amountSent, balance }), | |
withdrawal | |
}) | |
} |
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 one ^ just adds it to the DB to prepare for sending. It looks the event.withdrawal
doesn't gets added to the webhook POST
request (the one that @wilsonianb commented), since body just has id
type
data
.
Maybe we can even just remote withdrawal
from the code completely on a follow-up PR?
@wilsonianb why are we skipping them? They pass, but I see the warning
|
We were updating dependencies 😅 |
- receiveAmount | ||
- state | ||
- stateAttempts | ||
- balance |
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.
- balance | |
- balance | |
- receiver |
receiver: 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.
Good catch!
Changes proposed in this pull request
Context
Checklist
fixes #number