-
Notifications
You must be signed in to change notification settings - Fork 166
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
refactor(katana): update execution error message #2322
Conversation
WalkthroughOhayo, sensei! The recent changes primarily focus on enhancing the error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System
participant ErrorHandler
User->>System: Initiates transaction
System->>ErrorHandler: Validates transaction
alt Invalid Nonce Detected
ErrorHandler->>User: Returns InvalidNonce error
ErrorHandler->>System: Logs detailed error info (address, current_nonce, tx_nonce)
end
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Additional comments not posted (11)
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2322 +/- ##
==========================================
- Coverage 67.31% 67.31% -0.01%
==========================================
Files 354 354
Lines 46433 46437 +4
==========================================
Hits 31258 31258
- Misses 15175 15179 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for the detailed reasons and update. 👍
Initially, i defined custom execution error messages for two main reasons:-
Debug
s formatting in the error messages returned by executor (ie blockifier)But reason no.1 is more intrusive as the errors are returned to user-space. Blockifier used to use
Debug
formatting in their error messages (they have since slowly removing theDebug
s but there are still some left). For example ref:and IMO this would output the error message string with debug info which results in not so appealing/readable message. This message is also used as a response data in the JSON-RPC server through the Starknet API. The Katana messages are more or less similar to its original counterpart but with slight variations in the wordings.
So the main idea of the redundancy was to provide user with more human readable and 'easier' to understand messages, but due to the string itself being different, if you wanna extract the individual atomic values (eg nonce, fee) from the string, you have to do it differently for katana vs other nodes (or gateway).
Important thing to note, however, the error message is not strictly defined in the JSON-RPC specs so Katana returning different error messages than blockifier is not technically wrong.
but @tarrencev needed them to be similar with the ones returned by Pathfinder, for some of Cartridge's internal operations. At least for these two errors:
Katana:
Pathfinder:
This is from older version of Katana, latest version outputs the same one as Pathfinder.
Katana:
Pathfinder:
Eventually, we'd probably remove this redundant error enum and use directly from blockifier. But for now, we just copy the exact string.
Summary by CodeRabbit
New Features
InvalidNonce
error, including specific fields for better debugging.Bug Fixes
InvalidNonce
variant in the transaction pre-validation error conversion, adding context for better error reporting.