refactor(toolkit): move keystore-factory to plugins module#4
refactor(toolkit): move keystore-factory to plugins module#4Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughReorganizes keystore classes from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.0)framework/src/test/java/org/tron/common/runtime/vm/PrecompiledContractsVerifyProofTest.javaframework/src/main/java/org/tron/core/Wallet.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
plugins/src/main/java/common/org/tron/plugins/KeystoreFactory.java (1)
105-148: Consider using try-with-resources for Scanner.The Scanner is created but only closed on explicit
exit/quitcommands. While acceptable for a CLI tool (JVM exit handles cleanup), using try-with-resources would be more defensive.♻️ Optional: Use try-with-resources for Scanner
`@Override` public void run() { - Scanner in = new Scanner(System.in); - help(); - while (in.hasNextLine()) { - try { - // ... existing code ... - case "exit": - case "quit": { - System.out.println("Exit !!!"); - in.close(); - return; - } - // ... - } catch (Exception e) { - logger.error(e.getMessage()); + try (Scanner in = new Scanner(System.in)) { + help(); + while (in.hasNextLine()) { + try { + // ... existing code ... + case "exit": + case "quit": { + System.out.println("Exit !!!"); + return; + } + // ... + } catch (Exception e) { + logger.error(e.getMessage()); + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/src/main/java/common/org/tron/plugins/KeystoreFactory.java` around lines 105 - 148, The run() method in KeystoreFactory creates a Scanner (Scanner in = new Scanner(System.in)) and only closes it on explicit "exit"/"quit" paths; change run() to use try-with-resources for the Scanner so it is always closed (even on exceptions/early returns) by wrapping the Scanner creation in a try(...) block and moving the loop inside that block, removing the explicit in.close() in the exit/quit branch; this ensures deterministic resource cleanup while keeping the existing command parsing and methods genKeystore(), importPrivateKey(), and help() intact.crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java (1)
34-46: Inconsistent exception declarations with the underlying implementation.
generateFullNewWalletFileandgenerateLightNewWalletFilestill declareNoSuchAlgorithmException,NoSuchProviderException, andInvalidAlgorithmParameterException, but the underlyinggenerateNewWalletFilethey call no longer throws these. These declarations are now misleading sinceSignUtils.getGeneratedRandomSign()doesn't throw checked exceptions.♻️ Proposed fix to align exception declarations
public static String generateFullNewWalletFile(String password, File destinationDirectory) - throws NoSuchAlgorithmException, NoSuchProviderException, - InvalidAlgorithmParameterException, CipherException, IOException { + throws CipherException, IOException { return generateNewWalletFile(password, destinationDirectory, true); } public static String generateLightNewWalletFile(String password, File destinationDirectory) - throws NoSuchAlgorithmException, NoSuchProviderException, - InvalidAlgorithmParameterException, CipherException, IOException { + throws CipherException, IOException { return generateNewWalletFile(password, destinationDirectory, false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java` around lines 34 - 46, The two wrapper methods generateFullNewWalletFile and generateLightNewWalletFile declare checked exceptions (NoSuchAlgorithmException, NoSuchProviderException, InvalidAlgorithmParameterException) that the called generateNewWalletFile no longer throws (SignUtils.getGeneratedRandomSign is not throwing checked exceptions); update the method signatures by removing those three exceptions and only declare the actual checked exceptions thrown (e.g., CipherException, IOException) so the throws clauses on generateFullNewWalletFile and generateLightNewWalletFile match generateNewWalletFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crypto/src/main/java/org/tron/common/crypto/keystore/Wallet.java`:
- Line 214: The decrypt path currently hardcodes EC reconstruction via
SignUtils.fromPrivate(privateKey, true) which breaks SM2; change Wallet.decrypt
to accept an explicit crypto-engine flag/enum (e.g., boolean isEc or
CryptoEngine engine) and call SignUtils.fromPrivate(privateKey, engineChoice)
instead of the hardcoded true, then update all callers that invoke
Wallet.decrypt(...) (credential loading paths and any direct calls) to pass the
active engine selection from runtime/config so SM2 deployments preserve their
engine choice.
In `@crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java`:
- Around line 50-52: The generateNewWalletFile() call currently hardcodes
SignUtils.getGeneratedRandomSign(..., true) which forces ECKey wallets; change
it to consult the node crypto engine setting by replacing the literal true with
CommonParameter.getInstance().isECKeyCryptoEngine(), and add the required import
for org.tron.common.parameter.CommonParameter; ensure
SignUtils.getGeneratedRandomSign(...) now receives the dynamic boolean so
SM2-configured systems are respected.
In `@plugins/README.md`:
- Around line 184-193: The README's fenced code blocks showing keystore naming
(the block containing "UTC--yyyy-MM-ddTHH-mm-ss.nVV--<address>.json" and the
example block
"UTC--2024-01-15T10-30-45.123Z--TJRyHRFpmjG6mZ95mZaMvHx5fU7fGvRQoN.json") are
missing language specifiers; update both fenced blocks to include a language
(e.g., add ```text after the opening backticks) so static analysis and renderers
apply correct syntax highlighting and the examples display properly.
---
Nitpick comments:
In `@crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java`:
- Around line 34-46: The two wrapper methods generateFullNewWalletFile and
generateLightNewWalletFile declare checked exceptions (NoSuchAlgorithmException,
NoSuchProviderException, InvalidAlgorithmParameterException) that the called
generateNewWalletFile no longer throws (SignUtils.getGeneratedRandomSign is not
throwing checked exceptions); update the method signatures by removing those
three exceptions and only declare the actual checked exceptions thrown (e.g.,
CipherException, IOException) so the throws clauses on generateFullNewWalletFile
and generateLightNewWalletFile match generateNewWalletFile.
In `@plugins/src/main/java/common/org/tron/plugins/KeystoreFactory.java`:
- Around line 105-148: The run() method in KeystoreFactory creates a Scanner
(Scanner in = new Scanner(System.in)) and only closes it on explicit
"exit"/"quit" paths; change run() to use try-with-resources for the Scanner so
it is always closed (even on exceptions/early returns) by wrapping the Scanner
creation in a try(...) block and moving the loop inside that block, removing the
explicit in.close() in the exit/quit branch; this ensures deterministic resource
cleanup while keeping the existing command parsing and methods genKeystore(),
importPrivateKey(), and help() intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed2652be-5726-4b5b-af67-f8c0cba2f3a4
📒 Files selected for processing (28)
common/src/main/java/org/tron/common/parameter/CommonParameter.javacrypto/src/main/java/org/tron/common/crypto/keystore/Credentials.javacrypto/src/main/java/org/tron/common/crypto/keystore/Wallet.javacrypto/src/main/java/org/tron/common/crypto/keystore/WalletFile.javacrypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/config/args/CLIParameter.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/main/java/org/tron/core/zen/address/DiversifierT.javaframework/src/main/java/org/tron/program/FullNode.javaframework/src/test/java/org/tron/common/runtime/vm/PrecompiledContractsVerifyProofTest.javaframework/src/test/java/org/tron/core/config/args/ArgsTest.javaframework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.javaframework/src/test/java/org/tron/core/db/TransactionExpireTest.javaframework/src/test/java/org/tron/core/db/TxCacheDBInitTest.javaframework/src/test/java/org/tron/core/db/TxCacheDBTest.javaframework/src/test/java/org/tron/core/jsonrpc/ApiUtilTest.javaframework/src/test/java/org/tron/core/zksnark/SendCoinShieldTest.javaframework/src/test/java/org/tron/core/zksnark/ShieldedReceiveTest.javaframework/src/test/java/org/tron/keystore/CredentialsTest.javaframework/src/test/java/org/tron/keystore/WalletFileTest.javaframework/src/test/java/org/tron/keystroe/CredentialsTest.javaframework/src/test/java/org/tron/program/SupplementTest.javaplugins/README.mdplugins/build.gradleplugins/src/main/java/common/org/tron/plugins/KeystoreFactory.javaplugins/src/main/java/common/org/tron/plugins/Toolkit.java
💤 Files with no reviewable changes (4)
- common/src/main/java/org/tron/common/parameter/CommonParameter.java
- framework/src/test/java/org/tron/core/config/args/ArgsTest.java
- framework/src/main/java/org/tron/program/FullNode.java
- framework/src/main/java/org/tron/core/config/args/CLIParameter.java
| byte[] privateKey = performCipherOperation(Cipher.DECRYPT_MODE, iv, encryptKey, cipherText); | ||
|
|
||
| return SignUtils.fromPrivate(privateKey, Args.getInstance().isECKeyCryptoEngine()); | ||
| return SignUtils.fromPrivate(privateKey, true); |
There was a problem hiding this comment.
Preserve crypto-engine selection in decrypt path.
At Line 214, forcing SignUtils.fromPrivate(privateKey, true) always reconstructs EC keys and drops SM2 compatibility. This propagates to credential loading paths that call Wallet.decrypt(...) directly.
💡 Proposed fix (make engine explicit instead of hardcoded)
- public static SignInterface decrypt(String password, WalletFile walletFile)
+ public static SignInterface decrypt(String password, WalletFile walletFile,
+ boolean isECKeyCryptoEngine)
throws CipherException {
@@
- return SignUtils.fromPrivate(privateKey, true);
+ return SignUtils.fromPrivate(privateKey, isECKeyCryptoEngine);
}Please also update callers to pass the active engine selection explicitly (for example from runtime config) so SM2 deployments keep working.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypto/src/main/java/org/tron/common/crypto/keystore/Wallet.java` at line
214, The decrypt path currently hardcodes EC reconstruction via
SignUtils.fromPrivate(privateKey, true) which breaks SM2; change Wallet.decrypt
to accept an explicit crypto-engine flag/enum (e.g., boolean isEc or
CryptoEngine engine) and call SignUtils.fromPrivate(privateKey, engineChoice)
instead of the hardcoded true, then update all callers that invoke
Wallet.decrypt(...) (credential loading paths and any direct calls) to pass the
active engine selection from runtime/config so SM2 deployments preserve their
engine choice.
| throws CipherException, IOException { | ||
|
|
||
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), | ||
| Args.getInstance().isECKeyCryptoEngine()); | ||
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if generateNewWalletFile is called from production code paths
rg -n "generateNewWalletFile|generateFullNewWalletFile|generateLightNewWalletFile" --type java -g '!**/test/**'Repository: Federico2014/java-tron
Length of output: 808
🏁 Script executed:
# Find all callers of the three methods across the entire codebase
rg -n "generateNewWalletFile|generateFullNewWalletFile|generateLightNewWalletFile" --type javaRepository: Federico2014/java-tron
Length of output: 1079
🏁 Script executed:
# Examine KeystoreFactory.java to verify the reference
find . -name "KeystoreFactory.java" -type f | head -5Repository: Federico2014/java-tron
Length of output: 134
🏁 Script executed:
# Check the signature and usage of Args vs CommonParameter for crypto engine config
rg -n "isECKeyCryptoEngine" --type java -B 2 -A 2 | head -50Repository: Federico2014/java-tron
Length of output: 5666
🏁 Script executed:
# Check KeystoreFactory.java around lines 62-63
sed -n '55,70p' ./plugins/src/main/java/common/org/tron/plugins/KeystoreFactory.javaRepository: Federico2014/java-tron
Length of output: 710
🏁 Script executed:
# Check WalletUtils.java imports and full context
head -60 crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.javaRepository: Federico2014/java-tron
Length of output: 2357
🏁 Script executed:
# Check if Args class exists and is used for crypto engine config
rg -n "class Args|isECKeyCryptoEngine" --type java crypto/src/main/java | head -20Repository: Federico2014/java-tron
Length of output: 1238
Hardcoded true breaks SM2 crypto engine support.
The hardcoded true in generateNewWalletFile() means this method always generates ECKey-based wallets, ignoring the node's configured crypto engine. This is inconsistent with the production pattern shown in KeystoreFactory.java (line 62), which correctly uses CommonParameter.getInstance().isECKeyCryptoEngine().
Although currently only used in tests, this method should respect the configured crypto engine for consistency and to support SM2-configured systems.
🔧 Proposed fix
public static String generateNewWalletFile(
String password, File destinationDirectory, boolean useFullScrypt)
throws CipherException, IOException {
- SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true);
+ SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(),
+ CommonParameter.getInstance().isECKeyCryptoEngine());
return generateWalletFile(password, ecKeyPair, destinationDirectory, useFullScrypt);
}Requires adding: import org.tron.common.parameter.CommonParameter;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throws CipherException, IOException { | |
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), | |
| Args.getInstance().isECKeyCryptoEngine()); | |
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); | |
| import org.tron.common.parameter.CommonParameter; | |
| // ... other code ... | |
| throws CipherException, IOException { | |
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), | |
| CommonParameter.getInstance().isECKeyCryptoEngine()); | |
| return generateWalletFile(password, ecKeyPair, destinationDirectory, useFullScrypt); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java` around
lines 50 - 52, The generateNewWalletFile() call currently hardcodes
SignUtils.getGeneratedRandomSign(..., true) which forces ECKey wallets; change
it to consult the node crypto engine setting by replacing the literal true with
CommonParameter.getInstance().isECKeyCryptoEngine(), and add the required import
for org.tron.common.parameter.CommonParameter; ensure
SignUtils.getGeneratedRandomSign(...) now receives the dynamic boolean so
SM2-configured systems are respected.
|
|
||
| Keystore files are saved in the `Wallet` directory with the following naming convention: | ||
| ``` | ||
| UTC--yyyy-MM-ddTHH-mm-ss.nVV--<address>.json | ||
| ``` | ||
|
|
||
| Example: | ||
| ``` | ||
| UTC--2024-01-15T10-30-45.123Z--TJRyHRFpmjG6mZ95mZaMvHx5fU7fGvRQoN.json | ||
| ``` |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks.
The static analysis tool flagged these code blocks as missing language specifiers. Adding them improves rendering and syntax highlighting in documentation viewers.
📝 Proposed fix
### Output:
Keystore files are saved in the `Wallet` directory with the following naming convention:
-```
+```text
UTC--yyyy-MM-ddTHH-mm-ss.nVV--<address>.jsonExample:
- +text
UTC--2024-01-15T10-30-45.123Z--TJRyHRFpmjG6mZ95mZaMvHx5fU7fGvRQoN.json
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Keystore files are saved in the `Wallet` directory with the following naming convention: | |
| ``` | |
| UTC--yyyy-MM-ddTHH-mm-ss.nVV--<address>.json | |
| ``` | |
| Example: | |
| ``` | |
| UTC--2024-01-15T10-30-45.123Z--TJRyHRFpmjG6mZ95mZaMvHx5fU7fGvRQoN.json | |
| ``` | |
| Keystore files are saved in the `Wallet` directory with the following naming convention: |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 186-186: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/README.md` around lines 184 - 193, The README's fenced code blocks
showing keystore naming (the block containing
"UTC--yyyy-MM-ddTHH-mm-ss.nVV--<address>.json" and the example block
"UTC--2024-01-15T10-30-45.123Z--TJRyHRFpmjG6mZ95mZaMvHx5fU7fGvRQoN.json") are
missing language specifiers; update both fenced blocks to include a language
(e.g., add ```text after the opening backticks) so static analysis and renderers
apply correct syntax highlighting and the examples display properly.
There was a problem hiding this comment.
2 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java:52">
P1: Do not hardcode the crypto engine to ECKey here; it ignores runtime configuration and can generate keystores incompatible with SM2-configured environments.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/keystore/Wallet.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/keystore/Wallet.java:214">
P2: Hardcoding `true` forces the EC key crypto engine and ignores the runtime configuration, which can cause decrypt/sign behavior to differ from configured expectations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), | ||
| Args.getInstance().isECKeyCryptoEngine()); | ||
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); |
There was a problem hiding this comment.
P1: Do not hardcode the crypto engine to ECKey here; it ignores runtime configuration and can generate keystores incompatible with SM2-configured environments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/keystore/WalletUtils.java, line 52:
<comment>Do not hardcode the crypto engine to ECKey here; it ignores runtime configuration and can generate keystores incompatible with SM2-configured environments.</comment>
<file context>
@@ -48,11 +47,9 @@ public static String generateLightNewWalletFile(String password, File destinatio
- SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(),
- Args.getInstance().isECKeyCryptoEngine());
+ SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true);
return generateWalletFile(password, ecKeyPair, destinationDirectory, useFullScrypt);
}
</file context>
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); | |
| SignInterface ecKeyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), org.tron.common.parameter.CommonParameter.getInstance().isECKeyCryptoEngine()); |
| byte[] privateKey = performCipherOperation(Cipher.DECRYPT_MODE, iv, encryptKey, cipherText); | ||
|
|
||
| return SignUtils.fromPrivate(privateKey, Args.getInstance().isECKeyCryptoEngine()); | ||
| return SignUtils.fromPrivate(privateKey, true); |
There was a problem hiding this comment.
P2: Hardcoding true forces the EC key crypto engine and ignores the runtime configuration, which can cause decrypt/sign behavior to differ from configured expectations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/keystore/Wallet.java, line 214:
<comment>Hardcoding `true` forces the EC key crypto engine and ignores the runtime configuration, which can cause decrypt/sign behavior to differ from configured expectations.</comment>
<file context>
@@ -212,7 +211,7 @@ public static SignInterface decrypt(String password, WalletFile walletFile)
byte[] privateKey = performCipherOperation(Cipher.DECRYPT_MODE, iv, encryptKey, cipherText);
- return SignUtils.fromPrivate(privateKey, Args.getInstance().isECKeyCryptoEngine());
+ return SignUtils.fromPrivate(privateKey, true);
}
</file context>
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by cubic
Moved the keystore factory into the
pluginsmodule as aToolkitsubcommand and removed the node flag-based launcher. Also repackaged keystore classes underorg.tron.common.crypto.keystoreand updated references.Refactors
keystore-factoryas aToolkitsubcommand inplugins; updatedplugins/README.mdwith usage.--keystore-factoryflag andCommonParameter.keystoreFactory;FullNodeno longer starts it.Credentials,Wallet,WalletFile,WalletUtilstoorg.tron.common.crypto.keystoreand updated imports.Args.isECKeyCryptoEngine()dependency).Migration
java -jar Toolkit.jar keystore-factory, then useGenKeystoreorImportPrivateKey.--keystore-factoryfrom any node startup scripts or configs.org.tron.keystore.*toorg.tron.common.crypto.keystore.*.Written for commit 6533628. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor