Skip to content

feat: add standard CLI mode with JSON output for AI agents and automation#921

Merged
gummy789j merged 60 commits intotronprotocol:release_v4.9.5from
parsoncryptoai:develop
Apr 17, 2026
Merged

feat: add standard CLI mode with JSON output for AI agents and automation#921
gummy789j merged 60 commits intotronprotocol:release_v4.9.5from
parsoncryptoai:develop

Conversation

@parsoncryptoai
Copy link
Copy Markdown

Summary

  • Standard CLI mode: Non-interactive, scriptable CLI (java -jar wallet-cli.jar --network nile get-account --address TXyz...) alongside the existing
    REPL mode. Supports --output json for structured output, --network for network selection, and --quiet flag. Designed for AI agents, scripts, and
    CI/CD pipelines.
  • Command framework: CommandRegistry/CommandDefinition pattern with fluent builder API. Commands organized by domain in cli/commands/ (Query,
    Transaction, Wallet, Staking, Contract, Exchange, Proposal, Witness, Misc). Supports aliases, typed options, and fuzzy command suggestion on typos.
  • JSON output envelope: All commands produce {"success":true,"data":...} or {"success":false,"error":"..."} in JSON mode, with stdout/stderr
    suppression to guarantee machine-parseable output.
  • Active wallet management: Persistent active wallet selection via set-active-wallet / list-wallets commands, stored in
    Wallet/active_wallet.conf.
  • QA verification suite: Shell-based parity tests (qa/) comparing REPL vs standard CLI output across text and JSON modes, with semantic comparison
    for format-independent validation.
  • Transfer USDT command: New transfer-usdt command for TRC20 USDT transfers with automatic contract address resolution per network.

Test plan

  • Build passes: ./gradlew build
  • Standard CLI text mode: java -jar build/libs/wallet-cli.jar --network nile get-account --address <addr>
  • Standard CLI JSON mode: java -jar build/libs/wallet-cli.jar --output json --network nile get-account --address <addr>
  • REPL mode still works: ./gradlew run
  • QA parity tests pass: TRON_TEST_APIKEY=<key> bash qa/run.sh verify
  • Active wallet commands: set-active-wallet, list-wallets
  • Transfer USDT: transfer-usdt --to <addr> --amount 1

HarukaMa and others added 11 commits April 1, 2026 15:47
Add a non-interactive standard CLI framework (--output json, --private-key,
--mnemonic flags) alongside the existing interactive REPL. Includes a bash
harness that verifies all 120+ commands across help, text, JSON, on-chain
transactions, REPL parity, and wallet management (321 tests, 315 pass, 0 fail).

Key changes:
- New cli/ package: StandardCliRunner, OutputFormatter, CommandRegistry,
  and per-domain command files (Query, Transaction, Staking, etc.)
- JSON mode suppresses stray System.out/err from WalletApi layer so only
  structured OutputFormatter output reaches stdout
- Remove debug print from AbiUtil.parseMethod() that contaminated stdout
- Harness scripts (harness/) for automated three-way parity verification
- Updated .gitignore for runtime artifacts

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

Rename harness/ → qa/ across shell scripts, Java classes, docs, and build config.
Fix vote-witness QA test that extracted keystore address instead of witness address
by filtering "keystore" lines from list-witnesses output. Add lock/unlock commands,
improve GlobalOptions parsing, and update CLAUDE.md baseline.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d auth options

Move JSON stream suppression earlier in StandardCliRunner to cover network
init and authentication (not just command execution), remove --private-key
and --mnemonic global options, and update QA/plan docs to reflect current
test baseline (321 tests, 314 passed, 1 failed, 6 skipped).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Simplify protobuf formatting to use Utils.formatMessageString for both
modes, suppress info messages in JSON mode, and update spec/plan docs
to clarify the strict JSON-only contract.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add text+JSON parity, JSON field validation, round-trip verification
(set then get-active-wallet), and error case tests (no args, both args,
invalid address) for wallet management commands.
Replace login/logout/backup/export commands with list-wallet, set-active-wallet,
and get-active-wallet for multi-wallet support. Implement transfer-usdt with
automatic energy estimation. Update CLAUDE.md docs and add QA tests for new
transaction commands.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Skip unit tests and QA verification when no code files (src/, build.gradle, *.java) have changed. Consolidates two separate hook blocks into one.
… to JSON errors

WalletApi.listWallets filtered to skip the .active-wallet config file,
which was incorrectly treated as a wallet keystore. OutputFormatter.error()
and usageError() now include "success": false in JSON output for consistent
envelope format.
@lxcmyf
Copy link
Copy Markdown
Contributor

lxcmyf commented Apr 3, 2026

This PR is currently under review.
Separately, wallet-cli also has some related MCP plans on our side. The general idea is to expose selected wallet-cli capabilities as MCP services, so they can be called directly from AI chat interfaces or other agent-based workflows, instead of being limited to traditional interactive CLI usage. We are still working through the design and scope, and we will share updates as things move forward.

HarukaMa and others added 3 commits April 3, 2026 17:05
…defaults

- OutputFormatter.success() now wraps jsonData in {"success":true,"data":{...}}
  envelope for consistent API response format
- OutputFormatter.protobuf() outputs single-line valid JSON via
  JsonFormat.printToString in JSON mode (was using formatJson which
  introduced illegal newlines inside string values)
- deploy-contract defaults token-id to empty string (matching REPL behavior)
  and origin-energy-limit to 1 (TRON requires > 0)
- add --force support for reset-wallet and clear-wallet-keystore
- de-interactivize standard CLI transaction signing with permission-id support
- align wallet command JSON schema with success/data envelope
- add standard CLI change-password command and QA coverage
- improve QA command counting, skip reasons, and stale result cleanup
- add --case support to qa/run.sh for targeted reruns
- strengthen transaction QA side-effect checks
- make send-coin JSON return txid and verify send-coin-balance via tx receipt fallback
- update QA reports and fix report documentation
@3for
Copy link
Copy Markdown

3for commented Apr 7, 2026

Right now the output json does not implement the unified envelope as described in the PR summary. Only success() emits {"success": true, "data": ...}; result() emits {success, message}, and printMessage()/raw()/keyValue() bypass the envelope entirely. This affects real commands, not just edge cases: get-account still uses printMessage(), register-wallet uses raw(), and many transaction/contract commands use result(). In its current state, JSON mode does not provide a stable machine-parseable contract.

  • JSON output envelope: All commands produce {"success":true,"data":...} or {"success":false,"error":"..."} in JSON mode, with stdout/stderr
    suppression to guarantee machine-parseable output.

@3for
Copy link
Copy Markdown

3for commented Apr 7, 2026

It says the active wallet config is stored at Wallet/active_wallet.conf, but the implementation actually uses Wallet/.active-wallet (ActiveWalletConfig.java (line 15)).

  • Active wallet management: Persistent active wallet selection via set-active-wallet / list-wallets commands, stored in
    Wallet/active_wallet.conf.

