-
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: fetch complete block numbers & ignore false positive error #52
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.
Left some minor code style comments.
More importantly, I've got a business logic question just to be super sure:
Are the ignored TimestampsUpdated
events skipped forever? Do they need to be reprocessed somehow in the future?
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.
Good just some smol comments
@@ -10,13 +13,11 @@ export interface IEventsFetcher { | |||
* @param chainId id of the chain | |||
* @param blockNumber block number to fetch events from | |||
* @param logIndex log index in the block to fetch events from | |||
* @param limit limit of events to fetch | |||
* @param limit limit of events to fetch\ |
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.
Hanging backslash
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.
will fix in another PR the typo
*/ | ||
private async getTotalEventsInBlock( | ||
chainId: ChainId, | ||
blockNumber: number, |
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 we wanting to use the {} syntax for just if there are 3+ params?
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 have to define our standard but given this is a private method, i wouldn't be so strict on the option, probably for public methods or interfaces is more important to follow the standard of {}
🤖 Linear
Closes GIT-228
Description
Some strategies throw false positive errors for the
TimestampUpdated
event because on contract init, it emit the events before thePoolCreated
event (all in the same transaction). This causes that the TimestampUpdated handler doesn't find the Pool and throw an error, however this is expected behaviour (from contract's perspective).To circumvent this, we need to have more events context to determine if the error was thrown on this edge case or is a real error. So we update the Indexer Client to:
Checklist before requesting a review