refactor(katana): pending block provider #2763
Conversation
WalkthroughOhayo, sensei! This pull request introduces several significant changes across multiple files, primarily focusing on enhancing 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: 4
🧹 Outside diff range and nitpick comments (11)
crates/katana/rpc/rpc/src/starknet/mod.rs (9)
74-75: Ohayo sensei! Consider removing commented-out codeThe
block_producerfield is commented out and replaced withpending_provider. Ifblock_produceris no longer needed, removing the commented code will improve readability and maintainability.
188-195: Remove obsolete commented-out functionThe
pending_executorfunction and its associated code are commented out. If this code is obsolete due to the introduction ofpending_provider, consider removing it to keep the codebase clean and clear.
209-213: Clean up commented-out code instatemethodThe old logic using
pending_executorin thestatemethod is commented out. Since the new implementation usespending_provider, removing the commented code will enhance code clarity.
229-232: Remove commented code inblock_env_atmethodThe commented-out code within the
block_env_atmethod refers topending_executor. If it's no longer required, deleting it will improve code readability.
327-333: Delete unnecessary commented code inblock_tx_countThe old implementation using
pending_executoris commented out in theblock_tx_countmethod. Removing this code will reduce clutter and make the current logic clearer.
442-450: Remove outdated commented code intransactionmethodThe commented-out code related to
pending_executorin thetransactionmethod is no longer needed with the newpending_providerimplementation. Cleaning up will enhance maintainability.
483-502: Eliminate commented code inreceiptmethodThe
receiptmethod contains commented-out logic for thepending_executor. If this is obsolete, consider removing it to keep the codebase tidy.
1026-1028: Handle absentpending_providerinevents_innermethodWhen
pending_providerisNone, the code proceeds with a default cursor but doesn't inform the caller that no pending events can be fetched. Consider providing a clear response or error in this scenario.You might update the code to return an appropriate error:
if let Some(provider) = self.pending_provider() { let cursor = utils::events::fetch_pending_events( provider, &filter, chunk_size, cursor, &mut events, )?; let continuation_token = Some(cursor.into_rpc_cursor().to_string()); Ok(EventsPage { events, continuation_token }) } else { - let cursor = Cursor::new_block(latest + 1); - let continuation_token = Some(cursor.into_rpc_cursor().to_string()); - Ok(EventsPage { events, continuation_token }) + Err(StarknetApiError::PendingProviderNotAvailable) }Ensure that
StarknetApiErrorincludes aPendingProviderNotAvailablevariant or use an existing appropriate error.
1045-1050: Consider error handling whenpending_provideris absentIn the case where both
from_blockandto_blockarePendingandpending_providerisNone, the method returns a default continuation token without events. It may be better to return an error indicating that pending events cannot be fetched.Modify the else block to return an error:
if let Some(provider) = self.pending_provider() { // Existing logic } else { - let latest = provider.latest_number()?; - let new_cursor = Cursor::new_block(latest); - let continuation_token = Some(new_cursor.into_rpc_cursor().to_string()); - Ok(EventsPage { events, continuation_token }) + Err(StarknetApiError::PendingProviderNotAvailable) }This change helps clients handle the absence of pending events more gracefully.
crates/katana/rpc/rpc/src/utils/events.rs (1)
89-89: Ohayo, sensei! Please remove the commented-out code for clarity.The commented lines at 89 and 96 (
// pending_executor: &PendingExecutor,and// let pending_block = pending_executor.read();) are remnants of previous implementations and can be safely removed to enhance readability.Apply this diff to remove the obsolete code:
pending_provider: &impl PendingBlockProvider, filter: &Filter, chunk_size: u64, cursor: Option<Cursor>, buffer: &mut Vec<EmittedEvent>, ) -> EventQueryResult<Cursor> { - // let pending_block = pending_executor.read(); let block_env = pending_provider.pending_block_env()?; let txs = pending_provider.pending_transactions()?; let receipts = pending_provider.pending_receipts()?;Also applies to: 96-96
crates/katana/rpc/rpc/src/starknet/trace.rs (1)
170-172: Optimize Transaction Trace LookupIn the
tracefunction, checking thepending_providerfor the transaction trace is a useful addition. Consider any potential performance impacts this may have in high-load situations and optimize if necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
crates/katana/core/src/backend/mod.rs(1 hunks)crates/katana/core/src/service/block_producer.rs(2 hunks)crates/katana/node/src/lib.rs(1 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(19 hunks)crates/katana/rpc/rpc/src/starknet/read.rs(2 hunks)crates/katana/rpc/rpc/src/starknet/trace.rs(5 hunks)crates/katana/rpc/rpc/src/starknet/write.rs(3 hunks)crates/katana/rpc/rpc/src/utils/events.rs(2 hunks)crates/katana/storage/provider/src/traits/mod.rs(1 hunks)crates/katana/storage/provider/src/traits/pending.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/storage/provider/src/traits/mod.rs
🔇 Additional comments (8)
crates/katana/rpc/rpc/src/starknet/read.rs (1)
25-29: Ohayo sensei! Verify trait bounds for consistency
In the implementation of StarknetApiServer, the trait bounds specify EF: ExecutorFactory and P: PendingBlockProvider. Ensure that all methods relying on these traits are correctly constrained and that there are no unintended consequences from the new generic parameter P.
crates/katana/core/src/backend/mod.rs (1)
35-35: Ohayo sensei! Confirm the removal of trait bound on Backend
The Backend struct no longer constrains EF with ExecutorFactory. This change may affect type safety and trait implementations elsewhere. Verify that all uses of Backend still have the necessary trait bounds where required.
Run the following script to check for missing trait bounds:
This script searches for implementations and usages of Backend to ensure that the ExecutorFactory trait bounds are applied where necessary.
✅ Verification successful
Let me gather more information about the trait bounds and usage of Backend.
Ohayo sensei! The trait bound removal is safe and properly constrained!
The verification shows that while the Backend struct definition doesn't have the trait bound, all implementations and usages properly constrain it where needed:
- The main implementation in
core/src/backend/mod.rscorrectly applies the bound:impl<EF: ExecutorFactory> Backend<EF> - All usages across the codebase (in RPC, node, pipeline, services) consistently use
Backend<EF>whereEFis properly constrained by their enclosing types - The pattern follows Rust's best practice of constraining traits at the impl level rather than the struct level, which provides more flexibility while maintaining type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances where Backend is used without the necessary trait bounds.
# Search for `Backend<` usage and check for trait bounds
rg 'Backend<[^>]+>' --type rust -A 5 | grep -B 5 -A 5 -E 'impl.*Backend<[^>]+>'
Length of output: 2739
Script:
#!/bin/bash
# Search for all usages of Backend struct
rg 'Backend\s*<' --type rust -A 3
# Search for trait bounds on Backend
ast-grep --pattern 'impl<$_> Backend<$_> where $_'
Length of output: 12091
crates/katana/node/src/lib.rs (1)
274-274: Ohayo sensei! Adjust constructor parameters to match signature
In the spawn function, StarknetApi::new is called with block_producer.clone(). Ensure that this matches the updated constructor signature and that block_producer implements the required traits for P.
crates/katana/core/src/service/block_producer.rs (1)
829-831: Ohayo, sensei! Verify if returning empty vectors for pending transactions and receipts is appropriate.
In InstantBlockProducer, the methods pending_transactions() and pending_receipts() return empty vectors. If there are pending transactions or receipts, they should be returned accordingly. Please confirm whether this is the intended behavior.
Also applies to: 833-835
crates/katana/rpc/rpc/src/starknet/write.rs (1)
15-19: Ohayo, sensei! Verify Generic Parameter Addition
The introduction of the generic parameter P constrained by PendingBlockProvider updates the StarknetApi struct. Please ensure all instances of StarknetApi throughout the codebase include this new parameter to prevent any compilation issues.
crates/katana/rpc/rpc/src/starknet/trace.rs (3)
24-28: Ohayo, sensei! Review New Generic Constraints
Lines 24-28 introduce the generic parameter P: PendingBlockProvider to StarknetApi. Verify that this change integrates smoothly with existing generic constraints and doesn't introduce conflicting requirements.
185-189: Ensure Trait Implementation Consistency
The StarknetTraceApiServer implementation now includes the generic parameter P. Confirm that all implementations of this trait and related traits are updated to include P, maintaining consistency across the codebase.
124-139:
Handle Pending Block Logic Carefully
The block_traces function now attempts to fetch pending transactions and traces using the pending_provider. Ensure that when no pending block is available, the fallback to the latest block functions as intended without unintended side effects.
| if let Some(provider) = this.pending_provider() { | ||
| Ok(provider.pending_transaction_status(hash)?) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct handling of missing pending_provider in transaction_status
In the transaction_status method, when pending_provider is None, the code currently returns Ok(None). To accurately reflect the absence of the transaction, consider returning Err(StarknetApiError::TxnHashNotFound) instead.
Apply this diff to correct the error handling:
if let Some(provider) = this.pending_provider() {
Ok(provider.pending_transaction_status(hash)?)
} else {
- // Err(StarknetApiError::TxnHashNotFound)
- Ok(None)
+ Err(StarknetApiError::TxnHashNotFound)
}Committable suggestion skipped: line range outside the PR's diff.
| fn pending_block(&self) -> ProviderResult<()>; | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! The pending_block method should return pending block data.
Currently, the pending_block method returns ProviderResult<()>, which doesn't convey any meaningful information about the pending block. Consider changing the return type to ProviderResult<Block>, or an appropriate block representation, to provide access to the pending block data.
Here's the suggested change:
- fn pending_block(&self) -> ProviderResult<()>;
+ fn pending_block(&self) -> ProviderResult<Block>;📝 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.
| fn pending_block(&self) -> ProviderResult<()>; | |
| fn pending_block(&self) -> ProviderResult<Block>; |
| fn pending_block(&self) -> ProviderResult<()> { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! The pending_block method is unimplemented and needs attention.
In both InstantBlockProducer and IntervalBlockProducer, the pending_block method contains a todo!() macro. This indicates that the method is unimplemented. Please provide a suitable implementation to fulfill the PendingBlockProvider trait requirements.
Also applies to: 887-889
| impl<EF, P> StarknetWriteApiServer for StarknetApi<EF, P> | ||
| where | ||
| EF: ExecutorFactory, | ||
| P: PendingBlockProvider, | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm Consistency in Trait Implementations
The implementation of StarknetWriteApiServer now includes the generic parameter P. Double-check that all related implementations and usages are updated accordingly to maintain consistency.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2763 +/- ##
==========================================
+ Coverage 55.50% 55.54% +0.03%
==========================================
Files 434 435 +1
Lines 54295 54436 +141
==========================================
+ Hits 30138 30237 +99
- Misses 24157 24199 +42 ☔ 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)
crates/katana/storage/provider/src/traits/pending.rs (1)
16-16: Ohayo, sensei! Consider movingPendingBlockProviderto therpccrate.The TODO comment on line 16 suggests that the
PendingBlockProvidertrait might be better placed in therpccrate since it's primarily used on the RPC side. Relocating it could improve code organization and maintainability.Would you like assistance in moving this trait to the
rpccrate?crates/katana/core/src/service/block_producer.rs (1)
830-830: Ohayo, sensei! Remove thedbg!macro from production code.The
dbg!macro on line 830 is intended for debugging and should be removed to prevent unintended output in production.Apply this diff to remove the
dbg!macro:- if let Some(receipt) = dbg!(self.backend.blockchain.provider().receipt_by_hash(hash)?) { + if let Some(receipt) = self.backend.blockchain.provider().receipt_by_hash(hash)? {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/katana/core/src/service/block_producer.rs(2 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(20 hunks)crates/katana/rpc/rpc/src/utils/events.rs(2 hunks)crates/katana/storage/provider/src/traits/pending.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/rpc/rpc/src/utils/events.rs
🔇 Additional comments (4)
crates/katana/storage/provider/src/traits/pending.rs (1)
21-22: Ohayo, sensei! The pending_block method should return pending block data.
Currently, the pending_block method returns ProviderResult<()>, which doesn't provide information about the pending block. Consider changing the return type to ProviderResult<Block> or an appropriate block representation to access the pending block data.
crates/katana/core/src/service/block_producer.rs (2)
818-820: Ohayo, sensei! Implement the pending_block method for InstantBlockProducer.
The pending_block method currently contains a todo!() macro, indicating it's unimplemented. Please provide an implementation to fulfill the PendingBlockProvider trait requirements.
887-889: Ohayo, sensei! Implement the pending_block method for IntervalBlockProducer.
Similar to InstantBlockProducer, the pending_block method here contains a todo!() macro. An implementation is needed to meet the PendingBlockProvider trait obligations.
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
516-517: Ohayo, sensei! Correct error handling when pending_provider is missing in transaction_status.
When pending_provider is None, the method currently returns Ok(None). To accurately indicate that the transaction hash was not found, return Err(StarknetApiError::TxnHashNotFound) instead.
Apply this diff to correct the error handling:
if let Some(provider) = this.pending_provider() {
Ok(provider.pending_transaction_status(hash)?)
} else {
- // Err(StarknetApiError::TxnHashNotFound)
- Ok(None)
+ Err(StarknetApiError::TxnHashNotFound)
}
yes, right now we're polling the gateway at a every fixed interval. the gateway doesn't provide a way to subscribe to this change. dojo/crates/katana/node/src/full/tip_watcher.rs Lines 34 to 47 in ea0973c |
kariy
force-pushed
the
katana/pending-block-provider
branch
from
a537e82 to
e1559da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
crates/katana/core/src/backend/mod.rs(1 hunks)crates/katana/core/src/service/block_producer.rs(2 hunks)crates/katana/node/src/lib.rs(1 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(20 hunks)crates/katana/rpc/rpc/src/starknet/read.rs(2 hunks)crates/katana/rpc/rpc/src/starknet/trace.rs(5 hunks)crates/katana/rpc/rpc/src/starknet/write.rs(3 hunks)crates/katana/rpc/rpc/src/utils/events.rs(2 hunks)crates/katana/storage/provider/src/traits/mod.rs(1 hunks)crates/katana/storage/provider/src/traits/pending.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/katana/node/src/lib.rs
- crates/katana/storage/provider/src/traits/mod.rs
- crates/katana/rpc/rpc/src/starknet/read.rs
- crates/katana/rpc/rpc/src/utils/events.rs
- crates/katana/core/src/backend/mod.rs
- crates/katana/rpc/rpc/src/starknet/write.rs
- crates/katana/storage/provider/src/traits/pending.rs
🔇 Additional comments (6)
crates/katana/rpc/rpc/src/starknet/trace.rs (3)
Line range hint 8-31: Ohayo, sensei! Clean trait abstraction!
The addition of the PendingBlockProvider trait and the generic parameter P to StarknetApi provides a clean abstraction for fetching pending block data.
Line range hint 183-215: Ohayo! Robust pending state handling!
The implementation gracefully handles both cases:
- When pending_provider is available, uses it to fetch pending state/block env
- When pending_provider is None, falls back to latest state/block env
Line range hint 516-517: Ohayo, sensei! Consider proper error handling when pending_provider is None
The current implementation returns Ok(None) when pending_provider is None, but this might mask the actual state. As suggested in a previous review, consider returning Err(StarknetApiError::TxnHashNotFound) to accurately reflect that the transaction cannot be found.
Apply this diff:
if let Some(provider) = this.pending_provider() {
Ok(provider.pending_transaction_status(hash)?)
} else {
- Ok(None)
+ Err(StarknetApiError::TxnHashNotFound)
}crates/katana/core/src/service/block_producer.rs (2)
794-812: Ohayo! Well-documented implementation!
The pending_block_env implementation is thoroughly documented and correctly handles block timestamp updates, which is crucial for accurate fee estimation in instant mining mode.
818-820:
Ohayo! The pending_block method remains unimplemented
The pending_block method contains todo!() in both InstantBlockProducer and IntervalBlockProducer. This was previously flagged and needs to be addressed.
Would you like assistance in implementing these methods?
Also applies to: 892-894
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
Line range hint 973-995: Ohayo! Robust event fetching implementation!
The implementation properly handles pending events with:
- Correct cursor management for pagination
- Proper error handling
- Clean integration with the pending provider abstraction
| &self, | ||
| hash: TxHash, | ||
| ) -> ProviderResult<Option<TransactionStatus>> { | ||
| if let Some(receipt) = dbg!(self.backend.blockchain.provider().receipt_by_hash(hash)?) { |
There was a problem hiding this comment.
Ohayo! Remove debug print
The dbg! macro should be removed from production code.
Apply this diff:
- if let Some(receipt) = dbg!(self.backend.blockchain.provider().receipt_by_hash(hash)?) {
+ if let Some(receipt) = self.backend.blockchain.provider().receipt_by_hash(hash)? {📝 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.
| if let Some(receipt) = dbg!(self.backend.blockchain.provider().receipt_by_hash(hash)?) { | |
| if let Some(receipt) = self.backend.blockchain.provider().receipt_by_hash(hash)? { |
The motivation is to abstract how pending block data is being fetched in the rpc server. This is to support having different source for where the pending block will be provided from. Right now in sequencing mode, the pending block will be provided by the block producer's state. But in the case for a full node, where block production doesn't happen locally, the pending block information needs to be fetched from the feeder gateway (as right now we want to support syncing from fgw). ref #2724
Summary by CodeRabbit
Release Notes
New Features
PendingBlockProvidertrait to enhance functionality related to pending blocks, allowing access to pending transactions, receipts, and their statuses.StarknetApito utilize the newPendingBlockProvider, improving interaction with pending transactions.BlockProducerwith methods for accessing pending transactions and their statuses.pendingmodule to encapsulate related functionalities in the storage provider.Bug Fixes
Documentation
PendingBlockProvidertrait and its methods.