Skip to content
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

feat(webhooks): add postgres persistence to clients and requests (feature branch) #108

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

daywiss
Copy link
Contributor

@daywiss daywiss commented Nov 20, 2024

motivation

We need to store client and webhook url data

changes

This adds persistence through postgres and typeorm entities. Removed the store concept for a direct connection to typeorm for simplicity, since we dont expect to use other stores.

Copy link

linear bot commented Nov 20, 2024

@@ -25,7 +25,8 @@
"express": "^4.19.2",
"express-bearer-token": "^3.0.0",
"redis": "^4.7.0",
"superstruct": "2.0.3-1"
"superstruct": "2.0.3-1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooc is this a beta version?

Copy link
Contributor

@melisaguevara melisaguevara Nov 20, 2024

Choose a reason for hiding this comment

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

Yes, their last official release has some import/export errors that are fixed in this beta version.

Comment on lines 14 to 16
@Column("simple-array")
domains: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these callback URIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, its a way to whitelist domains for the webhook registration. for instance if coinbase registers coinbase.com, only that client would be able to include coinbase.com in their webhook urls

Copy link
Contributor

@melisaguevara melisaguevara left a comment

Choose a reason for hiding this comment

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

looks good! Just two small comments
(Will approve when the migration files are included)

packages/webhooks/src/database/webhookClientRepository.ts Outdated Show resolved Hide resolved
packages/webhooks/src/factory.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@melisaguevara melisaguevara left a comment

Choose a reason for hiding this comment

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

lgtm!

@Column()
apiKey: string;

@Column("simple-array")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a primitive type in postgres? Never heard of it. So far we handled such cases with the jsonb type

Suggested change
@Column("simple-array")
@Column({ type: "jsonb"})

Comment on lines 13 to 16
await queryRunner.query(
`ALTER TABLE "evm"."root_bundle_canceled" ALTER COLUMN "finalised" DROP DEFAULT`,
);
await queryRunner.query(
Copy link
Contributor

Choose a reason for hiding this comment

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

All these COLUMN "finalised" DROP DEFAULT should be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Just OOC, why should they be removed? Curious if there's anything we have to fix in the entities

throw new Error(`Client with id ${client.id} already exists.`);
}
await this.store.set(client.id, client);
await this.repository.save(client);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should avoid using save() and perform specific SQL queries such as INSERT or UPDATE.

Suggested change
await this.repository.save(client);
await this.repository.insert(client);

@@ -0,0 +1,16 @@
import { Entity, PrimaryColumn, Column } from "typeorm";

@Entity()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should enforce uniqueness of the apiKey

Suggested change
@Entity()
@Entity()
@Unique(`UK_webhook_client_apiKey`, ["apiKey"])

public async findClientsByApiKey(
apiKey: string,
): Promise<entities.WebhookClient[]> {
return this.repository.find({ where: { apiKey } });
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a findOne operation since the requirements don't allow duplicated apiKeys

throw new Error(`Webhook with id ${webhookId} does not exist.`);
}
await this.store.delete(webhookId);
await this.repository.delete({ id: webhookId });
}

public async getWebhook(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public async getWebhook(
public async getWebhookRequest(

}
}
return webhooks;
public async filterWebhooks(
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name is a bit misleading because it suggests that you can filter webhooks by custom fields. It should sound more like getWebhookRequestsByFilter

Comment on lines 48 to 64
public async hasWebhook(webhookId: string): Promise<boolean> {
return this.store.has(webhookId);
const count = await this.repository.count({ where: { id: webhookId } });
return count > 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, a SELECT query executes faster than a COUNT, so checking existence is recommended to be done with SELECT LIMIT 1.

public async hasWebhookRequest(webhookId: string): Promise<boolean> {
    const webhookRequest = await this.repository.findOne({
      where: { id: webhookId },
    });
    return webhookRequest !== null;
  }

Comment on lines 66 to 77
this.logger.info(
`Attempting to register webhook of type: ${params.type} with URL: ${params.url}`,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we lower the level of these logs? Maybe debug.
I use to filter GCP logs >= info so I can watch the more important logs during indexing

notify: (params: NotificationPayload) => void;
postgres: DataSource;
};
export class DepositStatusProcessor implements IEventProcessor {
private webhookRequests: WebhookRequestRepository;
private notify: (params: NotificationPayload) => void;
private postgres: DataSource;
// Type shoudl be uniqe across all event processors, this is to avoid colliding with multiple
// processors writing to the same tables
static type = "DepositStatus";
Copy link
Contributor

Choose a reason for hiding this comment

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

hardcoded strings should really be avoided. If this is common to all event processors, should we add the type field tot the IEventProcessor interface?
And also making it to have the WebhookTypes type

name: string;

@PrimaryColumn()
id: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a @PrimaryGeneratedColumn() ? I think it would be easier to have it as an incremental number managed by postgres

import { Entity, PrimaryColumn, Column } from "typeorm";

@Entity()
export class WebhookClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also have the clientId as FK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this complicates things, but i can make it optional. right now we allow no client if configured that way

@daywiss daywiss force-pushed the david/acx-3360-add-persistence-to-webhooks-package branch 2 times, most recently from 139d2f9 to 0bdfdfa Compare November 22, 2024 18:40
@amateima amateima changed the title feat(webhooks): add postgres persistence to clients and requests feat(webhooks): add postgres persistence to clients and requests (feature branch) Nov 22, 2024
daywiss and others added 6 commits November 26, 2024 12:48
Signed-off-by: david <david@umaproject.org>
Signed-off-by: david <david@umaproject.org>
Signed-off-by: david <david@umaproject.org>
Signed-off-by: david <david@umaproject.org>
Signed-off-by: david <david@umaproject.org>
Signed-off-by: david <david@umaproject.org>
Co-authored-by: david <david@umaproject.org>
@amateima amateima force-pushed the david/acx-3360-add-persistence-to-webhooks-package branch from ae14e87 to 22ddb90 Compare November 26, 2024 11:48
@amateima amateima merged commit c90f73a into master Nov 26, 2024
3 checks passed
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.

4 participants