Conversation
Code Review ReportProject: PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR migrates The intent is clean and the direction is correct: removing deprecated Additionally, Change SummaryGroup 1 — Agent Wallet v2.3.0 Migration (
|
| File | Change Type | Description |
|---|---|---|
src/lib/wallet.ts |
Modified | Removes shim/bridge env-var logic; delegates fully to resolveWalletProvider() |
Purpose: agent-wallet@2.3.0 now handles env-var resolution internally. The old code in initWallet() manually read TRON_* and AGENT_WALLET_* env vars, picked one, and re-wrote them into AGENT_WALLET_* before calling resolveWalletProvider. That layer is now removed in favour of the library doing it natively. The type import also changes from BaseWallet → Wallet as BaseWallet to match the new export name in 2.3.0.
Group 2 — CLI Version Sync (src/bin.ts, package.json, package-lock.json)
| File | Change Type | Description |
|---|---|---|
src/bin.ts |
Modified | Version string sourced dynamically from package.json instead of hardcoded |
package.json |
Modified | Version bumped 1.0.1 → 1.0.2; agent-wallet dep bumped ^2.2.1 → ^2.3.0 |
package-lock.json |
Modified | Lockfile updated to reflect new package versions |
Purpose: Avoids version drift between package.json and the string displayed by sun --version. Keeps package metadata consistent.
Group 3 — Documentation Cleanup (README.md)
| File | Change Type | Description |
|---|---|---|
README.md |
Modified | Removes TRON_PRIVATE_KEY / TRON_MNEMONIC / TRON_MNEMONIC_ACCOUNT_INDEX from all examples, tables, and security notes |
Purpose: Aligns user-facing docs with the env-var migration — only AGENT_WALLET_* names are referenced going forward.
Detailed Findings
Critical
[C-01] CLI Flags -k / -m / -i Silently Broken — Write to Removed Env Vars
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness / Security |
| File | src/bin.ts : Lines 61–66 (preAction hook, unchanged in this PR) |
Description
The preAction hook in bin.ts maps the -k/--private-key, -m/--mnemonic, and -i/--mnemonic-account-index CLI flags to environment variables. It writes to the old legacy names that were just deprecated:
if (rootOpts.privateKey) process.env.TRON_PRIVATE_KEY = rootOpts.privateKey
if (rootOpts.mnemonic) process.env.TRON_MNEMONIC = rootOpts.mnemonic
if (rootOpts.mnemonicAccountIndex) {
process.env.TRON_MNEMONIC_ACCOUNT_INDEX = rootOpts.mnemonicAccountIndex
}In main, the deleted bridge code in wallet.ts forwarded TRON_PRIVATE_KEY → AGENT_WALLET_PRIVATE_KEY before calling resolveWalletProvider. That bridge no longer exists. resolveWalletProvider (agent-wallet 2.3.0) reads AGENT_WALLET_PRIVATE_KEY / AGENT_WALLET_MNEMONIC, not TRON_PRIVATE_KEY / TRON_MNEMONIC.
Impact: Any user or agent that passes -k <privateKey> or -m <mnemonic> on the command line will have no wallet loaded at runtime. Because initWallet() now calls resolveWalletProvider() unconditionally without a null-guard, this likely throws an error (or returns a null/no-op wallet), causing all write commands to fail silently or with a confusing error message. The raw private key is also still written to the process environment under the wrong name — it never reaches the wallet library, yet it exists in process memory.
Code
// src/bin.ts — preAction hook (unchanged in this PR, but now broken)
if (rootOpts.privateKey) process.env.TRON_PRIVATE_KEY = rootOpts.privateKey // ← writes OLD var
if (rootOpts.mnemonic) process.env.TRON_MNEMONIC = rootOpts.mnemonic // ← writes OLD var
if (rootOpts.mnemonicAccountIndex) {
process.env.TRON_MNEMONIC_ACCOUNT_INDEX = rootOpts.mnemonicAccountIndex // ← writes OLD var
}Recommendation
Update bin.ts to write to the new AGENT_WALLET_* env vars that resolveWalletProvider reads:
if (rootOpts.privateKey) process.env.AGENT_WALLET_PRIVATE_KEY = rootOpts.privateKey
if (rootOpts.mnemonic) process.env.AGENT_WALLET_MNEMONIC = rootOpts.mnemonic
if (rootOpts.mnemonicAccountIndex) {
process.env.AGENT_WALLET_MNEMONIC_ACCOUNT_INDEX = rootOpts.mnemonicAccountIndex
}Also remove the now-dead TRON_* assignments entirely to prevent key material leaking into unused env slots.
Major
[MJ-01] initWallet() Loses Graceful "No Wallet" Early-Exit — Breaks Optional-Wallet Commands
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | src/lib/wallet.ts : Lines 14–17 |
Description
The old initWallet() checked configuredModes === 0 and returned early, leaving _wallet = null. This allowed commands that use isWalletConfigured() to detect a read-only mode and skip wallet-dependent operations gracefully.
The new initWallet() removes this guard entirely:
// NEW — always calls into resolveWalletProvider, no early exit
export async function initWallet(): Promise<void> {
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
_wallet = new AgentWalletAdapter(activeWallet)
}If no wallet env vars are set, resolveWalletProvider / getActiveWallet will either throw or return undefined. Wrapping undefined in new AgentWalletAdapter(undefined) will create a broken object that crashes on the first method call (getAddress, signTransaction, etc.), producing a confusing runtime error deep in the call stack rather than a clean "no wallet configured" message at startup.
Code
// src/lib/wallet.ts (before) — graceful early exit preserved
if (configuredModes === 0) {
_wallet = null
return
}
// src/lib/wallet.ts (after) — no guard, always proceeds
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
_wallet = new AgentWalletAdapter(activeWallet)Recommendation
Check whether resolveWalletProvider / getActiveWallet can indicate "no wallet configured" without throwing, and handle it explicitly:
export async function initWallet(): Promise<void> {
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
if (!activeWallet) {
_wallet = null
return
}
_wallet = new AgentWalletAdapter(activeWallet)
}If getActiveWallet() always throws when no wallet is configured, wrap the entire block in a try/catch and set _wallet = null on the appropriate error type.
[MJ-02] No Null/Undefined Guard on getActiveWallet() Return Value
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | src/lib/wallet.ts : Lines 15–17 |
Description
provider.getActiveWallet() is called and its result is immediately passed to new AgentWalletAdapter(activeWallet) without checking whether activeWallet is null or undefined. If the agent-wallet library returns a nullable type (common for "no active wallet" states), every method on the adapter (getAddress, signTransaction, etc.) will throw a TypeError: Cannot read properties of undefined — a cryptic error with no actionable guidance.
Code
const activeWallet = await provider.getActiveWallet()
_wallet = new AgentWalletAdapter(activeWallet) // ← no null check before wrappingRecommendation
const activeWallet = await provider.getActiveWallet()
if (activeWallet == null) {
_wallet = null
return
}
_wallet = new AgentWalletAdapter(activeWallet)Minor
[MN-01] require() Mixed With ES Module import Statements in bin.ts
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | src/bin.ts : Line 20 |
Description
The new version-reading code uses CommonJS require:
const { version } = require('../package.json') as { version: string }This is added to a file that otherwise uses only ES module import syntax. While it compiles and works because package.json sets "type": "commonjs", the mixing of require and import is inconsistent and will cause confusion if the project ever moves to ESM output, or if ts-node / a bundler is configured differently.
Recommendation
Use a proper ESM/TypeScript import instead. With "resolveJsonModule": true in tsconfig.json (verify it is enabled):
import { version } from '../package.json'This is fully type-safe, consistent with the rest of the file, and does not require a manual type cast.
[MN-02] No User-Facing Migration Notice for TRON_* → AGENT_WALLET_* Rename
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | README.md |
Description
The README silently removes all references to TRON_PRIVATE_KEY and TRON_MNEMONIC, and the wallet.ts shim that silently accepted them is deleted. Existing users who have these variable names in .env files, CI pipelines, or deployment secrets will have their wallets stop working with no guidance. There is no changelog entry, migration section, or deprecation notice explaining what changed.
Recommendation
Add a brief migration note to the README (e.g., under a ## Migrating from v1.0.1 or ## Breaking Changes section):
## Breaking Changes in v1.0.2
The legacy `TRON_PRIVATE_KEY`, `TRON_MNEMONIC`, and `TRON_MNEMONIC_ACCOUNT_INDEX`
environment variables are no longer supported. Rename them to:
| Old | New |
|-----|-----|
| `TRON_PRIVATE_KEY` | `AGENT_WALLET_PRIVATE_KEY` |
| `TRON_MNEMONIC` | `AGENT_WALLET_MNEMONIC` |
| `TRON_MNEMONIC_ACCOUNT_INDEX` | `AGENT_WALLET_MNEMONIC_ACCOUNT_INDEX` |Suggestions
[S-01] initWallet() Should Reset _wallet at the Start of Each Call
File: src/lib/wallet.ts
Description: _wallet is a module-level singleton. If initWallet() is called more than once (e.g., during testing or re-initialization), the first call sets _wallet to a valid adapter and subsequent calls overwrite it — but on any intermediate error, _wallet retains its old value. Starting with _wallet = null at the top of initWallet() would make re-initialization safe and predictable.
Suggestion:
export async function initWallet(): Promise<void> {
_wallet = null // reset before attempting re-init
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
if (activeWallet == null) return
_wallet = new AgentWalletAdapter(activeWallet)
}[S-02] Verify package-lock.json Integrity Hash Is Valid Before Merge
File: package-lock.json
Description: The @bankofai/agent-wallet integrity hash changed from sha512-397cAkV45... (v2.2.1) to sha512-Jezo0ffh... (v2.3.0). Since agent-wallet is a private/scoped package, it is worth confirming the new hash in the lockfile matches what is on the registry. If the npm install was run in a different environment or with a local tarball, the hash may not match what users will pull.
Suggestion: Run npm ci in a clean environment (e.g., CI) and confirm it succeeds without lockfile drift warnings before merging.
Positive Observations
| Area | Observation |
|---|---|
| Simplification | Removing the env-var bridge shim cuts wallet.ts from ~40 lines to ~14 lines for initWallet(), making it dramatically easier to read and maintain. |
| Version sync | Sourcing the CLI version from package.json dynamically (require('../package.json')) eliminates the class of bug where package.json and sun --version drift out of sync. |
| Doc alignment | README is thoroughly updated to match the env-var rename — the option tables, examples, and security notes are all consistent. |
| Import rename | Changing type BaseWallet to type Wallet as BaseWallet correctly reflects the new export name from agent-wallet@2.3.0, and the local alias avoids a name collision with @bankofai/sun-kit's Wallet. |
| Type safety | The require('../package.json') as { version: string } cast, while a minor style issue, is explicit and type-safe in a CommonJS TypeScript context. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 5 | 3 | 0 | CLI flags silently broken; no null guard on wallet; no unconfigured-wallet exit |
| Security | 6 | 5 | 1 | 0 | Private key still written to env var (wrong name) |
| Performance | 7 | 7 | 0 | 0 | No new performance concerns |
| Code Quality | 10 | 8 | 2 | 0 | require mixed with import; no re-init reset |
| Testing | 7 | 0 | 0 | 7 | No test changes in this PR; no new tests for new behavior |
| Documentation | 6 | 4 | 2 | 0 | No migration guide; README references updated |
| Compatibility | 5 | 2 | 3 | 0 | Breaking change for TRON_* users; CLI flags regressed; graceful degradation removed |
| Observability | 4 | 4 | 0 | 0 | Error messages updated correctly |
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/update-agent-wallet. Runtime behavior, integration testing against agent-wallet@2.3.0, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-03-21
Code Review ReportProject: PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR upgrades The change is a reasonable simplification — the env-var bookkeeping in the old The test suite is generally well-updated with good new coverage for the null-return and error-throw paths in Change Summary1. Dependency & Version Bump
Purpose: Adopt the new 2. Core Wallet Initialization Refactor (
|
| File | Change Type | Description |
|---|---|---|
src/lib/wallet.ts |
Modified | initWallet() rewritten; manual env merging + conflict detection removed; import type renamed |
Purpose: Delegate all credential resolution to resolveWalletProvider in the new agent-wallet version, removing the local duplication of that logic.
3. CLI Entry-Point Env Var Rename (src/bin.ts)
| File | Change Type | Description |
|---|---|---|
src/bin.ts |
Modified | -k/-m/-i flags now write AGENT_WALLET_* vars instead of TRON_*; version read from package.json |
Purpose: Align CLI flag-to-env mappings with the canonical env var names used by @bankofai/agent-wallet.
4. Dead Code Removal
| File | Change Type | Description |
|---|---|---|
src/lib/command.ts |
Modified | Unused BroadcastResult type removed |
src/commands/liquidity.ts |
Modified | Unused printKeyValue import removed |
Purpose: Lint cleanup.
5. Documentation Update (README.md)
| File | Change Type | Description |
|---|---|---|
README.md |
Modified | Removed all TRON_PRIVATE_KEY, TRON_MNEMONIC, TRON_MNEMONIC_ACCOUNT_INDEX references; flag table descriptions updated |
Purpose: Documentation parity with the renamed env vars.
6. Test Suite Updates
| File | Change Type | Description |
|---|---|---|
test/lib/wallet.test.ts |
Modified | Dropped TRON_* cleanup; added getActiveWalletError/getActiveWalletValue mock options; replaced old "no credentials" and "multi-mode reject" tests |
test/bin.test.ts |
Modified | Updated env var assertions to AGENT_WALLET_* |
test/lib/output.test.ts |
Modified | Updated error string fixture to AGENT_WALLET_PRIVATE_KEY |
test/lib/wallet.test.js (+ other .js / .d.ts) |
Modified | Compiled JS artifacts regenerated to match TS changes, also reformatted with single-quotes/no-semicolons |
Purpose: Keep tests green after the wallet init refactor and env var renaming.
Detailed Findings
Major
[MJ-01] Silent Error Swallowing in initWallet()
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Observability |
| File | src/lib/wallet.ts : Lines 14–27 |
Description
The new initWallet() wraps the entire wallet-provider call in a bare catch block that discards the error and sets _wallet = null. This means any failure — invalid private key format, unreadable wallet file, missing passphrase, library bug — is silently swallowed. The caller only observes isWalletConfigured() === false, which is indistinguishable from "no credentials supplied at all."
When a user does supply credentials but has a typo in AGENT_WALLET_PRIVATE_KEY, the CLI will silently skip wallet setup and then throw 'No wallet configured' at the first write command — with no indication of what went wrong or why. This is a significant regression compared to the previous behaviour, which propagated errors directly.
Code
export async function initWallet(): Promise<void> {
_wallet = null
try {
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
if (activeWallet === null || activeWallet === undefined) {
_wallet = null
return
}
_wallet = new AgentWalletAdapter(activeWallet)
} catch { // <-- error is silently discarded
_wallet = null
}
}Recommendation
Re-throw errors that are not "no wallet configured" cases, or at minimum log/surface the error message so the user knows what failed. A targeted approach:
export async function initWallet(): Promise<void> {
_wallet = null
try {
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
if (activeWallet === null || activeWallet === undefined) {
return // no credentials supplied — expected, stay unconfigured
}
_wallet = new AgentWalletAdapter(activeWallet)
} catch (err) {
// Re-throw so callers (and users) know what went wrong
throw new Error(
`Failed to initialise wallet: ${err instanceof Error ? err.message : String(err)}`,
)
}
}If the design intent truly is "always be lenient", the error should at least be forwarded to console.error or the structured outputError helper so it is not invisible to operators.
[MJ-02] Removal of Multi-Mode Conflict Detection Without Documented Replacement
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Documentation |
| File | src/lib/wallet.ts : Lines 14–27 (diff context) |
Description
The old initWallet() counted how many of {privateKey, mnemonic, walletPassword} were non-empty and threw a clear error when more than one was set:
'Set only one of TRON_PRIVATE_KEY, TRON_MNEMONIC, or AGENT_WALLET_PASSWORD.'
The new implementation removes this guard entirely and delegates conflict resolution to resolveWalletProvider. There is no documentation (in code, tests, or README) explaining which mode takes precedence when multiple AGENT_WALLET_* env vars are set simultaneously.
If resolveWalletProvider silently picks one source (e.g., private key wins over mnemonic), a user who accidentally sets both will have no warning that one of their settings is being ignored. If the library throws, the exception is swallowed by the new catch block (see MJ-01), making the user's misconfiguration completely invisible.
Recommendation
Either:
- Document explicitly (in README and in code comments) that
resolveWalletProvideris the single source of truth and state what its precedence rules are, or - Restore the conflict detection guard ahead of the
resolveWalletProvidercall so that the user gets an actionable error message.
Minor
[MN-01] Sensitive Data Written to process.env — Exposure in Process Listings
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Security |
| File | src/bin.ts : Lines 59–63 |
Description
The preAction hook copies CLI flag values (private key, mnemonic) into process.env. On Linux/macOS, environment variables of a running process are visible to any user that can read /proc/<pid>/environ (or via ps e). This was also true of the old TRON_* mapping, so the change does not introduce a new risk, but it persists an existing one.
Code
if (rootOpts.privateKey) process.env.AGENT_WALLET_PRIVATE_KEY = rootOpts.privateKey
if (rootOpts.mnemonic) process.env.AGENT_WALLET_MNEMONIC = rootOpts.mnemonicRecommendation
Consider passing credentials via a closure or scoped object rather than writing them into process.env. If the env-var route must remain (for compatibility with the library's own env reading), add a comment in the security section of the README noting this risk — the existing "prefer environment variables over command-line wallet flags" advice already partially covers it, but the risk should be acknowledged explicitly.
[MN-02] Duplicate delete Statements in Test beforeEach Cleanup
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | test/lib/wallet.test.ts : Lines 8–13 (diff), test/lib/wallet.test.js : Lines 1387–1393 |
Description
Both the TypeScript source and the compiled JS test have duplicate delete statements for AGENT_WALLET_PASSWORD and AGENT_WALLET_DIR in the beforeEach block. This is harmless but indicates a copy-paste issue.
Code
delete process.env.AGENT_WALLET_PASSWORD // line 8
delete process.env.AGENT_WALLET_DIR // line 9
delete process.env.AGENT_WALLET_PRIVATE_KEY
delete process.env.AGENT_WALLET_MNEMONIC
delete process.env.AGENT_WALLET_MNEMONIC_ACCOUNT_INDEX
delete process.env.AGENT_WALLET_PASSWORD // duplicate — line 13
delete process.env.AGENT_WALLET_DIR // duplicate — line 14Recommendation
Remove the duplicated two delete lines.
[MN-03] require() with TypeScript Type Cast for package.json Version
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | src/bin.ts : Line 21 |
Description
The dynamic version string is read using a CommonJS require() call with a TypeScript type assertion. While this works (the project uses "type": "commonjs"), it is inconsistent with the rest of the file, which uses ES-style import statements. It also bypasses TypeScript's resolveJsonModule path.
Code
const { version } = require('../package.json') as { version: string }Recommendation
Prefer an import declaration, which is type-safe and consistent with the module style used in the rest of the file:
import { version } from '../package.json'(Ensure "resolveJsonModule": true is set in tsconfig.json, which is standard for this kind of project.)
Suggestions
[S-01] Add a Test for getActiveWallet() Returning undefined
File: test/lib/wallet.test.ts
Description: initWallet() explicitly checks for both null and undefined returns from provider.getActiveWallet(), but the test suite only covers the null case. The undefined path is exercised indirectly through the error-throw test but not explicitly. Adding a dedicated getActiveWalletValue: undefined test case would make the intent clearer and guard against future refactoring.
[S-02] Add JSDoc Comment to initWallet() Describing the Try/Catch Contract
File: src/lib/wallet.ts
Description: The new initWallet() has a different contract from the old one — it no longer throws. Callers and future contributors need to know that errors are deliberately absorbed. A short JSDoc explaining "errors from resolveWalletProvider are swallowed; check isWalletConfigured() after calling" would prevent unexpected re-introduction of throw-aware try/catch wrappers in callers.
[S-03] README — Mention the TRON_NETWORK Env Var Is Still Used
File: README.md
Description: The PR correctly removes TRON_PRIVATE_KEY / TRON_MNEMONIC references from the docs. However, TRON_NETWORK is still read by the CLI (process.env.TRON_NETWORK in bin.ts) and is listed in the global-flags table. This is consistent and correct; a clarifying note that TRON_NETWORK is a network-selector variable (not a wallet credential) and intentionally kept under the TRON_ prefix would prevent user confusion about why some TRON_* vars are gone but this one remains.
Positive Observations
| Area | Observation |
|---|---|
| Simplification | Removing the manual env-var bookkeeping (copying AGENT_WALLET_* from TRON_* in initWallet) significantly reduces code surface and eliminates a potential source of drift between the two variable sets. |
| Dynamic Version | Reading the CLI version from package.json at runtime (src/bin.ts line 21) ensures the --version flag is always in sync with the package, eliminating the old hardcoded '1.0.0' that was already out of date. |
| Test Coverage for Null/Error paths | The new wallet.test.ts adds dedicated tests for null return and thrown error from getActiveWallet(). These paths were previously untested. |
| Dead Code Removal | Removing BroadcastResult from command.ts and printKeyValue from liquidity.ts is clean housekeeping. |
| Consistent Lint | Compiled JS test artifacts have been reformatted to single-quotes/no-semicolons matching the project's Prettier/ESLint config — this keeps the repo tidy and avoids noisy future diffs. |
| Backward-Compatible Docs | The README security-considerations section was correctly updated to reference only the canonical AGENT_WALLET_* names, tightening the security guidance. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 6 | 2 | 0 | Silent error swallowing (MJ-01); missing conflict guard (MJ-02) |
| Security | 10 | 9 | 1 | 0 | CLI flag-to-env exposure of private key (MN-01) |
| Performance | 7 | 7 | 0 | 0 | No issues found |
| Code Quality | 10 | 8 | 2 | 0 | require() type cast (MN-03); duplicate deletes (MN-02) |
| Testing | 7 | 6 | 1 | 0 | Missing undefined return test (S-01) |
| Documentation | 6 | 5 | 1 | 0 | No docs on new try/catch contract or conflict resolution (S-02, MJ-02) |
| Compatibility | 5 | 5 | 0 | 0 | Breaking env var renames handled cleanly; version bump correct |
| Observability | 4 | 3 | 1 | 0 | Silent error swallowing (MJ-01) removes user-visible error messages |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between the specified branches. Runtime behavior, integration testing, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-03-21
Code Review ReportProject: PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR upgrades The overall direction of the change is sound — removing the legacy variable fallback layer reduces complexity and delegates responsibility to the SDK as intended. However, two issues warrant attention before merge. First, the new Change Summary1. Core Wallet Initialization Refactor (
|
| File | Change Type | Description |
|---|---|---|
src/lib/wallet.ts |
Modified | Rewrote initWallet() to delegate unconditionally to resolveWalletProvider() inside a try/catch. Removed the multi-source env var resolution and the multiple-modes guard. Updated type import alias for BaseWallet. Refactored getTronWeb to avoid a direct cast assignment on a separate line. |
Purpose: Simplify wallet initialization by delegating all credential resolution logic to the agent-wallet SDK (v2.3.0), eliminating the hand-rolled variable merging layer.
2. CLI Entry-Point & Version Bump (src/bin.ts, package.json, package-lock.json)
| File | Change Type | Description |
|---|---|---|
src/bin.ts |
Modified | Changed -k/-m/-i preAction env writes to target AGENT_WALLET_* vars. Version is now read dynamically from package.json via require(). |
package.json |
Modified | Bumped version 1.0.1 → 1.0.2; bumped @bankofai/agent-wallet ^2.2.1 → ^2.3.0. |
package-lock.json |
Modified | Lock-file updated to reflect the new agent-wallet 2.3.0 resolution. |
Purpose: Ensure CLI flags write to the env vars that agent-wallet v2.3.0 actually reads, and keep the version string DRY.
3. Dead-Code Removal (src/lib/command.ts, src/commands/liquidity.ts)
| File | Change Type | Description |
|---|---|---|
src/lib/command.ts |
Modified | Removed the unused local BroadcastResult type. |
src/commands/liquidity.ts |
Modified | Removed the unused printKeyValue import. |
Purpose: Lint/quality clean-up.
4. Documentation Update (README.md)
| File | Change Type | Description |
|---|---|---|
README.md |
Modified | Restructured "Wallet & Runtime Configuration" sections; added explicit deprecation notice for TRON_* vars; added security note about not storing secrets in MCP config files; updated flag reference table. |
Purpose: Align documentation with the new wallet model and call out security guidance explicitly.
5. Test-Suite Refresh (all test/ files)
| File | Change Type | Description |
|---|---|---|
test/bin.test.ts / .js |
Modified | Updated env var names in setup/assertions from TRON_* → AGENT_WALLET_*. |
test/lib/wallet.test.ts / .js |
Modified | Replaced "no credentials" and "multiple modes" test cases with "provider throws" and "provider returns null" cases; added getActiveWalletError/getActiveWalletValue fixture options. |
test/lib/output.test.ts / .js |
Modified | Updated error-message fixture from TRON_PRIVATE_KEY → AGENT_WALLET_PRIVATE_KEY. |
All compiled .js / .d.ts |
Modified | Reformatted from 4-space indented double-quoted CommonJS style to 2-space single-quoted style (unified lint). |
Purpose: Align tests with the new wallet initialization behavior and apply a unified lint format.
Detailed Findings
Major
[MJ-01] Silent Error Swallowing in initWallet() Hides All Diagnostic Information
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Observability |
| File | src/lib/wallet.ts : Lines 14–27 |
Description
The new
initWallet()wraps its entire body in a barecatch {}block with no error binding and no logging. Any error thrown byresolveWalletProvider()orprovider.getActiveWallet()— including a corrupt keystore, an incorrect password, a missing wallet file, a network/IPC failure, or an internal SDK assertion — is silently discarded. The function returns normally and sets_wallet = null, causingisWalletConfigured()to returnfalse.From the user's perspective, a misconfigured keystore is indistinguishable from "no wallet provided at all". The only error they eventually see is the generic
getWallet()exception:
"No wallet configured. Set AGENT_WALLET_PRIVATE_KEY, AGENT_WALLET_MNEMONIC, or AGENT_WALLET_PASSWORD for agent-wallet."This message is actively misleading when the user has provided credentials but something went wrong during resolution. It also makes automated agent integrations harder to debug, since the root cause is irreversibly lost.
The old code avoided this by only calling
resolveWalletProviderafter confirming at least one credential was present, and propagating errors as thrown exceptions. The new approach has traded that explicit guard for implicit SDK delegation — but without at minimum logging the caught error.
Code
export async function initWallet(): Promise<void> {
_wallet = null
try {
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
if (activeWallet === null || activeWallet === undefined) {
_wallet = null
return
}
_wallet = new AgentWalletAdapter(activeWallet)
} catch { // <-- bare catch, no error binding, no logging
_wallet = null
}
}Recommendation
At minimum, log the caught error to stderr (behind a debug flag or unconditionally), so users and automated processes can distinguish "no credentials supplied" from "credentials supplied but initialization failed":
export async function initWallet(): Promise<void> {
_wallet = null
try {
const provider = resolveWalletProvider({ network: 'tron' })
const activeWallet = await provider.getActiveWallet()
if (activeWallet === null || activeWallet === undefined) {
_wallet = null
return
}
_wallet = new AgentWalletAdapter(activeWallet)
} catch (err) {
// Emit to stderr so callers can distinguish misconfiguration from
// "no wallet supplied". Do not throw — callers check isWalletConfigured().
process.stderr.write(
`[sun-cli] wallet init error: ${err instanceof Error ? err.message : String(err)}\n`,
)
_wallet = null
}
}Alternatively, if the intent is that initWallet() should never throw (to support read-only commands), consider re-throwing when an error is encountered and at least one AGENT_WALLET_* variable is set (i.e., the user clearly intended to configure a wallet but something went wrong).
[MJ-02] Breaking Change — TRON_PRIVATE_KEY / TRON_MNEMONIC Silently Ignored Without Warning
| Property | Value |
|---|---|
| Severity | Major |
| Category | Compatibility |
| File | src/lib/wallet.ts (removed lines), src/bin.ts : Lines 59–63 |
Description
Prior to this PR, the wallet initializer accepted
TRON_PRIVATE_KEY,TRON_MNEMONIC, andTRON_MNEMONIC_ACCOUNT_INDEXas aliases and internally remapped them toAGENT_WALLET_*before callingresolveWalletProvider(). This PR removes that remapping entirely.Any user or deployment that has
TRON_PRIVATE_KEYorTRON_MNEMONICset in their environment or.envfile will now silently receive an unconfigured wallet. There is no deprecation warning, no error, and no hint in the runtime output. This is especially problematic for automated/AI agent deployments where environment files are set centrally and may not be updated immediately when the package is upgraded from 1.0.1 to 1.0.2.The README documents the removal, but documentation alone is not a safe migration strategy for a published npm package.
Code
// REMOVED from src/lib/wallet.ts (old code):
const privateKey =
process.env.AGENT_WALLET_PRIVATE_KEY?.trim() || process.env.TRON_PRIVATE_KEY?.trim() || ''
const mnemonic =
process.env.AGENT_WALLET_MNEMONIC?.trim() || process.env.TRON_MNEMONIC?.trim() || ''
// REMOVED from src/bin.ts (old preAction):
if (rootOpts.privateKey) process.env.TRON_PRIVATE_KEY = rootOpts.privateKey
if (rootOpts.mnemonic) process.env.TRON_MNEMONIC = rootOpts.mnemonicRecommendation
Add a runtime deprecation warning in initWallet() (or in the preAction hook) that fires when legacy TRON_* wallet variables are present but the new AGENT_WALLET_* equivalents are absent:
function warnLegacyEnvVars(): void {
const legacyMappings: Array<[string, string]> = [
['TRON_PRIVATE_KEY', 'AGENT_WALLET_PRIVATE_KEY'],
['TRON_MNEMONIC', 'AGENT_WALLET_MNEMONIC'],
['TRON_MNEMONIC_ACCOUNT_INDEX', 'AGENT_WALLET_MNEMONIC_ACCOUNT_INDEX'],
]
for (const [legacy, replacement] of legacyMappings) {
if (process.env[legacy] && !process.env[replacement]) {
process.stderr.write(
`[sun-cli] WARNING: ${legacy} is no longer supported. ` +
`Use ${replacement} instead. See https://github.com/BofAI/sun-cli for migration.\n`,
)
}
}
}
export async function initWallet(): Promise<void> {
warnLegacyEnvVars()
// ... rest of implementation
}Minor
[MN-01] Sensitive Credentials Written to process.env via CLI Flags
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Security |
| File | src/bin.ts : Lines 59–63 |
Description
When users pass
-k <private-key>or-m <mnemonic>via the CLI, thepreActionhook writes those values intoprocess.env.AGENT_WALLET_PRIVATE_KEYandprocess.env.AGENT_WALLET_MNEMONICrespectively. Writing plaintext private keys and mnemonics intoprocess.envexposes them to any child process spawned by the CLI and to tools that inspect the process environment (e.g.,/proc/<pid>/environon Linux,ps -eon some systems, crash-dump tools).This behavior predates the PR and was present using
TRON_*vars; the PR perpetuates it under the new variable names. The README already warns about this ("Prefer environment variables over command-line wallet flags when possible, because shell history and process lists may expose secrets"), but the implementation continues the env-injection pattern.
Code
if (rootOpts.privateKey) process.env.AGENT_WALLET_PRIVATE_KEY = rootOpts.privateKey
if (rootOpts.mnemonic) process.env.AGENT_WALLET_MNEMONIC = rootOpts.mnemonic
if (rootOpts.mnemonicAccountIndex) {
process.env.AGENT_WALLET_MNEMONIC_ACCOUNT_INDEX = rootOpts.mnemonicAccountIndex
}Recommendation
Pass the credential values directly into
resolveWalletProvider()(if the SDK supports it) rather than staging them inprocess.env. If the SDK requires env vars, consider clearing the env vars immediately afterinitWallet()completes:// After initWallet() in the preAction hook or command handler: delete process.env.AGENT_WALLET_PRIVATE_KEY delete process.env.AGENT_WALLET_MNEMONICThis is a pre-existing issue and not a regression introduced by this PR; it is noted here for awareness given the security-focused documentation additions in this same PR.
[MN-02] require('../package.json') Uses CommonJS require() with a Type Assertion in an ESM-style TypeScript File
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | src/bin.ts : Line 21 |
Description
The version is now read with:
const { version } = require('../package.json') as { version: string }While this works in a CommonJS output target (
"type": "commonjs"inpackage.json), mixingimportstatements (for ESM-style imports) withrequire()calls is inconsistent and can cause confusion when the build target changes. TypeScript also supportsimport ... assert { type: 'json' }(TS 4.5+) orimport pkg from '../package.json'withresolveJsonModule: true, which are more idiomatic.Additionally, the
as { version: string }assertion provides no runtime guarantee; ifpackage.jsonis missing theversionfield, this would silently produceundefinedandprogram.version(undefined)would set the version to the string"undefined".
Recommendation
Use a typed JSON import instead if the TypeScript config permits it:
import pkg from '../package.json' // ... .version(pkg.version)Or keep
require()but add a runtime guard:const pkgJson = require('../package.json') as { version?: string } const version = pkgJson.version ?? 'unknown'
Suggestions
[S-01] Redundant Double-Assignment _wallet = null Inside the null/undefined Check
File: src/lib/wallet.ts : Lines 19–21
Description: Inside the try block, _wallet is already initialized to null at line 15, so the reassignment _wallet = null on line 20 (inside the if (activeWallet === null || activeWallet === undefined) branch) is a no-op. It adds visual noise without contributing correctness.
Suggestion: Remove the redundant assignment:
if (activeWallet === null || activeWallet === undefined) {
return // _wallet is already null from line 15
}[S-02] Consider Emitting a Deprecation Warning for TRON_* Network Variables as Well
File: README.md, src/lib/wallet.ts
Description: The deprecation story focuses only on wallet variables (TRON_PRIVATE_KEY, TRON_MNEMONIC). The TRON_GRID_API_KEY fallback (an alias for TRONGRID_API_KEY) is still checked in the context module and its deprecation is not surfaced in the README.
Suggestion: Include TRON_GRID_API_KEY in the "deprecated" section of the README or add a runtime warning in src/lib/context.ts when TRON_GRID_API_KEY is detected alongside the non-deprecated TRONGRID_API_KEY, to maintain consistency with the deprecation strategy applied to wallet vars.
Positive Observations
| Area | Observation |
|---|---|
| Architecture | Delegating credential resolution entirely to the SDK is the right long-term direction. The PR correctly identifies that the hand-rolled variable merging layer in initWallet() was a leaky abstraction. |
| Security Documentation | The new README "CRITICAL SECURITY NOTE" block explicitly warns against storing private keys in MCP configuration files — a real-world risk for AI agent deployments that is worth calling out prominently. |
| Type Hygiene | The import { type Wallet as BaseWallet } change correctly uses a type-only import, which eliminates a potential runtime footprint for a type that is only used for static checking. |
| Dead-Code Removal | Removing the unused BroadcastResult type (src/lib/command.ts) and the unused printKeyValue import (src/commands/liquidity.ts) is good hygiene. |
| Test Coverage for New Behaviors | The new getActiveWalletError and getActiveWalletValue: null fixture paths in wallet.test.ts directly exercise the two new silent-failure branches of initWallet(), which is exactly the right approach for testing error-handling code paths. |
| Dynamic Versioning | Reading the CLI version from package.json at runtime eliminates the prior risk of the hardcoded '1.0.0' string drifting out of sync with the published package version. |
| Unified Lint Style | The reformatting of all compiled test files from 4-space double-quoted style to 2-space single-quoted style improves consistency across the repository. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 6 | 1 | 1 | Error handling in initWallet() swallows all exceptions (MJ-01) |
| Security | 10 | 8 | 1 | 1 | CLI flags write secrets to process.env (MN-01, pre-existing) |
| Performance | 7 | 7 | 0 | 0 | No performance regressions identified |
| Code Quality | 10 | 9 | 0 | 1 | Minor: require() + type assertion for JSON (MN-02) |
| Testing | 7 | 6 | 0 | 1 | New branches are tested; no test for diagnostic stderr output |
| Documentation | 6 | 6 | 0 | 0 | README updated and security note added |
| Compatibility | 5 | 3 | 1 | 1 | TRON_* wallet vars removed without runtime migration warning (MJ-02) |
| Observability | 4 | 2 | 1 | 1 | No logging when wallet init silently fails (MJ-01) |
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/update-agent-wallet. Runtime behavior, integration testing against agent-wallet v2.3.0, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-03-21
2 participants
No description provided.