Add --keystore flag for signing with an encrypted keystore - #795
Open
palango wants to merge 2 commits into
Open
Conversation
celocli could sign with a raw private key or a Ledger, but had no way to use an encrypted keystore file, which is how geth and cast users keep keys on disk. --keystore takes a keystore file, or a directory of them in which case --from selects the account. The password comes from --passwordFile, or a hidden prompt when that is not given. Resolution is memoized so a keystore is decrypted, and its password requested, once per command. The resolved key flows into the same paths --privateKey already uses, so both the ContractKit and viem signing paths are covered without further branching. Export the V3 primitives from @celo/keystores so the CLI can decrypt a single file without going through FileKeystore.
Both --useLedger and --useAKV now declare --keystore as exclusive, matching the reciprocal style of the other signer flags so the conflict shows up in generated help from either side. Enforcement already worked one-sided. Add tests for the BaseCommand wiring itself: that the wallet client signs with the keystore's address, that the password is requested once however many times the key is needed, and that a mismatched --from is reported against the keystore. Only the public client is stubbed; the key really is decrypted and its address really is derived. Bump @celo/keystores to minor, since exporting the V3 primitives adds public API.
🦋 Changeset detectedLatest commit: 0874c69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
size-limit report 📦
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## palango/keystores-drop-ethereumjs-wallet #795 +/- ##
============================================================================
- Coverage 69.61% 69.03% -0.58%
============================================================================
Files 213 153 -60
Lines 11312 8911 -2401
Branches 1973 1672 -301
============================================================================
- Hits 7875 6152 -1723
+ Misses 3349 2688 -661
+ Partials 88 71 -17
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #792 — base that PR's branch, not
master. Merge #792 first and this will retarget cleanly.Why
celocli could sign with a raw private key or a Ledger, but had no way to use an encrypted keystore file — which is how geth and
castusers actually keep keys on disk.--privateKeyputs the key in shell history and the process table; a keystore does not.Incidentally this gives
@celo/keystoresits first consumer. Nothing in the monorepo imported it before.Usage
--keystoreis mutually exclusive with--privateKey,--useLedgerand--useAKV.Design
The resolved key flows into the same slot
--privateKeyalready used, so both the ContractKit path (getKit) and the viem path (getWalletClient) are covered by one change rather than two new branches. Resolution is memoized, so a keystore is decrypted — and its password requested — once per command, no matter how many times the key is needed.Deliberate divergences from
cast--passwordFile, not--password-file. Every multi-word flag in this CLI is camelCase (--privateKey,--useLedger,--derivationPath).--passwordin cleartext. It leaks into shell history andps.castnames its equivalent--unsafe-passwordon most subcommands, which says enough.ETH_KEYSTORE/ETH_PASSWORDenv vars. No flag in this CLI currently uses oclif'senv:, andcast's own tracker has a standing bug where a strayETH_KEYSTOREhijacks an explicit--private-key(foundry#2691).cast's help text claims "folder or file" but its code explicitly rejects directories — its docs are stale. Supporting them here is a small superset.--account. That only makes sense withcast's~/.foundry/keystoresconvention, which Celo has no equivalent of.FileKeystoreis deliberately not used: it appendskeystore/to whatever directory it is given and creates directories as a side effect, neither of which suits a flag pointing straight at a keystore directory.Verification
20 automated tests.
utils/keystore.test.tscovers the resolver against realencryptV3/decryptV3round-trips — file and directory forms, case-insensitive--from, wrong password, non-keystore files skipped, empty directory, password-file trailing newline, prompt vs no-prompt.base.test.tscovers flag exclusivity and the wiring itself (wallet client address, password asked once,--frommismatch). Only the public RPC client is ever stubbed; every key is really decrypted and every address really derived.Also exercised end-to-end through the built binary against Celo Sepolia — file, directory, and interactive prompt all reach:
failing only on the empty test account, as it should.
Two things a reviewer should know
--keystoredoes not work withbridge:*. Those commands bypassBaseCommandand build their own signer. Tracked in Bridge commands duplicate signer construction and miss new signer types #794, deliberately out of scope here.anvil 1.7.1cannot parse@celo/devchain-anvil's checked-inl2-devchain.json(data did not match any variant of untagged enum SerializableTransactionType). This is pre-existing and unrelated — untouched test files such ascommands/account/balance.test.tsfail identically onmaster. Every test added here was written to run without anvil, and all 20 pass.PR-Codex overview
This PR introduces a
--keystoreflag for the CLI, allowing users to sign transactions with an encrypted keystore file. It enhances the@celo/keystorespackage by exporting V3 keystore primitives and adds functionality for managing keystores in commands.Detailed summary
--keystoreflag for transaction signing.@celo/keystorespackage version inpackage.json.passwordFileoption for keystore commands.privateKeyFromKeystorefunction for better error handling.BaseCommandto integrate keystore signing logic.