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

Extend fork_choice test format with on_payload_info #2965

Merged
merged 3 commits into from
Sep 12, 2022

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Aug 3, 2022

Extends fork_choice tests format with payload_info, discussion in ethereum/consensus-spec-tests#28

@mkalinin mkalinin requested a review from hwwhww August 3, 2022 12:20
@hwwhww hwwhww mentioned this pull request Aug 23, 2022
4 tasks
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Hey @mkalinin, I implemented a basic test in #2982

I am still on the fence between extending tests/formats/fork_choice/README.md or create a new tests/formats/sync/README.md that refers to the former. Thinking if the latter may be more clear in some ways. What do you think?

tests/formats/fork_choice/README.md Outdated Show resolved Hide resolved
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
@mkalinin
Copy link
Collaborator Author

Hey @mkalinin, I implemented a basic test in #2982

I am still on the fence between extending tests/formats/fork_choice/README.md or create a new tests/formats/sync/README.md that refers to the former. Thinking if the latter may be more clear in some ways. What do you think?

Unless there is any concern or obstacle, I would take a simple path forward which is extending the test by one more step, the step is optional and may not be supported by CL clients

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants