Skip to content

Conversation

@danieljrc888
Copy link
Contributor

@danieljrc888 danieljrc888 commented Sep 15, 2025

Fixes DXP-666

What

  • Do not allow sim_config logic for eth_sendRawTransaction endpoint for hosted studio

Why

  • To avoid to submit mocked transactions in hosted environment

Testing done

  • Tested the bug fix

Decisions made

Checks

  • I have tested this code
  • I have reviewed my own PR
  • I have created an issue for this PR
  • I have set a descriptive PR title compliant with conventional commits

Reviewing tips

User facing release notes

Summary by CodeRabbit

  • Bug Fixes

    • Hosted deployments now consistently return a standardized error for restricted actions.
    • Submitting raw transactions with a simulation configuration in hosted environments is blocked with "Non-allowed operation" (code -32000).
    • Normal usage without a simulation configuration continues to work in hosted environments; non-hosted deployments remain unaffected.
  • Tests

    • Added comprehensive tests covering hosted vs. non-hosted behavior and simulation-config paths, including error and normal-flow scenarios.

@danieljrc888 danieljrc888 self-assigned this Sep 15, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 15, 2025

Walkthrough

Adds a centralized hosted-environment guard function and applies it in the decorator and in send_raw_transaction to block non-empty sim_config when running in hosted mode. Introduces unit tests covering hosted vs non-hosted flows and guard behavior across environment values.

Changes

Cohort / File(s) Summary
Hosted-environment guard & endpoint changes
backend/protocol_rpc/endpoints.py
Adds raise_non_allowed_in_hosted_studio(); updates check_forbidden_method_in_hosted_studio to call it; adds a check in send_raw_transaction to invoke the guard when sim_config is provided and non-empty, preserving the JSONRPCError code/message (-32000, "Non-allowed operation").
Unit tests for endpoint and guard
tests/unit/test_send_raw_transaction.py
New test module with mocks for dependencies; tests hosted vs non-hosted behavior, sim_config empty/non-empty/None cases, and direct tests of raise_non_allowed_in_hosted_studio() across various VITE_IS_HOSTED values.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant Endpoint as send_raw_transaction
  participant Guard as raise_non_allowed_in_hosted_studio
  participant Parser as transactions_parser.decode_signed_transaction
  participant Processor as transactions_processor.process_transaction

  Client->>Endpoint: send_raw_transaction(raw_tx, sim_config, ...)
  Endpoint->>Guard: validate hosted restriction (checks VITE_IS_HOSTED and sim_config)
  alt Hosted AND sim_config non-empty
    Guard-->>Endpoint: throw JSONRPCError(-32000, "Non-allowed operation")
    Endpoint-->>Client: error
  else Allowed path (not-hosted OR sim_config empty/None)
    Guard-->>Endpoint: OK
    Endpoint->>Parser: decode_signed_transaction(raw_tx)
    Parser-->>Endpoint: decoded_tx
    Endpoint->>Processor: process_transaction(decoded_tx, ...)
    Processor-->>Endpoint: tx_hash / result
    Endpoint-->>Client: result
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • cristiam86

Poem

I twitch my whiskers, check the gate,
If hosted’s "true," we pause—can’t wait.
A sim_config wanders in—oh no!
I thump my foot and stop the show.
Off-host, we hop and let the transactions go. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: dont allow sim_config param in eth_sendRawTransaction endpoint" is concise, follows conventional-commit style, and accurately summarizes the primary change—disallowing the sim_config parameter for the eth_sendRawTransaction/send_raw_transaction endpoint—as reflected by the hosted-studio guard added in the code. It clearly communicates the main intent without unnecessary detail or unrelated noise. The title is relevant to the changes in the diff and not misleading.
Description Check ✅ Passed The PR description follows the repository template and includes the Fixes line, What, Why, Testing done, and the checklist, so it meets the basic structural requirements of the template. However the "Testing done" entry is vague ("Tested the bug fix") and the "Decisions made", "Reviewing tips", and "User facing release notes" sections are empty or lack actionable detail, which reduces reviewer confidence. Adding concrete test names/commands, a short rationale or trade-offs, and a brief user-facing note would complete the description.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dxp-666-do-not-allow-mocked-validators-for-hosted-studio

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@danieljrc888 danieljrc888 force-pushed the dxp-666-do-not-allow-mocked-validators-for-hosted-studio branch from a229f67 to 972de2b Compare September 15, 2025 14:58
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (6)
backend/protocol_rpc/endpoints.py (3)

59-70: Add return type and keep env check focused.

Please add a return type to comply with our typing guideline.

-def raise_non_allowed_in_hosted_studio():
+def raise_non_allowed_in_hosted_studio() -> None:

8-8: Remove duplicate import of JSONRPCError.

flask_jsonrpc.exceptions.JSONRPCError is imported twice.

-from flask_jsonrpc.exceptions import JSONRPCError
...
-from flask_jsonrpc.exceptions import JSONRPCError
+from flask_jsonrpc.exceptions import JSONRPCError

Also applies to: 50-50


820-823: Guard placement is correct; simplify condition and confirm empty dict policy.

  • Minor: if sim_config: is sufficient since None and {} are falsy.
  • Confirm that allowing {} in hosted is intentional; it will still be written to DB as sim_config={} at insert.
-    if sim_config is not None and sim_config:
+    if sim_config:
         raise_non_allowed_in_hosted_studio()
tests/unit/test_send_raw_transaction.py (3)

49-75: Stabilize mocks, use insert_transaction, and assert the result (fixes Ruff F841).

The function calls insert_transaction, not process_transaction. Also, without stubbing get_genlayer_transaction, MagicMock truthiness can make the flow brittle. Force the SEND path and assert the returned hash.

@@
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, patch
+from types import SimpleNamespace
+from backend.domain.types import TransactionType
@@
-        mock_decoded_transaction = MagicMock()
-        mock_decoded_transaction.from_address = "0x123"
-        mock_decoded_transaction.value = 100
-        mock_decoded_transaction.data = "test_data"
-        mock_dependencies['transactions_parser'].decode_signed_transaction.return_value = mock_decoded_transaction
-        mock_dependencies['accounts_manager'].is_valid_address.return_value = True
-        mock_dependencies['transactions_parser'].transaction_has_valid_signature.return_value = True
-        mock_dependencies['transactions_processor'].process_transaction.return_value = "tx_hash"
+        mock_decoded_transaction = MagicMock()
+        mock_decoded_transaction.from_address = "0x123"
+        mock_decoded_transaction.to_address = None
+        mock_decoded_transaction.nonce = 0
+        mock_decoded_transaction.value = 100
+        mock_decoded_transaction.data = "test_data"
+        mock_dependencies['transactions_parser'].decode_signed_transaction.return_value = mock_decoded_transaction
+        mock_dependencies['accounts_manager'].is_valid_address.return_value = True
+        mock_dependencies['transactions_parser'].transaction_has_valid_signature.return_value = True
+        mock_dependencies['transactions_parser'].get_genlayer_transaction.return_value = SimpleNamespace(
+            type=TransactionType.SEND,
+            from_address=mock_decoded_transaction.from_address,
+            max_rotations=0,
+            num_of_initial_validators=0,
+        )
+        mock_dependencies['transactions_processor'].insert_transaction.return_value = "tx_hash"
@@
-            result = send_raw_transaction(
+            result = send_raw_transaction(
@@
-            # Verify the function continues execution
-            mock_dependencies['transactions_parser'].decode_signed_transaction.assert_called_once()
+            assert result == "tx_hash"
+            mock_dependencies['transactions_parser'].decode_signed_transaction.assert_called_once()

76-102: Same stabilization for hosted + sim_config=None; assert the result.

Mirror the fixes above to avoid brittle behavior and fix F841.

@@
-        mock_decoded_transaction = MagicMock()
-        mock_decoded_transaction.from_address = "0x123"
-        mock_decoded_transaction.value = 100
-        mock_decoded_transaction.data = "test_data"
-        mock_dependencies['transactions_parser'].decode_signed_transaction.return_value = mock_decoded_transaction
-        mock_dependencies['accounts_manager'].is_valid_address.return_value = True
-        mock_dependencies['transactions_parser'].transaction_has_valid_signature.return_value = True
-        mock_dependencies['transactions_processor'].process_transaction.return_value = "tx_hash"
+        mock_decoded_transaction = MagicMock()
+        mock_decoded_transaction.from_address = "0x123"
+        mock_decoded_transaction.to_address = None
+        mock_decoded_transaction.nonce = 0
+        mock_decoded_transaction.value = 100
+        mock_decoded_transaction.data = "test_data"
+        mock_dependencies['transactions_parser'].decode_signed_transaction.return_value = mock_decoded_transaction
+        mock_dependencies['accounts_manager'].is_valid_address.return_value = True
+        mock_dependencies['transactions_parser'].transaction_has_valid_signature.return_value = True
+        mock_dependencies['transactions_parser'].get_genlayer_transaction.return_value = SimpleNamespace(
+            type=TransactionType.SEND,
+            from_address=mock_decoded_transaction.from_address,
+            max_rotations=0,
+            num_of_initial_validators=0,
+        )
+        mock_dependencies['transactions_processor'].insert_transaction.return_value = "tx_hash"
@@
-            result = send_raw_transaction(
+            result = send_raw_transaction(
@@
-            # Verify the function continues execution
-            mock_dependencies['transactions_parser'].decode_signed_transaction.assert_called_once()
+            assert result == "tx_hash"
+            mock_dependencies['transactions_parser'].decode_signed_transaction.assert_called_once()

103-129: Same stabilization for hosted + empty sim_config; assert the result.

Apply the same adjustments to keep the test deterministic and fix F841.

@@
-        mock_decoded_transaction = MagicMock()
-        mock_decoded_transaction.from_address = "0x123"
-        mock_decoded_transaction.value = 100
-        mock_decoded_transaction.data = "test_data"
-        mock_dependencies['transactions_parser'].decode_signed_transaction.return_value = mock_decoded_transaction
-        mock_dependencies['accounts_manager'].is_valid_address.return_value = True
-        mock_dependencies['transactions_parser'].transaction_has_valid_signature.return_value = True
-        mock_dependencies['transactions_processor'].process_transaction.return_value = "tx_hash"
+        mock_decoded_transaction = MagicMock()
+        mock_decoded_transaction.from_address = "0x123"
+        mock_decoded_transaction.to_address = None
+        mock_decoded_transaction.nonce = 0
+        mock_decoded_transaction.value = 100
+        mock_decoded_transaction.data = "test_data"
+        mock_dependencies['transactions_parser'].decode_signed_transaction.return_value = mock_decoded_transaction
+        mock_dependencies['accounts_manager'].is_valid_address.return_value = True
+        mock_dependencies['transactions_parser'].transaction_has_valid_signature.return_value = True
+        mock_dependencies['transactions_parser'].get_genlayer_transaction.return_value = SimpleNamespace(
+            type=TransactionType.SEND,
+            from_address=mock_decoded_transaction.from_address,
+            max_rotations=0,
+            num_of_initial_validators=0,
+        )
+        mock_dependencies['transactions_processor'].insert_transaction.return_value = "tx_hash"
@@
-            result = send_raw_transaction(
+            result = send_raw_transaction(
@@
-            # Verify the function continues execution
-            mock_dependencies['transactions_parser'].decode_signed_transaction.assert_called_once()
+            assert result == "tx_hash"
+            mock_dependencies['transactions_parser'].decode_signed_transaction.assert_called_once()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e55b92a and a229f67.

📒 Files selected for processing (2)
  • backend/protocol_rpc/endpoints.py (2 hunks)
  • tests/unit/test_send_raw_transaction.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{backend,examples,tests}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Python code must be formatted with Black and include type hints

Files:

  • tests/unit/test_send_raw_transaction.py
  • backend/protocol_rpc/endpoints.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Backend tests should be written and run with pytest (unit, integration, e2e)

Files:

  • tests/unit/test_send_raw_transaction.py
backend/protocol_rpc/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Implement JSON-RPC endpoint handlers under backend/protocol_rpc/

Files:

  • backend/protocol_rpc/endpoints.py
🧬 Code graph analysis (2)
tests/unit/test_send_raw_transaction.py (3)
backend/protocol_rpc/endpoints.py (2)
  • send_raw_transaction (811-948)
  • raise_non_allowed_in_hosted_studio (60-69)
backend/protocol_rpc/transactions_parser.py (2)
  • decode_signed_transaction (67-165)
  • transaction_has_valid_signature (235-239)
backend/database_handler/accounts_manager.py (1)
  • is_valid_address (50-51)
backend/protocol_rpc/endpoints.py (1)
backend/protocol_rpc/message_handler/base.py (1)
  • wrapper (160-211)
🪛 Ruff (0.12.2)
tests/unit/test_send_raw_transaction.py

63-63: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)


90-90: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)


117-117: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)

🪛 GitHub Actions: pre-commit
tests/unit/test_send_raw_transaction.py

[error] 1-1: Command 'pre-commit run --show-diff-on-failure --color=always --all-files' failed: trailing-whitespace hook modified the file.


[error] 1-1: Black formatting changed tests/unit/test_send_raw_transaction.py. Re-run pre-commit to re-apply formatting.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Load Tests / load-test
  • GitHub Check: test
  • GitHub Check: load-test
  • GitHub Check: backend-unit-tests
🔇 Additional comments (5)
backend/protocol_rpc/endpoints.py (1)

73-79: Decorator centralization LGTM.

Calling the shared guard keeps policy in one place.

tests/unit/test_send_raw_transaction.py (4)

31-47: Hosted + non-empty sim_config test looks good.

Asserts error code/message/data match the guard.


148-160: Helper tests LGTM.

Covers hosted=true path thoroughly.


161-180: Non-hosted variants LGTM.

Good coverage of false/not-set/other values.


1-4: Run pre-commit locally and fix trailing whitespace / Black formatting

Verification here failed with "/bin/bash: line 3: pre-commit: command not found". Run pre-commit locally, fix trailing whitespace and Black formatting in tests/unit/test_send_raw_transaction.py, then push.

Local commands:
pip install --user pre-commit black
pre-commit run --all-files
pytest -q tests/unit/test_send_raw_transaction.py

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
tests/unit/test_send_raw_transaction.py (4)

76-76: Remove unused result assignments (Ruff F841).

They’re not asserted and trigger linter errors.

-            result = send_raw_transaction(
+            send_raw_transaction(
@@
-            result = send_raw_transaction(
+            send_raw_transaction(
@@
-            result = send_raw_transaction(
+            send_raw_transaction(

Also applies to: 113-113, 150-150


58-66: Stabilize mocks to avoid hidden MagicMock truthiness.

Explicitly set consensus_service.web3.is_connected.return_value = False in “non-hosted/hosted no-sim/empty-sim” tests so the consensus branch can’t raise inadvertently if add_transaction returns None.

         mock_dependencies["accounts_manager"].is_valid_address.return_value = True
         mock_dependencies[
             "transactions_parser"
         ].transaction_has_valid_signature.return_value = True
-        mock_dependencies["transactions_processor"].insert_transaction.return_value = "tx_hash"
+        mock_dependencies["transactions_processor"].insert_transaction.return_value = "tx_hash"
+        mock_dependencies["consensus_service"].web3.is_connected.return_value = False

Apply the same addition to the other two tests that continue execution (hosted with sim_config=None and hosted with sim_config={}).

Also applies to: 103-107, 140-146


165-183: LGTM: duplicate coverage for hosted + non‑empty sim_config.

Redundant with the first test but okay. Consider parametrization to reduce duplication.


210-217: Optional: parametrize the “other values” cases.

Cleaner with @pytest.mark.parametrize("value", [...]).

-    def test_does_not_raise_when_hosted_is_other_value(self):
-        """Test that the function does not raise error when VITE_IS_HOSTED has other values"""
-        test_values = ["TRUE", "True", "1", "yes", "on", ""]
-
-        for value in test_values:
-            with patch.dict(os.environ, {"VITE_IS_HOSTED": value}):
-                # Should not raise an exception for any value other than exactly "true"
-                raise_non_allowed_in_hosted_studio()
+    @pytest.mark.parametrize("value", ["TRUE", "True", "1", "yes", "on", ""])
+    def test_does_not_raise_when_hosted_is_other_value(self, value: str) -> None:
+        """Does not raise for any value other than exactly 'true'."""
+        with patch.dict(os.environ, {"VITE_IS_HOSTED": value}):
+            raise_non_allowed_in_hosted_studio()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a229f67 and 972de2b.

📒 Files selected for processing (2)
  • backend/protocol_rpc/endpoints.py (2 hunks)
  • tests/unit/test_send_raw_transaction.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/protocol_rpc/endpoints.py
🧰 Additional context used
📓 Path-based instructions (2)
{backend,examples,tests}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Python code must be formatted with Black and include type hints

Files:

  • tests/unit/test_send_raw_transaction.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Backend tests should be written and run with pytest (unit, integration, e2e)

Files:

  • tests/unit/test_send_raw_transaction.py
🧬 Code graph analysis (1)
tests/unit/test_send_raw_transaction.py (3)
backend/protocol_rpc/endpoints.py (2)
  • send_raw_transaction (811-948)
  • raise_non_allowed_in_hosted_studio (60-69)
backend/protocol_rpc/transactions_parser.py (2)
  • decode_signed_transaction (67-165)
  • transaction_has_valid_signature (235-239)
backend/database_handler/accounts_manager.py (1)
  • is_valid_address (50-51)
🪛 Ruff (0.12.2)
tests/unit/test_send_raw_transaction.py

76-76: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)


113-113: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)


150-150: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Load Tests / load-test
  • GitHub Check: test
  • GitHub Check: load-test
🔇 Additional comments (2)
tests/unit/test_send_raw_transaction.py (2)

34-53: LGTM: hosted + non‑empty sim_config is correctly blocked.

This asserts the JSON-RPC error shape and code as intended.


188-197: LGTM: guard helper behavior when hosted=true is validated.

Asserts code, message, and data.

@danieljrc888 danieljrc888 force-pushed the dxp-666-do-not-allow-mocked-validators-for-hosted-studio branch from 972de2b to 0846ad0 Compare September 15, 2025 15:23
@sonarqubecloud
Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
tests/unit/test_send_raw_transaction.py (2)

18-27: Add missing type hints per repo policy (tests must include type hints).

Annotate fixtures and tests. This mirrors earlier feedback and aligns with Coding Guidelines.

@@
-    @pytest.fixture
-    def mock_dependencies(self):
+    @pytest.fixture
+    def mock_dependencies(self) -> dict[str, MagicMock]:
@@
-    @pytest.fixture
-    def valid_signed_transaction(self):
+    @pytest.fixture
+    def valid_signed_transaction(self) -> str:
@@
-    def test_send_raw_transaction_with_sim_config_in_hosted_environment(
-        self, mock_dependencies, valid_signed_transaction
-    ):
+    def test_send_raw_transaction_with_sim_config_in_hosted_environment(
+        self,
+        mock_dependencies: dict[str, MagicMock],
+        valid_signed_transaction: str,
+    ) -> None:
@@
-    def test_send_raw_transaction_with_sim_config_in_non_hosted_environment(
-        self, mock_dependencies, valid_signed_transaction
-    ):
+    def test_send_raw_transaction_with_sim_config_in_non_hosted_environment(
+        self,
+        mock_dependencies: dict[str, MagicMock],
+        valid_signed_transaction: str,
+    ) -> None:
@@
-    def test_send_raw_transaction_without_sim_config_in_hosted_environment(
-        self, mock_dependencies, valid_signed_transaction
-    ):
+    def test_send_raw_transaction_without_sim_config_in_hosted_environment(
+        self,
+        mock_dependencies: dict[str, MagicMock],
+        valid_signed_transaction: str,
+    ) -> None:
@@
-    def test_send_raw_transaction_with_empty_sim_config_in_hosted_environment(
-        self, mock_dependencies, valid_signed_transaction
-    ):
+    def test_send_raw_transaction_with_empty_sim_config_in_hosted_environment(
+        self,
+        mock_dependencies: dict[str, MagicMock],
+        valid_signed_transaction: str,
+    ) -> None:
@@
-    def test_send_raw_transaction_with_non_empty_sim_config_in_hosted_environment(
-        self, mock_dependencies, valid_signed_transaction
-    ):
+    def test_send_raw_transaction_with_non_empty_sim_config_in_hosted_environment(
+        self,
+        mock_dependencies: dict[str, MagicMock],
+        valid_signed_transaction: str,
+    ) -> None:
@@
-    def test_raises_error_when_hosted_is_true(self):
+    def test_raises_error_when_hosted_is_true(self) -> None:
@@
-    def test_does_not_raise_when_hosted_is_false(self):
+    def test_does_not_raise_when_hosted_is_false(self) -> None:
@@
-    def test_does_not_raise_when_hosted_not_set(self):
+    def test_does_not_raise_when_hosted_not_set(self) -> None:

Also applies to: 29-33, 34-36, 54-56, 91-93, 128-130, 165-167, 188-188, 198-198, 204-204


70-72: Resolved: using insert_transaction in mocks.

Previous review flagged mocking the wrong method; this now targets insert_transaction correctly.

Also applies to: 107-109, 144-146

🧹 Nitpick comments (4)
tests/unit/test_send_raw_transaction.py (4)

76-84: Resolve Ruff F841: use or drop the unused result variables.

Assert the function’s return value to both satisfy Ruff and strengthen checks.

@@
-            result = send_raw_transaction(
+            result = send_raw_transaction(
                 transactions_processor=mock_dependencies["transactions_processor"],
                 msg_handler=mock_dependencies["msg_handler"],
                 accounts_manager=mock_dependencies["accounts_manager"],
                 transactions_parser=mock_dependencies["transactions_parser"],
                 consensus_service=mock_dependencies["consensus_service"],
                 signed_rollup_transaction=valid_signed_transaction,
                 sim_config={"some": "config"},
             )
+            assert result == "tx_hash"
@@
-            result = send_raw_transaction(
+            result = send_raw_transaction(
                 transactions_processor=mock_dependencies["transactions_processor"],
                 msg_handler=mock_dependencies["msg_handler"],
                 accounts_manager=mock_dependencies["accounts_manager"],
                 transactions_parser=mock_dependencies["transactions_parser"],
                 consensus_service=mock_dependencies["consensus_service"],
                 signed_rollup_transaction=valid_signed_transaction,
                 sim_config=None,
             )
+            assert result == "tx_hash"
@@
-            result = send_raw_transaction(
+            result = send_raw_transaction(
                 transactions_processor=mock_dependencies["transactions_processor"],
                 msg_handler=mock_dependencies["msg_handler"],
                 accounts_manager=mock_dependencies["accounts_manager"],
                 transactions_parser=mock_dependencies["transactions_parser"],
                 consensus_service=mock_dependencies["consensus_service"],
                 signed_rollup_transaction=valid_signed_transaction,
                 sim_config={},
             )
+            assert result == "tx_hash"

Also applies to: 113-121, 150-158


86-90: Strengthen verification: assert DB write path invoked.

Assert insert_transaction was called to ensure the non-hosted/allowed flows reach persistence.

@@
             ].decode_signed_transaction.assert_called_once()
+            mock_dependencies["transactions_processor"].insert_transaction.assert_called_once()
@@
             ].decode_signed_transaction.assert_called_once()
+            mock_dependencies["transactions_processor"].insert_transaction.assert_called_once()
@@
             ].decode_signed_transaction.assert_called_once()
+            mock_dependencies["transactions_processor"].insert_transaction.assert_called_once()

Also applies to: 123-127, 160-164


59-66: Stabilize mocks: set to_address and nonce on decoded tx.

Prevents brittle reliance on MagicMock defaults as endpoint code reads these attributes.

@@
         mock_decoded_transaction.from_address = "0x123"
         mock_decoded_transaction.value = 100
         mock_decoded_transaction.data = "test_data"
+        mock_decoded_transaction.to_address = "0x456"
+        mock_decoded_transaction.nonce = 1
@@
         mock_decoded_transaction.from_address = "0x123"
         mock_decoded_transaction.value = 100
         mock_decoded_transaction.data = "test_data"
+        mock_decoded_transaction.to_address = "0x456"
+        mock_decoded_transaction.nonce = 1
@@
         mock_decoded_transaction.from_address = "0x123"
         mock_decoded_transaction.value = 100
         mock_decoded_transaction.data = "test_data"
+        mock_decoded_transaction.to_address = "0x456"
+        mock_decoded_transaction.nonce = 1

Also applies to: 96-103, 133-140


210-217: Parametrize “other env values” test instead of manual loop.

Clearer failure reporting and aligns with pytest style.

-    def test_does_not_raise_when_hosted_is_other_value(self):
-        """Test that the function does not raise error when VITE_IS_HOSTED has other values"""
-        test_values = ["TRUE", "True", "1", "yes", "on", ""]
-
-        for value in test_values:
-            with patch.dict(os.environ, {"VITE_IS_HOSTED": value}):
-                # Should not raise an exception for any value other than exactly "true"
-                raise_non_allowed_in_hosted_studio()
+    @pytest.mark.parametrize("value", ["TRUE", "True", "1", "yes", "on", ""])
+    def test_does_not_raise_when_hosted_is_other_value(self, value: str) -> None:
+        """Test that the function does not raise error when VITE_IS_HOSTED has other values"""
+        with patch.dict(os.environ, {"VITE_IS_HOSTED": value}):
+            # Should not raise an exception for any value other than exactly "true"
+            raise_non_allowed_in_hosted_studio()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 972de2b and 0846ad0.

📒 Files selected for processing (2)
  • backend/protocol_rpc/endpoints.py (2 hunks)
  • tests/unit/test_send_raw_transaction.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/protocol_rpc/endpoints.py
🧰 Additional context used
📓 Path-based instructions (2)
{backend,examples,tests}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Python code must be formatted with Black and include type hints

Files:

  • tests/unit/test_send_raw_transaction.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Backend tests should be written and run with pytest (unit, integration, e2e)

Files:

  • tests/unit/test_send_raw_transaction.py
🧬 Code graph analysis (1)
tests/unit/test_send_raw_transaction.py (4)
backend/protocol_rpc/endpoints.py (2)
  • send_raw_transaction (811-948)
  • raise_non_allowed_in_hosted_studio (60-69)
backend/protocol_rpc/transactions_parser.py (2)
  • decode_signed_transaction (67-165)
  • transaction_has_valid_signature (235-239)
backend/database_handler/accounts_manager.py (1)
  • is_valid_address (50-51)
backend/database_handler/transactions_processor.py (1)
  • insert_transaction (216-290)
🪛 Ruff (0.12.2)
tests/unit/test_send_raw_transaction.py

76-76: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)


113-113: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)


150-150: Local variable result is assigned to but never used

Remove assignment to unused variable result

(F841)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Load Tests / load-test
  • GitHub Check: test
  • GitHub Check: backend-unit-tests
  • GitHub Check: load-test
🔇 Additional comments (1)
tests/unit/test_send_raw_transaction.py (1)

34-53: LGTM: Hosted guard error-path assertions are correct.

Accurately checks JSON-RPC error code/message and blocks non-empty sim_config in hosted mode.

Also applies to: 165-183, 188-197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants