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: standardized error responses for payload validation #7939

Merged

Conversation

merklefruit
Copy link
Contributor

Fixes #7901

Changes

  • Added more descriptive payload validation error responses in engine API

Types of changes

What types of changes does your code introduce?

  • Optimization

Testing

Requires testing

  • Yes
  • No

Documentation

Requires documentation update

  • Yes
  • No

Remarks

More context on this change can be found in #7901

@merklefruit
Copy link
Contributor Author

Hi @MarekM25, can I help add context for a review? Would love to see this merged if possible

@LukaszRozmej
Copy link
Member

@kamilchodola It would be good to run engine and pyspec hive tests on this PR. But I cannot seem to run them on outside branches from actions.

@LukaszRozmej LukaszRozmej requested a review from flcl42 January 6, 2025 09:24
@flcl42 flcl42 requested review from rubo and a team as code owners January 6, 2025 11:07
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

LGTM, but Hive tests will have to be updated to handle new error codes

@merklefruit
Copy link
Contributor Author

@MarekM25 I can do that, can you provide a quick pointer to where i should look?

@MarekM25
Copy link
Contributor

MarekM25 commented Jan 7, 2025

Quote reply

I think you will have to search for either our old errors or geth errors in this repo and provide a PR: https://github.com/ethereum/hive here

I can contact you with maintainers if needed. Thx for your contribution! 🙏

@merklefruit
Copy link
Contributor Author

merklefruit commented Jan 9, 2025

@MarekM25 hi, I've been able to run the ethereum/engine hive test suite for both this branch and nethermind's master.
Here are the results:

from this run, it seems like this specific branch is not causing any new errors, but as I previously mentioned I'm not familiar enough with Hive to be 100% sure that I didn't miss something.

I have also tried to look for the hardcoded error messages in the hive codebase but haven't found them: the behaviour there seems to be to only check the "Status: Invalid" property, and not the actual validation error message

edit: I also ran the ethereum/eest suite on this branch, results are available here: http://remotebeast:8083/

@merklefruit merklefruit requested a review from MarekM25 January 10, 2025 08:41
@MarekM25 MarekM25 merged commit 6364cdb into NethermindEth:master Jan 14, 2025
79 checks passed
@benaadams
Copy link
Member

@merklefruit thank you for your contribution

mralj pushed a commit that referenced this pull request Feb 5, 2025
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.

feat: add standardized error messages in engine API responses
6 participants