Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new feature was implemented to support marking blocks and their transactions as Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All observed changes are directly related to implementing and testing the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
website/docs/blocks.md (3)
72-76: Add language specification to code block.The static analysis tool correctly identifies that this JSON response should have a language specification for better syntax highlighting and clarity.
-Result: +Result: -``` +```json {"block_hash": "0x115e1b390cafa7942b6ab141ab85040defe7dee9bef3bc31d8b5b3d01cc9c67"}--- `133-139`: **Add language specification to code block.** This JSON response should have a language specification for consistency with other code blocks. ```diff -Result: +Result: -``` +```json { "aborted": [BLOCK_HASH_0, BLOCK_HASH_1, ...] }--- `147-183`: **LGTM! Comprehensive documentation for the new feature.** The new "Accepting blocks on L1" section provides excellent documentation for the feature: - Clear explanation of the functionality and its limitations - Good examples showing the behavior - Proper request/response format documentation - Important note about simulation vs actual L1 operations **Minor improvements for code block formatting:** ```diff -Result: +Result: -``` +```json { "accepted": [BLOCK_HASH_0, BLOCK_HASH_1, ...] }**Consider rewording to reduce repetitive sentence beginnings:** The example section (line 159) has three consecutive sentences starting with "If". Consider varying the sentence structure for better readability. </blockquote></details> <details> <summary>tests/integration/test_accepting_blocks_on_l1.rs (1)</summary><blockquote> `1-226`: **Consider adding edge case test for genesis block.** The test suite is comprehensive and well-structured, but consider adding a test for the edge case where acceptance starts from the genesis block (block 0). This would verify that the implementation correctly handles the case where there are no parent blocks to traverse. Add a test like: ```rust #[tokio::test] async fn should_accept_genesis_block_only() { let devnet = BackgroundDevnet::spawn().await.unwrap(); let accepted_block_hashes = accept_on_l1(&devnet, &BlockId::Number(0)).await.unwrap(); assert_eq!(accepted_block_hashes.len(), 1); // Verify only genesis block is accepted let genesis_block = devnet.json_rpc_client.get_block_with_tx_hashes(BlockId::Number(0)).await.unwrap(); assert_eq!(genesis_block.status, BlockStatus::AcceptedOnL1); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/starknet-devnet-core/src/starknet/mod.rs(2 hunks)crates/starknet-devnet-server/src/api/http/models.rs(1 hunks)crates/starknet-devnet-server/src/api/json_rpc/mod.rs(7 hunks)crates/starknet-devnet-server/src/api/json_rpc/write_endpoints.rs(2 hunks)tests/integration/lib.rs(1 hunks)tests/integration/test_accepting_blocks_on_l1.rs(1 hunks)website/docs/blocks.md(4 hunks)website/static/devnet_api.json(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: FabijanC
PR: 0xSpaceShard/starknet-devnet#755
File: crates/starknet-devnet-server/test_data/spec/0.8.1/starknet_executables.json:1154-1168
Timestamp: 2025-04-14T09:33:59.391Z
Learning: FabijanC wants to configure CodeRabbit to ignore spec files from review, specifically the JSON schema files in the test_data/spec directory.
crates/starknet-devnet-core/src/starknet/mod.rs (1)
Learnt from: FabijanC
PR: 0xSpaceShard/starknet-devnet#799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, `get_class_hash_at` has different behaviors at different layers: at the internal state level (blockifier), it returns `Ok(ClassHash(Felt::ZERO))` for undeployed addresses, while at the RPC layer, this gets converted to `Err(Error::ContractNotFound)`. When checking if a contract is deployed at the internal state level, use `is_ok_and(|h| h.0 == Felt::ZERO)` to detect undeployed addresses.
tests/integration/test_accepting_blocks_on_l1.rs (1)
Learnt from: FabijanC
PR: 0xSpaceShard/starknet-devnet#799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, `get_class_hash_at` has different behaviors at different layers: at the internal state level (blockifier), it returns `Ok(ClassHash(Felt::ZERO))` for undeployed addresses, while at the RPC layer, this gets converted to `Err(Error::ContractNotFound)`. When checking if a contract is deployed at the internal state level, use `is_ok_and(|h| h.0 == Felt::ZERO)` to detect undeployed addresses.
🧬 Code Graph Analysis (1)
crates/starknet-devnet-core/src/starknet/mod.rs (3)
crates/starknet-devnet-server/src/api/json_rpc/write_endpoints.rs (2)
abort_blocks(155-158)accept_on_l1(161-165)tests/integration/common/background_devnet.rs (1)
abort_blocks(397-419)tests/integration/test_accepting_blocks_on_l1.rs (1)
accept_on_l1(16-27)
🪛 markdownlint-cli2 (0.17.2)
website/docs/blocks.md
74-74: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 LanguageTool
website/docs/blocks.md
[style] ~159-~159: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., 2, 3 and 4 shall be ACCEPTED_ON_L1. If a new block is mined after that (number...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (18)
tests/integration/lib.rs (1)
11-11: LGTM! Test module addition follows established pattern.The addition of the
test_accepting_blocks_on_l1module is consistent with the existing test organization structure and clearly indicates the purpose of testing the new L1 acceptance feature.crates/starknet-devnet-server/src/api/http/models.rs (1)
64-74: LGTM! New HTTP models follow established patterns.The
AcceptOnL1RequestandAcceptedOnL1Blocksstructs are well-designed and consistent with existing models in the codebase. The field names, types, and serde attributes follow the established patterns used by similar structures likeAbortingBlocksandAbortedBlocks.crates/starknet-devnet-server/src/api/json_rpc/write_endpoints.rs (2)
22-25: LGTM! Import additions are consistent with the new feature.The new imports for
AcceptOnL1RequestandAcceptedOnL1Blocksare properly added and follow the existing import organization in the file.
160-166: LGTM! Implementation follows established patterns.The
accept_on_l1method implementation is consistent with other write endpoints in the file:
- Proper async signature with
StrictRpcResultreturn type- Appropriate state locking and delegation to core logic
- Consistent error handling with the
?operator- Proper response wrapping in
DevnetResponseenumwebsite/static/devnet_api.json (2)
355-359: LGTM! Improved clarity for abort blocks method.The refinement of the
devnet_abortBlocksmethod description provides better clarity about the operation scope and parameter usage.
390-426: LGTM! Well-structured API specification for the new method.The
devnet_acceptOnL1method specification is comprehensive and follows the established patterns in the API documentation. The parameter and response schemas are properly defined with clear descriptions and appropriate references to shared schemas.crates/starknet-devnet-server/src/api/json_rpc/mod.rs (7)
56-61: LGTM: Import additions are correctly organizedThe new imports
AcceptOnL1RequestandAcceptedOnL1Blocksare properly added to the existing import statement and follow the alphabetical ordering pattern.
549-549: LGTM: Request handling follows established patternThe new
AcceptOnL1variant is correctly handled in the execute method, following the same pattern as other devnet methods likeAbortBlocks. The method properly delegates toself.accept_on_l1(data).await.
734-735: LGTM: Enum variant follows naming conventionsThe new
AcceptOnL1variant is properly defined with the correct serde rename attribute"devnet_acceptOnL1"and appropriate parameter typeAcceptOnL1Request.
765-765: LGTM: Correctly classified as requiring notificationThe
AcceptOnL1variant is appropriately added to therequires_notifying()method, which makes sense since accepting blocks on L1 would be a state-changing operation that subscribers should be notified about.
862-862: LGTM: Correctly classified as not forwardable to originThe
AcceptOnL1variant is correctly added to theis_forwardable_to_origin()method returningfalse. This is appropriate since this is a devnet-specific operation that shouldn't be forwarded to the origin network.
887-887: LGTM: Correctly classified as dumpableThe
AcceptOnL1variant is appropriately added to theis_dumpable()method returningtrue. This is consistent with other state-changing devnet operations likeAbortBlocksandCreateBlock.
1045-1045: LGTM: Response enum variant properly definedThe new
AcceptedOnL1Blocks(AcceptedOnL1Blocks)variant is correctly added to theDevnetResponseenum, following the same pattern as other devnet response types.crates/starknet-devnet-core/src/starknet/mod.rs (2)
931-934: LGTM! Type-safe return type improvement.The change from
Vec<Felt>toVec<TransactionHash>improves type safety while maintaining functionality, asTransactionHashis a type alias forFelt. This makes the API more semantically meaningful.
1022-1031: LGTM! Well-designed validation method.The validation logic correctly identifies that only
AcceptedOnL2blocks can be accepted on L1, with clear error messages for invalid states. The implementation follows established patterns in the codebase.tests/integration/test_accepting_blocks_on_l1.rs (3)
12-59: LGTM! Well-designed test helpers.The helper functions provide good abstractions for common test operations:
send_dummy_txgenerates test transactionsaccept_on_l1wraps the JSON-RPC call cleanlyassert_accepted_on_l1andassert_latest_accepted_on_l2provide comprehensive status verificationThe helpers follow established test patterns and handle errors appropriately.
61-151: LGTM! Comprehensive happy path test coverage.The tests thoroughly cover the main functionality:
- Different block ID types (latest, numeric, hash)
- Both instant and demand-based block generation modes
- Verification of both block and transaction status changes
- Proper boundary testing (blocks not included remain on L2)
The test structure is clear and follows good testing practices with proper setup, execution, and verification phases.
153-225: LGTM! Thorough error case testing.The error tests comprehensively cover invalid scenarios:
- Duplicate acceptance attempts
- Invalid block states (pre-confirmed, rejected)
- Non-existent block IDs
Each test properly verifies the expected error responses and ensures system state remains consistent after errors. The test cases align well with the validation logic in the core implementation.
Usage related changes
devnet_acceptOnL1ACCEPTED_ON_L1blocks fromstarting_block_idto last accepted on L1, does the same for their txs.Development related changes
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
New Features
Bug Fixes
Tests