client: add blockTimestamp to eth_getLogs, add tests#4074
client: add blockTimestamp to eth_getLogs, add tests#4074acolytec3 merged 4 commits intoethereumjs:masterfrom
Conversation
jochem-brouwer
left a comment
jochem-brouwer
left a comment
There was a problem hiding this comment.
Heya, thanks for opening this PR! I got some comments regarding the tests 😄 👍
| assert.fail(`should return the correct logs (fromBlock/toBlock as 'earliest' and 'latest')`) | ||
| } | ||
|
|
||
| if (hexToBigInt(res.result[0].blockTimestamp) === block.header.timestamp) { |
There was a problem hiding this comment.
I think this test should assert.isEqual or something similar here, right? The if/else statement together with assert.fail / assert.isTrue(true simulates the behavior of the equal check
There was a problem hiding this comment.
Agreed, was just following the style of the surrounding tests, which did this a fair bit
| res.result[0].blockTimestamp === oldTimestamp && | ||
| res.result[10].blockTimestamp === oldTimestamp && | ||
| res.result[20].blockTimestamp === newTimestamp && | ||
| res.result[30].blockTimestamp === newTimestamp |
There was a problem hiding this comment.
This test is not very intuitive. Why should these values be these values? It seems like a range for eth_getLogs is tested but it is not clear to me why index 10 is oldTimestamp and 20 newTimestamp for instance
There was a problem hiding this comment.
Pushed some changes, hopefully its clearer
acolytec3
left a comment
acolytec3
left a comment
There was a problem hiding this comment.
This LGTM. @jochem-brouwer will leave it here for you to have another round of review before we merge though.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4 participants
This PR adds
blockTimestampas a parameter foreth_getLogsThis is a non-standard but non-breaking change from the execution-api specs, there's an open PR for it currently: ethereum/execution-apis#639
The reth team have already gone ahead with this change over a year ago: https://github.com/paradigmxyz/reth/pull/7606/files
So I don't see any downsides in supporting this feature to signal more general support for it.
This is my first PR for ethJS, so feel free to critique / nitpick / educate me on anything that needs changing.
Thanks!