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_getBlockByHash/Number to engine API #27

Merged
merged 4 commits into from May 13, 2022
Merged

Add eth_getBlockByHash/Number to engine API #27

merged 4 commits into from May 13, 2022

Conversation

Ruteri
Copy link
Contributor

@Ruteri Ruteri commented May 11, 2022

Registers and enables eth API namespace with EngineBackend.

For now only implements eth_getBlockByHash and eth_getBlockByNumber in limited fashion (no full transactions and no pending blocks).

engine.go Outdated Show resolved Hide resolved
@Ruteri Ruteri requested a review from protolambda May 12, 2022 07:57
@metachris
Copy link
Contributor

Related: #24

Copy link
Owner

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Good enough for testing, but would clean up the code a little bit maybe

engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
engine.go Outdated
if err != nil {
return nil, err
}
if inclTx {
Copy link
Owner

Choose a reason for hiding this comment

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

The parameter name is confusing (inclTx but really includes Td instead), and it's always set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. From geth's ethapi I'd avoid deviating since it'll be even more confusing!

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe could link to the upstream code in the function docstring 🤔

@metachris
Copy link
Contributor

@protolambda could you enable and run the CI workflow for this PR? an admin needs to do that for PRs of first-time contributors.

engine.go Outdated
@@ -267,3 +274,63 @@ func (e *EngineBackend) ForkchoiceUpdatedV1(ctx context.Context, heads *types.Fo

return &types.ForkchoiceUpdatedResult{PayloadStatus: types.PayloadStatusV1{Status: types.ExecutionValid, LatestValidHash: &heads.HeadBlockHash}, PayloadID: &id}, nil
}

Choose a reason for hiding this comment

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

Super minor suggestion but I think the code will be more maintainable longer-term if the EthBackend was moved to eth.go and the new RPCMarshal functions moved to types/rpc_encoding.go

Copy link
Collaborator

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

LGTM - annoying that marhsaling code is stuck behind internal/ in geth. Oh well.

@lightclient lightclient merged commit c1c66c2 into protolambda:master May 13, 2022
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

5 participants