crates/starknet-devnet-server/src/api/http/endpoints/postman.rs (2)

75-81: Make lock release explicit; clarify the comment to reference the lock guard.

The chained call holds the Starknet lock until fetch_messages_to_l2().await completes (which is fine), and then drops it before executing RPCs. To make this intent unambiguous and resilient to future edits, consider scoping the guard explicitly and rewording the comment to reference the lock guard rather than api.starknet.

Apply this diff to scope the lock and clarify the comment:

-        // It is important that api.starknet is dropped immediately to allow rpc execution
-        messages_to_l2 = api.starknet.lock().await.fetch_messages_to_l2().await.map_err(|e| {
-            ApiError::RpcError(RpcError::internal_error_with(format!(
-                "Error in fetching messages to L2: {e}"
-            )))
-        })?;
+        // Important: acquire a short-lived Starknet lock only to fetch messages;
+        // drop it before executing JSON-RPC calls to avoid deadlocks.
+        {
+            let mut starknet = api.starknet.lock().await;
+            messages_to_l2 = starknet.fetch_messages_to_l2().await.map_err(|e| {
+                ApiError::RpcError(RpcError::internal_error_with(format!(
+                    "Error in fetching messages to L2: {e}"
+                )))
+            })?;
+        } // lock dropped here

---

95-111: Tighten Starknet lock around L1 calls & confirm dry-run side effects

• collect_messages_to_l1 mutates internal state (it extends l2_to_l1_messages_to_flush and advances last_local_block). In dry-run mode this will enqueue messages and bump the block pointer—please confirm this behavior is intended.
• Reduce the scope of api.starknet.lock().await to only each async operation (collect, send, get_ethereum_url) to minimize mutex contention around potentially slow network calls.

Suggested refactor:

-    let mut starknet = api.starknet.lock().await;
-    let messages_to_l1 = starknet.collect_messages_to_l1().await.map_err(|e| {
-        ApiError::RpcError(RpcError::internal_error_with(format!(
-            "Error in collecting messages to L1: {e}"
-        )))
-    })?;
-
-    let l1_provider = if is_dry_run {
-        "dry run".to_string()
-    } else {
-        starknet.send_messages_to_l1().await.map_err(|e| {
-            ApiError::RpcError(RpcError::internal_error_with(format!(
-                "Error in sending messages to L1: {e}"
-            )))
-        })?;
-        starknet.get_ethereum_url().unwrap_or("Not set".to_string())
-    };
+    // Lock only for collecting messages
+    let messages_to_l1 = {
+        let mut starknet = api.starknet.lock().await;
+        starknet.collect_messages_to_l1().await.map_err(|e| {
+            ApiError::RpcError(RpcError::internal_error_with(format!(
+                "Error in collecting messages to L1: {e}"
+            )))
+        })?
+    };
+
+    let l1_provider = if is_dry_run {
+        "dry run".to_string()
+    } else {
+        // Lock only for sending
+        {
+            let mut starknet = api.starknet.lock().await;
+            starknet.send_messages_to_l1().await.map_err(|e| {
+                ApiError::RpcError(RpcError::internal_error_with(format!(
+                    "Error in sending messages to L1: {e}"
+                )))
+            })?;
+        }
+        // Short lock just to read the URL
+        api.starknet
+            .lock()
+            .await
+            .get_ethereum_url()
+            .unwrap_or_else(|| "Not set".to_string())
+    };

website/docs/intro.md (2)

18-25: Tighten phrasing and punctuation in the new finality semantics bullets.

Minor grammar/punctuation issues. Suggest:

• End the last bullet with a period, not a comma.

Apply this minimal fix:

-  - Transactions are never automatically `ACCEPTED_ON_L1`, unless the user performs an action,
+  - Transactions are never automatically `ACCEPTED_ON_L1` unless the user performs an action.

---

14-17: Typos: “transactions” singular and “free” → “fee”.

Two small nits above the changed block for clarity:

- - By default, a new block is mined for each new transactions.
+ - By default, a new block is mined for each new transaction.
   - This can be modified by directing all new transactions into a pre-confirmed block, and at some point triggering block creation.
-  - Transactions in a pre-confirmed block cannot be replaced by sending a transaction with a higher free from the same account.
+  - Transactions in a pre-confirmed block cannot be replaced by sending a transaction with a higher fee from the same account.

crates/starknet-devnet-types/src/rpc/transaction_receipt.rs (1)

39-49: Consider returning the finality status by value to avoid tying callers to a borrow

If TransactionFinalityStatus is Clone (likely) or Copy, returning by value improves ergonomics and avoids exposing an internal borrow.

Apply this diff if acceptable:

