feat: webhook module v1 #101

daywiss · 2024-11-14T18:30:47Z

Motivation

We want to be able to handle webhook requests to the indexer, starting with deposit status

changes

This adds a module in the indexer to startup an express based webhook manager, with an interface to add new event types, as well as register many webhooks.

usage

factory in the main entry point which gives you a WebhookFactory call, which returns {webooks ,express ,notifier}. As the indexer, all you need to do is call webhooks.write({type:'DepositStatus',event:EventData})
You also need to setup an interval to run the notification worker by calling notifier.tick() which will try to send out all pending notifications to webhooks

integrators will call into the express app on POST /webhook, passing body = {type:'DepositStatus',filter:{originChainId,depositTxHash}, url:string}. This will begin the listening process for indexer updates. It should also emit a first deposit status if one exists.

testing

this has not been tested

TODO

In future prs:

some testing
better api gating and client management
add persistence to stores

linear · 2024-11-14T18:30:52Z

ACX-3349 add webhook module

daywiss · 2024-11-14T18:47:50Z

packages/indexer/src/webhooks/notifier.ts

+          await this.deps.notify(
+            notification.payload.url,
+            notification.payload.event,
+          );


this is where the webhook callback happens. further down you can see we try a backoff strategy if it fails

daywiss · 2024-11-14T18:49:04Z

packages/indexer/src/webhooks/webhook.ts

+  notify: (params: NotificationPayload) => void;
+  postgres: DataSource;
+};
+export class DepositStatusWebhook implements IWebhook {


this is the specific deposit status processor we care about, its wrapped in a higher level webhooks class in case we want to define different types

daywiss · 2024-11-14T18:49:34Z

packages/indexer/src/webhooks/webhooks.ts

+    assert(webhook, "Webhook does not exist by type: ${event.type}");
+    return webhook;
+  }
+  write(event: EventType): void {


as the indexer, you want to call this function to write your events

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

daywiss · 2024-11-14T18:51:01Z

packages/indexer/src/webhooks/factory.ts

+  logger: Logger;
+};
+
+export function WebhookFactory(config: Config, deps: Dependencies) {


main entry point as an indexer

daywiss · 2024-11-14T18:51:35Z

packages/indexer/src/webhooks/factory.ts

+            hooks,
+            notify: notifier.notify,
+          }),
+        );


this is where we instance and register each webhook event type, currently we only have 'DepositStatus'
we can see it has a callback notify which queues it in the notifier, which persists notification state

daywiss · 2024-11-14T18:52:05Z

packages/indexer/src/webhooks/notifier.ts

