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

Implement support for the "blockHash" parameter #4387

Merged
merged 3 commits into from
Aug 10, 2022
Merged

Conversation

rubo
Copy link
Contributor

@rubo rubo commented Aug 8, 2022

Fixes #4384

Changes

Added support for the blockHash parameter to the eth_getLogs RPC method. When this parameter is specified, the fromBlock and toBlock parameters are ignored if provided.

Types of changes

What types of changes does your code introduce?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

@rubo rubo marked this pull request as ready for review August 8, 2022 22:42
@rubo rubo changed the title Implement support for the "blockhash" parameter Implement support for the "blockHash" parameter Aug 8, 2022
Comment on lines +631 to 634
if (filter.FromBlock == filter.ToBlock)
fromBlockResult = toBlockResult = _blockFinder.SearchForHeader(filter.ToBlock);
else
{
Copy link
Member

Choose a reason for hiding this comment

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

Minor-style: One thing that is bothering me and makes it harder to read is the inconsistency of brackets in code blocks in same if-else. if is bracketless and then you have else that is longer block and has {}. Looks weird and inconsistent to me, makes it harder to follow the logic and boundaries at just a glance.

Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

please test it with CL clients on some merge network. eth_getLogs is used by CLs

Comment on lines +40 to +47
if (blockHash is null)
{
JObject jObject = serializer.Deserialize<JObject>(jsonValue.ToJsonTextReader());
FromBlock = BlockParameterConverter.GetBlockParameter(jObject["fromBlock"]?.ToObject<string>());
ToBlock = BlockParameterConverter.GetBlockParameter(jObject["toBlock"]?.ToObject<string>());
Address = GetAddress(jObject["address"]);
Topics = GetTopics(jObject["topics"] as JArray);
FromBlock = BlockParameterConverter.GetBlockParameter(filter["fromBlock"]?.Value<string>());
ToBlock = BlockParameterConverter.GetBlockParameter(filter["toBlock"]?.Value<string>());
}
else
FromBlock =
ToBlock = BlockParameterConverter.GetBlockParameter(blockHash);
Copy link
Member

Choose a reason for hiding this comment

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

This is the main change

@rubo
Copy link
Contributor Author

rubo commented Aug 10, 2022

@MarekM25 Tested with Lighthouse. Everything looks alright. I'm going to merge soon.

@rubo rubo merged commit 8c645cf into master Aug 10, 2022
@rubo rubo deleted the feature/eth_getlogs branch August 10, 2022 14:15
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.

eth_getLogs returns bogus results that are different every time
3 participants