fix: RPC should return error on failed contract deployment instead of address#1276
fix: RPC should return error on failed contract deployment instead of address#1276kstroobants wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdded an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint
participant TransactionsProcessor
Note over Endpoint: get_transaction_by_hash / get_block_by_hash
Client->>Endpoint: Request transaction by hash
Endpoint->>TransactionsProcessor: get_transaction_by_hash(hash, hide_fields=True)
TransactionsProcessor->>TransactionsProcessor: _process_round_data -> evaluate hide_fields & type
alt Conditions for showing contract met
TransactionsProcessor-->>Endpoint: Return transaction with contract fields
else
TransactionsProcessor-->>Endpoint: Return transaction with contract fields redacted/removed
end
Endpoint-->>Client: Respond with transaction data
sequenceDiagram
participant Client
participant Endpoint
participant TransactionsProcessor
Note over Endpoint: get_transaction_receipt flow
Client->>Endpoint: Request transaction receipt
Endpoint->>TransactionsProcessor: get_transaction_by_hash(hash, hide_fields=True)
TransactionsProcessor-->>Endpoint: Return transaction
Endpoint->>Endpoint: Check type == DEPLOY_CONTRACT, last_round.result==6, leader_receipt exists, execution_result==SUCCESS
alt All conditions met
Endpoint-->>Client: Respond with contractAddress = transaction["to_address"]
else
Endpoint-->>Client: Respond with contractAddress = None
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
PR SummaryHere's a concise summary of the Pull Request changes based on the provided reviews:
Conclusion: This appears to be an empty PR with identical pre/post code. Developers should clarify its purpose before merging. |
|
Sherlock AI FindingsThe automated tool identified the following potential security issues in the codebase. Please review the details for each issue in the linked dashboard.
Next Steps: Review the linked issues in the dashboard and address high-severity bugs first. Contact the team if you need assistance. Full report available at: https://ai.sherlock.xyz/runs/6251b583-ea18-4eca-a78b-d11a7eec2560 |
danieljrc888
force-pushed
the
dxp-186-rpc-should-return-error-on-failed-contract-deployment
branch
from
1835cb4 to
821a454
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/protocol_rpc/endpoints.py (1)
1424-1431: Missing None check for transaction causes crash if not found.
get_transaction_by_hashcan returnNone. If the transaction doesn't exist, line 1430 will raiseAttributeError: 'NoneType' object has no attribute 'get'.🐛 Suggested fix
transaction = transactions_processor.get_transaction_by_hash(transaction_hash) + if transaction is None: + raise NotFoundError( + message=f"Transaction {transaction_hash} not found", + data={"hash": transaction_hash}, + ) + event_signature = "NewTransaction(bytes32,address,address)" event_signature_hash = eth_utils.keccak(text=event_signature).hex() to_addr = transaction.get("to_address") if transaction.get("to_address") else None
🤖 Fix all issues with AI agents
In @backend/database_handler/transactions_processor.py:
- Line 590: The method signature line with parameters (self, transaction_hash:
str, sim_config: dict | None = None, hide_fields: bool = False) exceeds Black's
line length and must be reformatted; edit the method signature (the
function/method definition containing transaction_hash, sim_config and
hide_fields) so the parameters are broken across multiple lines (one parameter
per line or wrapped to satisfy Black), include a trailing comma if needed and
preserve the existing type hints and default values so Black will accept the
file.
- Around line 634-649: Guard against None consensus_data and replace the magic
number: in the block inside transactions_processor where you check
transaction_data["type"] == TransactionType.DEPLOY_CONTRACT.value and inspect
transaction_data["last_round"]["result"] == 6 and "leader_receipt" in
transaction_data["consensus_data"], first ensure consensus =
transaction_data.get("consensus_data") is not None and is a mapping (e.g., if
consensus and "leader_receipt" in consensus) before using the membership test to
avoid TypeError; also replace the literal 6 with the named constant (e.g.,
ConsensusResult.MAJORITY_AGREE.value) when comparing
transaction_data["last_round"]["result"] to improve clarity and maintainability,
keeping the rest of the existing checks on leader_receipt and execution_result
intact.
In @backend/protocol_rpc/endpoints.py:
- Line 1497: get_transaction_by_hash is being called with True as a positional
second argument, which is being bound to sim_config (expects dict|None); change
the call to pass sim_config explicitly (e.g., None) and pass hide_fields as a
keyword (hide_fields=True) or use hide_fields=True and sim_config=None by name
so the boolean goes to hide_fields; update the call at
transactions_processor.get_transaction_by_hash(block_hash, ...) accordingly.
🧹 Nitpick comments (1)
backend/protocol_rpc/endpoints.py (1)
1430-1430: Consider simplifying the falsy check.The current pattern
transaction.get("to_address") if transaction.get("to_address") else Nonecallsget()twice. A cleaner approach:♻️ Optional simplification
- to_addr = transaction.get("to_address") if transaction.get("to_address") else None + to_addr = transaction.get("to_address") or None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/database_handler/transactions_processor.pybackend/protocol_rpc/endpoints.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Target Python 3.12, use 4-space indentation, and rely on Black via pre-commit for formatting consistency
**/*.py: Apply Black formatter for Python code formatting
Include type hints in all Python code
Files:
backend/database_handler/transactions_processor.pybackend/protocol_rpc/endpoints.py
backend/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Align backend filenames with their behavior (e.g.,
validators/llm_validator.py) and mirror that pattern in tests
Files:
backend/database_handler/transactions_processor.pybackend/protocol_rpc/endpoints.py
🧬 Code graph analysis (2)
backend/database_handler/transactions_processor.py (2)
backend/domain/types.py (1)
TransactionType(147-151)backend/node/types.py (1)
ExecutionResultStatus(123-132)
backend/protocol_rpc/endpoints.py (3)
backend/domain/types.py (1)
TransactionType(147-151)backend/node/types.py (1)
ExecutionResultStatus(123-132)backend/database_handler/transactions_processor.py (1)
get_transaction_by_hash(589-650)
🪛 GitHub Actions: pre-commit
backend/database_handler/transactions_processor.py
[error] 1-1: Black formatting failed. File reformatted by Black hook; commit changes or run 'pre-commit run --all-files' to fix.
⏰ 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). (1)
- GitHub Check: backend-unit-tests
🔇 Additional comments (1)
backend/protocol_rpc/endpoints.py (1)
1050-1051: LGTM!The call correctly passes
sim_configas the second argument andTrueforhide_fieldsas the third argument.
|
|
||
| def get_transaction_by_hash( | ||
| self, transaction_hash: str, sim_config: dict | None = None | ||
| self, transaction_hash: str, sim_config: dict | None = None, hide_fields: bool = False |
There was a problem hiding this comment.
Fix Black formatting to pass CI.
The pipeline failure indicates Black reformatted this line. The parameter list on a single line likely exceeds the line length limit.
🔧 Suggested fix
def get_transaction_by_hash(
- self, transaction_hash: str, sim_config: dict | None = None, hide_fields: bool = False
+ self,
+ transaction_hash: str,
+ sim_config: dict | None = None,
+ hide_fields: bool = False,
) -> dict | None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self, transaction_hash: str, sim_config: dict | None = None, hide_fields: bool = False | |
| self, | |
| transaction_hash: str, | |
| sim_config: dict | None = None, | |
| hide_fields: bool = False, |
🤖 Prompt for AI Agents
In @backend/database_handler/transactions_processor.py at line 590, The method
signature line with parameters (self, transaction_hash: str, sim_config: dict |
None = None, hide_fields: bool = False) exceeds Black's line length and must be
reformatted; edit the method signature (the function/method definition
containing transaction_hash, sim_config and hide_fields) so the parameters are
broken across multiple lines (one parameter per line or wrapped to satisfy
Black), include a trailing comma if needed and preserve the existing type hints
and default values so Black will accept the file.
| if hide_fields: | ||
| if transaction_data[ | ||
| "type" | ||
| ] == TransactionType.DEPLOY_CONTRACT.value and not ( | ||
| transaction_data["last_round"]["result"] == 6 | ||
| and "leader_receipt" in transaction_data["consensus_data"] | ||
| and transaction_data["consensus_data"]["leader_receipt"] is not None | ||
| and len(transaction_data["consensus_data"]["leader_receipt"]) > 0 | ||
| and transaction_data["consensus_data"]["leader_receipt"][0][ | ||
| "execution_result" | ||
| ] | ||
| == ExecutionResultStatus.SUCCESS.value | ||
| ): | ||
| transaction_data["data"]["contract_address"] = None | ||
| if "contract_snapshot" in transaction_data: | ||
| del transaction_data["contract_snapshot"] |
There was a problem hiding this comment.
Potential TypeError if consensus_data is None; also use a named constant for magic number 6.
-
If
transaction_data["consensus_data"]isNoneandlast_round["result"] == 6, the expression"leader_receipt" in transaction_data["consensus_data"]will raiseTypeError: argument of type 'NoneType' is not iterable. -
The magic number
6should be replaced with a named constant (e.g.,ConsensusResult.MAJORITY_AGREE) for clarity and maintainability.
🔧 Suggested fix
+ from backend.consensus.types import ConsensusResult
+
if hide_fields:
if transaction_data[
"type"
] == TransactionType.DEPLOY_CONTRACT.value and not (
- transaction_data["last_round"]["result"] == 6
- and "leader_receipt" in transaction_data["consensus_data"]
+ transaction_data["last_round"]["result"]
+ == int(ConsensusResult.MAJORITY_AGREE)
+ and transaction_data["consensus_data"] is not None
+ and "leader_receipt" in transaction_data["consensus_data"]
and transaction_data["consensus_data"]["leader_receipt"] is not None
and len(transaction_data["consensus_data"]["leader_receipt"]) > 0
and transaction_data["consensus_data"]["leader_receipt"][0][
"execution_result"
]
== ExecutionResultStatus.SUCCESS.value
):
transaction_data["data"]["contract_address"] = None
if "contract_snapshot" in transaction_data:
del transaction_data["contract_snapshot"]🤖 Prompt for AI Agents
In @backend/database_handler/transactions_processor.py around lines 634 - 649,
Guard against None consensus_data and replace the magic number: in the block
inside transactions_processor where you check transaction_data["type"] ==
TransactionType.DEPLOY_CONTRACT.value and inspect
transaction_data["last_round"]["result"] == 6 and "leader_receipt" in
transaction_data["consensus_data"], first ensure consensus =
transaction_data.get("consensus_data") is not None and is a mapping (e.g., if
consensus and "leader_receipt" in consensus) before using the membership test to
avoid TypeError; also replace the literal 6 with the named constant (e.g.,
ConsensusResult.MAJORITY_AGREE.value) when comparing
transaction_data["last_round"]["result"] to improve clarity and maintainability,
keeping the rest of the existing checks on leader_receipt and execution_result
intact.
| if ( | ||
| transaction["type"] == TransactionType.DEPLOY_CONTRACT.value | ||
| and transaction["last_round"]["result"] == 6 | ||
| and "leader_receipt" in transaction["consensus_data"] | ||
| and transaction["consensus_data"]["leader_receipt"] is not None | ||
| and len(transaction["consensus_data"]["leader_receipt"]) > 0 | ||
| and transaction["consensus_data"]["leader_receipt"][0]["execution_result"] | ||
| == ExecutionResultStatus.SUCCESS.value | ||
| ): | ||
| contract_address = transaction["to_address"] | ||
| else: | ||
| contract_address = None |
There was a problem hiding this comment.
Duplicated logic and magic number 6; also potential TypeError if consensus_data is None.
-
This condition duplicates the logic in
transactions_processor.py(lines 634-646). Consider extracting to a shared helper function for maintainability. -
The magic number
6should use a named constant (e.g.,ConsensusResult.MAJORITY_AGREE). -
If
transaction["consensus_data"]isNone, the"leader_receipt" in transaction["consensus_data"]check will raiseTypeError.
♻️ Suggested refactor
+from backend.consensus.types import ConsensusResult
+
+def _is_successful_deployment(transaction: dict) -> bool:
+ """Check if a DEPLOY_CONTRACT transaction was successfully deployed."""
+ return (
+ transaction["type"] == TransactionType.DEPLOY_CONTRACT.value
+ and transaction["last_round"]["result"] == int(ConsensusResult.MAJORITY_AGREE)
+ and transaction.get("consensus_data") is not None
+ and "leader_receipt" in transaction["consensus_data"]
+ and transaction["consensus_data"]["leader_receipt"] is not None
+ and len(transaction["consensus_data"]["leader_receipt"]) > 0
+ and transaction["consensus_data"]["leader_receipt"][0].get("execution_result")
+ == ExecutionResultStatus.SUCCESS.value
+ )
+
# In get_transaction_receipt:
- if (
- transaction["type"] == TransactionType.DEPLOY_CONTRACT.value
- and transaction["last_round"]["result"] == 6
- and "leader_receipt" in transaction["consensus_data"]
- and transaction["consensus_data"]["leader_receipt"] is not None
- and len(transaction["consensus_data"]["leader_receipt"]) > 0
- and transaction["consensus_data"]["leader_receipt"][0]["execution_result"]
- == ExecutionResultStatus.SUCCESS.value
- ):
- contract_address = transaction["to_address"]
- else:
- contract_address = None
+ contract_address = transaction["to_address"] if _is_successful_deployment(transaction) else NoneCommittable suggestion skipped: line range outside the PR's diff.
| ) -> dict | None: | ||
|
|
||
| transaction = transactions_processor.get_transaction_by_hash(block_hash) | ||
| transaction = transactions_processor.get_transaction_by_hash(block_hash, True) |
There was a problem hiding this comment.
Type error: True is passed as sim_config instead of hide_fields.
The method signature is get_transaction_by_hash(transaction_hash, sim_config, hide_fields). Passing True positionally makes it the sim_config argument (which expects dict | None), not hide_fields.
🐛 Suggested fix
- transaction = transactions_processor.get_transaction_by_hash(block_hash, True)
+ transaction = transactions_processor.get_transaction_by_hash(block_hash, None, True)Or use keyword argument for clarity:
- transaction = transactions_processor.get_transaction_by_hash(block_hash, True)
+ transaction = transactions_processor.get_transaction_by_hash(block_hash, hide_fields=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transaction = transactions_processor.get_transaction_by_hash(block_hash, True) | |
| transaction = transactions_processor.get_transaction_by_hash(block_hash, hide_fields=True) |
🤖 Prompt for AI Agents
In @backend/protocol_rpc/endpoints.py at line 1497, get_transaction_by_hash is
being called with True as a positional second argument, which is being bound to
sim_config (expects dict|None); change the call to pass sim_config explicitly
(e.g., None) and pass hide_fields as a keyword (hide_fields=True) or use
hide_fields=True and sim_config=None by name so the boolean goes to hide_fields;
update the call at transactions_processor.get_transaction_by_hash(block_hash,
...) accordingly.
|
1 participant
Fixes #DXP-186
What
hide_fieldsparameter to theget_transaction_by_hashmethod inTransactionsProcessorto conditionally hide certain fields in the transaction data. Internally we do not want to hide those field because we use them, but externally like for an endpoint call we want to hide them.get_transaction_by_hashfunction inendpoints.pyto passTruefor thehide_fieldsparameter.get_transaction_receiptto determine thecontractAddressbased on: only show the contract address when it is a deployment transaction and when the contract was deployed.Why
Testing done
get_transaction_by_hashmethod correctly hides fields and the consensus is still working.get_transaction_receiptfunction to ensure it returns the correctcontractAddressbased on the new logic.Decisions made
Checks
Reviewing tips
hide_fieldsparameter and its impact on transaction data.get_transaction_receiptfor determining thecontractAddress.User facing release notes
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.