update agent wallet to 2.3.0

[roger-gan]

No description provided.

[github-actions]

Code Audit Report

Repository: @bankofai/mcp-server-tron
PR: main → dev/agent-wallet2.3.0
Audit Date: 2026-03-21
Auditor: Claude (claude-sonnet-4-6)

---

1. PR Overview

Branch Info

• Base branch: origin/main (version 1.1.6)
• Head branch: origin/dev/agent-wallet2.3.0 (version 1.1.7)

Commit Summary

SHA	Message
0c5405a	add changelog
85da60c	change version
4091aea	refactor codes
990c865	remove env
9ecb20c	Create audit-pr.yml
50ac2cf	update module
7e61e5e	update agent wallet to 2.3.0

Files Changed (25 total)

 .env.example                                 |  30 +-
 AGENTS.md                                    |  12 +-
 CHANGELOG.md                                 |   8 +
 README.md                                    |  59 +---
 mcp_example.json                             |   3 +-
 package-lock.json                            |  12 +-
 package.json                                 |   6 +-
 server.json                                  |  20 +-
 src/core/prompts.ts                          |  19 +-
 src/core/services/agent-wallet.ts            |  77 ++---
 src/core/tools/index.ts                      |  31 +-
 src/core/tools/types.ts                      |   1 -
 src/core/tools/wallet.ts                     |   7 +-
 tests/core/services/account-resource.test.ts |  52 +--
 tests/core/services/account.test.ts          |   5 +-
 tests/core/services/agent-wallet.test.ts     | 492 +++++++++------------------
 tests/core/services/deploycontract.test.ts   |   6 +-
 tests/core/services/governance.test.ts       |   5 +-
 tests/core/services/proposals.test.ts        |   5 +-
 tests/core/services/staking.test.ts          |  20 +-
 tests/core/services/transfer.test.ts         |  32 +-
 tests/core/tools.test.ts                     | 106 +++---
 tests/core/tools_integration.test.ts         |  60 ++--
 tests/integration_stdio.ts                   |  64 +++-
 tests/server/http-app.test.ts                |   2 +-

Statistics: 395 insertions, 739 deletions (net reduction of ~344 lines)

---

2. Change Summary

2.1 Core Architectural Change: "Register-Then-Check" Model

The primary change replaces the previous "register-only-if-configured" model with an always-register, check-at-runtime approach:

• Before (1.1.6): Write tools and wallet-dependent read tools were only registered if a wallet was detected at startup (via getActiveWalletId() !== null). A no-wallet startup meant those tools were completely hidden from MCP clients.
• After (1.1.7): All tools are registered unconditionally (except in explicit --readonly mode). Tools that need a wallet fail gracefully at execution time by catching the getActiveWallet() error and returning isError: true in the MCP response.

2.2 Dependency Upgrade: @bankofai/agent-wallet 2.2.0 → 2.3.0

The SDK's BaseWallet type was renamed to Wallet. The listWallets() return format changed from Array<{id, type, ...}> to Array<[walletId, walletConfig, isActive]>. Dual-format handling was added to listAgentWallets().

2.3 Removal of Legacy TRON_* Env Variable Mapping

The ensureEnvMapping() function that mapped TRON_PRIVATE_KEY → AGENT_WALLET_PRIVATE_KEY etc. was deleted. These environment variables are no longer supported.

2.4 Removal of requiresWallet Annotation

The custom requiresWallet annotation on tools and prompts was removed. Tools that previously used requiresWallet: true (e.g., get_wallet_address, list_wallets, select_wallet) now rely on runtime error handling.

2.5 Error Message Simplification

Error messages in agent-wallet.ts that previously named specific env vars have been replaced with generic messages (e.g., "Wallet not configured.").

2.6 Test Suite Overhaul

• agent-wallet.test.ts: Mock model restructured. Env-var-based test helpers (setStaticEnv, setAgentWalletEnv, clearAllWalletEnv) removed. Tests now control behavior through mockResolveWalletProvider directly.
• Integration tests: hasWallet hardcoded to false; tests that previously ran only when a wallet was configured now permanently skip their write-path coverage.
• tools_integration.test.ts: Wallet mocked to always reject; a new test verifies that get_wallet_address returns isError: true without a wallet.
• integration_stdio.ts: Removes dummy private key injection; now strips all wallet env vars and redirects HOME to a temp dir.

2.7 Documentation and Config Updates

• .env.example, README.md, AGENTS.md, server.json, mcp_example.json: All references to TRON_PRIVATE_KEY, TRON_MNEMONIC, AGENT_WALLET_PASSWORD wallet configuration removed; replaced with a single reference to agent-wallet file-backed configuration.
• package.json: "module" field changed from src/index.ts to build/index.js.

---

3. Detailed Findings

CRITICAL

None identified.

---

MAJOR

MAJOR-1: Permanent Loss of Write-Operation Test Coverage

File: Multiple test files — tests/core/services/account-resource.test.ts, tests/core/services/account.test.ts, tests/core/services/deploycontract.test.ts, tests/core/services/governance.test.ts, tests/core/services/proposals.test.ts, tests/core/services/staking.test.ts, tests/core/services/transfer.test.ts

Code Evidence:

// Before (conditional):
const hasWallet =
  !!process.env.TRON_PRIVATE_KEY ||
  !!process.env.TRON_MNEMONIC ||
  !!(process.env.AGENT_WALLET_DIR && process.env.AGENT_WALLET_PASSWORD);

// After (hardcoded false):
const hasWallet = false;

Severity: Major

Impact: All write-path integration tests (staking, transfer, governance, contract deployment, account resource delegation) are now permanently disabled. The tests use it.runIf(hasWallet)(...) — with hasWallet = false these tests will never run in any CI environment regardless of whether wallet credentials are provided. A future regression in any write operation (e.g., transferTRX, freezeBalanceV2, delegateResource) will not be caught by the test suite.

Recommendation: Replace const hasWallet = false; with a dynamic check against new-style env vars (e.g., !!process.env.AGENT_WALLET_PRIVATE_KEY || !!(process.env.AGENT_WALLET_DIR && process.env.AGENT_WALLET_PASSWORD)). Keep the it.runIf(hasWallet) guard so tests skip cleanly when no wallet is present but activate in CI when credentials are injected as secrets.

---

MAJOR-2: listAgentWallets Throws on Invalid Wallet ID Instead of Skipping

File: src/core/services/agent-wallet.ts, lines 120–122

Code Evidence:

if (!walletId) {
  throw new Error("Invalid wallet id returned by agent-wallet provider");
}

Severity: Major

Impact: If the agent-wallet SDK returns an entry with a missing or empty id (a plausible edge case during a partially-failed wallet import), listAgentWallets() throws an unhandled error. The list_wallets tool catches the error and returns isError: true, but the entire list is lost — the user cannot see any wallets even if only one entry is malformed. This is an availability concern.

Recommendation: Log a warning and continue to the next entry rather than throwing. Alternatively, collect and return a structured errors field alongside the valid wallet list.

---

MAJOR-3: Singleton Module State Not Reset Between Wallet Switches

File: src/core/services/agent-wallet.ts, lines 20–22, 86–87

Code Evidence:

let provider: WalletProvider | null = null;
let activeWallet: Wallet | null = null;
let activeAddress: string | null = null;

// In selectWallet():
activeWallet = wallet;
activeAddress = address;

Severity: Major

Impact: getProvider() caches the result of resolveWalletProvider() in a module-level singleton. If the SDK throws on the first call (no wallet configured), provider stays null and subsequent calls return null permanently for the lifetime of the process — even if credentials later become available (e.g., dynamically loaded). More importantly, selectWallet() updates activeWallet and activeAddress but does not update the provider singleton, so a call to getActiveWallet() from a different code path will still return the pre-switch wallet if cached. This is a subtle correctness issue in multi-wallet environments.

Recommendation: In selectWallet(), clear activeWallet and activeAddress before re-fetching (already partially done), and document that provider is intentionally frozen at first successful resolve. Consider adding a resetProvider() escape hatch for testing and future re-configuration scenarios.

---

MAJOR-4: getOwnerAddress Double-Fetches the Address

File: src/core/services/agent-wallet.ts, lines 60–69

Code Evidence:

export async function getOwnerAddress(): Promise<string> {
  if (activeAddress) return activeAddress;
  const wallet = await getActiveWallet();
  // getActiveWallet() already sets activeAddress on first call:
  //   activeAddress = await activeWallet.getAddress();  (line 53)
  const address = await wallet.getAddress();  // <-- second call
  if (address == null) {
    throw new Error("Failed to resolve active wallet address");
  }
  activeAddress = address;
  return address;
}

Severity: Major (minor performance, but also a correctness risk if getAddress() is non-idempotent or has side effects in some wallet implementations)

Impact: getActiveWallet() sets activeAddress as a side effect (line 53 in the full source). getOwnerAddress() then calls wallet.getAddress() again unconditionally. If activeAddress was already set by getActiveWallet(), the early-return guard on line 61 would have short-circuited — but on the first call, getActiveWallet() sets activeAddress internally and then getOwnerAddress() calls getAddress() a second time anyway. This is wasteful and breaks the single-call contract.

Recommendation: After getActiveWallet() returns, check if (activeAddress) return activeAddress; again (or rely on the assignment inside getActiveWallet). Alternatively, do not set activeAddress inside getActiveWallet(); let getOwnerAddress() be the sole owner of that cache variable.

---

MINOR

MINOR-1: select_wallet Description References Removed Mode

File: src/core/tools/wallet.ts, line 111

Code Evidence:

description:
  "Switch the active wallet at runtime. Use list_wallets to see available wallet IDs. Only available in Encrypted Storage mode.",

Severity: Minor

Impact: The phrase "Only available in Encrypted Storage mode" references the old nomenclature that was removed. With the new model, the tool is always registered; it fails at runtime if the provider lacks setActive. The description will mislead LLM agents about the tool's availability.

Recommendation: Update to: "Switch the active wallet at runtime. Use list_wallets to see available wallet IDs. Returns an error if the configured provider does not support multi-wallet selection."

---

MINOR-2: list_wallets Returns Empty List (No Error) When No Wallet Configured

File: src/core/services/agent-wallet.ts, line 99

Code Evidence:

