fix(platform-wallet): type signer-reported missing key as MessageSigningKeyUnavailable - #4321
Conversation
…ingKeyUnavailable
A missing private key reported by the signer during classic message
signing was wrapped into MessageSigningFailed with context text
prepended before the reserved key-unavailable marker, so the FFI
boundary could no longer recognize the condition and it flattened to
ErrorUnknown instead of ErrorSigningKeyUnavailable (31).
Close it with both halves of the marker contract, in-repo:
- MnemonicResolverCoreSigner::NotFound (the production missing-key
completion) now renders with DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX
at position 0 — the one typed signal a Display-only Signer::Error can
carry.
- CoreWallet::sign_message checks that prefix on the signer's unwrapped
rendering BEFORE composing the "signer rejected the digest at {path}"
context, and returns the typed
PlatformWalletError::MessageSigningKeyUnavailable, which the FFI
already maps to code 31 without parsing any formatted reason. The
check stays position-0 only; a mid-string mention of the marker still
wraps as MessageSigningFailed (per the #4183 review rule).
Flips signer_key_unavailable_is_not_preserved_during_message_signing —
which pinned the old limitation — to
signer_key_unavailable_is_typed_during_message_signing asserting the
typed result, and adds producer (marker at position 0 of NotFound) and
guard (mid-string marker not promoted) tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesMessage-signing error propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MnemonicResolverCoreSigner
participant WalletCoreSignMessage
participant WalletFFI
MnemonicResolverCoreSigner->>WalletCoreSignMessage: return marker-prefixed NotFound error
WalletCoreSignMessage->>WalletCoreSignMessage: classify position-zero marker
WalletCoreSignMessage->>WalletFFI: convert MessageSigningKeyUnavailable
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 8f067da) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4321 +/- ##
============================================
- Coverage 87.61% 87.61% -0.01%
============================================
Files 2704 2704
Lines 345206 345211 +5
============================================
+ Hits 302445 302446 +1
- Misses 42761 42765 +4
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Opus
This is a narrowly-scoped, well-tested fix: it makes MnemonicResolverSignerError::NotFound stamp the reserved key-unavailable marker at position 0 of its Display, and teaches sign_message to check that marker (position-0 only, correctly avoiding the #4183 substring-sniff antipattern) before composing context, promoting the failure to the typed MessageSigningKeyUnavailable/FFI code 31. All reviewer lanes (Claude and Codex, general/security/rust-quality/ffi-engineer) converge on one real, verified issue: MessageSigningKeyUnavailable's doc comment was updated to describe two producers but its #[error(...)] Display text still only describes the original address-resolution producer, so hosts hitting the new signer-reported-missing-key path see a factually wrong diagnostic ('it belongs to no signable funds account of this wallet') surfaced verbatim via error.to_string() in the FFI result. No other in-scope issues were found; out-of-scope architectural/hardening notes from Codex/Claude specialists were reviewed and excluded as pre-existing or overly broad. Source: reviewer backend model gpt-5.6-sol via Codex (general, security-auditor, rust-quality, ffi-engineer) and claude-sonnet-5 via Claude (general, security-auditor, ffi-engineer, rust-quality retry3); verifier backend model claude-sonnet-5 via Claude (final-verifier). openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Opus reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:142-146: MessageSigningKeyUnavailable's Display text is stale for its new second producer
The doc comment above `MessageSigningKeyUnavailable` was correctly updated (lines 123-140) to describe two producers: (1) address resolution finding no signable funds account, and (2) the signer itself reporting a missing key via the position-0 marker — the new behavior `sign_message` adds in this PR. The `#[error(...)]` attribute that generates `Display`, however, was left unchanged: "no signing key for message-signing address {address}: it belongs to no signable funds account of this wallet". That text hardcodes producer (1)'s explanation only. After this PR, `sign_message` (packages/rs-platform-wallet/src/wallet/core/sign_message.rs) also constructs this variant when `sign_ecdsa` fails and the signer's rendered error starts with `SIGNER_KEY_UNAVAILABLE_PREFIX` — in that case a derivation path WAS resolved (the address does belong to a signable account) and `sign_ecdsa` was actually invoked; the real cause is the signer/keychain having no stored key (e.g. `MnemonicResolverCoreSigner::NotFound`, no mnemonic present). `PlatformWalletFFIResult::from`/`err(code, error.to_string())` (packages/rs-platform-wallet-ffi/src/error.rs) forwards this exact string to FFI hosts alongside code 31, and Swift/Kotlin bindings preserve it. A host debugging a signer-side key-repair scenario will see a message falsely claiming the address has no signable account, misdirecting troubleshooting toward address correction instead of key/mnemonic restoration.
2 participants
Issue being fixed or feature implemented
During classic message signing (
CoreWallet::sign_message, added in #4319), everysign_ecdsafailure was wrapped intoMessageSigningFailedwith context text prepended, which buried the reserved key-unavailable marker mid-string. Since the FFI boundary only recognizes the marker structurally at position 0 (never as a substring — per the #4183 review rule), a missing private key surfaced asErrorUnknowninstead of the typedErrorSigningKeyUnavailable(31), losing the host's key-repair routing. Flagged by CodeRabbit.What was done?
Key unavailability is now a typed condition end to end; the FFI boundary never parses formatted reasons. Both halves of the marker contract are closed in-repo:
rs-sdk-ffi):MnemonicResolverSignerError::NotFound— the production missing-key completion (no mnemonic stored for the wallet) — now renders withDASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIXat position 0. Since key-wallet'sSigner::Erroris bounded only byDisplay, the start-of-rendering marker is the one typed signal that surface can carry.rs-platform-wallet):sign_messagechecks that prefix on the signer's unwrapped rendering BEFORE composing the"signer rejected the digest at {path}: …"context, and returns the typedPlatformWalletError::MessageSigningKeyUnavailable, which the FFI already maps to code 31 structurally. The check remains position-0 only; a mid-string mention of the marker still wraps asMessageSigningFailed.rs-platform-wallet's error docs andplatform-wallet-ffi's conversion NOTE, and thecore_wallet_sign_messagedoc to name the new code-31 producer.How Has This Been Tested?
signer_key_unavailable_is_not_preserved_during_message_signing— which pinned the old limitation and instructed "flip me when this becomes reachable" — is flipped tosigner_key_unavailable_is_typed_during_message_signing, assertingMessageSigningKeyUnavailablewith the correct address.NotFound's marker at position 0 (rs-sdk-ffi); new guard test pins that a mid-string marker is NOT promoted (the feat(kotlin-sdk)!: keystore rework — policy-alias split, layered key recovery, durable repair, structured signer errors (stacked on #4191) #4183 substring-sniff rule).cargo test -p platform-wallet --lib(525 passed),cargo test -p rs-sdk-ffi --lib(308 passed),cargo test -p platform-wallet-ffi --lib(227 passed, includes the FFI mapping tests);cargo fmtandcargo clippy --all-targetsclean on the touched crates.Breaking Changes
None.
MessageSigningKeyUnavailableand its FFI code 31 already exist and are already mirrored in the host SDKs; this only adds a producer for it. TheNotFoundDisplaystring gains the machine prefix, which no code matched as a full string.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation