refactor(katana-rpc): starknet rpc clean up #2205
Conversation
WalkthroughOhayo, sensei! The recent changes enhance the Starknet API by introducing new features for transaction writing and tracing while improving the overall modularity of the code. Key modifications include the addition of new traits for transaction management and tracing, updates to existing methods for clarity, and a restructuring of the API server to support multiple functionalities concurrently, ultimately streamlining the integration process. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as StarknetAPI
participant Trace as StarknetTraceApi
participant Write as StarknetWriteApi
Client->>API: Request transaction simulation
API->>Write: add_invoke_transaction
Write-->>API: Return transaction hash
API->>Trace: simulate
Trace-->>API: Return simulation results
API-->>Client: Return final results
sequenceDiagram
participant Client
participant API as StarknetAPI
participant Trace as StarknetTraceApi
Client->>API: Request trace for transaction
API->>Trace: trace
Trace-->>API: Return trace results
API-->>Client: Return trace results
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (8)
Additional comments not posted (22)
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: 3
Outside diff range, codebase verification and nitpick comments (16)
crates/katana/rpc/rpc/src/starknet/trace.rs (2)
74-75: Clarify the comment on transaction validation.The comment on transaction validation could be clarified for better understanding.
- // If the node is run with transaction validation disabled, then we should not validate - // even if the `SKIP_VALIDATE` flag is not set. + // If the node is run with transaction validation disabled, we should not validate transactions + // even if the `SKIP_VALIDATE` flag is not set.
78-79: Clarify the comment on fee charging.The comment on fee charging could be clarified for better understanding.
- // If the node is run with fee charge disabled, then we should disable charing fees even - // if the `SKIP_FEE_CHARGE` flag is not set. + // If the node is run with fee charge disabled, we should disable charging fees + // even if the `SKIP_FEE_CHARGE` flag is not set.crates/katana/rpc/rpc/src/starknet/read.rs (14)
Line range hint
40-46:
Handle potential errors innonce_at.The
nonce_atmethod call should handle potential errors more gracefully.- let nonce = this - .inner - .sequencer - .nonce_at(block_id, contract_address.into()) - .map_err(StarknetApiError::from)? - .ok_or(StarknetApiError::ContractNotFound)?; + let nonce = this + .inner + .sequencer + .nonce_at(block_id, contract_address.into()) + .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? + .ok_or(StarknetApiError::ContractNotFound)?;
Line range hint
54-60:
Handle potential errors intransaction.The
transactionmethod call should handle potential errors more gracefully.- let tx = this - .inner - .sequencer - .transaction(&transaction_hash) - .map_err(StarknetApiError::from)? - .ok_or(StarknetApiError::TxnHashNotFound)?; + let tx = this + .inner + .sequencer + .transaction(&transaction_hash) + .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? + .ok_or(StarknetApiError::TxnHashNotFound)?;
Line range hint
63-69:
Handle potential errors inblock_tx_count.The
block_tx_countmethod call should handle potential errors more gracefully.- let count = this - .inner - .sequencer - .block_tx_count(block_id) - .map_err(StarknetApiError::from)? - .ok_or(StarknetApiError::BlockNotFound)?; + let count = this + .inner + .sequencer + .block_tx_count(block_id) + .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? + .ok_or(StarknetApiError::BlockNotFound)?;
Line range hint
72-79:
Handle potential errors inclass_hash_at.The
class_hash_atmethod call should handle potential errors more gracefully.- let class_hash = self - .on_io_blocking_task(move |this| { - this.inner - .sequencer - .class_hash_at(block_id, contract_address.into()) - .map_err(StarknetApiError::from)? - .ok_or(StarknetApiError::ContractNotFound) - }) - .await?; + let class_hash = self + .on_io_blocking_task(move |this| { + this.inner + .sequencer + .class_hash_at(block_id, contract_address.into()) + .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? + .ok_or(StarknetApiError::ContractNotFound) + }) + .await?;
Line range hint
90-92:
Handle potential errors inlatest_hash.The
latest_hashmethod call should handle potential errors more gracefully.- let latest_hash = provider.latest_hash().map_err(StarknetApiError::from)?; + let latest_hash = provider.latest_hash().map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })?;
Line range hint
98-99:
Clarify the comment on transaction filtering.The comment on transaction filtering could be clarified for better understanding.
- // TODO(kariy): create a method that can perform this filtering for us instead - // of doing it manually. + // TODO(kariy): Create a method to perform this filtering automatically.
Line range hint
126-127:
Handle potential errors inconvert_block_id.The
convert_block_idmethod call should handle potential errors more gracefully.- let block_num = BlockIdReader::convert_block_id(provider, block_id) - .map_err(StarknetApiError::from)? - .map(BlockHashOrNumber::Num) - .ok_or(StarknetApiError::BlockNotFound)?; + let block_num = BlockIdReader::convert_block_id(provider, block_id) + .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? + .map(BlockHashOrNumber::Num) + .ok_or(StarknetApiError::BlockNotFound)?;
Line range hint
129-131:
Handle potential errors inconvert_block_id.The
convert_block_idmethod call should handle potential errors more gracefully.- let block_num = BlockIdReader::convert_block_id(provider, block_id) - .map_err(StarknetApiError::from)? - .map(BlockHashOrNumber::Num) - .ok_or(StarknetApiError::BlockNotFound)?; + let block_num = BlockIdReader::convert_block_id(provider, block_id) + .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? + .map(BlockHashOrNumber::Num) + .ok_or(StarknetApiError::BlockNotFound)?;
Line range hint
134-136:
Handle potential errors inlatest_hash.The
latest_hashmethod call should handle potential errors more gracefully.- let latest_hash = provider.latest_hash().map_err(StarknetApiError::from)?; + let latest_hash = provider.latest_hash().map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })?;
Line range hint
142-143:
Clarify the comment on transaction filtering.The comment on transaction filtering could be clarified for better understanding.
- // TODO(kariy): create a method that can perform this filtering for us instead - // of doing it manually. + // TODO(kariy): Create a method to perform this filtering automatically.
Line range hint
170-171:
Handle potential errors inconvert_block_id.The
convert_block_idmethod call should handle potential errors more gracefully.- let block_num = BlockIdReader::convert_block_id(provider, block_id) - .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? - .map(BlockHashOrNumber::Num) - .ok_or(StarknetApiError::BlockNotFound)?; + let block_num = BlockIdReader::convert_block_id(provider, block_id) + .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? + .map(BlockHashOrNumber::Num) + .ok_or(StarknetApiError::BlockNotFound)?;
Line range hint
176-178:
Handle potential errors inlatest_hash.The
latest_hashmethod call should handle potential errors more gracefully.- let latest_hash = provider.latest_hash().map_err(StarknetApiError::from)?; + let latest_hash = provider.latest_hash().map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })?;
Line range hint
184-185:
Clarify the comment on transaction filtering.The comment on transaction filtering could be clarified for better understanding.
- // TODO(kariy): create a method that can perform this filtering for us instead - // of doing it manually. + // TODO(kariy): Create a method to perform this filtering automatically.
Line range hint
224-225:
Handle potential errors inconvert_block_id.The
convert_block_idmethod call should handle potential errors more gracefully.- let </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // TODO: merge these into a single logic. | ||
| let server = StarknetApi::new(sequencer.clone()); | ||
| methods.merge(StarknetApiServer::into_rpc(server.clone()))?; | ||
| methods.merge(StarknetWriteApiServer::into_rpc(server.clone()))?; | ||
| methods.merge(StarknetTraceApiServer::into_rpc(server))?; |
There was a problem hiding this comment.
Ohayo, sensei! The changes look good, but consider merging the logic.
The updates to handle StarknetApiServer, StarknetWriteApiServer, and StarknetTraceApiServer are correct. However, the TODO comment suggests merging these into a single logic block for better maintainability.
// TODO: Merge these into a single logic.
let server = StarknetApi::new(sequencer.clone());
methods.merge(StarknetApiServer::into_rpc(server.clone()))?;
methods.merge(StarknetWriteApiServer::into_rpc(server.clone()))?;
methods.merge(StarknetTraceApiServer::into_rpc(server))?;| // TODO: compute the state diff | ||
| let state_diff = None; |
There was a problem hiding this comment.
Implement state diff computation.
The TODO comment indicates that state diff computation is missing.
Do you want me to implement the state diff computation or open a GitHub issue to track this task?
| let state = sequencer.state(&block_id).map_err(StarknetApiError::from)?; | ||
| let env = sequencer | ||
| .block_env_at(block_id) | ||
| .map_err(StarknetApiError::from)? | ||
| .ok_or(StarknetApiError::BlockNotFound)?; |
There was a problem hiding this comment.
Handle potential errors in block_env_at.
The block_env_at method call should handle potential errors more gracefully.
- let env = sequencer
- .block_env_at(block_id)
- .map_err(StarknetApiError::from)?
- .ok_or(StarknetApiError::BlockNotFound)?;
+ let env = sequencer
+ .block_env_at(block_id)
+ .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })?
+ .ok_or(StarknetApiError::BlockNotFound)?;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 state = sequencer.state(&block_id).map_err(StarknetApiError::from)?; | |
| let env = sequencer | |
| .block_env_at(block_id) | |
| .map_err(StarknetApiError::from)? | |
| .ok_or(StarknetApiError::BlockNotFound)?; | |
| let state = sequencer.state(&block_id).map_err(StarknetApiError::from)?; | |
| let env = sequencer | |
| .block_env_at(block_id) | |
| .map_err(|e| StarknetApiError::UnexpectedError { reason: e.to_string() })? | |
| .ok_or(StarknetApiError::BlockNotFound)?; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2205 +/- ##
==========================================
- Coverage 68.20% 68.18% -0.03%
==========================================
Files 336 339 +3
Lines 44056 44066 +10
==========================================
- Hits 30050 30047 -3
- Misses 14006 14019 +13 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores