fix: add payload to exception to improve debugging capabilities #60
Conversation
WalkthroughConsolidates ABI file resource loading, introduces a payload-capable GenLayerError, updates provider to attach the raw response in JSON decode errors, adds TransactionStatus.FINALIZED to decided states, and reformats several unit tests without changing behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client Code
participant P as GenLayerProvider
participant S as Remote Service
C->>P: make_request(payload)
P->>S: HTTP POST payload
S-->>P: HTTP response
alt JSON parse success
P-->>C: parsed JSON result
else JSON parse fails
note over P: Build response_preview<br/>Attach payload={"response": response}
P-->>C: raise GenLayerError(message, payload)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
genlayer_py/exceptions.py (1)
25-27: Preserve GenLayerError’s backward-compatible signature
GenLayerErrorused to accept any positional args. Tightening the signature to just(message, payload=None)risks breaking existing call sites that passed extra context positionally. Please continue to forward any additional args toExceptionwhile still capturingpayload.- def __init__(self, message: str, payload: Optional[dict[str, Any]] = None): - super().__init__(message) - self.payload = payload + def __init__( + self, + message: str, + payload: Optional[dict[str, Any]] = None, + *args: Any, + ): + if payload is None: + super().__init__(message, *args) + else: + super().__init__(message, payload, *args) + self.payload = payloadtests/unit/transactions/test_wait_for_transaction_receipt.py (3)
225-228: Drop the explicit comparison to TrueRuff flags this pattern (E712), and it’s cleaner to assert the predicate directly.
- for status_num in decided_status_numbers: - assert ( - is_decided_state(status_num) == True - ), f"Status {status_num} should be decided" + for status_num in decided_status_numbers: + assert is_decided_state( + status_num + ), f"Status {status_num} should be decided"
243-246: Invert the predicate without comparing to FalseSame Ruff E712 warning here—prefer
not predicateover== False.- for status_num in non_decided_status_numbers: - assert ( - is_decided_state(status_num) == False - ), f"Status {status_num} should not be decided" + for status_num in non_decided_status_numbers: + assert not is_decided_state( + status_num + ), f"Status {status_num} should not be decided"
254-256: Same clean-up for the invalid-status branchApplying the same simplification keeps the test consistent and quiets Ruff.
if status is not None: - assert ( - is_decided_state(status) == False - ), f"Invalid status {status} should not be decided" + assert not is_decided_state( + status + ), f"Invalid status {status} should not be decided"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
genlayer_py/consensus/abi/__init__.py(1 hunks)genlayer_py/exceptions.py(1 hunks)genlayer_py/provider/provider.py(1 hunks)genlayer_py/types/transactions.py(1 hunks)tests/unit/transactions/test_wait_for_transaction_receipt.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
genlayer_py/provider/provider.py (1)
genlayer_py/exceptions.py (1)
GenLayerError(4-27)
tests/unit/transactions/test_wait_for_transaction_receipt.py (3)
tests/unit/conftest.py (1)
mock_client(23-80)genlayer_py/transactions/actions.py (2)
get_transaction(108-137)wait_for_transaction_receipt(68-105)genlayer_py/types/transactions.py (2)
TransactionStatus(14-28)is_decided_state(75-78)
🪛 Ruff (0.13.1)
tests/unit/transactions/test_wait_for_transaction_receipt.py
227-227: Avoid equality comparisons to True; use is_decided_state(status_num): for truth checks
Replace with is_decided_state(status_num)
(E712)
245-245: Avoid equality comparisons to False; use not is_decided_state(status_num): for false checks
Replace with not is_decided_state(status_num)
(E712)
255-255: Avoid equality comparisons to False; use not is_decided_state(status): for false checks
Replace with not is_decided_state(status)
(E712)
🔇 Additional comments (1)
genlayer_py/types/transactions.py (1)
65-78: LGTM —FINALIZEDbelongs inDECIDED_STATES.Including
TransactionStatus.FINALIZEDin the decided bucket makes sense and keeps the number/name lookups consistent. No further action needed.
| with ( | ||
| importlib.resources.as_file( | ||
| importlib.resources.files("genlayer_py.consensus.abi").joinpath( | ||
| "consensus_data_abi.json" | ||
| ) | ||
| ) as path, | ||
| open(path, "r", encoding="utf-8") as f, | ||
| ): | ||
| CONSENSUS_DATA_ABI = json.load(f) | ||
|
|
||
| with importlib.resources.as_file( | ||
| importlib.resources.files("genlayer_py.consensus.abi").joinpath( | ||
| "consensus_main_abi.json" | ||
| ) | ||
| ) as path, open(path, "r", encoding="utf-8") as f: | ||
| with ( | ||
| importlib.resources.as_file( | ||
| importlib.resources.files("genlayer_py.consensus.abi").joinpath( | ||
| "consensus_main_abi.json" | ||
| ) | ||
| ) as path, | ||
| open(path, "r", encoding="utf-8") as f, | ||
| ): |
There was a problem hiding this comment.
Avoid Python 3.10-only with (...) syntax unless you’ve dropped 3.9 support.
Using parentheses around multiple context managers (Line 4) is only valid starting in Python 3.10. If the package still supports Python 3.9 (or earlier), this will raise a SyntaxError at import time and break every consumer. Please stick to the nested with form or otherwise confirm that requires-python has been bumped to >=3.10 across the project before landing this.
🤖 Prompt for AI Agents
In genlayer_py/consensus/abi/__init__.py around lines 4 to 21, the code uses the
Python 3.10-only "with (a, b):" multiple-context-manager syntax which will raise
SyntaxError on Python 3.9; change these to nested with statements (open the
resource with importlib.resources.as_file(...) as path: then inside that block
open(path, "r", encoding="utf-8") as f:) for each file load, or alternatively
ensure the project's requires-python metadata has been raised to >=3.10 before
keeping the current syntax.
| response_preview = ( | ||
| response.text[:500] | ||
| if len(response.text) <= 500 | ||
| else f"{response.text[:500]}..." | ||
| ) | ||
| raise GenLayerError( | ||
| f"{method} returned invalid JSON: {err}. Response content: {response_preview}" | ||
| f"{method} returned invalid JSON: {err}. Response content: {response_preview}", | ||
| payload={"response": response}, | ||
| ) from err |
There was a problem hiding this comment.
Avoid storing the raw Response object in payload
Keeping the requests.Response instance in GenLayerError.payload makes the payload non‑serializable and exposes request headers (often containing credentials) when downstream tooling logs or forwards the payload. That breaks observability pipelines and increases leakage risk. Please persist only primitive, serializable fields (status code, headers copy, truncated body) so callers can safely inspect the data.
except ValueError as err:
response_preview = (
response.text[:500]
if len(response.text) <= 500
else f"{response.text[:500]}..."
)
+ response_payload = {
+ "status_code": response.status_code,
+ "headers": dict(response.headers),
+ "body_preview": response_preview,
+ }
raise GenLayerError(
f"{method} returned invalid JSON: {err}. Response content: {response_preview}",
- payload={"response": response},
+ payload=response_payload,
) from err📝 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.
| response_preview = ( | |
| response.text[:500] | |
| if len(response.text) <= 500 | |
| else f"{response.text[:500]}..." | |
| ) | |
| raise GenLayerError( | |
| f"{method} returned invalid JSON: {err}. Response content: {response_preview}" | |
| f"{method} returned invalid JSON: {err}. Response content: {response_preview}", | |
| payload={"response": response}, | |
| ) from err | |
| except ValueError as err: | |
| response_preview = ( | |
| response.text[:500] | |
| if len(response.text) <= 500 | |
| else f"{response.text[:500]}..." | |
| ) | |
| response_payload = { | |
| "status_code": response.status_code, | |
| "headers": dict(response.headers), | |
| "body_preview": response_preview, | |
| } | |
| raise GenLayerError( | |
| f"{method} returned invalid JSON: {err}. Response content: {response_preview}", | |
| payload=response_payload, | |
| ) from err |
🧰 Tools
🪛 Ruff (0.13.1)
46-49: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In genlayer_py/provider/provider.py around lines 41 to 49, the GenLayerError
currently stores the raw requests.Response in payload which is non-serializable
and can leak sensitive headers; instead build a serializable payload with
primitive fields only — e.g., include response.status_code,
dict(response.headers) (a shallow copy), a truncated body (use the existing
response_preview), and optionally response.url or reason — and pass that dict as
payload to GenLayerError; remove the raw Response object from the payload.
Related #57
Fixes: DXP-687
What
Example: Accessing the payload
Summary by CodeRabbit
New Features
Refactor
Style
Tests