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

feat(core/types): Body RLP codec hooks #107

Closed
wants to merge 5 commits into from

Conversation

qdm12
Copy link
Collaborator

@qdm12 qdm12 commented Jan 21, 2025

Why this should be merged

How this works

How this was tested

Comment on lines +342 to +347
return &Body{
Transactions: b.transactions,
Uncles: b.uncles,
Withdrawals: b.withdrawals,
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note this change is compulsory since there are more fields present in the struct than these 3.

core/types/block.libevm.go Show resolved Hide resolved
core/types/block.libevm.go Show resolved Hide resolved
core/types/block.libevm_test.go Show resolved Hide resolved
libevm/legacy/legacy.go Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/core/types/body-hooks-rlp branch from edebe13 to e7193b7 Compare January 21, 2025 17:45
@qdm12 qdm12 force-pushed the qdm12/core/types/body-hooks-rlp branch from e7193b7 to 7f6afca Compare January 22, 2025 09:40
@qdm12 qdm12 changed the base branch from darioush/libevm-phase-2.5 to qdm12/core/types/header-copy-hooks January 22, 2025 09:41

// EncodeRLP implements the [rlp.Encoder] interface.
func (b *Body) EncodeRLP(w io.Writer) error {
return b.hooks().EncodeRLP(b, w)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Completely overriding RLP {en,de}coding is a blunt tool that we used for Header only because of the complexity of the modifications to that type (with the view to hopefully changing it in the future). I think it's possible to achieve this for Body without a hook, using just the rules of RLP optional, but I need to tinker a bit to confirm.

My greatest concern with a complete override is that the user still has to implement a lot of the upstream functionality, reducing the benefit of libevm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Progress on #109 suggests that this should be possible, with a hook that only needs to write fields to an rlp.EncoderBuffer. I'll confirm tomorrow.

@qdm12 qdm12 changed the title feat(core/types): Body RLP codec hooks to be closed - feat(core/types): Body RLP codec hooks Feb 3, 2025
@qdm12 qdm12 added the Status: 🔴 DO NOT MERGE This PR is not meant to be merged in its current state label Feb 3, 2025
@qdm12 qdm12 force-pushed the qdm12/core/types/header-copy-hooks branch 2 times, most recently from 18383c7 to def07f8 Compare February 4, 2025 15:04
Base automatically changed from qdm12/core/types/header-copy-hooks to main February 4, 2025 15:56
@qdm12 qdm12 changed the title to be closed - feat(core/types): Body RLP codec hooks feat(core/types): Body RLP codec hooks Feb 6, 2025
@qdm12
Copy link
Collaborator Author

qdm12 commented Feb 6, 2025

Closed in favor of #109

@qdm12 qdm12 closed this Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: 🔴 DO NOT MERGE This PR is not meant to be merged in its current state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants