refactor(katana-rpc): remove some components dependency from starknet rpc handler - #2759
Conversation
kariy
changed the title
WalkthroughOhayo, sensei! This pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/katana/node/src/lib.rs(3 hunks)crates/katana/rpc/rpc/src/starknet/config.rs(1 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(4 hunks)
🔇 Additional comments (6)
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
53-53: Great work exporting StarknetApiConfig publicly, sensei!
This allows for better modularity and flexibility in configuring the Starknet API.
1129-1136: Solid implementation of the Clone trait.
The Clone implementation correctly clones the inner Arc, ensuring proper reference counting.
crates/katana/rpc/rpc/src/starknet/config.rs (1)
1-7: Nice addition of StarknetApiConfig, sensei!
The StarknetApiConfig struct is well-defined, and the documentation clearly explains the usage of max_event_page_size.
crates/katana/node/src/lib.rs (3)
139-139: Updated node_components tuple looks good.
By removing TxValidator from the node_components, you've streamlined the node initialization. Ensure all references to TxValidator are updated accordingly.
251-254: Adjusted spawn function signature and node components.
The spawn function now accepts (TxPool, Arc<Backend<EF>>, BlockProducer<EF>, Option<ForkedClient>), aligning with the updated components. Please verify that all usages of node_components reflect this change and handle the optional forked_client appropriately.
271-271: Passing Some(block_producer.clone()) to StarknetApi::new.
Ensure that the optional block_producer is correctly handled within StarknetApi::new, and any cases where block_producer might be None are safely managed.
| blocking_task_pool: BlockingTaskPool, | ||
| block_producer: Option<BlockProducer<EF>>, |
There was a problem hiding this comment.
Sensei, ensure proper handling of None for block_producer.
Since block_producer is now an Option, please verify that all usages of block_producer in StarknetApiInner safely handle the None case to prevent potential None dereferences and runtime errors.
| self.inner.block_producer.as_ref().and_then(|bp| match &*bp.producer.read() { | ||
| BlockProducerMode::Instant(_) => None, | ||
| BlockProducerMode::Interval(producer) => Some(producer.executor()), | ||
| } | ||
| }) |
There was a problem hiding this comment.
Potential None dereference in pending_executor method.
In the pending_executor method, when accessing self.inner.block_producer.as_ref(), please ensure that the scenario where block_producer is None is properly handled to avoid panics.
| // perhaps a pending/pool state provider that implements all the state provider traits. | ||
| let result = if let BlockIdOrTag::Tag(BlockTag::Pending) = block_id { | ||
| this.inner.validator.pool_nonce(contract_address)? | ||
| this.inner.pool.validator().pool_nonce(contract_address)? |
There was a problem hiding this comment.
Careful, sensei! validator() may no longer be available.
The call to this.inner.pool.validator() might be invalid since TxValidator has been removed from StarknetApi dependencies. Please update this call to reflect the new structure or provide an alternative means to access the required functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2759 +/- ##
==========================================
- Coverage 54.69% 54.44% -0.26%
==========================================
Files 427 427
Lines 53918 54118 +200
==========================================
- Hits 29490 29463 -27
- Misses 24428 24655 +227 ☔ View full report in Codecov by Sentry. |
… rpc handler (dojoengine#2759) the idea is to make the StarknetApi struct not depend on components that are not shared in a full and sequencer node configuration.
1 participant
the idea is to make the
StarknetApistruct not depend on components that are not shared in a full and sequencer node configuration.this is just a small refactor for now and still doesn't unblock anything just yet.