feat: integrate webhooks in the indexer #114

amateima · 2024-11-22T21:18:24Z

integrate webhooks package in the indexer
trigger initial notification after webhook request registration using WebhookRequest queue
make apiKey required

Signed-off-by: david <[email protected]>

…o david/acx-3360-add-persistence-to-webhooks-package-1

linear · 2024-11-22T21:18:26Z

ACX-3360 Add persistence to webhooks package

james-a-morris

few nits + few questions. Looks great

james-a-morris · 2024-11-22T21:29:37Z

packages/webhooks/src/utils.ts

  const response = await fetch(url, {
    method: "POST",
    headers: {
      "Content-Type": "application/json",
+      Authorization: `Bearer ${apiKey}`,


OOC - why did we go with fetch vs axios?

🤷‍♂️ this package was started with fetch so I went with it too. Also, I am a fan of using as few external packages as possible

axios has a nicer syntax tho...

james-a-morris · 2024-11-22T21:30:19Z

packages/webhooks/src/types.ts

@@ -6,7 +6,7 @@ export interface IEventProcessor {
    id: string,
    url: string,
    params: JSONValue,
-    clientId?: string,
+    clientId: number,


Could we not represent a number as a numeric string?

not sure I understand the question. This change happened because I updated the WebhookClient primary key from a string id computed by us to an incremental number managed by postgres

i tihnk he means its more flexible to keep this as a string and use number.toString or parseInt to convert from and to postgres

james-a-morris · 2024-11-22T21:31:07Z

packages/webhooks/src/router.ts

      res: express.Response,
      next: express.NextFunction,
    ) => {
      try {
        const parsedBody = RegistrationParams.create(req.body);
+        if (!req.token) {


If we created a local token variable const token = req.token and checked it directly then we wouldn't need to typecast below.

james-a-morris · 2024-11-22T21:32:43Z

packages/webhooks/src/database/webhookClientRepository.ts

OOC what if two or more separate clients ask to send data to the same callback url?

I don't think that's possible because we check the callback url agains the domains list from the WebhookClient and that table is managed manually by us for now

james-a-morris · 2024-11-22T21:33:22Z

packages/webhooks/src/adapter/messaging/WebhooksQueuesService.ts

+    const q = this.queues[queue];
+    if (q) {
+      await q.add(queue, message, options);
+    }


Suggested change

const q = this.queues[queue];

if (q) {

await q.add(queue, message, options);

}

await this.queues[queue]?.add(queue, message, options);

We may want to use this in multiple spots

@james-a-morris I plan to add a warn || error log on else, that's why I used if

daywiss · 2024-11-25T12:43:19Z

packages/webhooks/src/adapter/messaging/WebhookRequestWorker.ts

+    );
+  }
+
+  private async run(webhookRequestJob: WebhookRequestQueueJob) {


really against putting this business logic here, im happy to take a crack at refactoring this in a future pr though.
i think what id do is basically pass in the eventprocessor manager in the webhookrequestworker constructor, then assume whenever "run" is called, its a webhook registration, and call "register" on the eventprocessormanager, which will call the correct event processor and put the logic for pulling in initial state and calling webhook in there.

its a subtle change, but means that we could remove this worker if everythign resides on the same process

I agree that this is not the best approach, but what if I place this worker in the indexer package?

put the logic for pulling in initial state and calling webhook in there.

I do feel that the write() function should be called only externally and not from the webhooks package. The package should only deal with forwarding notification payloads from our systems to clients and not dealing at all from where write() is being called. It can be procedural code, cron jobs, Bull workers, etc. wdyt?

we should discuss further offline, but as of now i think this is ok, we can figure out how to change later

daywiss · 2024-11-25T12:44:14Z

packages/webhooks/src/eventProcessorManager.ts

@@ -101,7 +113,7 @@ export class EventProcessorManager {
    this.logger.debug(
      `Attempting to unregister webhook of type: ${params.type} with ID: ${params.id}`,
    );
-    const webhook = this.getEventProcessor(params.type);
+    const webhook = this.getEventProcessor(params.type as WebhookTypes);


could make a function to assert this

daywiss · 2024-11-25T12:50:03Z

packages/webhooks/src/eventProcessors/depositStatus.ts

-          data: event,
-        });
+      webhookRequests.map((hook) => {
+        const client = clientsMap[hook.clientId];


theres a lot of work above just to get this cache, not sure why u dont just make this function async(you are already using promise.all even though function is currently sync), and query for the client directly here, and delete all the code from 54-69

I removed Promise.all(). Basically I don't want to fetch the client for each request because this will trigger many duplicated queries, so I fetch only the unique clients

daywiss · 2024-11-25T12:51:39Z

packages/webhooks/src/types.ts

@@ -6,7 +6,7 @@ export interface IEventProcessor {
    id: string,
    url: string,
    params: JSONValue,
-    clientId?: string,
+    clientId: number,


i tihnk he means its more flexible to keep this as a string and use number.toString or parseInt to convert from and to postgres

Signed-off-by: david <[email protected]> Co-authored-by: david <[email protected]>

daywiss and others added 8 commits November 22, 2024 19:33

feat(webhooks): add postgres persistence to clients and requests

aabb853

Signed-off-by: david <[email protected]>

remove unneeded dependencies

c298f34

Signed-off-by: david <[email protected]>

add webhook migrations

d90f0c5

Signed-off-by: david <[email protected]>

refactored event processor interface

8bbf5c5

Signed-off-by: david <[email protected]>

Comment code

f482aeb

Merge branch 'david/acx-3360-add-persistence-to-webhooks-package' int…

0a26b15

…o david/acx-3360-add-persistence-to-webhooks-package-1

Remove migrations

e543d4e

feat: integrate webhooks in the indexer

89cb8c1

amateima requested review from daywiss, melisaguevara and james-a-morris November 22, 2024 21:20

james-a-morris reviewed Nov 22, 2024

View reviewed changes

Fix

5d32c0b

amateima requested a review from james-a-morris November 22, 2024 21:59

daywiss reviewed Nov 25, 2024

View reviewed changes

Remove promise.all

5503792

amateima requested a review from daywiss November 25, 2024 14:27

daywiss approved these changes Nov 25, 2024

View reviewed changes

amateima merged commit ae14e87 into david/acx-3360-add-persistence-to-webhooks-package Nov 25, 2024
3 checks passed

amateima added a commit that referenced this pull request Nov 26, 2024

feat: integrate webhooks in the indexer (#114)

22ddb90

Signed-off-by: david <[email protected]> Co-authored-by: david <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate webhooks in the indexer #114

feat: integrate webhooks in the indexer #114

amateima commented Nov 22, 2024 •

edited

Loading

linear bot commented Nov 22, 2024

james-a-morris left a comment

james-a-morris Nov 22, 2024

amateima Nov 22, 2024

amateima Nov 22, 2024

james-a-morris Nov 22, 2024

amateima Nov 22, 2024

daywiss Nov 25, 2024

james-a-morris Nov 22, 2024

amateima Nov 22, 2024

james-a-morris Nov 22, 2024

amateima Nov 22, 2024

james-a-morris Nov 22, 2024

james-a-morris Nov 22, 2024

amateima Nov 22, 2024 •

edited

Loading

daywiss Nov 25, 2024

amateima Nov 25, 2024 •

edited

Loading

daywiss Nov 25, 2024

daywiss Nov 25, 2024

daywiss Nov 25, 2024

amateima Nov 25, 2024

daywiss Nov 25, 2024

feat: integrate webhooks in the indexer #114

feat: integrate webhooks in the indexer #114

Conversation

amateima commented Nov 22, 2024 • edited Loading

linear bot commented Nov 22, 2024

james-a-morris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amateima Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amateima Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amateima commented Nov 22, 2024 •

edited

Loading

amateima Nov 22, 2024 •

edited

Loading

amateima Nov 25, 2024 •

edited

Loading