-impl TransactionReceipt {
-    pub fn finality_status(&self) -> &TransactionFinalityStatus {
+impl TransactionReceipt {
+    pub fn finality_status(&self) -> TransactionFinalityStatus {
         let common = match self {
             TransactionReceipt::Deploy(receipt) => &receipt.common,
             TransactionReceipt::L1Handler(receipt) => &receipt.common,
             TransactionReceipt::Common(common) => common,
         };
 
-        &common.finality_status
+        common.finality_status.clone()
     }
 }

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

1201-1222: get_events now supports finality filtering — consider an options struct to reduce arg count

Functionally correct. To avoid growing argument lists and the need for allow(too_many_arguments), consider introducing a GetEventsOptions struct to carry address/keys/finality/skip/limit in a single parameter.

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

105-114: Clarify semantics of get_sender_address across variants (especially Deploy/DeployAccount).

The mapping looks correct w.r.t. how we surface “sender”:

• Declare/Invoke: actual sender_address
• DeployAccount: derived contract address
• L1Handler: contract handling the L1 message
• Deploy: None

To avoid ambiguity for downstream users, please document this contract explicitly at the API point.

Apply this inline doc to make the intent unambiguous:

 impl Transaction {
+    /// Returns the logical initiator address for the transaction:
+    /// - DECLARE / INVOKE: the `sender_address`
+    /// - DEPLOY_ACCOUNT: the derived `contract_address` of the deployed account
+    /// - L1_HANDLER: the `contract_address` handling the message
+    /// - DEPLOY: `None` (no L2 sender)
     pub fn gas_vector_computation_mode(&self) -> GasVectorComputationMode {

---

1024-1032: Be explicit about ordering semantics for TransactionFinalityStatus and update affected documentation

• In crates/starknet-devnet-types/src/rpc/transactions.rs, add a doc comment above the enum to spell out the ascending progression and guard against variant reordering.
• (Optional) Add a unit test that asserts the expected ordering, e.g.:

 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord, Copy)]
 #[serde(rename_all = "SCREAMING_SNAKE_CASE")]
+/// Finality progresses in ascending order as declared below; do not reorder
+/// without adjusting comparisons relying on `PartialOrd`:
+/// RECEIVED < CANDIDATE < PRE_CONFIRMED < ACCEPTED_ON_L2 < ACCEPTED_ON_L1
 pub enum TransactionFinalityStatus {
     Received,
     Candidate,
     PreConfirmed,
     AcceptedOnL2,
     AcceptedOnL1,
 }

• Update the docs in website/docs/intro.md (around line 21) to remove the legacy “PENDING” reference and simply describe the new status:

-  - `PRE_CONFIRMED` behaves like the old `PENDING` status - it is the status assigned to the transactions in a pre-confirmed block, which is a precursor to a stable latest block.
+  - `PRE_CONFIRMED` is the status assigned to transactions in a pre-confirmed block, which is a precursor to a stable latest block.

• You can re-run the provided script to ensure no other “Pending…” references remain:

rg -n -C2 -g '!**/target/**' -P '\bPending(Transaction|Transactions)\b|PENDING(_ON_\w+)?'

tests/integration/test_subscription_to_new_txs.rs (1)

140-175: Sender-address filtering coverage looks solid; consider also covering Deploy (None) sender.

You cover Declare and Invoke via a predeployed account, which is great. Since Transaction::get_sender_address() returns None for plain Deploy, consider adding a negative test asserting that a sender_address filter does not match a Deploy tx, to lock in semantics.

I can add a small test that deploys via UDC (or any Deploy path) and asserts no notifications when filtering by a concrete sender address. Want me to draft it?

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

11-22: Update function docs to include finality_status_filter.

The signature gained finality_status_filter, but the docstring doesn’t mention it. Please include it and clarify semantics (“returns events whose transaction finality >= filter”).

 /// * `keys_filter` - Optional. The keys to filter the events by.
-/// * `skip` - The number of elements to skip.
-/// * `limit` - Optional. The maximum number of elements to return.
+/// * `finality_status_filter` - Optional. Minimum transaction finality required for events to be included.
+///   Events from txs with finality strictly lower than the filter are skipped.
+/// * `skip` - The number of elements to skip (applied after filtering).
+/// * `limit` - Optional. The maximum number of elements to return.
 #[allow(clippy::too_many_arguments)]
 pub(crate) fn get_events(

---

44-49: Finality filter logic aligns with derived ordering.

if transaction.finality_status < finality_status_filter { continue } correctly implements “minimum finality” semantics when combined with the new enum ordering.

Two nits:

• Minor shadowing: consider renaming the inner finality_status_filter binding to avoid shadowing the arg.
• Add a unit test covering this filter (e.g., PreConfirmed filter excludes events from a Received tx but includes AcceptedOnL2/L1).

I can add a focused unit test around get_events to assert the filter behavior using a small in-memory setup. Want me to include it?

tests/integration/test_subscription_to_events.rs (2)

206-249: Pre-confirmed-only subscription behavior is pinned well.

Asserting no re-notify after pre_confirmed -> latest confirms dedup semantics for the same event across finalities. Good coverage.

Two small suggestions:

• Consider one extra assertion that block_hash/block_number are absent in the pre-confirmed notification (you already implicitly do by not including them in the expected JSON).
• Add a comment clarifying that finality_status in payload reflects the tx’s current finality at emission time.

---

251-327: Avoid brittle assumption on block_number; use runtime value.

Hardcoding "block_number": 1 may become flaky if the devnet boots with a different genesis/state in future. Capture the latest block after create_block() and assert that number instead.

Apply this refactor:

-    let created_block_hash = devnet.create_block().await.unwrap();
+    let created_block_hash = devnet.create_block().await.unwrap();
+    let created_block = devnet.get_latest_block_with_tx_hashes().await.unwrap();
...
-            json!({
-                "block_hash": created_block_hash,
-                "block_number": 1, // the only created block
+            json!({
+                "block_hash": created_block_hash,
+                "block_number": created_block.block_number,
                 "transaction_hash": invocation.transaction_hash,
                 "from_address": contract_address,
                 "keys": [static_event_key()],
                 "data": [],
                 "finality_status": finality_status,
             })

tests/integration/test_subscription_to_new_tx_receipts.rs (1)

167-189: Sender filter on receipts: solid; consider adding explicit Received/Candidate subscription case.

Given the PR note that subscribing with CANDIDATE or RECEIVED maps to PRE_CONFIRMED, consider a minimal extra test where finality_status: [RECEIVED] still yields a PreConfirmed receipt notification. That will lock in the “promotion” logic.

If helpful, I can draft a small test mirroring this one but with finality_status: [TransactionFinalityStatus::Received] and assert the received receipt has finality_status: PreConfirmed.

tests/integration/test_subscription_to_tx_status.rs (1)

197-211: Minor ergonomics nit: avoid moving sockets if you plan to reuse them.

Using for (mut ws, subscription_id) in [(ws_before_tx, id_before), (ws_after_tx, id_after)] moves both sockets. It’s fine here since you don’t reuse them later in this test, but in similar patterns consider iterating over mutable references to avoid accidental moves if future assertions are added.

Apply this localized refactor for future-proofing:

-    for (mut ws, subscription_id) in
-        [(ws_before_tx, subscription_id_before), (ws_after_tx, subscription_id_after)]
-    {
-        let notification = receive_rpc_via_ws(&mut ws).await.unwrap();
+    for (ws, subscription_id) in
+        [&mut ws_before_tx, &mut ws_after_tx].into_iter().zip([subscription_id_before, subscription_id_after])
+    {
+        let notification = receive_rpc_via_ws(ws).await.unwrap();
-        assert_no_notifications(&mut ws).await;
+        assert_no_notifications(ws).await;
     }

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

14-15: Coupling to subscribe.rs types is acceptable, but keep layering in mind.

Importing TransactionFinalityStatusWithoutL1 and TransactionStatusWithoutL1 from crate::subscribe keeps enums single-sourced but ties models to the subscription layer. If these inputs are reused outside WS, consider moving the enums to a neutral module (e.g., types or models) to avoid circular concerns later.

---

251-256: EventsSubscriptionInput: consider documenting finality semantics.

You accept a single finality_status for events. Downstream code uses it to both filter and determine the historical scan window. A short doc comment here would prevent misuse and clarify that “no value” defaults to L2-accepted.

crates/starknet-devnet-server/src/api/json_rpc/mod.rs (1)

411-420: Prefer stable error message without “should be defined.”

The internal error string is fine for now, but consider a more actionable message (e.g., “No aborted block recorded in state”). Not critical.

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

176-226: Transactions subscription: inputs OK; consider semantics for RECEIVED/CANDIDATE.

You accept TransactionStatusWithoutL1 (including RECEIVED/CANDIDATE), but the server only emits PRE_CONFIRMED and ACCEPTED_ON_L2 notifications for transactions. As-is, filters containing only RECEIVED/CANDIDATE will never match. If that’s by design, document it; otherwise, map RECEIVED/CANDIDATE to PRE_CONFIRMED similarly to receipts.

Would you like a small patch that normalizes RECEIVED|CANDIDATE to PRE_CONFIRMED for NewTransactions subscriptions?

---

229-281: Receipts subscription: good filter construction; same lock-holding caveat.

The filter/scan logic is sound. However, as in other subs, you hold the sockets mutex while sending the initial backlog. Consider reducing lock scope to the subscribe step and perform notifications without holding the collection lock.

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

395-401: Notification payload for receipts drops sender_address.

You include sender_address in NewTransactionReceiptNotification for filtering, but the serialized notification sends only the TransactionReceipt. If the spec later requires including sender for receipts, you’ll need to adjust SubscriptionNotification::NewTransactionReceipt accordingly. For now, this is fine and intentional.

Configuration used: .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.