Conversation
gummy789j
force-pushed
the
feat/privy-support
branch
from
d4ee2ae to
f0851be
Compare
gummy789j
force-pushed
the
feat/privy-support
branch
from
f0851be to
c4fba9a
Compare
Code Review Audit ReportPR Overview
Change Summary1. Core Privy Wallet Adapter (New Feature)New 2. Privy HTTP Client (New Feature)New 3. Privy Configuration (New Feature)New 4. Provider / Resolver Refactoring
5. Utility ExtractionHelper functions previously duplicated across 6. KV Store — Test-Speed Optimization
7. CLI Enhancements
8. TestsNew test files: 9. Documentation / Spec FilesLarge volumes of Detailed FindingsCritical[C-01] Python
|
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 65–88 |
Description:
urllib.request.urlopen raises urllib.error.HTTPError for any HTTP response with a status code >= 400. The current implementation reads the response body inside the with urlopen(req) block, and only checks status >= 400 after the with block exits. Because HTTPError is raised on entry, execution never reaches the status check for 4xx/5xx responses. This means:
- 401 Unauthorized responses are never caught as
PrivyAuthError. - 429 Too Many Requests responses are never caught for retry.
- All 4xx/5xx errors surface as uncaught
urllib.error.HTTPError, which propagates out to callers without being converted to aPrivyRequestError.
The TypeScript client uses fetch(), which never throws on HTTP errors, so this is a Python-only defect.
Code:
with urlopen(req) as response:
status = getattr(response, "status", 0)
payload = _read_json(response)
if status == 429: # never reached for 4xx/5xx
...
if status >= 400: # never reached for 4xx/5xx
...Recommendation:
Catch urllib.error.HTTPError to extract the status code and body:
from urllib.error import HTTPError, URLError
import urllib.error
def _request(self, method, path, body=None, *, authorization_signature=None):
attempt = 0
while True:
req = Request(...)
try:
with urlopen(req) as response:
status = response.status
payload = _read_json(response)
except HTTPError as exc:
status = exc.code
try:
payload = json.loads(exc.read().decode("utf-8"))
except Exception:
payload = {}
if status == 429:
...
if status >= 400:
...
return payload[C-02] Python PrivyAdapter — _tron_sign_bytes uses tronpy.keys.hash_message while TypeScript uses keccak256; hashing semantics diverge (Critical)
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 116–122 vs packages/typescript/src/core/adapters/privy.ts : Lines 114–117 |
Description:
The Python implementation uses tronpy.keys.hash_message(data), which prepends \x19TRON Signed Message:\n{len} before hashing. The TypeScript implementation uses keccak256(bytes) directly without any prefix. The local TronSigner uses tron_key.sign_msg() (also uses tronpy.keys.sign_msg_hash(keccak256(data))), not hash_message.
The two Privy adapters will produce different digests for sign_message, causing incompatible signatures between the Python and TypeScript implementations and potentially against the local TronSigner baseline. The compare_sign_consistency examples in the PR exist to test cross-language compatibility, so this divergence is a regression.
Code:
# Python - WRONG: prepends TRON prefix
digest = hash_message(data) # hash_message adds "\x19TRON Signed Message:\n"
# TypeScript - no prefix
const hashHex = keccak256(bytes)Recommendation:
Align both to use plain keccak256 (no prefix), matching the local TronSigner.sign_raw behavior:
from eth_utils import keccak
digest = keccak(primitive=data) # keccak256(data), no prefixMajor
[M-01] app_secret stored in plaintext in wallets_config.json (Major)
| Property | Value |
|---|---|
| Severity | Major |
| Category | Security |
| File | packages/python/src/agent_wallet/core/config.py : PrivyWalletParams; packages/typescript/src/core/config.ts : PrivyWalletParamsSchema |
Description:
PrivyWalletParams stores app_secret as a plain string field. The wallet config is written to ~/.agent-wallet/wallets_config.json in cleartext. The app_secret is an API key that grants signing authority over all wallets in the Privy application — this is equivalent to a private key. The existing local_secure wallet type stores the actual private key in an AES-128-CTR encrypted keystore file; raw_secret similarly stores the key in config as a plain hex string (an existing known limitation), but Privy's app_secret + wallet_id combination grants remote signing without any additional password.
Recommendation:
- Consider storing the
app_secretusing the existingSecureKVStore(same as local_secure key material) and referencing it via asecret_ref. This maintains the same security boundary as other key types. - Alternatively, document clearly that the
app_secretis stored in plaintext and advise using filesystem permissions or environment variable injection for production deployments. - At minimum, add a visible warning during
privywallet creation in the CLI.
[M-02] Python Privy client is synchronous (time.sleep) but called from async context — will block the event loop (Major)
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Performance |
| File | packages/python/src/agent_wallet/core/clients/privy.py : all public methods; packages/python/src/agent_wallet/core/adapters/privy.py : all async methods |
Description:
PrivyClient.get_wallet(), .rpc(), .raw_sign() and ._request() are all synchronous methods that call urllib.request.urlopen() (blocking I/O) and time.sleep() (blocking sleep) directly. They are called from async methods in PrivyAdapter without await (correctly — they are not coroutines), but since they perform blocking I/O, any use inside an asyncio event loop will block the entire loop for the duration of each HTTP request.
This is particularly harmful in an agentic environment where multiple concurrent operations may share the same event loop.
Code:
async def get_address(self) -> str:
...
payload = self._client.get_wallet(self._wallet_id) # blocks event loopRecommendation:
Either:
- Make
PrivyClientfully async usingaiohttporhttpx[asyncio](preferred for consistency with the TypeScript client that usesfetch). - Or run blocking calls in a thread pool:
await asyncio.to_thread(self._client.get_wallet, self._wallet_id).
[M-03] chainType comparison is case-sensitive in TypeScript getAddress but .toLowerCase() in getChainType — inconsistent caching (Major)
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 34–39 vs Lines 97–101 |
Description:
getAddress() caches wallet.chainType verbatim (e.g. "ethereum" or "Ethereum"), while getChainType() stores wallet.chainType?.toLowerCase(). If getAddress() is called first, cachedChainType is set without .toLowerCase(). The subsequent call to getChainType() returns the cached value from cachedChainType without applying .toLowerCase(), so a value like "TRON" would not match the === 'tron' check.
Code:
// getAddress: stores without toLowerCase
this.cachedChainType = wallet.chainType ?? null
// getChainType: if already cached, returns without toLowerCase
if (this.cachedChainType) return this.cachedChainTypeRecommendation:
Normalize casing at the point of storage in getAddress():
this.cachedChainType = wallet.chainType?.toLowerCase() ?? null[M-04] _scrypt_params relies on PYTEST_CURRENT_TEST env var — silently uses reduced security parameters in all pytest runs (Major)
| Property | Value |
|---|---|
| Severity | Major |
| Category | Security / Correctness |
| File | packages/python/src/agent_wallet/local/kv_store.py : Lines 49–58; packages/typescript/src/local/kv-store.ts : getScryptParams() |
Description:
The _scrypt_params() / getScryptParams() functions automatically reduce the scrypt work factor from N=262144 to N=16384 when PYTEST_CURRENT_TEST (Python) or process.env.VITEST (TypeScript) is detected. While this is intended to speed up test execution, it creates a risk:
- Any production code inadvertently invoked while
PYTEST_CURRENT_TESTis set (e.g. integration tests, CI environments that run pytest) will encrypt with weaker parameters. The stored keystore will then be decrypted correctly (parameters are stored in the file), but the encryption security was reduced. - For TypeScript, the
VITESTenv var may be set in unrelated CI environments. - The approach bakes test-specific behavior into production code.
Recommendation:
Use a separate injectable parameter for tests rather than probing for test runner env vars. The SecureKVStore constructor or encryptBytes/decryptBytes functions could accept an optional scryptParams override, which tests pass explicitly.
[M-05] EnvWalletResolved type in TypeScript uses a union of one — misleading type definition (Major)
| Property | Value |
|---|---|
| Severity | Major |
| Category | Code Quality |
| File | packages/typescript/src/core/providers/wallet-builder.ts : Lines 47–52 |
Description:
The EnvWalletResolved type is declared as:
export type EnvWalletResolved =
| {
params: RawSecretPrivateKeyParams | RawSecretMnemonicParams
network: string | undefined
}A union of one member has no practical use. The network: string | undefined also loses the guarantee from resolveNetwork() that network can be undefined when no network is provided — a subtle semantic issue as callers of createEnvAdapter pass resolved.network which is then passed to RawSecretSigner, and parseNetworkFamily(undefined) throws "network is required". This means using EnvWalletProvider without a network will throw at signing time rather than at construction time.
Recommendation:
Consider removing the union wrapper and using a plain interface, and document or enforce the network requirement earlier.
[M-06] PrivyConfigResolver in TypeScript does not read from environment variables — inconsistency with the general pattern (Major)
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Documentation |
| File | packages/typescript/src/core/providers/privy-config.ts |
Description:
The PrivyConfigResolver.merge() only reads from this.source (the parsed config file). There is no provision for overriding app_id, app_secret, or wallet_id via environment variables (e.g. PRIVY_APP_ID, PRIVY_APP_SECRET, PRIVY_WALLET_ID). By contrast, the wallet password and wallet directory already support env var overrides. For a server-side deployment, operators may prefer to supply the Privy secret via env to avoid writing it to disk even in plaintext config. The Python implementation has the same limitation.
This is particularly important given finding [M-01] — env var support would be a mitigation for the plaintext secret storage concern.
Recommendation:
Add env var fallbacks (PRIVY_APP_ID, PRIVY_APP_SECRET, PRIVY_WALLET_ID) to merge(), document them in the README, and include them in the CLAUDE.md env vars table.
Minor
[N-01] TypeScript PrivyClient.rpc — object literal has inconsistent indentation (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | packages/typescript/src/core/clients/privy.ts : Lines 64–69 |
Description:
The object literal passed as body to this.request() in rpc() has the keys indented at the same level as the enclosing function call arguments, not inside the object. This may or may not cause a lint/prettier failure but is visually inconsistent with the rawSign method below it.
Code:
return this.request(
'POST',
`/v1/wallets/${walletId}/rpc`,
{
method, // <-- should be indented two more spaces
params,
},
options,
) as Promise<PrivyRpcResponse>Recommendation:
{
method,
params,
},[N-02] toHexValue / _to_hex_value — negative integers produce incorrect hex in Python (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 222–234 |
Description:
In Python, hex(-1) returns '-0x1', so passing a negative integer value to _to_hex_value would produce '-0x1' as the transaction field value instead of raising an error. This is unlikely in practice for gas/fee fields but is a silent corruption bug.
Recommendation:
Add a guard for negative integers:
if isinstance(value, int):
if value < 0:
raise ValueError(f"Expected non-negative integer for hex conversion, got {value}")
return hex(value)[N-03] tronSignBytes in TypeScript and Python use different hashing (keccak vs hash_message) — inconsistency with local TronSigner (Minor)
(This is a duplicate of finding [C-02] but noted here for the TypeScript side as a lesser concern.)
The TypeScript PrivyAdapter.tronSignBytes applies keccak256(bytes) before signing. The Python local TronSigner.sign_message also applies keccak256 via tron_key.sign_msg() (which internally calls sign_msg_hash(keccak(data))). This part is consistent. The Python Privy adapter is the outlier.
[N-04] normalizeTypedDataPayload mutates input dict in Python (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 237–242 |
Description:
_normalize_typed_data_payload mutates the typed dict in place by calling typed.pop("primaryType"). If the caller reuses the same payload dict after calling this function, the primaryType key will be unexpectedly absent.
Code:
if isinstance(typed, dict) and "primaryType" in typed and "primary_type" not in typed:
typed["primary_type"] = typed.pop("primaryType") # mutates caller's dictSimilarly in _tron_sign_typed_data, the normalized payload is mutated.
Recommendation:
Work on a copy: typed = dict(typed) before mutation, or use a non-mutating approach.
[N-05] buildPrivyConfig in TypeScript CLI reuses app credentials from existing wallet — app_secret reuse path leaks secret from config (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Security |
| File | packages/typescript/src/delivery/cli.ts : Lines 664–697 |
Description:
When adding a second Privy wallet that reuses credentials from an existing one, buildPrivyConfig reads the app_secret from the existing PrivyWalletParams config and embeds it in the new wallet config. This is the intended behavior, but it means the secret is passed through multiple layers in memory without zeroing. This is a low-severity concern given that it is already stored on disk, but should be noted.
[N-06] recoverTronRecoveryId silently swallows all exceptions in recovery loop (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 278–290; packages/python/src/agent_wallet/core/adapters/privy.py : Lines 258–272 |
Description:
The recovery loop catches all exceptions with a bare except (Python) / catch {} (TypeScript). If secp256k1.Signature.fromCompact() fails due to a malformed signature (not a recovery failure), the exception is silently consumed and both recovery bits are tried, ultimately leading to UnsupportedOperationError rather than a more descriptive error.
Recommendation:
Narrow the catch to ValueError / secp256k1-specific exceptions and let unexpected errors propagate.
[N-07] doc/how-to-add-privy-wallet.md omits warning about plaintext app_secret storage (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | doc/how-to-add-privy-wallet.md |
Description:
The how-to guide describes adding a Privy wallet but does not warn users that app_secret is stored in plaintext in wallets_config.json. This is a security-relevant omission that could lead users to inadvertently commit or expose the file containing their API secret.
[N-08] EnvWalletProvider constructor signature change is a breaking API change (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality / Compatibility |
| File | packages/typescript/src/core/providers/env-provider.ts; packages/python/src/agent_wallet/core/providers/env_provider.py |
Description:
EnvWalletProvider previously accepted privateKey, mnemonic, and accountIndex constructor parameters, allowing callers to inject secrets directly. These parameters have been removed. Any external code (e.g. integrations that build EnvWalletProvider directly) that passed these parameters will break silently at runtime in Python (no TypeError for keyword-only args in most cases) or fail at compile time in TypeScript.
The public API exports EnvWalletProvider via src/index.ts, making this a public breaking change.
[N-09] _scrypt_params catches ValueError on invalid AGENT_WALLET_TEST_SCRYPT_N but falls through to a separate PYTEST_CURRENT_TEST check instead of erroring (Minor)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/python/src/agent_wallet/local/kv_store.py : Lines 49–58 |
Description:
If AGENT_WALLET_TEST_SCRYPT_N is set to an invalid (non-integer) string, the except ValueError block silently sets n = DEFAULT_SCRYPT_N and then falls through to the PYTEST_CURRENT_TEST check, which could return the low-N test params. The original intent was to allow AGENT_WALLET_TEST_SCRYPT_N to override even in test contexts, but the silent fallback makes the behavior confusing.
Suggestions
[S-01] Consider making PrivyClient injectable via interface rather than concrete class in adapters
Having PrivyClient as a concrete dependency of PrivyAdapter (constructor parameter) is good. However, there is no abstract interface or protocol for PrivyClient that would allow easier mocking in unit tests beyond the ad-hoc FakePrivyClient in test files. Introducing a PrivyClientProtocol (Python) / interface (TypeScript) would formalize the contract.
[S-02] normalizeTransactionPayload — the pick() helper returns undefined if no keys match, and assign() skips undefined, but null values are passed through
If a caller explicitly passes { value: null }, assign('value', null) would skip it (correct for Privy's API). This is intended behavior but should be documented or verified against Privy's API spec.
[S-03] Retry backoff for rate limiting is very aggressive
Max backoff is 1 second with 2 retries (200ms * 1, 200ms * 2 = 400ms, max 1s). Privy's rate limits may be strict enough that a longer backoff (e.g. exponential starting at 1s) would be more effective.
[S-04] Test fixtures using session-scoped template dirs (initialized_template_dir, signer_template_dir)
The session-scoped fixtures in Python tests/test_cli.py that copy a pre-initialized template are a good optimization. However, the template setup uses a module-level monkeypatch in a session fixture, which is not the recommended pattern. Consider using pytest.MonkeyPatch() as a context manager instead of monkeypatch.undo() in a finally block.
Positive Observations
-
Dual-language parity is excellent. The Python and TypeScript implementations mirror each other closely in structure and behavior. The new utility modules (
core/utils/) properly eliminate the duplicated helper functions that existed before this PR. -
SignOptionsdesign is clean. The optionalauthorizationSignatureparameter threads through all signing methods without breaking the existingWalletinterface for non-Privy adapters. The existing adapters correctly accept and ignore the option parameter. -
Recovery-bit computation for TRON is correct. The approach of trying both
v=0andv=1, recovering the public key, deriving the Tron address, and comparing against the cached address is the right approach for araw_signAPI that returns 64-byte r||s without the recovery bit. -
Cache invalidation on wallet-info fetch is appropriately minimal. Both
getAddress()andgetChainType()share the same cache, preventing double-fetches. The lazy initialization pattern is appropriate. -
Non-interactive environment detection (
requireInteractive/_require_interactive) is a useful improvement that provides clear error messages rather than hanging on TTY prompts. -
Scrypt parameters are now stored and read back from the keystore, ensuring that wallets encrypted with test-mode parameters (N=16384) can always be decrypted correctly even if the test environment flag is not set at decryption time.
-
Error hierarchy (
PrivyConfigError,PrivyRequestError,PrivyRateLimitError,PrivyAuthError) is well-designed and allows callers to handle specific Privy failure modes. -
Test coverage is comprehensive. New test files cover happy paths and edge cases for the Privy adapter, client, and config resolver. The existing CLI test suite is extended with Privy-specific flows.
Review Verdict
Request Changes
Must fix before merge:
- [C-01] Python
urllib.request.urlopenraisesHTTPErroron 4xx/5xx — HTTP error responses are never handled correctly. Rate limiting, auth failures, and request errors are all silently propagated as uncaughturllib.error.HTTPErrorinstead of the typedPrivy*Errorclasses. - [C-02] Python
_tron_sign_bytesusestronpy.keys.hash_message(adds TRON-prefix) while TypeScript uses plainkeccak256— produces incompatible signatures for TRON message signing across the two implementations. - [M-01]
app_secretstored in plaintext inwallets_config.json— at minimum, a CLI warning and documentation note are required. - [M-02] Python Privy HTTP client is synchronous (blocking
urlopen+time.sleep) called from async context — will block the asyncio event loop in practice. - [M-03] TypeScript
cachedChainTypepopulated without.toLowerCase()ingetAddress()— can cause TRON routing to fail silently.
Should fix before merge:
- [M-04] Test-environment detection baked into production
_scrypt_params(). - [M-06] No env var override support for Privy credentials.
- [N-08] Breaking change to
EnvWalletProviderpublic constructor API.
Audit Report — PR: main → feat/privy-supportRepository: agent-wallet 1. PR Overview
The PR introduces Privy API-backed wallet support to both TypeScript and Python packages. It also adds:
2. Commit History
3. Change Summary3.1 New core components
3.2 Interface changes
3.3 Provider and resolver updates
3.4 Utility modules extracted
3.5 Address resolution
3.6 CLI enhancements
3.7 Storage
4. Detailed FindingsCriticalC-01 — Correctness | Python
|
| Category | Status | Notes |
|---|---|---|
| Correctness | Needs Work | C-01 (crash), MJ-03 (silent failure), MJ-04 (cache inconsistency), MJ-05 (hash mismatch) |
| Security | Needs Work | C-02 (plaintext secret at rest), MJ-02 (no explicit TLS enforcement in Python) |
| Performance | Needs Work | MJ-01 (blocking I/O in async Python code) |
| Code Quality | Acceptable | MN-01 (mutation), MN-03 (indentation), MN-07 (broad exception catch) |
| Testing | Acceptable | Good coverage; MN-06 (untyped fake client) |
| Documentation | Acceptable | MN-04 (missing security warning) |
| Compatibility | Pass | SignOptions parameter is optional; backward-compatible |
| Observability | Pass | Error messages are descriptive; error types are granular |
7. Review Verdict
Request Changes
The PR introduces a well-designed and well-tested Privy integration. However, two issues block approval:
-
C-01 is a crash bug:
remove_wallet()will raiseAttributeErrorin Python whenever a wallet has been cached, which is the normal operating state. This is a regression against existing wallet management. -
C-02 is a design-level security concern: storing the Privy
app_secret(a server-side credential with authority over all wallets in a Privy application) in plaintext JSON contradicts the project's "secure signing SDK" identity. This is particularly severe because the existing keystore mechanism is available and well-tested. -
MJ-01 (blocking I/O in an async Python context) and MJ-04 (cache normalization bug causing chain mis-detection) should also be addressed before merge.
After fixing C-01, C-02, MJ-01, and MJ-04, the PR would be in good shape to approve.
Code Review ReportProject: agent-wallet ( PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR introduces Privy-backed wallet support across both TypeScript and Python implementations, adding a third wallet type ( However, there is one critical bug that must be fixed before merge: the Python Change Summary1. Privy Wallet Adapter (Core Feature)
Purpose: Enables AI agents to sign with wallets hosted by Privy without holding private key material locally. 2.
|
| File | Change Type | Description |
|---|---|---|
packages/typescript/src/core/base.ts |
Modified | Added SignOptions type; authorizationSignature? field |
packages/python/src/agent_wallet/core/base.py |
Modified | Added SignOptions dataclass; propagated to all abstract methods |
packages/*/core/adapters/evm.*, tron.*, local.*, local_secure.*, raw_secret.* |
Modified | Updated signatures to accept optional SignOptions |
Purpose: Allows callers to pass a Privy authorization signature when the Privy wallet requires server-side authorization checks.
3. WalletType & Config Extended for Privy
| File | Change Type | Description |
|---|---|---|
packages/typescript/src/core/base.ts |
Modified | Added WalletType.PRIVY = 'privy' |
packages/python/src/agent_wallet/core/base.py |
Modified | Added WalletType.PRIVY |
packages/typescript/src/core/config.ts |
Modified | PrivyWalletParamsSchema added; WalletConfigSchema union extended |
packages/python/src/agent_wallet/core/config.py |
Modified | PrivyWalletParams model; validator extended |
packages/*/core/providers/wallet-builder.* |
Modified | createAdapter/create_adapter handles privy type |
Purpose: Privy wallets participate in all existing config-driven wallet lifecycle (add, list, sign, remove).
4. Provider Refactors
| File | Change Type | Description |
|---|---|---|
packages/*/core/providers/env-provider.* |
Modified | EnvWalletProvider reads env map lazily; getWallet → getActiveWallet alignment |
packages/*/core/providers/config-provider.* |
Modified | Cache key changed from "id:network" string to (id, type, network) tuple; Privy wallets skip network resolution |
packages/*/core/resolver.* |
Modified | Env constants moved to base; refactored to use shared utilities |
Purpose: Privy wallets are network-agnostic (address comes from Privy API, not derivation), so the cache and network-resolution paths were updated to handle undefined network.
5. Utility Modules Extracted
| File | Change Type | Description |
|---|---|---|
packages/typescript/src/core/utils/env.ts |
Added | cleanEnvValue, firstEnv, parseAccountIndex |
packages/typescript/src/core/utils/hex.ts |
Added | stripHexPrefix |
packages/typescript/src/core/utils/keys.ts |
Added | decodePrivateKey, deriveKeyFromMnemonic |
packages/typescript/src/core/utils/network.ts |
Added | parseNetworkFamily, resolveNetwork |
packages/python/src/agent_wallet/core/utils/env.py |
Added | Python counterparts |
packages/python/src/agent_wallet/core/utils/network.py |
Added | Python counterparts |
packages/python/src/agent_wallet/core/utils/keys.py |
Added | Python counterparts (from wallet_builder.py) |
Purpose: Deduplication — these were scattered across resolver, local.ts, raw-secret.ts, wallet_builder.py, reducing import coupling.
6. Address Resolution
| File | Change Type | Description |
|---|---|---|
packages/typescript/src/core/address-resolution.ts |
Added | Resolves EVM+TRON addresses for local wallets, single address for Privy |
packages/python/src/agent_wallet/core/address_resolution.py |
Added | Python counterpart |
Purpose: Powers the new CLI resolve-address command; Privy returns one canonical address, local wallets return both EVM and TRON addresses.
7. KV-Store: Test-Mode Scrypt Parameters
| File | Change Type | Description |
|---|---|---|
packages/typescript/src/local/kv-store.ts |
Modified | Adds getScryptParams() that lowers N in VITEST / when AGENT_WALLET_TEST_SCRYPT_N is set |
packages/python/src/agent_wallet/local/kv_store.py |
Modified | Same; uses PYTEST_CURRENT_TEST env var instead |
Purpose: Speed up test runs that exercise encryption without weakening production defaults.
Detailed Findings
Critical
[C-01] Python PrivyClient Error Handling and Retry Logic Are Unreachable
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 63–89 |
Description
urllib.request.urlopen raises urllib.error.HTTPError for all HTTP error responses (4xx, 5xx). HTTPError is an exception, so control never reaches the if status == 429 or if status >= 400 branches that appear after the with urlopen(req) as response: block. In practice:
- Rate-limit retry (
429) will never trigger — theHTTPErrorpropagates up unhandled. - Auth errors (
401,403) will surface as a rawurllib.error.HTTPErrorinstead ofPrivyAuthError. - General request errors (
4xx/5xx) will be an uncaughtHTTPErrorinstead ofPrivyRequestError.
The TypeScript client uses the fetch API, which does not throw for non-2xx responses; the Python port incorrectly assumed urlopen behaves the same way.
Code
# privy.py – Lines 63–89
def _request(self, method, path, body=None, *, authorization_signature=None):
attempt = 0
while True:
req = Request(
f"{self._base_url}{path}",
method=method,
headers=self._headers(authorization_signature),
data=json.dumps(body).encode("utf-8") if body else None,
)
# urlopen RAISES HTTPError for 4xx/5xx — the with-block body is never entered for errors
with urlopen(req) as response:
status = getattr(response, "status", 0)
payload = _read_json(response)
if status == 429: # ← DEAD CODE for 429
if attempt >= self._retries:
raise PrivyRateLimitError("Privy rate limit exceeded")
attempt += 1
self._sleep(_backoff_seconds(attempt))
continue
if status >= 400: # ← DEAD CODE for 4xx/5xx
message = _extract_error(payload) or f"Privy request failed with status {status}"
if status in (401, 403):
raise PrivyAuthError(message)
raise PrivyRequestError(message)
return payloadRecommendation
Catch urllib.error.HTTPError explicitly and re-use its status code and body:
from urllib.error import HTTPError, URLError
def _request(self, method, path, body=None, *, authorization_signature=None):
attempt = 0
while True:
req = Request(
f"{self._base_url}{path}",
method=method,
headers=self._headers(authorization_signature),
data=json.dumps(body).encode("utf-8") if body else None,
)
try:
with urlopen(req, timeout=30) as response:
status = response.status
payload = _read_json(response)
except HTTPError as exc:
status = exc.code
try:
payload = json.loads(exc.read().decode("utf-8"))
except Exception:
payload = {}
except URLError as exc:
raise PrivyRequestError(f"Privy request network error: {exc}") from exc
if status == 429:
if attempt >= self._retries:
raise PrivyRateLimitError("Privy rate limit exceeded")
attempt += 1
self._sleep(_backoff_seconds(attempt))
continue
if status >= 400:
message = _extract_error(payload) or f"Privy request failed with status {status}"
if status in (401, 403):
raise PrivyAuthError(message)
raise PrivyRequestError(message)
return payloadMajor
[MJ-01] Privy app_secret Stored in Cleartext in wallets_config.json
| Property | Value |
|---|---|
| Severity | Major |
| Category | Security |
| File | packages/typescript/src/core/config.ts : Lines 43–48 / packages/python/src/agent_wallet/core/config.py : Lines 42–48 |
Description
PrivyWalletParams stores app_secret as a plain string that is written directly into ~/.agent-wallet/wallets_config.json. The existing local_secure wallet type protects private keys with Keystore V3 (scrypt + AES-128-CTR). The new Privy wallet type stores its credential with no encryption. If the config file is accessed by a malicious process, the app secret is immediately readable and can be used to sign any transaction on behalf of every wallet under that Privy app.
Code
// config.ts
export const PrivyWalletParamsSchema = z.object({
app_id: z.string(),
app_secret: z.string(), // ← stored verbatim in wallets_config.json
wallet_id: z.string(),
})Recommendation
At minimum, document clearly that app_secret is stored unencrypted and recommend users restrict file permissions (chmod 600). For a stronger fix, encrypt app_secret via the existing KV-store (Keystore V3) using the user's master password — identical to how local_secure stores private keys. The wallets_config.json would then store only a secret_ref for the Privy credentials, consistent with the existing architecture.
[MJ-02] TRON Signature v-Byte Inconsistency Between Python and TypeScript Privy Adapters
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 133–138 vs packages/python/src/agent_wallet/core/adapters/privy.py : Lines 148–151 |
Description
When the Privy adapter signs a TRON transaction/message, the final signature byte (v) is computed inconsistently:
- TypeScript: adds
27to the recovery bit → appends0x1bor0x1c - Python: appends the raw recovery bit → appends
0x00or0x01
The local TronSigner in TypeScript also adds 27 (sig.recovery + 27). Broadcasting a signature with the wrong v-byte to TronGrid will cause the transaction to be rejected or replayed incorrectly. One of the two implementations produces invalid TRON signatures.
Code
// privy.ts – tronSignHash (TypeScript)
const v = await recoverTronRecoveryId(sigBytes, hash, await this.getAddress())
const vHex = (v + 27).toString(16).padStart(2, '0') // → "1b" or "1c"
return `${sigHex}${vHex}`# privy.py – _tron_sign_hash (Python)
v = _recover_tron_v(sig_bytes, digest, await self.get_address())
return (sig_hex + f"{v:02x}").lower() # → "00" or "01"Recommendation
Determine the canonical format expected by TronGrid (compare with compare_sign_consistency examples already in this PR). Once confirmed, apply the same convention in both adapters. If +27 is correct (consistent with TypeScript TronSigner), add the offset in Python:
v_byte = v + 27
return (sig_hex + f"{v_byte:02x}").lower()[MJ-03] Python PrivyClient Blocks the Async Event Loop
| Property | Value |
|---|---|
| Severity | Major |
| Category | Performance |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 37–100 |
Description
PrivyClient uses urllib.request.urlopen, a synchronous blocking call. All PrivyAdapter methods (get_address, sign_transaction, sign_message, etc.) are declared async and call this client directly — without asyncio.run_in_executor or equivalent. Under asyncio (the assumed runtime for agent workloads), each Privy API call will stall the entire event loop until the HTTP round-trip completes. This blocks all other coroutines for the duration of the call, including timeouts and concurrency-dependent agent logic.
Code
# privy.py – adapter method
async def get_address(self) -> str:
if self._cached_address:
return self._cached_address
payload = self._client.get_wallet(self._wallet_id) # ← synchronous, blocks event loop
...Recommendation
Either:
- (Preferred) Replace
urllib.requestwithhttpx(async) oraiohttp, makingPrivyClientmethodsasync def— consistent with the TypeScript client that usesfetch. - (Interim) Wrap the synchronous calls in
asyncio.get_event_loop().run_in_executor(None, ...)to offload to a thread pool, keeping the event loop unblocked.
[MJ-04] No HTTP Request Timeout in Python PrivyClient
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 63–70 |
Description
urlopen is called without a timeout parameter. If the Privy API is unreachable or slow, the call will block indefinitely. In production agent workflows this can cause the entire agent process to hang. The TypeScript client inherits a default timeout from the platform's fetch implementation (browser/Node 18+ defaults), but the Python client has no such protection.
Code
with urlopen(req) as response: # ← no timeout
status = getattr(response, "status", 0)
payload = _read_json(response)Recommendation
Pass a timeout argument (and make it configurable):
class PrivyClient:
def __init__(self, *, app_id, app_secret, retries=2, timeout=30.0, sleep=None):
...
self._timeout = timeout
# In _request:
with urlopen(req, timeout=self._timeout) as response:
...Minor
[MN-01] normalizeTypedDataPayload Mutates the Caller's Input Object
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 218–227 / packages/python/src/agent_wallet/core/adapters/privy.py : Lines 199–208 |
Description
normalizeTypedDataPayload modifies the typed_data object in-place when renaming primaryType → primary_type. Because JavaScript objects and Python dicts are passed by reference, the caller's original data dict is silently mutated. This is surprising behavior and can cause hard-to-trace bugs if the caller reuses the same object.
Code
// privy.ts
function normalizeTypedDataPayload(data: Record<string, unknown>): Record<string, unknown> {
const payload = 'typed_data' in data ? data : { typed_data: data }
const typed = payload.typed_data as Record<string, unknown> | undefined
if (!typed) return payload
if ('primaryType' in typed && !('primary_type' in typed)) {
typed.primary_type = typed.primaryType // mutates caller's object
delete typed.primaryType // mutates caller's object
}
return payload
}Recommendation
Work on a shallow copy of typed_data:
function normalizeTypedDataPayload(data: Record<string, unknown>): Record<string, unknown> {
const rawTyped = 'typed_data' in data ? data.typed_data : data
const typed = { ...(rawTyped as Record<string, unknown>) } // shallow copy
if ('primaryType' in typed && !('primary_type' in typed)) {
typed.primary_type = typed.primaryType
delete typed.primaryType
}
return 'typed_data' in data ? { ...data, typed_data: typed } : { typed_data: typed }
}[MN-02] bs58checkInterop Compatibility Shim Duplicated Between privy.ts and tron.ts
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 7–19 / packages/typescript/src/core/adapters/tron.ts : Lines 5–17 |
Description
The identical CJS/ESM interop shim for bs58check appears verbatim in both privy.ts and tron.ts. Code duplication means any future fix (e.g., when bs58check releases a cleaner export) must be applied in two places.
Code
// Duplicated in both privy.ts and tron.ts:
type Bs58checkLike = { encode?: (input: Uint8Array) => string; default?: typeof bs58checkModule }
const bs58checkInterop = bs58checkModule as Bs58checkLike
const bs58check: typeof bs58checkModule =
typeof bs58checkInterop.encode === 'function'
? bs58checkModule
: (bs58checkInterop.default ?? bs58checkModule)Recommendation
Extract to packages/typescript/src/core/utils/bs58check.ts and import from both adapters.
[MN-03] EnvWalletResolved Is a Superfluous Single-Variant Union
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | packages/typescript/src/core/providers/wallet-builder.ts : Lines 38–44 |
Description
EnvWalletResolved is declared as a discriminated union with only one variant. A plain object type would be simpler, less noisy, and equally expressive. The Python equivalent (EnvWalletResolved = tuple[..., str | None]) is a tuple type alias, which is also inconsistent with the TypeScript definition.
Code
export type EnvWalletResolved =
| {
params: RawSecretPrivateKeyParams | RawSecretMnemonicParams
network: string | undefined
}Recommendation
export type EnvWalletResolved = {
params: RawSecretPrivateKeyParams | RawSecretMnemonicParams
network: string | undefined
}[MN-04] Test-Mode Scrypt Downgrade via Env Var Poses Production Risk
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Security |
| File | packages/typescript/src/local/kv-store.ts : Lines 42–60 / packages/python/src/agent_wallet/local/kv_store.py : Lines 34–51 |
Description
getScryptParams() / _scrypt_params() reduce the Keystore V3 N parameter to 16384 (1/16 of the production value) whenever the VITEST or PYTEST_CURRENT_TEST env vars are set. The AGENT_WALLET_TEST_SCRYPT_N env var allows arbitrary further reduction. If any of these are set in a production environment (e.g., via a misconfigured CI deployment), newly encrypted keystores will use a far weaker KDF, lowering brute-force resistance by a factor of 16×.
Recommendation
Add a guard that explicitly refuses to apply test parameters if the process appears to be running in production (e.g., NODE_ENV === 'production'). Log a visible warning if test scrypt parameters are active. Alternatively, gate the entire mechanism behind a compile-time constant that is stripped from release builds.
[MN-05] resolveNetwork Changed from Throwing to Returning undefined — Silent Breakage Risk
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/utils/network.ts : Lines 11–17 / packages/python/src/agent_wallet/core/utils/network.py : Lines 19–24 |
Description
The old resolveNetwork raised an error when both explicit and providerDefault were undefined. The new version returns undefined silently. For Privy wallets this is correct (network-agnostic). For RawSecretSigner and LocalSigner, undefined is passed to parseNetworkFamily, which does throw — but the error surface has moved downstream, making it harder to trace. There is no early-fail guard for callers that expect a network to be set.
Recommendation
Consider an overloaded or typed variant — e.g., resolveRequiredNetwork(explicit, default): string (throws) and resolveOptionalNetwork(...): string | undefined (returns undefined) — so callers can express their intent explicitly and IDE type-checking catches mismatches.
Suggestions
[S-01] Add Jitter to Retry Backoff in Both Clients
File: packages/typescript/src/core/clients/privy.ts : Line 152 / packages/python/src/agent_wallet/core/clients/privy.py : Line 121
Description: Both clients use a simple linear backoff (200ms * attempt / 0.2s * attempt). With multiple concurrent agents hitting Privy simultaneously, all will retry at the same interval — a classic thundering-herd scenario.
Suggestion: Add randomized jitter: backoffMs = Math.min(1000, 200 * attempt) + Math.random() * 100.
[S-02] Export PrivyClient and PrivyConfigResolver Only as Internal — or Document Public API Contract
File: packages/typescript/src/index.ts : Lines 44–46
Description: PrivyClient and PrivyConfigResolver are now part of the public index.ts exports. These are low-level infrastructure classes; exporting them signals they are stable public API. If they change (e.g., async client rewrite), it constitutes a semver-breaking change.
Suggestion: Either mark them @internal in JSDoc, place them behind a sub-path export (@bankofai/agent-wallet/privy), or deliberately accept the API commitment. Similarly document in Python's __all__.
[S-03] PrivyConfigResolver Should Support Env Var Fallback
File: packages/typescript/src/core/providers/privy-config.ts / packages/python/src/agent_wallet/core/providers/privy_config.py
Description: All other credential sources (AGENT_WALLET_PASSWORD, AGENT_WALLET_PRIVATE_KEY, etc.) support env var injection. PrivyConfigResolver only reads from source object — there is no PRIVY_APP_ID / PRIVY_APP_SECRET / PRIVY_WALLET_ID fallback. This forces every deployment to write Privy credentials to a config file.
Suggestion: Extend PrivyConfigResolver to fall back to AGENT_WALLET_PRIVY_APP_ID, AGENT_WALLET_PRIVY_APP_SECRET, and AGENT_WALLET_PRIVY_WALLET_ID env vars, consistent with the env-first pattern already used by the raw-secret provider.
[S-04] Python PrivyAdapter._tron_sign_bytes Uses tronpy.hash_message While TypeScript Uses Raw keccak256 — Verify Equivalence
File: packages/python/src/agent_wallet/core/adapters/privy.py : Lines 130–135
Description: tronpy.keys.hash_message may apply a TRON-specific prefix before hashing (similar to Ethereum's \x19Ethereum Signed Message:\n prefix). If it does, the Python and TypeScript Privy adapters will sign different hashes for the same input bytes, breaking cross-implementation consistency.
Suggestion: Replace tronpy.keys.hash_message(data) with a direct keccak(data) call (using eth_utils.keccak) to match the TypeScript keccak256(bytes) behavior, and add a cross-language consistency test (the compare_sign_consistency examples already added are a good model).
Positive Observations
| Area | Observation |
|---|---|
| Dual-language parity | Python and TypeScript implementations closely mirror each other's structure, making it easier to maintain both in lockstep |
| Utility extraction | env.ts, hex.ts, keys.ts, network.ts (and Python equivalents) cleanly decouple shared logic from adapters, reducing import coupling |
| Error taxonomy | Four distinct Privy error types (PrivyConfigError, PrivyRequestError, PrivyRateLimitError, PrivyAuthError) give callers precise error handling ability |
| Cache strategy upgrade | Changing the config-provider wallet cache from a flat "id:network" string to a typed (id, type, network) tuple correctly handles Privy wallets that have no network dimension |
| SignOptions propagation | The authorizationSignature option is threaded through every signing method consistently, ready for Privy server-authorization workflows |
| TRON txID flexibility | Both TronSigner and PrivyAdapter now accept transactions without a pre-computed txID, computing SHA256(raw_data_hex) locally — a useful robustness improvement |
| Test coverage | New files (test_privy_adapter, test_privy_client, test_privy_config, privy-adapter.test.ts, privy-client.test.ts, privy-config.test.ts) provide dedicated unit tests for all new components |
| Address resolution | The resolveWalletAddresses helper correctly returns a single canonical address for Privy (network-agnostic) vs. dual EVM+TRON addresses for local wallets — clean polymorphism |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 5 | 3 | 0 | urlopen error handling (C-01), v-byte mismatch (MJ-02), input mutation (MN-01) |
| Security | 5 | 3 | 2 | 0 | app_secret plaintext (MJ-01), test scrypt via env (MN-04) |
| Performance | 4 | 2 | 2 | 0 | Blocking I/O (MJ-03), no timeout (MJ-04) |
| Code Quality | 6 | 4 | 2 | 0 | bs58check duplication (MN-02), EnvWalletResolved type (MN-03) |
| Testing | 5 | 5 | 0 | 0 | Good coverage for all new components |
| Documentation | 4 | 3 | 1 | 0 | app_secret cleartext not documented; new env vars undocumented |
| Compatibility | 3 | 3 | 0 | 0 | Backward-compatible; new wallet type is additive |
| Observability | 2 | 2 | 0 | 0 | Error messages are descriptive |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between main and feat/privy-support. Runtime behavior, integration testing against live Privy API, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-03-31
Code Review ReportProject: agent-wallet ( PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR introduces Privy as a third wallet provider type alongside the existing However, the implementation has two critical defects that could cause production failures. First, the Python Four additional major findings round out the review: blocking synchronous I/O inside Change Summary1 — New Privy Wallet Provider (Core)
Purpose: Adds Privy as a cloud-custody signing backend. The adapter routes EVM signing through Privy's 2 — Config & Provider Extension
Purpose: Wire the new 3 — SignOptions Threading
Purpose: Allows callers to pass a Privy-required 4 — TRON txID Auto-Derivation
Purpose: Makes the SDK resilient to callers that omit 5 — Address Resolution Command
Purpose: CLI command to display a wallet's addresses across both network families. 6 — KV-Store / Keystore V3 Improvements
Purpose: Fixes a latent bug where decryption always used hardcoded params regardless of what params were stored in the keystore. Also speeds up tests significantly. 7 — Utility Module Refactor
Purpose: DRY refactor — shared parsing/derivation utilities now live in their own modules. Detailed FindingsCritical[C-01] Python
|
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 56–84 |
Description
urllib.request.urlopen raises urllib.error.HTTPError (a subclass of urllib.error.URLError) for any response with a 4xx or 5xx status code — it does not return the response object. The current code reads status and payload after the with urlopen(req) block, which means urlopen throws before those lines can execute. As a result:
- The rate-limit retry loop (
if status == 429) is never reached; a 429 from Privy surfaces as an unhandledHTTPErrorto the caller. - The typed error classification (
PrivyAuthErrorfor 401/403,PrivyRequestErrorfor other 4xx) is never executed. - All 4xx/5xx errors propagate as raw
urllib.error.HTTPErrorobjects, bypassing the SDK's error hierarchy entirely.
The TypeScript counterpart using fetch is correct because fetch does not throw on non-2xx responses.
Code
# packages/python/src/agent_wallet/core/clients/privy.py
def _request(self, method, path, body=None, *, authorization_signature=None):
attempt = 0
while True:
req = Request(...)
with urlopen(req) as response: # ← raises HTTPError for 4xx/5xx
status = getattr(response, "status", 0) # never reached on error
payload = _read_json(response) # never reached on error
if status == 429: # dead code for any 429 response
if attempt >= self._retries:
raise PrivyRateLimitError(...)
attempt += 1
self._sleep(_backoff_seconds(attempt))
continue
if status >= 400: # dead code for any 4xx/5xx response
...
raise PrivyAuthError(message)
raise PrivyRequestError(message)Recommendation
Catch urllib.error.HTTPError, which itself exposes .code, .read():
import urllib.error
def _request(self, method, path, body=None, *, authorization_signature=None):
attempt = 0
while True:
req = Request(
f"{self._base_url}{path}",
method=method,
headers=self._headers(authorization_signature),
data=json.dumps(body).encode("utf-8") if body else None,
)
try:
with urlopen(req, timeout=30) as response:
payload = _read_json(response)
return payload
except urllib.error.HTTPError as exc:
status = exc.code
try:
payload = json.loads(exc.read().decode("utf-8"))
except Exception:
payload = {}
if status == 429:
if attempt >= self._retries:
raise PrivyRateLimitError("Privy rate limit exceeded") from exc
attempt += 1
self._sleep(_backoff_seconds(attempt))
continue
message = _extract_error(payload) or f"Privy request failed with status {status}"
if status in (401, 403):
raise PrivyAuthError(message) from exc
raise PrivyRequestError(message) from exc
except urllib.error.URLError as exc:
raise PrivyRequestError(f"Privy network error: {exc.reason}") from exc[C-02] app_secret persisted in plaintext in wallets_config.json
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Security |
| File | packages/typescript/src/core/config.ts : Lines 43–48; packages/python/src/agent_wallet/core/config.py : Lines 41–49 |
Description
PrivyWalletParams is serialised directly into wallets_config.json as plain JSON. This means the Privy app_secret — which grants full signing authority over any wallet associated with the app — is stored unencrypted on disk. The existing local_secure wallet type encrypts private keys using Keystore V3 (scrypt + AES-128-CTR) precisely to prevent credential exposure from a stolen config file. Introducing a new wallet type that skips this protection is a regression in the security model.
Code
// packages/typescript/src/core/config.ts
export const PrivyWalletParamsSchema = z.object({
app_id: z.string(),
app_secret: z.string(), // ← written verbatim to wallets_config.json
wallet_id: z.string(),
})// resulting wallets_config.json entry (example)
{
"type": "privy",
"params": {
"app_id": "clxxxxxxxxxxxxxxx",
"app_secret": "actual-secret-in-plaintext",
"wallet_id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
}
}Recommendation
Apply one or more of the following mitigations:
-
Env-var resolution (preferred short-term): Resolve
app_secretfrom an environment variable (e.g.PRIVY_APP_SECRET) at runtime and never persist it to disk. Store onlyapp_idandwallet_idin the config file. -
Keystore encryption (preferred long-term): Encrypt
app_secretwith the master password using the same Keystore V3 mechanism used for private keys, storing the keystore blob assecret_<id>.jsonand referencing it bysecret_refin the config. -
Minimum: Document clearly that
wallets_config.jsoncontains sensitive credentials and ensure the file permissions (already0o700on the directory) are enforced on the file itself.
Major
[MJ-01] Python PrivyAdapter performs synchronous (blocking) HTTP I/O inside async methods
| Property | Value |
|---|---|
| Severity | Major |
| Category | Performance / Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 31–34, 50–57, 63–70, 75–81 |
Description
PrivyAdapter.get_address(), sign_transaction(), sign_message(), sign_typed_data(), _tron_sign_hash(), etc. are all declared async def but call self._client.get_wallet(), self._client.rpc(), and self._client.raw_sign() which are synchronous methods backed by urllib.request.urlopen (blocking I/O). Running blocking I/O directly inside an asyncio event loop blocks the thread for the duration of each HTTP request, preventing the event loop from processing any other coroutines.
# packages/python/src/agent_wallet/core/adapters/privy.py
async def get_address(self) -> str:
...
payload = self._client.get_wallet(self._wallet_id) # ← blocks the event loop
...
async def _tron_sign_hash(self, digest: bytes, options=None) -> str:
response = self._client.raw_sign(...) # ← blocks the event loopRecommendation
Use asyncio.get_event_loop().run_in_executor(None, ...) to run the synchronous calls in a thread pool, or replace urllib with an async HTTP client (aiohttp, httpx[asyncio]):
import asyncio
async def _async_request(self, *args, **kwargs):
loop = asyncio.get_event_loop()
return await loop.run_in_executor(
None, lambda: self._client._request(*args, **kwargs)
)Alternatively, expose an async PrivyClient backed by aiohttp/httpx and inject it.
[MJ-02] Missing HTTP request timeout in Python PrivyClient
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Performance |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Line 65 |
Description
urlopen(req) is called without a timeout argument. Python's default is no timeout, meaning a request can hang indefinitely if the Privy API server is unreachable or unresponsive. This would deadlock any async context and hang CLI commands.
The TypeScript fetch also does not set a timeout, but browsers and Node.js fetch implementations typically surface errors more quickly; the Python case is more hazardous given the synchronous-in-async issue noted in MJ-01.
Code
# packages/python/src/agent_wallet/core/clients/privy.py line ~65
with urlopen(req) as response: # no timeout – can hang indefinitelyRecommendation
PRIVY_REQUEST_TIMEOUT_SECONDS = 30
with urlopen(req, timeout=PRIVY_REQUEST_TIMEOUT_SECONDS) as response:
...Consider making the timeout configurable via PrivyClientConfig.
[MJ-03] Empty chainType string causes cache miss — getWallet API called on every signing operation
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Performance |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 26, 95–100; packages/python/src/agent_wallet/core/adapters/privy.py : Lines 38–43 |
Description
Both PrivyAdapter.getAddress() and getChainType() use the truthiness of the cached value to decide whether to skip the API call:
// TypeScript
if (this.cachedAddress) return this.cachedAddress // falsy on ""
if (this.cachedChainType) return this.cachedChainType // falsy on ""# Python
if self._cached_chain_type:
return self._cached_chain_type # falsy on ""For an EVM wallet, the Privy GET /v1/wallets/{id} response returns chain_type = "ethereum" (or similar), which is truthy. However, if the API ever returns an empty string, null, or an unknown variant, the adapter stores "" (the normalised fallback) and re-fetches on every subsequent call. Since signTransaction, signMessage, and signTypedData all call getChainType(), a single signing session could generate O(n) redundant wallet-lookup API calls. Because getAddress() has the same guard, the first address fetch would also not be cached.
Code
// packages/typescript/src/core/adapters/privy.ts
private async getChainType(): Promise<string> {
if (this.cachedChainType) return this.cachedChainType // '' is falsy
const wallet = await this.client.getWallet(this.config.walletId)
this.cachedAddress = wallet.address
this.cachedChainType = wallet.chainType?.toLowerCase() ?? ''
return this.cachedChainType
}Recommendation
Use null as the sentinel (not '') to distinguish "not yet fetched" from "API returned empty":
private cachedChainType: string | null = null
private async getChainType(): Promise<string> {
if (this.cachedChainType !== null) return this.cachedChainType
const wallet = await this.client.getWallet(this.config.walletId)
this.cachedAddress = wallet.address
this.cachedChainType = wallet.chainType?.toLowerCase() ?? ''
return this.cachedChainType
}Apply the same fix to getAddress() and the Python adapter.
[MJ-04] kdfparams values from keystore file are trusted without bounds validation
| Property | Value |
|---|---|
| Severity | Major |
| Category | Security |
| File | packages/typescript/src/local/kv-store.ts : Lines 113–119; packages/python/src/agent_wallet/local/kv_store.py : Lines 116–123 |
Description
Prior to this PR, scrypt parameters were hardcoded constants. The new decryptBytes reads n, r, p, dklen from the keystore JSON file and passes them directly to the scrypt KDF. If an attacker can modify the keystore file (e.g. via path traversal, compromised backup, or a shared filesystem), they can set n=2 to reduce the scrypt work factor to nearly zero, making an offline dictionary attack trivially fast.
This is a known Keystore V3 design tradeoff, but the risk should be mitigated by at minimum validating that N is not below a safe floor.
Code
// packages/typescript/src/local/kv-store.ts
const derivedKey = deriveKey(password, salt, {
N: Number(kdfparams.n), // no lower-bound check
r: Number(kdfparams.r),
p: Number(kdfparams.p),
dklen: Number(kdfparams.dklen),
})Recommendation
Validate parameters before use:
const MIN_SCRYPT_N = 16384 // same as test value; absolute floor
function validateScryptParams(params: { N: number; r: number; p: number; dklen: number }) {
if (!Number.isInteger(params.N) || params.N < MIN_SCRYPT_N)
throw new DecryptionError(`Invalid scrypt N: ${params.N} (minimum ${MIN_SCRYPT_N})`)
if (!Number.isInteger(params.r) || params.r < 1)
throw new DecryptionError(`Invalid scrypt r: ${params.r}`)
if (!Number.isInteger(params.p) || params.p < 1)
throw new DecryptionError(`Invalid scrypt p: ${params.p}`)
if (!Number.isInteger(params.dklen) || params.dklen < 16)
throw new DecryptionError(`Invalid scrypt dklen: ${params.dklen}`)
}Minor
[MN-01] recoverTronRecoveryId throws UnsupportedOperationError for a signing failure
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 217–228; packages/python/src/agent_wallet/core/adapters/privy.py : Lines 228–236 |
Description
When neither recovery bit (0 or 1) produces a public key that matches the wallet address, the function throws UnsupportedOperationError. This is semantically incorrect — the operation (TRON signing) is supported; what failed is the post-signing recovery computation, which is a signing error.
Recommendation
throw new SigningError('Unable to derive recovery id for TRON signature — address mismatch')[MN-02] TypeScript PrivyClient.rpc() body has inconsistent indentation
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | packages/typescript/src/core/clients/privy.ts : Lines 60–67 |
Description
The rpc method has misaligned braces in the request body object, inconsistent with the rawSign method immediately below it.
Code
return this.request(
'POST',
`/v1/wallets/${walletId}/rpc`,
{
method, // ← not indented relative to the opening brace
params,
},
options,
)Recommendation
return this.request(
'POST',
`/v1/wallets/${walletId}/rpc`,
{
method,
params,
},
options,
)[MN-03] No environment-variable support for Privy credentials
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation / Usability |
| File | packages/typescript/src/core/providers/privy-config.ts (all); Python mirror |
Description
PrivyConfigResolver reads only from the source object passed to its constructor. Unlike other credential types that fall back to environment variables (AGENT_WALLET_PRIVATE_KEY, AGENT_WALLET_MNEMONIC, etc.), there is no support for env vars like PRIVY_APP_SECRET. This forces all users to write the app_secret into the config file, which is the root cause of finding C-02.
Recommendation
Extend PrivyConfigResolver._merge() to overlay env vars:
private merge(): PrivyConfigSource {
return {
app_id: process.env.PRIVY_APP_ID ?? normalizeValue(this.source?.app_id),
app_secret: process.env.PRIVY_APP_SECRET ?? normalizeValue(this.source?.app_secret),
wallet_id: process.env.PRIVY_WALLET_ID ?? normalizeValue(this.source?.wallet_id),
}
}Document the new env vars in CLAUDE.md / README.md.
[MN-04] AGENT_WALLET_TEST_SCRYPT_N and PYTEST_CURRENT_TEST reduce cryptographic strength at runtime
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Security |
| File | packages/typescript/src/local/kv-store.ts : Lines 38–55; packages/python/src/agent_wallet/local/kv_store.py : Lines 38–60 |
Description
The KV-store now checks process.env.VITEST (TS) or os.environ.get("PYTEST_CURRENT_TEST") (Python) at runtime and reduces scrypt N from 262 144 to 16 384 whenever those variables are present. While PYTEST_CURRENT_TEST is only set by pytest, AGENT_WALLET_TEST_SCRYPT_N is an arbitrary env var that any operator could accidentally set in a staging or production environment, silently weakening encryption of any wallets created during that session.
Recommendation
Consider moving this logic to build-time or test-fixture injection rather than runtime env-var inspection. Alternatively, print a warning to stderr whenever the reduced parameter path is taken:
import warnings
if os.environ.get("PYTEST_CURRENT_TEST"):
warnings.warn("Using reduced scrypt N (test mode) — do not use in production", stacklevel=2)[MN-05] normalizeTronTxPayload txID validation regex accepts uppercase hex but docs say lowercase
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 192–208 |
Description
normalizeTronTxPayload validates txID with /^[0-9a-fA-F]{64}$/ (case-insensitive hex) but does not normalise to lowercase before returning. The TRON signing path in PrivyAdapter.tronSignTransaction then embeds this mixed-case txID directly into the returned JSON. TRON node APIs may be case-sensitive.
Recommendation
Normalise to lowercase on acceptance:
const txId = stripHexPrefix(txIdRaw.trim()).toLowerCase()The Python _normalize_tron_payload already calls .hexdigest() (inherently lowercase) for the computed path but silently passes through the caller-supplied txID unchanged.
Suggestions
[S-01] Consider adding a walletId format guard before making API calls
File: packages/typescript/src/core/clients/privy.ts, packages/python/src/agent_wallet/core/clients/privy.py
Description: The wallet ID is passed directly into the URL path (/v1/wallets/${walletId}/rpc). A malformed wallet ID (e.g., containing / or ..) could result in hitting unintended API endpoints. A lightweight UUID-format check on walletId before use would mitigate this risk.
Suggestion: Validate the wallet ID matches /^[0-9a-f-]{36}$/i (UUID pattern) in PrivyConfigResolver.resolve().
[S-02] PrivyAdapter address/chain-type fetching could be consolidated into a single ensureLoaded() call
File: packages/typescript/src/core/adapters/privy.ts : Lines 36–42, 95–100
Description: Both getAddress() and getChainType() independently call this.client.getWallet(...) if their respective cache is cold, potentially causing two sequential API round trips during first use if both are needed (e.g., signTransaction calls getChainType, which is cold; then tronSignTransaction could call getAddress, which is also cold).
Suggestion: Introduce a private ensureLoaded() that fetches once and populates both cachedAddress and cachedChainType:
private async ensureLoaded(): Promise<void> {
if (this.cachedAddress !== null) return
const wallet = await this.client.getWallet(this.config.walletId)
this.cachedAddress = wallet.address
this.cachedChainType = wallet.chainType?.toLowerCase() ?? ''
}[S-03] Python PrivyAdapter._tron_sign_bytes uses tronpy.keys.hash_message while TypeScript uses keccak256
File: packages/python/src/agent_wallet/core/adapters/privy.py : Lines 109–113; packages/typescript/src/core/adapters/privy.ts : Lines 118–121
Description: The TypeScript adapter computes keccak256(bytes) for tronSignBytes; the Python adapter calls tronpy.keys.hash_message(data), which prepends a TRON-specific message prefix before hashing. These two implementations will produce different digests for the same input, breaking cross-language consistency tests.
Suggestion: Align both implementations. Verify which behaviour is expected by Privy for raw_sign and document it explicitly.
Positive Observations
| Area | Observation |
|---|---|
| Interface consistency | SignOptions is threaded through every signing method in both TypeScript and Python, allowing callers to attach an authorizationSignature without any breaking change to existing code. |
| Dual-language parity | Nearly every TypeScript change has a Python mirror. The structural symmetry is maintained well. |
| TRON recovery-id computation | The local recovery-id brute-force (try bit 0, then 1, compare recovered address) is a correct and clean approach for a service that returns only r||s without v. |
| Keystore V3 fix | Reading actual kdfparams from the keystore on decryption (instead of hardcoded constants) fixes a real latent bug where cross-environment keystores with different params would silently fail. |
| txID resilience | Allowing signTransaction to derive txID = SHA256(raw_data_hex) when missing makes the SDK more robust to partially-formed unsigned transactions from TronGrid. |
PrivyConfigResolver.isEnabled() |
Clean, side-effect-free check for whether Privy config is complete, preventing noisy errors during provider resolution when Privy is not configured. |
| Error type hierarchy | Four Privy-specific error classes (PrivyConfigError, PrivyRequestError, PrivyRateLimitError, PrivyAuthError) are well-scoped and exported from the public API surface. |
| Retry with back-off | The TypeScript PrivyClient correctly implements exponential back-off with a configurable sleep injectable for testing. |
| Test coverage | New test files (privy-adapter.test.ts, privy-client.test.ts, privy-config.test.ts) provide meaningful unit coverage of the new modules; the adapter tests use vi.fn() mocks properly. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 5 | 3 | 0 | C-01 (urlopen), MJ-03 (cache miss), MN-05 (txID case) |
| Security | 6 | 3 | 2 | 1 | C-02 (plaintext secret), MJ-04 (kdfparams trust) |
| Performance | 5 | 3 | 2 | 0 | MJ-01 (blocking async), MJ-02 (no timeout) |
| Code Quality | 7 | 5 | 1 | 1 | MN-01 (wrong error type), MN-02 (indentation) |
| Testing | 5 | 4 | 0 | 1 | Good coverage of new modules |
| Documentation | 4 | 2 | 1 | 1 | MN-03 (missing env var docs) |
| Compatibility | 3 | 3 | 0 | 0 | SignOptions is additive; no breaking changes |
| Observability | 3 | 2 | 0 | 1 | No sensitive data in logs |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analysed only the diff between main and feat/privy-support. Runtime behaviour (e.g. actual Privy API responses), integration testing, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-04-01
Audit Report — PR: feat/privy-supportDate: 2026-04-01 1. PR OverviewBranch Info
Commit SummaryChange Statistics
Key Files Changed
2. Change Summary2.1 Privy Adapter Layer (Core Feature)A new
2.2 Privy HTTP Client
2.3 Configuration Layer
2.4 Provider Integration
2.5 Address ResolutionA new 2.6 CLI AdditionsThe 2.7 Utility RefactorUtilities previously inline or duplicated were extracted into 3. Detailed Findings[severity-01] Privy
|
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Security |
| File | packages/python/src/agent_wallet/core/config.py : Lines 42-48; packages/typescript/src/core/config.ts : Lines 44-48 |
Description:
PrivyWalletParams is stored as-is in wallets_config.json, meaning the app_secret is written as plaintext JSON on disk. Unlike local_secure wallets where private keys are AES-128-CTR encrypted inside Keystore V3 files (the whole point of SecureKVStore), a Privy app_secret is never encrypted — it sits at the same privilege level as a raw private key in a raw_secret wallet. However, the UX implies this is a "secure" alternative when it is not: an attacker with read access to ~/.agent-wallet/wallets_config.json can immediately make Privy API calls on behalf of all wallets configured for that app_id.
The app_secret is also printed verbatim in error messages through PrivyConfigError (the missing-keys error lists the key names but the resolved config object with the secret could leak through logging).
Code:
class PrivyWalletParams(BaseModel):
"""Privy wallet params for API-backed signing."""
app_id: str
app_secret: str # <-- stored plaintext in wallets_config.json
wallet_id: strAnd save_config writes this through config.model_dump(exclude_none=True) with no secret scrubbing.
Recommendation:
Consider one of:
- Encrypt
app_secretthe same way local secrets are encrypted — store it in asecret_<ref>.jsonKeystore V3 file, store only asecret_refinwallets_config.json, and decrypt at runtime using the master password. This is the established pattern in the codebase. - Alternatively, support
app_secretvia an environment variable (e.g.,AGENT_WALLET_PRIVY_APP_SECRET) so it is never persisted to disk.
At minimum, add a prominent warning in the documentation and CLI output when storing a Privy wallet that the app_secret will be in plaintext.
[severity-02] Python PrivyClient uses synchronous urlopen in async context without a thread executor
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 57-99 |
Description:
PrivyClient._request() is a synchronous method using urllib.request.urlopen (a blocking I/O call). It is called from multiple async methods in PrivyAdapter — e.g., get_address, sign_transaction, sign_message — which run in an async event loop. Calling blocking I/O directly from an async function blocks the entire event loop thread for the duration of the HTTP call. Under any real-world async usage this will cause significant performance degradation and can cause timeouts for other coroutines.
The TypeScript equivalent uses fetch (non-blocking), making this a structural divergence. The Python adapter methods are marked async (async def get_address, async def sign_message, etc.) but delegate to the synchronous client without await asyncio.to_thread(...) or equivalent.
Code:
# PrivyAdapter.get_address() — async method calls synchronous HTTP
async def get_address(self) -> str:
if self._cached_address:
return self._cached_address
payload = self._client.get_wallet(self._wallet_id) # BLOCKS the event loop
...Recommendation:
Wrap the blocking HTTP calls in asyncio.to_thread:
import asyncio
async def get_address(self) -> str:
if self._cached_address:
return self._cached_address
payload = await asyncio.to_thread(self._client.get_wallet, self._wallet_id)
...Or convert PrivyClient to use aiohttp / httpx for async HTTP, keeping parity with the TypeScript fetch-based implementation.
[severity-03] tronSignBytes in TypeScript hashes with keccak256 but Python uses tronpy.keys.hash_message
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 114-118; packages/python/src/agent_wallet/core/adapters/privy.py : Lines 116-122 |
Description:
In PrivyAdapter the TRON message signing path diverges between languages:
- TypeScript (
tronSignBytes): applies rawkeccak256to the input bytes. - Python (
_tron_sign_bytes): callstronpy.keys.hash_message(data), which applies the "TRON personal sign" prefix (\x19TRON Signed Message:\n+ length) before hashing.
These two are not equivalent. A message signed via the TypeScript adapter will produce a different signature hash than the same message signed via the Python adapter, breaking interoperability. The compare_sign_consistency.py and compare-sign-consistency.ts example files exist precisely to detect this, but no automated test currently enforces cross-language consistency for TRON signMessage.
Code:
// TypeScript — privy.ts L114-118
private async tronSignBytes(bytes: Uint8Array, options?: SignOptions): Promise<string> {
const hashHex = keccak256(bytes) // Raw keccak256, no prefix
const hash = Buffer.from(hashHex.slice(2), 'hex')
return this.tronSignHash(hash, options)
}# Python — privy.py L116-122
async def _tron_sign_bytes(self, data: bytes, options: SignOptions | None = None) -> str:
from tronpy.keys import hash_message
digest = hash_message(data) # Applies TRON personal-sign prefix then hashes
return await self._tron_sign_hash(digest, options)Note: the TronSigner (local) also uses tronpy.keys.sign_msg in Python (which prefixes) vs keccak256 directly in TypeScript — but that inconsistency predates this PR. For the PrivyAdapter, a decision must be made and enforced consistently.
Recommendation:
Align both implementations to the same hashing strategy. Because Privy's raw_sign takes a raw hash (no additional prefix) and Tron's "personal_sign" behavior adds a prefix, decide which semantic signMessage should expose:
- If
signMessageshould match the localTronSignerbehavior, use the prefixed hash in both languages. - If
signMessageshould sign a raw digest, use rawkeccak256in both.
Add a cross-language consistency test (similar to the example scripts) that runs in CI.
[severity-04] TRON recovery ID loop silently swallows all exceptions
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 289-307 |
Description:
recoverTronRecoveryId iterates recovery ∈ {0, 1}, wraps each attempt in a broad try/catch, and silently continues on any exception. If the signature has a valid recovery=0 candidate but the public key computation fails for an unexpected reason (e.g., invalid curve point, encoding error), the function falls through to recovery=1 which might also fail — and then throws an UnsupportedOperationError. The real root cause is masked completely. This pattern can hide bugs in the cryptographic path that would otherwise be immediately apparent.
Code:
for (const recovery of [0, 1]) {
try {
const sig = secp256k1.Signature.fromCompact(signature).addRecoveryBit(recovery)
const pub = sig.recoverPublicKey(hash)
const tronAddress = tronAddressFromPublicKey(pub.toRawBytes(false))
if (tronAddress === address) {
return recovery
}
} catch {
continue // Silently swallows all errors
}
}
throw new UnsupportedOperationError('Unable to derive recovery id for TRON signature')Recommendation:
Distinguish between expected secp256k1 errors (invalid recovery bit, point at infinity) and unexpected errors (invalid input shape, encoding failures). Re-throw unexpected errors immediately:
for (const recovery of [0, 1]) {
try {
const sig = secp256k1.Signature.fromCompact(signature).addRecoveryBit(recovery)
const pub = sig.recoverPublicKey(hash)
const tronAddress = tronAddressFromPublicKey(pub.toRawBytes(false))
if (tronAddress === address) return recovery
} catch (e) {
if (e instanceof Error && e.message.includes('recovery')) continue
throw e // Unexpected errors surface immediately
}
}[severity-05] No timeout on Privy HTTP requests; unbounded blocking possible
| Property | Value |
|---|---|
| Severity | Major |
| Category | Performance / Correctness |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 57-99; packages/typescript/src/core/clients/privy.ts : Lines 88-123 |
Description:
Neither the Python (urlopen) nor the TypeScript (fetch) implementation sets an explicit timeout on individual HTTP requests to the Privy API. If the Privy API is slow or unresponsive, wallet operations block indefinitely. In an agent/automated context this could stall a pipeline forever. The retries parameter handles rate-limit retries but there is no circuit-breaker for hanging connections.
Code (Python):
with urlopen(req) as response: # No timeout parameter
...Code (TypeScript):
const response = await fetch(`${this.baseUrl}${path}`, {
method,
headers: this.buildHeaders(options),
body: body ? JSON.stringify(body) : undefined,
// No signal/AbortController
})Recommendation:
Add a configurable timeout:
- Python:
urlopen(req, timeout=self._timeout)with a default of e.g. 30 seconds. - TypeScript: Pass an
AbortControllersignal:signal: AbortSignal.timeout(this.timeoutMs ?? 30_000).
Expose the timeout as a constructor parameter with a sensible default.
[severity-06] wallets_config.json file permissions not restricted after Privy wallet write
| Property | Value |
|---|---|
| Severity | Major |
| Category | Security |
| File | packages/typescript/src/core/config.ts : Lines 105-114; packages/python/src/agent_wallet/core/config.py : Lines 99-131 |
Description:
save_config / _write_config write wallets_config.json and attempt a chmod 0o600. However, the Privy app_secret is now stored in this file. If chmod silently fails (e.g., on FAT-formatted volumes or Windows), the file may be world-readable. This issue exists for raw_secret wallets too (a pre-existing concern), but is now more severe because app_secret is a cloud credential giving programmatic access to every Privy wallet associated with that app_id — not just the one wallet configured.
Additionally, the TypeScript saveConfig wraps chmodSync in a bare catch {} with a comment // ignore on platforms without chmod support. Silently ignoring a failed permission restriction on a file containing a credential is a security concern that should at least be logged.
Recommendation:
- If
chmodfails, emit a warning to stderr rather than silently swallowing it. - In CLI output, warn users when storing Privy credentials that the
wallets_config.jsoncontains a sensitive API secret and recommend filesystem-level access controls. - Long-term: encrypt
app_secret(see finding severity-01).
[severity-07] PrivyConfigResolver does not read from environment variables, creating inconsistency
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness / Design |
| File | packages/typescript/src/core/providers/privy-config.ts : Lines 44-52; packages/python/src/agent_wallet/core/providers/privy_config.py : Lines 46-52 |
Description:
All other wallet credential sources in this SDK support environment variable fallback (e.g., AGENT_WALLET_PASSWORD, AGENT_WALLET_PRIVATE_KEY, AGENT_WALLET_MNEMONIC). PrivyConfigResolver only accepts an explicit source object. There is no way to provide AGENT_WALLET_PRIVY_APP_ID, AGENT_WALLET_PRIVY_APP_SECRET, or AGENT_WALLET_PRIVY_WALLET_ID environment variables. This creates inconsistency and prevents "config-free" usage patterns (e.g., in container environments where secrets are injected as env vars).
Recommendation:
Add environment variable fallback to PrivyConfigResolver.merge():
private merge(): PrivyConfigSource {
const source = normalizeSource(this.source)
return {
app_id: source.app_id ?? cleanEnvValue(process.env.AGENT_WALLET_PRIVY_APP_ID),
app_secret: source.app_secret ?? cleanEnvValue(process.env.AGENT_WALLET_PRIVY_APP_SECRET),
wallet_id: source.wallet_id ?? cleanEnvValue(process.env.AGENT_WALLET_PRIVY_WALLET_ID),
}
}Document the new env vars in CLAUDE.md and README.md.
[severity-08] normalizeTransactionPayload always wraps in { transaction: ... } even for flat payloads
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 171-236; packages/python/src/agent_wallet/core/adapters/privy.py : Lines 155-219 |
Description:
When payload is a flat viem-style transaction (no transaction key), hasTransaction is false, and the function returns { transaction: normalized } — correctly wrapping into Privy's expected shape. However when payload already has a transaction key, it returns { ...payload, transaction: normalized }, which preserves all original top-level keys. This means stray top-level keys (e.g., _meta, chainId at root level alongside a transaction object) survive into the Privy request. While this is unlikely to cause failures today, it leaks unintended fields to the Privy API.
Recommendation:
For the hasTransaction=true path, only pass through known top-level Privy envelope keys rather than spreading ...payload:
if (hasTransaction) {
return { method: payload.method, params: normalized } // or just { transaction: normalized }
}[severity-09] _recover_tron_v in Python silently swallows all exceptions in the loop
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 260-271 |
Description:
Same concern as finding severity-04 but for the Python implementation. The bare except Exception: continue in _recover_tron_v masks any unexpected error in the tronpy signature recovery path.
Code:
for v in (0, 1):
try:
sig = keys.Signature(signature_rs + bytes([v]))
pub = sig.recover_public_key_from_msg_hash(digest)
if pub.to_base58check_address() == address:
return v
except Exception:
continueRecommendation:
Log or re-raise unexpected exceptions:
except (ValueError, Exception) as e:
if isinstance(e, ValueError):
continue
raise[severity-10] Python __init__.py does not export PrivyAdapter, PrivyClient, or PrivyConfigError
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Quality / API |
| File | packages/python/src/agent_wallet/__init__.py : Lines 1-63 |
Description:
The TypeScript index.ts exports PrivyAdapter, PrivyClient, PrivyConfigResolver, PrivyConfigError, PrivyRequestError, PrivyRateLimitError, and PrivyAuthError. The Python __init__.py exports none of these, despite the __all__ list containing references to symbols that aren't imported (EvmSigner, LocalSigner, etc. are deferred). Users relying on from agent_wallet import PrivyAdapter will get an AttributeError since __getattr__ does not handle PrivyAdapter.
Code:
# __init__.py — __getattr__ handles LocalSigner, EvmSigner, TronSigner but NOT:
# PrivyAdapter, PrivyClient, PrivyConfigResolver, PrivyConfigError, PrivyRequestError, etc.Recommendation:
Add PrivyAdapter and the Privy error classes to __getattr__ (or directly import them at module level):
if name == "PrivyAdapter":
from agent_wallet.core.adapters.privy import PrivyAdapter
return PrivyAdapter
if name in ("PrivyConfigError", "PrivyRequestError", "PrivyRateLimitError", "PrivyAuthError"):
from agent_wallet.core.errors import ...
return ...Also update __all__ to match the TypeScript public API.
[severity-11] generatePassword in TypeScript CLI has modular bias in character selection
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Security |
| File | packages/typescript/src/delivery/cli.ts : Lines 325-354 |
Description:
The pick helper uses charset[randomBytes(1)[0] % charset.length]. For charsets whose length is not a power of 2, this produces a slight modular bias (some characters are slightly more likely). For example upper has 26 characters; randomBytes(1)[0] is in [0,255], and 256 % 26 = 22, making the first 22 characters fractionally more likely than the last 4. While this is a negligible bias for password generation (well within acceptable randomness), it is a known pattern to flag in security-sensitive code.
Recommendation:
Use rejection sampling to eliminate bias:
const pick = (charset: string, count: number): string[] => {
const result: string[] = []
while (result.length < count) {
const byte = randomBytes(1)[0]
if (byte < Math.floor(256 / charset.length) * charset.length) {
result.push(charset[byte % charset.length])
}
}
return result
}[severity-12] TRON v byte recovery in TypeScript PrivyAdapter uses v + 27 but local TronSigner also uses v + 27
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Correctness / Documentation |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 148-160 |
Description:
tronSignHash computes the final v value as (recovery + 27). This is consistent with how TronSigner.signDigest produces its v. However, Tron's native signature format actually stores v differently in various contexts — TRONWEB uses v = recovery (not +27) in some paths. The test (routes TRON signing through raw_sign and appends v) validates that the output matches what secp256k1.sign().recovery + 27 produces, which is the same convention as the local signer. This is probably correct, but the rationale for +27 vs raw recovery bit is not documented.
Recommendation:
Add a comment explaining why v + 27 is used (for compatibility with which specific Tron transaction broadcast format) and add a reference to Tron documentation or the comparison example script.
[severity-13] Python PrivyClient.retries allows _FakeResponse to simulate 429 responses without the HTTPError path; real 429 responses from urllib come as HTTPError
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Testing |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Lines 73-91; packages/python/tests/test_privy_client.py : Lines 68-88 |
Description:
In PrivyClient._request, a 429 from urlopen arrives as an HTTPError, but the test's FakeResponse returns a response object with status=429 (simulating the successful-context-manager path). The real code would receive an HTTPError on 429, but the test exercises the if status == 429 branch that is only reachable when the response does not raise HTTPError. This means the retry logic is only tested via the fake 429 response path, which diverges from production behavior where urlopen raises HTTPError for 4xx/5xx responses.
Code:
# In _request():
except HTTPError as exc:
status = exc.code
...
# then falls through to:
if status == 429:
... # This branch IS reachable (from HTTPError path)But the test patches urlopen to return a non-raising _FakeResponse(429, ...). The _FakeResponse.__enter__ returns the object successfully, so status is set from response.status — bypassing the HTTPError branch entirely.
Recommendation:
Add a test variant that raises HTTPError(code=429) to verify the retry logic works through the exception-handling path, not just the direct status-code path.
[severity-14] Missing test: TRON signTransaction via PrivyAdapter is not tested end-to-end
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Testing |
| File | packages/typescript/tests/privy-adapter.test.ts; packages/python/tests/test_privy_adapter.py |
Description:
The adapter tests cover signMessage (EVM/TRON), signTransaction (EVM), signTypedData (EVM/TRON), and signRaw (unsupported). There is no test for TRON signTransaction via PrivyAdapter.tronSignTransaction. This is a non-trivial path: it parses raw_data_hex, computes or validates a txID, signs via raw_sign, and appends the recovered v. Both the txID-provided and txID-computed branches are untested.
Recommendation:
Add tests for:
tronSignTransactionwith an explicittxIDin the payload.tronSignTransactionwithraw_data_hexonly (computedtxID).- Invalid
raw_data_hex/ invalidtxIDformat (error path).
4. Positive Observations
-
Consistent dual-language implementation. Every new TypeScript construct has a Python mirror, and vice versa. The mirrored tests (e.g.,
test_privy_adapter.pyvsprivy-adapter.test.ts) cover the same scenarios and both inject fake clients, demonstrating disciplined cross-language parity. -
Dependency injection for testability.
PrivyAdapteraccepts aPrivyClient | None(Python) or a duck-typed client object (TypeScript), andPrivyClientaccepts asleepfunction. This enables hermetic unit tests without any network calls or mocking of globals. -
Error hierarchy is well-structured. The four new Privy-specific error classes (
PrivyConfigError,PrivyRequestError,PrivyRateLimitError,PrivyAuthError) all extend the existingWalletErrorbase, preserving the caller-visible error hierarchy. Callers can distinguish auth failures (non-retryable) from rate limits (retryable) without parsing strings. -
Recovery ID computation is correct and tested. The
tronSignHash→recoverTronRecoveryIdflow is cryptographically sound (iterate over{0,1}, recover public key, compare derived Tron address). The tests construct a real secp256k1 keypair, sign a hash locally, and verify the recovered address matches, providing solid coverage. -
PrivyConfigis validated before use.PrivyConfigResolver.resolve()callsrequiredMissing()before constructing the config object, ensuring the adapter never receives empty credentials. The testtest_missing_required_fields_redacts_secretsverifies the error message does not leak theapp_secret, which is good defensive security practice. -
Retry with backoff on rate limits. The capped exponential back-off (
min(1000ms, 200*attempt)in TS;min(1.0s, 0.2*attempt)in Python) with a configurableretrieslimit is a sensible resilience pattern for a rate-limited external API. -
Wallet cache invalidation.
ConfigWalletProvider.removeWallet()evicts the wallet from the in-memory cache to prevent stale references after removal. This subtle correctness detail is easy to miss and was handled properly.
5. Checklist Results
| Dimension | Status | Notes |
|---|---|---|
| Correctness & Logic | Partial | TRON message hash divergence (TS vs Python), recovery error swallowing |
| Security | Fail | app_secret stored in plaintext; chmod failure silently ignored |
| Performance | Partial | Blocking HTTP in async Python context; no request timeout |
| Code Quality | Pass | Clean architecture; mirrors established patterns; good error types |
| Testing | Partial | TRON signTransaction untested; retry path tested via wrong code path in Python |
| Documentation | Pass | how-to-add-privy-wallet.md added; AGENTS.md added; README.md updated |
6. Review Verdict
Decision: Request Changes
Rationale
The PR delivers a well-structured Privy integration that faithfully mirrors the existing architecture. The code quality is high: the adapter follows established patterns, error handling is deliberate, and test coverage is reasonable for a new feature.
However, two issues require resolution before merging:
-
Critical security issue (severity-01): The
app_secretis a cloud credential that gives access to all wallets under the Privy application. Storing it in plaintext inwallets_config.jsonis inconsistent with the SDK's stated "secure signing" philosophy and its own encryption infrastructure. Either the secret must be encrypted at rest, or the PR must explicitly document this as a known limitation and implement an env-var alternative. -
Major correctness issue (severity-02): The Python
PrivyClientis synchronous and will block the asyncio event loop on every Privy API call. Given thatPrivyAdaptermethods are allasync, this will cause deadlocks or severe degradation in any real async caller.
Additionally, the TRON message hash divergence (severity-03) between TypeScript and Python is a functional correctness bug that would produce different signatures for the same input across languages, breaking cross-language interoperability.
Required Before Merge
- Encrypt
app_secretat rest or add explicit env-var alternative + documentation warning (severity-01) - Wrap
PrivyClient._requestinasyncio.to_threador switch to async HTTP (severity-02) - Align TRON
signMessagehash strategy between Python and TypeScript (severity-03)
Recommended Before Merge
- Add request timeout to both Python and TypeScript clients (severity-05)
- Add
PrivyAdapterand Privy error types to Python__init__.pyexports (severity-10) - Add TRON
signTransactionadapter tests (severity-14)
Code Review ReportProject: Agent-Wallet (bankofai-agent-wallet / @bankofai/agent-wallet) PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR introduces Privy API-backed wallet support across both the TypeScript and Python packages, brings Four major issues require attention before merging. The Python The overall quality of the implementation is high. The Privy adapter correctly recovers the TRON recovery bit, the payload-normalisation helpers handle both camelCase and snake_case inputs, wallet config caching has been upgraded to a proper three-level keyed map to support Change Summary1. Privy Wallet Adapter (
|
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 36, 101 |
Description:
get_address() and _get_chain_type() call self._client.get_wallet() which is a plain synchronous method implemented in PrivyClient. This is fine from a correctness standpoint because the call is not awaited (it is not a coroutine), but it means the HTTP request — which uses urllib.request.urlopen — runs synchronously and blocks the event loop when called inside an async def. In any asyncio context (including all async test runners and real agent usage), this will block the event loop for the duration of the HTTP round-trip, potentially starving other coroutines. The TypeScript client is correctly async/await, making this an asymmetry between the two implementations.
Code:
async def get_address(self) -> str:
if self._cached_address:
return self._cached_address
payload = self._client.get_wallet(self._wallet_id) # blocks event loop
...
async def _get_chain_type(self) -> str:
if self._cached_chain_type:
return self._cached_chain_type
payload = self._client.get_wallet(self._wallet_id) # blocks event loopRecommendation:
Either run the blocking call in a thread pool executor:
import asyncio
loop = asyncio.get_event_loop()
payload = await loop.run_in_executor(None, self._client.get_wallet, self._wallet_id)Or convert PrivyClient._request() and its callers to async/await using aiohttp or httpx. The same applies to sign_transaction, sign_message, sign_typed_data, and _tron_sign_hash which all call synchronous rpc() / raw_sign() methods while themselves being async def. Caching partially mitigates the get_wallet calls but does not protect the first call or any signing call.
[major-02] Test-time scrypt downgrade leaks into production via environment sentinel
| Property | Value |
|---|---|
| Severity | Major |
| Category | Security |
| File | packages/python/src/agent_wallet/local/kv_store.py : Lines 40-52, packages/typescript/src/local/kv-store.ts : Lines 43-60 |
Description:
_scrypt_params() (Python) and getScryptParams() (TypeScript) check os.environ.get("PYTEST_CURRENT_TEST") / process.env.VITEST respectively, and when present reduce N from 262144 to 16384. The intent is fast CI. The problem is that production environment discovery is now entangled with test-framework sentinels. If PYTEST_CURRENT_TEST is ever set in a staging/production shell (e.g. by a misconfigured CI agent that persists env vars, or through user error), wallets will silently be encrypted with a weaker work-factor. The mismatch would not be detected at runtime because decryption reads N from the stored keystore. Additionally, AGENT_WALLET_TEST_SCRYPT_N with a value > 1 overrides N entirely with no lower bound other than > 1, meaning N=2 is accepted.
Code:
def _scrypt_params() -> tuple[int, int, int, int]:
...
if os.environ.get("PYTEST_CURRENT_TEST"):
return TEST_SCRYPT_N, DEFAULT_SCRYPT_R, DEFAULT_SCRYPT_P, DEFAULT_SCRYPT_DKLENRecommendation:
Remove the PYTEST_CURRENT_TEST / VITEST environment checks from production code. Instead, inject scrypt params as a constructor argument to SecureKVStore / the encrypt/decrypt helpers, and configure reduced params only in tests via the constructor. This is a standard dependency-injection approach that keeps test concerns out of production paths. For AGENT_WALLET_TEST_SCRYPT_N, enforce a minimum value of at least 4096 and document it as a test-only escape hatch.
[major-03] resolveNetwork silently returns undefined, breaking the parseNetworkFamily error message chain
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/typescript/src/core/utils/network.ts : Lines 11-17, packages/python/src/agent_wallet/core/utils/network.py : Lines 20-24 |
Description:
The old resolveNetwork() helpers in config-provider.ts and env-provider.ts threw descriptive Error("network is required") when neither explicit nor default network was available. The refactored version in utils/network.ts returns undefined silently. When this undefined reaches parseNetworkFamily(undefined) the error thrown is "network is required" with no indication of which call site failed or which wallet was involved. More importantly, RawSecretSigner calls parseNetworkFamily(network) immediately in its constructor, so the error is still raised, but downstream code that relied on the old contract (e.g. places that called resolveNetwork and expected a string) may pass undefined to APIs that do not expect it.
Code:
export function resolveNetwork(
explicit: string | undefined,
providerDefault: string | undefined,
): string | undefined { // was: throws if both undefined
if (explicit) return explicit
if (providerDefault) return providerDefault
return undefined // silent; old code threw here
}Recommendation:
Maintain two variants: resolveNetwork returning string | undefined for contexts where undefined is valid (Privy wallets), and requireNetwork that throws with a contextual message for local signer paths. Update call sites in ConfigWalletProvider.getActiveWallet() and EnvWalletProvider.getActiveWallet() to use requireNetwork when constructing local signers.
[major-04] TypeScript PrivyClient.rpc() body has misaligned indentation
| Property | Value |
|---|---|
| Severity | Major |
| Category | Code Quality |
| File | packages/typescript/src/core/clients/privy.ts : Lines 64-68 |
Description:
The rpc() method's request body object literal has method and params keys indented 6 spaces instead of the project standard of 2-space nesting relative to the enclosing block. The rawSign() method directly below uses correct 8-space (4-level) indentation. This will fail prettier --check (the project has a format:check target) and is inconsistent with every other object literal in the file.
Code:
async rpc(...): Promise<PrivyRpcResponse> {
return this.request(
'POST',
`/v1/wallets/${walletId}/rpc`,
{
method, // <-- should be indented 6 spaces (2 more than opening brace)
params,
},Recommendation:
{
method,
params,
},[minor-01] TRON recovery-bit calculation uses v + 27 in TypeScript but raw v in Python
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/adapters/privy.ts : Line 158, packages/python/src/agent_wallet/core/adapters/privy.py : Line 152 |
Description:
In tronSignHash() (TypeScript) the recovery id v is adjusted by +27 before being appended to the signature: (v + 27).toString(16). In _tron_sign_hash() (Python), the raw v value (0 or 1) is appended directly: f"{v:02x}". TRON's signature format for the Privy raw_sign path uses Ethereum-style v values of 27 or 28 (legacy). The Python implementation will produce 00 or 01 as the v byte, while the TypeScript implementation produces 1b or 1c. Only one of these is correct for the Privy/TRON contract. This breaks the cross-language consistency guarantee stated in CLAUDE.md.
Code:
// TypeScript – line 158
const vHex = (v + 27).toString(16).padStart(2, '0')# Python – line 152
return (sig_hex + f"{v:02x}").lower() # missing + 27Recommendation:
Align Python with TypeScript. Change the Python return to:
return (sig_hex + f"{v + 27:02x}").lower()Add a cross-language integration test asserting both implementations produce identical signatures for the same TRON hash.
[minor-02] PrivyConfigResolver does not validate config from environment variables, only from explicit source
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/providers/privy-config.ts : Lines 16-50, packages/python/src/agent_wallet/core/providers/privy_config.py : Lines 21-57 |
Description:
PrivyConfigResolver accepts only a single source dict / mapping. There is no env-var fallback path (e.g. PRIVY_APP_ID, PRIVY_APP_SECRET, PRIVY_WALLET_ID). A user who wants to avoid storing the app_secret in wallets_config.json cannot do so. The spec document (kiro/specs/privy-adapter-providers/requirements.md) mentions "env var override" as a requirement. While omitting env-var support may be intentional for the MVP, it leaves the app_secret — a sensitive credential — necessarily stored in plaintext in the config file. This is documented as a known limitation but warrants a finding for visibility.
Recommendation:
Consider adding an env-var layer (PRIVY_APP_ID, PRIVY_APP_SECRET, PRIVY_WALLET_ID) to PrivyConfigResolver.merge() as a fallback after the explicit source, consistent with how EnvWalletProvider handles private keys. At minimum, add a warning in the CLI output when a Privy wallet is stored to config, noting the plaintext app_secret.
[minor-03] normalizeTypedDataPayload mutates the input dict in Python
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 237-242 |
Description:
_normalize_typed_data_payload(data) calls typed.pop("primaryType") and assigns primary_type. Because payload = data if "typed_data" in data else {"typed_data": data}, when data already contains "typed_data", payload is the same object as data. Mutating typed (a value inside data) changes the caller's dict, which is unexpected for a function named "normalize". The TypeScript version has the same mutation pattern on line 261-263.
Code:
def _normalize_typed_data_payload(data: dict) -> dict:
payload = data if "typed_data" in data else {"typed_data": data}
typed = payload.get("typed_data")
if isinstance(typed, dict) and "primaryType" in typed and "primary_type" not in typed:
typed["primary_type"] = typed.pop("primaryType") # mutates caller's dict
return payloadRecommendation:
Return a deep copy or build a new dict for the typed_data value rather than mutating in place:
if isinstance(typed, dict) and "primaryType" in typed and "primary_type" not in typed:
typed = {**typed, "primary_type": typed["primaryType"]}
del typed["primaryType"]
return {**payload, "typed_data": typed}[minor-04] EnvWalletProvider constructor silently ignores network=None when env has no key
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/typescript/src/core/providers/env-provider.ts : Lines 46-56 |
Description:
The refactored resolveEnvWallet() calls resolveNetwork(explicitNetwork, providerDefault) which can return undefined. That undefined is stored as network inside EnvWalletResolved and eventually passed to RawSecretSigner(params, undefined), which in turn calls parseNetworkFamily(undefined) and throws "network is required". This is correct behaviour, but the error occurs late (when createEnvAdapter is called) rather than at EnvWalletProvider construction time, making the error harder to attribute. The Python path has the same late-error pattern.
Recommendation:
Call parseNetworkFamily eagerly in resolveEnvWallet so that the "network is required" error is thrown with the env-provider context clearly visible in the stack trace.
[minor-05] _recover_tron_v in Python silently swallows all exceptions per recovery attempt
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 260-271 |
Description:
The recovery loop catches Exception broadly on each attempt. If both v=0 and v=1 fail due to an unrelated error (corrupt signature bytes, wrong hash length, tronpy library error), the loop exhausts silently and raises UnsupportedOperationError("Unable to derive recovery id"). This masks the actual root cause. The TypeScript version has the same silent-catch pattern.
Code:
for v in (0, 1):
try:
sig = keys.Signature(signature_rs + bytes([v]))
pub = sig.recover_public_key_from_msg_hash(digest)
if pub.to_base58check_address() == address:
return v
except Exception: # swallows all errors
continueRecommendation:
Accumulate exceptions and include the last error in the UnsupportedOperationError message:
last_exc: Exception | None = None
for v in (0, 1):
try:
...
except Exception as exc:
last_exc = exc
continue
raise UnsupportedOperationError(
f"Unable to derive recovery id for TRON signature: {last_exc}"
)[suggestion-01] PrivyAdapter in Python stores app_id and app_secret as constructor args but discards them
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Code Quality |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 17-29 |
Description:
The PrivyAdapter.__init__ receives app_id and app_secret only to pass them to PrivyClient() if no client is provided. After construction they are not retained on self. The TypeScript version stores a PrivyConfig object instead. The Python version's pattern is functional but means that when an external client is injected (as in all tests), the app_id / app_secret args are silently ignored. This is slightly surprising and differs from the TypeScript constructor signature.
Recommendation:
Either store app_id/app_secret on the instance for debugging/serialisation purposes, or follow the TypeScript pattern and accept a PrivyConfig dataclass directly.
[suggestion-02] PrivyConfigResolver in TypeScript does not support env-var override (asymmetry with Python)
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Docs |
| File | packages/typescript/src/core/providers/privy-config.ts |
Description:
The Python PrivyConfigResolver and TypeScript PrivyConfigResolver only accept an explicit source dict. The CLAUDE.md dual-language parity requirement means any future change to either must be mirrored. A comment should document that env-var support is intentionally deferred.
Recommendation:
Add a // NOTE: env-var override not yet supported; tracked in issue #XYZ comment above merge() in both implementations.
[suggestion-03] _to_hex_value in Python does not handle float inputs
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 222-234 |
Description:
_to_hex_value(value) has branches for str and int but not for float. The TypeScript toHexValue() explicitly handles Number.isFinite(value) and truncates. If a Python caller passes a float (e.g. gas_price: 1.5e9 from a JSON payload without strict typing), isinstance(value, int) will be False (Python float is not int), and the value will pass through unchanged, resulting in a non-hex string reaching the Privy API.
Recommendation:
Add a float branch analogous to the TypeScript implementation:
if isinstance(value, float) and value == int(value):
return hex(int(value))[suggestion-04] TypeScript PrivyClient missing user-agent header (Python has it, TypeScript does not)
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Code Quality |
| File | packages/typescript/src/core/clients/privy.ts : Lines 125-136 |
Description:
The Python PrivyClient._headers() sets "user-agent": "agent-wallet-python". The TypeScript buildHeaders() does not set a user-agent. This is a minor cosmetic asymmetry but may matter for Privy's server-side logging or rate-limit differentiation.
Recommendation:
Add 'user-agent': 'agent-wallet-typescript' to buildHeaders().
[suggestion-05] .kiro/ spec and design documents committed to the repo may not belong in version control
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Docs |
| File | .kiro/ (33 new files) |
Description:
This PR adds 33 files under .kiro/ including design documents, requirements, research notes, and task lists generated by the Kiro AI design tool. These are internal planning artifacts and not referenced by any code. Committing them adds noise to the repo history, may include sensitive design details, and may conflict with .gitignore conventions (the PR also adds .codex/ to .gitignore, suggesting tool-generated directories are being excluded). If these files are intended as living documentation they should be in doc/ with an explanation in README.md.
Recommendation:
Either add .kiro/ to .gitignore (parallel to .claude/ and .codex/ which are already excluded), or move relevant content to doc/ with a clear audience note.
Positive Observations
| Area | Observation |
|---|---|
| TRON signing correctness | recoverTronRecoveryId correctly iterates v ∈ {0, 1}, recovers the public key, derives the TRON address using keccak256 + Base58Check, and compares against the cached wallet address — a precise implementation of TRON's signature scheme. |
| Payload normalisation | normalizeTransactionPayload handles both snake_case (Privy-native) and camelCase (viem-native) inputs elegantly, converting numeric fields to hex strings in a single pass. The pass-through of unknown keys prevents silent data loss. |
| Wallet cache upgrade | Upgrading the ConfigWalletProvider cache from Map<string, Wallet> to `Map<string, Map<WalletType, Map<string |
| Test coverage | New privy-adapter.test.ts, privy-client.test.ts, test_privy_adapter.py, test_privy_client.py cover happy paths, error paths, caching, and the full TRON signing round-trip with a real key fixture. |
| KV-store read-back of params | Fixing decryptBytes / _decrypt_bytes to read scrypt parameters from the stored keystore rather than compile-time constants is a correctness improvement that aligns with EIP-55/V3 and allows future work-factor migration. |
requireInteractive guard |
The new requireInteractive / promptInput wrappers in cli.ts provide a clean, centralised mechanism for non-interactive mode detection, which was previously missing and is needed for the change-password --password flag and CI use cases. |
| Utility module extraction | Moving parseNetworkFamily, decodePrivateKey, deriveKeyFromMnemonic, and env helpers into core/utils/ reduces coupling and eliminates duplicated implementations that existed across cli.ts, raw-secret.ts, and wallet_builder.py. |
| Dual-language parity | The Python and TypeScript implementations mirror each other closely, as required by CLAUDE.md. API shapes, error class names, and signing flows are consistent between the two languages. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 4 | 4 | 0 | TRON v-recovery asymmetry (TS vs Py), blocking HTTP in async context, silent network undefined, typed-data mutation |
| Security | 5 | 3 | 2 | 0 | app_secret in plaintext config; test-time scrypt downgrade via env sentinel |
| Performance | 3 | 2 | 1 | 0 | Synchronous HTTP in async context blocks event loop |
| Code Quality | 6 | 4 | 2 | 0 | Indentation issue in PrivyClient.rpc(); user-agent header asymmetry |
| Testing | 5 | 5 | 0 | 0 | Good coverage of new adapter, client, config resolver, and CLI paths |
| Documentation | 4 | 2 | 1 | 1 | .kiro/ planning artifacts committed; no env-var override caveat documented |
| Compatibility | 3 | 3 | 0 | 0 | SignOptions is backward-compatible (optional param); WalletType enum extended safely |
| Observability | 2 | 1 | 1 | 0 | Silent exception swallowing in TRON recovery loop hides root causes |
Disclaimer
This is an automated code review. It supplements but does not replace human review.
Report generated by Code Review Skill v1.0.0
Date: 2026-04-01
Audit Report: feat/privy-supportRepository: agent-wallet 1. PR Overview
Commit History
2. Change SummaryGroup 1: Privy Wallet Adapter (Core Feature)New Group 2: Privy HTTP ClientNew Group 3: Privy Configuration Resolver
Group 4: Config Schema ExtensionBoth Group 5: Provider and Resolver Updates
Group 6: Address ResolutionNew Group 7: Utility RefactoringPython's single Group 8: CLI EnhancementsBoth CLIs gained Group 9: TestsComprehensive new test files: Group 10: DocumentationNew 3. Detailed FindingsCritical Findings[C-01] Python TRON Privy Signature v-Byte Incompatible with TypeScript
Description: The TypeScript Code: TypeScript ( const vHex = (v + 27).toString(16).padStart(2, '0')
return `${sigHex}${vHex}`Python ( return (sig_hex + f"{v:02x}").lower()
# v is 0 or 1, so appends "00" or "01"The TypeScript TronSigner's own Recommendation: Align the Python implementation to match the TypeScript convention: return (sig_hex + f"{v + 27:02x}").lower()Alternatively, if tronpy's own output uses [C-02] Python ConfigWalletProvider Cache Eviction Calls
|
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness / Runtime Error |
| File | packages/python/src/agent_wallet/core/providers/config_provider.py : Lines 99-102 |
Description: self._wallets is typed as dict[tuple[str, WalletType, str | None], Wallet] (line 46) and cache keys are tuples like (wallet_id, conf.type, resolved_network) (line 138). However, in remove_wallet, the eviction comprehension calls cache_key.startswith(...), which is a string method and will raise AttributeError: 'tuple' object has no attribute 'startswith' at runtime whenever a wallet is removed after at least one wallet has been accessed (i.e., when the cache is non-empty).
Code:
# Line 46: keys are tuples
self._wallets: dict[tuple[str, WalletType, str | None], Wallet] = {}
# Line 138: cache_key assigned as tuple
cache_key = (wallet_id, conf.type, resolved_network)
# Line 99-102: bug — tuple does not have startswith
self._wallets = {
cache_key: wallet
for cache_key, wallet in self._wallets.items()
if not cache_key.startswith(f"{wallet_id}:") # AttributeError!
}Recommendation:
self._wallets = {
cache_key: wallet
for cache_key, wallet in self._wallets.items()
if cache_key[0] != wallet_id
}[C-03] Privy app_secret Stored in Plaintext in wallets_config.json
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Security / Credential Exposure |
| File | packages/python/src/agent_wallet/core/config.py : Lines 42-48; packages/typescript/src/core/config.ts : Lines 44-48 |
Description: The PrivyWalletParams model stores app_secret directly in wallets_config.json as plaintext. The Privy App Secret is a high-value server-side credential that grants signing authority over all wallets in the application. Persisting it unencrypted in a config file means that any process with read access to ~/.agent-wallet/wallets_config.json (or the value of AGENT_WALLET_DIR) obtains full API signing access. While wallets_config.json is written with chmod 0o600, that protection is insufficient if the file is backed up, stored in a dotfile manager, or accidentally committed to version control.
The existing local_secure wallet type protects private keys via Keystore V3 encryption. No equivalent protection exists for Privy credentials.
Code:
class PrivyWalletParams(BaseModel):
app_id: str
app_secret: str # stored in plaintext in wallets_config.json
wallet_id: strRecommendation:
In the short term, document clearly in the CLI help text and how-to-add-privy-wallet.md that app_secret is stored unencrypted and advise users to use environment variables (PRIVY_APP_SECRET) instead of persisting credentials in the config file. In the medium term, consider encrypting the app_secret field using the same Keystore V3 mechanism used for private keys, or offer an env-var fallback override for app_secret. At minimum, add a warning in the CLI when persisting a Privy wallet.
Major Findings
[M-01] No HTTP Request Timeout in Python PrivyClient
| Property | Value |
|---|---|
| Severity | Major |
| Category | Performance / Reliability |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Line 74 |
Description: urlopen(req) is called without a timeout argument. A hung or slow Privy API call will block the Python process indefinitely, causing the CLI or any calling agent to hang. The TypeScript implementation also has no explicit timeout, but Node.js's fetch can be cancelled externally via AbortController; Python's urlopen has no such ambient mechanism.
Code:
with urlopen(req) as response: # no timeout — can block forever
status = response.status
payload = _read_json(response)Recommendation:
with urlopen(req, timeout=30) as response:
...Add a timeout parameter to PrivyClient.__init__ (defaulting to 30 seconds) and pass it to urlopen. Also add a timeout to the TypeScript fetch call via AbortController with signal.
[M-02] Python __init__.py Does Not Export Privy Classes
| Property | Value |
|---|---|
| Severity | Major |
| Category | API Contract / Maintainability |
| File | packages/python/src/agent_wallet/__init__.py |
Description: The TypeScript index.ts exports PrivyAdapter, PrivyClient, PrivyConfigResolver, and all Privy error classes. The Python __init__.py exports none of these. Users of the Python SDK who want to interact with Privy programmatically must know to import from internal submodule paths (agent_wallet.core.adapters.privy, etc.), which is fragile and breaks the principle of a stable public API surface. The Python package's __all__ list also does not include any Privy symbols, making the package surface appear incomplete relative to its TypeScript counterpart.
Code:
# __init__.py — Privy symbols completely absent from imports and __all__
from agent_wallet.core.errors import (
DecryptionError,
NetworkError,
SigningError,
# PrivyConfigError, PrivyRequestError, PrivyRateLimitError, PrivyAuthError — missing
...
)Recommendation: Add the following to __init__.py:
- Import and re-export
PrivyAdapter,PrivyClient,PrivyConfigResolver - Import and re-export
PrivyConfigError,PrivyRequestError,PrivyRateLimitError,PrivyAuthError - Add all new symbols to
__all__
[M-03] Tron Typed Data Hashing Inconsistency Between Python and TypeScript
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 134-136 vs packages/typescript/src/core/adapters/privy.ts : Lines 138-145 |
Description: The Python _tron_sign_typed_data uses eth_account.messages.encode_typed_data and manually constructs the digest as keccak(b"\x19" + version + header + body). The TypeScript tronSignTypedData uses viem.hashTypedData which computes the EIP-712 standard hash directly. These two approaches may produce different digests for identical inputs depending on how the EIP-712 domain separator and struct hash are handled. In particular, the Python path accesses signable.version, signable.header, and signable.body as raw bytes from SignableMessage, while viem.hashTypedData follows the full EIP-712 spec. A discrepancy here would cause Python and TypeScript Privy TRON typed-data signatures to not be verifiable by the same contract.
Code:
Python:
signable = encode_typed_data(full_message=payload)
digest = keccak(b"\x19" + signable.version + signable.header + signable.body)TypeScript:
const hashHex = hashTypedData({
domain, types: messageTypes, primaryType, message,
})Recommendation: Add a cross-language integration test that generates the EIP-712 hash for a known typed data payload and asserts it is byte-identical across Python and TypeScript. If a discrepancy is found, align both to the hashTypedData (viem) behavior which is the canonical EIP-712 implementation.
[M-04] _tron_sign_bytes in Python Uses tronpy.hash_message While TypeScript Uses keccak256
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 119-122 vs packages/typescript/src/core/adapters/privy.ts : Lines 114-117 |
Description: For TRON sign_raw and sign_message, the Python implementation calls tronpy.keys.hash_message(data) while TypeScript calls keccak256(bytes). hash_message in tronpy typically prepends a Tron-specific message prefix before hashing (similar to Ethereum's \x19Ethereum Signed Message:\n prefix). Using different hash functions will produce different digests and therefore different signatures for the same input bytes. The Python TronSigner.sign_raw uses tronpy.sign_msg directly (which applies tronpy's own hashing), while the Python Privy adapter calls hash_message explicitly — these are consistent within Python, but the TypeScript counterpart uses plain keccak256, creating a cross-language divergence.
Code:
Python:
from tronpy.keys import hash_message
digest = hash_message(data) # applies tron message prefixTypeScript:
const hashHex = keccak256(bytes) # raw keccak256 without prefixRecommendation: Verify which behavior is expected by the Privy TRON API. If Privy's raw_sign operates on an un-prefixed hash, both implementations should use raw keccak256. If Privy or TRON nodes expect a message prefix, both must apply the same prefix. Align both implementations and add a cross-language test.
[M-05] Missing Blank Line in Python resolver.py (PEP 8 Violation at Module Level)
| Property | Value |
|---|---|
| Severity | Major |
| Category | Code Quality / Lint |
| File | packages/python/src/agent_wallet/core/resolver.py : Lines 25-26 |
Description: The constant definition _DEFAULT_SECRETS_DIR and the function definition def resolve_wallet_provider are placed on consecutive lines without the required two blank lines separating top-level definitions. While not a runtime issue, this will cause the ruff linter to fail in CI, potentially blocking the PR.
Code:
_DEFAULT_SECRETS_DIR = os.path.join(Path.home(), ".agent-wallet")
def resolve_wallet_provider( # missing two blank lines before functionRecommendation:
_DEFAULT_SECRETS_DIR = os.path.join(Path.home(), ".agent-wallet")
def resolve_wallet_provider(Minor Findings
[N-01] Python PrivyClient sleep Parameter Uses callable (Not Callable[...])
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality / Type Safety |
| File | packages/python/src/agent_wallet/core/clients/privy.py : Line 22 |
Description: The type hint for the sleep parameter uses lowercase callable which is a built-in Python function, not a type. It should use collections.abc.Callable for a proper type annotation. This generates a ruff UP006 warning in modern Python.
Code:
sleep: callable | None = None, # wrong — callable is a builtin, not a typeRecommendation:
from collections.abc import Callable
sleep: Callable[[float], None] | None = None,[N-02] TS PrivyClient.rpc Response Type Cast Hides Potential Mismatches
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality / Type Safety |
| File | packages/typescript/src/core/clients/privy.ts : Lines 62-71 |
Description: rpc() calls this.request(...) (which returns Promise<unknown>) and casts the result to Promise<PrivyRpcResponse> via as Promise<PrivyRpcResponse>. This cast bypasses TypeScript type checking. If the Privy API changes its response shape, this will silently fail at runtime rather than producing a compile-time error. Similarly, rawSign uses the same pattern.
Code:
return this.request('POST', ...) as Promise<PrivyRpcResponse>Recommendation: Return unknown from request and perform explicit runtime validation before casting, or use a Zod schema to parse the response. At minimum, type the request method's return as the expected type with a runtime shape check.
[N-03] normalizeTypedDataPayload Mutates the Input Object In-Place (TS)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness / Side Effects |
| File | packages/typescript/src/core/adapters/privy.ts : Lines 260-265 |
Description: The normalizeTypedDataPayload function mutates typed_data in-place by deleting primaryType and setting primary_type on the same object reference. Callers who hold a reference to the original payload will observe the mutation.
Code:
if ('primaryType' in typed && !('primary_type' in typed)) {
typed.primary_type = typed.primaryType
delete typed.primaryType // mutates original object
}Recommendation: Clone the object before mutating:
const mutableTyped = { ...typed }
mutableTyped.primary_type = mutableTyped.primaryType
delete mutableTyped.primaryType[N-04] Python PrivyAdapter Creates a Redundant PrivyClient When One Is Provided
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | packages/python/src/agent_wallet/core/adapters/privy.py : Lines 17-29 |
Description: The PrivyAdapter.__init__ accepts an optional client parameter for dependency injection (used in tests). However, wallet_builder.py (line 43-46) constructs the adapter with explicit app_id and app_secret parameters — meaning the adapter internally creates its own PrivyClient even though wallet_builder.py could also pass a pre-built client. This is a minor violation of DRY but more importantly it means the TypeScript and Python adapters have different constructor signatures: TypeScript takes a (config, client) pair while Python takes separate (app_id, app_secret, wallet_id, client?) args.
Recommendation: Either align the Python constructor to match TypeScript's (config, client) signature, or accept the divergence as intentional and document it. The current approach is functional but inconsistent.
[N-05] Backoff Strategy Uses Very Short Delays (200ms max 1s)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Performance / Reliability |
| File | packages/typescript/src/core/clients/privy.ts : Line 143-145; packages/python/src/agent_wallet/core/clients/privy.py : Lines 133-134 |
Description: The rate-limit retry backoff maxes out at 1 second (Python: 1.0s, TypeScript: 1000ms). With only 2 retries by default, the client will exhaust retries within 1-2 seconds when hitting a Privy rate limit. Privy rate limits typically require longer backoff windows (e.g., 5-60 seconds). Under sustained rate limiting, the client will throw PrivyRateLimitError after a 1-second wait, leaving the caller with no practical recovery path.
Recommendation: Increase the maximum backoff to at least 5-10 seconds and consider adding jitter. Expose retries and maxBackoffMs as configurable parameters.
[N-06] resolveWalletAddresses Makes a Live Privy API Call to Resolve a Privy Wallet Address
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Performance |
| File | packages/typescript/src/core/address-resolution.ts : Lines 66-80; packages/python/src/agent_wallet/core/address_resolution.py : Lines 64-79 |
Description: The resolve-address CLI command calls resolvePrivyAddress which creates a new PrivyAdapter and issues a live GET /v1/wallets/{id} API call. This means every resolve-address invocation for a Privy wallet makes an external network call, which fails if offline, adds latency, and may consume API quota. There is no caching of the resolved address across CLI invocations.
Recommendation: Cache the resolved Privy wallet address in wallets_config.json after the first successful fetch, and display it immediately on subsequent resolve-address calls unless --refresh is passed. This is already done for the in-memory cachedAddress within a session but does not persist.
Suggestions
[S-01] Add Integration Test for Cross-Language TRON Signature Compatibility
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Testing |
Description: There is no automated test that verifies TRON signatures produced by the Python implementation are verifiable by the TypeScript implementation and vice-versa. Given the identified v-byte discrepancy and hash-function divergence (C-01, M-04), this gap means bugs can exist at the cross-language boundary without being caught by any existing test.
Recommendation: Add a test in CI that generates a TRON signature using both Python and TypeScript adapters for the same input and asserts the outputs match.
[S-02] Consider Env-Var Override for app_secret Without Persisting to Config
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Security / UX |
Description: Many teams prefer to inject secrets via environment variables rather than config files. Support PRIVY_APP_SECRET and PRIVY_APP_ID as env-var overrides when constructing Privy wallets, allowing users to omit app_secret from wallets_config.json entirely.
[S-03] .kiro/ Directory Should Be Excluded from Package Distribution
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Maintainability |
| File | .kiro/ |
Description: The .kiro/ directory (AI agent design rules, spec templates, steering principles) is developer tooling and should not be shipped in the published packages. It is already excluded by the Python pyproject.toml wheel configuration, but verify it is not included in npm files or sdist. It should also be added to .gitignore if it is intended to be team-local rather than shared.
[S-04] TypeScript PrivyClient Tests Pollute Global fetch Without Try/Finally
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Testing |
| File | packages/typescript/tests/privy-client.test.ts |
Description: Each test in privy-client.test.ts restores globalThis.fetch = originalFetch after the assertion, but only in the happy path. If an assertion throws before the restore line, globalThis.fetch is permanently overridden for the rest of the test suite. Use afterEach hooks or vi.spyOn with vi.restoreAllMocks() instead.
Code:
const originalFetch = globalThis.fetch
// ... test ...
// If expect() throws, this line is never reached:
globalThis.fetch = originalFetchRecommendation: Use vi.spyOn(globalThis, 'fetch').mockResolvedValue(...) and call vi.restoreAllMocks() in afterEach.
4. Positive Observations
-
Comprehensive dual-language parity. The PR implements the same feature across Python and TypeScript with very high structural fidelity. The
PrivyClient,PrivyAdapter,PrivyConfigResolver, and error types have matching designs, which makes cross-language maintenance practical. -
Excellent test coverage for new components. Each major new component (
PrivyClient,PrivyAdapter,PrivyConfigResolver) has its own dedicated test file with fake/mock clients. The tests cover success paths, error handling, rate-limit retry, address caching, and TRON-specific signing flows. -
Clean dependency injection. Both
PrivyAdapterandPrivyClientaccept injectableclientandsleepdependencies, making unit testing straightforward without any monkey-patching of module internals (Python tests patch only at theurlopenboundary; TypeScript tests usevi.fn()onglobalThis.fetch). -
Appropriate error taxonomy. The PR introduces four distinct Privy error types (
PrivyConfigError,PrivyRequestError,PrivyRateLimitError,PrivyAuthError), all extending the baseWalletError. This allows callers to catch-and-retry on rate limits specifically, a thoughtful design for AI agents. -
Payload normalization is well-structured. The
normalizeTransactionPayloadfunction handles both camelCase and snake_case field names and auto-converts integer values to hex strings, providing a clean compatibility layer between viem-style transaction objects and the Privy API format. -
Keystore V3
wallets_config.jsonpermissions. Config files continue to be written withchmod 0o600on both platforms, and the directory is set to0o700. This is a good baseline defense for local file protection.
5. Checklist Results
A. Correctness & Logic
| Check | Result | Notes |
|---|---|---|
| Logic errors / incorrect conditions | FAIL | C-01: Python/TS TRON v-byte mismatch; C-02: startswith on tuple |
| Null/undefined handling | PASS | Both cachedAddress and cachedChainType are guarded |
| Error handling and propagation | PASS | Custom error hierarchy used consistently |
| Concurrency issues | N/A | No shared mutable state across requests |
| Resource leaks | WARN | Python HTTP connections use with urlopen() so are properly closed |
| API contract breaks | WARN | Privy adapter constructor differs between Python and TypeScript |
B. Security
| Check | Result | Notes |
|---|---|---|
| Injection vulnerabilities | PASS | No user input is interpolated into queries |
| Auth/Authz | PASS | Basic auth header correctly constructed |
| Input validation | PASS | PrivyConfigResolver validates required fields |
| Hardcoded secrets | PASS | No hardcoded credentials found |
| Data exposure in logs | PASS | No logging of secrets observed |
| Credential storage | FAIL | C-03: app_secret stored as plaintext in wallets_config.json |
| New dependency risks | WARN | No new dependencies added; relies on existing tronpy, eth-account |
C. Performance
| Check | Result | Notes |
|---|---|---|
| N+1 queries | PASS | Address caching prevents repeated Privy API calls per session |
| Unbounded operations | WARN | No HTTP timeout (M-01) |
| Unnecessary computation | PASS | |
| Missing caching | WARN | Privy address not persisted across CLI sessions (N-06) |
D. Code Quality
| Check | Result | Notes |
|---|---|---|
| Naming clarity | PASS | Consistent naming across languages |
| Function complexity | PASS | No function exceeds 50 lines meaningfully |
| Code duplication | PASS | Payload normalization is well-factored |
| Dead code | PASS | No unused imports or dead code observed |
| Separation of concerns | PASS | Client, adapter, config, and provider layers are clean |
E. Testing
| Check | Result | Notes |
|---|---|---|
| New logic without tests | WARN | Cross-language TRON signing consistency untested |
| Missing edge case tests | WARN | No test for remove_wallet cache eviction (masks C-02 bug) |
| Test pollution | WARN | TS client tests restore globalThis.fetch in happy path only (S-04) |
F. Documentation & Maintainability
| Check | Result | Notes |
|---|---|---|
| Public APIs without docs | FAIL | Python __init__.py does not export Privy public API (M-02) |
| Complex logic without comments | PASS | TRON v-recovery logic has inline comments |
| Migration scripts | N/A | Config format is backward-compatible (new privy type added) |
6. Review Verdict
REQUEST CHANGES
Rationale:
Three critical issues must be resolved before merge:
-
C-01 — The TRON v-byte value is
00/01in Python and1b/1cin TypeScript, producing incompatible signatures across the two SDK implementations. This will cause real-world integration failures for any cross-language usage of TRON Privy signing. -
C-02 —
remove_wallet()in the PythonConfigWalletProvidercallsstartswithon a tuple cache key, which will raiseAttributeErrorat runtime in production when any user removes a wallet that was previously accessed. -
C-03 — The Privy
app_secretis persisted in plaintext inwallets_config.json. This is a significant security regression compared to the existing encryptedlocal_securewallet type and should at minimum be documented with a strong security warning or mitigated before shipping.
The Major findings (M-01 through M-05) should also be addressed prior to merge, particularly M-02 (broken Python public API) and M-04/M-05 (linting failures and hashing inconsistency). The Minor and Suggestion items are recommended follow-ups.
2 participants
No description provided.