crates/starknet-devnet-core/src/transactions.rs (2)

57-57: Remove stray TODO marker.

The previous #[allow(unused)] was intentionally dropped; the TODO comment is noise now. Delete it.

-// TODO #[allow(unused)]

---

70-76: Pass TransactionWithHash by value to avoid clone and strip the inline note.

Callers already own TransactionWithHash and compute the hash before passing it, so moving it eliminates an extra clone here.

-    pub fn pre_confirm(
-        transaction: &TransactionWithHash, // consider passing the object, not reference
+    pub fn pre_confirm(
+        transaction: TransactionWithHash,
         execution_info: TransactionExecutionInfo,
         trace: TransactionTrace,
         pre_confirmed_block_number: BlockNumber,
     ) -> Self {
         Self {
             finality_status: TransactionFinalityStatus::PreConfirmed,
@@
-            inner: transaction.clone(),
+            inner: transaction,

Also update call sites (examples):

• crates/starknet-devnet-core/src/starknet/mod.rs (Lines 451–456): pass transaction (no &).
• Tests in this file (Lines 279, 289, 312): pass tx.clone() to keep later equality checks using tx. See patch below for tests.

-        let sn_tx = StarknetTransaction::pre_confirm(
-            &tx,
+        let sn_tx = StarknetTransaction::pre_confirm(
+            tx.clone(),
             TransactionExecutionInfo::default(),
             trace.clone(),
             block_number,
         );
@@
-            StarknetTransaction::pre_confirm(
-                &tx,
+            StarknetTransaction::pre_confirm(
+                tx.clone(),
                 TransactionExecutionInfo::default(),
                 trace,
                 block_number,
             ),
@@
-        let sn_tran = StarknetTransaction::pre_confirm(
-            &tx,
+        let sn_tran = StarknetTransaction::pre_confirm(
+            tx.clone(),
             TransactionExecutionInfo::default(),
             trace,
             block_number,
         );

crates/starknet-devnet-core/src/starknet/mod.rs (2)

392-400: Optional invariant check: ensure tx.block_number matches the finalized block.

Block number is now set during pre_confirm; add a debug assertion here to guard against future divergence.

-        // update block hash for each pre_confirmed tx; block number should already be set
+        // update block hash for each pre_confirmed tx; block number should already be set
         new_block.get_transactions().iter().for_each(|tx_hash| {
             if let Some(tx) = self.transactions.get_by_hash_mut(tx_hash) {
+                debug_assert_eq!(
+                    tx.block_number,
+                    Some(new_block_number),
+                    "pre_confirmed tx has block_number {:?}, expected {:?}",
+                    tx.block_number,
+                    new_block_number
+                );
                 tx.block_hash = Some(new_block_hash);
                 tx.finality_status = TransactionFinalityStatus::AcceptedOnL2;
             } else {
                 error!("Transaction is not present in the transactions collection");
             }
         });

---

451-456: If adopting pass-by-value, update this call.

Drop & to avoid cloning in pre_confirm.

-        let transaction_to_add = StarknetTransaction::pre_confirm(
-            &transaction,
+        let transaction_to_add = StarknetTransaction::pre_confirm(
+            transaction,
             tx_info,
             trace,
             self.blocks.pre_confirmed_block.block_number(),
         );

crates/starknet-devnet-server/src/subscribe.rs (2)

332-336: Use structured tracing for cheaper logging.

Avoid string interpolation in the message; emit as a field to keep formatting lazy and searchable.

-        tracing::trace!("Subscription response: {resp_serialized}");
+        tracing::trace!(target: "ws.subscriptions", response = %resp_serialized, "subscription response");

---

367-374: Confirm unsubscribe API semantics (error vs. false) and consider idempotency.

Changing missing-ID behavior to ApiError::InvalidSubscriptionId alters the contract; many WS APIs return result=false instead. If you prefer idempotent unsubscribe, consider this tweak.

-        self.subscriptions.remove(&subscription_id).ok_or(ApiError::InvalidSubscriptionId)?;
-        self.send(SubscriptionResponse::Confirmation {
-            rpc_request_id,
-            result: SubscriptionConfirmation::Unsubscription(true),
-        })
+        let ok = self.subscriptions.remove(&subscription_id).is_some();
+        self.send(SubscriptionResponse::Confirmation {
+            rpc_request_id,
+            result: SubscriptionConfirmation::Unsubscription(ok),
+        })
             .await;
         Ok(())

crates/starknet-devnet-server/src/api/json_rpc/endpoints_ws.rs (2)

142-166: NewTx subscriptions now “future-only” — LGTM; drop unnecessary clone.

Behavior aligns with PR goal (no pre-population). Minor nit: StatusFilter is moved; cloning is unnecessary.

-        let subscription =
-            Subscription::NewTransactions { address_filter, status_filter: status_filter.clone() };
+        let subscription =
+            Subscription::NewTransactions { address_filter, status_filter };

---

189-205: Receipts subscriptions also “future-only” — LGTM; drop unnecessary clone.

Same clone nit as above.

-        let subscription = Subscription::NewTransactionReceipts {
-            address_filter,
-            status_filter: status_filter.clone(),
-        };
+        let subscription = Subscription::NewTransactionReceipts {
+            address_filter,
+            status_filter,
+        };

tests/integration/test_subscription_to_new_txs.rs (2)

23-35: Make shared WS helpers a common module to decouple tests.

These helpers are reused across integration tests; consider moving them under tests/common (e.g., tests/common/ws.rs) and importing from there to avoid inter-test coupling.

---

121-144: Unsubscribe flow looks stable; tiny robustness tweak optional.

Draining one WS message before unsubscribe helps ordering. If flakes surface, consider draining all pending messages (with a short timeout) before asserting the unsubscribe ack.

tests/integration/test_subscription_to_new_tx_receipts.rs (1)

161-181: Guard against out-of-order notifications when subscribing to both streams.

TX and receipt events can interleave. If flakes appear, read two messages per iteration and branch by method instead of assuming TX arrives first.

-        let tx_raw = receive_new_tx(&mut ws, tx_subscription_id.clone()).await.unwrap();
+        // Read until we have both a tx and a receipt for this iteration
+        let mut maybe_tx: Option<serde_json::Value> = None;
+        let mut maybe_receipt: Option<serde_json::Value> = None;
+        while maybe_tx.is_none() || maybe_receipt.is_none() {
+            // Try TX first, then receipt; ignore non-matching messages
+            if maybe_tx.is_none() {
+                if let Ok(val) = receive_new_tx(&mut ws, tx_subscription_id.clone()).await {
+                    maybe_tx = Some(val);
+                    continue;
+                }
+            }
+            if maybe_receipt.is_none() {
+                if let Ok(val) = receive_new_tx_receipt(&mut ws, receipt_subscription_id.clone()).await {
+                    maybe_receipt = Some(val);
+                }
+            }
+        }
+        let tx_raw = maybe_tx.unwrap();
+        let receipt_raw = maybe_receipt.unwrap();
-        let receipt_raw =
-            receive_new_tx_receipt(&mut ws, receipt_subscription_id.clone()).await.unwrap();

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.