feat(staking-cli): add private key signing and calldata export#3894
feat(staking-cli): add private key signing and calldata export#3894
Conversation
Add --private-key flag for raw key signing alongside mnemonic/ledger. Add --export-calldata mode for Safe multisig users to output transaction data instead of executing. Validates via eth_call simulation using --sender-address (skip with --skip-simulation). Supports JSON, TOML, and human-readable output formats. Consolidate all state-changing operations into Transaction enum with unified calldata generation for both execute and export modes. Other changes: - Move main.rs logic into cli.rs module for better encapsulation - Simplify ValidSignerConfig::wallet() to return EthereumWallet directly - Add AddressExt trait for cleaner address resolution from wallet - Use trimmed format for ESP token display (e.g. "100 ESP" not "100.0") - Reduce public API surface: make internal modules pub(crate) - Remove funding.rs module, use Transaction::Transfer for ESP transfers
Consolidate 3 concrete implementations into 2 generic ones, reducing code duplication while maintaining the same functionality.
|
/gemini review |
alysiahuggins
left a comment
There was a problem hiding this comment.
very good change. what would be the best way to manually test this PR?
| } => { | ||
| let mut config = toml::from_str::<Config>(include_str!("../config.decaf.toml"))?; | ||
| config.signer.mnemonic = mnemonic; | ||
| config.signer.private_key = private_key; |
There was a problem hiding this comment.
so this private key is stored on disk? anyway that can be avoided or provide a clear warning about it?
There was a problem hiding this comment.
It can be stored in the config file, provided as argument or env var. It was a feature request we got from node operators to also support private keys instead of only mnemonics and the ledger. In terms of security it's no worse than mnemonics that are already supported. It's not uncommon or bad to store secrets on disk or in env vars / secret managers as long as other security measures are taken to secure the infra and avoid keys leaking.
So it's mostly up to the user / node operators to secure their keys, or use a hardware wallet for better security. I think if someone isn't careful with their keys there's not much we can do here. We could potentially only support hardware Ethereum wallets but I think this would be inconvenient for us and for node operators and for our consensus keys we don't have hardware wallet support anyway.
There was a problem hiding this comment.
ok got it @ feature request. It would be good to note (if not already done), a warning about choosing to use private keys
|
|
||
| Ok(Some(Transaction::ClaimRewards { | ||
| reward_claim: data.reward_claim_address, | ||
| lifetime_rewards: data.claim_input.lifetime_rewards, |
There was a problem hiding this comment.
do we have to check that this is none zero or if data is returned it's because it is def non-zero?
There was a problem hiding this comment.
Here's the flow when a user calls staking-cli claim-rewards with no rewards balance:
Summary
The CLI fails early with a user-friendly error before calling the contract. The API returns 404 if the reward balance is zero.
Check Locations
- API Endpoint (first check)
Returns 404 NOT_FOUND if zero reward balance:
espresso-network/sequencer/src/api/endpoints.rs
Lines 247 to 253 in 92e8026
- staking-cli (handles 404)
Returns Ok(None) when API returns 404:
espresso-network/staking-cli/src/claim.rs
Lines 80 to 82 in 92e8026
- staking-cli (converts to error)
Converts None to user-facing error:
espresso-network/staking-cli/src/cli.rs
Lines 549 to 556 in aa13c9b
- Contract (never reached in this case)
If somehow a zero amount reached the contract, it would revert:
And for already claimed rewards:
A test is here:
espresso-network/staking-cli/src/claim.rs
Lines 243 to 269 in aa13c9b
There was a problem hiding this comment.
ok thanks a lot for this
Recommend hardware wallet (Ledger) for mainnet funds, warn about key leakage risks, and advise using environment variables for secrets.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
The pull request introduces significant enhancements to the staking-cli, primarily by adding support for private key signing and a new calldata export mode for multisig users. The core logic for state-changing operations has been consolidated into a new Transaction enum, improving modularity and maintainability. The main.rs file has been refactored, moving its logic into a dedicated cli.rs module, which is a good architectural improvement. Documentation in README.md has been updated to reflect these new features and options. Comprehensive test cases have been added or updated to cover the new functionalities, including crucial roundtrip tests for calldata export and error decoding for both contract and RPC errors. The changes also include minor formatting adjustments and public API surface reduction for internal modules.
I am having trouble creating individual review comments. Click here to see my feedback.
staking-cli/tests/cli.rs (1227-1294)
The test_cli_export_calldata_roundtrip test is highly valuable. It ensures that calldata exported by the CLI can be directly executed on-chain and produce the expected results, validating the integrity and correctness of the calldata generation process. This is a critical test for the new feature.
staking-cli/src/transaction.rs (1-371)
The introduction of the transaction.rs module and the Transaction enum is a significant architectural improvement. It centralizes all state-changing operations, providing a single, consistent interface for both execution and calldata export. This design pattern greatly enhances maintainability, testability, and reduces code duplication across the CLI.
staking-cli/src/cli.rs (538-543)
Requiring --sender-address for claim-rewards when export_calldata is enabled, even with --skip-simulation, is a critical detail. The sender address is necessary to fetch reward proofs from the Espresso node, and this ensures the command behaves correctly in export mode.
staking-cli/tests/cli.rs (1408-1427)
The test_cli_claim_rewards_requires_sender_address_even_with_skip_simulation test is critical. It highlights a specific edge case where --sender-address is always required for claim-rewards due to its interaction with the Espresso node, even when simulation is skipped. This ensures correct behavior for a complex command.
staking-cli/src/transaction.rs (97-244)
The calldata method is a well-designed core component of the Transaction enum. By consolidating the logic for generating contract addresses and encoded calldata for all transaction types, it ensures consistency and correctness across both execution and export modes. The handling of different StakeTableContractVersion for RegisterValidator and UpdateConsensusKeys is particularly robust.
staking-cli/src/signature.rs (293-295)
This new check ensures that an address is explicitly provided when using direct keys for signing, which is essential for correctly creating NodeSignatures in scenarios where a wallet might not be available (e.g., export_calldata mode).
staking-cli/README.md (183-186)
The security recommendations for using hardware wallets for mainnet funds are excellent advice.
staking-cli/src/cli.rs (172-186)
The resolve_node_signatures function centralizes the logic for obtaining NodeSignatures, handling both direct key input and pre-prepared payloads, and correctly integrating with the export_calldata mode. This is a good consolidation of logic.
staking-cli/src/cli.rs (445-450)
This check for StakeTableContractVersion::V1 when export_calldata is enabled is important. It prevents unsupported operations and provides a clear error message to the user, improving the CLI's robustness.
staking-cli/src/claim.rs (128-150)
The new fetch_claim_rewards_inputs function is a good addition. It encapsulates the logic for fetching reward claim data and constructing a Transaction::ClaimRewards enum variant, which aligns with the overall refactoring to use the Transaction enum for all state-changing operations.
staking-cli/src/cli.rs (579)
Calling tx.validate_delegate_amount(&readonly_provider).await? even in export_calldata mode is a good practice. It ensures that basic validation logic runs early, catching potential errors before the calldata is exported or executed, which improves user experience and reduces wasted effort.
staking-cli/src/cli.rs (586-593)
The logic for requiring --sender-address for calldata simulation, but allowing it to be skipped with --skip-simulation, provides flexibility while maintaining a default level of safety. This is a well-thought-out design for the calldata export feature.
staking-cli/src/funding.rs (1-36)
The removal of funding.rs is a positive change, as its functionality has been successfully integrated into the new Transaction enum or handled directly via TransactionRequest. This reduces code duplication and centralizes transaction logic.
staking-cli/src/info.rs (83-94)
The new fetch_stake_table_version function centralizes the logic for retrieving the contract version, which is crucial for version-aware transaction handling in the Transaction enum.
staking-cli/src/lib.rs (21-36)
Changing the visibility of most modules to pub(crate) significantly reduces the public API surface of the library, as mentioned in the PR description. This is a good practice for encapsulation and maintainability, making it clearer which parts are internal implementation details.
staking-cli/src/lib.rs (80-87)
The export_calldata flag with conflicts_with_all is well-defined. It correctly ensures that when calldata export is enabled, no signer-related flags are used, which is logical as no transaction is being sent directly.
staking-cli/src/lib.rs (90-93)
The sender_address option is crucial for calldata simulation, allowing users to specify the address that would initiate the transaction on-chain. This enhances the accuracy of pre-execution checks.
staking-cli/src/lib.rs (95-98)
The skip_simulation flag, with its requires = "export_calldata" constraint, provides necessary flexibility for advanced users who might want to bypass eth_call validation during calldata export. This is a good balance between safety and control.
staking-cli/src/main.rs (1-504)
The complete refactoring of main.rs to delegate all logic to staking_cli::run() is an excellent architectural improvement. It significantly cleans up the entry point, making it lean and focused, while centralizing the application's core logic in the cli.rs module. This enhances maintainability and testability.
staking-cli/src/output.rs (38-74)
The CalldataInfo struct and output_calldata function are essential for the new calldata export feature. They provide a structured way to represent and output transaction data in various formats (JSON, TOML, human-readable), which is crucial for multisig wallet integration.
staking-cli/src/signature.rs (42-55)
The removal of NodeSignaturesSol is a good simplification. By handling the Solidity-specific serialization directly within the Transaction enum's calldata method, the need for this intermediate struct is eliminated, reducing complexity and potential for inconsistencies.
staking-cli/src/signature.rs (279-282)
Changing the TryFrom implementation for NodeSignatureInput to accept Option<Address> instead of &EthereumWallet is a crucial change. This allows NodeSignatureInput to be created without a wallet, which is necessary for the export_calldata mode where no signer is present.
contracts/rust/adapter/src/evm.rs (25-35)
The addition of this impl block for Result<T, RpcError<TransportErrorKind>> is a significant improvement. It allows for consistent error decoding for RPC-related errors, aligning with the existing contract error decoding. This enhances the robustness of error handling.
staking-cli/README.md (245-300)
The new "Calldata Export (for Multisig Wallets)" section is well-detailed, providing examples for different output formats and explaining the simulation behavior. This is a critical feature for multisig users.
staking-cli/src/cli.rs (1-634)
Moving the main CLI logic from main.rs to cli.rs significantly improves the project's modularity and encapsulation. This makes the codebase easier to navigate, understand, and maintain, as the core application logic is now separated from the entry point.
staking-cli/src/transaction.rs (257-279)
The validate_delegate_amount method is a crucial addition for pre-emptively catching invalid delegation amounts. By integrating this validation directly into the Transaction enum, it ensures that checks are performed consistently, even in calldata export mode, improving user feedback and preventing failed on-chain transactions.
staking-cli/src/transaction.rs (281-296)
The decode_revert method provides a centralized and type-safe way to decode contract-specific revert errors based on the transaction type. This significantly improves error reporting and debugging capabilities for on-chain interactions.
staking-cli/src/transaction.rs (299-305)
The simulate method is a powerful feature for the calldata export mode. By performing an eth_call simulation, it allows the CLI to catch potential transaction failures before they are even submitted to a multisig, greatly enhancing the reliability and user experience of the export feature.
staking-cli/tests/cli.rs (128-148)
This test test_cli_delegate_error_decoding further confirms that error decoding is consistent across execution and simulation modes for delegation-related errors.
staking-cli/tests/cli.rs (1100-1126)
This new test test_cli_create_config_file_private_key is essential for verifying that the CLI can correctly initialize a configuration file using a raw private key, ensuring the new feature works as expected.
staking-cli/tests/cli.rs (1128-1148)
The test_cli_register_validator_private_key test confirms that validator registration works correctly when using a private key for signing, which is a core part of the new functionality.
staking-cli/tests/cli.rs (1150-1169)
The test_cli_export_calldata_delegate test verifies the basic functionality of exporting calldata for a delegation transaction, ensuring the output format is as expected.
staking-cli/tests/cli.rs (1171-1197)
The test_cli_export_calldata_json test specifically checks the JSON output format for calldata export, ensuring that the to, data, and value fields are present and correctly formatted.
staking-cli/tests/cli.rs (1199-1225)
The test_cli_export_calldata_toml test specifically checks the TOML output format for calldata export, ensuring that the to, data, and value fields are present and correctly formatted.
staking-cli/README.md (174-175)
Adding a security warning about private key leakage is a responsible and important addition, especially given the nature of staking operations.
staking-cli/tests/cli.rs (1296-1311)
The test_cli_export_calldata_no_signer test confirms that exporting calldata does not require a signer, which is the intended behavior for multisig scenarios.
staking-cli/tests/cli.rs (1313-1334)
This regression test test_cli_export_calldata_register_validator_direct_keys is important for ensuring that the calldata export feature works correctly when private keys are provided directly, not just via node signatures.
staking-cli/tests/cli.rs (1336-1358)
This regression test test_cli_export_calldata_update_consensus_keys_direct_keys is important for ensuring that the calldata export feature works correctly when private keys are provided directly for updating consensus keys.
staking-cli/tests/cli.rs (1360-1382)
The test_cli_export_calldata_requires_sender_address_for_simulation test correctly verifies that --sender-address is mandatory for calldata simulation, enforcing a safe default behavior.
staking-cli/tests/cli.rs (1384-1406)
The test_cli_skip_simulation_does_not_require_sender_address test confirms that the --skip-simulation flag correctly bypasses the --sender-address requirement, providing flexibility for users.
contracts/rust/adapter/src/evm.rs (66-87)
This new test case test_decode_revert_rpc_error is excellent. It specifically verifies the functionality of the newly added DecodeRevert implementation for RpcError<TransportErrorKind>, ensuring that RPC errors are correctly decoded and handled.
staking-cli/tests/cli.rs (1429-1447)
The test_cli_export_calldata_claim_rewards test verifies that calldata export works for the claim-rewards command, ensuring this important operation is covered by the new feature.
staking-cli/tests/cli.rs (1449-1468)
The test_cli_export_calldata_validation_succeeds test confirms that the simulation logic correctly identifies valid transactions, ensuring that the validation step is not overly restrictive.
staking-cli/tests/cli.rs (1470-1688)
The test_cli_export_calldata_all_operations_manual_inspect test is a comprehensive manual inspection test. By exercising all export-calldata commands and printing their output, it provides a valuable way to visually verify the correctness of the generated calldata across the entire range of state-changing operations. This is excellent for ensuring the quality of the new feature.
staking-cli/tests/common/mod.rs (62-71)
The export_calldata_cmd function provides a standardized way to create a base command for calldata export tests, including necessary RPC URL, stake table address, and sender address for simulation. This reduces boilerplate in individual tests.
alysiahuggins
left a comment
There was a problem hiding this comment.
lgtm, however, i didn't manually test this
…with no balance Add setup_reward_claim_not_found_mock helper and parameterize reward tests with rstest to cover both with-balance and no-balance cases.
|
I'll merge this now if CI passes and then test once I get sepolia Eth. Since simulations in the safe UI seem to work I don't see why things shouldn't work. |
|
Delegation from multisig: https://sepolia.etherscan.io/tx/0x0ab4df76c025b0cdecdde5833862e6f1b4e437642dfdaf20769390871d50142f To use, send ESP to multisig, then approve Copy data to safe UI and sign, exec. Then delegate Use multisig address as Now have to wait a bit to test reward claims. |
|
And here's a reward claim: https://sepolia.etherscan.io/tx/0x4a6675561c8a4ee172ee7740cba041e074f9f25ee7ac3f9c2fc22390599e4048 |
Add --private-key flag for raw key signing alongside mnemonic/ledger.
Add --export-calldata mode for Safe multisig users to output transaction data instead of executing. Validates via eth_call simulation using --sender-address (skip with --skip-simulation). Supports JSON, TOML, and human-readable output formats.
Consolidate all state-changing operations into Transaction enum with unified calldata generation for both execute and export modes.
Other changes:
A simulated
RegisterValidatorV2transaction can be seen here: https://dashboard.tenderly.co/public/safe/safe-apps/simulator/22046676-7878-4e76-8782-be9bf66b8af7Sorry, this is quite a big diff but we have very good test coverage for the staking-cli. The main two reasons for refactoring a lot are:
main.rswe had to have a really big public API in the staking-cli. So moving this to the lib (cli.rs) we can avoid this and avoid exposing most of the crate.