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 API: send payload validation error to CL #120

Closed
mkalinin opened this issue Nov 9, 2021 · 2 comments · Fixed by #135
Closed

Engine API: send payload validation error to CL #120

mkalinin opened this issue Nov 9, 2021 · 2 comments · Fixed by #135

Comments

@mkalinin
Copy link
Collaborator

mkalinin commented Nov 9, 2021

Currently there is a message: STRING|null field in the response object of the engine_executePayload method. This field is supposed to be used in the case when the payload is invalid to provide additional information on what exactly has failed. This should improve CL client logs readability as it will give an idea of what went wrong during the execution without the strong need of looking into EL client logs.

@MariusVanDerWijden pointed out that the message field is confusing as it doesn't correspond to the regular JSON-RPC error format. There is a couple of options:

  1. Replace message with validationError: null|{code: errorCode, message: errorMessage} field
  2. Rename message to validationError and doesn't render the code

IMO, the first option does make sense if there is a list of messages and codes corresponding to errors occurred during validation and execution of a payload. If there is no such list it probably doesn't worth spending a time on standardising these errors and then adjusting clients to adhere to this standard, the option 2 could work good as long as there is a meaningful message put in the validationError field, event though the messages vary from one EL client to another.

@MarekM25
Copy link

MarekM25 commented Nov 9, 2021

Not a strong opinion, but I prefer option number one.
If a consensus client has to react in a standard way to a given error type in the future, it will be better to have error codes.

@mkalinin
Copy link
Collaborator Author

Not a strong opinion, but I prefer option number one.
If a consensus client has to react in a standard way to a given error type in the future, it will be better to have error codes.

Ended up with option number two. Because if we'd want a standard reaction of CL client it'd require standardising error codes which sounds overcomplicated in the context of UX improvement that was the original intention behind bubbling this error message.

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 a pull request may close this issue.

2 participants