Comment thread src/main/java/org/tron/qa/QARunner.java Outdated
CommandCapture cap = new CommandCapture();
cap.startCapture();
try {
String[] cliArgs = {"--network", network, "--mnemonic", mnemonic, cmdName};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the private key / mnemonic as process arguments is a serious security issue. In QARunner, values are constructed into CLI args like --private-key <key> and --mnemonic <words>, which makes them visible via process listings such as ps on Linux/macOS. That is not acceptable on shared machines or in CI environments. These secrets should be passed via environment variables or stdin instead of command-line arguments. (QARunner.java:130, 144, 301)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think the ps exposure concern applies here. QARunner does not spawn a subprocess with --private-key/--mnemonic; it passes an in-memory String[] directly into GlobalOptions.parse() within the same JVM. That said, this QA path is stale and should be cleaned up separately because it still assumes old global auth flags.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gummy789j Agreed. QARunner does not expose these values via ps. However, from a secret-handling perspective, this approach is still not ideal: both values are kept as Strings in process memory and could be exposed via heap dumps, crash artifacts, or debugging tools.

Also, it looks like the entire QARunner.java file was newly introduced in this PR. Since it’s described as “this QA path is stale”, would it make sense to clean it up as part of this PR?

Additionally, for external calls like qa/config.sh that run java -jar ... --private-key/--mnemonic, there is still a real risk of leaking secrets via ps.

Repro steps

  1. In terminal A, start a polling ps watcher:

while true; do
ps axww -o pid=,command= | grep '[w]allet-cli.jar' | grep 'import-wallet'
sleep 0.05
done

  1. In terminal B, run:

bash qa/run.sh verify

Then paste the private key when prompted:


$ bash qa/run.sh verify
TRON_TEST_APIKEY not set. Please enter your Nile testnet private key:
*****THE_ACTUAL_PRIVATE_KEY******
TRON_TEST_MNEMONIC not set (optional). Mnemonic tests will be skipped.
=== Wallet CLI QA — Mode: verify, Network: nile ===

Building wallet-cli...
Build complete.

Phase 1: Setup & connectivity check...
✓ nile connectivity OK
Standard CLI commands: 118

.........

  1. You can observe multiple entries with the private key in terminal A:

15456 java -jar build/libs/wallet-cli.jar --network nile import-wallet --private-key *****THE_ACTUAL_PRIVATE_KEY******

The same applies if TRON_TEST_MNEMONIC is set — the mnemonic can also be observed in ps output.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that String-based in-memory secret handling is not ideal. For QARunner, though, this is a local, short-lived QA helper running in the same JVM, so I do not view it as the same severity as the external argv/ps exposure.

The concrete reproducible issue here was the QA shell path passing secrets to an external java -jar ... process, and that is fixed now. I am not reworking QARunner’s internal in-memory secret handling in this PR because that would be a broader QA/auth handoff redesign rather than a localized fix.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gummy789j This leaves qa/run.sh java-verify on a stale code path. QARunner still builds --private-key / --mnemonic global args, but GlobalOptions no longer supports those flags. In practice, the Java-side verifier is no longer exercising the intended authenticated CLI flows, so its results are unreliable as regression coverage. This is not just dead code: it creates a false sense of QA coverage for the current standard CLI contract.

Reproduction Steps

export TRON_TEST_APIKEY=<your test private key>
export TRON_NETWORK=nile

Run:

$ java -cp build/libs/wallet-cli.jar org.tron.qa.QARunner baseline qa/baseline
=== QA Baseline Capture ===
Network: nile
Output dir: qa/baseline
Commands: 118

  Capturing: get-address... OK
  Capturing: get-balance... OK
  Capturing: current-network... OK
  Capturing: get-block... OK
  Capturing: get-chain-parameters... OK
  Capturing: get-bandwidth-prices... OK
  Capturing: get-energy-prices... OK
  Capturing: get-memo-fee... OK
  Capturing: get-next-maintenance-time... OK
  Capturing: list-nodes... OK
  Capturing: list-witnesses... OK
  Capturing: list-asset-issue... OK
  Capturing: list-proposals... OK
  Capturing: list-exchanges... OK
  Capturing: get-market-pair-list... OK

Baseline capture complete: 15 captured, 0 skipped

Although it prints OK, this does not mean the commands actually succeeded.
In captureBaseline(), the code prints OK unconditionally ([QARunner.java:124]), without checking whether the captured output is a success result or an error.

Root Cause

The issue originates from this line:

String[] cliArgs = {"--network", network, "--private-key", privateKey, cmdName};

([QARunner.java:130])

However, GlobalOptions does not support the global flag --private-key ([GlobalOptions.java:48]).

As a result, the parsed arguments are not interpreted as “run get-address with a private key”, but instead:

  • command = null
  • commandArgs = ["--private-key", "<privateKey>", "get-address"]

Then the runner executes:

CommandDefinition cmd = registry.lookup(cmdName);

Here, cmdName is null, which eventually triggers:

nameOrAlias.toLowerCase()

inside lookup().

Verification

You can verify this by inspecting the captured baseline files:

sed -n '1,40p' qa/baseline/get-address.json
sed -n '1,40p' qa/baseline/current-network.json

Example outputs:

{
  "command": "get-address",
  "text_stdout": "User defined config file doesn't exists, use default config file in jar\nError: Cannot invoke \"String.toLowerCase()\" because \"nameOrAlias\" is null\n",
  "text_stderr": "",
  "json_stdout": "Error: Cannot invoke \"String.toLowerCase()\" because \"nameOrAlias\" is null\n",
  "json_stderr": ""
}
"text_stdout": "Error: Cannot invoke \"String.toLowerCase()\" because \"nameOrAlias\" is null\n"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by retirement rather than rehabilitation. QARunner now only keeps the supported list helper; the stale baseline / verify modes have been retired and fail with a clear unsupported/deprecation message, and qa/run.sh java-verify now does the same instead of pretending to provide current regression coverage.

Comment on lines +85 to +89
byte[] priKey = ByteArray.fromHexString(opts.getString("private-key"));
String walletName = opts.has("name") ? opts.getString("name") : "mywallet";

ECKey ecKey = ECKey.fromPrivate(priKey);
WalletFile walletFile = Wallet.createStandard(passwd, ecKey);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Arrays.fill(priKey, (byte) 0) here, so the private key bytes remain in memory until GC, which unnecessarily increases secret exposure.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@gummy789j
Copy link
Copy Markdown
Collaborator

Right now the output json does not implement the unified envelope as described in the PR summary. Only success() emits {"success": true, "data": ...}; result() emits {success, message}, and printMessage()/raw()/keyValue() bypass the envelope entirely. This affects real commands, not just edge cases: get-account still uses printMessage(), register-wallet uses raw(), and many transaction/contract commands use result(). In its current state, JSON mode does not provide a stable machine-parseable contract.

  • JSON output envelope: All commands produce {"success":true,"data":...} or {"success":false,"error":"..."} in JSON mode, with stdout/stderr
    suppression to guarantee machine-parseable output.

fixed, still working on rest of comments.

Wipe temporary secret buffers in the standard CLI import-wallet path after
keystore creation.

- add try/finally around private-key import flow
- clear private key bytes with Arrays.fill(priKey, (byte) 0)
- clear derived password bytes after use for consistency with existing secret handling
@gummy789j
Copy link
Copy Markdown
Collaborator

  • Active wallet management: Persistent active wallet selection via set-active-wallet / list-wallets commands, stored in
    Wallet/active_wallet.conf.

Good catch. The implementation uses Wallet/.active-wallet; the PR description will be updated to match the actual behavior.

Copy link
Copy Markdown
Contributor

@lxcmyf lxcmyf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Code Review

Reviewed the new Standard CLI mode implementation. Found 6 critical, 4 high, and 4 medium severity issues. Key concerns:

  1. Fund Safety: register-wallet doesn't return mnemonic; --network silently drops missing values; auto-confirm stdin bypasses safety prompts
  2. Security: Password/key byte arrays not zeroed after use; MASTER_PASSWORD bypasses password strength checks
  3. Correctness: System.exit() in OutputFormatter bypasses finally cleanup and makes code untestable
  4. Silent Failures: ActiveWalletConfig swallows all exceptions; authenticate() fails silently with no feedback

See inline comments for details.

ActiveWalletConfig.setActiveAddress(address);
out.raw("Register a wallet successful, keystore file name is " + keystoreName);
} else {
out.error("register_failed", "Register wallet failed");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical - Fund Safety] register-wallet only outputs the keystore filename. The mnemonic phrase generated by wrapper.registerWallet() is never returned to the user.

If the user loses the keystore file, they cannot recover the wallet. For a cryptocurrency wallet, the mnemonic must be surfaced in both text and JSON output so the user can back it up.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not changing this in the current PR. The standard CLI currently matches the existing REPL behavior by writing the keystore and encrypted mnemonic artifacts rather than printing the mnemonic to stdout/JSON. Changing that would be a product and secret-handling behavior change, so I would rather scope it separately.


/** Print an error for usage mistakes and exit with code 2. */
public void usageError(String message, CommandDefinition cmd) {
if (mode == OutputMode.JSON) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical - Correctness] System.exit(1) here terminates the JVM before the finally block in StandardCliRunner.execute() can restore System.in, System.out, and System.err.

In JSON mode, stdout/stderr are redirected to /dev/null — the finally cleanup at StandardCliRunner:107-113 is bypassed. Also makes the code completely untestable.

Same issue in usageError() (line 175) and result() (line 105).

Suggestion: Throw a CliExitException(int exitCode) instead. Only call System.exit() once at the main() entry point.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. OutputFormatter no longer calls System.exit(). It now signals abort via CliAbortException, StandardCliRunner.execute() maps that to the final exit code, and stream restoration stays in finally. I also added a focused regression test for the in-process runner path.

}

// Load specific wallet file and authenticate
byte[] password = StringUtils.char2Byte(envPwd.toCharArray());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical - Security] The password byte array is never zeroed after use. If checkPassword() throws or the method exits early, password bytes remain in memory.

Suggestion: Wrap lines 150-161 in a try/finally that calls Arrays.fill(password, (byte) 0) on failure paths.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The auto-auth password handling is now wrapped in try/finally, and the temporary byte[] is cleared with Arrays.fill(password, (byte) 0) after use.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gummy789j There are still remaining leftovers. Intermediate char[] is not zeroized in StandardCliRunner.authenticate()

  • File: StandardCliRunner.java:154

  • Description:
    The intermediate char[] created via envPwd.toCharArray() is passed to char2Byte() and then becomes garbage, but remains in heap memory without being cleared.

  • Fix:
    Capture the char[] in a local variable and zeroize it in a finally block.
    It is also recommended to review and fix all usages of System.getenv(...).toCharArray() accordingly.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The intermediate char[] from toCharArray() is now stored in a named variable and cleared with StringUtils.clear() immediately after char2Byte() conversion, before the try block. Pattern matches register-wallet in WalletCommands.java.

char[] charPassword = envPwd.toCharArray();
byte[] password = StringUtils.char2Byte(charPassword);
StringUtils.clear(charPassword);

if (i + 1 < args.length) opts.output = args[++i];
break;
case "--network":
if (i + 1 < args.length) opts.network = args[++i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical - Fund Safety] If --network is the last argument (no value follows), it is silently ignored and the default network is used.

Worse: wallet-cli --network send-coin --to TXyz --amount 100 consumes send-coin as the network name, leaving the actual command as null.

Running a transaction on the wrong network (mainnet vs testnet) could mean real money lost.

Suggestion: Throw IllegalArgumentException when the required value for --output, --network, --wallet, or --grpc-endpoint is missing.

Same issue on lines 60, 66, 69.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Global option parsing is now strict for --output, --network, --wallet, and --grpc-endpoint: missing values fail fast, and --network / --output also validate allowed values so command tokens are no longer consumed as option values.

// "1\n" — wallet file selection (choose first)
// "y\n" — additional signing confirmations
// Repeated to cover multiple rounds of signing prompts.
String autoInput = "y\n1\ny\ny\n1\ny\ny\n1\ny\ny\n";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical - Fund Safety] This hardcoded auto-confirm stream blindly answers "y" to all interactive prompts and always selects wallet #1.

  1. Auto-confirms dangerous operations (e.g., "Are you sure you want to delete this wallet?").
  2. Users with multiple wallets cannot control which one signs the transaction.
  3. If the legacy code adds/removes a prompt, this silently feeds wrong answers.

Suggestion: Short-circuit interactive prompts at a higher level (e.g., the PERMISSION_ID_OVERRIDE pattern) rather than faking stdin.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the risk. I am not changing this in the current PR because removing that compatibility path safely would require reworking shared REPL / legacy interaction behavior, and I do not want to take that larger behavioral change as part of this review-fix scope.


