Skip to content
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

Add getLogs by blockHash #1269

Merged
merged 5 commits into from
Apr 3, 2019

Conversation

charles-cooper
Copy link
Contributor

What was wrong?

getLogs did not accept the blockHash parameter.

Related to Issue #1236

How was it fixed?

Added blockHash as a parameter in contract.getLogs. Marked WIP because I was not sure if my approach was in line with the style of the rest of the code base, but I did not want to touch construct_event_filter_params because that can also be used to create a filter (for which blockHash is not a valid parameter). Feedback/guidance on my approach would be appreciated!

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style/approach looks to be in line with the rest of the codebase, @charles-cooper. Thanks for taking it on!

@charles-cooper
Copy link
Contributor Author

Great! The next step I wanted to take was to change the defaults for fromBlock and toBlock in contract.getLogs to be None. That way we can a) enforce that only fromBlock/toBlock or blockHash is set (following the semantics of eth_getLogs) and b) 'pass through' the default semantics of eth_getLogs. Would that be OK? (paging @miohtama for input)

fromBlock: QUANTITY|TAG - (optional, default: "latest") Integer block number, or "latest" for the last mined block or "pending", "earliest" for not yet mined transactions.
toBlock: QUANTITY|TAG - (optional, default: "latest") Integer block number, or "latest" for the last mined block or "pending", "earliest" for not yet mined transactions.
...
blockhash: DATA, 32 Bytes - (optional) With the addition of EIP-234 (Geth >= v1.8.13 or Parity >= v2.1.0), blockHash is a new filter option which restricts the logs returned to the single block with the 32-byte hash blockHash. Using blockHash is equivalent to fromBlock = toBlock = the block number with hash blockHash. If blockHash is present in the filter criteria, then neither fromBlock nor toBlock are allowed.

Defaults for fromBlock and toBlock are now None, and additional
validation is provided for blockHash.
@kclowes
Copy link
Collaborator

kclowes commented Mar 11, 2019

That makes sense to me. @pipermerriam do you see issues with that approach?

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 apart from the test and lint failures.

@charles-cooper charles-cooper changed the title [WIP] Add getLogs by blockHash Add getLogs by blockHash Mar 15, 2019
@kclowes
Copy link
Collaborator

kclowes commented Mar 25, 2019

I didn't forget about this @charles-cooper. I should be able to get to it early next week! Thanks!

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @charles-cooper! Thanks!

@kclowes kclowes merged commit 814c3c0 into ethereum:master Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants