feat(platform-wallet): classic Dash message signing (signMessage) over FFI, JNI, Kotlin, and Swift - #4319
Conversation
…r FFI, JNI, Kotlin, and Swift Squash of PR #4259 (feat/kotlin-sdk-sign-message, head 09a27ac), rebased onto v4.2-dev. CoreWallet::sign_message — classic Dash signed messages, exposed through the full binding chain (Rust -> C FFI -> Kotlin/JNI and Swift). Given a P2PKH address the wallet holds keys for and an arbitrary UTF-8 message, returns the 65-byte recoverable signature base64-encoded — the same wire format as dashj's ECKey.signMessage and Dash Core's signmessage RPC. Includes digest-capability guard, typed errors (FFI code 31 ErrorSigningKeyUnavailable), null-pointer empty-message ABI contract, unpaired-surrogate rejection in Kotlin, and dashj byte-for-byte parity goldens. Rebase resolution: the TxMetadataPayloadTooLarge FFI mapping arm was dropped — the variant it matched was removed from v4.2-dev by the encrypted-txMetadata revert (#4279), so the arm no longer compiles and the error cannot be produced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kotlin-sdk porting guideline: when porting behavior from iOS, cite the Swift source file in the KDoc, as the neighbouring methods do (sendToAddresses -> SendViewModel.swift, tokens -> TokenActions.swift). Names ManagedCoreWallet.signMessage(address:message:) and explains the one signature divergence (Swift builds a per-call MnemonicResolver, so it takes no signer parameter). Review follow-up from the original PR (#4259). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds classic Dash message signing to CoreWallet and exposes it through Rust FFI, JNI, Kotlin, and Swift APIs. It validates addresses and message encoding, returns base64 recoverable signatures, maps signing errors, and adds compatibility and regression tests. ChangesClassic Dash message signing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SDK
participant JNI
participant FFI
participant CoreWallet
participant Signer
SDK->>JNI: request message signature
JNI->>FFI: pass address, message bytes, and signer handle
FFI->>CoreWallet: invoke sign_message
CoreWallet->>Signer: sign Dash message digest
Signer-->>CoreWallet: return signature data
CoreWallet-->>FFI: return base64 recoverable signature
FFI-->>JNI: return owned native string
JNI-->>SDK: return signature or mapped error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🔍 Review in progress — actively reviewing now (commit 2eb6dd2) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4319 +/- ##
=========================================
Coverage 87.61% 87.61%
=========================================
Files 2704 2704
Lines 345206 345206
=========================================
Hits 302446 302446
Misses 42760 42760
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs (1)
164-166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRun message signing through
block_on_worker. ConstructMnemonicResolverCoreSignerinside itsasync moveclosure so the signer remains alive for the synchronous call and the future satisfiesSend + 'static. This uses the configured worker stack.block_on_workerstill usesruntime().block_oninternally, so it does not make calls from an existing Tokio runtime safe.🤖 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 `@packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs` around lines 164 - 166, Update the message-signing flow around wallet.sign_message to call it through block_on_worker instead of runtime().block_on. Construct MnemonicResolverCoreSigner inside the async move closure, keeping the signer alive for the synchronous call and ensuring the future is Send + 'static while using the configured worker stack.Source: Learnings
🤖 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.
Inline comments:
In `@packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- Around line 1093-1095: Update the `env.new_string(signature)` handling in the
signing function to throw a `DashSdkException` through the JNI environment when
string creation fails before returning the null pointer. Preserve the existing
successful `into_raw()` path and ensure the failure path matches the other
exception-first returns and the documented null-after-throw contract.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs`:
- Around line 164-166: Update the message-signing flow around
wallet.sign_message to call it through block_on_worker instead of
runtime().block_on. Construct MnemonicResolverCoreSigner inside the async move
closure, keeping the signer alive for the synchronous call and ensuring the
future is Send + 'static while using the configured worker stack.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f8c54e0-e7ce-4ff5-82e3-0af557d79db8
📒 Files selected for processing (15)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/sign_message.rspackages/rs-unified-sdk-jni/src/wallet_manager.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
…JNI string-allocation failure Two review follow-ups from the first round on this PR: - core_wallet_sign_message now polls through block_on_worker instead of runtime().block_on, so the signing round-trip (which includes proof-free but recursion-happy key derivation in the resolver path) runs on a runtime worker with the 8 MB stack rather than the caller's thread — iOS dispatch threads default to ~512 KB. The MnemonicResolverCoreSigner is constructed inside the async move block, so the future is Send + 'static without borrowing the caller's frame; the calling thread still blocks until completion, so the resolver handle stays pinned for the signer's whole lifetime. - The JNI coreWalletSignMessage now throws DashSdkException when env.new_string fails instead of silently returning null. Kotlin declares a non-null return, so the bare null surfaced as an unexplained NullPointerException at the platform-type boundary, breaking the KDoc's "null only after throwing" contract every other failure path in the function honors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Union-resolves module/test-helper both-sides-adds with #4319 (signMessage); adapts its merged-in test fixtures to this branch's PlatformWalletInfo.generation (Arc<WalletGeneration> replaced the bare balance Arc as the per-generation identity). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Supersedes #4259, which lives on an external fork and had merge conflicts with
v4.2-dev. This branch remakes it in-repo as two commits:09a27ac60d),rebased onto
v4.2-dev.All review findings on #4259 were addressed there before its final approval —
the digest-capability guard, FFI lock lifetime, null-pointer empty-message ABI
test, UTF-8 length-prefix docs, unpaired-surrogate rejection, the
MessageSigningMessageInvalid→ErrorInvalidParametermapping, and theMessageSigningFaileddoc correction. thepastaclaw approved at09a27ac60d.What this adds
CoreWallet::sign_message— classic Dash signed messages ("proof you own thisaddress"), exposed through the full binding chain. Given a P2PKH address the
wallet holds keys for and an arbitrary UTF-8 message, it returns the 65-byte
recoverable signature, base64-encoded — the same wire format as dashj's
ECKey.signMessageand Dash Core'ssignmessageRPC, so existing verifiers(
verifymessage, dashjsignedMessageToKey) accept it as-is.Primary consumer: the Android/iOS wallets' CrowdNode integrations, where API
withdrawal and email registration are signed-message proofs of address
ownership (currently done in dashj on Android; this makes the kotlin-sdk /
swift-sdk route possible).
API surface
CoreWallet::sign_message(&self, address, message, signer) -> Result<String>core_wallet_sign_message(handle, address_ptr/len, message_ptr/len, core_signer_handle, out_signature)ManagedPlatformWallet.signMessage(address: String, message: String, coreSignerHandle: Long): String(suspend)ManagedCoreWallet.signMessage(address: String, message: String) throws -> String(Swift takes no signer parameter deliberately — the swift-sdk convention is a
per-call internal
MnemonicResolver, as at every other seed-backed call site.)Design decisions
serialization come from
dashcore::sign_message; the recovery id is foundby trying the four candidates against the signer's public key — the same
approach dashj itself uses (
ECKey.findRecoveryId). EverySignerbackendgets signed-message support without needing a recoverable-signing method.
SignerMethod::Digestbeforesign_ecdsais called; a refusal maps to thetyped
MessageSigningFailed.core_wallet_sign_messagetakes thecaller's existing
MnemonicResolverHandle, exactly like the send paths.with a typed error; BIP44/BIP32/CoinJoin and DashPay receiving accounts sign.
(
ErrorSigningKeyUnavailable); malformed address or non-UTF-8 message bytes→
ErrorInvalidParameter; mirrored in Kotlin(
DashSdkError.PlatformWallet.SigningKeyUnavailable) and Swift(
signingKeyUnavailable).Arc-backed
CoreWalletout ofHandleStorageand runsblock_onoutsidethe guard, matching the broadcast paths.
Deltas vs #4259 (the fixup commit + rebase resolution)
TxMetadataPayloadTooLargeFFI arm dropped (rebase resolution, in thesquash commit): the variant it matched was removed from
v4.2-devby theencrypted-txMetadata revert (Revert "feat(platform-wallet): add encrypted txMetadata document support" #4279), so the arm no longer compiles and the
error cannot be produced. This also resolves CodeRabbit's scope note on
feat(platform-wallet): classic Dash message signing (signMessage) over FFI, JNI, Kotlin, and Swift #4259.
ManagedCoreWallet.signMessage) perthe kotlin-sdk porting guideline — the remaining CodeRabbit nitpick.
nextReceiveAddress/nextChangeAddress): both featureskept side by side in
ManagedCoreWallet.kt.Verification
signed-message test vector), unchanged across the port.
cargo test -p platform-wallet -p platform-wallet-ffigreen on the mergedtree.
:sdk:compileDebugKotlinclean.cargo fmt --allproduced no diff.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes