feat(katana): include more data in logs on tx flow status #2657
Conversation
WalkthroughOhayo, sensei! This pull request introduces enhancements primarily focused on logging within the Changes
Possibly related PRs
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
crates/katana/executor/src/implementation/blockifier/mod.rs (2)
194-194: Excellent logging enhancement, sensei!The log message structure is well-formed with proper hex formatting. Consider extracting the format string
"{hash:#x}"into a constant if it's used in multiple places for consistency.
Line range hint
91-92: Track gas price handling TODOs, sensei!The TODOs raise important points about:
- Gas price enforcement
- L1 data gas price values
- Potential function alterations for block context
Would you like me to create GitHub issues to track these improvements?
Also applies to: 96-97, 100-103
crates/katana/pool/src/pool.rs (2)
167-167: Excellent addition of nonce context, sensei!The inclusion of both transaction and current nonce values provides clear visibility into dependency issues. Consider adding this information to a metrics system in the future for better monitoring of nonce-related issues.
Line range hint
140-180: Consider adopting structured logging for better observability, sensei!While the current logging improvements are excellent, consider adopting a more structured logging approach in the future. This could involve:
- Using consistent field names across log messages
- Adding common fields like
pool_size- Structuring error details in a machine-parseable format
This would enhance log aggregation and analysis capabilities.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/katana/executor/src/implementation/blockifier/mod.rs(3 hunks)crates/katana/pool/src/pool.rs(3 hunks)
🔇 Additional comments (4)
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
184-184: Ohayo! Clean variable extraction, sensei!
The hash extraction is well-placed and follows good practices for improving log context.
crates/katana/pool/src/pool.rs (3)
140-140: Ohayo! LGTM on the transaction received log enhancement, sensei!
Adding the "pool" target parameter improves log categorization and filtering capabilities.
160-160: Nice enhancement to invalid transaction logging, sensei!
The addition of error details alongside the transaction hash provides better context for debugging invalid transactions.
178-180: Well-structured error handling and logging, sensei!
The pattern matching ensures the hash is available, and the error context provides valuable debugging information for validation failures.
| @@ -201,7 +202,7 @@ impl<'a> BlockExecutor<'a> for StarknetVMProcessor<'a> { | |||
| } | |||
|
|
|||
| ExecutionResult::Failed { error } => { | |||
| info!(target: LOG_TARGET, %error, "Executing transaction."); | |||
| info!(target: LOG_TARGET, hash = format!("{hash:#x}"), %error, "Executing transaction."); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust the log message for failed transactions, sensei!
The message "Executing transaction" doesn't accurately reflect that this is a failure case. Consider updating it to something more descriptive like "Transaction execution failed".
- info!(target: LOG_TARGET, hash = format!("{hash:#x}"), %error, "Executing transaction.");
+ info!(target: LOG_TARGET, hash = format!("{hash:#x}"), %error, "Transaction execution failed.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| info!(target: LOG_TARGET, hash = format!("{hash:#x}"), %error, "Executing transaction."); | |
| info!(target: LOG_TARGET, hash = format!("{hash:#x}"), %error, "Transaction execution failed."); |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2657 +/- ##
==========================================
- Coverage 57.46% 57.45% -0.01%
==========================================
Files 400 400
Lines 49963 49954 -9
==========================================
- Hits 28709 28703 -6
+ Misses 21254 21251 -3 ☔ View full report in Codecov by Sentry. |
easier to understand why and which tx fails
Summary by CodeRabbit
New Features
Bug Fixes
Documentation