-
Notifications
You must be signed in to change notification settings - Fork 70
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
Support future end block #917
Conversation
}); | ||
|
||
switch (source.type) { | ||
case "log": { | ||
if (!isHistoricalSyncRequired) { | ||
this.logFilterProgressTrackers[source.id] = new ProgressTracker({ | ||
target: [startBlock, finalizedBlockNumber], | ||
completed: [[startBlock, finalizedBlockNumber]], |
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 is what was causing the original issue with unfinalized start blocks
@@ -342,6 +342,10 @@ export async function buildConfigAndIndexingFunctions({ | |||
? undefined | |||
: endBlockMaybeNan; | |||
|
|||
if (endBlock !== undefined && endBlock < startBlock) { |
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.
Nice
@@ -201,20 +199,15 @@ export class HistoricalSyncService extends Emittery<HistoricalSyncEvents> { | |||
await Promise.all( | |||
this.sources.map(async (source) => { | |||
const { isHistoricalSyncRequired, startBlock, endBlock } = | |||
validateHistoricalBlockRange({ | |||
getHistoricalBlockRange({ |
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.
Inline this?
|
||
// "realtime" property may be undefined when `kill()` has been | ||
// invoked but hasn't completed. | ||
if (networkService.realtime === undefined) return; |
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.
Would it be simpler to just use ?.
instead of !.
above?
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.
There's actually two different things that can be undefined. networkService
can be undefined depending on the result of .find()
. However, it's an invariant that it will always be defined, which is why the !
is used.
The realtime
property could be undefined if the case described in the comment occurs.
realtimeSyncEvent.checkpoint.blockNumber > | ||
networkService.realtime.endBlock | ||
) { | ||
networkService.realtime.realtimeSync.kill(); |
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 we should add an info log here: Synced final end block for '${network.name}' (${networkService.realtime.endBlock}), killing realtime sync service
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.
One more nit but LGTM!
}); | ||
const startBlock = source.startBlock; | ||
const endBlock = source.endBlock ?? finalizedBlockNumber; | ||
const isHistoricalSyncRequired = |
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 might be able to go one step further and consolidate the if (!isHistoricalSyncRequired) { ...
logic from each branch below
Fixes several bugs pertaining to unfinalized start and end blocks
closes #813