Skip to content

feat: Move transaction webhooks to Redis #549

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

Merged
merged 4 commits into from
Jun 19, 2024
Merged

feat: Move transaction webhooks to Redis #549

merged 4 commits into from
Jun 19, 2024

Conversation

arcoraven
Copy link
Contributor

@arcoraven arcoraven commented Jun 19, 2024

PR-Codex overview

This PR introduces changes related to handling webhooks and transactions. It refactors webhook handling, updates transaction schemas, and improves webhook processing efficiency.

Detailed summary

  • Refactored webhook handling logic
  • Updated transaction schemas
  • Improved webhook processing efficiency
  • Renamed functions for clarity

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

}

if (resp && !resp.ok) {
if (!resp.ok) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resp has type WebhookResponse | undefined. would this change cause unexpected behavior if resp is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default catches if resp is undefined, but I see how it can be a trap that bites us in the future.

}

if (resp && !resp.ok) {
if (!resp.ok) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default catches if resp is undefined, but I see how it can be a trap that bites us in the future.

Static<typeof transactionResponseSchema>[] | null
> => {
const tx = await prisma.transactions.findMany({
export const getTransactionsByQueueIds = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified this to just return DB results.

@@ -3,6 +3,9 @@ import { prisma } from "../client";

export const getAllWebhooks = async (): Promise<Webhooks[]> => {
return await prisma.webhooks.findMany({
where: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never care about deleted webhooks so omit it in the query to simplify business logic.

@@ -198,3 +200,23 @@ export enum TransactionStatus {
// Tx was cancelled and will not be re-attempted.
Cancelled = "cancelled",
}

export const toTransactionSchema = (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maps the internal DB "transaction" to the external "transaction" schema.

Comment on lines +128 to +131
const webhooks = [
...(await getWebhooksByEventType(WebhooksEventTypes.ALL_TX)),
...(await getWebhooksByEventType(data.type)),
];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get all webhooks that match the exact type or ALL_TX which cares about all event types.

@@ -52,7 +70,7 @@ const handler: Processor<any, void, string> = async (job: Job<string>) => {
let _worker: Worker | null = null;
if (redis) {
_worker = new Worker(SEND_WEBHOOK_QUEUE_NAME, handler, {
concurrency: 1,
concurrency: 10,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increase concurrency now that there's more webhooks to send. Webhook calls are independent.

@arcoraven arcoraven merged commit bac0a7e into main Jun 19, 2024
4 checks passed
@arcoraven arcoraven deleted the ph/webhookWorkers branch June 19, 2024 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants