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

Initial implementation #1

Merged
merged 15 commits into from
Sep 27, 2023
Merged

Initial implementation #1

merged 15 commits into from
Sep 27, 2023

Conversation

ckoopmann
Copy link
Collaborator

@ckoopmann ckoopmann commented Sep 25, 2023

@ckoopmann ckoopmann marked this pull request as draft September 25, 2023 05:18
@ckoopmann ckoopmann marked this pull request as ready for review September 26, 2023 08:34
Copy link
Member

@alextes alextes left a comment

Choose a reason for hiding this comment

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

Amazing, looks good, lets merge and test it!

src/cli_ext.rs Outdated Show resolved Hide resolved
Comment on lines +7 to +12
/// Structure to deserialize execution payloads sent according to the builder api spec
/// Numeric fields deserialized as decimals (unlike crate::eth::engine::ExecutionPayload)
#[derive(Derivative)]
#[derivative(Debug)]
#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)]
#[allow(missing_docs)]
Copy link

Choose a reason for hiding this comment

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

Why do you need new types here? Seems like maintain headache, do we need to make the deser logic better?

Copy link
Collaborator Author

@ckoopmann ckoopmann Sep 27, 2023

Choose a reason for hiding this comment

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

This is really just because of difference in the formatting of the current api that we are using (snake_case and decimal encoded numbers).
I agree this is a headache and we should probably just use the existing formatting (camelCase and hex numbers), in which case we can drop these additional types and just use the existing ExecutionPayload struct.

@alextes What do you think ? (as is we will have to adjust the client code anyway because this api is differs from the existing one already)

Copy link
Collaborator Author

@ckoopmann ckoopmann Sep 27, 2023

Choose a reason for hiding this comment

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

If we do that I could probably also remove these re-exports from my pr against the reth repo. (if they are unwanted)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I opened an issue for this, and suggest doing this in a separate PR:
#2

@ckoopmann ckoopmann merged commit 8f1f00c into main Sep 27, 2023
@alextes alextes deleted the initial-implementation branch October 23, 2023 13:07
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