Conversation
Code Review Audit ReportPR Overview
Files Changed
Change Summary1. Privy TRON Signature V-Byte Fix (
|
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/providers/config_provider.py : Lines 99-103 |
Description: The original code called .startswith() on a tuple value, which is a method that exists only on strings. The _wallets dictionary uses tuple[str, WalletType, str | None] keys (as declared on line 46 and constructed on line 138). Any call to remove_wallet would unconditionally raise AttributeError: 'tuple' object has no attribute 'startswith', meaning the wallet cache was never evicted and the wallet entry deletion path was broken in all cases.
Code (original — now fixed):
self._wallets = {
cache_key: wallet
for cache_key, wallet in self._wallets.items()
if not cache_key.startswith(f"{wallet_id}:") # AttributeError: tuple has no .startswith
}Fixed code:
self._wallets = {
cache_key: wallet
for cache_key, wallet in self._wallets.items()
if cache_key[0] != wallet_id
}Recommendation: The fix is correct. The first element of the cache key tuple (cache_key[0]) is always the wallet_id string, so this comparison is semantically equivalent to what was intended. No further changes needed.
[CRITICAL-02] Incorrect TRON Signature V-Byte Produces Unverifiable Signatures
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 151-152 |
Description: The _tron_sign_hash method was appending the raw ECDSA recovery ID (0 or 1) as the final byte of the signature hex. However, TronWeb's signature convention (used by verifyMessage and other on-chain verifiers) expects the v-byte to be recovery_id + 27 (i.e., 0x1b or 0x1c). Signatures produced without this offset would fail verification in any TRON ecosystem tool, making the Privy TRON signing path completely non-functional for real-world use.
The TypeScript counterpart already implemented this correctly: const vHex = (v + 27).toString(16).padStart(2, '0') (TS file line 158).
Code (original — now fixed):
v = _recover_tron_v(sig_bytes, digest, await self.get_address())
return (sig_hex + f"{v:02x}").lower()Fixed code:
v = _recover_tron_v(sig_bytes, digest, await self.get_address())
return (sig_hex + f"{v + 27:02x}").lower()Recommendation: The fix is correct and aligns the Python implementation with the TypeScript implementation. No further changes needed.
[MINOR-01] Test Assertion Logic Depends on External tronpy Signature Format
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Testing |
| File | packages/python/tests/test_privy_adapter.py : Lines 189-192 |
Description: The updated test derives the expected v-byte by extracting the last two hex characters of sig.hex() and adding 27. This approach couples the test logic to tronpy's internal signature format (its output must contain a 1-byte v suffix as the final 2 hex characters). If tronpy's sign_msg output format ever changes, the test could give misleading results. A more robust approach would directly verify that the output ends with either 1b or 1c (the only valid TronWeb v-bytes).
Code:
tronpy_v = int(sig.hex()[-2:], 16)
expected = raw_sig + f"{tronpy_v + 27:02x}"
assert signature == expectedRecommendation: Consider supplementing the assertion with an explicit check that the final byte is one of the two valid values:
assert signature[-2:] in ("1b", "1c"), f"Expected TronWeb v-byte 0x1b or 0x1c, got {signature[-2:]}"This makes the intent self-documenting and adds defense against unexpected v-byte values.
[MINOR-02] Missing Test Coverage for remove_wallet Cache Eviction
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Testing |
| File | packages/python/tests/test_registry.py : Lines 259-271 |
Description: The existing test_remove_wallet_deletes_secret_and_clears_active test does not call get_wallet before calling remove_wallet, so self._wallets is empty during the eviction loop. The bug (calling .startswith() on a tuple) would never have been exercised by the test suite. A test that pre-populates the wallet cache before calling remove_wallet would have caught this regression and will guard against future regressions to cache eviction logic.
Recommendation: Add a test that calls get_wallet (to populate the cache) before remove_wallet, then verifies that the cached entry is no longer present and that a subsequent get_wallet call creates a fresh instance.
[SUGGESTION-01] TypeScript Config Provider Uses String Keys — Structural Divergence
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Docs / Maintainability |
| File | packages/typescript/src/core/providers/config-provider.ts : Line 115 |
Description: The TypeScript implementation uses string-keyed cache entries (key.startsWith(...)) while the Python implementation uses tuple-keyed entries. The structural divergence between the two implementations allowed the Python bug to persist undetected. The CLAUDE.md notes that changes to core logic should typically be applied to both languages. While both implementations are now individually correct, documenting the key-type difference (or unifying the approach) would aid future maintainers.
Recommendation: No code change required, but a comment in config_provider.py near the _wallets declaration noting that keys are tuples (not strings as in the TS counterpart) would prevent future authors from copying the TS string-based cache invalidation pattern incorrectly.
Positive Observations
-
Symmetry with TypeScript fixed. The Python
_tron_sign_hashnow matches the TypeScripttronSignHashimplementation exactly in its v-byte encoding (v + 27). The dual-language codebase is now consistent on this critical signing convention. -
Bug fix is minimal and targeted. Both fixes change exactly one operator or expression without restructuring surrounding logic, minimizing the risk of introducing new issues.
-
Test was updated in the same commit. The test for the v-byte fix was updated alongside the fix, ensuring the test suite reflects the new expected behavior and prevents regression.
-
Clear commit message. The commit message "fix: privy sign v & tuple no start with" accurately describes both changes, making the intent clear in the git history.
-
_recover_tron_vhelper design. The helper cleanly separates recovery-ID derivation from v-byte formatting, which made the bug easy to isolate and fix.
Checklist Results
| Category | Result | Notes |
|---|---|---|
| A. Correctness & Logic | PASS (after fix) | Two critical bugs corrected |
| B. Security | PASS | No security regressions; no credentials introduced |
| C. Performance | PASS | No performance-relevant changes |
| D. Code Quality | PASS | Small, focused changes with clear naming |
| E. Testing | PARTIAL | Test updated for privy fix; cache eviction still lacks coverage |
| F. Documentation & Maintainability | PARTIAL | Structural divergence from TS not documented |
Review Verdict
Approve (with suggestions)
Both changes are correct bug fixes that resolve critical runtime failures. The TRON v-byte fix corrects signatures that would have been unverifiable by any TRON ecosystem tool. The cache eviction fix corrects code that would have unconditionally raised AttributeError whenever remove_wallet was called on a provider with any cached wallet entries.
The test updates are adequate for the immediate changes. Two minor gaps remain:
- No test exercises the cache eviction path (the bug could have been caught by an existing test with a minor addition).
- The test assertion logic for the v-byte could be made more self-documenting.
Neither gap blocks merging, but the cache eviction test gap should be addressed in a follow-up to prevent regression.
2 participants
No description provided.