Skip to content

feat: added error codes for supervisor RPC errors #661

Merged
sebastianst merged 15 commits intoethereum-optimism:mainfrom
07Vaishnavi-Singh:feat/error_codes
Apr 24, 2025
Merged

feat: added error codes for supervisor RPC errors #661
sebastianst merged 15 commits intoethereum-optimism:mainfrom
07Vaishnavi-Singh:feat/error_codes

Conversation

@07Vaishnavi-Singh
Copy link
Contributor

Closes - #660

Changes

  • Added an "Errors" section to the Supervisor spec document with two subsections:
    • Standard JSON-RPC Error Codes (-32700 to -32006)
    • Protocol-Specific Error Codes (-32100 to -32119)
  • Mapped Go error types from the codebase to specific error codes
  • Used consistent error code grouping with shared prefixes
  • Followed standard JSON-RPC error formatting and descriptions

@07Vaishnavi-Singh 07Vaishnavi-Singh requested review from a team as code owners April 10, 2025 22:16
@07Vaishnavi-Singh
Copy link
Contributor Author

hey @emhane, can you provide a review....will fix it according to the suggestions. ty!

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

great start! first pass, will give this another pass in a bit. I like that you make the distinction between e.g. unknown chain and errors more specifically related to data availability by using the 3210 and 3211 prefixes respectively

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

thought again about how to do the codes, let's do this:

the 6 digit error code is made up as follows

  • first 2 digits reserved for 32XXXX meaning server error
  • following digit pair reserved for gRPC error category (1-16)
    e.g. 320600 refers to the first server error of category ALREADY_EXISTS = 6.
    we use 06 so we can make the distinction between status code CANCELLED = 1, 3201XX, and ABORTED = 10, 3210XX.
  • last digit pair is reserved for indexing errors of same gRPC code on the same server. so, 320601 is the second error of category ALREADY_EXISTS on the server.

@07Vaishnavi-Singh
Copy link
Contributor Author

Hey @emhane, I have adjusted the error codes according to the 6-digit standard, and also made the small refactoring nits asked. lmk if something still needs to be done. ty!

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

super nice! some nitpicks remaining

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

awesome. thanks a lot !

@07Vaishnavi-Singh
Copy link
Contributor Author

07Vaishnavi-Singh commented Apr 11, 2025

hey @emhane , can you provide some TG would be helpful while implementing these error codes in reth

@emhane
Copy link
Member

emhane commented Apr 14, 2025

hey @emhane , can you provide some TG would be helpful while implementing these error codes in reth

sure, when this pr is merged is a good time to start impl in reth

@tynes
Copy link
Contributor

tynes commented Apr 14, 2025

flagging @alfonso-op on this. we will want to make sure that the error codes make sense for proxyd and that the user story of a transaction being sent that includes an invalid access list is dropped returns an error code that is well defined and included in common libraries like viem

@emhane
Copy link
Member

emhane commented Apr 16, 2025

apologies for the slow review @07Vaishnavi-Singh !

@07Vaishnavi-Singh
Copy link
Contributor Author

hey @tynes , if @alfonso-op is occupied at the moment I can handle the issue further. Can you please just tell me what needs to be done and would be great if you can share some resources for the same. Will wrap it up as soon as possible and and start working on paradigmxyz/reth#15671

@alfonso-op
Copy link
Contributor

Apologies for not replying to this directly last week. @jelias2 and @yashvardhan-kukreja are implementing the changes to proxyd to integrate with supervisor and will share any feedback.

@jelias2
Copy link

jelias2 commented Apr 22, 2025

overall lgtm, needs a spec reviewer

@yashvardhan-kukreja
Copy link
Contributor

Thank you @07Vaishnavi-Singh, this looks neat and useful!

Looking forwards to more such contributions :)

@emhane emhane requested a review from tynes April 23, 2025 10:14
@emhane emhane added C-good-first-issue Good for newcomers H-interop Hardfork: change planned for Interop upgrade labels Apr 24, 2025
@emhane emhane added A-supervisor Area: supervisor A-rpc Area: RPC API labels Apr 24, 2025
@sebastianst sebastianst merged commit 41a2ea8 into ethereum-optimism:main Apr 24, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from In Review to Done in Project Tracking Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-rpc Area: RPC API A-supervisor Area: supervisor C-good-first-issue Good for newcomers H-interop Hardfork: change planned for Interop upgrade

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants