tests/integration/test_abort_blocks.rs (1)

36-47: Include tx hash in error for easier debugging

If this ever fails, the tx hash in the error is handy.

-        match devnet.json_rpc_client.get_transaction_by_hash(tx_hash).await {
+        match devnet.json_rpc_client.get_transaction_by_hash(tx_hash).await {
             Err(ProviderError::StarknetError(StarknetError::TransactionHashNotFound)) => (),
-            other => anyhow::bail!("Unexpected tx response: {other:?}"),
+            other => anyhow::bail!("Unexpected tx response for {tx_hash:#x}: {other:?}"),
         }

tests/integration/test_fork.rs (1)

309-312: Minor: simplify the match into unwrap_or_else

Functionally identical, but a tad cleaner.

-match assert_tx_succeeded_accepted(&result.transaction_hash, &devnet.json_rpc_client).await {
-    Ok(_) => (),
-    Err(e) => panic!("Transaction failed: {}", e),
-};
+assert_tx_succeeded_accepted(&result.transaction_hash, &devnet.json_rpc_client)
+    .await
+    .unwrap_or_else(|e| panic!("Transaction failed: {e}"));

tests/integration/test_v3_transactions.rs (1)

309-312: Minor: prefer unwrap_or_else over manual match

Same behavior, slightly tighter.

-match assert_tx_succeeded_accepted(&result.transaction_hash, &devnet.json_rpc_client).await {
-    Ok(_) => (),
-    Err(e) => panic!("Transaction failed: {}", e),
-};
+assert_tx_succeeded_accepted(&result.transaction_hash, &devnet.json_rpc_client)
+    .await
+    .unwrap_or_else(|e| panic!("Transaction failed: {e}"));

tests/integration/common/utils.rs (4)

105-123: Add tx-hash context to errors for faster triage

Bubbling provider errors is fine, but including the tx hash in messages makes debugging much faster. Also include the tx hash in assertion failures.

 pub async fn assert_tx_succeeded_accepted<T: Provider>(
     tx_hash: &Felt,
     client: &T,
 ) -> Result<(), anyhow::Error> {
-    let receipt = match client.get_transaction_receipt(tx_hash).await {
-        Ok(receipt) => receipt.receipt,
-        Err(err) => return Err(err.into()),
-    };
+    let receipt = match client.get_transaction_receipt(tx_hash).await {
+        Ok(receipt) => receipt.receipt,
+        Err(err) => return Err(anyhow::anyhow!("Failed to fetch receipt for {tx_hash:#x}: {err}")),
+    };

     match receipt.execution_result() {
         ExecutionResult::Succeeded => (),
-        other => anyhow::bail!("Should have succeeded; got: {other:?}"),
+        other => anyhow::bail!("Tx {tx_hash:#x} should have succeeded; got: {other:?}"),
     }

     match receipt.finality_status() {
-        starknet_rs_core::types::TransactionFinalityStatus::AcceptedOnL2 => Ok(()),
-        other => anyhow::bail!("Should have been accepted on L2; got: {other:?}"),
+        starknet_rs_core::types::TransactionFinalityStatus::AcceptedOnL2 => Ok(()),
+        other => anyhow::bail!("Tx {tx_hash:#x} should have been accepted on L2; got: {other:?}"),
     }
 }

---

165-187: Include tx hash in revert-path errors

Helpful when reading CI logs.

-        other => anyhow::bail!("Should have reverted; got: {other:?}; receipt: {receipt:?}"),
+        other => anyhow::bail!(
+            "Tx {tx_hash:#x} should have reverted; got: {other:?}; receipt: {receipt:?}"
+        ),

---

457-473: Remove panic and redundant length check

You both assert and then check lengths; keep a single non-panicking path that returns an error.

-pub fn assert_equal_elements<T>(iterable1: &[T], iterable2: &[T]) -> Result<(), anyhow::Error>
+pub fn assert_equal_elements<T>(iterable1: &[T], iterable2: &[T]) -> Result<(), anyhow::Error>
 where
     T: PartialEq,
 {
-    assert_eq!(iterable1.len(), iterable2.len());
-    match iterable1.len() == iterable2.len() {
-        true => (),
-        false => return Err(anyhow::anyhow!("Length mismatch")),
-    }
+    if iterable1.len() != iterable2.len() {
+        return Err(anyhow::anyhow!(
+            "Length mismatch: left={}, right={}",
+            iterable1.len(),
+            iterable2.len()
+        ));
+    }
     for e in iterable1 {
-        match iterable2.contains(e) {
-            true => (),
-            false => return Err(anyhow::anyhow!("Element mismatch")),
-        }
+        if !iterable2.contains(e) {
+            return Err(anyhow::anyhow!("Element mismatch"));
+        }
     }
     Ok(())
 }

---

612-622: Don’t rely on error strings for timeout detection

String matching on “deadline has elapsed” is brittle. Downcast to tokio’s Elapsed instead.

-    match receive_rpc_via_ws(ws).await {
-        Ok(resp) => anyhow::bail!("Expected no notifications; found: {resp}"),
-        Err(e) if e.to_string().contains("deadline has elapsed") => {
-            /* expected */
-            Ok(())
-        }
-        Err(e) => anyhow::bail!("Expected to error out due to empty channel; found: {e}"),
-    }
+    match receive_rpc_via_ws(ws).await {
+        Ok(resp) => anyhow::bail!("Expected no notifications; found: {resp}"),
+        Err(e) if e.downcast_ref::<tokio::time::error::Elapsed>().is_some() => Ok(()), // expected
+        Err(e) => anyhow::bail!("Expected timeout due to empty channel; found: {e}"),
+    }

tests/integration/test_subscription_with_invalid_block_id.rs (1)

37-37: LGTM: unwrap on assert_no_notifications matches new helper contract.

Consider .expect("no notifications expected") for slightly clearer failures, but optional.

Also applies to: 61-61

tests/integration/test_subscription_to_new_txs.rs (1)

27-27: DRY option: wrap repeated .await.unwrap() in a tiny macro.

Many identical calls to assert_no_notifications(&mut ws).await.unwrap();. Optional macro reduces noise:

+// in a common test module (e.g., tests/integration/common/mod.rs) or at file top:
+macro_rules! assert_no_notifs {
+    ($ws:expr) => {{
+        crate::common::utils::assert_no_notifications($ws)
+            .await
+            .expect("Unexpected WS notifications");
+    }};
+}

Then replace lines like:

-assert_no_notifications(&mut ws).await.unwrap();
+assert_no_notifs!(&mut ws);

Also applies to: 47-47, 75-75, 98-98, 123-123, 161-161, 175-175, 195-195, 217-217, 237-237, 252-252, 269-269, 289-289, 311-311, 315-315, 334-334

tests/integration/test_subscription_to_events.rs (1)

107-107: LGTM; optional helper to reduce repetition.

Same macro pattern as suggested in new_txs can shorten these .await.unwrap() repetitions and standardize the failure message.

Also applies to: 112-112, 142-142, 173-173, 203-203, 249-250, 315-316, 339-340, 392-392, 425-425, 444-444, 495-495, 510-510, 553-553

tests/integration/test_subscription_to_tx_status.rs (1)

85-85: LGTM; consider a shared macro for clearer, consistent failures.

Adopt the assert_no_notifs! macro (see earlier comment) to reduce boilerplate and give uniform panic text.

Also applies to: 94-94, 109-109, 125-125, 136-136, 162-162, 175-175, 211-211, 229-229, 234-234, 256-256, 257-257, 275-275, 299-299, 321-321

tests/integration/test_subscription_to_reorg.rs (1)

62-62: LGTM; optional macro for the no-notifications checks.

Switching to a shared assert_no_notifs! macro would keep these concise across tests.

Also applies to: 125-125

tests/integration/test_account_impersonation.rs (3)

203-203: Good: helpers now return Result; unwrapping at call sites matches PR intent.

Using .unwrap() on assert_contains(...) centralizes failures at the call site and improves trace quality. Consider .expect("context") where extra context helps future debugging.

Also applies to: 271-272

---

269-272: Make contains-check robust to caller casing.

Lowercase both haystack and needle to avoid case fragility.

 fn assert_anyhow_error_contains_message(error: anyhow::Error, message: &str) {
   let error_string = format!("{:?}", error.root_cause()).to_lowercase();
-  assert_contains(&error_string, message).unwrap();
+  let message = message.to_lowercase();
+  assert_contains(&error_string, &message).unwrap();
 }

---

245-265: Balance assertion is directionally correct; consider clarifying intent.

initial >= amount + final implies ≥ amount spent (plus fees). A short comment can prevent misreads (amount vs fee vs token).

tests/integration/test_call.rs (1)

142-154: LGTM: Result-returning extractors now unwrapped at the assertion site.

This preserves precise failure locations. If failures are noisy, switch to .expect("failed to extract nested error") for clarity.

tests/integration/test_subscription_to_new_tx_receipts.rs (1)

52-52: LGTM: assert_no_notifications unwrapped across scenarios.

Pattern is consistent and improves failure locality. If this becomes repetitive, consider a tiny helper assert_no_notifications_ok(ws).await? in utils and let tests return anyhow::Result<()>.

Also applies to: 72-72, 104-104, 129-129, 154-154, 177-177, 216-216, 231-231, 250-250, 271-271, 293-293, 309-309, 328-328, 350-350, 377-377, 381-381, 403-403

tests/integration/test_advancing_time_on_fork.rs (2)

67-79: Explicit unwraps match the new Result-returning helpers — OK; consider expect(...) for context.

Current approach satisfies PR goals. Optionally add short expect("extraction failed at <step>") messages to speed up triage when this fails.

---

142-142: Guard against missing revert_reason to avoid a panic.

revert_reason() returns an Option; use expect to provide context if it’s None.

-            assert_contains(tx_details.revert_reason().unwrap(), "Wait a bit more").unwrap();
+            assert_contains(
+                tx_details.revert_reason().as_deref().expect("missing revert_reason"),
+                "Wait a bit more",
+            ).unwrap();

tests/integration/test_deploy.rs (1)

71-82: Result-based error extraction and assertions look good.

Unwraps are consistent with the helpers now returning Result. If you want slightly clearer failures, swap a few unwrap()s for expect("reason") (optional).

Also applies to: 111-111, 140-141, 166-167

tests/integration/test_advancing_time.rs (2)

631-643: Good: explicit unwraps on nested/message extraction align with the new signatures.

No functional issues spotted. Optional: replace some unwrap()s with expect("context") to make failures self-descriptive.

---

689-689: Minor: provide context when unwrapping revert reason.

Same nit as elsewhere; expect("missing revert_reason") would clarify failures.

tests/integration/test_estimate_fee.rs (2)

279-289: Simplify revert assertion control flow.

Use expect to shorten and keep intent crisp.

-    match assert_tx_reverted(
-        &unsuccessful_invoke_tx.transaction_hash,
-        &devnet.json_rpc_client,
-        &["Insufficient max L2Gas"],
-    )
-    .await
-    {
-        Ok(_) => {}
-        Err(e) => panic!("Transaction not reverted: {:?}", e),
-    };
+    assert_tx_reverted(
+        &unsuccessful_invoke_tx.transaction_hash,
+        &devnet.json_rpc_client,
+        &["Insufficient max L2Gas"],
+    )
+    .await
+    .expect("Transaction not reverted");

---

367-372: Optional: factor repetitive nested-error unwrap chain into a tiny helper.

A small test-only utility like fn deepest_msg(err: &ExecutionError) -> String would remove repetition across tests.

tests/integration/test_messaging.rs (1)

1041-1041: Minor: stabilize revert reason unwrapping.

Consider expect("missing revert_reason") for clearer panic if provider omits it.

tests/integration/test_accepting_blocks_on_l1.rs (2)

21-37: Make helper fully Result-based (avoid assert!/unwrap! inside).

Propagate errors via ?/ensure! to align with the PR goal and improve failure locality.

Apply:

 async fn assert_accepted_on_l1(
     devnet: &BackgroundDevnet,
     block_hashes: &[Felt],
     tx_hashes: &[Felt],
 ) -> Result<(), anyhow::Error> {
     for block_hash in block_hashes {
         match devnet.json_rpc_client.get_block_with_tx_hashes(BlockId::Hash(*block_hash)).await {
-            Ok(MaybePreConfirmedBlockWithTxHashes::Block(block)) => {
-                assert_eq!(block.status, BlockStatus::AcceptedOnL1)
-            }
+            Ok(MaybePreConfirmedBlockWithTxHashes::Block(block)) => {
+                anyhow::ensure!(
+                    block.status == BlockStatus::AcceptedOnL1,
+                    "expected block {block_hash:?} to be ACCEPTED_ON_L1, got {:?}",
+                    block.status
+                );
+            }
             other => anyhow::bail!("Unexpected block: {other:?}"),
         }
     }
 
     for tx_hash in tx_hashes {
-        let tx_status = devnet.json_rpc_client.get_transaction_status(tx_hash).await.unwrap();
-        assert_eq!(tx_status.finality_status(), SequencerTransactionStatus::AcceptedOnL1);
+        let tx_status = devnet.json_rpc_client.get_transaction_status(tx_hash).await?;
+        anyhow::ensure!(
+            tx_status.finality_status() == SequencerTransactionStatus::AcceptedOnL1,
+            "expected tx {tx_hash:?} to be ACCEPTED_ON_L1, got {:?}",
+            tx_status.finality_status()
+        );
     }
 
     Ok(())
 }

---

68-69: Consistent unwrap messaging at call sites.

Optional: use .expect("assert_accepted_on_l1 failed") for clearer failure context.

Example:

-assert_accepted_on_l1(&devnet, &block_hashes, &tx_hashes).await.unwrap();
+assert_accepted_on_l1(&devnet, &block_hashes, &tx_hashes)
+    .await
+    .expect("assert_accepted_on_l1 failed");

Also applies to: 90-91, 112-114, 135-136

tests/integration/test_account_selection.rs (2)

152-160: Simplify to expect/unwrap for consistency.

Replace verbose match + panic with .await.expect(...) to match surrounding style.

-    match assert_tx_succeeded_accepted(
-        &account_deployment.transaction_hash,
-        &devnet.json_rpc_client,
-    )
-    .await
-    {
-        Ok(_) => (),
-        Err(e) => panic!("Transaction failed: {}", e),
-    };
+    assert_tx_succeeded_accepted(
+        &account_deployment.transaction_hash,
+        &devnet.json_rpc_client,
+    )
+    .await
+    .expect("Transaction failed");

---

81-83: Optional: add context to unwraps.

Use .expect("tx should be ACCEPTED_ON_L2") to improve failure messages.

Also applies to: 95-97, 109-111, 138-140, 214-216

tests/integration/test_blocks_generation.rs (2)

100-127: Make assert_latest_block_with_receipts fully Result-driven (no panics).

Use ensure! and ? to avoid assert_eq! panics and manual matches.

 async fn assert_latest_block_with_receipts(
     devnet: &BackgroundDevnet,
     block_number: u64,
     tx_count: usize,
 ) -> Result<(), anyhow::Error> {
     let latest_block = &devnet
         .send_custom_rpc("starknet_getBlockWithReceipts", json!({ "block_id": "latest" }))
         .await
         .unwrap();
 
-    assert_eq!(latest_block["transactions"].as_array().unwrap().len(), tx_count);
-    assert_eq!(latest_block["block_number"], block_number);
-    assert_eq!(latest_block["status"], "ACCEPTED_ON_L2");
+    anyhow::ensure!(
+        latest_block["transactions"].as_array().unwrap().len() == tx_count,
+        "unexpected tx count in latest block"
+    );
+    anyhow::ensure!(latest_block["block_number"] == block_number, "unexpected block number");
+    anyhow::ensure!(latest_block["status"] == "ACCEPTED_ON_L2", "unexpected block status");
 
     for tx in latest_block["transactions"].as_array().unwrap() {
-        match assert_tx_succeeded_accepted(
-            &Felt::from_hex_unchecked(tx["receipt"]["transaction_hash"].as_str().unwrap()),
-            &devnet.json_rpc_client,
-        )
-        .await
-        {
-            Ok(_) => {}
-            Err(e) => anyhow::bail!("Transaction failed: {}", e),
-        };
+        assert_tx_succeeded_accepted(
+            &Felt::from_hex_unchecked(tx["receipt"]["transaction_hash"].as_str().unwrap()),
+            &devnet.json_rpc_client,
+        )
+        .await?;
     }
 
     Ok(())
 }

---

249-252: Avoid usize math/casts when building Felt.

Prefer u128 to prevent platform-dependent usize casts.

-    assert_balance(&devnet, Felt::from(tx_count * DUMMY_AMOUNT as usize), BlockTag::PreConfirmed)
+    assert_balance(
+        &devnet,
+        Felt::from((tx_count as u128) * DUMMY_AMOUNT),
+        BlockTag::PreConfirmed,
+    )
         .await;
-    assert_balance(&devnet, Felt::ZERO, BlockTag::Latest).await;
+    assert_balance(&devnet, Felt::ZERO, BlockTag::Latest).await;

-    assert_balance(&devnet, Felt::from(tx_count * DUMMY_AMOUNT as usize), BlockTag::PreConfirmed)
+    assert_balance(
+        &devnet,
+        Felt::from((tx_count as u128) * DUMMY_AMOUNT),
+        BlockTag::PreConfirmed,
+    )
         .await;
-    assert_balance(&devnet, Felt::from(tx_count * DUMMY_AMOUNT as usize), BlockTag::Latest).await;
+    assert_balance(
+        &devnet,
+        Felt::from((tx_count as u128) * DUMMY_AMOUNT),
+        BlockTag::Latest,
+    )
+    .await;

Also applies to: 264-267

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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/common/utils.rs (1)

443-458: Consider set-based comparison for large inputs.

Current O(n²) search is fine for small test slices; if inputs can grow, prefer HashSet to avoid quadratic cost. Requires T: Eq + Hash.

Example:

use std::collections::HashSet;
anyhow::ensure!(HashSet::<&T>::from_iter(iterable1) == HashSet::<&T>::from_iter(iterable2));

tests/integration/test_messaging.rs (1)

982-987: Re: borrowing from temporary JSON — this pattern is valid

Referencing a field of a temporary serde_json::Value is OK here due to Rust’s temporary lifetime extension. Previous warning was incorrect.

tests/integration/common/errors.rs (1)

52-59: Nice: RpcError now implements Error; consider improving Display output.

Current Display prints Debug, which is noisy in logs. Prefer a concise "code:message" format.

 impl fmt::Display for RpcError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{self:?}")
+        match &self.data {
+            Some(data) => write!(f, "RPC error {}: {} | data={}", self.code, self.message, data),
+            None => write!(f, "RPC error {}: {}", self.code, self.message),
+        }
     }
 }

tests/integration/test_dump_and_load.rs (1)

22-39: Avoid unwrap in Result-returning helper; propagate with ?

This aligns with the PR’s goal (no panics in helpers).

-        devnet_dump.create_block().await.unwrap();
+        devnet_dump.create_block().await?;

tests/integration/test_advancing_time.rs (1)

33-38: Prevent potential u64 overflow when computing upper_limit.

Use checked_add (error) or saturating_add (clamp) instead of plain +.

 pub fn assert_ge_with_buffer(val1: u64, val2: u64) -> Result<(), anyhow::Error> {
     anyhow::ensure!(val1 >= val2, "Failed inequation: {val1:?} >= {val2:?}");
-    let upper_limit = val2 + BUFFER_TIME_SECONDS;
+    let upper_limit = val2
+        .checked_add(BUFFER_TIME_SECONDS)
+        .ok_or_else(|| anyhow::anyhow!("timestamp overflow computing upper limit"))?;
     anyhow::ensure!(val1 <= upper_limit, "Failed inequation: {val1:?} <= {upper_limit:?}");
     Ok(())
 }
 
 pub fn assert_gt_with_buffer(val1: u64, val2: u64) -> Result<(), anyhow::Error> {
     anyhow::ensure!(val1 > val2, "Failed inequation: {val1:?} > {val2:?}");
-    let upper_limit = val2 + BUFFER_TIME_SECONDS;
+    let upper_limit = val2
+        .checked_add(BUFFER_TIME_SECONDS)
+        .ok_or_else(|| anyhow::anyhow!("timestamp overflow computing upper limit"))?;
     anyhow::ensure!(val1 <= upper_limit, "Failed inequation: {val1:?} <= {upper_limit:?}");
     Ok(())
 }

