-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(webhooks): add postgres persistence to clients and requests (feature branch) #108
Conversation
@@ -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", |
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.
Ooc is this a beta version?
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.
Yes, their last official release has some import/export errors that are fixed in this beta version.
@Column("simple-array") | ||
domains: 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.
Are these callback URIs?
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.
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
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.
looks good! Just two small comments
(Will approve when the migration files are included)
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.
lgtm!
@Column() | ||
apiKey: string; | ||
|
||
@Column("simple-array") |
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.
Is this a primitive type in postgres? Never heard of it. So far we handled such cases with the jsonb
type
@Column("simple-array") | |
@Column({ type: "jsonb"}) |
await queryRunner.query( | ||
`ALTER TABLE "evm"."root_bundle_canceled" ALTER COLUMN "finalised" DROP DEFAULT`, | ||
); | ||
await queryRunner.query( |
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.
All these COLUMN "finalised" DROP DEFAULT
should be removed
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.
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); |
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.
we should avoid using save()
and perform specific SQL queries such as INSERT
or UPDATE
.
await this.repository.save(client); | |
await this.repository.insert(client); |
@@ -0,0 +1,16 @@ | |||
import { Entity, PrimaryColumn, Column } from "typeorm"; | |||
|
|||
@Entity() |
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.
We should enforce uniqueness of the apiKey
@Entity() | |
@Entity() | |
@Unique(`UK_webhook_client_apiKey`, ["apiKey"]) |
public async findClientsByApiKey( | ||
apiKey: string, | ||
): Promise<entities.WebhookClient[]> { | ||
return this.repository.find({ where: { apiKey } }); |
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 should be a findOne
operation since the requirements don't allow duplicated apiKey
s
throw new Error(`Webhook with id ${webhookId} does not exist.`); | ||
} | ||
await this.store.delete(webhookId); | ||
await this.repository.delete({ id: webhookId }); | ||
} | ||
|
||
public async getWebhook( |
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.
public async getWebhook( | |
public async getWebhookRequest( |
} | ||
} | ||
return webhooks; | ||
public async filterWebhooks( |
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.
The function name is a bit misleading because it suggests that you can filter webhooks by custom fields. It should sound more like getWebhookRequestsByFilter
public async hasWebhook(webhookId: string): Promise<boolean> { | ||
return this.store.has(webhookId); | ||
const count = await this.repository.count({ where: { id: webhookId } }); | ||
return count > 0; | ||
} |
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.
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;
}
this.logger.info( | ||
`Attempting to register webhook of type: ${params.type} with URL: ${params.url}`, | ||
); |
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.
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"; |
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.
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; |
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.
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 { |
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.
We should also have the clientId
as FK
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 complicates things, but i can make it optional. right now we allow no client if configured that way
139d2f9
to
0bdfdfa
Compare
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>
ae14e87
to
22ddb90
Compare
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.