fix(key-wallet): actionable error when add_account(_, None) hits a keyless wallet#847
Open
lklimek wants to merge 2 commits into
Open
fix(key-wallet): actionable error when add_account(_, None) hits a keyless wallet#847lklimek wants to merge 2 commits into
lklimek wants to merge 2 commits into
Conversation
… wallets
Deriving an account from an in-wallet private key on a watch-only or
external-signable wallet used to bubble up a cryptic
InvalidParameter("External signable wallet has no private key"), which
got misdiagnosed as a production bug three times instead of being read
as a usage error.
Add a typed, pattern-matchable Error::KeylessWalletRequiresAccountKey
{ account_type, required_key } and map ONLY the keyless-wallet failure
of root_extended_priv_key() to it in the derive-from-root branch of
add_account / add_bls_account / add_eddsa_account. Its Display names
its own remedy: supply the account's xpub/seed via the Some(..)
argument. The None hot-wallet convenience path, the Some(..) branch,
and every other error path are left unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BqMDY7ZZNwSvdPgBrJQQNq
Contributor
📝 WalkthroughWalkthroughAdds a new ChangesKeyless wallet account error
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet/src/wallet/accounts.rs (1)
46-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated derive-or-error pattern into a helper.
The
self.root_extended_priv_key().map_err(|_| Error::KeylessWalletRequiresAccountKey { account_type, required_key: "..." })?block is duplicated verbatim (aside from therequired_keystring) acrossadd_account,add_bls_account, andadd_eddsa_account. A small private helper (e.g.fn require_root_priv_key(&self, account_type: AccountType, required_key: &'static str) -> Result<ExtendedPrivKey>) would remove the duplication and keep the mapping consistent if the logic ever changes.♻️ Example helper
+ fn require_root_priv_key( + &self, + account_type: AccountType, + required_key: &'static str, + ) -> Result<ExtendedPrivKey> { + self.root_extended_priv_key().map_err(|_| Error::KeylessWalletRequiresAccountKey { + account_type, + required_key, + }) + }Also applies to: 111-116, 180-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/wallet/accounts.rs` around lines 46 - 51, The duplicate root-key error mapping in add_account, add_bls_account, and add_eddsa_account should be extracted into a small private helper so the derive-or-error logic is centralized. Add a helper on the accounts implementation, such as require_root_priv_key, that calls root_extended_priv_key and converts failures into KeylessWalletRequiresAccountKey using the supplied AccountType and required_key string. Then replace each inline map_err block with the helper so the behavior stays identical while removing the repeated pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@key-wallet/src/wallet/accounts.rs`:
- Around line 46-51: The duplicate root-key error mapping in add_account,
add_bls_account, and add_eddsa_account should be extracted into a small private
helper so the derive-or-error logic is centralized. Add a helper on the accounts
implementation, such as require_root_priv_key, that calls root_extended_priv_key
and converts failures into KeylessWalletRequiresAccountKey using the supplied
AccountType and required_key string. Then replace each inline map_err block with
the helper so the behavior stays identical while removing the repeated pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31fb0bff-18cc-45e9-844f-a361b0a93a08
📒 Files selected for processing (3)
key-wallet/src/error.rskey-wallet/src/tests/unit_variant_wallet_tests.rskey-wallet/src/wallet/accounts.rs
Collaborator
|
@lklimek CI is broken here. |
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
2 participants
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.
Why this PR exists
add_account(account_type, None)— and its BLS/EdDSA siblings — fail deep insideroot_extended_priv_key()with a bare, non-actionable string:"External signable wallet has no private key". The message says what broke, never what to do.add_account(_, Some(xpub)). A keyless-wallet caller gets no signal pointing them at the right overload; the failure reads like a missing capability rather than an API misuse.What was done
Error::KeylessWalletRequiresAccountKey { account_type, required_key }(manual-Display, matching the crate's existing hand-rolledError; fully pattern-matchable).add_account(xpub),add_bls_account(32-byte BLS seed) andadd_eddsa_account(32-byte Ed25519 seed), the keyless-wallet failure now maps to this variant, whose message names the remedy: supply the account's key material via theSome(..)argument.root_extended_priv_key()errors solely on keyless wallets, so theSome(..)path, all derivation logic, and every other error path are untouched. TheNoneAPI shape (the legitimate hot-wallet convenience path) is preserved.Testing
add_account(_, None)on watch-only + external-signable wallets returns the new variant and its message names theSome(..)remedy (all three siblings, feature-gated for BLS/EdDSA). 11/11 in-module pass.cargo build -p key-walletclean ·cargo clippy -p key-wallet --all-targets -- -D warningsclean ·cargo fmtapplied.matchonErrorbreaks (all have_ =>arms).Breaking changes
None — additive
Errorvariant only.Checklist
-D warnings);cargo fmtapplied🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes