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

Added engine_getPayloadV4 and engine_newPayloadV4 for Prague (EIP-7002) #528

Merged
merged 6 commits into from
Mar 19, 2024

Conversation

lucassaldanha
Copy link
Contributor

EIP-7002 has been confirmed for Prague and it requires changes in the ExecutionPayload leading us to ExecutionPayloadV4. This also impacts two Engine API methods, leading us to engine_getPayloadV4 and engine_newPayloadV4.

Please note that Prague will also have EIP-6110 that also introduce new fields to ExeuctionPayload. I noticed that @mkalinin has already an experimental spec for the changes here.

I am not sure what is the best way to consolidate those changes. We can either merge the changes related to EIP-7002 first, and later consolidate the changes from eip6110.md, or I can integrate eip6110.md changes into this PR. Let me know what is the best approach.

This is my first PR on this repo so I apologize if I missed anything in the process.

PS.: due to the size of the blobs field in the examples, we can't see the diff for src/engine/openrpc/methods/payload.yaml online.

Copy link
Collaborator

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

Thanks @lucassaldanha 🚀

My suggestion would be to merge this PR, and then EIP-6110 spec can be added on top of it. cc @ensi321

Copy link
Member

@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.

PR looks good to me, but the blobs in the openrpc sample are very big. Is it possible to omit them or shorten them significantly? Maybe try a single blob instead of 3? I'm worried because just viewing the file on github lags the page so I assume the openrpc generated page will lag.

@lucassaldanha
Copy link
Contributor Author

Maybe try a single blob instead of 3?

Done. I have updated the examples with a single blob in each method.

@lucassaldanha
Copy link
Contributor Author

Is it possible to omit them or shorten them significantly?

This is something to consider as well. But it might make the example an invalid object. I guess this depends on what we expect from the example (do we want it to be a valid object).

@mkalinin
Copy link
Collaborator

OpenRPC page wasn’t lagging with 3 blobs, tho we will have more of them every time a new version of getPayload method is created so reducing the example to a single blob makes sense to me. Not having a diff on GH is annoying too but I think we can deal with this complexity for the next few times, there should not be many changes in the future as Ethereum is ossifying 🔜 (don’t quote me on that).

@lucassaldanha
Copy link
Contributor Author

I agree that we might need to look into a better way of handling the blobs as we add more and more versions. But I think this should be handled in a separate PR.

I took a quick look into how OpenRPC page loads the example data and it is smart enough not to push the whole content to the blobs to the DOM (it only adds 10k characters for each). And you can only copy the full content to clipboard. So even though the browser still needs to download the whole content, it prevents the page to getting too slow.

Reducing the number of blobs in each example to 1 per list results in a smaller footprint then before so at least we are not making things worse :)

@lightclient
Copy link
Member

Sorry I didn't realize we already had the blobs in the cancun example. It would be great to improve this eventually, it's a lot of data to throw into text areas. But I don't see it as blocking now.

@mkalinin mkalinin merged commit 199894b into ethereum:main Mar 19, 2024
3 checks passed
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.

3 participants