Also applies to: 40-45

tests/integration/test_account_selection.rs (1)

44-65: Add messages to ensure! checks for clearer failures.

Current ensures are silent on mismatches; include expected vs. actual.

-    anyhow::ensure!(retrieved_class_hash == expected_hash);
+    anyhow::ensure!(
+        retrieved_class_hash == expected_hash,
+        "unexpected class hash: got {retrieved_class_hash:#x}, expected {expected_hash:#x}"
+    );
@@
-    anyhow::ensure!(Felt::from_hex_unchecked(config_class_hash_hex) == expected_hash);
+    anyhow::ensure!(
+        Felt::from_hex_unchecked(config_class_hash_hex) == expected_hash,
+        "config.account_contract_class_hash mismatch: got {config_class_hash_hex}, expected {expected_hash_hex}"
+    );

tests/integration/common/utils.rs (5)

122-134: Convert pre-confirmed helper to Result for consistency.

Keep it feature‑gated or stubbed, but make the signature return Result to align with this PR’s goal and call‑sites.

Apply this minimal stub until starknet-rs 0.17 is adopted:

-pub async fn assert_tx_succeeded_pre_confirmed<T: Provider>(_tx_hash: &Felt, _client: &T) {
-    eprintln!("TODO: Currently suppressed; undo when starknet-rs updated to 0.17");
+pub async fn assert_tx_succeeded_pre_confirmed<T: Provider>(
+    _tx_hash: &Felt,
+    _client: &T,
+) -> Result<(), anyhow::Error> {
+    // TODO: Re-enable real checks when starknet-rs is updated to 0.17.
+    Ok(())

---

222-226: Avoid unwrap on ABI extraction.

as_str().unwrap() can panic if ABI isn’t a string; return a structured error instead.

Apply:

 fn take_abi_from_json(value: &mut serde_json::Value) -> Result<serde_json::Value, anyhow::Error> {
     let abi_jsonified = value["abi"].take();
-    anyhow::ensure!(abi_jsonified != serde_json::json!(null));
-    Ok(serde_json::from_str(abi_jsonified.as_str().unwrap())?)
+    anyhow::ensure!(abi_jsonified != serde_json::json!(null), "Missing `abi` field");
+    let abi_str = abi_jsonified
+        .as_str()
+        .ok_or_else(|| anyhow::anyhow!("`abi` field is not a string"))?;
+    Ok(serde_json::from_str(abi_str)?)
 }

---

470-486: Remove downcast unwrap to avoid panic.

Gracefully handle unexpected provider error types.

-            let impl_specific_error: &JsonRpcClientError<HttpTransportError> =
-                provider_impl_error.as_any().downcast_ref().unwrap();
-            match impl_specific_error {
+            if let Some(impl_specific_error) =
+                provider_impl_error.as_any().downcast_ref::<JsonRpcClientError<HttpTransportError>>()
+            {
+                match impl_specific_error {
                 JsonRpcClientError::JsonRpcError(json_rpc_error) => Ok(json_rpc_error.clone()),
-                other => {
-                    Err(anyhow::Error::msg(format!("Cannot extract RPC error from: {:?}", other)))
-                }
-            }
+                other => Err(anyhow::anyhow!("Cannot extract RPC error from: {other:?}")),
+                }
+            } else {
+                Err(anyhow::anyhow!("Provider error is not JsonRpcClientError; got: {provider_impl_error:?}"))
+            }

---

588-597: Add context to ensures for easier diffs.

Including expected/actual in error text speeds up debugging.

-    anyhow::ensure!(notification["jsonrpc"] == "2.0");
-    anyhow::ensure!(notification["method"] == method);
+    anyhow::ensure!(notification["jsonrpc"] == "2.0", "jsonrpc != 2.0: {notification}");
+    anyhow::ensure!(notification["method"] == method, "method mismatch: {notification}");

---

601-609: Match timeout by type, not substring.

String-matching “deadline has elapsed” is brittle; downcast to tokio timeout error.

 match receive_rpc_via_ws(ws).await {
-    Ok(resp) => anyhow::bail!("Expected no notifications; found: {resp}"),
-    Err(e) if e.to_string().contains("deadline has elapsed") => Ok(()), /* expected */
+    Ok(resp) => anyhow::bail!("Expected no notifications; found: {resp}"),
+    Err(e) if e.downcast_ref::<tokio::time::error::Elapsed>().is_some() => Ok(()), /* expected */
     Err(e) => anyhow::bail!("Expected to error out due to empty channel; found: {e}"),
 }

tests/integration/test_subscription_to_tx_status.rs (2)

34-54: Prefer field-wise checks over whole-JSON equality.

Object order or unrelated fields could cause fragile failures; assert specific fields instead.

-    anyhow::ensure!(
-        notification
-            == json!({
-                "jsonrpc": "2.0",
-                "method": "starknet_subscriptionTransactionStatus",
-                "params": {
-                    "result": {
-                        "transaction_hash": tx_hash,
-                        "status": {
-                            "finality_status": expected_finality_status,
-                            "failure_reason": null,
-                            "execution_status": "SUCCEEDED",
-                        },
-                    },
-                    "subscription_id": subscription_id,
-                }
-            })
-    );
+    anyhow::ensure!(notification["jsonrpc"] == "2.0", "bad jsonrpc: {notification}");
+    anyhow::ensure!(notification["method"] == "starknet_subscriptionTransactionStatus", "bad method: {notification}");
+    anyhow::ensure!(notification["params"]["subscription_id"] == subscription_id, "bad sub id: {notification}");
+    let result = &notification["params"]["result"];
+    anyhow::ensure!(result["transaction_hash"] == tx_hash, "bad tx hash: {notification}");
+    anyhow::ensure!(result["status"]["execution_status"] == "SUCCEEDED", "bad exec status: {notification}");
+    anyhow::ensure!(result["status"]["finality_status"] == expected_finality_status, "bad finality: {notification}");
+    anyhow::ensure!(result["status"]["failure_reason"].is_null(), "unexpected failure_reason: {notification}");

---

141-179: Use ? instead of unwrap() inside Result-returning helper.

Keep propagation consistent and avoid test panics.

-        let notification = receive_rpc_via_ws(ws_before_tx).await.unwrap();
+        let notification = receive_rpc_via_ws(ws_before_tx).await?;
@@
-        let notification = receive_rpc_via_ws(ws_after_tx).await?;
+        let notification = receive_rpc_via_ws(ws_after_tx).await?;

tests/integration/test_gas_modification.rs (2)

174-184: Avoid float-to-integer casts for big constants.

8.5e18 as u128 loses precision; use integer literals.

-    let l1_fri_price = 8.5e18 as u128;
-    let l1_data_fri_price = 7.5e18 as u128;
-    let l2_fri_price = 6.5e18 as u128;
+    let l1_fri_price: u128 = 8_500_000_000_000_000_000;
+    let l1_data_fri_price: u128 = 7_500_000_000_000_000_000;
+    let l2_fri_price: u128 = 6_500_000_000_000_000_000;
@@
-        "gas_price_wei": 9e18 as u128,
+        "gas_price_wei": 9_000_000_000_000_000_000_u128,
@@
-        "data_gas_price_wei": 8e18 as u128,
+        "data_gas_price_wei": 8_000_000_000_000_000_000_u128,
@@
-        "l2_gas_price_wei": 7e18 as u128,
+        "l2_gas_price_wei": 7_000_000_000_000_000_000_u128,
@@
-    let new_l2_fri_price = 9e7 as u128; // approximate upper limit that will pass
+    let new_l2_fri_price: u128 = 90_000_000; // approximate upper limit that will pass
@@
-        "gas_price_wei": 9e18 as u128,
-        "data_gas_price_wei": 8e18 as u128,
-        "l2_gas_price_wei": 7.5e18 as u128,
-        "gas_price_fri": 7e18 as u128,
-        "data_gas_price_fri": 6e18 as u128,
-        "l2_gas_price_fri": 5.5e18 as u128,
+        "gas_price_wei": 9_000_000_000_000_000_000_u128,
+        "data_gas_price_wei": 8_000_000_000_000_000_000_u128,
+        "l2_gas_price_wei": 7_500_000_000_000_000_000_u128,
+        "gas_price_fri": 7_000_000_000_000_000_000_u128,
+        "data_gas_price_fri": 6_000_000_000_000_000_000_u128,
+        "l2_gas_price_fri": 5_500_000_000_000_000_000_u128,

Also applies to: 189-201, 203-217, 448-455

---

193-201: Optional: validate response shape before indexing [0].

Safer to check array length and bail with a clear message.

let resp = devnet.send_custom_rpc("starknet_simulateTransactions", params).await?;
let first = resp.get(0).ok_or_else(|| anyhow::anyhow!("empty simulateTransactions response"))?;

Also applies to: 203-217

tests/integration/test_v3_transactions.rs (1)

321-343: Eliminate unwraps inside Result-returning helper; propagate with ?.

Prevents accidental panics if callers change Action/options.

-        .await
-        .map_err(|e| anyhow::Error::msg(e.to_string()))?,
+        .await
+        .map_err(|e| anyhow::Error::msg(e.to_string()))?,
@@
-                    DeclarationV3::new(sierra_class.clone(), *casm_hash, account.unwrap())
+                    DeclarationV3::new(
+                        sierra_class.clone(),
+                        *casm_hash,
+                        account.ok_or(anyhow::anyhow!("Account not found"))?,
+                    )
@@
-                match declaration.send().await.unwrap_err() {
+                match declaration.send().await.unwrap_err() {
@@
-                let account_deployment = AccountDeploymentV3::new(*salt, account_factory.unwrap())
+                let account_deployment = AccountDeploymentV3::new(
+                    *salt,
+                    account_factory.ok_or(anyhow::anyhow!("Account Factory is None"))?,
+                )
@@
-                let execution = ExecutionV3::new(calls.clone(), account.unwrap())
+                let execution = ExecutionV3::new(
+                    calls.clone(),
+                    account.ok_or(anyhow::anyhow!("Account not found"))?,
+                )
@@
-                                assert_contains(reason.as_str(), "Insufficient max L2Gas").unwrap();
+                                assert_contains(reason.as_str(), "Insufficient max L2Gas")?;

Also applies to: 358-373, 375-390, 392-399, 401-423, 427-428

tests/integration/test_abort_blocks.rs (2)

24-38: Make “block not found” assertion robust (avoid brittle full-equality on RpcError)

Comparing whole RpcError (including exact message and data) is fragile. Match code and check substring on message instead.

-    anyhow::ensure!(err == RpcError { code: 24, message: "Block not found".into(), data: None });
-    Ok(())
+    assert_contains(&err.message, "Block not found")?;
+    anyhow::ensure!(err.code == 24);
+    Ok(())

---

40-51: Tighten match: handle Ok separately for clearer failures

Currently Ok() and unexpected Err() are merged. Split them for sharper diagnostics.

-    for tx_hash in tx_hashes {
-        match devnet.json_rpc_client.get_transaction_by_hash(tx_hash).await {
-            Err(ProviderError::StarknetError(StarknetError::TransactionHashNotFound)) => (),
-            other => anyhow::bail!("Unexpected tx response for {tx_hash:#x}: {other:?}"),
-        }
-    }
+    for tx_hash in tx_hashes {
+        match devnet.json_rpc_client.get_transaction_by_hash(tx_hash).await {
+            Err(ProviderError::StarknetError(StarknetError::TransactionHashNotFound)) => {}
+            Ok(_) => anyhow::bail!("Unexpectedly found tx {tx_hash:#x}"),
+            Err(e) => anyhow::bail!("Unexpected error for {tx_hash:#x}: {e:?}"),
+        }
+    }

tests/integration/test_minting.rs (1)

11-48: Avoid unwraps in Result-returning helper; harden tx_hash check

Use ? instead of unwrap and validate tx_hash presence before starts_with.

-    let tx_hash_value = resp_body["tx_hash"].take();
-    anyhow::ensure!(tx_hash_value.as_str().unwrap().starts_with("0x"));
+    let tx_hash_value = resp_body["tx_hash"].take();
+    let tx_hash_str = tx_hash_value
+        .as_str()
+        .ok_or_else(|| anyhow::anyhow!("tx_hash missing or not a string"))?;
+    anyhow::ensure!(tx_hash_str.starts_with("0x"));
@@
-    let new_balance =
-        devnet.get_balance_latest(&Felt::from_hex_unchecked(address), unit).await.unwrap();
+    let new_balance =
+        devnet.get_balance_latest(&Felt::from_hex_unchecked(address), unit).await?;

tests/integration/test_accepting_blocks_on_l1.rs (1)

31-37: Prefer ? over unwrap() in helper to preserve error context

This helper already returns Result; propagate provider errors directly.

-    for tx_hash in tx_hashes {
-        let tx_status = devnet.json_rpc_client.get_transaction_status(tx_hash).await.unwrap();
-        anyhow::ensure!(tx_status.finality_status() == SequencerTransactionStatus::AcceptedOnL1);
-    }
+    for tx_hash in tx_hashes {
+        let tx_status = devnet.json_rpc_client.get_transaction_status(tx_hash).await?;
+        anyhow::ensure!(tx_status.finality_status() == SequencerTransactionStatus::AcceptedOnL1);
+    }

tests/integration/test_messaging.rs (1)

196-225: Don’t unwrap() inside a Result-returning helper

Propagate the balance query error with ?.

-    anyhow::ensure!(get_balance(devnet, from_address, user).await.unwrap() == [Felt::ZERO]);
+    anyhow::ensure!(get_balance(devnet, from_address, user).await? == [Felt::ZERO]);

tests/integration/test_blocks_generation.rs (1)

271-275: Avoid architecture-dependent arithmetic with usize when creating Felts

Use explicit u128 to prevent surprises across platforms.

-    assert_balance(&devnet, Felt::from(tx_count * DUMMY_AMOUNT as usize), BlockTag::PreConfirmed)
+    assert_balance(&devnet, Felt::from((tx_count as u128) * DUMMY_AMOUNT), BlockTag::PreConfirmed)
@@
-    assert_balance(&devnet, Felt::from(tx_count * DUMMY_AMOUNT as usize), BlockTag::PreConfirmed)
+    assert_balance(&devnet, Felt::from((tx_count as u128) * DUMMY_AMOUNT), BlockTag::PreConfirmed)
@@
-    assert_balance(&devnet, Felt::from(tx_count * DUMMY_AMOUNT as usize), BlockTag::Latest)
+    assert_balance(&devnet, Felt::from((tx_count as u128) * DUMMY_AMOUNT), BlockTag::Latest)

Also applies to: 287-293

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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.

Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_messaging.rs:960-964
Timestamp: 2025-09-16T08:50:01.391Z
Learning: In Rust, when binding a reference to a field or method result of a temporary value (like `let ref_var = &temporary.field` or `let ref_var = &temporary["key"]`), the temporary's lifetime is extended to match the lifetime of the reference binding. This prevents dangling references and makes patterns like `let generated_l2_txs_raw = &devnet.send_custom_rpc(...).await.unwrap()["generated_l2_transactions"];` perfectly valid.