fix(python): preserve raw_data_hex in tron approvals#66
Conversation
Code Review ReportProject: bankofai/x402 (Python) PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR fixes a real bug: when building the unsigned transaction payload for TRON approval signing, the previous code called The approach is pragmatic and the test coverage correctly validates the happy path. However, there is one major issue: the method silently assembles and returns a payload whose core fields ( Change Summary1. New Helper:
|
| File | Change Type | Description |
|---|---|---|
python/x402/src/bankofai/x402/signers/client/tron_signer.py |
Modified | Added static method _build_unsigned_tx_payload; updated ensure_allowance to use it |
Purpose: Decouple the payload assembly from ensure_allowance, ensuring raw_data_hex is reliably included in the dict passed to wallet.sign_transaction(). Provides three escalating fallback strategies to extract raw_data_hex from any tronpy transaction object regardless of SDK version quirks.
2. New Integration-Style Unit Test (test_signers.py)
| File | Change Type | Description |
|---|---|---|
python/x402/tests/client/test_signers.py |
Modified | Added test_tron_signer_ensure_allowance_builds_agent_wallet_payload |
Purpose: Validates end-to-end that when ensure_allowance is called, the wallet's sign_transaction is invoked with a payload that includes both txID and raw_data_hex, guarding against regression of the original bug.
Detailed Findings
Major
[MJ-01] _build_unsigned_tx_payload can silently return None for critical fields
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | python/x402/src/bankofai/x402/signers/client/tron_signer.py : Lines 88–93 |
Description
When none of the three extraction strategies (
.to_json(), direct attribute, callable private attribute) can populatetxIDorraw_data_hex, the method still returns a dict withNonevalues for those keys. The dict is then passed directly towallet.sign_transaction(). Because the wallet layer is external (agent-wallet), the resulting error will originate there rather than here, producing a confusing traceback that doesn't point to where the value was lost. The fix should either raise a descriptiveValueErrorwhen the payload is incomplete or at minimum emit a warning so the source of the failure is immediately clear.
Code
unsigned: dict[str, Any] = {
"txID": payload.get("txID"), # can be None
"raw_data_hex": payload.get("raw_data_hex"), # can be None
}
if payload.get("raw_data") is not None:
unsigned["raw_data"] = payload.get("raw_data")
return unsignedRecommendation
unsigned: dict[str, Any] = {
"txID": payload.get("txID"),
"raw_data_hex": payload.get("raw_data_hex"),
}
if payload.get("raw_data") is not None:
unsigned["raw_data"] = payload.get("raw_data")
# Fail fast with a clear message rather than sending None values downstream
missing = [k for k in ("txID", "raw_data_hex") if not unsigned.get(k)]
if missing:
raise ValueError(
f"Unable to build unsigned TRON tx payload: missing fields {missing}. "
f"Transaction object type: {type(txn).__name__}"
)
return unsignedMinor
[MN-01] Silent swallowing of exception in _raw_data_hex callable fallback
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness / Observability |
| File | python/x402/src/bankofai/x402/signers/client/tron_signer.py : Lines 80–84 |
Description
The bare
except Exception: raw_data_hex = Noneswallows any exception raised by callingtxn._raw_data_hex(). If the callable exists but raises an unexpected error (e.g., a type error, a network call failure, or an encoding exception), the failure is completely invisible. At minimum it should be logged so that debugging a missingraw_data_hexscenario is feasible.
Code
if callable(maybe):
try:
raw_data_hex = maybe()
except Exception:
raw_data_hex = NoneRecommendation
if callable(maybe):
try:
raw_data_hex = maybe()
except Exception as exc:
logger.debug(
"txn._raw_data_hex() callable raised an exception, "
"raw_data_hex will not be populated from this path: %s",
exc,
)
raw_data_hex = None[MN-02] No log warning emitted when raw_data_hex is not found through any path
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Observability |
| File | python/x402/src/bankofai/x402/signers/client/tron_signer.py : Lines 75–86 |
Description
After exhausting all three strategies, if
raw_data_hexremainsNone, the code continues silently. For operators debugging a signing failure in production, this leaves no log trail explaining why the wallet received an incomplete payload. Alogger.warningat this point would immediately surface the root cause during incident triage.
Code
if raw_data_hex:
payload["raw_data_hex"] = raw_data_hex
unsigned: dict[str, Any] = {
"txID": payload.get("txID"),
"raw_data_hex": payload.get("raw_data_hex"), # silently None if all strategies failed
}Recommendation
if raw_data_hex:
payload["raw_data_hex"] = raw_data_hex
else:
logger.warning(
"_build_unsigned_tx_payload: could not extract raw_data_hex from txn of type %s. "
"Signing may fail.",
type(txn).__name__,
)[MN-03] Accessing private attribute _raw_data_hex creates brittle coupling to tronpy internals
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | python/x402/src/bankofai/x402/signers/client/tron_signer.py : Lines 79–84 |
Description
The fallback
getattr(txn, "_raw_data_hex", None)accesses a private/internal attribute of thetronpytransaction object. Private attributes are not part of a library's public contract and can be renamed, removed, or change semantics in any patch release without a breaking-change notice. This is an appropriate escape hatch only if documented as a workaround for a specific version or bug. A comment explaining why this private attribute path exists and which tronpy version(s) it targets would significantly reduce future maintenance risk.
Recommendation
Add an inline comment explaining the version or SDK behaviour that necessitates the private-attribute fallback, for example:
# tronpy < 0.x.y: raw_data_hex is stored as a private callable `_raw_data_hex` # and is not surfaced by to_json() or as a plain attribute. maybe = getattr(txn, "_raw_data_hex", None)Additionally, consider tracking this with a
# TODO: remove once tronpy >=X.Y.Z is the minimum requirementnote so the workaround has a clear expiry path.
Suggestions
[S-01] Test missing: payload is sent with None values when raw_data_hex is unavailable
File: python/x402/tests/client/test_signers.py
Description: The new test validates only the happy path where raw_data_hex is present as a direct attribute on the transaction mock. There is no test covering the case where neither to_json() nor any attribute provides raw_data_hex, confirming the failure behaviour (whether that becomes a raised ValueError per MJ-01 or a specific log warning per MN-02).
Suggestion: Add a test that sets txn.to_json.return_value = {"txID": "txid"} (no raw_data_hex key) and removes txn.raw_data_hex, then asserts the expected outcome (exception or warning). This will lock in the intended degraded-path behaviour and prevent silent regressions.
[S-02] Test missing: _raw_data_hex callable fallback path is not exercised
File: python/x402/tests/client/test_signers.py
Description: _build_unsigned_tx_payload has three distinct code paths for resolving raw_data_hex. The new test covers only path 2 (direct attribute). Path 3 (callable _raw_data_hex) is not tested, so a future breakage of that branch would go undetected.
Suggestion: Add a focused unit test for _build_unsigned_tx_payload directly (it is a static method, so no signer instantiation is needed):
def test_build_unsigned_tx_payload_callable_fallback():
txn = MagicMock(spec=[]) # no to_json, no raw_data_hex attribute
txn._raw_data_hex = MagicMock(return_value="cafebabe")
# Provide txID via attribute
txn.txID = "txid-123"
result = TronClientSigner._build_unsigned_tx_payload(txn)
assert result["raw_data_hex"] == "cafebabe"
assert result["txID"] == "txid-123"Positive Observations
| Area | Observation |
|---|---|
| Defensive extraction | The three-tier fallback strategy (to_json() → attribute → callable) is a thoughtful approach to handling multiple tronpy SDK versions gracefully. |
| Minimal payload | Assembling a minimal unsigned dict (only txID, raw_data_hex, and optional raw_data) rather than forwarding the full to_json() output reduces the risk of sending unexpected or sensitive internal fields to the external wallet. |
| Test-first shape assertion | The new test's use of assert_awaited_once_with(...) with a full dict literal is excellent — it precisely pins the contract between the signer and the wallet interface. |
| Static method design | Extracting payload building into a @staticmethod makes the logic trivially unit-testable in isolation and avoids hidden dependency on instance state. |
| Docstrings preserved | The new method includes a clear one-line docstring consistent with the rest of the class. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 6 | 2 | 0 | MJ-01 (None payload), MN-01 (silent exception) |
| Security | 10 | 10 | 0 | 0 | No security regressions introduced |
| Performance | 7 | 7 | 0 | 0 | No performance concerns |
| Code Quality | 10 | 8 | 1 | 1 | MN-03 (private attr coupling); naming/style OK |
| Testing | 7 | 5 | 2 | 0 | S-01 (missing None path), S-02 (missing callable fallback path) |
| Documentation | 6 | 5 | 1 | 0 | MN-03 (missing version rationale comment) |
| Compatibility | 5 | 5 | 0 | 0 | Backward-compatible change in ensure_allowance |
| Observability | 4 | 2 | 2 | 0 | MN-01, MN-02 (missing logging on failure paths) |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between the specified branches (main...dev/tron-allowance-raw-data-hex-main). Runtime behaviour, integration testing against a live TRON node, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-04-07
2 participants
Fixes #64
Description
Tests
Checklist