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 WalkthroughThis PR executes a comprehensive update of the Starknet ecosystem dependencies (blockifier, cairo-vm, cairo-lang, starknet-rs-\* crates) and Rust toolchain (1.86 to 1.89). The update includes corresponding code migrations: compiled class hashing API changes from Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Justification: The PR spans 40+ files with heterogeneous changes across multiple architectural layers (core, types, server, integration tests). While integration test updates follow a repetitive pattern of replacing Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
* compiles, but ContractFactory::new gives warnings * [skip ci]
365f673 to
5829fa1
Compare
9f1da3e to
a3e975f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/starknet-devnet-core/src/transactions.rs (1)
212-222: Complete the error propagation instead of usingexpect("TODO").The
expect("TODO")on line 217 is a temporary workaround that introduces a runtime panic risk. The related filecrates/starknet-devnet-types/src/rpc/transactions.rs(lines 958-961) properly handles this by usingOrderedMessageToL1::new(...)?and propagating errors. This function should follow the same pattern.Refactor
get_l2_to_l1_messagesto returnDevnetResult<Vec<MessageToL1>>and propagate the error:- pub fn get_l2_to_l1_messages(&self) -> Vec<MessageToL1> { + pub fn get_l2_to_l1_messages(&self) -> DevnetResult<Vec<MessageToL1>> { let mut messages = vec![]; fn get_blockifier_messages_recursively(call_info: &CallInfo) -> Vec<OrderedMessageToL1> {Then update the message construction to propagate errors:
messages.extend(call_info.execution.l2_to_l1_messages.iter().map(|m| { - OrderedMessageToL1 { + OrderedMessageToL1::new(m, call_info.call.storage_address.into()) - order: m.order, - #[allow(clippy::expect_used)] // TODO - message: MessageToL1 { - to_address: m.message.to_address.try_into().expect("TODO"), - from_address: from_address.into(), - payload: m.message.payload.0.clone(), - }, - } - })); + }).collect::<DevnetResult<Vec<_>>>()?);Note: You'll also need to update the function signature for
get_blockifier_messages_recursivelyto returnDevnetResult<Vec<OrderedMessageToL1>>and handle errors appropriately. Finally, update the return statement on line 239 toOk(messages)and adjust all call sites ofget_l2_to_l1_messages()to handle theDevnetResult.crates/starknet-devnet-core/src/error.rs (1)
148-150: Don't map all DeclareTransactionError variants to ClassAlreadyDeclaredThis collapses distinct declare failures (e.g., size limits, hash mismatch) into a single variant, misleading callers and tests.
Minimal safe fix: fall back to a generic contract execution error here; or match specific inner declare variants if you need structured mapping.
- err @ TransactionExecutionError::DeclareTransactionError { .. } => { - Error::ClassAlreadyDeclared { msg: err.to_string() } - } + other @ TransactionExecutionError::DeclareTransactionError { .. } => { + // Preserve accurate semantics; avoid mislabeling non-"already declared" errors. + Self::ContractExecutionError(other.into()) + }If you prefer structured mapping, match the inner DeclareTransactionError and map only ClassAlreadyDeclared to Error::ClassAlreadyDeclared, forwarding others appropriately.
tests/integration/test_simulate_transactions.rs (1)
918-926: Same mismatch here: switch to deploy_contract for new UDCThis test expects an “Insufficient …” revert due to fee bounds. Using the legacy entrypoint on the new UDC could instead fail with ENTRYPOINT_NOT_FOUND, breaking the assertion.
- let execution = account.execute_v3(vec![Call { - to: UDC_CONTRACT_ADDRESS, - selector: get_selector_from_name("deployContract").unwrap(), + let execution = account.execute_v3(vec![Call { + to: UDC_CONTRACT_ADDRESS, + selector: get_selector_from_name("deploy_contract").unwrap(),
🧹 Nitpick comments (16)
crates/starknet-devnet-server/src/api/endpoints.rs (1)
514-524: Consider refactoring to avoid lint suppressions.The current pattern checks
origin_range.is_some()and then calls.expect()on the sameOption, requiring two lint suppressions. Refactoring to useif letwould be more idiomatic and eliminate the need for these suppressions.Apply this diff to refactor using
if let:- let events_chunk = if origin_range.is_some() + let events_chunk = if let Some((from_origin, to_origin)) = origin_range && filter .continuation_token .clone() .is_none_or(|token| token.starts_with(CONTINUATION_TOKEN_ORIGIN_PREFIX)) { - #[allow(clippy::unnecessary_unwrap)] - #[allow(clippy::expect_used)] - let (from_origin, to_origin) = - origin_range.expect("Continuation token implies there are more origin events"); - self.fetch_origin_events_chunk(crates/starknet-devnet-types/src/rpc/eth_address.rs (1)
37-47: Implementation is correct; minor clarity improvement possible.The
TryFrom<L1Address>implementation correctly converts viaEthAddress::from_feltand properly maps errors. The logic is sound.For slightly improved readability, consider extracting the fallible conversion before wrapping in
Ok:impl TryFrom<L1Address> for EthAddressWrapper { type Error = Error; fn try_from(value: L1Address) -> Result<Self, Self::Error> { - Ok(Self { - inner: EthAddress::from_felt(&value.0).map_err(|e| { - Error::ConversionError(ConversionError::OutOfRangeError(e.to_string())) - })?, - }) + let inner = EthAddress::from_felt(&value.0).map_err(|e| { + Error::ConversionError(ConversionError::OutOfRangeError(e.to_string())) + })?; + Ok(Self { inner }) } }tests/integration/test_fork.rs (2)
235-249: Prefer integer literals over float casts for gas valuesCasting float literals to u64 is unnecessary and brittle. Use integer literals for clarity.
- .l2_gas(5e7 as u64) + .l2_gas(50_000_000_u64) - .l1_data_gas(1000) + .l1_data_gas(1_000_u64) ... - .l1_data_gas(1e3 as u64) + .l1_data_gas(1_000_u64) - .l2_gas(1e7 as u64) + .l2_gas(10_000_000_u64)Also applies to: 287-291
572-581: Avoid brittle assertion on platform-specific error text"Connection refused" wording can vary by OS/transport. Keep the origin-communication and URL checks; drop the OS-specific fragment to reduce flakiness.
- assert_contains(&received_error.message, "Connection refused").unwrap(); + // Platform-dependent socket error wording omitted to avoid flakiness.tests/integration/test_minting.rs (1)
115-146: V3 declare after FRI mint — add a basic success check on mintFlow is sound: fail without funds, mint FRI, then succeed. Minor improvement: assert the mint call result to surface failures earlier.
- devnet.mint_unit(account_address, 1_000_000_000_000_000_000_000, FeeUnit::Fri).await; + let _tx = devnet + .mint_unit(account_address, 1_000_000_000_000_000_000_000, FeeUnit::Fri) + .await + .expect("mint_unit should succeed");Also consider asserting the first error is fee-related for clearer intent.
tests/integration/test_subscription_to_new_tx_receipts.rs (2)
410-482: Receipt polling may hang — wrap receive with a timeoutThe loop awaits notifications without a timeout; if none arrive, the test can hang. Add a short timeout to fail fast and keep CI stable. Optionally extract a small helper for repeated “valid block info” checks.
- match receive_new_tx_receipt(&mut ws, subscription_id.clone()).await { + match tokio::time::timeout(std::time::Duration::from_secs(5), + receive_new_tx_receipt(&mut ws, subscription_id.clone())).await { - Ok(notification) => { + Ok(Ok(notification)) => { // unchanged } - Err(_) => break, // No more notifications + Ok(Err(_)) => break, // No more notifications + Err(_) => break, // Timed out waiting for next receipt }Optionally factor a small assert_valid_block_ref(block) helper to reduce duplication.
485-549: Good end-to-end validation of TransactionReceiptWithBlockInfoCovers structure, fees, events, execution status, and block existence. Consider reusing the same block validation helper to avoid repetition.
tests/integration/test_subscription_to_new_txs.rs (1)
329-333: Avoid redundant assertionsAfter deserializing to DeployAccountTransaction and asserting its hash, the JSON field equality adds little value. Consider keeping only one of them.
tests/integration/test_messaging.rs (1)
302-315: Felt-based payload fields are finePassing l2_contract_address and entry_point_selector as Felt relies on serde’s hex serialization — compatible with devnet RPC.
If this payload appears in multiple tests, consider extracting a small builder in utils to avoid duplication.
tests/integration/test_v3_transactions.rs (4)
46-49: Don’t over-broaden to ProviderError::Other without content checksCatching ProviderError::Other(_) makes the test less diagnostic. Consider asserting the error text mentions resource/balance issues to retain intent.
- starknet_rs_accounts::AccountFactoryError::Provider(ProviderError::Other(_)) => {} + starknet_rs_accounts::AccountFactoryError::Provider(ProviderError::Other(e)) => { + assert!(e.to_string().contains("resource") || e.to_string().contains("balance")); + }
162-166: Same here: tighten the “Other” matchCheck the message content when accepting ProviderError::Other to avoid masking unrelated failures.
361-369: Declaration path: prefer a small helper for error matchingCreate a helper is_resource_bounds_error(&ProviderError) to match both typed InsufficientResourcesForValidate and “Other” with resource-bound hints.
fn is_resource_bounds_error(e: &ProviderError) -> bool { matches!(e, ProviderError::StarknetError(StarknetError::InsufficientResourcesForValidate)) || e.to_string().contains("Insufficient") || e.to_string().contains("resource") || e.to_string().contains("gas bound") }Then:
- starknet_rs_accounts::AccountError::Provider(ProviderError::Other(_)) => {} + starknet_rs_accounts::AccountError::Provider(e) if is_resource_bounds_error(e) => {}
382-389: Account deployment path: same tighteningApply the helper here as well to keep assertions meaningful.
crates/starknet-devnet-core/src/error.rs (1)
175-193: Lint placement: move clippy lint level out of expression scopeThe #[warn(clippy::wildcard_enum_match_arm)] attribute inside the function is unusual; place lint levels at module or function scope for consistency and tool compatibility.
-impl From<TransactionFeeError> for Error { - fn from(value: TransactionFeeError) -> Self { - #[warn(clippy::wildcard_enum_match_arm)] - match value { +#[warn(clippy::wildcard_enum_match_arm)] +impl From<TransactionFeeError> for Error { + fn from(value: TransactionFeeError) -> Self { + match value { … } } }tests/integration/test_deploy.rs (1)
145-148: Remove duplicate legacy UDC case in loopThe first two tuples are identical; this runs the same path twice without added coverage.
- for (udc_address, deployment_method) in [ - (UDC_LEGACY_CONTRACT_ADDRESS, legacy_deployment_method), - (UDC_LEGACY_CONTRACT_ADDRESS, legacy_deployment_method), - (UDC_CONTRACT_ADDRESS, "deploy_contract"), - ] { + for (udc_address, deployment_method) in [ + (UDC_LEGACY_CONTRACT_ADDRESS, legacy_deployment_method), + (UDC_CONTRACT_ADDRESS, "deploy_contract"), + ] {tests/integration/common/utils.rs (1)
126-141: Avoid unwrap/panic in assert_tx_succeeded_pre_confirmed; return errors.Align with assert_tx_succeeded_accepted: propagate errors instead of panicking.
-pub async fn assert_tx_succeeded_pre_confirmed<T: Provider>( - tx_hash: &Felt, - client: &T, -) -> Result<(), anyhow::Error> { - let receipt = client.get_transaction_receipt(tx_hash).await.unwrap().receipt; - match receipt.execution_result() { - ExecutionResult::Succeeded => (), - other => panic!("Should have succeeded; got: {other:?}"), - } - - match receipt.finality_status() { - starknet_rs_core::types::TransactionFinalityStatus::PreConfirmed => (), - other => panic!("Should have been pre-confirmed; got: {other:?}"), - } - Ok(()) -} +pub async fn assert_tx_succeeded_pre_confirmed<T: Provider>( + tx_hash: &Felt, + client: &T, +) -> Result<(), anyhow::Error> { + let receipt = client.get_transaction_receipt(tx_hash).await?.receipt; + match receipt.execution_result() { + ExecutionResult::Succeeded => (), + other => anyhow::bail!("Tx {tx_hash:#x} should have succeeded; got: {other:?}"), + } + match receipt.finality_status() { + starknet_rs_core::types::TransactionFinalityStatus::PreConfirmed => Ok(()), + other => anyhow::bail!("Tx {tx_hash:#x} should have been pre-confirmed; got: {other:?}"), + } +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.clippy.toml(1 hunks)Cargo.toml(1 hunks)crates/starknet-devnet-core/src/error.rs(1 hunks)crates/starknet-devnet-core/src/starknet/add_declare_transaction.rs(5 hunks)crates/starknet-devnet-core/src/starknet/add_deploy_account_transaction.rs(4 hunks)crates/starknet-devnet-core/src/starknet/mod.rs(2 hunks)crates/starknet-devnet-core/src/starknet/state_update.rs(2 hunks)crates/starknet-devnet-core/src/state/mod.rs(3 hunks)crates/starknet-devnet-core/src/state/state_diff.rs(5 hunks)crates/starknet-devnet-core/src/transactions.rs(1 hunks)crates/starknet-devnet-core/src/utils.rs(2 hunks)crates/starknet-devnet-server/src/api/endpoints.rs(1 hunks)crates/starknet-devnet-types/src/rpc/eth_address.rs(2 hunks)crates/starknet-devnet-types/src/rpc/messaging.rs(1 hunks)crates/starknet-devnet-types/src/rpc/state.rs(2 hunks)crates/starknet-devnet-types/src/rpc/transactions.rs(1 hunks)docker/Dockerfile(1 hunks)rust-toolchain.toml(1 hunks)scripts/clippy_check.sh(1 hunks)tests/integration/Cargo.toml(1 hunks)tests/integration/common/constants.rs(0 hunks)tests/integration/common/utils.rs(10 hunks)tests/integration/get_transaction_receipt_by_hash.rs(4 hunks)tests/integration/test_account_selection.rs(2 hunks)tests/integration/test_advancing_time.rs(2 hunks)tests/integration/test_blocks_generation.rs(2 hunks)tests/integration/test_deploy.rs(5 hunks)tests/integration/test_dump_and_load.rs(2 hunks)tests/integration/test_estimate_fee.rs(8 hunks)tests/integration/test_estimate_message_fee.rs(2 hunks)tests/integration/test_fork.rs(2 hunks)tests/integration/test_gas_modification.rs(3 hunks)tests/integration/test_get_class.rs(2 hunks)tests/integration/test_get_events.rs(2 hunks)tests/integration/test_messaging.rs(4 hunks)tests/integration/test_minting.rs(2 hunks)tests/integration/test_old_state.rs(4 hunks)tests/integration/test_simulate_transactions.rs(13 hunks)tests/integration/test_subscription_to_events.rs(2 hunks)tests/integration/test_subscription_to_new_tx_receipts.rs(1 hunks)tests/integration/test_subscription_to_new_txs.rs(2 hunks)tests/integration/test_trace.rs(2 hunks)tests/integration/test_v3_transactions.rs(7 hunks)tests/integration/test_websocket.rs(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/common/constants.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-16T07:35:24.725Z
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.
Applied to files:
crates/starknet-devnet-core/src/starknet/state_update.rstests/integration/test_old_state.rstests/integration/test_deploy.rstests/integration/test_get_class.rscrates/starknet-devnet-core/src/state/mod.rscrates/starknet-devnet-core/src/state/state_diff.rscrates/starknet-devnet-types/src/rpc/state.rstests/integration/common/utils.rs
🔇 Additional comments (65)
docker/Dockerfile (1)
1-1: Rust toolchain upgrade aligned across Dockerfile and rust-toolchain.toml.The base image update to
rust:1.89-alpine3.21is consistent with the rust-toolchain.toml update and aligns with the PR objectives. No other changes to the build flow.rust-toolchain.toml (1)
2-2: Rust toolchain version update is consistent and aligned with Dockerfile.The channel update to
1.89.0mirrors the Dockerfile base image update and maintains consistency across the build environment. No changes to components or profile..clippy.toml (1)
3-3: Good practice for accommodating larger error types from dependency updates.Increasing the threshold to 256 bytes is a reasonable configuration adjustment as dependencies evolve and error types grow. This is standard practice during major dependency upgrades.
scripts/clippy_check.sh (1)
8-15: Verify consistency between clippy configuration approaches.The centralization of flags into
$CLIPPY_FLAGSis good for maintainability. However, there may be a redundancy between the two configuration methods:
.clippy.tomlsetslarge-error-threshold = 256(raises the threshold for theresult_large_errlint)scripts/clippy_check.shnow uses-A clippy::result_large_err(completely suppresses the lint)The command-line
-Aflag takes precedence, making the.clippy.tomlthreshold setting irrelevant for this script.Clarify the intent: If the goal is to completely allow result errors (current behavior), the
.clippy.tomlthreshold is unnecessary. If the goal is to only allow errors above 256 bytes, consider removing the-Aflag from$CLIPPY_FLAGSand relying solely on the.clippy.tomlconfiguration.crates/starknet-devnet-core/src/starknet/state_update.rs (2)
20-20: LGTM: Import updated for new hashing API.The new imports correctly support the migrated compiled class hash computation below.
42-43: LGTM: Compiled class hash computation updated to new API.The migration from the previous
compiled_class_hash()API to the explicithash(&HashVersion::V1)pattern correctly aligns with the starknet-rs 0.17 API changes. UsingHashVersion::V1is appropriate for Sierra contracts, and the.unwrap()is acceptable in test code.crates/starknet-devnet-types/src/rpc/eth_address.rs (1)
3-3: LGTM: Import additions are correct.The additions of
L1AddressandConversionErrorare necessary for the newTryFromimplementation and properly scoped.Also applies to: 6-6
crates/starknet-devnet-types/src/rpc/transactions.rs (1)
958-961: LGTM! Proper error propagation.The refactor from map-based collection to an explicit loop with
OrderedMessageToL1::new(...)?correctly handles the new fallible constructor signature and propagates errors appropriately.crates/starknet-devnet-types/src/rpc/messaging.rs (1)
82-94: LGTM! Improved error handling.The change from returning
SelftoDevnetResult<Self>with fallibletry_into()?for address conversion is a proper improvement that enables error propagation instead of panicking on conversion failures.crates/starknet-devnet-core/src/starknet/mod.rs (2)
532-532: LGTM! Appropriate default for L2 Devnet.The
is_l3: falsefield correctly identifies Devnet as operating in L2 mode, which aligns with its intended behavior as a Starknet L2 development environment.
746-756: Verify that both execute() error paths have been updated consistently for the blockifier API changes.The change at line 749 correctly boxes the error for
TransactionExecutionError::ExecutionError. However, there's a secondexecute()call at line 1368 that handles errors differently usingContractExecutionError::from(err). Verify that this second error path is also compatible with the updated blockifier API and doesn't require similar adjustments. Confirm whether the two calls operate on the same blockifier types or if the different error handling is intentional and correct.crates/starknet-devnet-core/src/utils.rs (2)
117-120: Remove or clarify the Blake hash comment—it documents an unused hash value that differs from the constant and creates confusion.The comment on line 117 documents a Blake hash that differs significantly from the actual
DUMMY_CAIRO_1_COMPILED_CLASS_HASHvalue on line 118-119. The codebase usesHashVersion::V1(Poseidon-based hashing) for compiled class hashes, and this Blake hash value appears nowhere else in the code.Either:
- Remove the Blake hash comment entirely if it's outdated
- Or clarify that it documents a legacy/historical alternative representation and explain its relevance
64-64: No issues found — theblake_weight: 5263value is correct.The default blake_weight in BouncerConfig for blockifier 0.16.0-rc.0 is 5263, which matches the value and comment in the code. The change is accurate for the blockifier dependency version in use.
crates/starknet-devnet-types/src/rpc/state.rs (2)
103-108: Type change in ClassHashPair looks correctSwitch to CompiledClassHash is consistent and future-proof. No further issues spotted.
7-7: No wire format concerns — CompiledClassHash is a transparent Felt aliasCompiledClassHash is defined as a transparent type alias (
pub type CompiledClassHash = Felt;) that inherits Felt's serialization behavior directly. Since Felt is imported from the canonicalstarknet_types_corelibrary (v0.2.3) with standard serde implementations, and no custom serializers wrap CompiledClassHash, it will serialize/deserialize identically to any other Felt-based type in the codebase. The wire format uses 0x-prefixed hex strings, consistent with the existing ecosystem. Importing it into scope instate.rsdoes not introduce breaking changes.tests/integration/test_fork.rs (2)
24-29: Updated imports LGTMNew helpers (new_contract_factory, send_ctrl_c_signal_and_wait) imported correctly.
240-243: Factory construction helper LGTMnew_contract_factory(class_hash, Arc) usage matches the helper’s intent and keeps call sites clean.
tests/integration/test_minting.rs (1)
3-11: Imports and constants alignment LGTMUsing CHAIN_ID and account constants aligns tests with the new toolchain and token unit semantics.
tests/integration/test_subscription_to_new_tx_receipts.rs (1)
401-408: Block info assertions LGTMValidates block_number and non-zero block_hash on receipts-with-block; this is valuable coverage.
tests/integration/test_websocket.rs (1)
288-301: Explicit tip(0) is good for deterministic hash/signingSetting tip(0) before prepared() ensures the signed hash matches the broadcast payload. Matches sendable_declaration.tip = 0 below.
tests/integration/test_get_class.rs (2)
3-3: LGTM! Necessary imports for the new CASM hash API.The imports support the migration from
compiled_class_hash()to the newhash(&HashVersion)API.
268-268: LGTM! Correct migration to the new hash API.The change from
compiled_class_hash()tohash(&HashVersion::V1).0aligns with the starknet-rs 0.17 API update.tests/integration/test_dump_and_load.rs (1)
10-12: LGTM! Standard refactor to use the new contract factory helper.The migration from
ContractFactory::newtonew_contract_factoryis consistent with the broader test suite refactoring pattern.Also applies to: 257-257
tests/integration/test_get_events.rs (1)
12-12: LGTM! Consistent migration to the new factory helper.The change aligns with the standardized contract factory creation pattern across the test suite.
Also applies to: 71-71
tests/integration/test_gas_modification.rs (2)
87-87: LGTM! Explicit zero tip for gas scenario testing.Setting
.tip(0)explicitly ensures consistent gas calculations in the test scenario.
377-379: Verification confirms error handling is correct—no changes needed.The upstream starknet-rs provider library exposes
ProviderErrorwith four variants:StarknetError,RateLimited,ArrayLengthMismatch, andOther(for implementation-specific errors). There is no more specific variant available for balance-related fee errors at the provider layer.The pattern in the code under review is correct and consistent with the codebase:
ProviderError::Otheris the correct variant when ResourcesBoundsExceedBalance errors propagate through the provider- String matching is the established approach throughout the test suite for identifying specific errors wrapped in
ProviderError::Other(seetest_v3_transactions.rslines 164, 364, 421, 385)- The same pattern is used in
test_simulate_transactions.rs(line 715) for similar balance-related errorsThe brittleness concern about error message changes is valid from an architectural perspective, but it reflects a limitation of the upstream provider design rather than a flaw in this code. The test is appropriately handling what the API provides.
tests/integration/test_trace.rs (1)
18-20: LGTM! Consistent migration to the new factory helper.The change follows the established pattern across the test suite.
Also applies to: 180-180
tests/integration/test_account_selection.rs (1)
18-18: LGTM! Consistent migration to the new factory helper.The refactoring aligns with the standardized approach across integration tests.
Also applies to: 190-190
tests/integration/Cargo.toml (1)
14-14: LGTM! Required dependency for the new hash API.Adding
starknet_apiwith thetestingfeature enables the HashVersion-based CASM hash computation used in the test suite.crates/starknet-devnet-core/src/starknet/add_declare_transaction.rs (2)
3-3: LGTM! Correct migration to the new hash API.The changes from
compiled_class_hash()tohash(&HashVersion::V1).0align with the starknet_api update and maintain correct CASM hash computation.Also applies to: 127-127
142-142: LGTM! Correct error type migration.The migration from
TransactionValidationErrorvariants toTransactionFeeErrorvariants reflects the upstream blockifier API changes:
InsufficientResourcesForValidate→InsufficientResourceBoundsInsufficientAccountBalance→ResourcesBoundsExceedBalanceAlso applies to: 266-269, 284-287
tests/integration/test_advancing_time.rs (2)
19-20: Factory helper import looks goodSwitch to utils::new_contract_factory aligns with other tests and centralizes construction.
71-75: Verification complete — no direct ContractFactory::new calls foundThe search confirms no direct
ContractFactory::new(calls remain in the codebase. The single match found wasContractFactory::new_with_udc, which is a different method variant. The test suite consistently uses thenew_contract_factoryhelper across all integration tests (test_trace.rs, test_simulate_transactions.rs, test_messaging.rs, test_get_events.rs, test_estimate_message_fee.rs), matching the pattern in test_advancing_time.rs:71-75.tests/integration/test_estimate_message_fee.rs (2)
10-11: Import of new_contract_factory — OKConverges factory creation to shared helper.
34-36: Helper usage — OKnew_contract_factory(class_hash, account.clone()) integrates cleanly with subsequent deploy_v3 and address derivation.
tests/integration/test_blocks_generation.rs (2)
20-24: Consolidated imports — OKBringing new_contract_factory and send_ctrl_c_signal_and_wait into scope is consistent with other files.
427-431: Factory helper — OKnew_contract_factory(...) before deploy_v3 keeps UDC flow unchanged.
tests/integration/test_messaging.rs (3)
37-41: Imports updated — OKnew_contract_factory and send_ctrl_c_signal_and_wait reflected here as elsewhere.
136-148: Factory helper in deploy_l2_msg_contract — OKThe helper-based deploy keeps salt/class_hash alignment with computed UDC address.
516-521: Use Felt for from_addressConsistent with earlier change; keeps type-safe serialization.
crates/starknet-devnet-core/src/starknet/add_deploy_account_transaction.rs (3)
71-75: Test import updated to new fee error type — OKAligns tests with blockifier’s TransactionFeeError variants.
168-176: Balance-related failure now matched via GasBoundsExceedBalance — OKAccurately reflects new error surface during execution.
204-212: Insufficient L1 gas bounds match updated error — OKMatching TransactionFeeError::InsufficientResourceBounds is appropriate here.
tests/integration/test_old_state.rs (3)
3-6: UDC constants update — OKUsing UDC_CONTRACT_CLASS_HASH is consistent with the address migration elsewhere.
18-22: Switch to UDC_CONTRACT_ADDRESS — OKUpdates call sites to canonical UDC address.
227-234: Assert against UDC constants — OKValidates both address and class hash for the nested error — stronger check.
tests/integration/test_v3_transactions.rs (1)
16-20: UDC constant migrated — OKUDC_CONTRACT_ADDRESS replaces legacy constant consistently.
crates/starknet-devnet-core/src/state/state_diff.rs (1)
8-8: Type migration to CompiledClassHash looks correctImports, StateDiff field, extraction from state maps, and ThinStateDiff conversion all align with the new CompiledClassHash and HashVersion-based hashing.
Also applies to: 26-27, 53-60, 127-129, 154-158, 184-185, 318-319
crates/starknet-devnet-core/src/state/mod.rs (1)
7-7: HashVersion-based CASM hashing and declared-check update LGTM
- Using compile_sierra_contract(...).hash(&HashVersion::V1) is aligned with the new API.
- is_contract_declared correctly treats CompiledClassHash(Felt::ZERO) as “not declared” and falls back to get_compiled_class.
Also applies to: 339-345, 366-374
tests/integration/test_deploy.rs (1)
47-48: Factory helper and UDC constants usage LGTMnew_contract_factory integration and UDC address/class-hash assertions look consistent with the migration.
Also applies to: 71-73, 103-104
tests/integration/test_simulate_transactions.rs (2)
262-271: tip(0) additions and factory helper are consistentSetting tip(0) across declare/deploy/invoke and using new_contract_factory looks good.
Also applies to: 291-296, 403-406, 143-145
525-533: Review comment approved as accurateVerification confirms the negative tests at lines 525-533 and 616-624 correctly use
no_such_methodas intentional test cases. The grep search results show all otherdeployContractcalls across the test suite are legitimate and in appropriate test contexts—no accidental deployContract calls were found. No action needed.tests/integration/test_estimate_fee.rs (3)
24-25: Imports aligned with new helpers/constants.Switch to QUERY_VERSION_OFFSET and UDC_CONTRACT_ADDRESS, and bringing in new_contract_factory, looks correct for the 0.16/0.17 stack.
Also applies to: 30-31
234-248: Use new_contract_factory for UDC deployments.Good migration. Centralizes UDC selector choice and avoids repeated factory wiring.
Also applies to: 327-341, 399-413
455-456: Include tip in prepared invoke.Including tip(0) before prepared() ensures the signed hash matches the constructed BroadcastedInvokeTransactionV3.
tests/integration/get_transaction_receipt_by_hash.rs (3)
15-16: Imports updated to new constants/helpers.Consistent with the rest of the PR.
Also applies to: 18-20
83-84: Factory creation via new_contract_factory.Good replacement; matches the new helper API.
Also applies to: 157-158
111-113: Receipt event source asserted against UDC_CONTRACT_ADDRESS.Correctly targets the UDC emitter for deployment events.
tests/integration/test_subscription_to_events.rs (2)
16-16: Import UDC_CONTRACT_ADDRESS.Aligned with constant rename across the suite.
365-366: Assert UDC as deployment event source.Correct event origin check after the rename.
tests/integration/common/utils.rs (4)
71-73: Hashing CASM with HashVersion::V1.Looks correct once the CasmContractClass import points to the types-core crate.
Please confirm the computed CASM hash matches the one used by devnet when declaring classes to avoid mismatches across toolchains.
287-288: Adopt new_contract_factory in helpers.Good consolidation; keeps UDC wiring consistent across test utilities.
Also applies to: 315-316, 345-351, 381-388
295-296: Explicit UDC address in UdcUniqueSettings.Correctly parameterizes the unique deployment address calculation.
12-14: The review comment is incorrect—the current import is correct and functional.The suggested import path
starknet_types_core::compiled_class::CasmContractClassdoes not exist in the codebase. Thestarknet_types_corecrate is used only forFeltandStarkHashutilities, not for compiled class types.The current import on line 13 (
use starknet_core::CasmContractClass;) is correct. The.hash(&HashVersion::V1).0method call on line 72 works successfully with this import, as confirmed by identical patterns used throughout the codebase (instate_diff.rs,state/mod.rs,state_update.rs,add_declare_transaction.rs, andtest_get_class.rs).No changes are needed.
Likely an incorrect or invalid review comment.
Cargo.toml (1)
81-90: No action required; review comment based on incorrect assumptions.The verification reveals the code is correct:
Legacy symbols (
UDC_LEGACY_CONTRACT_ADDRESS,deployContract) in tests are intentional. The test suite properly covers both legacy and new deployment paths (test_deploy.rs:143-147).CasmContractClass sources follow the expected pattern post-upgrade:
starknet_core::CasmContractClassin tests/server (public API from starknet-rs-core 0.16.0)cairo_lang_starknet_classes::casm_contract_class::CasmContractClassin core libraries (internal implementation)- starknet-types-core does not export CasmContractClass; this is not a concern.
The dual-source pattern reflects proper architectural layering between public API and internal implementation, and is consistent with the 0.16.0 upgrade.
Likely an incorrect or invalid review comment.
crates/starknet-devnet-core/src/starknet/add_deploy_account_transaction.rs
Show resolved
Hide resolved
tests/integration/common/utils.rs
Outdated
| pub const fn new_contract_factory<A: Account>(class_hash: Felt, account: A) -> ContractFactory<A> { | ||
| ContractFactory::new_with_udc(class_hash, account, UdcSelector::New) | ||
| } |
There was a problem hiding this comment.
new_contract_factory must not be const.
const fn cannot call non-const ContractFactory::new_with_udc. Make it a regular fn.
-pub const fn new_contract_factory<A: Account>(class_hash: Felt, account: A) -> ContractFactory<A> {
+pub fn new_contract_factory<A: Account>(class_hash: Felt, account: A) -> ContractFactory<A> {
ContractFactory::new_with_udc(class_hash, account, UdcSelector::New)
}Optionally tighten bound to ConnectedAccount for clarity.
📝 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.
| pub const fn new_contract_factory<A: Account>(class_hash: Felt, account: A) -> ContractFactory<A> { | |
| ContractFactory::new_with_udc(class_hash, account, UdcSelector::New) | |
| } | |
| pub fn new_contract_factory<A: Account>(class_hash: Felt, account: A) -> ContractFactory<A> { | |
| ContractFactory::new_with_udc(class_hash, account, UdcSelector::New) | |
| } |
🤖 Prompt for AI Agents
In tests/integration/common/utils.rs around lines 645 to 647, the function is
declared const but calls the non-const ContractFactory::new_with_udc; change the
declaration from pub const fn to a regular pub fn (i.e., remove const) so it can
call new_with_udc, and optionally tighten the generic bound from A: Account to
A: ConnectedAccount for clarity; no other logic changes required.
| )) => {} | ||
| Err(starknet_rs_accounts::AccountError::Provider(ProviderError::Other(_))) => {} | ||
| Err(error) => anyhow::bail!("Unexpected error {:?}", error), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Execution path: avoid bare ProviderError::Other acceptance
Gate on message content or use the shared helper.
Quick scan to ensure no leftover legacy UDC constants remain:
🏁 Script executed:
#!/bin/bash
rg -nP --type=rust 'UDC_.*LEGACY' || trueLength of output: 1158
🏁 Script executed:
#!/bin/bash
# Get full context around lines 420-423 in test_v3_transactions.rs
rg -n -B10 -A5 'Err\(starknet_rs_accounts::AccountError::Provider\(ProviderError::Other' tests/integration/test_v3_transactions.rsLength of output: 816
🏁 Script executed:
#!/bin/bash
# Search for other ProviderError::Other matches and patterns
rg -nP --type=rust 'ProviderError::Other' -C3Length of output: 4623
🏁 Script executed:
#!/bin/bash
# Search for helper functions or utilities for error handling
rg -nP --type=rust '(helper|Helper|shared.*error|error.*helper)' tests/integration/ -iLength of output: 262
Gate ProviderError::Other on message content or use the extract_json_rpc_error helper
Line 421 (and similar patterns at lines 48, 164, 364, 385) silently accept any provider error without inspection. The codebase provides:
- Helper function:
extract_json_rpc_errorintests/integration/common/utils.rs:474-481for structured error extraction - Precedent:
test_gas_modification.rs:377-379shows proper gating on message content using conditionals
Apply message-based guards or the helper to distinguish recoverable transient errors from genuine failures.
Usage related changes
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
Release Notes
New Features
Bug Fixes
Chores