export async function listAgentWallets() {
  const p = getProvider();
  if (!p) return [];  // Silent empty list

Severity: Minor

Impact: When no wallet is configured, list_wallets returns a success response containing an empty wallet list and the message "Found 0 wallet(s). Use select_wallet to switch the active wallet." This is misleading — the correct message should explain that no wallet is configured, not imply that zero wallets are available to switch between.

Recommendation: Return an isError: true response or throw an error when no provider is configured, consistent with how get_wallet_address behaves.

---

MINOR-3: Hardcoded Test Address May Fail in Future

File: tests/core/services/account-resource.test.ts, tests/core/services/staking.test.ts, tests/core/services/transfer.test.ts

Code Evidence:

const receiverAddress = "T9yD14Nj9j7xAB4dbGeiX9h8unkKHxuWwb";

Severity: Minor

Impact: These tests now use a hardcoded Nile testnet address instead of a configurable env var. If this address is ever deactivated or becomes unfunded on Nile, tests that do execute (via it.runIf(hasWallet)) would fail without an obvious fix. Note that since hasWallet = false, this is currently a latent issue.

Recommendation: Move the hardcoded address to a named constant at the top of each test file, or to a shared test fixtures file, with a comment indicating it is a known Nile faucet address. This makes future maintenance easier.

---

MINOR-4: integration_stdio.ts Creates a Temp Directory But Never Cleans It Up

File: tests/integration_stdio.ts, lines 9, 33

Code Evidence:

const noWalletHome = mkdtempSync(join(tmpdir(), "mcp-server-tron-stdio-home-"));
// ...
env.HOME = noWalletHome;
// No cleanup

Severity: Minor

Impact: Each test run leaves a temporary directory in tmpdir(). On most systems this is cleaned up periodically, but in containerized CI environments with ephemeral storage this accumulates across runs. There is no process.on('exit', ...) or finally block to remove the directory.

Recommendation: Add cleanup in the finally block using fs.rmSync(noWalletHome, { recursive: true, force: true }).

---

MINOR-5: Inconsistent Error Handling in listAgentWallets for Single-Wallet Fallback

File: src/core/services/agent-wallet.ts, lines 131–134

Code Evidence:

// Single-wallet mode
const wallet = await p.getActiveWallet();
const address = await wallet.getAddress();
return [{ id: "single", type: "single", address }];

Severity: Minor

Impact: The single-wallet fallback path does not handle getAddress() returning null (though the new getOwnerAddress does check for null). If the SDK-returned wallet's getAddress() resolves to null, this would push { id: "single", type: "single", address: null } into the result, causing subtle downstream errors.

Recommendation: Add a null check consistent with the pattern in getOwnerAddress().

---

MINOR-6: package.json "module" Field Inconsistency

File: package.json, line 3

Code Evidence:

"module": "build/index.js",
"main": "build/index.js",

Severity: Minor

Impact: The "module" field is not a standard Node.js package.json field (it is a bundler convention for ESM entry points). The package already uses "type": "module" and "main" for the build output. The previous value src/index.ts was likely being used for some local development tooling. The new value build/index.js is fine but the field itself is redundant with "main". This is a cosmetic/clarity issue.

Recommendation: Consider removing the "module" field entirely, or document why it is needed for a specific bundler or downstream consumer.

---

SUGGESTIONS

SUGGESTION-1: getActiveWalletId Silently Returns null for Single-Wallet Providers

File: src/core/services/agent-wallet.ts, lines 140–148

Code Evidence:

export function getActiveWalletId(): string | null {
  const p = getProvider();
  if (!p) return null;

  if (typeof (p as any).getActiveId === "function") {
    return (p as any).getActiveId();
  }
  return null;  // Single-wallet providers without getActiveId always return null
}

Severity: Suggestion

Context: The previous code returned "default" for single-wallet providers. Now it returns null. The list_wallets tool response includes activeWalletId: null for single-wallet setups, which is less informative. The get_wallet_address tool also has walletId: undefined (via walletId ?? undefined).

Recommendation: Consider returning a sentinel value like "current" or the wallet address itself for single-wallet providers that lack getActiveId, to provide users and LLMs with a non-null active wallet identifier.

---

SUGGESTION-2: signTransactionRaw is Functionally Identical to signTransaction

File: src/core/services/agent-wallet.ts, lines 166–173

Code Evidence:

export async function signTransactionRaw(
  unsignedTx: Record<string, unknown>,
  _network = "mainnet",
): Promise<any> {
  const wallet = await getActiveWallet();
  const signedJson = await wallet.signTransaction(unsignedTx);
  return JSON.parse(signedJson);
}

Severity: Suggestion

Context: Both signTransaction and signTransactionRaw now call wallet.signTransaction(unsignedTx) — they are identical. The _network parameter is unused (prefixed with _ to suppress the lint warning). This duplication existed before this PR but remains unchanged.

Recommendation: Either unify into a single function and update call sites, or document why the distinction exists (e.g., reserved for future network-specific signing behavior).

---

SUGGESTION-3: Tests Lack Coverage for listAgentWallets with Array-Format Entries

File: tests/core/services/agent-wallet.test.ts

Code Evidence: The new dual-format logic in listAgentWallets (handling both Array<[walletId, walletConfig, isActive]> for SDK 2.3+ and Array<{id,type,...}> for 2.2) is only tested with the object format. There is no test case for the new tuple/array format from SDK 2.3+.

Recommendation: Add a test where mockListWallets resolves with [["wallet-1", { type: "tron_local" }, true]] (array format) to exercise the Array.isArray(w) branch.

---

SUGGESTION-4: integration_stdio.ts — walletRes.result?.isError Does Not Match MCP Error Format

File: tests/integration_stdio.ts, line 141

Code Evidence:

if (!walletRes.result?.isError) {
  throw new Error("Expected get_wallet_address to fail without a wallet.");
}

Severity: Suggestion

Context: The MCP SDK returns tool errors as { result: { isError: true, content: [...] } } when the handler returns { isError: true, ... }. This check works today but depends on the MCP SDK propagating the isError field as-is. If the SDK version is bumped and changes how it encodes tool errors, this check could silently stop working.

Recommendation: Also assert on walletRes.result.content[0].text containing the expected error message for a more robust assertion.

---

4. Positive Observations

1. Clean removal of env-var mapping technical debt: The deletion of ensureEnvMapping() removes an implicit, order-dependent mutation of process.env that was a maintenance hazard and a source of surprising test interactions. The new code is straightforward.

2. Consistent error surfacing at the tool handler level: All wallet tools now have try/catch blocks that return { isError: true, content: [...] } rather than throwing, which is the correct MCP pattern. LLM agents receive a structured error instead of a protocol-level fault.

3. Improved test isolation: The agent-wallet.test.ts overhaul replaces brittle env-var mutation helpers (process.env = { ...ORIGINAL_ENV }) with vi.unstubAllEnvs() and direct mock control via mockResolveWalletProvider. This is a clear improvement in test reliability.

4. null address guard in getOwnerAddress: The new null check for wallet.getAddress() resolving to null is a defensive improvement that prevents silent propagation of undefined addresses into transaction flows.

5. integration_stdio.ts cleanly isolates the server: Stripping all wallet env vars and overriding HOME to a fresh temp directory ensures the server under test cannot accidentally pick up the developer's real wallet configuration, making this test genuinely reproducible.

6. Backward-compatible dual-format parsing in listAgentWallets: The SDK format change (tuple vs. object) is handled gracefully with a comment explaining the version split, which will help future maintainers understand the intent.

7. Version bump is consistent: package.json, package-lock.json, server.json, http-app.test.ts, and CHANGELOG.md all reflect 1.1.7 consistently.

---

5. Checklist Results

Category	Status	Notes
Correctness & Logic	Needs Attention	Double getAddress() call (MAJOR-4); singleton state edge case (MAJOR-3)
Security	Pass	No hardcoded secrets; private keys still never in tool arguments; legacy plaintext-key env vars removed
Performance	Minor Issue	Redundant getAddress() call (MAJOR-4); no N+1 or unbounded operations introduced
Code Quality	Needs Attention	Dead duplicate function signTransactionRaw (SUGGESTION-2); stale tool description (MINOR-1); empty-list ambiguity (MINOR-2)
Testing	Needs Attention	Write-path tests permanently disabled (MAJOR-1); missing test for new tuple format (SUGGESTION-3); temp directory not cleaned (MINOR-4)
Documentation & Maintainability	Pass	Docs accurately reflect new model; CHANGELOG updated; inline comments explain SDK version differences
Dependency Risks	Pass	Version bump from 2.2.0 → 2.3.0 is a minor release; dual-format handling mitigates format change risk

---

6. Review Verdict

Verdict: Request Changes

Rationale

The core architectural change — registering all tools unconditionally and checking wallet availability at runtime — is sound and makes the server more useful to MCP clients that cannot inspect server-side state before listing tools. The removal of the legacy TRON_* env var mapping is appropriate given the SDK's new file-backed configuration model.

However, two issues warrant fixes before merge:

1. MAJOR-1 (hardcoded hasWallet = false) is the most impactful: all write-operation integration tests are now permanently dead code. The test guards existed specifically to protect against regressions in the most sensitive user-facing operations (fund transfers, staking, governance). Replacing false with a live env-var check preserves the guard semantics while enabling CI coverage when credentials are available.

2. MAJOR-4 (double getAddress() call) is a correctness issue that should be fixed before the module-level singleton pattern causes confusion in future debugging sessions.

MAJOR-2 and MAJOR-3 are worth noting but are lower priority — they address edge cases (malformed wallet provider responses, re-configuration after startup) that are unlikely to occur in normal operation with agent-wallet 2.3.0.

The MINOR and SUGGESTION items can be addressed in a follow-up PR without blocking merge after the two required fixes above are in place.

[github-actions]

Code Review Audit Report

Project: @bankofai/mcp-server-tron
Date: 2026-03-21
Reviewer: Automated Code Review (Claude Sonnet 4.6)
Review Type: Pull Request Diff Audit

---

1. PR Overview

Field	Details
Source Branch	dev/agent-wallet2.3.0
Target Branch	main
Version Bump	1.1.6 → 1.1.7
Total Commits	8
Files Changed	25
Lines Added	~400
Lines Removed	~740
Net Change	−340 lines (significant simplification)

Commit History

ba460af fix audit
0c5405a add changelog
85da60c change version
4091aea refactor codes
990c865 remove env
9ecb20c Create audit-pr.yml
50ac2cf update module
7e61e5e update agent wallet to 2.3.0

Files Changed (Grouped)

Category	Files
Core source	src/core/services/agent-wallet.ts, src/core/tools/index.ts, src/core/tools/types.ts, src/core/tools/wallet.ts, src/core/prompts.ts
Package metadata	package.json, package-lock.json, server.json
Configuration / docs	.env.example, README.md, AGENTS.md, CHANGELOG.md, mcp_example.json
Tests (unit)	tests/core/services/agent-wallet.test.ts, tests/core/tools.test.ts
Tests (integration)	tests/core/tools_integration.test.ts, tests/integration_stdio.ts
Tests (service)	tests/core/services/account-resource.test.ts, tests/core/services/account.test.ts, tests/core/services/deploycontract.test.ts, tests/core/services/governance.test.ts, tests/core/services/proposals.test.ts, tests/core/services/staking.test.ts, tests/core/services/transfer.test.ts
Server / ops	tests/server/http-app.test.ts, start.sh (modified, content in diff context)

---

2. Change Summary

2.1 Dependency Upgrade: @bankofai/agent-wallet 2.2.0 → 2.3.0

The core motivation of this PR. The SDK's BaseWallet type is replaced by the renamed Wallet type. The resolveWalletProvider call gains a new multi-format listWallets() return shape: in 2.3+ each element is a tuple [id, config, isActive] instead of an object {id, type, …}. The integration layer now handles both shapes with a polymorphic loop.

2.2 Architectural Change: Eager Tool Registration ("register all, fail at runtime")

Previously, write-capable tools and wallet-dependent read tools were not registered when no wallet was configured (getActiveWalletId() === null). This PR reverses that decision: all tools are now registered regardless of wallet availability. A wallet check is deferred to the handler at invocation time, and handlers already wrapped their wallet calls in try/catch that surfaces isError: true to the MCP client.

The requiresWallet annotation field is removed from RegisterToolFn, types.ts, and all call sites. The ensureEnvMapping() function (which translated TRON_* env vars into AGENT_WALLET_* equivalents) is deleted entirely, dropping the backward-compatibility bridge for legacy environment variables.

2.3 Environment Variable Deprecation

TRON_PRIVATE_KEY, TRON_MNEMONIC, TRON_ACCOUNT_INDEX, and TRON_MNEMONIC_ACCOUNT_INDEX are no longer read or mapped. Documentation, .env.example, mcp_example.json, server.json, and AGENTS.md all remove references to these variables. Users must now configure wallets exclusively through the agent-wallet file-backed mechanism.

2.4 getActiveWalletId() Behavior Change

In the previous version, when no multi-wallet getActiveId() method was present (i.e., static/env mode), the function returned "default". It now returns null in that situation, aligning the return value with "no configured wallet ID" semantics.

2.5 listAgentWallets() Behavior Change

The fallback path (when listWallets is not on the provider) previously returned [{ id: "default", type: "static", address }]. It now returns [{ id: "single", type: "single", address }] or [] when the address is null.

2.6 Test Modernization

• agent-wallet.test.ts is substantially rewritten to be provider-mock-centric rather than environment-variable-centric. Env helpers (setStaticEnv, setAgentWalletEnv, clearAllWalletEnv) are removed and replaced with createProvider() / createMockWallet() factories.
• Multiple integration tests now hard-code const hasWallet = false — tests that were conditionally wallet-dependent are permanently skipped.
• tools_integration.test.ts mocks getOwnerAddress to throw "Wallet not configured." and validates that get_wallet_address surfaces isError: true.
• integration_stdio.ts removes the dummy-key injection pattern and instead validates that write tools are registered but get_wallet_address fails gracefully without a wallet.

2.7 package.json module Field Change

"module": "src/index.ts" → "module": "build/index.js". This points the bundler/module field to the compiled output rather than the TypeScript source.

---

3. Detailed Findings

3.1 CRITICAL

FINDING-C1: Permanent Removal of Backward-Compatibility for Legacy Env Vars (Breaking Change)

• File: src/core/services/agent-wallet.ts
• Lines Removed: ~30–52 (the ensureEnvMapping() function and its call)
• Code Evidence:

  // REMOVED: ensureEnvMapping() which mapped:
  //   TRON_PRIVATE_KEY  -> AGENT_WALLET_PRIVATE_KEY
  //   TRON_MNEMONIC     -> AGENT_WALLET_MNEMONIC
  //   TRON_MNEMONIC_ACCOUNT_INDEX -> AGENT_WALLET_MNEMONIC_ACCOUNT_INDEX

• Severity: Critical
• Category: Breaking Change / Backward Compatibility
• Description: Any existing deployment that relies on TRON_PRIVATE_KEY or TRON_MNEMONIC environment variables will silently lose wallet access after upgrading to 1.1.7. Write tools will be registered (they are no longer hidden) but every handler invocation will throw "Wallet not configured." at runtime. Users will see tool failures with no obvious explanation unless they consult release notes. This is a silent operational breakage for all existing static-key deployments.
• Recommendation: The removal is intentional, but the migration path must be clearly communicated. Consider adding a startup warning log that detects the presence of TRON_PRIVATE_KEY / TRON_MNEMONIC and emits an actionable message directing users to the agent-wallet migration guide. Alternatively, gate the removal behind a major version bump (1.x → 2.0) to comply with semantic versioning expectations.

---

3.2 MAJOR

FINDING-M1: Write Tools Now Always Visible — Discovery Misleads LLM Agents Without a Wallet

• File: src/core/tools/index.ts, lines 60–65
• Code Evidence:

  const isReadOnly = annotations.readOnlyHint === true;
  // No wallet check; tool is registered unconditionally (unless readOnly mode)
  if (options.readOnly && !isReadOnly) { return; }
  server.registerTool(name, definition as any, handler as any);

• Severity: Major
• Category: UX / Correctness
• Description: In the previous design, write tools were hidden from the MCP tool listing when no wallet was available, providing clean capability discovery. In the new design, an LLM agent or human user calling tools/list will see transfer_trx, write_contract, etc. regardless of wallet configuration. They will attempt to invoke these tools and receive runtime errors. The MCP specification encourages tools/list to reflect actual capability; returning tools that will always fail when the server has no wallet is a correctness concern for agent workflows.
• Recommendation: If the eager-registration pattern is retained, update tool descriptions with a clear "(requires wallet configuration)" notice, and ensure all write-capable handlers surface a standardized, parseable error (e.g., error code or structured JSON) rather than a plain text string, so LLM agents can detect "wallet not available" programmatically and skip further attempts.

FINDING-M2: getOwnerAddress() Has a Redundant Null Check That Can Never Be Reached

• File: src/core/services/agent-wallet.ts, lines 60–67
• Code Evidence:

  export async function getOwnerAddress(): Promise<string> {
    if (activeAddress) return activeAddress;
    await getActiveWallet();          // sets activeAddress internally
    if (activeAddress == null) {      // dead code: getActiveWallet() throws before returning null
      throw new Error("Failed to resolve active wallet address");
    }
    return activeAddress;
  }

• Severity: Major
• Category: Logic / Correctness
• Description: getActiveWallet() sets activeAddress = await activeWallet.getAddress() on line 53 and only returns when that succeeds. If getAddress() throws, getActiveWallet() propagates the exception and never returns. Therefore the if (activeAddress == null) guard after await getActiveWallet() is dead code. The dead branch introduces a misleading error message ("Failed to resolve active wallet address") that cannot actually be triggered, which could confuse future maintainers troubleshooting wallet issues.
• Recommendation: Remove the dead null check and simplify:

  export async function getOwnerAddress(): Promise<string> {
    if (activeAddress) return activeAddress;
    await getActiveWallet(); // throws if unavailable; sets activeAddress
    return activeAddress!;
  }

FINDING-M3: select_wallet Description Still References "Encrypted Storage Mode" — Stale Documentation in Code

• File: src/core/tools/wallet.ts, line 113
• Code Evidence:

  description:
    "Switch the active wallet at runtime. Use list_wallets to see available wallet IDs. Only available in Encrypted Storage mode.",

• Severity: Major
• Category: Documentation / Correctness
• Description: The PR removes the concept of "Encrypted Storage mode" vs "Static mode" from user-facing language everywhere except this tool description. An LLM or user reading the description will receive inaccurate guidance: the capability is now gated on whether the provider exposes a setActive() method, not on a specific wallet mode.
• Recommendation: Update the description to: "Switch the active wallet at runtime. Use list_wallets to see available wallet IDs. Only supported when the agent-wallet provider manages multiple wallets."

FINDING-M4: Integration Tests Permanently Hardcode hasWallet = false — No CI Path for Wallet-Backed Tests

• File: tests/core/services/account-resource.test.ts (line 10), tests/core/services/account.test.ts (line 14), tests/core/services/deploycontract.test.ts (line 29), tests/core/services/governance.test.ts (line 14), tests/core/services/proposals.test.ts (line 11), tests/core/services/staking.test.ts (line 12), tests/core/services/transfer.test.ts (line 12)
• Code Evidence:

  const hasWallet = false; // hardcoded; was: !!process.env.TRON_PRIVATE_KEY || ...

• Severity: Major
• Category: Testing
• Description: Seven integration test files now permanently skip all wallet-dependent test cases. While this was previously conditional on environment variables (allowing CI runners with secrets to exercise live paths), this change makes those test branches permanently dead. There is no documented mechanism to re-enable them. The skipped tests cover critical operations: TRX transfer, TRC20 transfer, contract deployment, staking, resource delegation, governance, and proposals.
• Recommendation: Replace const hasWallet = false with a configurable check (e.g., const hasWallet = !!process.env.AGENT_WALLET_TEST_ENABLED) that CI can opt in to with appropriate secrets, allowing write-path coverage on demand without requiring all developers to configure wallets.

FINDING-M5: listAgentWallets() — API Shape Compatibility Heuristic Is Fragile

• File: src/core/services/agent-wallet.ts, lines 106–128
• Code Evidence:

  for (const w of wallets) {
    if (Array.isArray(w)) {
      walletId = w[0] as string;
      walletType = ((w[1] as { type?: string })?.type ?? "unknown") as string;
    } else {
      walletId = (w as { id?: string }).id ?? "";
      walletType = ((w as { type?: string }).type ?? "unknown") as string;
    }

• Severity: Major
• Category: Correctness / Robustness
• Description: The dual-shape parser (tuple vs. object) was introduced to support both agent-wallet@2.2 (object) and @2.3+ (tuple) return shapes from listWallets(). However, the code comment at line 102 acknowledges this explicitly. The issue is that the package.json dependency is "^2.3.0", meaning any consumer running this package will always have 2.3+. The legacy object-shape branch is only useful for tests that mock the old format. In production, the non-array branch can still be triggered if the SDK is downgraded or resolves to a 2.3.x build that diverges. This creates a long-term maintenance obligation with no clear removal criteria.
• Recommendation: Document a deprecation timeline for the object-shape fallback. Add a comment indicating when it can be removed (e.g., "Remove after agent-wallet@3.0.0 is baseline"). Alternatively, add an explicit SDK version check at startup and fail fast if the SDK is below 2.3.

---

3.3 MINOR

FINDING-m1: package.json Version Mismatch Between package.json and package-lock.json Source-of-Truth

• File: package.json, line 6; package-lock.json, lines 4 and 14
• Code Evidence:

  // package.json
  "version": "1.1.7"
  // package-lock.json
  "version": "1.1.7"  // consistent — no mismatch currently

  However, the diff also shows package.json previously had "version": "1.1.6" while package-lock.json had "version": "1.1.5", indicating these were already out of sync on main. Both are updated to 1.1.7 in this PR, which resolves the existing mismatch, but the root cause (updates to package.json not triggering lock file regeneration) should be addressed by process.
• Severity: Minor
• Category: Code Quality / Process
• Recommendation: Enforce lock file regeneration as part of the version bump process. Add a CI step that fails if package.json version differs from package-lock.json.

FINDING-m2: src/index.ts Listed as Changed in Git Status But Not in the PR Diff

• File: src/index.ts
• Severity: Minor
• Category: Code Quality
• Description: The git working tree status shows src/index.ts as modified, but it does not appear in the git diff main...dev/agent-wallet2.3.0 output. This suggests the file may have uncommitted or unstaged local modifications in the working copy that were not included in the PR commits. The diff from remotes/origin/main to remotes/origin/dev/agent-wallet2.3.0 is canonical, so this is a local state issue rather than a PR defect, but it warrants attention to ensure no accidental changes are left uncommitted.
• Recommendation: Run git status and git diff on the source branch before merge to confirm no unintended changes are present in tracked files.

FINDING-m3: bin/cli.js Listed as Modified But Not Reflected in Diff

• File: bin/cli.js
• Severity: Minor
• Category: Code Quality / Process
• Description: Same observation as src/index.ts above — listed as modified in working tree but not in the inter-branch diff. The current content of bin/cli.js references ../build/index.js, which aligns with the package.json module field change, suggesting the file may already be in the correct state but not tracked cleanly.
• Recommendation: Confirm the file is committed on the source branch and matches the remote.

FINDING-m4: Error Messages Stripped of Actionable Guidance

• File: src/core/services/agent-wallet.ts, lines 49, 75, 505
• Code Evidence:

  // Before:
  throw new Error("Wallet not configured. Please set AGENT_WALLET_PASSWORD, TRON_PRIVATE_KEY, TRON_MNEMONIC, or TRON_MNEMONIC_ACCOUNT_INDEX.");
  // After:
  throw new Error("Wallet not configured.");
  
  // Before:
  throw new Error("select_wallet is not available. Ensure AGENT_WALLET_PASSWORD is configured for encrypted storage mode.");
  // After:
  throw new Error("select_wallet is not available.");

• Severity: Minor
• Category: UX / Diagnostics
• Description: The PR reduces error messages to bare statements without guidance. Given that the wallet configuration mechanism changed significantly (removal of static key support), users seeing "Wallet not configured." in an MCP tool response have no in-band hint about what to do next. This is particularly impactful for LLM agents that parse error messages to determine next steps.
• Recommendation: Restore actionable guidance: "Wallet not configured. Follow the agent-wallet setup guide at https://github.com/BofAI/agent-wallet to configure a wallet." or at minimum a reference to the relevant configuration mechanism.

FINDING-m5: start.sh Still Shown as Modified — Potential Uncommitted Change

• File: start.sh
• Severity: Minor
• Category: Process
• Description: start.sh appears in the git diff --stat output for the PR, but the working tree status also flags it as modified. The reviewed content of start.sh does not show obvious issues (it correctly starts in --readonly mode via PM2). Ensure the committed version aligns with the reviewed content.
• Recommendation: Verify committed content matches reviewed content before merge.

FINDING-m6: tests/core/services/agent-wallet.test.ts — Missing Coverage for listAgentWallets Tuple Format

• File: tests/core/services/agent-wallet.test.ts
• Severity: Minor
• Category: Testing
• Description: The new listAgentWallets() implementation handles both tuple [id, config, isActive] and legacy object {id, type} return shapes. The test suite only covers the legacy object format ({ id: "w1", type: "tron_local" }); there is no test exercising the new tuple format that agent-wallet@2.3+ is documented to return. If the SDK changes its return shape, the tuple parsing branch will be silently untested.
• Recommendation: Add a test case passing [["w1", { type: "tron_local" }, true], ["w2", { type: "tron_local" }, false]] to mockListWallets and asserting the correct result.

FINDING-m7: tests/core/services/account-resource.test.ts — Hardcoded Nile Address Used for Wallet-Dependent Tests

• File: tests/core/services/account-resource.test.ts, lines 28, 45, 59, 76, 93
• Code Evidence:

  const receiverAddress = "T9yD14Nj9j7xAB4dbGeiX9h8unkKHxuWwb";

• Severity: Minor
• Category: Testing
• Description: The address is hardcoded to a specific Nile testnet address that appears to be a known test fixture address. Combined with hasWallet = false, these tests will never run, making the hardcoded address harmless but also pointless. If the hasWallet flag is ever re-enabled, the hardcoded address may not be valid for the caller's context (e.g., if testing delegation, the caller's address matters).
• Recommendation: Align with FINDING-M4: make hasWallet configurable via env, and when enabled, derive the receiver address from the configured wallet or from an explicit test env var.

---

3.4 SUGGESTIONS

FINDING-S1: Consider a Startup Banner Indicating Wallet Status

• File: src/index.ts or src/server/server.js
• Severity: Suggestion
• Category: UX
• Description: Now that all tools are registered regardless of wallet state, a startup log line indicating whether a wallet is configured would greatly aid operators and developers. Something like: "Wallet status: configured (wallet-1) | No wallet configured — write operations will fail at runtime".
• Recommendation: Call getActiveWalletId() during server initialization and log the result to console.error (stderr).

FINDING-S2: signTransactionRaw Is Functionally Identical to signTransaction — Consider Merging

• File: src/core/services/agent-wallet.ts, lines 168–175
• Code Evidence:

  export async function signTransactionRaw(
    unsignedTx: Record<string, unknown>,
    _network = "mainnet",
  ): Promise<any> {
    const wallet = await getActiveWallet();
    const signedJson = await wallet.signTransaction(unsignedTx);
    return JSON.parse(signedJson);
  }

• Severity: Suggestion
• Category: Code Quality / Maintainability
• Description: signTransactionRaw is byte-for-byte identical in logic to signTransaction (the _network parameter is unused). In the previous version, this function used a different low-level signing path (raw crypto.signTransaction). The simplification to use wallet.signTransaction in both is correct, but the duplication should be resolved.
• Recommendation: Remove signTransactionRaw and have its callers use signTransaction directly, or make signTransaction accept an optional network parameter.

FINDING-S3: resolveWalletProvider Errors Are Silently Swallowed

• File: src/core/services/agent-wallet.ts, lines 28–35
• Code Evidence:

  try {
    provider = resolveWalletProvider({ network: "tron" });
    return provider;
  } catch (_e) {
    return null;
  }

• Severity: Suggestion
• Category: Diagnostics / Observability
• Description: Any exception from resolveWalletProvider (including configuration errors, file permission issues, or corrupted wallet files) is silently discarded. Operators diagnosing "Wallet not configured." errors have no server-side log to reference.
• Recommendation: Log the caught error at debug or warning level: console.error("[agent-wallet] Provider initialization failed:", _e). This preserves the graceful degradation behavior while providing actionable diagnostic output.

FINDING-S4: mcp_example.json Removes All Wallet Env Vars Without Replacement

• File: mcp_example.json
• Severity: Suggestion
• Category: Documentation / UX
• Description: The example MCP configuration now only includes TRONGRID_API_KEY in the env block, with no indication of how to configure the wallet. A developer copying this file will have a working read-only server but will not understand why write tools fail.
• Recommendation: Add a comment or placeholder env var (e.g., "AGENT_WALLET_DIR": "~/.agent-wallet") with a link to setup documentation.

---

4. Positive Observations

1. Clean Architecture Simplification: The removal of ensureEnvMapping() and the requiresWallet annotation eliminates a complex conditional registration path that was difficult to reason about. The new model (register all, fail at runtime with clear error) is conceptually simpler.

2. Improved Test Structure: agent-wallet.test.ts is substantially improved. The shift from env-var-mutation-based test setup to explicit mock provider factories (createProvider(), createMockWallet()) makes tests more isolated, deterministic, and readable. Using vi.unstubAllEnvs() and vi.resetModules() in afterEach is correct practice.

3. Graceful Runtime Error Handling: Every wallet-dependent tool handler wraps its logic in a try/catch and returns { isError: true, content: [...] }, which is the correct MCP pattern for surfacing errors without crashing the server. This behavior is now validated by new test cases (e.g., get_wallet_address error test in tools.test.ts).

4. Correct listAgentWallets() Empty State: The fallback to [] when address == null in single-wallet mode (line 133–134 of agent-wallet.ts) is a proper edge case fix that prevents a crash or misleading result when a provider is present but no wallet is active.

5. Integration Test Cleanup: Removing the dummy private key injection from tools_integration.test.ts (process.env.TRON_PRIVATE_KEY = "0000...0001") eliminates a test anti-pattern that could have caused false-positive test results by making otherwise-broken paths appear to work.

6. integration_stdio.ts Improvements: The new stdio integration test properly isolates the process environment by stripping all wallet-related keys and setting a fresh HOME, preventing tests from picking up developer machine credentials. Adding validation that get_wallet_address fails gracefully with isError: true is a valuable E2E regression guard.

7. listAgentWallets() Dual-Shape Handling: The polymorphic parsing of both old object and new tuple shapes from listWallets() is pragmatic and prevents upgrade-induced breakage if the SDK response shape changes between patch releases.

---

5. Checklist Results

Category	Check	Result	Notes
Correctness & Logic	Core logic correctly implements intended behavior	Pass	Eager registration + runtime check is logically sound
Correctness & Logic	No dead code paths	Fail	getOwnerAddress() null guard is dead code (FINDING-M2)
Correctness & Logic	signTransactionRaw vs signTransaction duplication	Warn	Functionally identical; should be merged (FINDING-S2)
Correctness & Logic	API shape compatibility handled correctly	Pass with caveat	Dual-shape parser present; new tuple format untested (FINDING-m6)
Security	Private keys not passed as tool arguments	Pass	Confirmed — no key material in tool schemas
Security	Env variable removal does not introduce new leak vectors	Pass	Removal is clean; no new plaintext exposure
Security	Error messages do not leak sensitive data	Pass	Errors are generic enough
Security	Backward compatibility break is documented	Fail	No migration warning in code; only in CHANGELOG (FINDING-C1)
Performance	Provider singleton caching works correctly	Pass	provider, activeWallet, activeAddress are module-level singletons
Performance	No unnecessary async operations	Pass	getProvider() is synchronous; lazy initialization pattern is efficient
Code Quality	Removed requiresWallet annotation consistently	Pass	Removed from types.ts, tools/index.ts, wallet.ts
Code Quality	Tool descriptions updated to match new behavior	Partial Fail	select_wallet still references "Encrypted Storage mode" (FINDING-M3)
Code Quality	No unused imports	Pass	import * as services from "./services/index.js" removed from tools/index.ts and prompts.ts
Code Quality	package.json module field corrected	Pass	Changed from src/index.ts to build/index.js
Testing	Unit test coverage for changed functions	Pass	agent-wallet.test.ts comprehensively covers new paths
Testing	Integration tests remain runnable	Fail	hasWallet = false permanently disables wallet-path integration tests (FINDING-M4)
Testing	New behavior (always-registered tools) tested	Pass	tools.test.ts validates tools are registered without wallet
Testing	Runtime error path for wallet-absent tools tested	Pass	Both unit and integration tests validate isError: true
Testing	listWallets tuple format tested	Fail	Only legacy object format tested in new test suite (FINDING-m6)
Documentation	README.md updated for new wallet flow	Pass	Thoroughly updated
Documentation	AGENTS.md updated	Pass	Updated accurately
Documentation	CHANGELOG.md updated	Pass	Entry added for 1.1.7
Documentation	In-code error messages provide actionable guidance	Fail	Error messages stripped to bare statements (FINDING-m4)
Documentation	mcp_example.json reflects new configuration	Partial Fail	Wallet config removed without replacement guidance (FINDING-S4)

---

6. Review Verdict

Verdict: Request Changes

This PR achieves its stated goals — upgrading @bankofai/agent-wallet to 2.3.0, simplifying the registration model, and cleaning up legacy TRON_* env var support. The code is generally clean, the test improvements are meaningful, and the architectural direction is sound.

However, the following issues require resolution before merge:

Must Fix (Blocking):

1. FINDING-C1 — The silent removal of TRON_PRIVATE_KEY / TRON_MNEMONIC support is a breaking change that will affect all existing static-key deployments without in-band warning. Add a runtime startup warning or handle this as a semver-major release.
2. FINDING-M4 — Permanently hardcoding const hasWallet = false in seven integration test files removes all CI coverage for wallet-backed write operations. Restore a conditional (env-gated) mechanism.

Should Fix (Non-blocking but important):

3. FINDING-M2 — Remove the dead null check in getOwnerAddress().
4. FINDING-M3 — Update select_wallet description to remove stale "Encrypted Storage mode" reference.
5. FINDING-m4 — Restore actionable guidance in wallet error messages.
6. FINDING-m6 — Add test coverage for the listWallets tuple return format.

Nice to Have:

7. FINDING-S1 — Add wallet status logging at startup.
8. FINDING-S2 — Merge signTransactionRaw into signTransaction.
9. FINDING-S3 — Log swallowed resolveWalletProvider errors.
10. FINDING-S4 — Improve mcp_example.json with wallet setup hint.

---

Report generated by automated code review on 2026-03-21.