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 eth_getBlockReceipts #438

Merged
merged 4 commits into from
Aug 15, 2023
Merged

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Jul 11, 2023

implement #393

@jsvisa jsvisa marked this pull request as draft July 11, 2023 15:02
src/eth/block.yaml Outdated Show resolved Hide resolved
@lightclient
Copy link
Member

@jsvisa this looks pretty good to me, is there a reason to not mark read for review now?

@lightclient
Copy link
Member

(noticed small error in result formatting, should pass ci now)

@jsvisa
Copy link
Contributor Author

jsvisa commented Jul 12, 2023

@lightclient I have to add the tests in this PR as well(need a PR in rpctestgen first)

@lightclient
Copy link
Member

My recommendation would be to undraft this with the spec so we can have client teams start approving the spec itself and in parallel work on adding the tests. But ultimately up to you.

@jsvisa jsvisa marked this pull request as ready for review July 12, 2023 16:22
@jsvisa
Copy link
Contributor Author

jsvisa commented Jul 12, 2023

thanks, let's move forward.

@ryanschneider
Copy link

Is the intent that this RPC can target non-canonical blocks? eth_getTransactionReceipt only returns txs included in canonical blocks (at least in geth) which is sometimes a problem. IMO it'd be nice if this RPC can target non-canonical blocks (e.g. a block that recently reorg'd out) but I'm not sure if concepts like that are included in the OpenRPC spec or not.

@jsvisa
Copy link
Contributor Author

jsvisa commented Jul 14, 2023

if this RPC can target non-canonical blocks

I think you can query this RPC(eth_getBlockReceipts) via block hash(the default behavior is requireCanonical: false)

Copy link

@siladu siladu 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. Happy to implement in Besu once this is merged.

@lightclient
Copy link
Member

Approved in ACD 166. Just need to get the tests in now.

Signed-off-by: jsvisa <delweng@gmail.com>

Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
@jsvisa
Copy link
Contributor Author

jsvisa commented Aug 10, 2023

@lightclient sorry for the late reply, now integrated the tests, which are picked from rpctestgen

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.

None yet

4 participants