Skip to content

rpc: implement header and header_by_hash queries#7270

Merged
cmwaters merged 13 commits intomasterfrom
fedekunze/rpc-client-header
Dec 2, 2021
Merged

rpc: implement header and header_by_hash queries#7270
cmwaters merged 13 commits intomasterfrom
fedekunze/rpc-client-header

Conversation

@fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Nov 10, 2021

Introduces header and header_by_hash queries to the RPC client. This is useful for Evmos JSON-RPC to avoid querying the block endpoint for the eth_header and eth_header_by_hash JSON-RPC endpoints

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Changes look reasonable to me. I think there are some unnecessary/redundant comments in the handler implementations.

Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

its fine to add this but note this isn't required on the Tendermint side. This doesn't provide much benefit since applications like evmos are wrapping the rpc and since the IO is the same for block and header there isn't much saving here.

Wrapping local client on the app side and exposing a header method on the evm rpc would suffice.

Do we think this will be used by others as well?

@tychoish
Copy link
Contributor

The implementation seems clear, and I think there isn't a real reason not to do this: I think it improves the ergonomics, and it could reduce the amount of data that you end up sending across the wire.

While I think we should in general avoid expanding the surface of the RPC, I don't think saying "yes" to this makes it harder to say "no" to other methods in the future.

@creachadair
Copy link

Yeah. I don't love expanding the RPC, but I agree with @tychoish's analysis.

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

As the others have already alluded to, we came to the conclusion in our meeting that these changes to the RPC API make sense. A few changes need to be made before merging. We also need to add an entry to internal/rpc/core/routes.go and light/proxy/routes.go. Lastly we'll need to make an update to the spec but I can go do that once this has merged.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Nov 28, 2021
@cmwaters cmwaters removed the stale for use by stalebot label Nov 30, 2021
@cmwaters cmwaters self-requested a review November 30, 2021 22:57
@cmwaters cmwaters merged commit 5f57d84 into master Dec 2, 2021
@cmwaters cmwaters deleted the fedekunze/rpc-client-header branch December 2, 2021 10:16
mergify bot pushed a commit that referenced this pull request Dec 2, 2021
(cherry picked from commit 5f57d84)

# Conflicts:
#	CHANGELOG_PENDING.md
#	rpc/client/mocks/client.go
#	rpc/client/rpc_test.go
lklimek referenced this pull request in dashpay/tenderdash Mar 28, 2022
@cmwaters cmwaters mentioned this pull request Aug 1, 2022
39 tasks
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.

6 participants