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

120-131: Don’t hold two locks while notifying old blocks.

subscribe_new_heads locks sockets, then locks starknet and loops while notifying — this multiplies contention and can deadlock if lock ordering in other code is starknet→sockets. Gather old headers first (only starknet lock), then subscribe and send notifications without holding starknet, and avoid holding the sockets map lock during awaits as noted above.

---

81-83: Confirm off-by-one policy for “too many blocks back”.

Using “> 1024” allows exactly 1024 blocks back. If the intended limit is “at most 1024 back,” this is correct; if it should be “strictly less than 1024,” change to “>= 1024”.

---

155-159: Sender address defaulting may unintentionally filter to zero address.

unwrap_or_default() likely yields 0x0-like default, which applies an address filter instead of “no filter.” Prefer passing Option and letting AddressFilter::new(None) mean “no filter.”

Can you confirm AddressFilter::new’s semantics for a “zero” address vs None?

Also applies to: 189-193

crates/starknet-devnet/src/main.rs (1)

339-341: Consider constructing a valid loopback URL instead of using the bound address string.

If the server binds to 0.0.0.0 or ::, http://{address} is not a routable/client URL (and IPv6 needs brackets). Prefer forming http://127.0.0.1:{port} or http://[::1]:{port} accordingly, or build a Url from IpAddr + port.

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

387-395: Verify error variant stability.

unsubscribe depends on ApiError::InvalidSubscriptionId. If that variant was renamed during the refactor, this will change error semantics.

If renamed, update the match site accordingly or add a compatibility alias.

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

18-23: Derive Debug/Clone on DevnetConfig for symmetry and ergonomics

Helps logging and passing config around without re-borrowing.

