-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feature/add tx filter #15
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the pull request, I commented so please check it!
Modify address check logic
↓ |
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.
Thank you for your update! I commented, so please check again.
src/services/transaction.service.ts
Outdated
webhook, | ||
); | ||
if (status >= 400) { | ||
webhookCheck = false; |
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 check after Promise.all
as the result depends on the order in which the Wenhooks finished.
const statuses = await Promise.all(hooks.map(...))
const result = statuses.every(...)
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.
Thanks.
I modified to checkWebhook
to throw an error if there is an error
verse-proxy/src/services/webhook.service.ts
Lines 25 to 41 in 127031f
try { | |
const res = await lastValueFrom( | |
this.httpService.post(webhook.url, webhookBody, axiosConfig).pipe( | |
retry(webhook.retry), | |
catchError((e) => { | |
throw e; | |
}), | |
), | |
); | |
if (res.status < 200 || res.status >= 300) | |
throw new Error('transaction is not allowed'); | |
} catch (e) { | |
if (e instanceof Error) { | |
throw new JsonrpcError(e.message, -32602); | |
} | |
throw e; | |
} |
src/services/webhook.service.ts
Outdated
|
||
const webhookBody = { | ||
...(webhook.parse ? tx : body), | ||
_meta: { ip, headers: verseHeaders }, |
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.
I think that headers
should be passed as it is, but what about it?
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.
Thanks, I agree with you.
Set the header of the proxy request in _meta.header.
127031f#diff-95cb68dc1b2fad5c19e602ed7ba97f727c4afdcfa35c27c06697cf93ad8f292cR13-R17
src/services/transaction.service.ts
Outdated
|
||
const valueCondition = condition.value; | ||
const valueCheck = this.allowCheckService.isAllowedValue( | ||
valueCondition, | ||
value, | ||
); | ||
|
||
if (fromCheck && toCheck && valueCheck) { | ||
let webhookCheck = true; |
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.
Webhooks are expensive, so please continue early.
e.g.
if (!(fromCheck && toCheck && contractCheck && valueCheck)) {
continue
}
// call webhooks
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.
Thanks.
Fixed as well as rate limit.
verse-proxy/src/services/transaction.service.ts
Lines 74 to 87 in 127031f
if (fromCheck && toCheck && contractCheck && valueCheck) { | |
if (condition.rateLimit) | |
await this.rateLimitService.checkRateLimit( | |
from, | |
to, | |
methodId, | |
condition.rateLimit, | |
); | |
if (condition.webhooks) { | |
await this.webhookService.checkWebhook( | |
webhookArg, | |
condition.webhooks, | |
); | |
} |
This reverts commit 40870fb.
No description provided.