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

engine: Rename DepositRequest to DepositReceipts #544

Closed
wants to merge 1 commit into from

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented May 6, 2024

Renames DepositRequest to DepositReceipt for the purpose of devnet-0.

The recent change introduced in #535 to align with the EIP-7685 requests created a slight discrepancy between the CL spec and implementations which still uses DepositReceipt and the Engine API server implementation on EL side. This renaming is a temporal quick fix to facilitate interop between CL and EL clients during planned devnet-0. The decision is to do a proper renaming after the devnet-0 launch and to incorporate it into further devnets.

@mkalinin mkalinin requested a review from lightclient May 6, 2024 15:01
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@lightclient
Copy link
Member

The decision is to do a proper renaming after the devnet-0 launch and to incorporate it into further devnets.

Why can't CL just rename now for devnet-0? Seems a little weird to switch this back to DepositReceipts just to again change to DepositRequests? Or is there a different idea for how it might be represented in the future?

@onbjerg
Copy link
Contributor

onbjerg commented May 7, 2024

I also think having every EL switch to from requests to receipts just to change it back to requests later again instead of having CLs rename the 1 field once seems a bit funky:)

@mkalinin
Copy link
Collaborator Author

mkalinin commented May 7, 2024

A proper renaming on CL side involves changing the name in the testing format and maybe other adjustments — this is gonna take some time. What suggested above is to marshal deposit_receipts as deposit_requests on the CL side, rather than doing it on the EL? It would be great to quickly resolve this question on the call this week but there is no call.

I am fine either way and I do see the argument in favour of CL making the change as doing and undoing on EL seems really weird.

@StefanBratanov
Copy link

For Teku we already use depositRequests for the EL Engine API and we map it to depositReceipts on CL, so now essentially we have to revert that change. I think to minimise changes on the EL side now and after devnet-0, it is easier to keep the spec as it is.

@mkalinin
Copy link
Collaborator Author

mkalinin commented May 8, 2024

Closing this PR in favour of: "CL marshals deposit_receipts as deposit_requests and after devnet-0 undergoes a full renaming”, the rough consensus is reached in the Discord

cc @ralexstokes

@mkalinin mkalinin closed this May 8, 2024
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.

5 participants