public void add(CommandDefinition cmd) {
commands.put(cmd.getName(), cmd);
aliasToName.put(cmd.getName(), cmd.getName());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Primary name is stored as-is here, but aliases are lowercased on line 17. lookup() normalizes input to lowercase.

If a command name ever contains uppercase letters, lookup would fail to match the primary name entry.

Suggestion: Normalize here too: aliasToName.put(cmd.getName().toLowerCase(), cmd.getName());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Primary command names are now normalized to lowercase when registered, so lookup stays consistent with the existing lowercase alias normalization.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The primary name is now stored with a lowercased key (nameLower), consistent with alias handling:

String nameLower = cmd.getName().toLowerCase();
// ...
aliasToName.put(nameLower, cmd.getName());

lookup() normalizes to lowercase, so primary names and aliases are matched uniformly.

while (i < args.length) {
String token = args[i];

if ("-m".equals(token)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Hard-coded -m"multi" mapping leaks a domain-specific concern into the general-purpose parser. All commands implicitly accept -m, even those that don't use multi-signature mode.

If a command wanted -m for --memo, it would collide.

Suggestion: Add a shortFlag field to OptionDef so individual commands declare their own short flags.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. -m is no longer treated as a global shorthand for every command. The parser now only accepts -m for commands that actually declare the multi option.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. -m is currently restricted to commands that explicitly declare a multi option — if the command does not have one, it throws CliUsageException:

OptionDef multiOption = optionsByName.get("multi");
if (multiOption == null || multiOption.getType() != OptionDef.Type.BOOLEAN) {
    throw new CliUsageException("Option -m is only supported for commands with --multi");
}

This prevents the collision scenario (-m for --memo), but the short-flag mapping is still hard-coded rather than declared per-command. A shortFlag field on OptionDef would be cleaner and is worth adding if we introduce more short flags in the future. Not changing this in the current PR.

WalletApi.setApiCli(WalletApi.initApiCli());
break;
default:
formatter.usageError("Unknown network: " + network
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] The default case calls formatter.usageError() which calls System.exit(2). But there is no return or throw statement after it. If System.exit() is ever replaced with an exception (per the OutputFormatter issue), execution would continue with no network configured.

Also: all four cases share identical WalletApi.setApiCli(WalletApi.initApiCli()) — consider extracting after the switch.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as part of the abort-handling refactor. usageError() now throws a controlled abort exception instead of exiting the JVM, so execution does not continue past this branch.

public boolean isVerbose() { return verbose; }
public String getCommand() { return command; }
public String[] getCommandArgs() { return commandArgs; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Returns the internal String[] reference directly. Callers can mutate the array and corrupt internal state.

Suggestion: return Arrays.copyOf(commandArgs, commandArgs.length);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. getCommandArgs() now returns a defensive copy instead of exposing the internal array directly.

return null;
}
try (FileReader reader = new FileReader(configFile)) {
Map map = gson.fromJson(reader, Map.class);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Raw type Map — if the JSON contains {"address": 12345} (number instead of string), the (String) cast on line 35 throws ClassCastException, which is then swallowed by the catch block.

Suggestion: Use Map<String, Object> and validate the type before casting.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The active-wallet config read path now uses typed validation instead of a raw cast, so invalid JSON types are rejected explicitly instead of relying on a swallowed ClassCastException.

Comment on lines +65 to +70
case "--wallet":
opts.wallet = requireValue(args, ++i, "--wallet");
break;
case "--grpc-endpoint":
opts.grpcEndpoint = requireValue(args, ++i, "--grpc-endpoint");
break;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--wallet and --grpc-endpoint are dead global flags. They are documented in help and parsed into GlobalOptions, but StandardCliRunner never consumes them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. StandardCliRunner now consumes both global flags: --wallet is used to resolve the auto-auth keystore target, and --grpc-endpoint overrides the ApiClient for the current run.

@3for
Copy link
Copy Markdown

3for commented Apr 9, 2026

Run ./gradlew test failed with:

org.tron.walletcli.cli.StandardCliRunnerTest > missingWalletDirectoryPrintsAutoLoginSkipInfoInTextMode FAILED
    java.lang.AssertionError at StandardCliRunnerTest.java:155

return;
}

String activeAddress = ActiveWalletConfig.getActiveAddressStrict();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list-wallet now uses getActiveAddressStrict(), so a malformed Wallet/.active-wallet prevents the command from listing any keystores. Since set-active-wallet requires an address or name, this also removes the main in-CLI recovery path for repairing wallet selection. list-wallet should treat an unreadable active-wallet config as "no active wallet" and still enumerate the available wallets.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. list-wallet now uses the lenient active-wallet read path instead of the strict one, so a malformed Wallet/.active-wallet is treated as no active wallet and the command still enumerates the available keystores.

Comment on lines +4702 to +4709
if (globalOpts.isInteractive() || shouldLaunchInteractiveByDefault(args, globalOpts)) {
Client cli = new Client();
JCommander.newBuilder()
.addObject(cli)
.build()
.parse(new String[0]);
cli.run();
return 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard CLI and the legacy REPL still share the same set of static network/RPC state in WalletApi. The interactive startup path does not reset these global values, which leads to state contamination.

For example, if within the same JVM you first call:

runMain(new String[]{"--network", "nile", "current-network"});

and then:

runMain(new String[]{});

to enter the REPL:

runMain(["--network", "nile", "current-network"])
  → applyNetwork("nile")
    → initApiCli()            // create ApiClient from config.conf
    → setCurrentNetwork(NILE) // override to NILE
    → updateRpcCli(...)       // replace global apiCli
  → command finishes, return exitCode

runMain([])
  → shouldLaunchInteractiveByDefault → true
  → new Client()              // Client.java:125: new WalletApiWrapper()
  → cli.run()                 // REPL starts
  // At this point:
  // WalletApi.currentNetwork == NILE (polluted by previous call)
  // WalletApi.apiCli == Nile ApiClient (not from config.conf)

The Client constructor does not call initApiCli() to reset global state. As a result, the REPL inherits a polluted network context.

Scenarios where this issue manifests

Scenario 1: Embedded usage

When wallet-cli is integrated and runMain() is invoked multiple times within the same long-lived process, this issue will occur.

Scenario 2: Same-JVM integration tests — confirmed

In ClientMainTest.java, multiple tests invoke Client.runMain() sequentially within the same JVM:

// test 1: line 18
int exitCode = Client.runMain(new String[]{"--help", "get-balance"});

// test 2: line 36
int exitCode = Client.runMain(new String[]{"--version"});

// test 3: line 51
int exitCode = Client.runMain(new String[]{"--output", "json", "--help"});

By default, Gradle uses:

  • forkEvery = 0 (no JVM restart between test classes)
  • maxParallelForks = 1

And build.gradle does not override these settings. This means all test classes run sequentially in the same JVM process.

If one test invokes StandardCliRunner.execute() and modifies currentNetwork, subsequent tests will observe the polluted global state.

The issue is not currently triggered only because:

  • ClientMainTest uses only --help / --version / --output json, which do not call applyNetwork()
  • StandardCliRunnerTest mostly uses custom registries and commands that do not involve network logic

However, if a future test introduces something like --network nile, it will pollute the global state for subsequent tests.

This represents a latent test fragility risk.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. The global state contamination between sequential runMain() calls is a direct consequence of the same static-field design noted above.

This PR does not change the existing WalletApi static lifecycle — both the legacy REPL and the new standard CLI paths use it as-is. Introducing per-invocation state reset or instance-scoped network configuration would be part of a broader concurrency / isolation refactor, which is out of scope here.

Re: test fragility — agreed that this is a latent risk. Current tests happen to avoid --network mutations, but a future test could trigger cross-test pollution. Will keep this in mind when adding network-dependent test cases.

Comment on lines +63 to +74
public static void setActiveAddress(String address) throws IOException {
File dir = getWalletDir();
if (!dir.exists()) {
dir.mkdirs();
}
FilePermissionUtils.setOwnerOnlyDirectory(dir.toPath());
File configFile = new File(dir, CONFIG_FILE);
Map<String, String> data = new LinkedHashMap<String, String>();
data.put("address", address);
try (FileWriter writer = new FileWriter(configFile)) {
gson.toJson(data, writer);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, these persistence paths lack both FileLock and a unified “write to temp file then atomic rename” strategy:

  • ActiveWalletConfig.java (line 63–75) uses FileWriter to directly overwrite .active-wallet
  • WalletUtils.java (line 88–92, 122–124) writes keystore JSON directly to the final path
  • WalletApi.java (line 572–597) store2Keystore() performs an unlocked check-delete-write sequence

As a result, there is no code-level mutual exclusion guarantee when multiple independent JVMs write to the Wallet/ directory concurrently.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. The file persistence paths (ActiveWalletConfig, WalletUtils, store2Keystore()) all lack FileLock and atomic write-then-rename.

This is another facet of the same concurrency gap — the existing codebase was designed for single-process, single-thread usage, and this PR does not change that assumption. Adding file-level locking and atomic persistence would be part of a broader concurrency-safety effort, out of scope for this PR.

Comment on lines +981 to +983
WalletFile wf = resolveSigningWalletFile();
boolean isLedgerFile = wf.getName().contains("Ledger");
byte[] passwd;
if (lockAccount && isUnifiedExist() && Arrays.equals(decodeFromBase58Check(wf.getAddress()), getAddress())) {
passwd = getUnifiedPassword();
} else {
System.out.println("Please input your password.");
passwd = char2Byte(inputPassword(false));
}
byte[] passwd = resolveSigningPassword(wf);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current implementation, WalletApi.java (line 980) signTransaction() has been changed to first call resolveSigningWalletFile() (line 898) and resolveSigningPassword() (line 906). These helpers, at line 899 and line 907 respectively, will directly return the currently logged-in wallet and cached password when isUnifiedExist() is true.

As a result, the following prompts will no longer appear:

Please choose your key for sign.
Please input your password.

In the previous version, signTransaction() would always call selectWalletFileE() first, and would only reuse the cached password if the selected wallet address matched the currently logged-in one. Otherwise, it would still prompt for the password of the selected wallet. In other words, the old REPL did allow a scenario where a user is logged into one wallet but temporarily selects another wallet for signing.

Conclusion

  • For single-wallet REPL users: this is a UX improvement (one less selection and password prompt).
  • For multi-wallet REPL users: this is a behavioral regression, as it is no longer possible to temporarily switch wallets during signing.

Suggestion

To balance both use cases, a safer approach would be:

  • Keep the auto-reuse logic in signTransactionForCli() (line 1051) unchanged
  • Restore the old behavior in REPL signTransaction() (line 966), i.e., “select wallet first, then decide whether to reuse the cached password”

This would make the semantic boundary between the standard CLI and the legacy interactive mode clearer.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The root cause was that resolveSigningWalletFile() / resolveSigningPassword() were shared between REPL and Standard CLI signing paths. When isUnifiedExist() was true, both helpers short-circuited: resolveSigningWalletFile() returned the current wallet directly (skipping selectWalletFileE()), and resolveSigningPassword() returned the cached password without checking lockAccount or address match.

This introduced two regressions in the REPL path:

  1. Multi-wallet users could no longer temporarily select a different wallet for signing
  2. The password reuse guard was relaxed from lockAccount && isUnifiedExist() && addressMatch to just isUnifiedExist()

Restored the original inline logic (selectWalletFileE() + 3-condition password check) in all three REPL signing methods:

  • signTransaction(tx)
  • signTransaction(tx, multi)
  • addTransactionSign(tx)

resolveSigningWalletFile() / resolveSigningPassword() are now only called from signTransactionForCli(), which is the intended Standard CLI path where auto-selecting the current wallet is correct behavior.

  resolveSigningWalletFile()/resolveSigningPassword() were shared between
  REPL and Standard CLI signing paths, which bypassed selectWalletFileE()
  and relaxed the 3-condition password reuse guard (lockAccount + isUnifiedExist
  + address match) to just isUnifiedExist(). This prevented REPL multi-wallet
  users from temporarily switching signing wallets.

  Restored inline selectWalletFileE() + original password logic in:
  - signTransaction(tx)
  - signTransaction(tx, multi)
  - addTransactionSign(tx)

  resolve* helpers are now only used by signTransactionForCli().
- Remove deprecated exchange-transaction command from standard CLI
  and user manual (REPL path unchanged).
- Split GasFreeApi.getMessage into REPL-facing (prints + swallows) and
  getMessageOrThrow (standard-CLI-facing, throws IllegalStateException);
  WalletApiWrapper maps the exception to a query_failed CommandErrorException
  so JSON output stays structured.
- gas-free-info now always requires an authenticated wallet, even when
  --address targets another account — the handler's triggerContract call
  requires a non-null wallet. Updated StandardCliRunnerTest accordingly.
- Restore non-paginated WalletApi.listProposals / listExchanges for
  list-proposals and list-exchanges standard CLI commands.
- Expand qa/ harness: +660 lines in qa/lib/cli.sh, manifest/case_resolver
  updates, and task_runner tweaks for broader parity coverage.
@gummy789j gummy789j force-pushed the develop branch 3 times, most recently from dc8e92f to 283dc47 Compare April 15, 2026 04:36
@parsoncryptoai parsoncryptoai changed the base branch from develop to release_v4.9.5 April 15, 2026 04:46
  W1: reset-wallet dry-run now includes ledger_files and config_files
      to match actual deletion scope

  W2: --grpc-endpoint validation failure no longer destroys the
      existing ApiClient; uses try-finally guard to close the new
      client and preserve the original connection

  W3: corrupt keystore files no longer crash list-wallet,
      set-active-wallet, or get-active-wallet; list-wallet marks
      them with an error field, findWalletFileByAddress/Name skips
      and continues scanning

  W4: clear-wallet-keystore no longer wipes the entire Ledger/
      directory; blanket cleanup is now exclusive to reset-wallet

  W5: replaced logger.warn with System.err.println in CLI code
      paths to prevent logback STDOUT appender from polluting
      --output json responses
  - Unify applyNetwork() + applyGrpcEndpointOverride() into
  resolveConnection()
    that validates before mutating global state
  - Use correct solidityNode for known networks instead of reusing
  grpc-endpoint
  - Zero sensitive key material after copy in deploy/trigger paths
  - Add permissionId range validation (0-2)
  - Migrate always-true emitBooleanResult() to emitSuccess()
  - Thread masterPasswordProvider through CommandContext
  - Strengthen mismatch test with assertSame
…A exit code

  - QA: qa/run.sh now exits non-zero when tests fail (was always exit 0)
  - WalletApi: clearContractAbiForCli/updateBrokerageForCli record gRPC error
    before returning false, matching all other *ForCli methods
  - WalletApi: voteWitnessForCli catches NumberFormatException on vote count
  - WalletApi: remove unused voteScore param from createAssetIssueForCli chain
  - gas-free-info: add getGasFreeInfoDataForCli using static constant call,
    allowing --address queries without wallet auth (REPL method unchanged)
  - Update QA manifest and unit test for gas-free-info auth policy change
  - Change deploy-contract --origin-energy-limit default from 1 to 10,000,000
    to match realistic contract energy allowance
  - Return success with empty list instead of error when no wallets exist,
    as an empty wallet directory is a valid state
  - Add unit test for list-wallet empty directory case
…sistency, and fail-fast on missing chain params

  - M8:  Add missing space before version string in global help output
  - M9:  Add requirePositive validation for exchange-inject, exchange-withdraw,
         and market-sell-asset quantity parameters
  - M10: Add 0-100 range validation for update-brokerage
  - M12: Unify send-coin non-multi output to use emitSuccess pattern while
         preserving to/amount/txid fields
  - M13: Replace hardcoded 420 SUN energy price fallback with explicit abort
         when getEnergyFee chain parameter is absent
…ling, and code hygiene

  - tronprotocol#2: Preserve specific Ledger signing error messages instead of
    overwriting with generic Transaction signing failed
  - tronprotocol#3: Add requirePositive validation for exchange-create balances
  - tronprotocol#4: Add requireNonNegative validation for update-asset limits
  - tronprotocol#5: Use validated address bytes via encode58Check instead of
    discarding getAddress() return values in delegated-resource queries
  - tronprotocol#7: Validate witness addresses in vote-witness before submission
  - tronprotocol#8: Validate mnemonic word count (12/24) in register-wallet
    using existing MnemonicUtils utility
  - tronprotocol#9: Specify StandardCharsets.UTF_8 in all String.getBytes() calls
    (47 occurrences across 12 files)
  - tronprotocol#13: Make resolveActiveWalletFileStrict throw IOException instead
    of returning null, matching its Strict contract
  - tronprotocol#17: Add requireNonNegative check for permission-id on 6 commands
…late QA classes

  - Move QA classes to src/test and build separate qaJar to keep production JAR clean
  - Add sanitizePermissionJson() to strip @type keys (fastjson deserialization defense)
  - Block absolute/traversal paths in wallet override resolution
  - Validate address format for gasfree-transfer --to
  - Add missing bounds checks: origin-energy-limit, value, token-value, duration,
    lock-period, proposal id, resource type
  - Simplify --help detection to work anywhere in args
  - Return descriptive message instead of null from extractTransactionReturnMessage
  - Make ActiveWalletConfig.clear() return boolean instead of printing to stderr
  - Replace JSON.parse() + unsafe cast with JSON.parseObject() in GasFree handlers
…ode stdout pollution

  - H1+H11: AbiUtil.encodeInput throws IllegalArgumentException instead of
    printStackTrace + return null, eliminating NPE chain in parseMethod callers
  - H2: wrap all 4 parseMethod calls in ContractCommands with try-catch,
    classifying malformed input as usage_error (exit 2)
  - H3: fix reference equality tokenId !=  → in
    WalletApi.triggerCallContract
  - H4: validate tokenId as numeric at ContractCommands handler level (3 sites)
    with defense-in-depth catch in WalletApi
  - H5: add WalletApi.broadcastTransactionForCli returning error string instead
    of printing to stdout; update broadcast-transaction command to use it
  - H6+H12: remove System.out.println from addressValid() and
    decodeFromBase58Check() — callers already handle null/false returns
  - H7: gas-free-info now uses getAddress() + encode58Check() for Base58
    validation, consistent with all other address-accepting commands
  - broadcast-transaction: InvalidProtocolBufferException classified as
    usage_error (malformed input) not execution_error
…tion, and error handling

  - M6: deploy-contract constructor encoding uses encodeInput instead of
    parseMethod to avoid prepending 4-byte selector (StandardCLI-only bug)
  - H6: getTransactionCountByBlockNum now throws on gRPC failure instead
    of silently returning 0 with success status
  - H9: participate-asset-issue adds requireNonBlank for asset parameter
  - H10: TODO comments for update-account/set-account-id/update-asset
    field length limits (node enforces, not yet validated client-side)
  - H11: TODO for deploy-contract to include contract_address in JSON
  - H13: null-guard e.getMessage() in 5 catch blocks (fallback to toString)
  - H14: exchange-inject/withdraw add requireNonBlank for token-id
  - C9: comment clarifying definite-assignment safety on uninitialized Triple
…handling

  - JsonFormatUtil: track inString/escaped state so structural characters
    inside JSON string values are no longer misinterpreted as structural
    tokens; replace custom formatter in Utils with Gson pretty-printer
  - ActiveWalletConfig: validate Base58Check address in setActiveAddress();
    enforce .json extension when resolving wallet file by selection
  - GlobalOptions: use Locale.ROOT for toLowerCase() to avoid locale traps
  - CommandSupport: add requirePermissionId() (0–2 range), requireMaxBytes(),
    and requireByteRange() helpers
  - Commands: replace requireNonNegative with requirePermissionId for
    permission-id across TransactionCommands, StakingCommands,
    ContractCommands, and WitnessCommands
  - TransactionCommands: resolve three open TODOs — enforce name ≤ 200 B,
    account-id 8–32 B, and contract description/url byte limits; fix
    transfer-usdt energy estimation to throw CommandErrorException instead
    of returning a false flag
  - QueryCommands: validate block number is non-negative before querying
  - WalletApiWrapper: ensure fee-limit buffer is at least 1 via Math.max
  - MiscCommands: remove duplicate help alias
  - Tests: use valid Base58Check addresses; isolate wallet-dir tests with
    TemporaryFolder; update TransactionCommandsTest for CommandErrorException
…validation, and UX

  - Remove auth requirement for constant/view calls (trigger-constant-contract,
    estimate-energy): these are read-only gRPC calls that don't need signing,
    so null owner is now passed through to the node directly
  - Standardize error code missing_env → execution_error in register-wallet
  - Detect duplicate witness addresses in vote-witness instead of silent overwrite
  - Add null guard to CommandDefinition.Builder.authPolicy()
  - Remove unused OptionDef.Type.ADDRESS enum value
  - Make help command delegate to registry.formatGlobalHelp() and support
    --command for per-command help, matching --help behavior
  - Improve text-mode output: structured data now renders as aligned Metadata table
  - Add design-intent comments for --help scanning and broad exception catch
…ant calls auth-free

  - deploy-contract: WalletApi.deployContractForCli now returns the base58
    contract address (derived via generateContractAddress) instead of boolean;
    ContractCommands includes it as contract_address in the JSON response data

  - trigger-constant-contract / estimate-energy: remove the conditional auth
    requirement — neither command broadcasts a transaction, so no wallet is
    needed; reflect this in the user manual and QA expectations

  - transfer-usdt: drop redundant boolean result variable; callContractForCli
    throws on failure, so emitSuccess is always the correct path

  - docs: correct origin-energy-limit default (1 → 10,000,000) and tighten the
    --grpc-endpoint validation comment to explain why we delegate to gRPC
…OADCAST_TX_ID ThreadLocal

  Replace the thread-local side-channel (LAST_BROADCAST_TX_ID) with direct
  return values — all *ForCli methods now return the txid String (null on
  failure) instead of void/boolean. deployContractForCli returns
  Pair<contractAddress, txid>; callContractForCli returns
  Triple<String, Long, Long> with txid as the left element.

  Other fixes in this commit:
  - Use lenient JSON parsing in Utils.formatMessageString to tolerate
    malformed on-chain data (e.g. asset tronprotocol#436 description on Nile)
  - Add QA manifest entry for the help command (noauth-help type)
  - Fix test compilation: update overrides to match new signatures
@gummy789j gummy789j merged commit 847ed4d into tronprotocol:release_v4.9.5 Apr 17, 2026
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.

6 participants