feat(katana): fetch forked block data - #2592
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant enhancements across several files in the Katana project, focusing on the integration of a Changes
Assessment against linked issues
Possibly related PRs
Feel free to reach out if you need further insights, sensei! 🪧 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: 11
🧹 Outside diff range and nitpick comments (9)
crates/katana/rpc/rpc/src/starknet/trace.rs (2)
99-99: Ohayo sensei! The type conversion looks good.The change from
itoi as u64fortransaction_indexaligns well with Starknet's type system and ensures consistency across the codebase. The conversion is safe since the index comes from a Vec iterator and will always be non-negative.For future consideration, you might want to add a debug assertion to catch any theoretical edge cases:
debug_assert!(i <= u64::MAX as usize, "transaction index exceeds u64::MAX");
Line range hint
1-99: Consider adding fork-related documentation.Ohayo sensei! Since this PR is part of the forked provider implementation, it would be helpful to add documentation comments explaining how the trace functionality behaves when working with forked data, particularly for methods like
block_tracesandtracethat fetch data through the provider abstraction.Example documentation:
/// Returns traces for transactions in the specified block. /// When using a forked provider, traces are fetched from the fork network /// for blocks that exist before the fork point. fn block_traces(...)crates/katana/rpc/rpc-types/src/block.rs (2)
327-338: Consider adding documentation for the conversion implementations, sensei!While the implementation is correct, adding documentation comments would help explain the purpose of these conversions in the context of forked block data fetching. Consider adding a doc comment like:
/// Converts from Starknet core's MaybePendingBlockWithReceipts to the local type. /// This conversion is used when fetching block data from the fork network. impl From<starknet::core::types::MaybePendingBlockWithReceipts> for MaybePendingBlockWithReceipts {
Line range hint
98-338: Well-structured type conversion system for forked block data!The three
Fromimplementations form a complete and consistent set of conversions necessary for fetching different types of block data from the fork network. The modular approach with separate implementations for transactions, transaction hashes, and receipts aligns well with Rust's type system and makes the code maintainable.crates/katana/core/src/backend/storage.rs (1)
177-178: Consider implementing the TODO suggestion.The TODO comment raises a valid point about using block numbers for range validation. This would improve the robustness of the forking mechanism.
Would you like me to create a GitHub issue to track this improvement? I can help draft the implementation as well.
crates/katana/rpc/rpc-types/src/transaction.rs (1)
400-404: Ohayo sensei! Consider adding documentation for the From implementation.The implementation looks correct and aligns with the PR objectives for fetching forked block data. However, it would be helpful to add documentation explaining:
- The purpose of this conversion
- How it differs from the
From<TxWithHash>implementation above- When to use this implementation vs the other one
Add documentation like this:
+/// Implements direct conversion from starknet core Transaction type. +/// This implementation is primarily used when fetching transactions from the fork network, +/// where we already have the complete transaction data and don't need the complex +/// conversion logic used in `From<TxWithHash>`. impl From<starknet::core::types::Transaction> for Tx { fn from(value: starknet::core::types::Transaction) -> Self { Self(value) } }crates/katana/node/src/lib.rs (1)
187-187: Ohayo, sensei! Correct the typo in the comment.There's a typo in the comment: "it'd bee nice" should be "it'd be nice".
Apply this diff to fix the typo:
- // TODO: it'd bee nice if the client can be shared on both the rpc and forked backend side + // TODO: it'd be nice if the client can be shared on both the rpc and forked backend sidecrates/katana/rpc/rpc/src/starknet/mod.rs (2)
620-621: Address the TODO regarding transaction status handlingOhayo, sensei!
The TODO comment indicates potential issues when querying for failed transactions from the provider. Implementing proper handling for failed transactions will improve the accuracy of the
transaction_statusmethod.Would you like assistance in implementing this functionality, or should I open a new GitHub issue to track this task?
85-93: Unify initialization methods forStarknetApiOhayo, sensei!
The
new_forkedmethod closely mirrors the existingnewmethod but adds aforked_clientparameter. Consider unifying these methods or using default parameters to streamline object creation and reduce redundancy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- crates/katana/core/src/backend/storage.rs (5 hunks)
- crates/katana/node/src/lib.rs (6 hunks)
- crates/katana/rpc/rpc-api/src/starknet.rs (2 hunks)
- crates/katana/rpc/rpc-types/src/block.rs (3 hunks)
- crates/katana/rpc/rpc-types/src/error/starknet.rs (3 hunks)
- crates/katana/rpc/rpc-types/src/receipt.rs (1 hunks)
- crates/katana/rpc/rpc-types/src/state_update.rs (4 hunks)
- crates/katana/rpc/rpc-types/src/transaction.rs (1 hunks)
- crates/katana/rpc/rpc/Cargo.toml (1 hunks)
- crates/katana/rpc/rpc/src/starknet/forking.rs (1 hunks)
- crates/katana/rpc/rpc/src/starknet/mod.rs (19 hunks)
- crates/katana/rpc/rpc/src/starknet/read.rs (3 hunks)
- crates/katana/rpc/rpc/src/starknet/trace.rs (1 hunks)
🔇 Additional comments (28)
crates/katana/rpc/rpc/Cargo.toml (1)
26-26: Ohayo sensei! Dependencies look good for forked provider implementation.The addition of
tokioand migration ofurlto main dependencies aligns perfectly with the requirements for implementing the forked provider functionality. These dependencies are essential for:
- Async operations when fetching remote block data
- URL handling for fork network connections
Also applies to: 28-28
crates/katana/rpc/rpc-types/src/state_update.rs (4)
13-24: Ohayo! Clean implementation of MaybePendingStateUpdate conversion, sensei!The pattern matching approach ensures proper handling of both variants while maintaining type safety through explicit conversions.
40-40: Clean use of Self for better maintainability!The change to use
Selfinstead of explicit type name follows Rust best practices.
90-94: Clean and consistent implementation!The From trait implementation follows the established pattern in the codebase.
Line range hint
79-88: Verify the default values for new fields, sensei!The implementation sets
replaced_classesanddeprecated_declared_classesto default values. Please confirm if this is the intended behavior for forked block data.✅ Verification successful
Based on the search results, I can now generate the final response since I have found clear evidence about the intended behavior.
Ohayo! These default values are actually the correct implementation, sensei!
The codebase explicitly documents that Katana does not support
replaced_classesanddeprecated_declared_classesas seen in the comments:
crates/saya/provider/src/rpc/state.rs: "Currently, Katana does not supportreplaced_classesanddeprecated_declared_classes"- The same file shows multiple instances where these fields are consistently set to default values
- Other implementations (like in
torii/grpc) initialize these fields as empty vectors/mapsSetting these fields to default values is the correct approach since Katana intentionally does not handle contract upgrades via the
replace_classsyscall at this time.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of these fields to verify if they should always be default rg -A 5 "replaced_classes|deprecated_declared_classes" --type rustLength of output: 7934
crates/katana/rpc/rpc-api/src/starknet.rs (2)
15-15: Ohayo! Import change looks good, sensei!The update to use
MaybePendingStateUpdatealigns well with the forked provider implementation, allowing for pending state handling.
64-64: Method signature update is well-crafted, sensei!The change to return
RpcResult<MaybePendingStateUpdate>enhances the API's capability to handle both finalized and pending state updates from the fork network.Let's verify the consistency of this change across the codebase:
✅ Verification successful
The change to
MaybePendingStateUpdateis consistently implemented across the codebase!The verification shows that:
- The trait definition in
rpc-api/src/starknet.rscorrectly usesMaybePendingStateUpdate- The implementation in
rpc/src/starknet/read.rsmatches the trait signature- The type is properly defined in
rpc-types/src/state_update.rswith correct variants- All related code paths handle both regular and pending state updates appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all implementations of get_state_update are updated # to use MaybePendingStateUpdate # Search for get_state_update implementations ast-grep --pattern 'fn get_state_update($$$) -> $result { $$$ }' # Search for any remaining StateUpdate usages that might need updating rg "StateUpdate" --type rustLength of output: 18670
crates/katana/rpc/rpc-types/src/receipt.rs (2)
124-128: Clean From implementation, sensei! ✨The From trait implementation provides a type-safe way to convert from the core Starknet type, which is perfect for the forking functionality.
122-122: Ohayo! Verify the necessity of making the inner field public.Making the inner field public weakens encapsulation. While this change enables direct access needed for forking functionality, we should verify that this is the most appropriate approach.
✅ Verification successful
Let me gather more context about the usage of TxReceiptWithBlockInfo to understand if making it public is justified.
Ohayo! The public field is justified and well-used, sensei!
The public field in
TxReceiptWithBlockInfois necessary because:
- It's used in the forking functionality where direct access is needed for conversion from the provider's receipt type
- The struct is part of the public API and is used across multiple crates (rpc, rpc-types-builder, rpc-api)
- The implementation provides a clean conversion layer between starknet core types and Katana's RPC types
The encapsulation concern is mitigated by the type's focused responsibility as a wrapper for starknet core types in the RPC layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct access to the inner field to confirm necessity rg -l "TxReceiptWithBlockInfo" | xargs rg "\.0\s*"Length of output: 2426
Script:
#!/bin/bash # Search for usages of TxReceiptWithBlockInfo rg -A 3 "TxReceiptWithBlockInfo" --type rust # Also check for any implementations or trait bounds ast-grep --pattern 'impl $_ for TxReceiptWithBlockInfo { $$ }'Length of output: 7596
crates/katana/rpc/rpc/src/starknet/read.rs (3)
2-2: Ohayo sensei! Import changes look good!The new imports align perfectly with the PR objectives for fetching forked block data and support the updated return types.
Also applies to: 9-9, 14-15
Line range hint
121-150: Architectural consistency check for blocking tasksThe retention of
on_io_blocking_taskandon_cpu_blocking_taskin these complex methods appears intentional and appropriate, given their CPU-intensive nature (especially for fee estimation) and complex state management requirements.Let's verify that these methods require blocking operations:
#!/bin/bash # Description: Check for CPU-intensive or blocking operations in these methods echo "Checking for executor usage in these methods..." rg "executor\." -B 2 -A 2 echo "Checking for complex state operations..." rg "state\." -B 2 -A 2Also applies to: 166-181, 182-197, 198-244, 245-289
43-43: Ohayo sensei! Nice simplification of the RPC methods!The removal of
on_io_blocking_taskwrappers across these methods creates a more streamlined implementation. The consistent pattern of using direct async calls with proper error propagation is clean and maintainable.Let's verify that all the underlying methods being called are properly async-safe:
Also applies to: 47-47, 70-70, 78-78, 85-85, 92-92, 95-96, 103-103
crates/katana/rpc/rpc-types/src/block.rs (2)
98-109: Ohayo sensei! Clean and correct implementation!The
Fromtrait implementation forMaybePendingBlockWithTxsfollows Rust's best practices for type conversion with proper variant mapping and exhaustive pattern matching.
197-208: Implementation maintains consistency with the type conversion pattern!The
Fromtrait implementation forMaybePendingBlockWithTxHashescorrectly mirrors the established pattern, ensuring consistent behavior across the codebase.crates/katana/core/src/backend/storage.rs (3)
6-6: LGTM! Clean import addition.The BlockNumber import is properly grouped with related block primitives.
152-153: LGTM! Clean block number extraction.The block number is properly extracted from the forked block and stored with a clear variable name.
200-201: LGTM! Proper tuple return implementation.The return statement correctly includes both the blockchain instance and block number, maintaining consistency with the method signature.
crates/katana/rpc/rpc-types/src/transaction.rs (1)
Line range hint
547-550: Ohayo sensei! Consider addressing the TODO comment.The TODO comment suggests avoiding conversions between types. Since this PR adds another conversion implementation, it might be a good time to consider a more comprehensive solution.
Could you clarify if:
- This is the right time to address this technical debt?
- The new
Fromimplementation adds to this problem?Let's analyze the impact:
crates/katana/node/src/lib.rs (7)
38-38: Ohayo, sensei! ImportingForkedClientis appropriate.The addition of the
ForkedClientimport is necessary for handling forked clients.
91-91: Ohayo, sensei! Addition offorked_clientfield toNodestruct is correct.Including
Option<ForkedClient>in theNodestruct properly allows for optional forked client handling.
98-98: Ohayo, sensei! Changingselftomut selfinlaunchmethod is necessary.Making
selfmutable allows modification ofself.forked_clientwithin the method.
144-144: Ohayo, sensei! Verify the use ofself.forked_client.take().Using
take()movesforked_clientout ofself. Please ensure thatforked_clientis not needed elsewhere after this operation.
242-248: Ohayo, sensei! Update tospawnfunction signature is correct.Including
Option<ForkedClient>in thespawnfunction allows passing the forked client appropriately.
251-251: Ohayo, sensei! Destructuringnode_componentsincludesforked_clientcorrectly.Unpacking
forked_clientfromnode_componentsensures all components are properly accessed.
256-272: Ohayo, sensei! Conditional initialization ofStarknetApiis well-implemented.Switching between
StarknetApi::new_forkedandStarknetApi::newbased on the presence offorked_clientis appropriate.crates/katana/rpc/rpc-types/src/error/starknet.rs (3)
10-11: Ohayo, sensei! Necessary imports added correctlyThe imports of
StarknetRsErrorandStarknetRsProviderErrorare appropriately added and aliased, ensuring proper error conversion.
246-261: Ohayo, sensei! Verify error mapping inFrom<StarknetRsProviderError>implementationEnsure that all variants of
StarknetRsProviderErrorare properly mapped toStarknetApiError. Providing more specific error messages, when possible, can aid in debugging and improve usability.You can use the following script to verify all variants are covered:
#!/bin/bash # Description: Verify all variants of 'StarknetRsProviderError' are mapped. # Test: List variants of 'StarknetRsProviderError' rg -A 5 'pub enum ProviderError' | grep '^\s\+\w\+,' | sed 's/,$//' > provider_variants.txt # List mapped variants in the 'From' implementation rg 'StarknetRsProviderError::' | sed 's/.*StarknetRsProviderError::\(\w\+\).*=>.*/\1/' > mapped_variants.txt # Compare the two lists echo "Variants not mapped:" comm -23 <(sort provider_variants.txt) <(sort mapped_variants.txt)
45-47: Ohayo, sensei! Ensure consistency in transaction index typesChanging
transaction_indexfromusizetou64enhances consistency and supports a broader range of indices. Please verify that all instances oftransaction_indexin the codebase are updated tou64to prevent any type mismatches.You can use the following script to check for any remaining occurrences of
transaction_indexusingusize:✅ Verification successful
Let me search for both the usage and type declarations to get a better understanding of the consistency across the codebase.
Ohayo, sensei! All transaction indices are consistently using u64
After checking the entire codebase, I can confirm that all instances of
transaction_indexare already usingu64. The change fromusizetou64has been consistently applied across:
- Error types in
error/starknet.rs- Transaction cursor in
transaction.rs- All usages in RPC implementations and tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all declarations of 'transaction_index' are using 'u64'. # Test: Search for declarations of 'transaction_index' with type 'usize'. rg 'transaction_index:\s*usize'Length of output: 371
Script:
#!/bin/bash # Let's search for transaction_index with broader context rg -B 2 -A 2 'transaction_index' # Also search for struct definitions containing transaction_index ast-grep --pattern 'struct $_ { $$$ transaction_index: $_, $$$ }'Length of output: 13429
| starknet::core::types::MaybePendingBlockWithTxs::PendingBlock(_) => { | ||
| panic!("shouldn't be possible to be pending") | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Consistently handle unexpected pending blocks without panicking.
Similar to the previous instance, replace the panic! with an appropriate error return to prevent crashing.
Apply this diff:
starknet::core::types::MaybePendingBlockWithTxs::PendingBlock(_) => {
- panic!("shouldn't be possible to be pending")
+ return Err(Error::UnexpectedPendingBlock)
}📝 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.
| starknet::core::types::MaybePendingBlockWithTxs::PendingBlock(_) => { | |
| panic!("shouldn't be possible to be pending") | |
| } | |
| starknet::core::types::MaybePendingBlockWithTxs::PendingBlock(_) => { | |
| return Err(Error::UnexpectedPendingBlock) | |
| } |
| starknet::core::types::MaybePendingBlockWithTxHashes::PendingBlock(_) => { | ||
| panic!("shouldn't be possible to be pending") | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Replace panic with error handling for pending blocks.
Consistently handle unexpected cases by returning errors instead of panicking.
Apply this diff:
starknet::core::types::MaybePendingBlockWithTxHashes::PendingBlock(_) => {
- panic!("shouldn't be possible to be pending")
+ return Err(Error::UnexpectedPendingBlock)
}Committable suggestion was skipped due to low confidence.
| starknet::core::types::MaybePendingBlockWithReceipts::PendingBlock(_) => { | ||
| panic!("shouldn't be possible to be pending") | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Prevent application crash on unexpected pending blocks.
To maintain stability, avoid using panic! and return a meaningful error instead.
Apply this diff:
starknet::core::types::MaybePendingBlockWithReceipts::PendingBlock(_) => {
- panic!("shouldn't be possible to be pending")
+ return Err(Error::UnexpectedPendingBlock)
}Committable suggestion was skipped due to low confidence.
| BlockIdOrTag::Tag(_) => { | ||
| panic!("shouldn't be possible to be tag") | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Avoid panicking when an unexpected tag is encountered.
Consistently return an error instead of panicking to improve reliability.
Apply this diff:
BlockIdOrTag::Tag(_) => {
- panic!("shouldn't be possible to be tag")
+ return Err(Error::UnexpectedTag)
}Committable suggestion was skipped due to low confidence.
| BlockIdOrTag::Tag(_) => { | ||
| panic!("shouldn't be possible to be tag") | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Handle unexpected tags without panicking.
Using panic! may lead to crashes. Return an error to handle unexpected tags gracefully.
Apply this diff:
BlockIdOrTag::Tag(_) => {
- panic!("shouldn't be possible to be tag")
+ return Err(Error::UnexpectedTag)
}Add the new error variant to the Error enum:
pub enum Error {
/// Error originating from the underlying [`Provider`] implementation.
#[error("Provider error: {0}")]
Provider(#[from] ProviderError),
#[error("Block out of range")]
BlockOutOfRange,
+ #[error("Unexpected tag encountered in block ID")]
+ UnexpectedTag,
}Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2592 +/- ##
==========================================
+ Coverage 69.65% 70.24% +0.59%
==========================================
Files 401 402 +1
Lines 50818 51130 +312
==========================================
+ Hits 35395 35914 +519
+ Misses 15423 15216 -207 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
crates/katana/rpc/rpc/tests/forking.rs (1)
88-88: Ohayo, sensei! Verify the Actual Return Values in AssertionsCurrently, you're asserting that the result is OK using
assert!(result.is_ok());. For more comprehensive tests, consider verifying the actual return values to ensure they match the expected outcomes.Also applies to: 118-118
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- crates/katana/rpc/rpc/src/starknet/forking.rs (1 hunks)
- crates/katana/rpc/rpc/tests/forking.rs (1 hunks)
🔇 Additional comments (8)
crates/katana/rpc/rpc/tests/forking.rs (1)
138-140:⚠️ Potential issueOhayo, sensei! Missing Assertions for Transaction Hash
You've assigned a transaction hash on line 138 but haven't performed any assertions or operations with it before the function ends. Did you intend to include further tests using this transaction hash?
crates/katana/rpc/rpc/src/starknet/forking.rs (7)
110-112: Ohayo, sensei! Replacepanic!with error return for pending blocksUsing
panic!may cause the application to crash unexpectedly. It's better to return a meaningful error to handle this case gracefully.
122-124: Ohayo, sensei! Avoid panicking when encountering unexpected tagsConsistently handle unexpected tags by returning an appropriate error instead of panicking.
143-145: Ohayo, sensei! Replacepanic!with proper error handling for pending blocksTo maintain application stability, avoid using
panic!and return a descriptive error instead.
161-163: Ohayo, sensei! Handle unexpected pending blocks without panickingReturning an error instead of panicking improves the robustness of your application.
181-183: Ohayo, sensei! Replace panic with error handlingConsistently handle unexpected pending blocks by returning errors instead of panicking.
230-232: Ohayo, sensei! Handle unexpected tags without panickingReturning an error for unexpected tags enhances the reliability of your application.
73-83: Ohayo, sensei! Simplify asynchronous error handling inget_transaction_statusThe current implementation awaits
receiptandstatusconcurrently, but only usesreceiptfor error checking. Consider restructuring for clarity and efficiency.
| @@ -0,0 +1,141 @@ | |||
| use anyhow::Result; | |||
| use assert_matches::assert_matches; | |||
| use cainome::rs::abigen_legacy; | |||
There was a problem hiding this comment.
Ohayo, sensei! Possible Typo in Import Statement
It appears there may be a typo in the import on line 3. Did you mean to import from cairo::rs::abigen_legacy instead of cainome::rs::abigen_legacy?
| impl From<Error> for StarknetApiError { | ||
| fn from(value: Error) -> Self { | ||
| match value { | ||
| Error::Provider(provider_error) => provider_error.into(), | ||
| Error::BlockOutOfRange => StarknetApiError::BlockNotFound, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Map new error variants to StarknetApiError
After adding new error variants to the Error enum, ensure they are appropriately mapped to StarknetApiError for consistent API error handling.
Apply this diff to enhance error mapping:
impl From<Error> for StarknetApiError {
fn from(value: Error) -> Self {
match value {
Error::Provider(provider_error) => provider_error.into(),
- Error::BlockOutOfRange => StarknetApiError::BlockNotFound,
+ Error::BlockOutOfRange
+ | Error::UnexpectedPendingBlock
+ | Error::UnexpectedTag => StarknetApiError::BlockNotFound,
}
}
}Committable suggestion was skipped due to low confidence.
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| /// Error originating from the underlying [`Provider`] implementation. | ||
| #[error("Provider error: {0}")] | ||
| Provider(#[from] ProviderError), | ||
|
|
||
| #[error("Block out of range")] | ||
| BlockOutOfRange, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider enhancing the Error enum for comprehensive error handling
The current Error enum only includes Provider and BlockOutOfRange variants. To handle unexpected cases more gracefully and avoid panicking, consider adding variants like UnexpectedPendingBlock and UnexpectedTag.
Apply this diff to expand the Error enum:
pub enum Error {
/// Error originating from the underlying [`Provider`] implementation.
#[error("Provider error: {0}")]
Provider(#[from] ProviderError),
#[error("Block out of range")]
BlockOutOfRange,
+ #[error("Unexpected pending block encountered")]
+ UnexpectedPendingBlock,
+ #[error("Unexpected tag encountered")]
+ UnexpectedTag,
}📝 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.
| #[derive(Debug, thiserror::Error)] | |
| pub enum Error { | |
| /// Error originating from the underlying [`Provider`] implementation. | |
| #[error("Provider error: {0}")] | |
| Provider(#[from] ProviderError), | |
| #[error("Block out of range")] | |
| BlockOutOfRange, | |
| } | |
| #[derive(Debug, thiserror::Error)] | |
| pub enum Error { | |
| /// Error originating from the underlying [`Provider`] implementation. | |
| #[error("Provider error: {0}")] | |
| Provider(#[from] ProviderError), | |
| #[error("Block out of range")] | |
| BlockOutOfRange, | |
| #[error("Unexpected pending block encountered")] | |
| UnexpectedPendingBlock, | |
| #[error("Unexpected tag encountered")] | |
| UnexpectedTag, | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
crates/katana/rpc/rpc/tests/starknet.rs (1)
Line range hint 1-900: Consider adding test coverage for forked network scenarios.
Ohayo, sensei! While the test file comprehensively covers various error scenarios, it would be beneficial to add specific test cases for the forked network functionality being implemented, as mentioned in the PR objectives.
Would you like me to help generate test cases that cover forked network scenarios for non-state data fetching?
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
663-663: Address TODO for ExecutionResult conversion
Ohayo, sensei! The TODO comment indicates a need for implementing From<ExecutionResult> for TransactionStatus. This would improve code maintainability and reduce the manual match patterns.
Would you like me to help implement the From trait for this conversion?
912-914: Consider adding documentation for pending block limitation
Ohayo, sensei! Consider adding a doc comment explaining why pending blocks are not supported for state updates. This would help future maintainers understand the design decision.
+ // Pending blocks are not supported for state updates as they represent
+ // an intermediate state that hasn't been committed to the chain yet.
BlockIdOrTag::Tag(BlockTag::Pending) => {
return Err(StarknetApiError::BlockNotFound);
}crates/katana/rpc/rpc/tests/forking.rs (1)
53-53: Ohayo, sensei! Fix typo in comment
There's a typo in your comment on line 53. 'emtpy' should be 'empty'.
Apply this diff to fix the typo:
- // create some emtpy blocks and dummy transactions
+ // create some empty blocks and dummy transactions📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/katana/rpc/rpc/src/starknet/mod.rs (19 hunks)
- crates/katana/rpc/rpc/tests/common/mod.rs (2 hunks)
- crates/katana/rpc/rpc/tests/forking.rs (1 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (7 hunks)
🔇 Additional comments (9)
crates/katana/rpc/rpc/tests/common/mod.rs (2)
74-74: Ohayo sensei! Verify the necessity of the unused function.
The split_felt function is marked as unused. Let's verify if this function is needed for future implementation or can be safely removed.
#!/bin/bash
# Description: Check if split_felt is used anywhere in the codebase
# or mentioned in issues/PRs
# Search for any references to split_felt in the code
echo "Searching for split_felt usage in code..."
rg "split_felt"
# Search for mentions in TODO comments
echo "Searching for related TODOs..."
rg -i "todo.*split.*felt"75-91: Ohayo sensei! The new assertion macros look great!
The implementation of both macros follows Rust best practices with clear documentation and proper error pattern matching. They provide a clean way to assert Starknet errors in tests.
Let's verify their usage in tests:
✅ Verification successful
The previous results didn't show the actual usage of these macros in tests, so let's specifically look for their usage patterns with some context lines. This will help us verify if they're being used correctly in test cases.
The previous search failed because we were looking in the wrong directory. Let's try with the correct path based on the file location of the macros.
Ohayo sensei! The new assertion macros are being used properly and extensively!
The macros are being actively used in test files with correct error pattern matching:
assert_account_starknet_err!is used 7 times instarknet.rsfor account-related errorsassert_provider_starknet_err!is used 8 times inforking.rsfor provider-related errors
All usages follow the intended pattern and correctly match against specific StarknetError variants like:
BlockNotFoundTransactionHashNotFoundInsufficientAccountBalanceInvalidTransactionNonce
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new assertion macros in tests
echo "Checking usage of assert_account_starknet_err..."
rg "assert_account_starknet_err!" --type rust
echo "Checking usage of assert_provider_starknet_err..."
rg "assert_provider_starknet_err!" --type rust
# Check if there are any remaining uses of old assertion patterns
echo "Checking for potential missed migrations..."
rg "assert_matches!.*StarknetError" --type rust
Length of output: 1640
Script:
#!/bin/bash
# Let's search specifically for the usage of our new macros in test files
echo "Searching for macro usages in test files..."
rg "assert_account_starknet_err!" -A 2 -B 2 --type rust tests/
rg "assert_provider_starknet_err!" -A 2 -B 2 --type rust tests/
# Let's also check if these macros are imported in test files
echo "Checking macro imports in test files..."
rg "use.*assert_.*starknet_err" --type rust tests/
Length of output: 510
Script:
#!/bin/bash
# Let's try searching from the repository root with proper path
echo "Searching for macro usages..."
rg "assert_account_starknet_err!" -A 2 -B 2 --type rust crates/katana/rpc/rpc/tests/
rg "assert_provider_starknet_err!" -A 2 -B 2 --type rust crates/katana/rpc/rpc/tests/
# Let's also check macro imports
echo "Checking macro imports..."
rg "use.*assert_.*starknet_err" --type rust crates/katana/rpc/rpc/tests/
# Let's find all test files to make sure we're searching in the right place
echo "Listing test files..."
fd ".*test.*\.rs$" crates/katana/rpc/rpc/tests/
Length of output: 6194
crates/katana/rpc/rpc/tests/starknet.rs (5)
176-176: LGTM! Macro replacement for class declaration error handling.
The change from assert_starknet_err! to assert_account_starknet_err! maintains the error checking behavior while providing better account-specific context.
407-407: LGTM! Enhanced error assertion for insufficient balance validation.
The macro replacement appropriately handles the account-specific insufficient balance error case.
449-449: LGTM! Improved fee-related error assertions.
The macro replacements correctly handle both insufficient max fee and insufficient account balance scenarios in the fee validation tests.
Also applies to: 470-470
526-526: LGTM! Updated signature validation error handling.
The macro replacement properly asserts the account validation failure for invalid signatures.
572-572: LGTM! Enhanced nonce validation error assertions.
The macro replacements correctly handle invalid transaction nonce scenarios, maintaining proper error checking behavior.
Also applies to: 597-597
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
3-3: LGTM! Structural changes for forking support
The addition of the forking module and ForkedClient integration aligns well with the PR objectives to support fetching non-state data from forked networks.
Also applies to: 10-10, 71-72
85-93: LGTM! Well-structured constructor implementation
Ohayo, sensei! The constructor implementation follows good practices:
- Avoids code duplication by using a shared
new_innermethod - Maintains backward compatibility with the existing
newmethod - Properly handles the optional
forked_client
Also applies to: 95-107
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
crates/katana/rpc/rpc/tests/forking.rs (2)
36-36: Ohayo, sensei! Consider a more descriptive type alias.
The type alias LocalTestVector could be more descriptive to better convey its purpose.
-type LocalTestVector = Vec<(BlockNumber, TxHash)>;
+type LocalBlockTransactions = Vec<(BlockNumber, TxHash)>;104-106: Ohayo, sensei! Consider tracking the TODO in an issue.
The TODO comment about including genesis forked state update should be tracked.
Would you like me to create a GitHub issue to track this TODO item?
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
72-72: Consider adding error handling for forked client initialization.
The new_forked constructor could benefit from additional error handling to ensure the forked client is properly initialized and connected.
Consider adding validation:
pub fn new_forked(
backend: Arc<Backend<EF>>,
pool: TxPool,
block_producer: BlockProducer<EF>,
validator: TxValidator,
forked_client: ForkedClient,
) -> Self {
+ // Validate forked client connection
+ if let Err(e) = forked_client.validate_connection() {
+ log::error!("Failed to initialize forked client: {}", e);
+ }
Self::new_inner(backend, pool, block_producer, validator, Some(forked_client))
}Also applies to: 85-93
Line range hint 108-124: Add documentation for blocking task helpers.
Ohayo, sensei! These helper methods would benefit from documentation explaining their purpose and usage patterns.
Consider adding documentation:
+/// Executes CPU-intensive tasks on a dedicated thread pool to avoid blocking the async runtime.
+///
+/// # Arguments
+/// * `func` - The function to execute
+///
+/// # Returns
+/// The result of the executed function
async fn on_cpu_blocking_task<F, T>(&self, func: F) -> T
where
F: FnOnce(Self) -> T + Send + 'static,
T: Send + 'static,
+/// Executes IO-bound tasks on a dedicated thread pool.
+///
+/// # Arguments
+/// * `func` - The function to execute
+///
+/// # Returns
+/// The result of the executed function
async fn on_io_blocking_task<F, T>(&self, func: F) -> T📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- crates/katana/rpc/rpc/src/starknet/mod.rs (19 hunks)
- crates/katana/rpc/rpc/tests/forking.rs (1 hunks)
🔇 Additional comments (5)
crates/katana/rpc/rpc/tests/forking.rs (4)
3-3: Ohayo, sensei! Fix the typo in import statement.
The import cainome::rs::abigen_legacy appears to be incorrect.
-use cainome::rs::abigen_legacy;
+use cairo::rs::abigen_legacy;14-14: Ohayo, sensei! Add missing imports for used types.
The types MaybePendingBlockWithReceipts, MaybePendingBlockWithTxHashes, and MaybePendingStateUpdate are used but not imported.
-use starknet::core::types::{MaybePendingBlockWithTxs, StarknetError};
+use starknet::core::types::{
+ MaybePendingBlockWithTxs,
+ MaybePendingBlockWithReceipts,
+ MaybePendingBlockWithTxHashes,
+ MaybePendingStateUpdate,
+ StarknetError,
+};59-59: Ohayo, sensei! Add missing import for Uint256.
The type Uint256 is used but not imported.
1-236: Ohayo, sensei! Well-structured test implementation.
The test file provides comprehensive coverage of forking functionality with well-organized test cases, proper error handling, and clear documentation. The test scenarios effectively cover both successful and error cases, making good use of helper functions for reusable assertions.
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
3-3: LGTM! Clean module organization.
The addition of the forking module and its import follows good Rust module organization practices.
Also applies to: 10-10
| // seach in the pending block if the transaction is not found | ||
| if let Some(pending_executor) = this.pending_executor() { | ||
| let pending_executor = pending_executor.read(); | ||
| let pending_txs = pending_executor.transactions(); | ||
| let (_, res) = pending_txs | ||
| .iter() | ||
| .find(|(tx, _)| tx.hash == hash) | ||
| .ok_or(StarknetApiError::TxnHashNotFound)?; | ||
|
|
||
| // TODO: should impl From<ExecutionResult> for TransactionStatus | ||
| let status = match res { | ||
| ExecutionResult::Failed { .. } => TransactionStatus::Rejected, | ||
| ExecutionResult::Success { receipt, .. } => { | ||
| if receipt.is_reverted() { | ||
| TransactionStatus::AcceptedOnL2( | ||
| TransactionExecutionStatus::Reverted, | ||
| ) | ||
| } else { | ||
| TransactionStatus::AcceptedOnL2( | ||
| TransactionExecutionStatus::Succeeded, | ||
| ) | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| Ok(Some(status)) | ||
| } else { | ||
| // Err(StarknetApiError::TxnHashNotFound) | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Race condition concern verified in pending transaction handling
Ohayo, sensei! After analyzing the codebase, I can confirm the race condition concern is valid. The PendingExecutor is indeed wrapped in an Arc<RwLock<...>>, and the current implementation has a potential TOCTOU (Time-of-Check-Time-of-Use) issue between acquiring the executor and reading the transactions.
The suggested fix in the review comment is correct:
- Combining the
pending_executor()call with theread()operation into a single atomic read - Restructuring the logic to avoid the
.ok_or()which could panic if the transaction is removed
The proposed solution would make the code more robust against concurrent modifications of the pending transaction pool.
🔗 Analysis chain
Potential race condition in pending transaction status.
Ohayo, sensei! There's a potential race condition when checking transaction status in the pending block. The transaction might be removed from the pending block between the time we check its existence and when we try to access its result.
Consider using a single atomic read:
-if let Some(pending_executor) = this.pending_executor() {
- let pending_executor = pending_executor.read();
- let pending_txs = pending_executor.transactions();
- let (_, res) = pending_txs
+if let Some(pending_executor) = this.pending_executor().map(|e| e.read()) {
+ if let Some((_, res)) = pending_executor.transactions()
.iter()
- .find(|(tx, _)| tx.hash == hash)
- .ok_or(StarknetApiError::TxnHashNotFound)?;
+ .find(|(tx, _)| tx.hash == hash) {Let me gather more context about the pending_executor implementation to better understand the concurrency model.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar patterns in other files
ast-grep --pattern 'if let Some(pending_executor) = $_.pending_executor() {
$$$
let pending_txs = $_.transactions();
$$$
}'
Length of output: 2866
Script:
#!/bin/bash
# Check the implementation of pending_executor and related types
ast-grep --pattern 'fn pending_executor($$$) -> $$ {
$$$
}'
# Also check for any RwLock usage
rg "RwLock" -A 2
Length of output: 12488
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
crates/katana/rpc/rpc-types-builder/src/block.rs (2)
51-53: Ohayo! Nice architectural improvement in block hash retrieval!
The new approach of first fetching the block hash aligns well with the forking functionality objectives. This separation of concerns allows for better handling of forked chain data.
55-56: Consider enhancing error handling, sensei!
While using expect is valid here, consider providing more context in error messages for better debugging, especially when dealing with forked chains.
- .expect("should exist if block exists");
+ .expect("block should exist as its hash was just verified");crates/katana/rpc/rpc/tests/forking.rs (2)
33-33: Ohayo, sensei! Grammatical correction in function comment
There's a grammatical error on line 33. It should be "This function will fork Sepolia..." instead of "forked Sepolia".
Apply this diff to fix the grammar:
-/// This function will forked Sepolia at block [`FORK_BLOCK_NUMBER`] and create 10 blocks, each has
+/// This function will fork Sepolia at block [`FORK_BLOCK_NUMBER`] and create 10 blocks, each has46-46: Ohayo, sensei! Typo in comment
There's a typo on line 46. The word "emtpy" should be "empty".
Apply this diff to correct the typo:
- // create some emtpy blocks and dummy transactions
+ // create some empty blocks and dummy transactions📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/katana/rpc/rpc-types-builder/src/block.rs (1 hunks)
- crates/katana/rpc/rpc-types/src/block.rs (5 hunks)
- crates/katana/rpc/rpc/src/starknet/mod.rs (19 hunks)
- crates/katana/rpc/rpc/tests/forking.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/rpc/rpc-types/src/block.rs
🔇 Additional comments (5)
crates/katana/rpc/rpc-types-builder/src/block.rs (1)
64-64: Clean and clear BlockWithReceipts construction!
The explicit use of hash in the constructor improves code clarity and maintains consistency with the new block handling approach.
crates/katana/rpc/rpc/src/starknet/mod.rs (4)
Line range hint 3-53: LGTM! Module structure aligns with forking feature.
The new forking module and related imports are well-organized and properly support the PR's objective of enabling forked block data fetching.
Line range hint 72-107: LGTM! Clean implementation of forked client support.
Ohayo, sensei! The changes to support forked clients are well-structured:
- Optional
forked_clientallows graceful fallback - Common initialization logic extracted to
new_innerfollows DRY principle
654-683: Race condition in pending transaction handling.
The existing review comment about the race condition is still valid.
909-913: Verify intended behavior for pending state updates.
Ohayo, sensei! The code immediately returns BlockNotFound for pending blocks in state updates. Please verify if this is the intended behavior or if pending state updates should be supported in the future.
| async fn block_with_txs( | ||
| &self, | ||
| hash: TxHash, | ||
| ) -> Result<TransactionStatus, StarknetApiError> { | ||
| self.on_io_blocking_task(move |this| { | ||
| let provider = this.inner.backend.blockchain.provider(); | ||
| let status = provider.transaction_status(hash)?; | ||
|
|
||
| if let Some(status) = status { | ||
| // TODO: this might not work once we allow querying for 'failed' transactions from | ||
| // the provider | ||
| let Some(receipt) = provider.receipt_by_hash(hash)? else { | ||
| return Err(StarknetApiError::UnexpectedError { | ||
| reason: "Transaction hash exist, but the receipt is missing".to_string(), | ||
| }); | ||
| }; | ||
| block_id: BlockIdOrTag, | ||
| ) -> StarknetApiResult<MaybePendingBlockWithTxs> { | ||
| let block = self | ||
| .on_io_blocking_task(move |this| { | ||
| let provider = this.inner.backend.blockchain.provider(); | ||
|
|
||
| if BlockIdOrTag::Tag(BlockTag::Pending) == block_id { | ||
| if let Some(executor) = this.pending_executor() { | ||
| let block_env = executor.read().block_env(); | ||
| let latest_hash = provider.latest_hash().map_err(StarknetApiError::from)?; | ||
|
|
||
| let l1_gas_prices = block_env.l1_gas_prices.clone(); | ||
| let l1_data_gas_prices = block_env.l1_data_gas_prices.clone(); | ||
|
|
||
| let header = PartialHeader { | ||
| l1_da_mode: L1DataAvailabilityMode::Calldata, | ||
| l1_gas_prices, | ||
| l1_data_gas_prices, | ||
| number: block_env.number, | ||
| parent_hash: latest_hash, | ||
| timestamp: block_env.timestamp, | ||
| sequencer_address: block_env.sequencer_address, | ||
| protocol_version: this.inner.backend.chain_spec.version.clone(), | ||
| }; | ||
|
|
||
| // TODO(kariy): create a method that can perform this filtering for us | ||
| // instead of doing it manually. | ||
|
|
||
| // A block should only include successful transactions, we filter out the | ||
| // failed ones (didn't pass validation stage). | ||
| let transactions = executor | ||
| .read() | ||
| .transactions() | ||
| .iter() | ||
| .filter(|(_, receipt)| receipt.is_success()) | ||
| .map(|(tx, _)| tx.clone()) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let block = PendingBlockWithTxs::new(header, transactions); | ||
| return Ok(Some(MaybePendingBlockWithTxs::Pending(block))); | ||
| } | ||
| } | ||
|
|
||
| if let Some(num) = provider.convert_block_id(block_id)? { | ||
| let block = katana_rpc_types_builder::BlockBuilder::new(num.into(), provider) | ||
| .build()? | ||
| .map(MaybePendingBlockWithTxs::Block); | ||
|
|
||
| let exec_status = if receipt.is_reverted() { | ||
| TransactionExecutionStatus::Reverted | ||
| StarknetApiResult::Ok(block) | ||
| } else { | ||
| TransactionExecutionStatus::Succeeded | ||
| }; | ||
| StarknetApiResult::Ok(None) | ||
| } | ||
| }) | ||
| .await?; | ||
|
|
||
| return Ok(match status { | ||
| FinalityStatus::AcceptedOnL1 => TransactionStatus::AcceptedOnL1(exec_status), | ||
| FinalityStatus::AcceptedOnL2 => TransactionStatus::AcceptedOnL2(exec_status), | ||
| }); | ||
| } | ||
| if let Some(block) = block { | ||
| Ok(block) | ||
| } else if let Some(client) = &self.inner.forked_client { | ||
| Ok(client.get_block_with_txs(block_id).await?) | ||
| } else { | ||
| Err(StarknetApiError::BlockNotFound) | ||
| } | ||
| } | ||
|
|
||
| // seach in the pending block if the transaction is not found | ||
| if let Some(pending_executor) = this.pending_executor() { | ||
| let pending_executor = pending_executor.read(); | ||
| let pending_txs = pending_executor.transactions(); | ||
| let (_, res) = pending_txs | ||
| .iter() | ||
| .find(|(tx, _)| tx.hash == hash) | ||
| .ok_or(StarknetApiError::TxnHashNotFound)?; | ||
|
|
||
| // TODO: should impl From<ExecutionResult> for TransactionStatus | ||
| let status = match res { | ||
| ExecutionResult::Failed { .. } => TransactionStatus::Rejected, | ||
| ExecutionResult::Success { receipt, .. } => { | ||
| if receipt.is_reverted() { | ||
| TransactionStatus::AcceptedOnL2(TransactionExecutionStatus::Reverted) | ||
| } else { | ||
| TransactionStatus::AcceptedOnL2(TransactionExecutionStatus::Succeeded) | ||
| } | ||
| async fn block_with_receipts( | ||
| &self, | ||
| block_id: BlockIdOrTag, | ||
| ) -> StarknetApiResult<MaybePendingBlockWithReceipts> { | ||
| let block = self | ||
| .on_io_blocking_task(move |this| { | ||
| let provider = this.inner.backend.blockchain.provider(); | ||
|
|
||
| if BlockIdOrTag::Tag(BlockTag::Pending) == block_id { | ||
| if let Some(executor) = this.pending_executor() { | ||
| let block_env = executor.read().block_env(); | ||
| let latest_hash = provider.latest_hash()?; | ||
|
|
||
| let l1_gas_prices = block_env.l1_gas_prices.clone(); | ||
| let l1_data_gas_prices = block_env.l1_data_gas_prices.clone(); | ||
|
|
||
| let header = PartialHeader { | ||
| l1_gas_prices, | ||
| l1_data_gas_prices, | ||
| number: block_env.number, | ||
| parent_hash: latest_hash, | ||
| timestamp: block_env.timestamp, | ||
| l1_da_mode: L1DataAvailabilityMode::Calldata, | ||
| sequencer_address: block_env.sequencer_address, | ||
| protocol_version: this.inner.backend.chain_spec.version.clone(), | ||
| }; | ||
|
|
||
| let receipts = executor | ||
| .read() | ||
| .transactions() | ||
| .iter() | ||
| .filter_map(|(tx, result)| match result { | ||
| ExecutionResult::Success { receipt, .. } => { | ||
| Some((tx.clone(), receipt.clone())) | ||
| } | ||
| ExecutionResult::Failed { .. } => None, | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let block = PendingBlockWithReceipts::new(header, receipts.into_iter()); | ||
| return Ok(Some(MaybePendingBlockWithReceipts::Pending(block))); | ||
| } | ||
| } | ||
|
|
||
| if let Some(num) = provider.convert_block_id(block_id)? { | ||
| let block = katana_rpc_types_builder::BlockBuilder::new(num.into(), provider) | ||
| .build_with_receipts()? | ||
| .map(MaybePendingBlockWithReceipts::Block); | ||
|
|
||
| StarknetApiResult::Ok(block) | ||
| } else { | ||
| StarknetApiResult::Ok(None) | ||
| } | ||
| }) | ||
| .await?; | ||
|
|
||
| if let Some(block) = block { | ||
| Ok(block) | ||
| } else if let Some(client) = &self.inner.forked_client { | ||
| Ok(client.get_block_with_receipts(block_id).await?) | ||
| } else { | ||
| Err(StarknetApiError::BlockNotFound) | ||
| } | ||
| } | ||
|
|
||
| async fn block_with_tx_hashes( | ||
| &self, | ||
| block_id: BlockIdOrTag, | ||
| ) -> StarknetApiResult<MaybePendingBlockWithTxHashes> { | ||
| let block = self | ||
| .on_io_blocking_task(move |this| { | ||
| let provider = this.inner.backend.blockchain.provider(); | ||
|
|
||
| if BlockIdOrTag::Tag(BlockTag::Pending) == block_id { | ||
| if let Some(executor) = this.pending_executor() { | ||
| let block_env = executor.read().block_env(); | ||
| let latest_hash = provider.latest_hash().map_err(StarknetApiError::from)?; | ||
|
|
||
| let l1_gas_prices = block_env.l1_gas_prices.clone(); | ||
| let l1_data_gas_prices = block_env.l1_data_gas_prices.clone(); | ||
|
|
||
| let header = PartialHeader { | ||
| l1_da_mode: L1DataAvailabilityMode::Calldata, | ||
| l1_data_gas_prices, | ||
| l1_gas_prices, | ||
| number: block_env.number, | ||
| parent_hash: latest_hash, | ||
| timestamp: block_env.timestamp, | ||
| protocol_version: this.inner.backend.chain_spec.version.clone(), | ||
| sequencer_address: block_env.sequencer_address, | ||
| }; | ||
|
|
||
| // TODO(kariy): create a method that can perform this filtering for us | ||
| // instead of doing it manually. | ||
|
|
||
| // A block should only include successful transactions, we filter out the | ||
| // failed ones (didn't pass validation stage). | ||
| let transactions = executor | ||
| .read() | ||
| .transactions() | ||
| .iter() | ||
| .filter(|(_, receipt)| receipt.is_success()) | ||
| .map(|(tx, _)| tx.hash) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let block = PendingBlockWithTxHashes::new(header, transactions); | ||
| return Ok(Some(MaybePendingBlockWithTxHashes::Pending(block))); | ||
| } | ||
| } | ||
|
|
||
| if let Some(num) = provider.convert_block_id(block_id)? { | ||
| let block = katana_rpc_types_builder::BlockBuilder::new(num.into(), provider) | ||
| .build_with_tx_hash()? | ||
| .map(MaybePendingBlockWithTxHashes::Block); | ||
|
|
||
| StarknetApiResult::Ok(block) | ||
| } else { | ||
| StarknetApiResult::Ok(None) | ||
| } | ||
| }) | ||
| .await?; | ||
|
|
||
| if let Some(block) = block { | ||
| Ok(block) | ||
| } else if let Some(client) = &self.inner.forked_client { | ||
| Ok(client.get_block_with_tx_hashes(block_id).await?) | ||
| } else { | ||
| Err(StarknetApiError::BlockNotFound) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider standardizing error handling across block methods.
Ohayo, sensei! While the block-related methods follow a consistent pattern for data retrieval (local → pending → forked), the error handling could be standardized. For example:
block_with_txsreturnsBlockNotFoundon failureblock_with_receiptsreturnsBlockNotFoundon failureblock_with_tx_hashesreturnsBlockNotFoundon failure
Consider creating a helper trait to standardize this error handling:
trait BlockResultHandler<T> {
fn handle_block_result(
result: Option<T>,
forked_client: &Option<ForkedClient>,
block_id: BlockIdOrTag,
) -> StarknetApiResult<T>;
}This would help ensure consistent error handling across all block-related methods.
| for ((num, _), _) in local_only_block { | ||
| let id = BlockIdOrTag::Number(num); | ||
|
|
||
| let block = provider.get_block_with_txs(id).await?; | ||
| assert_matches!(block, MaybePendingBlockWithTxs::Block(b) if b.block_number == num); | ||
|
|
||
| let block = provider.get_block_with_receipts(id).await?; | ||
| assert_matches!(block, starknet::core::types::MaybePendingBlockWithReceipts::Block(b) if b.block_number == num); | ||
|
|
||
| let block = provider.get_block_with_tx_hashes(id).await?; | ||
| assert_matches!(block, starknet::core::types::MaybePendingBlockWithTxHashes::Block(b) if b.block_number == num); | ||
|
|
||
| let count = provider.get_block_transaction_count(id).await?; | ||
| assert_eq!(count, 1, "all the locally generated blocks should have 1 tx"); | ||
|
|
||
| // TODO: uncomment this once we include genesis forked state update | ||
| // let state = provider.get_state_update(id).await?; | ||
| // assert_matches!(state, starknet::core::types::MaybePendingStateUpdate::Update(_)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider refactoring repetitive code in block assertions
The assertions within the loop from lines 141 to 159 are repetitive. Refactoring them into a helper function can enhance readability and reduce code duplication.
Here's how you might refactor the code:
async fn assert_block_details(provider: &impl Provider, id: BlockIdOrTag, num: BlockNumber) -> Result<()> {
let block = provider.get_block_with_txs(id).await?;
assert_matches!(block, MaybePendingBlockWithTxs::Block(b) if b.block_number == num);
let block = provider.get_block_with_receipts(id).await?;
assert_matches!(block, MaybePendingBlockWithReceipts::Block(b) if b.block_number == num);
let block = provider.get_block_with_tx_hashes(id).await?;
assert_matches!(block, MaybePendingBlockWithTxHashes::Block(b) if b.block_number == num);
let count = provider.get_block_transaction_count(id).await?;
assert_eq!(count, 1, "All the locally generated blocks should have 1 tx");
Ok(())
}Then, replace the code within the loop with a call to this function:
for ((num, _), _) in local_only_block {
let id = BlockIdOrTag::Number(num);
assert_block_details(&provider, id, num).await?;
}| let tx = provider.get_transaction_by_hash(tx_hash).await?; | ||
| assert_eq!(*tx.transaction_hash(), tx_hash); | ||
|
|
||
| let tx = provider.get_transaction_receipt(tx_hash).await?; | ||
| assert_eq!(*tx.receipt.transaction_hash(), tx_hash); | ||
|
|
||
| let result = provider.get_transaction_status(tx_hash).await; | ||
| assert!(result.is_ok()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Enhance error handling in transaction status checks
In the loop starting at line 360, the transaction status is checked, but potential errors aren't handled. Consider adding error handling to improve test robustness.
Modify the code to handle errors explicitly:
let result = provider.get_transaction_status(tx_hash).await;
match result {
Ok(status) => assert!(status.tx_status.is_confirmed()),
Err(e) => panic!("Failed to get transaction status: {:?}", e),
}
1 participant
resolves #1466
enable fetching block data from the forked chain. current forking feature only covers up to state data only. meaning doing any operations that requires only doing contract executions - estimate fee, tx execution - are allowed.
this is a pretty simple solution as it just forwards the request to the forked network provider but doesn't do any caching of the requested data.
Unsupported methods:
rpc methods that this PR doesn't yet cover.
getEventsSummary by CodeRabbit
Release Notes
New Features
ForkedClientfor enhanced interaction with Starknet blockchain forks.Bug Fixes
Documentation
Tests