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

Standardise the Engine JSON-RPC error codes #8695

Merged

Conversation

Malaydewangan09
Copy link
Contributor

@Malaydewangan09 Malaydewangan09 commented Oct 9, 2024

PR Description

This pull request standardises the handling of JSON-RPC error codes in the Web3JClient::doRequest method according to the JSON-RPC 2.0 specification. It introduces specific error code handling for well-known JSON-RPC errors and customizes error messages to reduce unnecessary stack trace logging.

Fixed Issue(s)

fixes #8671

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

Thanks! Looks nice so far.
I have some comments and I think we should add some tests in Web3JClientTest

@Malaydewangan09
Copy link
Contributor Author

Hey @tbenr
I've refactored the JsonRpcErrorCodes class to use an enum for improved structure and efficiency.
However, I'm looking for help in writing or updating the corresponding test cases for this change.

@tbenr
Copy link
Contributor

tbenr commented Oct 11, 2024

Hey @Malaydewangan09 I don't see the update on the Enum front. Can you double check?
Thanks!

@Malaydewangan09
Copy link
Contributor Author

Hey @tbenr, my bad.
I've updated the class to an Enum.
Can you please check the changes?

@tbenr
Copy link
Contributor

tbenr commented Oct 11, 2024

WRT testing you could:

take inspiration from shouldNotUpdateAvailabilityWhenNonCriticalMethodFailsWithErrorResponse on how to simulate an error and from shouldTimeoutIfResponseNotReceived on how to get the response and check that the mapping has been applied. I'd add one test covering the fact that the errors are decoded via JsonRpcErrorCodes as expected.

@Malaydewangan09 Malaydewangan09 force-pushed the standardise-json-rpc-error-codes branch from 7e468d5 to dc68ccc Compare October 13, 2024 18:45
@tbenr
Copy link
Contributor

tbenr commented Oct 14, 2024

I think there is still something to improve:

image

maybe the final error should be something like JSON-RPC error: Invalid params (-32602) ?

@Malaydewangan09
Copy link
Contributor Author

Ah yes, that would be a more concise error message.
I'll update the message.

@tbenr
Copy link
Contributor

tbenr commented Oct 15, 2024

sorry I got confused by repetition of Invalid params. The first is the decoded error description, the second is the message coming from the server. I'd change the message to be something different in the test.

@Malaydewangan09
Copy link
Contributor Author

@tbenr
I'd like to clarify the purpose of your comment about the repetition of 'Invalid params'. Were you suggesting we make the test case more realistic by using a distinct server error message, or was there another reason for pointing out this repetition?

@tbenr
Copy link
Contributor

tbenr commented Oct 16, 2024

@tbenr I'd like to clarify the purpose of your comment about the repetition of 'Invalid params'. Were you suggesting we make the test case more realistic by using a distinct server error message, or was there another reason for pointing out this repetition?

@Malaydewangan09 Yes, I was suggesting to make the test more realistic and thus less confusing. The message from the server could be something more verbose like "engine_newPaylad method has been called with invalid parameters"

Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@tbenr tbenr enabled auto-merge (squash) October 17, 2024 07:45
@tbenr tbenr merged commit 06d4078 into Consensys:master Oct 17, 2024
17 checks passed
@Malaydewangan09
Copy link
Contributor Author

@tbenr Thanks for your consistent feedback!

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.

Standardise the Engine Json RPC error codes
3 participants