+    });
+  };
+
+  public tick = async (now = Date.now()): Promise<void> => {


you will manually need to call notifier.tick on an interval somewhere in indexer code ( or whoever consumes this module)

daywiss · 2024-11-14T18:52:41Z

packages/indexer/src/webhooks/notifier.ts

+
+export class WebhookNotifier extends BaseNotifier {
+  constructor(deps: Omit<Dependencies, "notify">) {
+    super({ ...deps, notify: post });


makes a specific webhook caller, we could theoretically pass in an event emitter for testing rather than https post

daywiss · 2024-11-14T18:53:16Z

packages/indexer/src/webhooks/store.ts

@@ -0,0 +1,107 @@
+import { createClient, RedisClientType } from "redis";
+
+export interface AsyncStore<V> {


abstract store, hopefully it wouldnt be too difficult to hook this up to postgres. will speed up development to have a common store interface

daywiss · 2024-11-14T18:53:54Z

packages/indexer/src/webhooks/store.ts

+  }
+}
+
+export class RedisStore<V> implements AsyncStore<V> {


redis store if we want to use it

daywiss · 2024-11-14T18:54:11Z

packages/indexer/src/webhooks/utils.ts

@@ -0,0 +1,17 @@
+export async function post(url: string, data: any): Promise<void> {


what we use to post to webhook

daywiss · 2024-11-14T18:55:03Z

packages/indexer/src/webhooks/webhooks.ts

+          isDomainValid,
+          "The base URL of the provided webhook does not match any of the client domains",
+        );
+      }


this was my attempt at gaurding a webhook for a domain, but this may need to be tested/rethought

daywiss · 2024-11-14T18:58:42Z

packages/indexer/src/webhooks/express.ts

+  app.use(bearerToken());
+
+  app.post(
+    "/webhook",


integrator register a new webhook

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

amateima

Looks good. Left a few comments.

amateima · 2024-11-18T11:08:06Z

packages/webhooks/src/clients.ts

+}
+
+// This class is intended to store integration clients allowed to use the webhook service.
+export class WebhookClientManager {


From what I see this WebhookClientManager is used as a repository to interact with a store/database, so it should be renamed to WebhookClientRepository or WebhookClientMemoryRepository and placed in a adapter/db directory

amateima · 2024-11-18T11:13:54Z

packages/webhooks/src/router.ts

+  id: ss.string(),
+});
+
+export function WebhookRouter(deps: Dependencies): express.Router {


all HTTP related logic: routers, controllers, validation, types for query parameters, types for URL parameters, types for HTTP bodies, etc should be placed in a /entrypoint/http directory

following our conversation on Friday, the best for now would be to remove the express logic from the webhooks package and declare a /webhooks/register endpoint in the indexer API that (1) saves the webhook request in the DB and (2) publishes a message on a queue like WebhookRequestRegistered to inform the indexer that a request has been received and it needs to send the initial status to the webhook client.

P.S I asked Matt if the initial status should be sent to the webhook request URL or if we can attach it to the response body of the /register endpoint. But must likely it will be option (1)

following our conversation on Friday, the best for now would be to remove the express logic from the webhooks package and declare a /webhooks/register endpoint in the indexer API that (1) saves the webhook request in the DB

This logic has been updated to remove references to express app, and only return a router, which is very portable. The reason i think we should at least return a router, is because there needs to be type validation on the routes, and you dont want the consumer of the webhook library to need to know the types the library expects to correctly validate.

P.S I asked Matt if the initial status should be sent to the webhook request URL or if we can attach it to the response body of the /register endpoint. But must likely it will be option (1)

would highly prefer all data updates go through webhook calls. it simplifies both our logic and consumer logic.

amateima · 2024-11-18T11:33:45Z

packages/webhooks/src/webhookRequests.ts

+import { AsyncStore } from "./store";
+import { Webhook } from "./types";
+
+export class WebhookRequests {


again, this behaves as a repository class and it should be placed in the /adapter/db directory with a more descriptive name: WebhookRequestRepository. Also maybe we can make the file to have the same name as the exported class

amateima · 2024-11-18T11:40:08Z

packages/webhooks/src/notifier.ts

+  public notify = (payload: NotificationPayload): void => {
+    this.create(payload).catch((error) => {
+      this.logger.error(`Error creating notification:`, error);
+    });
+  };


After creation, the notification should be sent right away, not scheduled to be sent later in a batch.

amateima · 2024-11-18T11:41:24Z

packages/webhooks/src/types.ts

+
+export type NotificationPayload = {
+  url: string;
+  event: JSONValue;


this should be named more generic, like "data" or smth. Not sure what event means in this context

event makes me think about past tense verbs: DepositStatusChanged, DepositFilled, etc but here the notification will contain data { status, depositTxHash }

ive renamed this to "data"

amateima · 2024-11-18T11:45:20Z

packages/webhooks/src/notifier.ts

+    this.pending = deps.pending;
+    this.completed = deps.completed;


If it would simplify things for you, we can skip saving the notifications in a store and we will make them stateful later

removed any stateful notifications

amateima · 2024-11-18T11:48:03Z

packages/webhooks/src/types.ts

+export interface Webhook {
+  id: string;
+  url: string;
+  filter: string;
+}


Following the language we used so far, I think WebhookRequests would be a better name for this

renamed this

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

james-a-morris · 2024-11-18T19:26:16Z

packages/webhooks/src/database/webhookClientRepository.ts

+  public async unregisterClient(clientId: string): Promise<void> {
+    if (!(await this.store.has(clientId))) {
+      throw new Error(`Client with id ${clientId} does not exist.`);
+    }
+    await this.store.delete(clientId);
+  }


Suggested change

public async unregisterClient(clientId: string): Promise<void> {

if (!(await this.store.has(clientId))) {

throw new Error(`Client with id ${clientId} does not exist.`);

}

await this.store.delete(clientId);

}

public unregisterClient(clientId: string): Promise<void> {

if (!(await this.store.has(clientId))) {

throw new Error(`Client with id ${clientId} does not exist.`);

}

return this.store.delete(clientId);

}

these need async, the stores are async cuz they will be calling a database

james-a-morris · 2024-11-18T19:26:40Z

packages/webhooks/src/database/webhookClientRepository.ts

+    await this.store.delete(clientId);
+  }
+
+  public async getClient(clientId: string): Promise<WebhookClient | undefined> {


Suggested change

public async getClient(clientId: string): Promise<WebhookClient | undefined> {

public getClient(clientId: string): WebhookClient | undefined {

james-a-morris · 2024-11-18T19:26:58Z

packages/webhooks/src/database/webhookClientRepository.ts

a lot of these functions may not need to be async

the store functions are all async, even though right now they are using a memory map, eventually will plugin a real database

james-a-morris · 2024-11-18T19:27:26Z

packages/webhooks/src/store.ts

+    }
+  }
+
+  async *entries(): AsyncIterableIterator<[string, V]> {


OOC what is this *entries invocation?

its a generator which emits values asyncronously (async iterator). you can iterate over it like:

for await (const [id, client] of repository.entries()) { console.log(`Client ID: ${id}, Client URL: ${client.url}`); }

its kind of like a stream where the consumer is pulling the values in

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

daywiss force-pushed the david/acx-3349-add-webhook-module branch from 922b374 to f9663ba Compare November 14, 2024 18:39

daywiss requested a review from amateima November 14, 2024 18:39

daywiss force-pushed the david/acx-3349-add-webhook-module branch from f9663ba to 991c51a Compare November 14, 2024 18:40

daywiss commented Nov 14, 2024

View reviewed changes

feat: webhook module v1

7c77181

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

daywiss force-pushed the david/acx-3349-add-webhook-module branch from 991c51a to 7c77181 Compare November 14, 2024 18:50

daywiss commented Nov 14, 2024

View reviewed changes

move webhook to package, refactor express to router

b36e367

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

daywiss force-pushed the david/acx-3349-add-webhook-module branch from 4547180 to b36e367 Compare November 15, 2024 20:01

amateima requested changes Nov 18, 2024

View reviewed changes

improve naming and organization, instantiate webhooks in indexer

ec6c1dd

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

daywiss marked this pull request as ready for review November 18, 2024 17:45

daywiss requested a review from amateima November 18, 2024 17:45

daywiss assigned james-a-morris and unassigned james-a-morris Nov 18, 2024

daywiss requested review from melisaguevara and james-a-morris November 18, 2024 17:45

update readme

143b641

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

james-a-morris reviewed Nov 18, 2024

View reviewed changes

daywiss requested a review from james-a-morris November 19, 2024 17:39

use enum for webhook types, simplify configs and return vals

aca4341

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

daywiss force-pushed the david/acx-3349-add-webhook-module branch from 43272ef to aca4341 Compare November 19, 2024 17:43

amateima approved these changes Nov 19, 2024

View reviewed changes

daywiss merged commit 5d97e5e into master Nov 20, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: webhook module v1 #101

feat: webhook module v1 #101

daywiss commented Nov 14, 2024 •

edited

Loading

linear bot commented Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

daywiss Nov 14, 2024

amateima left a comment

amateima Nov 18, 2024

amateima Nov 18, 2024

amateima Nov 18, 2024

daywiss Nov 18, 2024

amateima Nov 18, 2024

amateima Nov 18, 2024

amateima Nov 18, 2024

amateima Nov 18, 2024

daywiss Nov 18, 2024

amateima Nov 18, 2024

daywiss Nov 18, 2024

amateima Nov 18, 2024

daywiss Nov 18, 2024

james-a-morris Nov 18, 2024

daywiss Nov 19, 2024

james-a-morris Nov 18, 2024

james-a-morris Nov 18, 2024

daywiss Nov 19, 2024

james-a-morris Nov 18, 2024

daywiss Nov 19, 2024 •

edited

Loading

		@@ -0,0 +1,107 @@
		import { createClient, RedisClientType } from "redis";

		export interface AsyncStore<V> {

		@@ -0,0 +1,17 @@
		export async function post(url: string, data: any): Promise<void> {

		this.pending = deps.pending;
		this.completed = deps.completed;

	public async getClient(clientId: string): Promise<WebhookClient \| undefined> {
	public getClient(clientId: string): WebhookClient \| undefined {

feat: webhook module v1 #101

feat: webhook module v1 #101

Conversation

daywiss commented Nov 14, 2024 • edited Loading

Motivation

changes

usage

testing

TODO

linear bot commented Nov 14, 2024

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 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

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

daywiss Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

daywiss commented Nov 14, 2024 •

edited

Loading

daywiss Nov 19, 2024 •

edited

Loading