refactor(katana-rpc): move API implementations from Sequencer to their dedicated server struct
#2209
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2209 +/- ##
=======================================
Coverage 69.35% 69.36%
=======================================
Files 340 340
Lines 44509 44429 -80
=======================================
- Hits 30869 30816 -53
+ Misses 13640 13613 -27 ☔ View full report in Codecov by Sentry. |
kariy
force-pushed
the
move-implementations
branch
from
7bcca44 to
8797a8c
Compare
WalkthroughOhayo, sensei! The recent changes bring substantial refinements to our codebase, emphasizing improved encapsulation and a clearer API interface. Key updates include transitioning from direct property access to method calls for backend interactions, revamping error handling strategies, and modifying the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DevApi
participant KatanaSequencer
participant Blockchain
User->>DevApi: Request to set next block timestamp
DevApi->>KatanaSequencer: Check for pending transactions
KatanaSequencer-->>DevApi: Return pending transaction status
DevApi-->>User: Return error if pending transactions exist
DevApi->>KatanaSequencer: Set next block timestamp
KatanaSequencer->>Blockchain: Update block timestamp
Blockchain-->>KatanaSequencer: Confirm update
KatanaSequencer-->>DevApi: Return success
DevApi-->>User: Confirm timestamp set
sequenceDiagram
participant User
participant StarknetApi
participant Backend
User->>StarknetApi: Request block state
StarknetApi->>Backend: Fetch block information
Backend-->>StarknetApi: Return block data
StarknetApi-->>User: Provide block state information
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (17)
Files skipped from review due to trivial changes (2)
Files skipped from review as they are similar to previous changes (12)
Additional comments not posted (17)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range, codebase verification and nitpick comments (1)
crates/katana/rpc/rpc/src/dev.rs (1)
20-26: Ohayo, sensei!Add test coverage for
has_pending_transactions.It appears that there are no existing test files covering the
has_pending_transactionsfunction. To ensure the robustness of your code, please add appropriate test cases for this function.
- File:
crates/katana/rpc/rpc/src/dev.rs- Function:
has_pending_transactionsAnalysis chain
LGTM! But add test coverage for
has_pending_transactions.The function correctly checks for pending transactions. However, the line is not covered by tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for `has_pending_transactions`. # Test: Search for test cases that cover `has_pending_transactions`. Expect: At least one test case. rg --type rust -A 5 $'has_pending_transactions'Length of output: 1427
Script:
#!/bin/bash # Description: Verify if there are any test files covering `has_pending_transactions`. # Search for test files in the repository fd --type f --extension rs --exec rg 'has_pending_transactions' {}Length of output: 207
Script:
#!/bin/bash # Description: Verify if there are any test files covering `has_pending_transactions`. # Search for test files in the repository fd --type f --extension rs --exec rg 'has_pending_transactions' {} || echo "No test files found covering has_pending_transactions"Length of output: 324
Tools
GitHub Check: codecov/patch
[warning] 22-22: crates/katana/rpc/rpc/src/dev.rs#L22
Added line #L22 was not covered by tests
| #[derive(thiserror::Error, Clone, Copy, Debug)] | ||
| #[allow(clippy::enum_variant_names)] | ||
| pub enum DevApiError { | ||
| #[error("Wait for pending transactions.")] | ||
| PendingTransactions, | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! The DevApiError enum looks good.
The enum is defined correctly with appropriate derived traits.
However, the static analysis tools indicate that this code is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 5-5: crates/katana/rpc/rpc-types/src/error/dev.rs#L5
Added line #L5 was not covered by tests
| impl From<DevApiError> for Error { | ||
| fn from(err: DevApiError) -> Self { | ||
| Error::Call(CallError::Custom(ErrorObject::owned(err as i32, err.to_string(), None::<()>))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! The From implementation for DevApiError looks good.
The conversion to Error using a custom ErrorObject is handled correctly.
However, the static analysis tools indicate that this code is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 13-15: crates/katana/rpc/rpc-types/src/error/dev.rs#L13-L15
Added lines #L13 - L15 were not covered by tests
| impl From<ContinuationTokenError> for StarknetApiError { | ||
| fn from(value: ContinuationTokenError) -> Self { | ||
| match value { | ||
| SequencerError::BlockNotFound(_) => StarknetApiError::BlockNotFound, | ||
| SequencerError::ContractNotFound(_) => StarknetApiError::ContractNotFound, | ||
| err => StarknetApiError::UnexpectedError { reason: err.to_string() }, | ||
| ContinuationTokenError::InvalidToken => StarknetApiError::InvalidContinuationToken, | ||
| ContinuationTokenError::ParseFailed(e) => { | ||
| StarknetApiError::UnexpectedError { reason: e.to_string() } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Improved error handling logic.
The conversion from ContinuationTokenError to StarknetApiError enhances clarity and robustness. However, these lines are not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 142-142: crates/katana/rpc/rpc-types/src/error/starknet.rs#L142
Added line #L142 was not covered by tests
[warning] 144-146: crates/katana/rpc/rpc-types/src/error/starknet.rs#L144-L146
Added lines #L144 - L146 were not covered by tests
| fn from(value: anyhow::Error) -> Self { | ||
| StarknetApiError::UnexpectedError { reason: value.to_string() } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Enhanced error reporting.
The conversion from anyhow::Error to StarknetApiError improves the robustness of error reporting by capturing unexpected errors. However, these lines are not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 153-155: crates/katana/rpc/rpc-types/src/error/starknet.rs#L153-L155
Added lines #L153 - L155 were not covered by tests
| fn state(&self, block_id: &BlockIdOrTag) -> Result<Box<dyn StateProvider>, StarknetApiError> { | ||
| let provider = self.inner.sequencer.backend().blockchain.provider(); | ||
|
|
||
| let state = match block_id { | ||
| BlockIdOrTag::Tag(BlockTag::Latest) => Some(provider.latest()?), | ||
|
|
||
| BlockIdOrTag::Tag(BlockTag::Pending) => { | ||
| if let Some(exec) = self.inner.sequencer.pending_executor() { | ||
| Some(exec.read().state()) | ||
| } else { | ||
| Some(provider.latest()?) | ||
| } | ||
| } | ||
|
|
||
| BlockIdOrTag::Hash(hash) => provider.historical((*hash).into())?, | ||
| BlockIdOrTag::Number(num) => provider.historical((*num).into())?, | ||
| }; | ||
|
|
||
| state.ok_or(StarknetApiError::BlockNotFound) | ||
| } |
There was a problem hiding this comment.
LGTM! State retrieval logic is correct, but add tests.
The state function correctly retrieves the state for a given block ID.
However, the function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 137-138: crates/katana/rpc/rpc/src/starknet/mod.rs#L137-L138
Added lines #L137 - L138 were not covered by tests
| async fn class_at_address( | ||
| &self, | ||
| block_id: BlockIdOrTag, | ||
| contract_address: ContractAddress, | ||
| ) -> Result<ContractClass, StarknetApiError> { | ||
| let hash = self.class_hash_at_address(block_id, contract_address).await?; | ||
| let class = self.class_at_hash(block_id, hash).await?; | ||
| Ok(class) | ||
| } |
There was a problem hiding this comment.
LGTM! Contract class retrieval logic is correct, but add tests.
The class_at_address function correctly retrieves the contract class for a given contract address.
However, the function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 217-225: crates/katana/rpc/rpc/src/starknet/mod.rs#L217-L225
Added lines #L217 - L225 were not covered by tests
| fn storage_at( | ||
| &self, | ||
| contract_address: ContractAddress, | ||
| storage_key: StorageKey, | ||
| block_id: BlockIdOrTag, | ||
| ) -> Result<StorageValue, StarknetApiError> { | ||
| let state = self.state(&block_id)?; | ||
|
|
||
| // check that contract exist by checking the class hash of the contract | ||
| let Some(_) = state.class_hash_of_contract(contract_address)? else { | ||
| return Err(StarknetApiError::ContractNotFound); | ||
| }; | ||
|
|
||
| let value = state.storage(contract_address, storage_key)?; | ||
| Ok(value.unwrap_or_default()) | ||
| } |
There was a problem hiding this comment.
LGTM! Storage value retrieval logic is correct, but add tests.
The storage_at function correctly retrieves the storage value for a given contract address and storage key.
However, the function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 227-233: crates/katana/rpc/rpc/src/starknet/mod.rs#L227-L233
Added lines #L227 - L233 were not covered by tests
[warning] 236-237: crates/katana/rpc/rpc/src/starknet/mod.rs#L236-L237
Added lines #L236 - L237 were not covered by tests
[warning] 240-242: crates/katana/rpc/rpc/src/starknet/mod.rs#L240-L242
Added lines #L240 - L242 were not covered by tests
| fn block_tx_count(&self, block_id: BlockIdOrTag) -> Result<u64, StarknetApiError> { | ||
| let provider = self.inner.sequencer.backend().blockchain.provider(); | ||
|
|
||
| let block_id: BlockHashOrNumber = match block_id { | ||
| BlockIdOrTag::Tag(BlockTag::Pending) => match self.inner.sequencer.pending_executor() { | ||
| Some(exec) => return Ok(exec.read().transactions().len() as u64), | ||
| None => provider.latest_hash()?.into(), | ||
| }, | ||
| BlockIdOrTag::Tag(BlockTag::Latest) => provider.latest_number()?.into(), | ||
| BlockIdOrTag::Number(num) => num.into(), | ||
| BlockIdOrTag::Hash(hash) => hash.into(), | ||
| }; | ||
|
|
||
| let count = provider | ||
| .transaction_count_by_block(block_id)? | ||
| .ok_or(StarknetApiError::BlockNotFound)?; | ||
|
|
||
| Ok(count) | ||
| } |
There was a problem hiding this comment.
LGTM! Transaction count retrieval logic is correct, but add tests.
The block_tx_count function correctly retrieves the transaction count for a given block ID.
However, the function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 244-245: crates/katana/rpc/rpc/src/starknet/mod.rs#L244-L245
Added lines #L244 - L245 were not covered by tests
[warning] 247-250: crates/katana/rpc/rpc/src/starknet/mod.rs#L247-L250
Added lines #L247 - L250 were not covered by tests
[warning] 252-254: crates/katana/rpc/rpc/src/starknet/mod.rs#L252-L254
Added lines #L252 - L254 were not covered by tests
[warning] 257-259: crates/katana/rpc/rpc/src/starknet/mod.rs#L257-L259
Added lines #L257 - L259 were not covered by tests
[warning] 261-262: crates/katana/rpc/rpc/src/starknet/mod.rs#L261-L262
Added lines #L261 - L262 were not covered by tests
| async fn latest_block_number(&self) -> Result<BlockNumber, StarknetApiError> { | ||
| self.on_io_blocking_task(move |this| { | ||
| Ok(this.inner.sequencer.backend().blockchain.provider().latest_number()?) | ||
| }) | ||
| .await | ||
| } |
There was a problem hiding this comment.
LGTM! Latest block number retrieval logic is correct, but add tests.
The latest_block_number function correctly retrieves the latest block number.
However, the function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 264-269: crates/katana/rpc/rpc/src/starknet/mod.rs#L264-L269
Added lines #L264 - L269 were not covered by tests
| async fn transaction(&self, hash: TxHash) -> Result<TxWithHash, StarknetApiError> { | ||
| self.on_io_blocking_task(move |this| { | ||
| let tx = | ||
| this.inner.sequencer.backend().blockchain.provider().transaction_by_hash(hash)?; | ||
|
|
||
| let tx = match tx { | ||
| tx @ Some(_) => tx, | ||
| None => { | ||
| // check if the transaction is in the pending block | ||
| this.inner.sequencer.pending_executor().as_ref().and_then(|exec| { | ||
| exec.read() | ||
| .transactions() | ||
| .iter() | ||
| .find(|(tx, _)| tx.hash == hash) | ||
| .map(|(tx, _)| tx.clone()) | ||
| }) | ||
| } | ||
| }; | ||
|
|
||
| tx.ok_or(StarknetApiError::TxnHashNotFound) | ||
| }) | ||
| .await |
There was a problem hiding this comment.
LGTM! Transaction retrieval logic is correct, but add tests.
The transaction function correctly retrieves the transaction for a given hash.
However, the function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 284-287: crates/katana/rpc/rpc/src/starknet/mod.rs#L284-L287
Added lines #L284 - L287 were not covered by tests
[warning] 289-290: crates/katana/rpc/rpc/src/starknet/mod.rs#L289-L290
Added lines #L289 - L290 were not covered by tests
kariy
force-pushed
the
move-implementations
branch
from
8797a8c to
fb1c943
Compare
| let tx = tx.into_tx_with_chain_id(this.inner.sequencer.backend().chain_id); | ||
| let contract_address = tx.contract_address(); | ||
|
|
||
| let tx = ExecutableTxWithHash::new(ExecutableTx::DeployAccount(tx)); | ||
| let tx_hash = tx.hash; | ||
|
|
||
| this.inner.sequencer.add_transaction_to_pool(tx); | ||
|
|
||
| this.inner.sequencer.pool().add_transaction(tx); |
There was a problem hiding this comment.
Validate transaction before adding to the pool.
Consider adding validation logic before adding the transaction to the pool to ensure it meets all necessary criteria.
+ // Validate transaction before adding to the pool
+ if !tx.is_valid() {
+ return Err(StarknetApiError::InvalidTransaction);
+ }Committable suggestion was skipped due to low confidence.
| let tx = tx.into_tx_with_chain_id(this.inner.sequencer.backend().chain_id); | ||
| let tx = ExecutableTxWithHash::new(ExecutableTx::Invoke(tx)); | ||
| let tx_hash = tx.hash; | ||
|
|
||
| this.inner.sequencer.add_transaction_to_pool(tx); | ||
|
|
||
| this.inner.sequencer.pool().add_transaction(tx); |
There was a problem hiding this comment.
Validate transaction before adding to the pool.
Consider adding validation logic before adding the transaction to the pool to ensure it meets all necessary criteria.
+ // Validate transaction before adding to the pool
+ if !tx.is_valid() {
+ return Err(StarknetApiError::InvalidTransaction);
+ }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.
| let tx = tx.into_tx_with_chain_id(this.inner.sequencer.backend().chain_id); | |
| let tx = ExecutableTxWithHash::new(ExecutableTx::Invoke(tx)); | |
| let tx_hash = tx.hash; | |
| this.inner.sequencer.add_transaction_to_pool(tx); | |
| this.inner.sequencer.pool().add_transaction(tx); | |
| let tx = tx.into_tx_with_chain_id(this.inner.sequencer.backend().chain_id); | |
| let tx = ExecutableTxWithHash::new(ExecutableTx::Invoke(tx)); | |
| let tx_hash = tx.hash; | |
| // Validate transaction before adding to the pool | |
| if !tx.is_valid() { | |
| return Err(StarknetApiError::InvalidTransaction); | |
| } | |
| this.inner.sequencer.pool().add_transaction(tx); |
| let tx = tx | ||
| .try_into_tx_with_chain_id(this.inner.sequencer.backend().chain_id) | ||
| .map_err(|_| StarknetApiError::InvalidContractClass)?; | ||
|
|
||
| let class_hash = tx.class_hash(); | ||
| let tx = ExecutableTxWithHash::new(ExecutableTx::Declare(tx)); | ||
| let tx_hash = tx.hash; | ||
|
|
||
| this.inner.sequencer.add_transaction_to_pool(tx); | ||
|
|
||
| this.inner.sequencer.pool().add_transaction(tx); |
There was a problem hiding this comment.
Validate transaction before adding to the pool.
Consider adding validation logic before adding the transaction to the pool to ensure it meets all necessary criteria.
+ // Validate transaction before adding to the pool
+ if !tx.is_valid() {
+ return Err(StarknetApiError::InvalidTransaction);
+ }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.
| let tx = tx | |
| .try_into_tx_with_chain_id(this.inner.sequencer.backend().chain_id) | |
| .map_err(|_| StarknetApiError::InvalidContractClass)?; | |
| let class_hash = tx.class_hash(); | |
| let tx = ExecutableTxWithHash::new(ExecutableTx::Declare(tx)); | |
| let tx_hash = tx.hash; | |
| this.inner.sequencer.add_transaction_to_pool(tx); | |
| this.inner.sequencer.pool().add_transaction(tx); | |
| let tx = tx | |
| .try_into_tx_with_chain_id(this.inner.sequencer.backend().chain_id) | |
| .map_err(|_| StarknetApiError::InvalidContractClass)?; | |
| let class_hash = tx.class_hash(); | |
| let tx = ExecutableTxWithHash::new(ExecutableTx::Declare(tx)); | |
| let tx_hash = tx.hash; | |
| // Validate transaction before adding to the pool | |
| if !tx.is_valid() { | |
| return Err(StarknetApiError::InvalidTransaction); | |
| } | |
| this.inner.sequencer.pool().add_transaction(tx); |
| self.on_io_blocking_task(move |this| { | ||
| let res = this.block_hash_and_number()?; | ||
| Ok(res.into()) | ||
| }) | ||
| .await |
There was a problem hiding this comment.
Consider removing on_io_blocking_task.
The logic inside the method is simple enough that on_io_blocking_task might not be necessary.
- self.on_io_blocking_task(move |this| {
- let res = this.block_hash_and_number()?;
- Ok(res.into())
- }).await
+ Ok(self.block_hash_and_number()?.into())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.
| self.on_io_blocking_task(move |this| { | |
| let res = this.block_hash_and_number()?; | |
| Ok(res.into()) | |
| }) | |
| .await | |
| Ok(self.block_hash_and_number()?.into()) |
| Ok(count) | ||
| }) | ||
| .await | ||
| self.on_io_blocking_task(move |this| Ok(this.block_tx_count(block_id)?)).await |
There was a problem hiding this comment.
Consider removing on_io_blocking_task.
The logic inside the method is simple enough that on_io_blocking_task might not be necessary.
- self.on_io_blocking_task(move |this| Ok(this.block_tx_count(block_id)?)).await
+ Ok(self.block_tx_count(block_id)?)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.
| self.on_io_blocking_task(move |this| Ok(this.block_tx_count(block_id)?)).await | |
| Ok(self.block_tx_count(block_id)?) |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests