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

Invalid params data #5241

Closed
wants to merge 6 commits into from
Closed

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Mar 20, 2023

Added data to error in case of INVALID_PARAMS to give users more info as to what's gone wrong

See #4212 - possible solution to #5098

Not sure if this will break some hive tests?

@github-actions
Copy link

github-actions bot commented Mar 20, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.
  • I thought about the changelog and included a changelog update if required.

Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Do we need to do something similar for web sockets or graphQL? Maybe it'll work for those already, I just noticed only http tests were updated

@@ -41,8 +41,10 @@ public JsonRpcResponse process(
try {
return method.response(request);
} catch (final InvalidJsonRpcParameters e) {
JsonRpcError invalidParamsError = JsonRpcError.INVALID_PARAMS;
invalidParamsError.setData(e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't touch the Engine API does it?
I think error messages are part of the spec in some cases for that: https://github.com/ethereum/execution-apis/blob/main/src/engine/common.md#errors

@macfarla
Copy link
Contributor Author

macfarla commented May 5, 2023

On further inspection, calling setData() on an enum value seems like a dodgy hack so I think we need to refactor JsonRpcError to separate these two concepts.

Closing this PR, needs further thought. Created an issue #5437

@macfarla macfarla closed this May 5, 2023
@macfarla macfarla deleted the invalid-params-data branch September 13, 2023 06:10
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.

2 participants