feat(katana-rpc): add traceTransaction and traceBlockTransactions API
#2486
Conversation
WalkthroughOhayo, sensei! This pull request introduces enhancements primarily focused on transaction execution and built-in tracking in the Katana project. Key modifications include the addition of the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 6
🧹 Outside diff range and nitpick comments (4)
crates/katana/rpc/rpc/src/starknet/trace.rs (2)
67-69: Simplify variable naming to improve readabilityOhayo, sensei! The variables
should_validateandshould_skip_feeinvolve double negatives, which can be a bit confusing. Consider renaming them toskip_validateandskip_fee_charge, and adjusting the logic accordingly to enhance code clarity.Apply this diff to simplify the variable names and logic:
#[allow(deprecated)] -let should_validate = !(simulation_flags.contains(&SimulationFlag::SkipValidate) - || self.inner.backend.config.disable_validate); +let skip_validate = simulation_flags.contains(&SimulationFlag::SkipValidate) + || self.inner.backend.config.disable_validate; #[allow(deprecated)] -let should_skip_fee = !(simulation_flags.contains(&SimulationFlag::SkipFeeCharge) - || self.inner.backend.config.disable_fee); +let skip_fee_charge = simulation_flags.contains(&SimulationFlag::SkipFeeCharge) + || self.inner.backend.config.disable_fee; let flags = katana_executor::SimulationFlag { - skip_validate: !should_validate, - skip_fee_transfer: !should_skip_fee, + skip_validate, + skip_fee_transfer: skip_fee_charge, ..Default::default() };Also applies to: 73-75
207-208: Implement thestate_difffor complete transaction tracesOhayo, sensei! There's a TODO indicating the need to compute the
state_diff. Implementing this will enhance the completeness and usefulness of the transaction traces.Do you want me to help implement the
state_diffcomputation or open a GitHub issue to track this task?crates/katana/executor/src/implementation/blockifier/utils.rs (2)
129-129: Consider renamingr#type()method totx_type(), sensei.Using the raw identifier
r#type()can affect code readability. Renaming the method totx_type()avoids raw identifiers and enhances clarity.Apply this diff to rename the method call:
- let trace = to_exec_info(info, tx.r#type()); + let trace = to_exec_info(info, tx.tx_type());Ensure to update the method definition in
ExecutableTxWithHashaccordingly.
566-568: Consider renamingr#typetotx_typefor improved readability, sensei.Using
r#typeas an identifier requires raw identifiers due totypebeing a reserved keyword. Renaming it totx_typeavoids this and improves code clarity.Apply this diff to rename the identifier:
-pub fn to_exec_info(exec_info: TransactionExecutionInfo, r#type: TxType) -> TxExecInfo { +pub fn to_exec_info(exec_info: TransactionExecutionInfo, tx_type: TxType) -> TxExecInfo { TxExecInfo { - r#type, + tx_type,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- crates/katana/executor/src/implementation/blockifier/utils.rs (3 hunks)
- crates/katana/primitives/src/trace.rs (3 hunks)
- crates/katana/primitives/src/transaction.rs (2 hunks)
- crates/katana/rpc/rpc/src/starknet/trace.rs (2 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (2 hunks)
🔇 Additional comments (5)
crates/katana/primitives/src/transaction.rs (1)
20-41: Sensei, theTxTypeenum definition is correct and well-documented.crates/katana/rpc/rpc/tests/starknet.rs (2)
29-29: AddingTransactionTraceto imports is appropriateThe addition of
TransactionTraceto the imports is necessary for the new tracing functionalities implemented in the test functions.
761-779: Ohayo, sensei! Thetracetest function is well-implementedThe
tracetest correctly verifies thetrace_transactionfunctionality by sending transactions and asserting the correct trace type. The implementation follows best practices and is clear.crates/katana/executor/src/implementation/blockifier/utils.rs (2)
52-52: Ohayo, sensei! IncludingTxTypein the imports is appropriate.This addition allows the proper handling of transaction types in the updated functions.
566-566: Verify all calls toto_exec_infoare updated with the new parameter, sensei.Since
to_exec_infonow requires an additionalTxTypeparameter, please ensure all invocations are updated to prevent compilation errors.Run the following script to find any outdated calls:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2486 +/- ##
==========================================
+ Coverage 68.62% 68.97% +0.34%
==========================================
Files 372 372
Lines 48441 48523 +82
==========================================
+ Hits 33244 33469 +225
+ Misses 15197 15054 -143 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
examples/rpc/starknet/starknet_trace.hurl (2)
7-8: Ohayo, sensei! The transaction hash has been simplified.The change to a shorter transaction hash ("0x1337") makes the test case more readable. However, consider using a more realistic transaction hash to better represent actual use cases.
Perhaps use a hash like "0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef" to maintain realism while still being easily recognizable in tests.
33-46: Ohayo, sensei! A new test case for non-existent blocks has been added.The addition of this test case for
starknet_traceBlockTransactionswith a non-existent block (677) improves the test coverage by handling the "Block not found" scenario. This is a valuable addition to ensure proper error handling in the API.Consider documenting the custom error codes (e.g., 29 for "Transaction hash not found" and 24 for "Block not found") in a central location or adding comments to explain their significance. This will help maintain consistency and clarity across the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- examples/rpc/starknet/starknet_getTransactionStatus.hurl (1 hunks)
- examples/rpc/starknet/starknet_trace.hurl (2 hunks)
🔇 Additional comments (3)
examples/rpc/starknet/starknet_getTransactionStatus.hurl (1)
15-15: Ohayo, sensei! Great addition to the test case!The new assertion for the error code enhances the test coverage for the
starknet_getTransactionStatusmethod. By verifying both the error message and the error code, we ensure a more robust validation of the API's error handling. This change aligns well with best practices in API testing.examples/rpc/starknet/starknet_trace.hurl (2)
13-14: Ohayo, sensei! The error handling has been refined.The change from a generic "Unsupported method" error to a specific "Transaction hash not found" error with a custom code (29) improves the API's error reporting. This enhancement will help users better understand and debug issues related to transaction tracing.
23-31: Ohayo, sensei! The block transaction tracing test has been enhanced.The changes improve the test in two ways:
- Using a specific block number (0) instead of "latest" makes the test more deterministic and reproducible.
- The new assertions provide a more comprehensive check of the API response, ensuring that the result exists but is empty for block 0.
These improvements enhance the reliability and thoroughness of the test case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
crates/katana/rpc/rpc/src/starknet/trace.rs (3)
24-112: Ohayo, sensei! LGTM with a small suggestion.The
simulate_txsmethod looks well-implemented and handles different transaction types correctly. It also properly considers simulation flags and node configuration for validation and fee charging.Consider adding a comment explaining the purpose of the
#[allow(deprecated)]attributes on lines 67 and 73. This will help future maintainers understand why these attributes are necessary.
114-163: Ohayo, sensei! LGTM with a small suggestion.The
block_tracesmethod is well-implemented and handles different block ID types correctly. It also properly handles the pending block case.On line 149, there's a TODO comment about simplifying the query. Consider creating a GitHub issue to track this improvement and link it in the comment. This will help ensure the simplification is not forgotten.
214-306: Ohayo, sensei! LGTM with a small suggestion.The conversion functions
to_rpc_trace,to_rpc_resources, andto_rpc_fee_estimateare well-implemented and handle all necessary conversions correctly.On line 220, there's a TODO comment about computing the state diff. Consider creating a GitHub issue to track this improvement and link it in the comment. This will help ensure the state diff computation is not forgotten.
crates/katana/rpc/rpc/tests/starknet.rs (2)
760-810: Ohayo, sensei! Thetracefunction looks solid, but let's add some spice!The implementation aligns well with the PR objective of implementing
starknet_traceTransaction. It covers both mined and pending transactions, which is great. However, I have a few suggestions to make it even better:
- Consider using a constant for the number of transactions in each test case. This makes the test more maintainable.
- Add more detailed assertions for the trace content, not just the type. This will ensure the trace data is correct, not just present.
Here's a suggested improvement:
+ const TRANSACTIONS_PER_BLOCK: usize = 2; - for _ in 0..2 { + for _ in 0..TRANSACTIONS_PER_BLOCK { let res = contract.transfer(&recipient, &amount).send().await?; dojo_utils::TransactionWaiter::new(res.transaction_hash, &provider).await?; hashes.push(res.transaction_hash); } // ... (similar change for the pending block section) for hash in hashes { let trace = provider.trace_transaction(hash).await?; assert_matches!(trace, TransactionTrace::Invoke(invoke_trace) => { + assert_eq!(invoke_trace.function_invocation.entry_point_type, Some(EntryPointType::External)); + assert_eq!(invoke_trace.function_invocation.calldata, vec![recipient.into(), amount.low, amount.high]); }); }These changes will make the test more robust and easier to maintain in the future.
812-903: Ohayo again, sensei! Theblock_tracesfunction is looking good, but let's make it even better!The implementation aligns well with the PR objective of implementing
starknet_traceBlockTransactions. It covers multiple scenarios, including mined and pending blocks. However, I have a few suggestions to enhance it further:
- Use constants for the number of transactions in each block to improve maintainability.
- Add more detailed assertions for the trace content, not just the transaction hash and type.
- Consider adding a test case for an empty block to ensure it's handled correctly.
Here's a suggested improvement:
+ const BLOCK_1_TRANSACTIONS: usize = 5; + const BLOCK_2_TRANSACTIONS: usize = 2; + const PENDING_BLOCK_TRANSACTIONS: usize = 3; // ... (in Block 1 section) - for _ in 0..5 { + for _ in 0..BLOCK_1_TRANSACTIONS { // ... (in Block 2 section) - for _ in 0..2 { + for _ in 0..BLOCK_2_TRANSACTIONS { // ... (in Block 3 (Pending) section) - for _ in 0..3 { + for _ in 0..PENDING_BLOCK_TRANSACTIONS { // ... (in assertion sections) assert_matches!(&traces[i].trace_root, TransactionTrace::Invoke(invoke_trace) => { + assert_eq!(invoke_trace.function_invocation.entry_point_type, Some(EntryPointType::External)); + assert_eq!(invoke_trace.function_invocation.calldata, vec![recipient.into(), amount.low, amount.high]); }); + // Test case for an empty block + rpc_client.generate_block().await?; + let empty_block_id = BlockId::Number(3); + let empty_block_traces = provider.trace_block_transactions(empty_block_id).await?; + assert_eq!(empty_block_traces.len(), 0, "Empty block should have no traces");These changes will make the test more robust, easier to maintain, and cover more edge cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- crates/katana/rpc/rpc/src/starknet/trace.rs (2 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (2 hunks)
🔇 Additional comments (5)
crates/katana/rpc/rpc/src/starknet/trace.rs (4)
165-183: Ohayo, sensei! LGTM!The
tracemethod is well-implemented. It correctly prioritizes checking the pending block before falling back to the provider, and the error handling is appropriate.
188-190: Ohayo, sensei! LGTM!The
trace_transactionmethod is correctly implemented as an async wrapper around thetracemethod, usingon_io_blocking_taskfor execution.
Line range hint
192-202: Ohayo, sensei! LGTM!The
simulate_transactionsmethod is correctly implemented as an async wrapper around thesimulate_txsmethod, usingon_cpu_blocking_taskfor execution.
204-209: Ohayo, sensei! LGTM!The
trace_block_transactionsmethod is correctly implemented as an async wrapper around theblock_tracesmethod, usingon_io_blocking_taskfor execution.crates/katana/rpc/rpc/tests/starknet.rs (1)
Line range hint
1-903: Ohayo, sensei! Overall, this implementation is looking sharp!The new test functions
traceandblock_tracesprovide comprehensive coverage for the implementedstarknet_traceTransactionandstarknet_traceBlockTransactionsAPI methods. They align well with the PR objectives and cover various scenarios, including mined and pending transactions/blocks.The suggested improvements, while not critical, would enhance the maintainability and robustness of the tests. Consider implementing these changes to make your tests even more awesome!
Great job on implementing these new features and their corresponding tests!
resolves #2458
this PR implements the last two methods of the Starknet trace APIl
starknet_traceTransactionandstarknet_traceBlockTransactions.to simplify the conversion from the internal type,
TxExecInfo, to its RPC counterpart,TransactionTrace, theTxExecInfois now self-contained with the introduction of a new field of typeTxTypeenum. otherwise, we would need to query either the receipt or the transaction itself to determine the transaction type of the execution info.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
These updates enhance the overall functionality and user experience within the Starknet ecosystem.