Skip to content

feat(cli): @bankofai/x402-cli — spec + read-only commands (config / doctor / balance)#70

Open
boboliu-1010 wants to merge 6 commits intomainfrom
002-bankofai-cli
Open

feat(cli): @bankofai/x402-cli — spec + read-only commands (config / doctor / balance)#70
boboliu-1010 wants to merge 6 commits intomainfrom
002-bankofai-cli

Conversation

@boboliu-1010
Copy link
Copy Markdown
Collaborator

Summary

First slice of the BankofAI x402 CLI under typescript/packages/cli. This PR delivers the read-only path; signing commands (transfer / pay / serve transfer / receipt) follow.

Three commits, in order:

  1. c1dab1c — Lands the feature spec at specs/002-bankofai-cli/bankofai-cli.md plus a notes/decisions.md resolving three implementation-blocking gaps (D1 wallet source, D2 JSON envelope, D3 env namespace).
  2. b6490b2 — D4 facilitator decision: lock to BankofAI's hosted endpoint, derive URL from network. No --facilitator-url flag, no profile field. Single internal escape hatch via X402_FACILITATOR_URL_OVERRIDE for e2e.
  3. 4038230 — Read-only CLI implementation: config (init/use/get/set/list), doctor, balance.

What's runnable today

pnpm --filter @bankofai/x402-cli build
node typescript/packages/cli/dist/index.js config init --json
export TRON_PRIVATE_KEY=0x...                       # 0x-prefixed hex
node typescript/packages/cli/dist/index.js doctor --json
node typescript/packages/cli/dist/index.js balance --json

Verified live against https://facilitator.bankofai.io/nile on 2026-04-27.

Command Outcome (no key) Outcome (with key)
config init/use/get/set/list All paths green All paths green
doctor overall=fail (wallet missing), other checks still run overall=ok — Node + wallet + facilitator + GasFree + token all green
balance WALLET_NOT_AVAILABLE Returns main address + canonical gasFreeAddress + asset table

Output shape

Every command emits the wrapped envelope from D2:

{ "ok": true,  "command": "balance", "network": "tron:nile", "result": { /* ... */ } }
{ "ok": false, "command": "doctor",  "error": { "code": "WALLET_NOT_AVAILABLE", "message": "...", "hint": "..." } }

15 standardized error codes (src/error.ts) — WALLET_NOT_AVAILABLE, PROFILE_NOT_FOUND, FACILITATOR_UNAVAILABLE, INVALID_INPUT, etc. — match the spec's catalog.

Decisions encoded (see specs/002-bankofai-cli/notes/decisions.md)

  • D1 — wallet source: env var only (TRON_PRIVATE_KEY / EVM_PRIVATE_KEY). No --private-key flag. Mirrors @bankofai/agent-wallet env conventions.
  • D2 — JSON envelope: wrapped form is the only shape emitted.
  • D3 — env namespace: X402_* for non-secret config (X402_PROFILE / X402_NETWORK / X402_SCHEME / X402_TOKEN / X402_OUTPUT / X402_CONFIG_FILE); native names (TRON_PRIVATE_KEY, etc.) for secrets so one .env works across SDK + e2e + CLI.
  • D4 — facilitator endpoint: locked to https://facilitator.bankofai.io/<network> derived from network. No flag, no profile field. X402_FACILITATOR_URL_OVERRIDE is the single e2e-only escape hatch and emits a stderr warning when active.

Tests

  • 19 unit tests in the new package (output / config / facilitator). All green.
  • Manual end-to-end against the live BankofAI Nile proxy confirms every documented command path produces the documented envelope shape and exit code.
  • Regression: pnpm --filter @bankofai/x402 test → 51 green, cd python/x402 && uv run pytest → 217 green, ./e2e/scenarios/run_all.sh → 8 mock scenarios green.

What's intentionally NOT in this PR

  • transfer, pay, serve transfer, receipt, inspect, request commands — covered in the spec, deferred to subsequent iterations.
  • EVM (BSC) wallet support — derivation lands when transfer / pay arrive.
  • Receipt store rotation, body-replay semantics for pay --body @file, serve transfer graceful shutdown — deferred items in notes/decisions.md § "Open items deferred".

Test plan (reviewer)

  • pnpm install && pnpm --filter @bankofai/x402-cli build — should produce dist/index.js and mark it executable.
  • pnpm --filter @bankofai/x402-cli test — expect 19 green.
  • node typescript/packages/cli/dist/index.js --help — shows the 7 commands.
  • X402_CONFIG_FILE=/tmp/x.json node …/dist/index.js config init --json — emits success envelope, file appears.
  • node …/dist/index.js doctor --json (no env) — emits failure envelope with WALLET_NOT_AVAILABLE and a hint, exit code 1.
  • TRON_PRIVATE_KEY=0x… node …/dist/index.js doctor --json — emits success envelope with all 5 checks green and wallet showing a masked address.
  • Inspect spec + decisions for clarity; flag any decision worth revisiting before signing-command iteration.

🤖 Generated with Claude Code

bobo and others added 3 commits April 27, 2026 15:25
Lands the CLI feature spec at specs/002-bankofai-cli/bankofai-cli.md
(authored externally) and a thin docs/cli-design.md pointer so the
repo has one source of truth.

Adds notes/decisions.md to resolve the three implementation-blocking
gaps the spec leaves open:

D1 — Wallet source: env var (TRON_PRIVATE_KEY / EVM_PRIVATE_KEY) for
     MVP. Skips Keychain, agent-wallet integration, and JSON keystore
     until we have concrete need. Documents the shell-history risk
     and how the CLI mitigates (no --private-key flag, doctor check).

D2 — JSON envelope: pin the wrapped { ok, command, network, scheme,
     result } shape from the spec's "输出格式" section as the only
     emitted form. Explicitly supersedes the flat shape shown in the
     `pay` example mid-spec; everything moves under `result`.

D3 — Env var namespace: X402_* prefix for non-secret config overrides
     (X402_PROFILE, X402_NETWORK, X402_FACILITATOR_URL, …). Keep
     wallet keys under their native names (TRON_PRIVATE_KEY,
     EVM_PRIVATE_KEY) so a single .env works across SDK, e2e harness,
     and CLI without three places to set the same secret.

Defers (NOT MVP-blocking, decisions can happen during impl): inspect
transfer semantics, receipt rotation, non-GasFree doctor, body @file
replay, serve transfer shutdown.

Suggested first PR boundary: config + doctor + balance (read-only,
no wallet signing). Then transfer / pay / serve transfer / receipt.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sted

Per follow-up: the CLI talks to BankofAI's hosted facilitator only.
URL is derived from `network` (e.g. tron:nile → facilitator.bankofai.io/nile)
via a getFacilitatorBaseUrl(network) helper. The spec's per-profile
"facilitatorUrl" field and the per-command "--facilitator-url" flag are
dropped from the user-facing surface.

Single internal escape hatch: X402_FACILITATOR_URL_OVERRIDE env var,
used only by the e2e harness to point at examples/facilitator/. When
set, every command emits a stderr warning so it's never silent.

Why: brand cohesion (BankofAI CLI → BankofAI facilitator), concrete
SLAs (one target), less to teach (users learn `network`, the rest
follows), and "lock first, expand if real demand" is cheaper than the
reverse.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…alance)

First slice of the BankofAI x402 CLI per specs/002-bankofai-cli/. This
PR delivers the read-only path: profile management, environment
diagnostics, and GasFree balance inspection. Signing commands (transfer,
pay, serve transfer, receipt) follow once this slice is reviewed.

Package: typescript/packages/cli, published as @bankofai/x402-cli with
the binary `x402`. Routes via commander; output via a wrapped envelope
matching D2 in specs/002-bankofai-cli/notes/decisions.md:

    { ok, command, network?, scheme?, result }            # success
    { ok, command, error: { code, message, hint? } }      # failure

Commands:
- `x402 config init / use / get / set / list` — pure file I/O at
  ~/.x402/config.json (override via X402_CONFIG_FILE). Defaults to a
  `nile` profile pinned to TRON Nile + exact_gasfree.
- `x402 doctor` — five non-throwing checks: Node version, wallet env,
  facilitator endpoint reachability, GasFree address info (when scheme
  is exact_gasfree on TRON), and token registry sanity. Each check
  reports ok/warn/fail/skipped; one failure flips `overall` but other
  checks still run.
- `x402 balance` — main wallet + derived gasFreeAddress + per-asset
  balance/transferFee/activateFee from the BankofAI Nile proxy.
  Addresses masked by default; --verbose shows full strings. Honors
  solutions.md #9 — never recursively re-queries gasFreeAddress.

Decisions encoded:
- D1 wallet via TRON_PRIVATE_KEY / EVM_PRIVATE_KEY env. No --private-key
  flag; deriveWalletInfo throws WALLET_NOT_AVAILABLE with a hint when
  the var is unset.
- D2 wrapped envelope is the only emitted shape; no flat fallback.
- D3 X402_* prefix for non-secret config (PROFILE / NETWORK / SCHEME /
  TOKEN / OUTPUT / CONFIG_FILE); native names for secrets.
- D4 facilitator URL is derived from network — no `--facilitator-url`
  flag, no profile field. X402_FACILITATOR_URL_OVERRIDE is the single
  e2e-only escape hatch and emits a stderr warning when active.

Standard error codes from the spec are normalized in src/error.ts and
surfaced through the envelope's `error.code` field.

Test plan (verified live against BankofAI Nile, 2026-04-27):
- 19 unit tests across config / output / facilitator (all green).
- `x402 config init/use/get/set/list` happy + error paths produce the
  envelope shape and exit codes documented in the spec.
- `x402 doctor` without TRON_PRIVATE_KEY: 4 ok, 1 fail (wallet), 1
  skipped (gasfree depends on wallet); overall=fail.
- `x402 doctor` with TRON_PRIVATE_KEY: all 5 checks ok; reports the
  real GasFree state for our test wallet (active=true, nonce=2 —
  matches the two on-chain settlements from PR #69's testnet run).
- `x402 balance --json` returns the canonical GasFree address and
  zero USDT balance (consumed by yesterday's gasfree e2e).

Regression: 217 pytest + 51 vitest (SDK) + 8 e2e mock scenarios still
green; the new package adds 19 vitest tests on its own.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Code Review Report

Project: x402 / BankofAI SDK
PR: main -> 002-bankofai-cli
Review Date: 2026-04-27
Reviewer: AI Code Reviewer (Code Review Skill v1.0.0)


PR Overview

Branch Information

Property Value
From Branch main
To Branch 002-bankofai-cli
Commits 3
Files Changed 21
Lines Added +3,134
Lines Removed -1

Commit History

Hash Message
4038230 feat(cli): @bankofai/x402-cli read-only commands (config / doctor / balance)
b6490b2 spec(002-bankofai-cli): D4 — lock facilitator endpoint to BankofAI hosted
1dab1c spec(002-bankofai-cli): land draft + decisions notes for MVP

Review Summary

Verdict

Verdict: Request Changes

Findings at a Glance

Critical Major Minor Suggestion
Count 0 2 5 4

Summary

This PR introduces the @bankofai/x402-cli package, an MVP read-only CLI providing config, doctor, and balance commands for the BankofAI x402 GasFree TRON workflow. It also lands the full feature spec (specs/002-bankofai-cli/) and the D1–D4 decisions log.

The overall code quality is high. The error taxonomy (X402CliError + ErrorCode union), JSON output envelope, profile/env-var layering, and the BankofAI-locked facilitator URL resolution are well-designed and align with the repo conventions. Tests cover utilities and the output formatter.

Two issues need to be resolved before merge. First, packages/mcp is referenced in both pnpm-workspace.yaml and pnpm-lock.yaml but the directory does not exist, which will break pnpm install in CI. Second, @bankofai/agent-wallet appears as a runtime dependency in package.json but is never imported in any source file, inflating the published package.


Change Summary

1. Feature Spec + Decisions

File Change Type Description
specs/002-bankofai-cli/bankofai-cli.md Added Full feature spec (826 lines): commands, module design, error codes, security boundaries, MVP acceptance criteria
specs/002-bankofai-cli/notes/decisions.md Added D1–D4 decisions log (wallet source, JSON envelope, env namespace, facilitator lock)
specs/CLAUDE.md Modified Registers the new spec in the current-feature table; updates spec directory description
docs/cli-design.md Added Thin pointer from docs/ to the authoritative spec

Purpose: Establishes the design contract before code. All implementation choices in the TypeScript code trace back to a named decision (D1–D4).


2. New CLI Package — typescript/packages/cli/

File Change Type Description
src/error.ts Added ErrorCode union type + X402CliError class + isCliError guard
src/config.ts Added Profile loading / saving, validateConfig, applyEnvOverrides (D3), configFilePath
src/output.ts Added buildSuccess/buildFailure envelope builders, emit, maskAddress, runCommand helper
src/facilitator.ts Added getFacilitatorBaseUrl — network → BankofAI URL (D4) + X402_FACILITATOR_URL_OVERRIDE hatch
src/wallet.ts Added readPrivateKey + deriveWalletInfo — derive TRON address from TRON_PRIVATE_KEY env var (D1)
src/commands/config.ts Added cmdInit, cmdUse, cmdGet, cmdSet, cmdList — manage local profiles
src/commands/doctor.ts Added cmdDoctor — 5-check read-only diagnostics (Node version, wallet, facilitator, GasFree, token)
src/commands/balance.ts Added cmdBalance — GasFree address info + per-asset balances via GasFreeAPIClient
src/index.ts Added commander-based command router; resolveOutputMode, exitWith
src/config.test.ts Added Tests for loadConfig, saveConfig, applyEnvOverrides, getProfile
src/facilitator.test.ts Added Tests for getFacilitatorBaseUrl including override hatch
src/output.test.ts Added Tests for envelope builders and maskAddress
package.json Added Package manifest: @bankofai/x402-cli v0.1.0
tsconfig.json Added Extends workspace base config
README.md Added First-run guide, command docs, env var table

Purpose: Implements the three read-only MVP commands that validate the CLI shell, profile loader, output envelope, and SDK-import path without requiring a signing key at runtime.


3. Workspace + Lockfile Updates

File Change Type Description
typescript/pnpm-workspace.yaml Modified Adds packages/cli and packages/mcp to workspace
typescript/pnpm-lock.yaml Modified Adds lockfile entries for packages/cli, packages/mcp, commander, tronweb, and transitive deps

Purpose: Wires the new CLI package into the pnpm workspace. Also introduces a packages/mcp workspace reference that has no corresponding directory (see MJ-01).


Detailed Findings


Major

[MJ-01] packages/mcp workspace entry has no corresponding directory

Property Value
Severity Major
Category Correctness
File typescript/pnpm-workspace.yaml : Line 3; typescript/pnpm-lock.yaml : Lines ~15–42

Description

pnpm-workspace.yaml adds 'packages/mcp' to the workspace package list, and pnpm-lock.yaml contains a full resolution section for it (with @modelcontextprotocol/sdk, hono, etc.). However, the directory typescript/packages/mcp/ does not exist in the repository at this commit.

Running pnpm install will fail with a workspace resolution error because pnpm tries to scan the declared directory and finds nothing there. This breaks CI for all contributors pulling this branch.

Code

# typescript/pnpm-workspace.yaml
packages:
  - 'packages/x402'
+  - 'packages/mcp'    # ← directory does not exist
+  - 'packages/cli'
  - '../examples/typescript/client'
# pnpm-lock.yaml (excerpt)
  packages/mcp:
    dependencies:
      '@bankofai/x402':
        specifier: workspace:*
        ...
      '@modelcontextprotocol/sdk':
        specifier: ^1.12.1

Recommendation

Either commit the typescript/packages/mcp/ package (with at least a package.json) in this PR, or remove the packages/mcp entry from both pnpm-workspace.yaml and pnpm-lock.yaml and land it in a separate PR when the MCP package is ready.


[MJ-02] @bankofai/agent-wallet declared as runtime dependency but never used

Property Value
Severity Major
Category Code Quality
File typescript/packages/cli/package.json : Line 23

Description

package.json lists @bankofai/agent-wallet: "^2.3.0" under dependencies. No source file in src/ imports anything from it. The comment in wallet.ts explicitly explains why it isn't used yet ("we don't pull in @bankofai/agent-wallet's full provider flow"), confirming this is dead dependency weight.

As a runtime dependency (not devDependency), it will be installed for every consumer of @bankofai/x402-cli, adding ~2.3 MB of transitive packages unnecessarily.

Code

// package.json
"dependencies": {
  "@bankofai/x402": "workspace:*",
  "@bankofai/agent-wallet": "^2.3.0",   // ← never imported
  "commander": "^12.1.0",
  "tronweb": "^6.0.0"
}
// wallet.ts — the comment explains the intentional non-use:
// "we don't pull in @bankofai/agent-wallet's full provider flow
//  for this"

Recommendation

Remove @bankofai/agent-wallet from dependencies now. Add it back when the signing commands (transfer, pay) are implemented and the package is actually imported. Track the future integration with a TODO comment in wallet.ts.


Minor

[MN-01] void opts.gasfree suppresses unused variable without communicating intent

Property Value
Severity Minor
Category Code Quality
File typescript/packages/cli/src/commands/balance.ts : Line 49

Description

The --gasfree / --no-gasfree flag is plumbed through opts and then immediately suppressed with void opts.gasfree. This silences the TypeScript "declared but never used" lint without making it obvious to the next developer whether the flag actually controls behavior (it doesn't, yet).

Code

// Default in MVP: --gasfree implied. Allow --no-gasfree once we have the
// alternative path; until then the flag is a no-op alias.
void opts.gasfree;

Recommendation

Use an explicit early-return guard or a named constant to make the no-op intent clearer, e.g.:

// --gasfree is always implied in MVP; --no-gasfree is a future placeholder.
const _gasfreeFlag = opts.gasfree; // suppressed: always true in MVP

Or, preferably, omit the --no-gasfree option entirely until it has an actual code path, and document in index.ts that the flag will be added when EVM balance support lands.


[MN-02] void X402CliError keepalive idiom is non-standard

Property Value
Severity Minor
Category Code Quality
File typescript/packages/cli/src/commands/doctor.ts : Line 186; typescript/packages/cli/src/output.ts : Line 151

Description

Two files end with void X402CliError; as a "type-only import keepalive". This is an undocumented pattern that will confuse maintainers. The standard ESM approach for type-only imports that are used purely as types is import type { ... } — which strips the import at compile time and doesn't need a keepalive.

Code

// doctor.ts — last line
void X402CliError;

// output.ts — last line
void X402CliError; // referenced for type-only import keepalive

Recommendation

Review whether X402CliError is actually referenced as a value (i.e., instanceof check or constructor call) in these files, or only as a type. doctor.ts calls isCliError(err) which does the instanceof check internally, so it doesn't need the class value at all. Switch to import type where applicable:

import { isCliError } from '../error.js';         // value import for isCliError
import type { X402CliError } from '../error.js';  // type-only import; no keepalive needed

[MN-03] formatSmallestUnit accepts number input which silently loses precision for large amounts

Property Value
Severity Minor
Category Correctness
File typescript/packages/cli/src/commands/balance.ts : Lines 1313–1324

Description

The function accepts string | number. When typeof amount === 'number', it converts via BigInt(Math.trunc(amount)). JavaScript Number only has 53 bits of precision (safe integer range ≤ 2^53 − 1 = ~9 × 10^15). For token amounts near or above this threshold (plausible for high-denomination tokens with many decimals), Math.trunc will silently truncate.

The TypeScript conventions in this repo explicitly state: "Never convert amounts through float, even briefly for formatting."

Code

function formatSmallestUnit(amount: string | number, decimals: number): string {
  const raw = typeof amount === 'number'
    ? BigInt(Math.trunc(amount))   // ← precision loss for amounts > 2^53
    : BigInt(amount || '0');

Recommendation

Remove the number overload entirely. The GasFree API response should return numeric amounts as strings (or the caller should stringify before passing). If a numeric response is observed in practice, handle it at the call-site where the API shape is known:

function formatSmallestUnit(amount: string, decimals: number): string {
  const raw = BigInt(amount || '0');
  // …
}
// At call-site:
balance: asset.balance ?? '0',   // already string from API

[MN-04] checkToken in doctor.ts performs format validation only — not registry presence

Property Value
Severity Minor
Category Testing / Correctness
File typescript/packages/cli/src/commands/doctor.ts : Lines 1698–1711

Description

The token check only validates that the symbol matches ^[A-Z0-9]{2,10}$. Any well-formed symbol (e.g., FAKE, ZZ, USDT) returns ok. This means x402 doctor will report a green checkmark for a token that the SDK cannot actually resolve, giving a misleading diagnostic.

The comment acknowledges the limitation: "The SDK's TokenRegistry isn't currently exported as a typed structure on the public surface."

Code

function checkToken(network: string, token: string): Check {
  if (!/^[A-Z0-9]{2,10}$/.test(token)) {
    return { name: 'token', status: 'warn', detail: `token '${token}' on ${network}: symbol shape unusual` };
  }
  return { name: 'token', status: 'ok', detail: `${token} on ${network}` };
}

Recommendation

Downgrade the check result to warn (not ok) until a real registry lookup is possible, so the output honestly communicates that the check is partial:

return {
  name: 'token',
  status: 'warn',
  detail: `${token} on ${network}: symbol format ok; registry lookup not available in this CLI version`,
};

Alternatively, expose a resolveToken(network, symbol) helper from @bankofai/x402 and call it here, returning fail with TOKEN_NOT_FOUND if the symbol isn't in the registry.


[MN-05] _overrideWarned module-level flag persists across test runs and long-lived processes

Property Value
Severity Minor
Category Testing / Correctness
File typescript/packages/cli/src/facilitator.ts : Line 118

Description

_overrideWarned is a module-level let flag that prevents duplicate stderr warnings when X402_FACILITATOR_URL_OVERRIDE is set. Because it lives in module scope and is never reset, it will never warn again once set to true for the lifetime of the Node process. In tests this is harmless today (only one test exercises the override), but if test isolation is added later (e.g., vi.resetModules()), the flag state bleeds across test cases unexpectedly.

Code

let _overrideWarned = false;   // module-level state

export function getFacilitatorBaseUrl(network: string): string {
  const override = process.env[OVERRIDE_ENV];
  if (override && override.trim()) {
    if (!_overrideWarned) {
      _overrideWarned = true;
      process.stderr.write(`[x402] CLI facilitator override active: …\n`);
    }
    return override.trim().replace(/\/$/, '');
  }
  // …
}

Recommendation

For test hygiene, export a _resetOverrideWarned() function (prefixed with _ to signal it's test-only) or use vi.isolateModules in the test file. Alternatively, emit the warning unconditionally on every call — for an escape-hatch that's only used in e2e tests, a duplicate warning per command invocation is acceptable.


Suggestions

[S-01] Missing command-level tests for cmdBalance, cmdDoctor, and cmdInit/Use/Get/Set/List

File: typescript/packages/cli/src/commands/
Description: The test suite covers utility modules (config.ts, output.ts, facilitator.ts) but does not exercise command execution end-to-end. For example, cmdBalance error paths (network not TRON, GasFree API failure) are untested.
Suggestion: Add src/commands/balance.test.ts, doctor.test.ts, config.test.ts using vitest and mock the GasFreeAPIClient with vi.mock. Focus on error-code propagation and output envelope shape.


[S-02] cmdGet spreads Profile into return object — fragile against future schema additions

File: typescript/packages/cli/src/commands/config.ts : Line 1422
Description: cmdGet returns { profile: resolved, isDefault: ..., ...profile }. If Profile ever gains a field named profile or isDefault, the spread silently overwrites the explicit keys.
Suggestion: Explicitly list the returned fields:

return {
  profile: resolved,
  isDefault: cfg.defaultProfile === resolved,
  network: profile.network,
  scheme: profile.scheme,
  token: profile.token,
  walletNetwork: profile.wallet.network,
};

[S-03] tronweb imported directly for address derivation — consider a thin helper in @bankofai/x402

File: typescript/packages/cli/src/wallet.ts : Line 1
Description: Per the TypeScript conventions: "No new wrappers — use existing helpers in typescript/packages/x402/src/mechanisms/." The CLI imports tronweb directly. While the comment in wallet.ts justifies this (avoiding the full TronClientSigner.create() flow for read-only address derivation), it creates a second callsite that replicates the toHex + replace(/^41/, '') pattern already present in signer.ts. If the hex conversion logic ever changes, both places must be updated.
Suggestion: Export a lightweight deriveTronAddress(hexPrivateKey: string): { base58: string; evmHex: string } utility from @bankofai/x402/src/signers/ and call it from wallet.ts.


[S-04] README references docs/solutions.md #9 by anchor — fragile to renumbering

File: typescript/packages/cli/README.md : Line 67
Description: The README links to docs/solutions.md #9 with a bare #9 reference in prose, which would break if entries are reordered. Entry #9 currently exists and is correctly titled "GasFree gasFreeAddress is per-query, not absolute."
Suggestion: Update the link to use the entry title as an anchor (if GitHub-rendered Markdown allows it), or add a short descriptive comment inline instead of relying on the numbering:

<!-- solutions.md §9: gasFreeAddress is derived per query-address, not recursive -->
do **not** recursively query that address again

Positive Observations

Area Observation
Error design X402CliError with a discriminated ErrorCode union type is exactly right for Agent consumers of --json output. Every user-facing failure maps to a stable, documented code.
Output envelope The buildSuccess / buildFailure design (D2) enforces a single JSON shape across all commands. Conditional spread for optional network/scheme fields keeps the envelope clean without null values.
Config validation validateConfig performs explicit field-by-field validation and throws structured errors with source paths, rather than relying on loose as casts. The defaultProfile cross-reference check at the end is particularly thorough.
Security: no --private-key Refusing to accept the private key as a CLI flag (with explicit documentation in README and decisions.md) prevents the most common key-leakage vector in CLIs. Env var with a direnv/.env workflow recommendation is the right guidance.
Facilitator lock (D4) Locking the facilitator URL to BankofAI-hosted and only exposing an X402_FACILITATOR_URL_OVERRIDE for e2e testing — with a stderr warning when active — is a well-reasoned product decision that prevents support-burden for third-party endpoints.
solutions.md respect balance.ts and README.md both explicitly reference solutions.md #3 and #9 (GasFree balance source and no-recursive-query rule), showing the team has internalized the hard-won knowledge base.
Address masking Default address masking in all read-only outputs (maskAddress with --verbose override) is a good privacy default for a CLI that may be used in screencasts or logs.
Test coverage for utilities config.test.ts covers round-trip, missing file, malformed JSON, defaultProfile mismatch, env override, and PROFILE_NOT_FOUND — comprehensive for the module.

Checklist Results

Category Items Checked Pass Fail N/A Notes
Correctness 8 6 2 0 BigInt precision path (MN-03); token check misleading (MN-04)
Security 10 9 0 1 No signing in this MVP; env-var key handling is correct
Performance 7 7 0 0 CLI tool; no server-side performance concerns
Code Quality 8 5 3 0 Dead dependency (MJ-02); void suppressions (MN-01, MN-02)
Testing 7 4 2 1 Missing command-level tests (S-01); _overrideWarned bleed (MN-05)
Documentation 6 5 0 1 All public APIs documented; spec & decisions thorough
Compatibility 5 4 1 0 packages/mcp workspace ghost (MJ-01)
Observability 4 4 0 0 Override warning, structured JSON, masked addresses

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-04-27

Phase A + B per the user's directive ("既要开发又要补充测试用例"):
backfills missing test coverage on the read-only commands shipped in
PR #70, then implements the first signing command — `x402 transfer`
— with its own tests and a real-chain Nile validation.

Phase A — backfill tests for read-only commands:
- src/wallet.test.ts (7 tests): TRON_PRIVATE_KEY parsing, address
  derivation correctness against a known key, EVM stub rejection.
- src/commands/config.test.ts (12 tests): tmp-dir end-to-end across
  init / use / get / set / list including --force, dotted paths, and
  PROFILE_NOT_FOUND / INVALID_INPUT error envelopes.
- src/commands/doctor.test.ts (4 tests): mock fetch + GasFreeAPIClient
  to validate ok / fail / skipped status matrix and overall flag.
- src/commands/balance.test.ts (5 tests): mock GasFree client to verify
  mask/verbose, --token filter, FACILITATOR_UNAVAILABLE on throw, and
  smallest-unit fee formatting.

Phase B — `x402 transfer`:
- src/commands/transfer.ts: TRON exact_gasfree direct payment that
  builds PaymentRequirements locally, runs through X402Client +
  ExactGasFreeClientMechanism for TIP-712 signing, then submits the
  resulting PaymentPermit straight to the BankofAI GasFree proxy via
  GasFreeAPIClient.submit + waitForSuccess. We do NOT route through
  /fee/quote /verify /settle — the BankofAI hosted endpoint is the
  GasFree proxy, not a full facilitator surface (revising the D4 plan
  in notes/decisions.md).
- src/amount.ts: token registry lookup with --asset / --decimals
  override path, parseHumanAmount with strict regex (rejects scientific
  notation / signs / trailing dot), formatSmallestUnit with trailing-
  zero strip, newPaymentId via globalThis.crypto.
- src/receipts.ts: append-only JSONL store at ~/.x402/receipts.jsonl
  (override via X402_RECEIPT_FILE), tolerant of malformed lines.
- src/facilitatorClient.ts: minimal HTTP wrapper for /fee/quote /verify
  /settle. Kept for future serve transfer + tests; not used by the
  in-process transfer path.
- src/index.ts: route `transfer` with the spec's flag matrix.
- Tests: amount.test.ts (15), receipts.test.ts (5), commands/transfer.
  test.ts (6) covering dry-run, scheme/network guards, full success
  with mocked sign+submit+wait, and SETTLE_FAILED on submit throw.

Real-chain validation (Nile testnet, 2026-04-27):
  tx 93f49d786d7a762ef679386c29830bb0b4c32f688a444d17a15349e0c357c473
  payer  TTX1Us19zqsLXhY39PPR7KRUoMa93s3J3i
  payTo  TJWdoJk8KyrfxZ2iDUqz7fwpXaMkNqPehx
  amount 0.001 USDT
  fee    0.1 USDT (transferFee from GasFree provider)
  trace  b436f4e4-f135-4968-96e8-9ce7322f5902

Test totals: 10 files, 73 tests, all green.

Operational note (will be folded into a follow-up README/solution): the
agent-wallet provider walks ~/.agent-wallet first; if a stale local_secure
config is there, schema validation throws before TRON_PRIVATE_KEY is
read. Workaround: set AGENT_WALLET_DIR to a fresh empty path so
agent-wallet falls through to its env-only EnvWalletProvider.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Code Review Report

Project: x402 / BankofAI SDK
PR: main002-bankofai-cli
Review Date: 2026-04-27
Reviewer: AI Code Reviewer (Code Review Skill v1.0.0)


PR Overview

Branch Information

Property Value
From Branch main
To Branch 002-bankofai-cli
Commits 4
Files Changed 32
Lines Added +4670
Lines Removed -1

Commit History

Hash Message
e2e7042 feat(cli): transfer command + tests for read-only command modules
4038230 feat(cli): @bankofai/x402-cli read-only commands (config / doctor / balance)
b6490b2 spec(002-bankofai-cli): D4 — lock facilitator endpoint to BankofAI hosted
c1dab1c spec(002-bankofai-cli): land draft + decisions notes for MVP

Review Summary

Verdict

Verdict: Request Changes — primarily due to three Major findings (documented env var with no implementation, nonce precision loss in the payment path, and implicit validBefore=0 SDK contract). These are straightforward fixes but touch the signing/settlement flow and the documented user interface.

Findings at a Glance

Critical Major Minor Suggestion
Count 0 4 7 3

Summary

This PR introduces @bankofai/x402-cli, a brand-new TypeScript CLI package for the x402 / BankofAI payment protocol. The scope is MVP: TRON exact_gasfree scheme only, covering config, doctor, balance, and transfer commands. The code is well-structured, follows the repo's ESM/viem/BigInt conventions closely, has solid unit test coverage (~14 test cases across all modules), and demonstrates clear attention to security basics (no key flags, stderr warnings for overrides, masked addresses by default).

No critical defects were found. The four major findings cluster around the transfer command's payment path: an implicit SDK contract around validBefore=0 that silently relies on the mechanism clamping it, a Number.parseInt nonce conversion that will silently lose precision for large nonces, message-body address fields not being normalized to EVM hex before submission (inconsistent with the domain field and with the repo's addressing conventions), and a documented env var (X402_PROFILE) that has no implementation.

Minor issues are largely DRY violations (a duplicate formatSmallestUnit), a broken CSPRNG fallback that can never work on the target platform (ESM + Node 20+), and some gaps in input validation and test isolation.


Change Summary

1. Spec & design documents

File Change Type Description
specs/002-bankofai-cli/bankofai-cli.md Added Full CLI spec (826 lines)
specs/002-bankofai-cli/notes/decisions.md Added Decision log D1–D4
docs/cli-design.md Added 7-line pointer to the spec
specs/CLAUDE.md Modified Added CLI spec to reading index

Purpose: Specification-first approach documenting all design decisions before implementation.


2. CLI package scaffolding

File Change Type Description
typescript/packages/cli/package.json Added Package manifest (@bankofai/x402-cli, v0.1.0)
typescript/packages/cli/tsconfig.json Added Extends tsconfig.base.json
typescript/packages/cli/README.md Added User-facing documentation
typescript/pnpm-workspace.yaml Modified Adds packages/mcp and packages/cli

Purpose: Wires the new package into the pnpm monorepo.


3. Core library modules

File Change Type Description
src/error.ts Added X402CliError class + ErrorCode discriminated union
src/config.ts Added Profile load/save/validate, env overrides
src/wallet.ts Added Private key reading and TRON address derivation
src/facilitator.ts Added Hosted facilitator URL resolution (D4: BankofAI-only)
src/facilitatorClient.ts Added HTTP client for /fee/quote, /verify, /settle
src/amount.ts Added Token resolution, human↔smallest-unit conversion, payment ID generation
src/output.ts Added JSON/human output envelope, runCommand helper
src/receipts.ts Added Append-only JSONL receipt store

Purpose: Foundation layer for all commands; no network access in most modules.


4. Commands

File Change Type Description
src/commands/config.ts Added init, use, get, set, list sub-commands
src/commands/doctor.ts Added Five-check diagnostics (node, wallet, facilitator, gasfree, token)
src/commands/balance.ts Added GasFree API balance query with address masking
src/commands/transfer.ts Added Direct exact_gasfree payment via TIP-712 + GasFree submit
src/index.ts Added Commander wiring, main() entry point

Purpose: User-facing commands. transfer is the only signing command in this MVP.


5. Tests

File Change Type Description
src/amount.test.ts Added 12 cases covering resolveToken, parseHumanAmount, formatSmallestUnit
src/config.test.ts Added 7 cases: roundtrip, missing file, malformed JSON, env overrides
src/facilitator.test.ts Added 6 cases: URL resolution, override escape hatch
src/output.test.ts Added Envelope building and emit tests
src/receipts.test.ts Added Append/read, malformed-line tolerance, env override
src/wallet.test.ts Added Private key reading, TRON address derivation, EVM stub
src/commands/balance.test.ts Added 5 cases including verbose, token filter, error path
src/commands/config.test.ts Added 6 cases: init, use, get, set, list, force flag
src/commands/doctor.test.ts Added 5 cases: pass/fail/skipped checks
src/commands/transfer.test.ts Added 5 cases: dry-run, scheme/network rejection, full sign+submit, error path

Purpose: Unit test suite with good happy-path and error-path coverage.


Detailed Findings


Major

[MJ-01] validBefore: 0 is an implicit SDK contract with no guard

Property Value
Severity Major
Category Correctness
File typescript/packages/cli/src/commands/transfer.ts : Lines 118–126

Description

When --valid-for is not provided, validBefore is set to 0 (Unix epoch 1970-01-01). The comment says "The mechanism overwrites nonce from the GasFree API and clamps validBefore to the network deadline window," but this is an implicit contract with X402Client/ExactGasFreeClientMechanism. If the mechanism ever fails to overwrite this field, the permit will have already expired (validBefore=0 < now), and the GasFree API will reject it. There is no fallback or assertion that the outgoing validBefore is in the future before submission.

Code

const paymentPermitContext: PaymentPermitContext = {
  meta: {
    kind: 'PAYMENT_ONLY',
    paymentId,
    nonce: '0',
    validAfter: now - 5,
    validBefore: opts.validForSeconds ? now + opts.validForSeconds : 0,  // <-- 0 = expired
  },
};

Recommendation

// Use a reasonable default deadline and let the mechanism clamp it:
const DEFAULT_VALID_SECONDS = 600; // 10 min; mechanism will clamp to network window
validBefore: opts.validForSeconds
  ? now + opts.validForSeconds
  : now + DEFAULT_VALID_SECONDS,

Alternatively, assert after createPaymentPayload that permit.meta.validBefore > now before submitting, to catch the case where the SDK did not set a deadline.


[MJ-02] nonce converted through Number.parseInt — precision loss for large nonces

Property Value
Severity Major
Category Correctness
File typescript/packages/cli/src/commands/transfer.ts : Lines 253

Description

buildGasFreeSubmitBody converts the nonce via Number.parseInt(permit.meta.nonce, 10). This caps safe precision at 2^53−1. GasFree nonces are per-account counters and can grow large over time on high-activity accounts. A nonce above Number.MAX_SAFE_INTEGER will silently round, producing a mismatched value that the GasFree API will reject as a replay or bad nonce — with no clear error message pointing to this root cause. The repo convention (typescript/conventions.md) explicitly states: "BigInt for all amounts. number is reserved for chain IDs, timestamps, and loop counters." Nonces are not in that safe list.

Code

const message = {
  ...
  nonce: Number.parseInt(permit.meta.nonce, 10),  // <-- precision loss
};

Recommendation

nonce: BigInt(permit.meta.nonce),
// or, if the GasFree API JSON serializes to number:
nonce: Number(BigInt(permit.meta.nonce)), // explicit, at least intentional

If the GasFree API endpoint requires a JSON number, document the precision constraint. If it accepts a string, use .toString().


[MJ-03] X402_PROFILE env var documented but not implemented

Property Value
Severity Major
Category Correctness / Docs
File typescript/packages/cli/src/config.ts : Lines 118–126 (applyEnvOverrides) · typescript/packages/cli/README.md : Line 84

Description

README.md documents X402_PROFILE as "Profile to load (default: nile)", but applyEnvOverrides (the only place env overrides are applied to a profile) handles X402_NETWORK, X402_SCHEME, and X402_TOKEN — not X402_PROFILE. None of the command implementations read X402_PROFILE either. A user who sets X402_PROFILE=mainnet will silently get the default profile instead, with no warning.

Code

// config.ts — applyEnvOverrides
export function applyEnvOverrides(profile: Profile): Profile {
  const env = process.env;
  return {
    ...profile,
    network: env.X402_NETWORK?.trim() || profile.network,
    scheme:  env.X402_SCHEME?.trim()  || profile.scheme,
    token:   env.X402_TOKEN?.trim()   || profile.token,
    // X402_PROFILE is never read here
  };
}

Recommendation

In each command that calls getProfile, read X402_PROFILE before the explicit opts.profile override:

const profileName = opts.profile ?? process.env.X402_PROFILE?.trim();
const { name, profile } = getProfile(cfg, profileName);

Or remove X402_PROFILE from the README until it is implemented.


[MJ-04] GasFree submit-body message fields not normalized to EVM hex

Property Value
Severity Major
Category Security / Correctness
File typescript/packages/cli/src/commands/transfer.ts : Lines 244–254

Description

buildGasFreeSubmitBody explicitly converts verifyingContract (in the domain) to EVM hex via base58ToEvmHex. However, the four address-typed fields in the message struct — token, serviceProvider, user, and receiver — are taken directly from the permit without conversion. Per the repo-wide convention and docs/solutions.md entry #1, all address fields in TIP-712 typed data must be 0x-prefixed EVM hex. If the SDK mechanism returns Base58 addresses in the PaymentPermit struct (which is plausible given the TRON context), the GasFree API submit endpoint would receive Base58 for those fields while the domain uses hex — causing a verification mismatch or an API error that is hard to diagnose.

Code

const message = {
  token:           permit.payment.payToken,   // possibly Base58
  serviceProvider: permit.fee.feeTo,          // possibly Base58
  user:            permit.buyer,              // possibly Base58
  receiver:        permit.payment.payTo,      // possibly Base58
  value:           permit.payment.payAmount,
  maxFee:          permit.fee.feeAmount,
  deadline:        String(permit.meta.validBefore),
  version:         1,
  nonce:           Number.parseInt(permit.meta.nonce, 10),
};

Recommendation

Apply base58ToEvmHex consistently to all address fields, guarding against already-hex values (the helper already handles the 0x prefix case):

const message = {
  token:           base58ToEvmHex(permit.payment.payToken),
  serviceProvider: base58ToEvmHex(permit.fee.feeTo),
  user:            base58ToEvmHex(permit.buyer),
  receiver:        base58ToEvmHex(permit.payment.payTo),
  value:           permit.payment.payAmount,
  maxFee:          permit.fee.feeAmount,
  deadline:        String(permit.meta.validBefore),
  version:         1,
  nonce:           BigInt(permit.meta.nonce),
};

Add a test that verifies the submit-body message fields are in EVM hex format when Base58 addresses are present in the permit.


Minor

[MN-01] Duplicate formatSmallestUnit implementation in balance.ts

Property Value
Severity Minor
Category Code Quality
File typescript/packages/cli/src/commands/balance.ts : Lines 93–104

Description

balance.ts contains a private formatSmallestUnit(amount: string | number, decimals: number) that duplicates the logic already in amount.ts's formatSmallestUnit(amount: bigint | string, decimals: number). The two implementations differ only in the string | number vs bigint | string input type, with balance.ts using BigInt(Math.trunc(amount)) for the number case. This is a DRY violation; if the formatting logic diverges (e.g., rounding behavior changes), one copy will drift.

Recommendation

Extend the exported formatSmallestUnit in amount.ts to accept bigint | string | number, or create a helper in amount.ts that balance.ts imports:

// amount.ts — extend the signature
export function formatSmallestUnit(amount: bigint | string | number, decimals: number): string {
  const raw = typeof amount === 'number'
    ? BigInt(Math.trunc(amount))
    : typeof amount === 'bigint' ? amount : BigInt(amount || '0');
  ...
}

[MN-02] Broken CSPRNG fallback — require() never works in ESM

Property Value
Severity Minor
Category Correctness / Code Quality
File typescript/packages/cli/src/amount.ts : Lines 133–151

Description

The randomBytes fallback (lines 141–150) attempts globalThis.require?.('node:crypto'). The package declares "type": "module" and targets Node 20+. In ESM there is no globalThis.require; the optional chaining returns undefined, so nodeCrypto is always undefined, and the code would throw 'No CSPRNG available'. Since Node 20 always provides globalThis.crypto.getRandomValues, the fallback is dead code in practice — but its comment ("Last-resort fallback for runtimes where globalThis.crypto is missing") misleads future maintainers into thinking it works. The @typescript-eslint/no-explicit-any suppression is also an indicator that the code is working around typing constraints.

Code

// Last-resort fallback — this can never work in ESM
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const nodeCrypto = (globalThis as any).require?.('node:crypto') as ...
if (!nodeCrypto) {
  throw new Error('No CSPRNG available: ...');
}

Recommendation

Remove the fallback entirely and rely on globalThis.crypto.getRandomValues, which is guaranteed on the declared target (Node 20+). If there is a genuine concern about non-Node runtimes, use a dynamic import('node:crypto') instead:

function randomBytes(n: number): Uint8Array {
  const out = new Uint8Array(n);
  // Node 20+ always provides globalThis.crypto on the main thread.
  globalThis.crypto.getRandomValues(out);
  return out;
}

[MN-03] No --to address format validation in transfer

Property Value
Severity Minor
Category Correctness
File typescript/packages/cli/src/commands/transfer.ts : Lines 93–95, 102

Description

The --to address is only checked for non-empty (trimmed). For TRON networks (the only MVP target), a valid recipient must be a Base58 string starting with T and 34 characters long. An invalid address is accepted by the CLI, passed to GasFreeAPIClient.submit, and only rejected by the remote API — with an error message that may not clearly point to the --to argument.

Recommendation

Add a lightweight client-side validation before building requirements:

if (network.startsWith('tron:') && !/^T[1-9A-HJ-NP-Za-km-z]{33}$/.test(opts.to.trim())) {
  throw new X402CliError('INVALID_INPUT', `--to '${opts.to}' is not a valid TRON Base58 address.`);
}

[MN-04] --payment-id user input not validated for correct format

Property Value
Severity Minor
Category Correctness
File typescript/packages/cli/src/commands/transfer.ts : Line 116

Description

The --payment-id option accepts any string without validating that it conforms to the protocol format: 0x + 32 lowercase hex characters (16 random bytes as bytes16). A malformed payment ID will be signed and submitted, and the GasFree API will reject it — potentially with a cryptic error. The convention is documented in .claude/rules/common/conventions.md.

Code

const paymentId = opts.paymentId?.trim() || newPaymentId();

Recommendation

const paymentId = opts.paymentId?.trim() || newPaymentId();
if (!/^0x[0-9a-f]{32}$/.test(paymentId)) {
  throw new X402CliError(
    'INVALID_INPUT',
    `--payment-id must be 0x followed by exactly 32 lowercase hex chars (got '${paymentId}').`,
  );
}

[MN-05] readReceipts reads the entire file into memory despite claiming streaming

Property Value
Severity Minor
Category Performance / Documentation
File typescript/packages/cli/src/receipts.ts : Lines 52–72

Description

The module doc comment states "Reads are streamed line-by-line so the file can grow without bloating memory," but readReceipts uses fs.readFile (loads the entire file as a string) and then splits on \n. For a long-running CLI used for many payments over time, the receipt file will grow unboundedly and each call to readReceipts (from a future receipt list command) will load it all. The comment is inaccurate and sets a false expectation.

Recommendation

Either use Node's readline interface for true line-by-line streaming, or correct the documentation to match the actual behavior and note that rotation is post-MVP:

import { createReadStream } from 'node:fs';
import { createInterface } from 'node:readline';

export async function readReceipts(filePath?: string): Promise<Receipt[]> {
  const target = filePath ?? receiptFilePath();
  // ... check for ENOENT first ...
  const rl = createInterface({ input: createReadStream(target), crlfDelay: Infinity });
  const out: Receipt[] = [];
  for await (const line of rl) { ... }
  return out;
}

[MN-06] Module-level _overrideWarned flag leaks between test runs

Property Value
Severity Minor
Category Testing
File typescript/packages/cli/src/facilitator.ts : Line 25

Description

_overrideWarned is a module-level boolean that is set to true the first time the override URL is used. Because Node's module cache is shared within a test process, once any test triggers the warning, subsequent tests won't see it — even if they independently set X402_FACILITATOR_URL_OVERRIDE. The facilitator.test.ts file does not test the warning side-effect at all, so this is currently invisible, but it will cause flaky test ordering issues if a warning assertion is added later.

Recommendation

Export a resetOverrideWarnedForTest() helper (guarded behind /* istanbul ignore */ or a process.env.NODE_ENV === 'test' check), or convert the flag to a closure/class so it can be reset in afterEach.


[MN-07] checkToken in doctor.ts only validates symbol shape, not registry membership

Property Value
Severity Minor
Category Correctness
File typescript/packages/cli/src/commands/doctor.ts : Lines 171–184

Description

The checkToken check reports ok for any token whose symbol matches /^[A-Z0-9]{2,10}$/ (e.g. GHOST on tron:nile would pass). The comment acknowledges this: "The SDK's TokenRegistry isn't currently exported as a typed structure on the public surface." However, amount.ts already calls getToken(network, symbol) from @bankofai/x402 (line 55) — so the registry is accessible. The doctor gives false confidence: a user with a mis-typed token in their profile will see ✓ token ok from doctor and then a TOKEN_NOT_FOUND error from balance or transfer.

Recommendation

Use the same getToken call that resolveToken uses:

import { getToken } from '@bankofai/x402';

function checkToken(network: string, token: string): Check {
  const info = getToken(network, token);
  if (!info) {
    return { name: 'token', status: 'fail', detail: `'${token}' not found in registry for ${network}` };
  }
  return { name: 'token', status: 'ok', detail: `${token} @ ${info.address}` };
}

Suggestions

[S-01] Implement X402_PROFILE env var support properly

File: typescript/packages/cli/src/commands/*.ts
Description: X402_PROFILE is documented in the README but never read. Since X402_NETWORK, X402_SCHEME, and X402_TOKEN all work, users will reasonably expect X402_PROFILE to also work. Adding it to applyEnvOverrides is not quite right (it's a profile-selection key, not a field within a profile); instead each command should check process.env.X402_PROFILE as a lower-priority fallback for the --profile flag, before defaulting to cfg.defaultProfile.


[S-02] Add a x402 receipt list command

File: typescript/packages/cli/src/index.ts, src/receipts.ts
Description: receipts.ts already exports readReceipts, but there is no command to surface the receipt store. The transfer command writes receipts and prints receiptPath in the result, but users have no ergonomic way to query past transactions. A receipt list [--limit N] [--json] command would complete the flow.


[S-03] Consider adding a transfer confirmation prompt before signing

File: typescript/packages/cli/src/commands/transfer.ts : Line 161
Description: The --yes flag is accepted and the comment says "currently always implicit." The CLI immediately signs and submits without any user-visible summary of what it's about to do. Even when --dry-run is not set, showing a pre-flight summary and prompting for confirmation (unless --yes is given) would significantly reduce the risk of accidental payments, especially given that GasFree transfers are not reversible.


Positive Observations

Area Observation
Error design X402CliError with a typed ErrorCode union is excellent. JSON consumers can dispatch on error.code without string parsing, and the optional hint field surfaces actionable guidance.
Security-first key handling Private keys are only accepted via env vars (never CLI flags), with explicit documentation warning about shell history. This is the correct pattern for a CLI tool.
Address masking maskAddress is applied by default in doctor and balance output, with --verbose to opt out. This is good defensive UX.
Override escape hatch X402_FACILITATOR_URL_OVERRIDE emits a stderr warning every time it's first used, ensuring the override is never silent in e2e tests or user sessions.
JSON envelope consistency Every command, success or failure, emits the same `{ ok, command, network?, scheme?, result
BigInt discipline Amounts throughout the codebase are consistently bigint. No number is used for amounts, conforming exactly to the repo conventions.
Test isolation Each test suite creates a tmpDir and cleans up after itself via afterEach, using X402_CONFIG_FILE and X402_RECEIPT_FILE env overrides to avoid touching real user state.
Dry-run path The --dry-run flag for transfer is well-implemented: it queries the GasFree API for fee info without signing or submitting, giving users a fee preview before committing.
runCommand helper The runCommand wrapper cleanly separates command logic from error handling and exit-code mapping, keeping each command body focused.
No console.log in library code All output goes through emit() / the envelope mechanism. The convention from typescript/conventions.md is honored throughout.

Checklist Results

Category Items Checked Pass Fail N/A Notes
Correctness 8 5 3 0 MJ-01 (validBefore=0), MJ-02 (nonce parseInt), MJ-03 (X402_PROFILE not implemented)
Security 9 8 1 0 MJ-04 (address field normalization in submit body); key-via-env-only is good
Performance 7 6 1 0 MN-05 (readReceipts reads entire file)
Code Quality 8 5 3 0 MN-01 (duplicate formatSmallestUnit), MN-02 (broken fallback), MN-07 (checkToken)
Testing 7 5 2 0 MN-06 (module state leak), MN-07 (doctor token check not tested against registry)
Documentation 6 5 1 0 MJ-03 (README documents unimplemented X402_PROFILE); MN-05 (misleading streaming comment)
Compatibility 5 5 0 0 ESM-only; Node >=20; pnpm workspace — all correct
Observability 4 4 0 0 Override warning on stderr; error codes in JSON; receipt store

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 (main002-bankofai-cli). Runtime behavior, integration testing, and deployment impact are not covered.


Report generated by Code Review Skill v1.0.0
Date: 2026-04-27

Closes the spec MVP: every command listed in
specs/002-bankofai-cli/bankofai-cli.md § "MVP 范围" now ships.

Commands:

- `x402 pay <url>` — wraps SDK X402FetchClient. Full HTTP 402 retry-
  with-payment loop, with --dry-run that probes the server and
  surfaces accepts[]. Decodes the PAYMENT-RESPONSE header, writes a
  receipt on success, and propagates SETTLE_FAILED for non-2xx after
  retry.

- `x402 serve transfer` — minimal HTTP server (Node's http module, no
  framework dependency). Endpoints: GET /health, GET
  /.well-known/x402-transfer, GET|POST /pay (issues 402 challenge,
  caches per paymentId for 5 min, validates payload.accepted vs
  issued, settles via the same in-process GasFreeAPIClient.submit +
  waitForSuccess path as `transfer`). Anti-tampering check matches
  scheme/network/asset/amount/payTo. SIGINT/SIGTERM trigger graceful
  shutdown.

- `x402 receipt list / show / export` — read-only over the JSONL
  store. List supports --profile / --network / --scheme / --token /
  --from / --to / --limit filters. Show finds by paymentId or tx
  hash. Export emits json or csv.

Real-chain validation (TRON Nile, 2026-04-27):
  pay  ↔ serve  end-to-end loop on http://127.0.0.1:4322/pay:
    challenge issued, payload signed, GasFree submit/wait
    Nile tx 350d129340daefbc7b08122d0e31c58d9c89dba6ca5d2c0bd7dcb3258d271bdf
    both serve-transfer and pay receipts written, identical tx hash.

Tests: 93 total across 13 files. New tests:
- src/commands/pay.test.ts (6): URL guard, EVM rejection, --dry-run
  paths, full success with mocked X402FetchClient + receipt assertion,
  SETTLE_FAILED on 5xx after retry.
- src/commands/serve.test.ts (4): validation guards (--pay-to /
  network / scheme), live HTTP probe of /health + .well-known +
  402-on-/pay; uses SIGTERM to clean up.
- src/commands/receipt.test.ts (10): list with filters + --limit,
  show by paymentId/tx, export json/csv, RECEIPT_NOT_FOUND.

Spec MVP scoreboard:
  ✓ config (init/use/get/set/list)   shipped in PR #70
  ✓ doctor                            shipped in PR #70
  ✓ balance --gasfree                 shipped in PR #70
  ✓ transfer                          shipped in e2e7042
  ✓ pay                               shipped here
  ✓ serve transfer                    shipped here
  ✓ receipt list/show/export          shipped here

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@boboliu-1010
Copy link
Copy Markdown
Collaborator Author

MVP closed (commits e2e7042 + 5aa41b6 just pushed)

All 7 spec MVP commands ship + real-chain validated on TRON Nile.

Real-chain evidence (2026-04-27, Nile testnet)

Command Tx hash Trace
transfer 93f49d786d7a762ef679386c29830bb0b4c32f688a444d17a15349e0c357c473 b436f4e4-f135-4968-96e8-9ce7322f5902
payserve transfer 350d129340daefbc7b08122d0e31c58d9c89dba6ca5d2c0bd7dcb3258d271bdf 9c46b375-7289-466e-928a-54ee1e976ff0

pay and serve transfer were validated end-to-end against each other: server issued 402, client signed, server validated payload.accepted vs the issued challenge, both halves wrote receipts, single shared tx hash.

Test coverage

Module Tests
output / errors / config / facilitator 19
wallet / amount / receipts 27
commands/config 12
commands/doctor 4
commands/balance 5
commands/transfer 6
commands/pay 6
commands/serve 4
commands/receipt 10
Total 93 (13 files)

Shipped commands

x402 config init / use / get / set / list
x402 doctor [--profile <name>] [--network <id>]
x402 balance [--token <sym>] [--verbose]
x402 transfer --to <addr> --amount <decimal> [--token <sym>] [--dry-run]
x402 pay <url> [--method] [--header k:v] [--body s] [--dry-run]
x402 serve transfer --pay-to <addr> --amount <decimal> [--token <sym>] [--port <n>]
x402 receipt list [--profile|--network|--scheme|--token|--from|--to|--limit]
x402 receipt show <paymentId|tx-hash>
x402 receipt export --format <json|csv>

Architectural notes worth flagging for review

  1. No facilitator HTTP for transfer / serve transfer. D4 in notes/decisions.md planned to call /fee/quote /verify /settle against https://facilitator.bankofai.io/<network>, but that hosted endpoint serves only the GasFree proxy — not the full facilitator surface. The CLI's transfer and serve transfer go in-process: build the PaymentPermit through the SDK, then submit straight to GasFree via GasFreeAPIClient.submit + waitForSuccess. examples/facilitator/ (the local one) keeps its role for our e2e harness; we don't pull it into the CLI runtime.
  2. agent-wallet config conflict. When ~/.agent-wallet/wallets_config.json carries an old local_secure entry (no params), Zod validation throws before our env-key fallback runs. Workaround documented for now: set AGENT_WALLET_DIR to a fresh directory. We may want a CLI-side x402 doctor --fix-wallet-config or migration helper later.
  3. Single GasFree provider on Nile. TooManyPendingTransferException happens when its queue is saturated. The CLI surfaces this as SETTLE_FAILED; user-side retry works.

What's intentionally NOT in this PR

  • inspect, request (post-MVP per spec § "MVP 范围")
  • EVM (exact, exact_permit) signing path — lands when EVM wallet derivation does
  • Receipt rotation / --receipt-db sqlite:
  • serve transfer --public-url tunnel integration

Ready for review.

Followup to the 2026-04-27 Nile evaluation. Two GasFree quirks were
worth correcting in user-facing code rather than just documenting.

balance: query the chain directly, not GasFree API
- New src/onchain.ts: getTrc20Balance() hits the Tron full-node
  /wallet/triggerconstantcontract endpoint with hand-built calldata.
  No tronweb abi-encoder, no signature, no broadcast.
- balance command now surfaces both chainBalance + apiBalance per
  asset, plus an apiBalanceStale flag and a stderr warning when the
  two disagree. Verified live: chain=23.2948 USDT vs api=0 → stale
  warning fired correctly.
- New balance test covers the stale path (chain ≠ api → warning + flag).

transfer --dry-run: surface GasFree fee economics
- Computes feeAsPercentageOfAmount = (transferFee + activateFee) /
  amount. Also emits a stderr WARNING when fees ≥ 10% of payment.
  At our default 0.001 USDT test amount the warning is 10000.0% — loud
  on purpose; users should know GasFree has flat per-tx fees and is
  uneconomical for sub-dollar payments.

Documentation:
- docs/solutions.md #11: GasFree assets[].balance can lag the chain;
  always read from the contract.
- docs/solutions.md #12: GasFree fee structure is flat-per-tx; small
  payments have huge fee/amount ratios. Recommends switching to
  ERC-3009 exact for micropayments.
- typescript/packages/cli/README.md: new "GasFree economics" section
  before the configuration reference, with the break-even rule of
  thumb and a pointer to the new solutions entries.

Tests: 94 (was 93). All green. balance.test.ts gained one stale-flag
test; transfer/dry-run continues to assert the wrapped envelope shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Code Review Report

Project: x402 / BankofAI SDK
PR: main002-bankofai-cli
Review Date: 2026-04-27
Reviewer: AI Code Reviewer (Code Review Skill v1.0.0)


PR Overview

Branch Information

Property Value
From Branch main
To Branch 002-bankofai-cli
Commits 6
Files Changed 40
Lines Added +6209
Lines Removed -1

Commit History

Hash Message
a0f4dc3 fix(cli): chain-balance read in balance, fee% warning in transfer
5aa41b6 feat(cli): pay / serve transfer / receipt — MVP closed
e2e7042 feat(cli): transfer command + tests for read-only command modules
4038230 feat(cli): @bankofai/x402-cli read-only commands (config / doctor / balance)
b6490b2 spec(002-bankofai-cli): D4 — lock facilitator endpoint to BankofAI hosted
c1dab1c spec(002-bankofai-cli): land draft + decisions notes for MVP

Review Summary

Verdict

Verdict: Request Changes

Findings at a Glance

Critical Major Minor Suggestion
Count 0 1 6 4

Summary

This PR introduces the @bankofai/x402-cli package — a brand-new command-line tool with six commands (config, doctor, balance, transfer, pay, serve transfer) plus a local receipt store and a facilitator URL resolver. The code is impressively well-structured for a greenfield CLI: clean separation of concerns, a consistent runCommand envelope pattern, proper use of BigInt throughout, thorough input validation, no hardcoded secrets in library code, and unit tests for every command module. The documented decisions (D1D4) and two new solutions.md entries (#11, #12) are excellent additions to the shared knowledge base.

One Major defect stands out: the pay command's receipt is hollow — it records freshly-generated, unrelated paymentId bytes and leaves payTo, token, asset, amount, and amountDisplay as empty strings. This makes the receipt store useless for financial reconciliation after any pay invocation, which is the primary audit trail for end-users and agents.

Six Minor issues are also flagged: duplicated settlement-body helpers across transfer.ts and serve.ts, an unbounded graceful-shutdown wait in serve transfer, a zero-valued validBefore in issued challenges, silent JSONL corruption tolerance, a module-level singleton that complicates test isolation, and several magic numbers that should be named constants.


Change Summary

1. New CLI package — @bankofai/x402-cli

File Change Type Description
typescript/packages/cli/src/index.ts Added Entry point; routes commander subcommands, drains stdout before exit
typescript/packages/cli/src/config.ts Added Profile file load/save/validate; applyEnvOverrides
typescript/packages/cli/src/wallet.ts Added Reads TRON_PRIVATE_KEY / EVM_PRIVATE_KEY, derives address via TronWeb
typescript/packages/cli/src/amount.ts Added resolveToken, parseHumanAmount, formatSmallestUnit, newPaymentId
typescript/packages/cli/src/onchain.ts Added Live TRC-20 balanceOf via /wallet/triggerconstantcontract
typescript/packages/cli/src/facilitator.ts Added Derives facilitator URL from network; single X402_FACILITATOR_URL_OVERRIDE escape hatch
typescript/packages/cli/src/facilitatorClient.ts Added Thin HTTP wrapper for /fee/quote, /verify, /settle
typescript/packages/cli/src/receipts.ts Added Append-only JSONL receipt store at ~/.x402/receipts.jsonl
typescript/packages/cli/src/output.ts Added Stable JSON envelope + human-readable renderer; runCommand helper
typescript/packages/cli/src/error.ts Added X402CliError with typed ErrorCode union
typescript/packages/cli/src/commands/config.ts Added init / use / get / set / list sub-commands
typescript/packages/cli/src/commands/doctor.ts Added Node-version, wallet, facilitator-ping, GasFree, token-registry checks
typescript/packages/cli/src/commands/balance.ts Added Chain + GasFree API balance with stale-data detection
typescript/packages/cli/src/commands/transfer.ts Added TRON exact_gasfree direct transfer; TIP-712 sign + GasFree submit
typescript/packages/cli/src/commands/pay.ts Added 402 auto-pay via X402FetchClient; receipt append
typescript/packages/cli/src/commands/serve.ts Added Minimal HTTP collection server; challenge store; GasFree settlement
typescript/packages/cli/src/commands/receipt.ts Added list / show / export over local JSONL store
typescript/packages/cli/src/*.test.ts (8 files) Added Unit tests for every module; vi.spyOn mocks for SDK and onchain.ts

Purpose: MVP CLI for BankofAI x402; covers TRON exact_gasfree end-to-end.

2. Package infrastructure

File Change Type Description
typescript/packages/cli/package.json Added @bankofai/x402-cli v0.1.0; commander, tronweb, @bankofai/agent-wallet deps
typescript/packages/cli/tsconfig.json Added Extends tsconfig.base.json; excludes test files from build
typescript/pnpm-workspace.yaml Modified Adds packages/mcp and packages/cli to workspace
typescript/pnpm-lock.yaml Modified Lock-file additions for new packages

3. Specs and documentation

File Change Type Description
specs/002-bankofai-cli/bankofai-cli.md Added Full feature spec: commands, error codes, decisions
specs/002-bankofai-cli/notes/decisions.md Added ADRs D1–D4
specs/CLAUDE.md Modified Adds CLI spec pointer; clarifies spec.md or <slug>.md naming
docs/cli-design.md Added Single-pointer page to specs/002-bankofai-cli/
docs/solutions.md Modified Adds entries #11 (GasFree balance lag) and #12 (flat-fee economics)

Detailed Findings


Major

[MJ-01] pay command writes an empty, unanchored receipt after every successful payment

Property Value
Severity Major
Category Correctness
File typescript/packages/cli/src/commands/pay.ts : Lines 141–167

Description

When cmdPay receives a successful PAYMENT-RESPONSE header it constructs a Receipt object and appends it to ~/.x402/receipts.jsonl. However:

  1. Wrong paymentId: The receipt calls newPaymentId() — generating a fresh random 16-byte ID — rather than the actual payment ID that was signed in the TIP-712 permit by the SDK's X402Client. The receipt's paymentId will never match the on-chain transaction or any external lookup.

  2. All financial fields are empty strings: payTo, token, asset, amount, and amountDisplay are hard-coded to ''. A receipt with no recipient, no token, and no amount is useless for reconciliation, auditing, or receipt show <id>.

The pay command is the most common user-facing flow; its receipts are the primary financial trail. Writing dummy receipts without correcting them corrupts the local store.

Code

// pay.ts lines 142–166
const receipt: Receipt = {
  paymentId: newPaymentId(),   // ← generates a fresh unrelated ID
  command: 'pay',
  ...
  payer,
  payTo: '',           // ← no recipient recorded
  token: '',           // ← no token recorded
  asset: '',           // ← no asset recorded
  amount: '',          // ← no amount recorded
  amountDisplay: '',   // ← no amount display recorded
  settlement: {
    success: true,
    transaction: paymentResponse.transaction,
  },
  extra: { url: opts.url, status: response.status, paymentResponse },
};
await appendReceipt(receipt);

Recommendation

The X402FetchClient/X402Client flow does not currently surface the signed paymentId or the resolved PaymentRequirements back to the caller. The most practical fix is to:

  1. Capture the PAYMENT-REQUIRED header on the initial 402 response (before the client retries) and parse it to extract accepts[0] (payTo, token, asset, amount).
  2. Extract paymentId from the paymentPermitContext that the SDK encodes in the PAYMENT-SIGNATURE header it sends on the retry — this header is readable by the CLI wrapping the fetch.
  3. Fall back to paymentResponse.transaction as the display key if the paymentId is not recoverable, and mark the receipt paymentId as tx:<hash> to distinguish it from a genuine permit ID.

At minimum, parse the PAYMENT-REQUIRED 402 header in the dry-run probe path and persist the first accepts entry so the financial fields are populated:

// Capture the 402 response before the SDK retry to extract requirements.
const accepts = pickAccepts(headerJson) as PaymentRequirements[] | null;
const req0 = accepts?.[0];
// ...
const receipt: Receipt = {
  paymentId: /* extracted from PAYMENT-SIGNATURE or generate */ ...,
  payTo:   req0?.payTo   ?? '',
  token:   req0?.asset   ?? '',   // use token registry lookup for symbol
  asset:   req0?.asset   ?? '',
  amount:  req0?.amount  ?? '',
  amountDisplay: req0 ? formatSmallestUnit(req0.amount, decimals) : '',
  ...
};

Minor

[MN-01] buildGasFreeSubmitBody and base58ToEvmHex duplicated verbatim in transfer.ts and serve.ts

Property Value
Severity Minor
Category Code Quality
File typescript/packages/cli/src/commands/transfer.ts : Lines 846–873 and typescript/packages/cli/src/commands/serve.ts : Lines 569–596

Description

Both transfer.ts and serve.ts define character-for-character identical buildGasFreeSubmitBody(network, permit) and base58ToEvmHex(address) functions. Any future bug fix or protocol change (e.g., a GasFree API schema update) must be made in both places, or the two commands will diverge silently.

Code

// Identical in both transfer.ts and serve.ts:
function buildGasFreeSubmitBody(network: string, permit: PaymentPermit) { ... }
function base58ToEvmHex(address: string): string { ... }

Recommendation

Extract both helpers into a shared internal module, e.g., typescript/packages/cli/src/gasfree-utils.ts, and import from there in both command files. The onchain.ts module already demonstrates this pattern for another TRON-specific helper.


[MN-02] serve transfer has no graceful-shutdown timeout — hung in-flight requests block forever

Property Value
Severity Minor
Category Correctness
File typescript/packages/cli/src/commands/serve.ts : Lines 399–407

Description

On SIGINT / SIGTERM, cmdServeTransfer calls server.close() and awaits its callback. server.close() stops accepting new connections but waits indefinitely for all existing connections to drain. If an in-flight GasFree waitForSuccess call hangs (e.g., network partition, GasFree API unresponsive), the process will never exit. Users pressing Ctrl-C twice will send SIGTERM again, which is now unregistered (process.once), so the second signal terminates the process abruptly instead.

Code

// serve.ts lines 399–407
await new Promise<void>((resolve) => {
  const shutdown = () => {
    process.stderr.write('[x402 serve] shutting down\n');
    clearInterval(cleanupTimer);
    server.close(() => resolve());   // ← no timeout guard
  };
  process.once('SIGINT', shutdown);
  process.once('SIGTERM', shutdown);
});

Recommendation

Add a forced-exit timeout after server.close():

server.close(() => resolve());
setTimeout(() => {
  process.stderr.write('[x402 serve] shutdown timeout — forcing exit\n');
  resolve();
}, 10_000); // 10s hard limit

[MN-03] serve.ts issued challenge sets validBefore: 0, which is undefined behaviour for non-SDK clients

Property Value
Severity Minor
Category Correctness / Documentation
File typescript/packages/cli/src/commands/serve.ts : Lines 462–475

Description

When serve transfer issues a 402 challenge it populates paymentPermitContext.meta.validBefore = 0. The intent (consistent with how transfer.ts uses the same sentinel) is that the client-side mechanism will compute a network-appropriate deadline. However, 0 as "let the mechanism decide" is an undocumented convention internal to the BankofAI SDK; the x402 spec (specs/schemes/exact-gasfree.md) defines validBefore as a Unix timestamp, not a sentinel. Any third-party or future client that interprets 0 literally as epoch-zero would generate a permit that the GasFree API rejects with a deadline-out-of-range error.

Code

// serve.ts lines 462–475
extensions: {
  paymentPermitContext: {
    meta: {
      kind: 'PAYMENT_ONLY',
      paymentId,
      nonce: '0',
      validAfter: Math.floor(Date.now() / 1000) - 5,
      validBefore: 0,   // ← sentinel, not documented in the wire spec
    },
  },
},

Recommendation

Either (a) compute a real deadline here — e.g., Math.floor(Date.now() / 1000) + 300 (5-minute window, within GasFree's [50, 3600] testnet bounds) — so the challenge is self-describing, or (b) document the 0 sentinel explicitly in the spec (specs/schemes/exact-gasfree.md) and add a comment in the code referencing that spec section.


[MN-04] Malformed JSONL lines silently skipped in readReceipts — no stderr indicator

Property Value
Severity Minor
Category Observability / Code Quality
File typescript/packages/cli/src/receipts.ts : Lines 1241–1248

Description

readReceipts ignores any line that fails JSON.parse with no feedback to the user. While the comment justifies this as tolerating a hypothetical corruption scenario, in practice there are two failure modes that would cause silent data loss: (a) a partial write during a crash (last line truncated) and (b) a receipts file written by a future CLI version with a breaking schema change. In both cases the user has no way to know how many receipts were dropped.

Code

// receipts.ts lines 1242–1248
try {
  out.push(JSON.parse(trimmed) as Receipt);
} catch {
  // Skip malformed lines — append-only file should never have them, but
  // be tolerant rather than wedge the whole `receipt list` command.
}

Recommendation

Emit a process.stderr.write warning for each skipped line and return a skippedCount in the result so callers can surface it to the user:

} catch {
  skipped++;
  process.stderr.write(`[x402] receipts: skipping malformed line at offset ${lineNo}: ${trimmed.slice(0,80)}\n`);
}

[MN-05] Module-level _overrideWarned singleton in facilitator.ts causes test cross-contamination

Property Value
Severity Minor
Category Testing / Code Quality
File typescript/packages/cli/src/facilitator.ts : Line 430

Description

_overrideWarned is a module-level let that persists across test cases in the same process. If one test calls getFacilitatorBaseUrl with X402_FACILITATOR_URL_OVERRIDE set, subsequent tests in facilitator.test.ts (or any other test file that imports the module) will no longer see the one-time warning. This makes tests that assert the warning message brittle and order-dependent.

Code

// facilitator.ts line 430
let _overrideWarned = false;

Recommendation

Expose a resetOverrideWarned() function (exported for test use only, e.g., guarded by /* @internal */) or make the warning unconditional in test mode via a dependency-injected warn callback. At minimum, add a beforeEach reset in facilitator.test.ts:

// facilitator.test.ts
import { resetOverrideWarned } from '../facilitator.js';
beforeEach(() => resetOverrideWarned());

[MN-06] Magic numbers -5 (clock skew) and 180 (maxTimeoutSeconds) used without named constants

Property Value
Severity Minor
Category Code Quality / Maintainability
File typescript/packages/cli/src/commands/transfer.ts : Lines 728, 716 and typescript/packages/cli/src/commands/serve.ts : Lines 449, 470

Description

The clock-skew padding (- 5 seconds) and the challenge timeout (maxTimeoutSeconds: 180) appear independently in transfer.ts and serve.ts without shared constants. The -5 sentinel is also semantically coupled to the GasFree deadline clamping logic documented in solutions.md #2 and .claude/rules/schemes/exact-gasfree.md, which requires [55, 595] for mainnet and [55, 3595] for testnet. Divergence in any one place will introduce subtle deadline-window bugs.

Code

// transfer.ts line 728, serve.ts line 470
validAfter: now - 5,             // magic: 5s clock-skew pad
// transfer.ts line 716, serve.ts line 449
maxTimeoutSeconds: 180,          // magic: 3-minute window

Recommendation

Define constants in amount.ts or a dedicated constants.ts:

export const CLOCK_SKEW_PAD_SECONDS = 5;
export const DEFAULT_MAX_TIMEOUT_SECONDS = 180;

Suggestions

[S-01] Test files use a possibly-real private key as a fixture constant

File: typescript/packages/cli/src/commands/balance.test.ts (and 5 other test files)
Description: SAMPLE_KEY = '0xddb8ff7605526a250bd37f5c3733badf9860f8708e808b79f40f8c56470004ba' is used across all command test files. The key itself is not labelled a well-known test vector. Readers cannot tell at a glance whether this is a dedicated throw-away key or a key that has ever held real funds.
Suggestion: Replace with a key explicitly documented as a zero-balance test fixture — e.g., the all-aa test private key 0xaaaa...aa (64 chars), or a key generated specifically for this repo's test suite with a comment linking to a public TronScan address showing a zero balance. Add a // TRON test-only key — zero balance, public comment.


[S-02] In-memory challenge store in serve.ts has no capacity cap

File: typescript/packages/cli/src/commands/serve.ts
Description: challenges is an unbounded Map. The 5-minute TTL and 60-second cleanup interval bound the steady-state size for legitimate traffic, but if the server is exposed beyond localhost (--host 0.0.0.0), a client making one unanswered request per second could accumulate 18 000 pending challenges before the first cleanup pass, each holding a full PaymentRequirements object.
Suggestion: Add a soft cap (e.g., 10 000 entries) that drops the oldest challenge when exceeded, and log a warning. Alternatively, document that --host 0.0.0.0 is unsupported for production use in this MVP.


[S-03] decodeMessage in onchain.ts uses Node-specific Buffer API

File: typescript/packages/cli/src/onchain.ts : Lines 1008–1014
Description: The error-message decoder uses Buffer.from(hex, 'hex').toString('utf8'), which is a Node.js-specific API, making the module harder to port to edge runtimes.
Suggestion: Use the Web-standard approach, which is already available in Node 20+:

const bytes = Uint8Array.from({ length: hex.length / 2 }, (_, i) =>
  parseInt(hex.slice(i * 2, i * 2 + 2), 16));
return new TextDecoder().decode(bytes);

[S-04] FacilitatorHttpClient in facilitatorClient.ts is defined but unused in MVP commands

File: typescript/packages/cli/src/facilitatorClient.ts
Description: The FacilitatorHttpClient class (with feeQuote, verify, settle methods) is fully implemented and tested but not imported by any command in this PR. transfer.ts and serve.ts call gasFreeClient.submit() directly; pay.ts delegates entirely to X402FetchClient. The file is scaffolding for a post-MVP EVM path but could confuse contributors who assume it is active.
Suggestion: Add a // NOTE: used by future EVM transfer/pay path; not wired up in MVP header comment so the intent is clear and the dead-import path is not flagged by lint.


Positive Observations

Area Observation
BigInt discipline All amounts flow as BigInt end-to-end; no floating-point conversions anywhere in the changed code. parseHumanAmount and formatSmallestUnit handle the boundary cleanly.
Error envelope pattern runCommand / buildSuccess / buildFailure gives every command a uniform JSON envelope with typed error codes, making the CLI readily consumable by agents parsing --json output.
No hardcoded secrets No credentials, tokens, or API keys in library code. TRON_PRIVATE_KEY is read from env; the hint message explicitly warns against shell-history exposure.
GasFree balance fix onchain.ts correctly bypasses the GasFree API's stale balance field (solutions.md #11) by issuing a live triggerConstantcontract balanceOf call, and the balance command surfaces both chainBalance and apiBalance so users can detect lag.
Address normalization wallet.ts correctly uses TronWeb.address.toHex(base58).replace(/^41/, '') to produce a 0x-prefixed 20-byte EVM hex address, conforming to the TRON convention required by solutions.md #1.
Fee transparency transfer --dry-run computes feeAsPercentageOfAmount and emits a prominent stderr warning when the GasFree relayer fee exceeds 10 % of the payment — exactly the economic pitfall documented in solutions.md #12.
Facilitator URL locking (D4) facilitator.ts derives the URL from the network identifier, never from user-supplied config, with a single escape hatch (X402_FACILITATOR_URL_OVERRIDE) that always emits a warning. This prevents user misconfiguration from silently routing payments to a wrong facilitator.
Test coverage breadth Unit tests exist for every module including edge cases (amount parsing, masking, JSONL round-trip, GasFree stale-balance detection).
doctor design Each diagnostic check is independent (never throws past its own failure) and produces a structured { name, status, detail } result, making the command reliable even when some checks fail.
spec + decisions colocation specs/002-bankofai-cli/notes/decisions.md documents D1–D4 inline with the implementation, providing future contributors with immediate context for why the facilitator URL is locked and why the wallet is env-var-only.

Checklist Results

Category Items Checked Pass Fail N/A Notes
Correctness 8 7 1 0 Receipt completeness (MJ-01)
Security 10 9 0 1 No secrets in code; private key handling is intentional env-var pattern
Performance 6 6 0 0 Promise.all for parallel balance queries; JSONL full-read is post-MVP known limitation
Code Quality 8 5 3 0 Duplication (MN-01), magic numbers (MN-06), unused import keepalives
Testing 7 6 0 1 Good coverage; SAMPLE_KEY annotation gap (S-01)
Documentation 6 6 0 0 All new public APIs documented; decisions.md; solutions.md updated
Compatibility 5 5 0 0 engines.node >= 20 declared; ESM throughout
Observability 4 3 1 0 Silent JSONL skip (MN-04); other errors surface cleanly

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-04-27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant