fix: last_checked_block retrieval #138
Conversation
observer/lib/observer.js
Outdated
There was a problem hiding this comment.
fix lgtm, could you please also add a test for this? You can mock ieContract so we only test the sql logic
observer/bin/dry-run.js
Outdated
| @@ -18,4 +18,8 @@ await pgPool.query('DELETE FROM daily_reward_transfers') | |||
|
|
|||
| await observeTransferEvents(pgPool, ieContract, provider) | |||
|
|
|||
| // Do it a second time, without clearing the table. | |||
| // This should find 0 events, unless rewards are currently being released. | |||
| await observeTransferEvents(pgPool, ieContract, provider) | |||
There was a problem hiding this comment.
I think it's safer to add an automated test instead
| ) | ||
| let queryFromBlock = rows[0].last_checked_block | ||
| let queryFromBlock = rows[0].last_checked_block + 1 |
There was a problem hiding this comment.
I think there is bug lurking here.
In the initial run against an empty database, last_checked_block will be undefined and queryFromBlock will be set to NaN.
❯ node
Welcome to Node.js v20.14.0.
Type ".help" for more information.
> undefined + 1
NaN
There was a problem hiding this comment.
We check the truthy value of the queryFromBlock directly after this, so it ends up being reset to the -1900 block
There was a problem hiding this comment.
Ah, I see. I verified that this works as you described.
> queryFromBlock = undefined + 1
NaN
> currentBlockNumber=6543
6543
> !queryFromBlock || queryFromBlock < currentBlockNumber - 1900
true
>
It's great that the bug is not there.
It would be nice to rewrite this part to make it easier for casual readers (or novice programmers) to correctly understand how exactly the code works and how it handles different edge cases.
There was a problem hiding this comment.
Ok! Do you prefer just a comment explaining it, or a logic change?
| const getDayAsISOString = d => d.toISOString().split('T')[0] | ||
| const today = () => getDayAsISOString(new Date()) |
There was a problem hiding this comment.
Was there any particular reason for moving these two helper functions into a describe block? Can we move them back to the top where they were before?
There was a problem hiding this comment.
Oops, merge conflicts confusion! Shall I open a PR for this?
Related to #134 and #102