Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note
|
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'Align with spec for RPC v0.10.0' clearly summarizes the main change to conform the RPC implementation with the v0.10.0 specification. |
| Description check | ✅ Passed | The PR description includes the required template sections and specifies the linked issue (#885). However, 'Development related changes' section is empty and docs update checkbox is unchecked. |
| Linked Issues check | ✅ Passed | The PR successfully implements the requirements from issue #885 by making transaction_index and event_index fields mandatory in EmittedEvent, requiring starknet_api v0.16.0-rc.2 and blockifier v0.16.0-rc.2 which provide the necessary fields from forking origin. |
| Out of Scope Changes check | ✅ Passed | All changes are aligned with the v0.10.0 RPC spec conformance objective. While several files were modified (dependencies, block headers, transactions, and tests), these changes are necessary for the spec alignment and the emitted events fix. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 86.21% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
emitted-events-spec-fix
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Pull request overview
This PR updates the codebase to align with Starknet RPC specification version 0.10.0. The changes involve updating dependencies, adding new block header fields for commitments and counts, and adjusting event handling to match the updated spec.
- Updated RPC URL endpoints from v0_8 to v0_10 and upgraded starknet-rs and starknet_api dependencies
- Added new block header fields: transaction_commitment, event_commitment, receipt_commitment, state_diff_commitment, transaction_count, event_count, and state_diff_length
- Modified EmittedEvent to use non-optional transaction_index and event_index fields
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Updated starknet dependencies to newer versions supporting RPC v0.10.0 |
| tests/integration/common/constants.rs | Updated forking URLs from v0_8 to v0_10 endpoints |
| tests/integration/test_get_events.rs | Added ignore annotations for tests pending mainnet RPC v0.10.0 upgrade |
| tests/integration/test_blocks_generation.rs | Added event count parameter to block assertions |
| crates/starknet-devnet-types/src/rpc/emitted_event.rs | Changed transaction_index and event_index from Option to u64 |
| crates/starknet-devnet-types/src/rpc/block.rs | Added new commitment and count fields to BlockHeader |
| crates/starknet-devnet-types/src/rpc/transactions.rs | Added get_signature method for extracting transaction signatures |
| crates/starknet-devnet-types/src/rpc/transactions/*.rs | Made signature field pub(crate) in V3 transaction types |
| crates/starknet-devnet-core/src/blocks/mod.rs | Added methods to set counts and commitments on blocks |
| crates/starknet-devnet-core/src/starknet/mod.rs | Implemented block commitment calculation and removed obsolete test |
| crates/starknet-devnet-core/src/starknet/events.rs | Updated event creation to use non-optional indices |
| crates/starknet-devnet-core/src/state/state_diff.rs | Added conversion to starknet_api::state::ThinStateDiff |
| crates/starknet-devnet-core/src/transactions.rs | Added conversion to TransactionHashingData for commitment calculation |
| crates/starknet-devnet-server/src/server.rs | Updated timeout layer configuration |
| crates/starknet-devnet-server/src/api/endpoints_ws.rs | Added population of new block header fields from origin blocks |
| crates/starknet-devnet-server/test_data/spec/0.10.0/starknet_api_openrpc.json | Added new block header field definitions to OpenRPC spec |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/starknet-devnet-types/src/rpc/transactions.rs (1)
144-151: Consider returningOption<TransactionSignature>for explicit absence handling.Returning
TransactionSignature::default()for non-V3 variants silently produces an empty signature, which may be indistinguishable from a legitimately empty signature. AnOption<TransactionSignature>return type would make the absence of a signature explicit and allow callers to handle both cases appropriately.That said, if the current behavior is intentional and callers are aware of this contract, the implementation is functionally correct.
crates/starknet-devnet-core/src/transactions.rs (1)
243-308: LGTM on the newFrom<&StarknetTransaction>implementation.The conversion logic correctly maps all transaction data fields to the
TransactionHashingDatastructure, including signature, hash, events, execution status, gas consumed, and messages. This enables proper block commitment calculations.Note: The
unwrap_or_default()on Line 295 was already flagged in a previous review. If the "Default should never happen" comment is accurate, consider using.expect()for better debugging visibility in case of unexpected failures.
🧹 Nitpick comments (1)
tests/integration/test_get_events.rs (1)
312-313: Add TODO comment for consistency.This
#[ignore]annotation lacks the explanatory TODO comment that other ignored tests in this file have. Adding// TODO: Remove ignore after mainnet upgrades to RPC v0.10.0would maintain consistency and help track when this test can be re-enabled.#[tokio::test] -#[ignore] +#[ignore] // TODO: Remove ignore after mainnet upgrades to RPC v0.10.0 async fn get_events_since_accepted_on_l1_on_origin() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/starknet-devnet-server/test_data/spec/0.10.0/starknet_api_openrpc.jsonis excluded by!crates/starknet-devnet-server/test_data/spec/**
📒 Files selected for processing (18)
Cargo.toml(1 hunks)crates/starknet-devnet-core/src/blocks/mod.rs(3 hunks)crates/starknet-devnet-core/src/starknet/events.rs(1 hunks)crates/starknet-devnet-core/src/starknet/mod.rs(3 hunks)crates/starknet-devnet-core/src/state/state_diff.rs(1 hunks)crates/starknet-devnet-core/src/transactions.rs(1 hunks)crates/starknet-devnet-core/src/utils.rs(1 hunks)crates/starknet-devnet-server/src/api/endpoints_ws.rs(2 hunks)crates/starknet-devnet-server/src/server.rs(1 hunks)crates/starknet-devnet-types/src/rpc/block.rs(3 hunks)crates/starknet-devnet-types/src/rpc/emitted_event.rs(2 hunks)crates/starknet-devnet-types/src/rpc/transactions.rs(1 hunks)crates/starknet-devnet-types/src/rpc/transactions/declare_transaction_v3.rs(1 hunks)crates/starknet-devnet-types/src/rpc/transactions/deploy_account_transaction_v3.rs(1 hunks)crates/starknet-devnet-types/src/rpc/transactions/invoke_transaction_v3.rs(1 hunks)tests/integration/common/constants.rs(1 hunks)tests/integration/test_blocks_generation.rs(6 hunks)tests/integration/test_get_events.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-18T13:11:58.957Z
Learnt from: Abeeujah
Repo: 0xSpaceShard/starknet-devnet PR: 848
File: tests/integration/test_blocks_generation.rs:65-78
Timestamp: 2025-09-18T13:11:58.957Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs was not converted to return Result<(), anyhow::Error> and still returns unit type (), unlike assert_tx_succeeded_accepted which was converted to return Result.
Applied to files:
tests/integration/common/constants.rscrates/starknet-devnet-core/src/starknet/mod.rstests/integration/test_blocks_generation.rscrates/starknet-devnet-core/src/transactions.rstests/integration/test_get_events.rscrates/starknet-devnet-types/src/rpc/block.rs
📚 Learning: 2025-06-16T07:35:24.725Z
Learnt from: FabijanC
Repo: 0xSpaceShard/starknet-devnet PR: 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.
Applied to files:
crates/starknet-devnet-core/src/starknet/mod.rscrates/starknet-devnet-core/src/state/state_diff.rscrates/starknet-devnet-core/src/blocks/mod.rs
📚 Learning: 2025-09-18T13:15:00.238Z
Learnt from: Abeeujah
Repo: 0xSpaceShard/starknet-devnet PR: 848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs is currently unimplemented - it only contains a TODO message printed via eprintln! and has all the actual assertion logic commented out. The function still returns unit type () rather than Result<(), anyhow::Error> and essentially acts as a no-op stub.
Applied to files:
crates/starknet-devnet-core/src/starknet/mod.rstests/integration/test_blocks_generation.rstests/integration/test_get_events.rs
📚 Learning: 2025-09-18T13:15:00.238Z
Learnt from: Abeeujah
Repo: 0xSpaceShard/starknet-devnet PR: 848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: In tests/integration/common/utils.rs, the function assert_tx_succeeded_pre_confirmed currently contains only a TODO comment and doesn't perform any actual assertions, making it effectively unimplemented despite having its signature updated to return Result<(), anyhow::Error>.
Applied to files:
crates/starknet-devnet-core/src/starknet/mod.rstests/integration/test_blocks_generation.rstests/integration/test_get_events.rs
🧬 Code graph analysis (1)
crates/starknet-devnet-core/src/starknet/mod.rs (1)
crates/starknet-devnet-core/src/blocks/mod.rs (1)
block_hash(357-359)
🔇 Additional comments (26)
crates/starknet-devnet-core/src/utils.rs (1)
5-5: LGTM!The
VersionedConstantsTraitimport is required to bring thegetmethod into scope forVersionedConstants::get(&StarknetVersion::V0_14_1)on line 46.crates/starknet-devnet-types/src/rpc/transactions/invoke_transaction_v3.rs (1)
19-19: LGTM!The
pub(crate)visibility for thesignaturefield is appropriate—it enables crate-internal access (e.g., for the newget_signaturemethod) while maintaining encapsulation from external consumers. This change is consistent with the similar visibility adjustments inDeclareTransactionV3andDeployAccountTransactionV3.crates/starknet-devnet-types/src/rpc/transactions/declare_transaction_v3.rs (1)
19-19: LGTM!Visibility change to
pub(crate)is consistent with the other V3 transaction types and enables crate-internal access for theget_signaturemethod.crates/starknet-devnet-types/src/rpc/transactions/deploy_account_transaction_v3.rs (1)
21-21: LGTM!Visibility change to
pub(crate)completes the consistent pattern across all V3 transaction types.crates/starknet-devnet-core/src/state/state_diff.rs (1)
180-235: No issues found. Thestarknet_api::state::ThinStateDiffstruct has a different field structure than the localThinStateDifftype. The conversion correctly maps only the fields that the externalstarknet_apiversion supports. The omission ofreplaced_classesanddeclared_classesis intentional and not an error.crates/starknet-devnet-core/src/transactions.rs (1)
317-317: Import update aligns with API versioning changes.The
VersionedConstantsTraitimport supports the updatedstarknet_apicrate interface.Cargo.toml (2)
82-83: Note: Using release candidate versions for core dependencies.
starknet_apiandblockifierare pinned to0.16.0-rc.2. This is appropriate for a development tool aligning with an upcoming spec version, but monitor for the stable release.
81-89: All renamed Starknet Rust packages are valid on crates.io with the specified versions. Verification confirmsstarknet-rust-signers(0.15.0),starknet-rust-core(0.17.0),starknet-rust-providers(0.17.0),starknet-rust-accounts(0.17.0), andstarknet-rust-contract(0.17.0) all exist in the registry. The package renames are legitimate and no further action is needed.crates/starknet-devnet-server/src/server.rs (1)
46-49: Good improvement: Explicit timeout status code.Using
TimeoutLayer::with_status_code(StatusCode::REQUEST_TIMEOUT, ...)ensures clients receive a proper 408 status code on timeout, improving HTTP semantics compliance and debuggability.crates/starknet-devnet-types/src/rpc/emitted_event.rs (1)
13-14: Correct spec alignment:transaction_indexandevent_indexnow mandatory.This change properly addresses issue #885 by making these fields required per the 0.14.1 spec. The
Fromimplementation correctly maps the values from the source type.Also applies to: 28-29
tests/integration/common/constants.rs (1)
93-97: RPC URLs updated to v0_10 for spec alignment.The forking endpoints are correctly updated to use RPC v0.10.0. This aligns with the PR objectives and explains why some forking tests are temporarily ignored (mainnet hasn't upgraded yet).
tests/integration/test_get_events.rs (1)
214-215: Appropriate temporary test disabling for forked mainnet tests.These tests correctly include TODO comments explaining the dependency on mainnet's RPC v0.10.0 upgrade. The tests will continue to function once mainnet upgrades.
Also applies to: 241-242, 277-278, 345-346, 373-374
tests/integration/test_blocks_generation.rs (2)
50-68: Good addition of event count validation.The
assert_latest_block_with_tx_hashesfunction now validates bothtransaction_countandevent_count, ensuring the new block header fields are correctly populated. The assertions at Lines 60-61 properly verify consistency between the transaction list and the reported counts.
256-258: Event count values are consistent with expected transaction behavior.The hardcoded event counts align with the expected behavior:
- Minting operations produce 2 transfer events each (charge + fund)
- The counts (2, 0, 10, 5, 6) match the transaction patterns in each test scenario
Also applies to: 288-288, 306-306, 495-495, 544-544
crates/starknet-devnet-core/src/starknet/mod.rs (3)
425-450: Block commitment calculation correctly integrated.The implementation properly:
- Collects transaction hashing data from the block's transactions
- Converts the state diff to the API's thin representation
- Calculates commitments using the official
calculate_block_commitmentsfunction- Updates block metadata with transaction/event/state-diff counts and computed commitments
The
filter_mapon Lines 430-433 is appropriate given the comment explaining that transaction hashes in the pre-confirmed block should always exist in the transactions collection.
377-378: Extracting header fields before mutation is correct.Capturing
starknet_versionandl1_da_modefrom the header before subsequent mutations ensures these values are preserved for the commitment calculation.
19-22: New imports support block commitment functionality.The
calculate_block_commitmentsandThinStateDiffimports enable the new block commitment calculation feature.crates/starknet-devnet-core/src/starknet/events.rs (1)
81-90: LGTM! Spec-compliant non-optional indices.The change from
Some(transaction_index as u64)totransaction_index as u64correctly aligns with the RPC v0.10.0 spec requirement. Theusizetou64cast is safe for realistic transaction/event counts.crates/starknet-devnet-types/src/rpc/block.rs (3)
3-5: LGTM! Correct imports for commitment types.The new imports from
starknet_api::coreproperly bring in the commitment wrapper types needed for the expandedBlockHeader.
64-69: Appropriate clippy suppression.The
#[allow(clippy::large_enum_variant)]is justified sinceBlockis the dominant variant in practice. Boxing would add unnecessary indirection overhead.
115-124: LGTM! BlockHeader extended with spec-compliant fields.The new commitment and count fields align with the RPC spec. The
serde(rename)attributes correctly map internal field names to the expected JSON keys (n_transactions→transaction_count,n_events→event_count).crates/starknet-devnet-server/src/api/endpoints_ws.rs (2)
13-17: LGTM! Imports for commitment types and hash wrapper.The imports correctly bring in the commitment wrapper types and
PoseidonHashneeded for constructing the enrichedBlockHeaderfrom origin blocks.
94-104: Origin block field mapping is correct; verify external provider compatibility.The code successfully maps all required fields from starknet-rs
BlockWithTxHashesto the localBlockHeaderstruct. Verification confirms that starknet-rs 0.17.0 providestransaction_count,event_count,state_diff_length,state_diff_commitment,transaction_commitment,event_commitment, andreceipt_commitmentas part of the Starknet block specification. The field mapping at lines 94-104 is accurate.However, if the forking origin provider is an older or non-compliant Starknet node that doesn't return these commitment fields, deserialization will fail. Consider documenting the minimum provider version requirements, or add error handling for missing fields if compatibility with older providers is needed.
crates/starknet-devnet-core/src/blocks/mod.rs (3)
5-5: LGTM! Import for BlockHeaderCommitments.Correct import for the commitment calculation result type from
starknet_api.
333-339: LGTM! BlockHeader conversion extended with new fields.The conversion correctly handles optional internal fields by using
unwrap_or_default()for commitments andunwrap_or(0)forstate_diff_length, providing sensible defaults for blocks that may not have these values computed yet.
415-431: LGTM! New setter methods for counts and commitments.The
set_countsandset_commitmentsmethods correctly update the internal block header state. The parameter types align with the expected sources (counts asusize, commitments asBlockHeaderCommitments).
* Align with spec for emitted events * Migrate starknet-rs crates * Fix spec * Bump blockifier * starknet-rust 0.18 bump
Usage related changes
Fixes #885
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
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.