-#[derive(Serialize)]
+#[derive(Debug, Clone, Serialize)]
 pub struct DevnetConfig {

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

284-299: Add tests for new error variants and GeneralError mapping

No tests cover DumpError/MessagingError/InvalidValueError/GeneralError → wildcard RPC code. Please add focused tests similar to existing ones.

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

23-25: Surface looks good; consider guarding RPC spec version drift

If other modules reference the spec version, consider centralizing or adding a quick test asserting it matches the spec reader.

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

71-83: Don’t downgrade structured errors from get_balance into GeneralError

You’re converting ApiError into GeneralError(String), losing fidelity. Propagate as-is.

-    let amount =
-        get_balance(starknet, address, erc20_address, BlockId::Tag(BlockTag::PreConfirmed))
-            .map_err(|e| ApiError::GeneralError(e.to_string()))?;
+    let amount = get_balance(
+        starknet,
+        address,
+        erc20_address,
+        BlockId::Tag(BlockTag::PreConfirmed),
+    )?;

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

650-673: Reduce lock hold time when building and enriching predeployed accounts.

You hold the Starknet mutex across the whole accounts build and per-account balance reads. Release it after fetching the list, then reacquire only for the balance loop to lower contention.

Apply this diff:

-        let mut starknet = self.api.starknet.lock().await;
-        let mut predeployed_accounts: Vec<_> = starknet
-            .get_predeployed_accounts()
-            .into_iter()
-            .map(|acc| SerializableAccount {
-                initial_balance: acc.initial_balance.to_string(),
-                address: acc.account_address,
-                public_key: acc.keys.public_key,
-                private_key: acc.keys.private_key,
-                balance: None,
-            })
-            .collect();
+        let mut predeployed_accounts: Vec<_> = {
+            let starknet = self.api.starknet.lock().await;
+            starknet
+                .get_predeployed_accounts()
+                .into_iter()
+                .map(|acc| SerializableAccount {
+                    initial_balance: acc.initial_balance.to_string(),
+                    address: acc.account_address,
+                    public_key: acc.keys.public_key,
+                    private_key: acc.keys.private_key,
+                    balance: None,
+                })
+                .collect()
+        };
@@
-        if let Some(true) =
-            params.unwrap_or(PredeployedAccountsQuery { with_balance: Option::None }).with_balance
-        {
-            for account in predeployed_accounts.iter_mut() {
+        if let Some(true) =
+            params.unwrap_or(PredeployedAccountsQuery { with_balance: Option::None }).with_balance
+        {
+            let mut starknet = self.api.starknet.lock().await;
+            for account in predeployed_accounts.iter_mut() {
                 let eth = get_balance_unit(&mut starknet, account.address, FeeUnit::WEI)?;
                 let strk = get_balance_unit(&mut starknet, account.address, FeeUnit::FRI)?;
 
                 account.balance = Some(AccountBalancesResponse { eth, strk });
             }
         }

---

663-672: Normalize error mapping for balance reads.

Here errors from get_balance_unit bubble up as-is, while devnet_getAccountBalance maps to ApiError::GeneralError. Consider consistent mapping to avoid divergent RPC error surfaces.

crates/starknet-devnet-server/src/api/models/json_rpc_request.rs (4)

243-279: Fix clippy lint attribute placement in method scope.

Inside functions, prefer inner attributes (#![...]) over outer (#[...]) on expressions to avoid attribute placement issues. Make this consistent with requires_notifying above.

-    pub fn is_forwardable_to_origin(&self) -> bool {
-        #[warn(clippy::wildcard_enum_match_arm)]
+    pub fn is_forwardable_to_origin(&self) -> bool {
+        #![warn(clippy::wildcard_enum_match_arm)]
         match self {

---

281-317: Same here: use inner attribute form.

Align is_dumpable with the inner-attribute style used elsewhere.

-    pub fn is_dumpable(&self) -> bool {
-        #[warn(clippy::wildcard_enum_match_arm)]
+    pub fn is_dumpable(&self) -> bool {
+        #![warn(clippy::wildcard_enum_match_arm)]
         match self {

---

387-401: And here for JsonRpcRequest methods.

Use inner attribute form to avoid attaching lints to expressions.

-    pub fn is_forwardable_to_origin(&self) -> bool {
-        #[warn(clippy::wildcard_enum_match_arm)]
+    pub fn is_forwardable_to_origin(&self) -> bool {
+        #![warn(clippy::wildcard_enum_match_arm)]
         match self {

---

459-481: Avoid brittle string matching for “method not found” vs “invalid params”.

Relying on error message substrings is fragile. Consider checking the method against the known rename set to decide Method Not Found; otherwise return Invalid Params.

-    serde_json::from_value::<D>(deserializable_call).map_err(|err| {
-        let err = err.to_string();
-        // since JSON-RPC specification requires returning a Method Not Found error,
-        // we apply a hacky way to decide - checking the stringified error message
-        if err.contains("Invalid method") || err.contains(&format!("unknown variant `{}`", call.method)) {
-            error!(target: "rpc", method = ?call.method, "failed to deserialize RPC call: unknown method");
-            RpcError::method_not_found()
-        } else {
-            error!(target: "rpc", method = ?call.method, ?err, "failed to deserialize RPC call: invalid params");
-            RpcError::invalid_params(err)
-        }
-    })
+    serde_json::from_value::<D>(deserializable_call).map_err(|err| {
+        let known = crate::api::models::JsonRpcRequest::all_variants_serde_renames();
+        if !known.iter().any(|m| m == &call.method) {
+            error!(target: "rpc", method = ?call.method, "failed to deserialize RPC call: unknown method");
+            RpcError::method_not_found()
+        } else {
+            let msg = err.to_string();
+            error!(target: "rpc", method = ?call.method, err = %msg, "failed to deserialize RPC call: invalid params");
+            RpcError::invalid_params(msg)
+        }
+    })

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

166-221: Tighten dry-run handling and clarify provider label.

Minor polish: compute is_dry_run via map/unwrap_or; set a clearer l1_provider label (e.g., "dry-run"). No behavior change.

-    pub async fn postman_flush(&self, data: Option<FlushParameters>) -> StrictRpcResult {
-        let is_dry_run = if let Some(params) = data { params.dry_run } else { false };
+    pub async fn postman_flush(&self, data: Option<FlushParameters>) -> StrictRpcResult {
+        let is_dry_run = data.map(|p| p.dry_run).unwrap_or(false);
@@
-        let l1_provider = if is_dry_run {
-            "dry run".to_string()
+        let l1_provider = if is_dry_run {
+            "dry-run".to_string()
         } else {

---

310-344: Mint revert reason matching is brittle.

String-matching "u256_add Overflow" may vary across toolchains/contracts. Consider normalizing with a helper that recognizes common overflow patterns or return the raw reason without rewriting.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro