feat(platform-wallet): classic Dash message signing (signMessage) over FFI, JNI, Kotlin, and Swift - #4259
Conversation
📝 WalkthroughWalkthroughAdded classic Dash message signing for wallet-owned P2PKH addresses. The implementation spans CoreWallet, Rust FFI, Kotlin, JNI, and Swift APIs. It returns Base64 recoverable signatures and maps unavailable signing keys to native error code 31. ChangesClassic Dash message signing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedPlatformWallet
participant WalletManagerNative
participant core_wallet_sign_message
participant CoreWallet
participant Signer
ManagedPlatformWallet->>WalletManagerNative: Request signature
WalletManagerNative->>core_wallet_sign_message: Pass address, message, and signer handle
core_wallet_sign_message->>CoreWallet: Invoke sign_message
CoreWallet->>Signer: Sign Dash message digest
Signer-->>CoreWallet: Return recoverable signature
CoreWallet-->>core_wallet_sign_message: Return Base64 signature
core_wallet_sign_message-->>WalletManagerNative: Return allocated signature
WalletManagerNative-->>ManagedPlatformWallet: Return signature
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
🔍 Review in progress — actively reviewing now (commit 9336bdb) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The signed-message implementation is cryptographically aligned with Dash Core and dashj, and its address-ownership and recovery-ID checks are sound. One blocking issue remains: the public generic Rust API ignores the signer's advertised capabilities and can invoke blind digest signing on a backend that explicitly disallows it. Non-blocking follow-ups cover binding documentation, FFI lock lifetime and ABI regression coverage, and malformed JVM strings.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
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:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 4 suggestion(s)
🤖 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/wallet/core/sign_message.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/sign_message.rs:189-196: Honor the signer's digest capability before signing
`key_wallet::signer::Signer` explicitly assigns capability dispatch to callers and documents `sign_ecdsa` as valid only when the signer advertises `SignerMethod::Digest`. This public generic method calls it unconditionally. The production mnemonic resolver supports digest signing, but a legitimate transaction-only hardware signer may reject the call, panic because the method is unreachable, or blind-sign a host-computed digest despite advertising a policy that forbids that operation. Fail before entering the backend, and add a regression signer that advertises only `Transaction(...)` and panics if `sign_ecdsa` is invoked.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:182-184: Document the signed message's UTF-8 byte length
`signed_msg_hash` prefixes `msg.len()`, which is the serialized UTF-8 byte count. Kotlin's `message.length` counts UTF-16 code units, so the documented formula is wrong for non-ASCII messages; for example, `"é"` has length 1 but contributes 2 UTF-8 bytes. State that the varint contains `message.toByteArray(Charsets.UTF_8).size`. The analogous Swift documentation at `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:145` must use `message.utf8.count` instead of `message.count`, which counts extended grapheme clusters.
In `packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs:109-138: Release the global Core-wallet storage lock before signing
`HandleStorage::with_item` retains the global storage map's read guard throughout the closure. This closure performs `runtime().block_on(...)` and invokes the host-owned mnemonic resolver callback, so a potentially slow signing operation prevents insertion or removal of every Core-wallet handle. A callback that re-enters an operation requiring the write lock can deadlock. `CoreWallet` is an inexpensive Arc-backed clone, and the broadcast entry points already clone it out of storage before blocking; use the same pattern here.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs:98-101: Cover the null-pointer empty-message ABI contract
The new FFI contract intentionally accepts `message_ptr == NULL` when `message_len == 0`, which is required by the Swift empty-array marshalling path. No endpoint test exercises this behavior, and the Rust wallet tests only sign `"hello"`. Add a direct `core_wallet_sign_message` regression test that passes a null message pointer with zero length, asserts success, and verifies the returned signature against `signed_msg_hash("")`; this prevents a future unconditional `check_ptr!(message_ptr)` from silently breaking empty messages.
In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:1063-1075: Do not silently alter malformed Java UTF-16 messages
`env.get_string(...).into()` decodes JNI modified UTF-8 through `jni::JavaStr`. For a Java/Kotlin `String` containing an unpaired UTF-16 surrogate, the JNI crate's decoder fails and falls back to `String::from_utf8_lossy`, silently replacing the input before its bytes are signed. A verifier encoding the same Java string through the platform UTF-8 encoder can therefore hash different replacement bytes, contradicting the API's claim that the message is signed verbatim. Marshal a Kotlin-produced UTF-8 `ByteArray`, or read the UTF-16 code units strictly and reject unpaired surrogates rather than signing transformed text.
| let hash = signed_msg_hash(message); | ||
| let (signature, public_key) = signer | ||
| .sign_ecdsa(&path, hash.to_byte_array()) | ||
| .await | ||
| .map_err(|e| PlatformWalletError::MessageSigningFailed { | ||
| address: target.to_string(), | ||
| reason: format!("signer rejected the digest at {path}: {e}"), | ||
| })?; |
There was a problem hiding this comment.
🔴 Blocking: Honor the signer's digest capability before signing
key_wallet::signer::Signer explicitly assigns capability dispatch to callers and documents sign_ecdsa as valid only when the signer advertises SignerMethod::Digest. This public generic method calls it unconditionally. The production mnemonic resolver supports digest signing, but a legitimate transaction-only hardware signer may reject the call, panic because the method is unreachable, or blind-sign a host-computed digest despite advertising a policy that forbids that operation. Fail before entering the backend, and add a regression signer that advertises only Transaction(...) and panics if sign_ecdsa is invoked.
| let hash = signed_msg_hash(message); | |
| let (signature, public_key) = signer | |
| .sign_ecdsa(&path, hash.to_byte_array()) | |
| .await | |
| .map_err(|e| PlatformWalletError::MessageSigningFailed { | |
| address: target.to_string(), | |
| reason: format!("signer rejected the digest at {path}: {e}"), | |
| })?; | |
| if !signer.supports(key_wallet::signer::SignerMethod::Digest) { | |
| return Err(PlatformWalletError::MessageSigningFailed { | |
| address: target.to_string(), | |
| reason: "signer does not support digest signing".to_string(), | |
| }); | |
| } | |
| let hash = signed_msg_hash(message); | |
| let (signature, public_key) = signer | |
| .sign_ecdsa(&path, hash.to_byte_array()) | |
| .await | |
| .map_err(|e| PlatformWalletError::MessageSigningFailed { | |
| address: target.to_string(), | |
| reason: format!("signer rejected the digest at {path}: {e}"), | |
| })?; |
source: ['codex']
There was a problem hiding this comment.
Addressed in 2965337. The method now checks that the signer advertises SignerMethod::Digest (via supports()) before calling sign_ecdsa, mirroring the check key-wallet itself performs in TransactionSigner::sig_and_pubkey. A refusal maps to the typed MessageSigningFailed with a precise reason — reused rather than a new variant because the production resolver advertises Digest (no host can currently hit this), and a new FFI code would spend one of the 27–30 slots reserved for the keystore rework numbering. The requested regression test is included: TransactionOnlySigner advertises only Transaction(Classical) and its sign_ecdsa panics, so a dropped guard fails loudly; it signs against a wallet-owned address so the refusal cannot be confused with the key-unavailable path.
| * **The format.** The signed digest is | ||
| * `SHA256d(prefix ‖ varint(message.length) ‖ message)`, where the prefix is | ||
| * the historical `"\x19DarkCoin Signed Message:\n"` — *not* `"Dash"`. Dash |
There was a problem hiding this comment.
🟡 Suggestion: Document the signed message's UTF-8 byte length
signed_msg_hash prefixes msg.len(), which is the serialized UTF-8 byte count. Kotlin's message.length counts UTF-16 code units, so the documented formula is wrong for non-ASCII messages; for example, "é" has length 1 but contributes 2 UTF-8 bytes. State that the varint contains message.toByteArray(Charsets.UTF_8).size. The analogous Swift documentation at packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:145 must use message.utf8.count instead of message.count, which counts extended grapheme clusters.
source: ['codex']
There was a problem hiding this comment.
Addressed in 4270d82 (docs only). The Kotlin doc now states the varint prefix is message.toByteArray(Charsets.UTF_8).size, and the Swift doc at ManagedCoreWallet.swift uses message.utf8.count — each with a note on why the language-native count (UTF-16 code units / grapheme clusters) diverges for non-ASCII input. The Rust module doc was already byte-accurate.
| let option = CORE_WALLET_STORAGE.with_item(handle, |wallet| { | ||
| let address = read_utf8(address_ptr, address_len, |e| { | ||
| PlatformWalletError::MessageSigningAddressInvalid { | ||
| address: "<non-UTF-8>".to_string(), | ||
| reason: format!("address is not valid UTF-8: {e}"), | ||
| } | ||
| })?; | ||
| // A non-UTF-8 message is reported against the (now known) address, so the | ||
| // error names the signing target the caller asked about. | ||
| let message = read_utf8(message_ptr, message_len, |e| { | ||
| PlatformWalletError::MessageSigningFailed { | ||
| address: address.clone(), | ||
| reason: format!("message is not valid UTF-8: {e}"), | ||
| } | ||
| })?; | ||
| let network = wallet.network(); | ||
| let wallet_id = wallet.wallet_id(); | ||
| // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller | ||
| // pinned alive for this call; the `MnemonicResolverCoreSigner` lives | ||
| // only on this stack frame and is dropped before returning. | ||
| let signer = MnemonicResolverCoreSigner::new( | ||
| signer_addr as *mut MnemonicResolverHandle, | ||
| wallet_id, | ||
| network, | ||
| ); | ||
| runtime().block_on(wallet.sign_message(&address, &message, &signer)) | ||
| }); | ||
|
|
||
| let result = unwrap_option_or_return!(option); | ||
| let signature = unwrap_result_or_return!(result); |
There was a problem hiding this comment.
🟡 Suggestion: Release the global Core-wallet storage lock before signing
HandleStorage::with_item retains the global storage map's read guard throughout the closure. This closure performs runtime().block_on(...) and invokes the host-owned mnemonic resolver callback, so a potentially slow signing operation prevents insertion or removal of every Core-wallet handle. A callback that re-enters an operation requiring the write lock can deadlock. CoreWallet is an inexpensive Arc-backed clone, and the broadcast entry points already clone it out of storage before blocking; use the same pattern here.
| let option = CORE_WALLET_STORAGE.with_item(handle, |wallet| { | |
| let address = read_utf8(address_ptr, address_len, |e| { | |
| PlatformWalletError::MessageSigningAddressInvalid { | |
| address: "<non-UTF-8>".to_string(), | |
| reason: format!("address is not valid UTF-8: {e}"), | |
| } | |
| })?; | |
| // A non-UTF-8 message is reported against the (now known) address, so the | |
| // error names the signing target the caller asked about. | |
| let message = read_utf8(message_ptr, message_len, |e| { | |
| PlatformWalletError::MessageSigningFailed { | |
| address: address.clone(), | |
| reason: format!("message is not valid UTF-8: {e}"), | |
| } | |
| })?; | |
| let network = wallet.network(); | |
| let wallet_id = wallet.wallet_id(); | |
| // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller | |
| // pinned alive for this call; the `MnemonicResolverCoreSigner` lives | |
| // only on this stack frame and is dropped before returning. | |
| let signer = MnemonicResolverCoreSigner::new( | |
| signer_addr as *mut MnemonicResolverHandle, | |
| wallet_id, | |
| network, | |
| ); | |
| runtime().block_on(wallet.sign_message(&address, &message, &signer)) | |
| }); | |
| let result = unwrap_option_or_return!(option); | |
| let signature = unwrap_result_or_return!(result); | |
| let wallet = unwrap_option_or_return!(CORE_WALLET_STORAGE.with_item(handle, Clone::clone)); | |
| let address = unwrap_result_or_return!(read_utf8(address_ptr, address_len, |e| { | |
| PlatformWalletError::MessageSigningAddressInvalid { | |
| address: "<non-UTF-8>".to_string(), | |
| reason: format!("address is not valid UTF-8: {e}"), | |
| } | |
| })); | |
| // A non-UTF-8 message is reported against the known address, so the | |
| // error identifies the signing target requested by the caller. | |
| let message = unwrap_result_or_return!(read_utf8(message_ptr, message_len, |e| { | |
| PlatformWalletError::MessageSigningFailed { | |
| address: address.clone(), | |
| reason: format!("message is not valid UTF-8: {e}"), | |
| } | |
| })); | |
| let network = wallet.network(); | |
| let wallet_id = wallet.wallet_id(); | |
| // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller | |
| // keeps alive for this call; the signer is dropped before returning. | |
| let signer = MnemonicResolverCoreSigner::new( | |
| signer_addr as *mut MnemonicResolverHandle, | |
| wallet_id, | |
| network, | |
| ); | |
| let signature = unwrap_result_or_return!( | |
| runtime().block_on(wallet.sign_message(&address, &message, &signer)) | |
| ); |
source: ['codex']
There was a problem hiding this comment.
Addressed in 2965337. The entry point now clones the Arc-backed CoreWallet out of storage via with_item(handle, Clone::clone) and runs block_on outside the guard, matching the pattern at broadcast.rs:43 and :82. Agreed the impact was real: the resolver callback can block on a biometric prompt, so the old shape stalled every handle operation for that duration. Note broadcast.rs:169 and the addresses.rs entry points share the old shape — left out of scope for this PR, tracked as a follow-up.
| // An address is never legitimately empty, so its pointer must be present. | ||
| // `message_ptr` is deliberately NOT checked: a null pointer with | ||
| // `message_len == 0` is the empty message, which is signable (see | ||
| // [`read_utf8`]). It is only dereferenced when the length is non-zero. |
There was a problem hiding this comment.
🟡 Suggestion: Cover the null-pointer empty-message ABI contract
The new FFI contract intentionally accepts message_ptr == NULL when message_len == 0, which is required by the Swift empty-array marshalling path. No endpoint test exercises this behavior, and the Rust wallet tests only sign "hello". Add a direct core_wallet_sign_message regression test that passes a null message pointer with zero length, asserts success, and verifies the returned signature against signed_msg_hash(""); this prevents a future unconditional check_ptr!(message_ptr) from silently breaking empty messages.
source: ['codex']
There was a problem hiding this comment.
Addressed in 2965337, split across layers rather than as a single FFI test: building the described test inside the FFI crate would need a full CoreWallet<SpvBroadcaster> in handle storage plus a live resolver (promoting the test wallet fixture to test-utils). Instead: (1) empty_message_is_signable in platform-wallet signs "" end-to-end and verifies it against signed_msg_hash(""); (2) empty_message_is_not_a_null_pointer_error in the FFI crate pins the null-pointer/zero-length acceptance by discrimination — reintroducing check_ptr!(message_ptr) makes it fail (verified by temporarily injecting it); (3) read_utf8_accepts_a_null_pointer_at_zero_length pins the primitive. Together these cover both the semantic and ABI halves of the contract.
| let message: String = match env.get_string(&message) { | ||
| Ok(v) => v.into(), | ||
| Err(_) => { | ||
| let _ = env.exception_clear(); | ||
| throw_sdk_exception(env, 1, "message string was invalid"); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| // Both cross as UTF-8 bytes + length (no trailing NUL), so an embedded | ||
| // NUL cannot truncate what actually gets signed. | ||
| let address_bytes = address.as_bytes(); | ||
| let message_bytes = message.as_bytes(); |
There was a problem hiding this comment.
🟡 Suggestion: Do not silently alter malformed Java UTF-16 messages
env.get_string(...).into() decodes JNI modified UTF-8 through jni::JavaStr. For a Java/Kotlin String containing an unpaired UTF-16 surrogate, the JNI crate's decoder fails and falls back to String::from_utf8_lossy, silently replacing the input before its bytes are signed. A verifier encoding the same Java string through the platform UTF-8 encoder can therefore hash different replacement bytes, contradicting the API's claim that the message is signed verbatim. Marshal a Kotlin-produced UTF-8 ByteArray, or read the UTF-16 code units strictly and reject unpaired surrogates rather than signing transformed text.
source: ['codex']
There was a problem hiding this comment.
Addressed in 4270d82, with one deviation from the suggested options: marshalling a Kotlin-produced UTF-8 ByteArray does not fix this — JDK's String.getBytes(UTF_8) also substitutes unpaired surrogates (with ? at 0x3f, verified on JDK 17, vs U+FFFD in the JNI path), so it would relocate and change the corruption rather than remove it. The fix validates surrogate pairing strictly at the Kotlin entry point (allocation-free scan behind require(...), consistent with the existing address check) — the last layer that still holds the exact UTF-16 form. Well-formed strings are unaffected and the JNI signature is unchanged. The lenient decode in the shared read_cstring_required affects every JNI string parameter in the crate and is flagged as a crate-wide follow-up rather than changed here.
… handle guard across the host callback Review findings 1, 3 and 4 on dashpay#4259. `Signer` assigns method dispatch to the caller and documents `sign_ecdsa` as valid only when the backend advertises `SignerMethod::Digest`. `sign_message` called it unconditionally, so a transaction-only hardware signer — one whose entire policy is to re-hash and display what it signs — could reject it, panic on an unreachable method, or blind-sign the host's digest and defeat that policy. Refused up front instead, mirroring the pre-check key-wallet performs in `TransactionSigner::sig_and_pubkey`. The refusal reuses `MessageSigningFailed` rather than taking a new code: this is the crate's "a path resolved but no signature came back" bucket, key-wallet folds the same refusal into `BuilderError::SigningFailed`, and it is unreachable with any shipping signer, so it does not justify an FFI code and the host mirror-enum churn one brings. The FFI entry point held `HandleStorage`'s process-global read guard across `runtime().block_on(...)` — and therefore across the host mnemonic-resolver callback, which in production is a Keychain/Keystore call that can block on a biometric prompt. That stalled every other wallet-handle operation for as long as the user took to respond, and would deadlock if the callback re-entered the FFI on a write-guard path. Clones the Arc-backed wallet out first, like `core_wallet_broadcast_*`. Tests: a transaction-only signer whose `sign_ecdsa` panics, proving the refusal happens before the backend is reached; `""` signed end-to-end and verified against `signed_msg_hash("")`, which nothing covered; and the FFI boundary contract that a null `message_ptr` at length 0 is accepted (verified to fail if an unconditional `check_ptr!(message_ptr)` returns). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…length-prefix docs Review findings 2 and 5 on dashpay#4259. The Kotlin and Swift docs described the digest's length prefix in each language's own string units — `message.length` (UTF-16 code units) and `message.count` (grapheme clusters). The format prefixes the UTF-8 BYTE count; the three agree only for ASCII. Corrected to `message.toByteArray(Charsets.UTF_8).size` and `message.utf8.count`. That doc bug has a real counterpart: a Kotlin `String` is an unvalidated UTF-16 sequence and may hold an unpaired surrogate, which has no UTF-8 encoding. Every conversion below is lenient and they do not even agree — the JNI bridge's string read substitutes U+FFFD, while `toByteArray(Charsets.UTF_8)` substitutes '?' (verified on JDK 17). So the wallet would sign bytes the caller never wrote and return a signature that verifies for a different message, silently, and "verbatim" in the docs would be false. Rejected at the Kotlin entry point, the last layer that still holds the exact UTF-16 and can explain why. Marshalling a UTF-8 ByteArray across the JNI instead — the other option raised in review — would NOT fix this: Kotlin's own encoder is equally lossy, so it would relocate the silent substitution and change it from U+FFFD to '?' while widening the FFI surface. Well-formed strings are unaffected. The lossy `get_string` read is the shared `read_cstring_required` behaviour on every JNI string parameter in the crate, so it is a codebase-wide convention rather than something this path introduced; flagged rather than changed here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ollisions The 29 collision is resolved and the renumber has now landed on dashpay#4185's branch: dashpay#4184 keeps 29 (ErrorAssetLockInsufficientFunds), dashpay#4185 takes 30 (ErrorReservationWalletMismatch). Table rows updated to match the code. Fixes the "30 is both free and assigned" inconsistency: the next-free line claimed 27-33 were claimed while the table showed 30 unallocated. 30 is now genuinely allocated to dashpay#4185, so the two agree. Adds allocations the survey had omitted, verified 2026-08-01 by reading error.rs at the head of all 62 open PRs: - dashpay#3968 numbers 26/27/28 (Persister* + a pre-merge TransactionBroadcastRejected) -> contradicts merged ABI at 26 and collides with dashpay#4185 at 27 and 28 - dashpay#3954 numbers ErrorShutdownIncomplete = 27 -> collides with dashpay#4185 at 27 - dashpay#4259 carries ErrorSigningKeyUnavailable = 31, inherited from dashpay#4183 rather than a new allocation The same sweep confirms no open PR anywhere defines a code 30.
…cord dashpay#4196 scope Clears the two review blockers on dashpay#4261 and re-syncs the registry with what the code on each branch actually does, re-read at every head rather than trusted from this file. Blocker (a) — dashpay#3968 / dashpay#3954 / dashpay#4259 were described in prose but had no rows, which is exactly what rule 2 forbids. They now have them: - A "Non-conforming allocations" table for dashpay#3968 (26/27/28) and dashpay#3954 (27). These are deliberately kept out of the proposed table: each row is a claim to be withdrawn and reissued, not an allocation of record. - An inherited-code table for the 31 that dashpay#4204 and dashpay#4259 carry but did not allocate (dashpay#4183 owns it), so it is not double-counted. - dashpay#4196 is recorded as claiming no integer at all: it routes a new token-less `StaleReservation` variant through the existing `ErrorStaleReservationToken`. The dashpay#3968 half is the serious one and is called out as such. Its 28 is not a new claim — it *moves the already-shipped* `ErrorTransactionBroadcastRejected` off 26 to make room for its own persister code. Rule 3 forbids that: a host compiled against merged ABI returns 26 for a broadcast rejection, and after dashpay#3968 the same condition returns 28 while 26 means a transient persister failure. Neither branch's diff shows the contradiction. Blocker (b) — 30 marked both free and assigned was already resolved by the preceding commit; verified consistent here (30 is allocated to dashpay#4185 throughout, frontier is 34, and the one remaining "genuinely free" is past tense explaining why dashpay#4185 could take it). Also corrected, all verified against the branches: - Survey provenance had dashpay#4185 at `0b0d5c76d6` labelled "(post-renumber)". Wrong twice: that commit is the *parent* of the renumber `d854debb`, and the head has since moved to `6c37e8679e`. dashpay#4184, dashpay#4247 and dashpay#4256 SHAs refreshed too. - dashpay#4256 has now taken 30 (`9481e5783b`) and dropped its stale "30 is reserved for the consent code" rationale; the equivalent comments on dashpay#4183 and dashpay#4204 are flagged as still present. - dashpay#4184 has a comment-only drift: it reserves "Codes 27-28" but names three codes. Correct when the trio was 27/28/29; it is now 27/28/30. Its discriminant is right and is the resolution of record — only the prose is stale, and dashpay#4184 is left untouched. - The dashpay#4196 section now records why the restack has not happened: its three own commits conflict in 3 files / 10 hunks against dashpay#4185's head, and the registry redesign underneath it (mandatory `registered_height`, new `WalletRemoved` variant, owner-stamped funding token) makes it author work rather than conflict resolution. Its trio numbers come from the dashpay#4185 copy it carries, so the restack fixes 28 -> 30 for free; the number dashpay#4196 itself must chase is 27, not 30. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 738 passed / 0 failed. Docs-only change.
…ssage) over FFI CoreWallet::sign_message signs a string with the key behind one of the wallet's own P2PKH addresses (signable funds accounts only) and returns the 65-byte BIP-137-style recoverable signature, base64 — same semantics as dashj ECKey.signMessage and Dash Core's signmessage RPC, verified byte-for-byte against dashj (RFC6979 golden + dashj test vector). The digest and serialization come from dashcore::sign_message; the recovery id is found by trial against the signer's pubkey, so every Signer backend gets signed-message support without a recoverable-signing method. Key material never crosses the FFI: core_wallet_sign_message takes the caller's MnemonicResolverHandle like the send paths do. Unknown/watch-only addresses map to ErrorSigningKeyUnavailable (31). Serves the Android/iOS wallets' CrowdNode integrations (API withdrawal and email registration are signed-message proofs of address ownership). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kotlin: ManagedPlatformWallet.signMessage(address, message, coreSignerHandle) suspend over the coreWalletSignMessage JNI binding. Swift: ManagedCoreWallet.signMessage(address:message:) constructing the per-call MnemonicResolver like every other seed-backed Swift call site. The Swift marshalling is what surfaced the FFI's empty-message contract bug (empty Swift strings marshal as a nil base address) — the read_utf8 len == 0 short-circuit shipped with the primitive commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… handle guard across the host callback Review findings 1, 3 and 4 on dashpay#4259. `Signer` assigns method dispatch to the caller and documents `sign_ecdsa` as valid only when the backend advertises `SignerMethod::Digest`. `sign_message` called it unconditionally, so a transaction-only hardware signer — one whose entire policy is to re-hash and display what it signs — could reject it, panic on an unreachable method, or blind-sign the host's digest and defeat that policy. Refused up front instead, mirroring the pre-check key-wallet performs in `TransactionSigner::sig_and_pubkey`. The refusal reuses `MessageSigningFailed` rather than taking a new code: this is the crate's "a path resolved but no signature came back" bucket, key-wallet folds the same refusal into `BuilderError::SigningFailed`, and it is unreachable with any shipping signer, so it does not justify an FFI code and the host mirror-enum churn one brings. The FFI entry point held `HandleStorage`'s process-global read guard across `runtime().block_on(...)` — and therefore across the host mnemonic-resolver callback, which in production is a Keychain/Keystore call that can block on a biometric prompt. That stalled every other wallet-handle operation for as long as the user took to respond, and would deadlock if the callback re-entered the FFI on a write-guard path. Clones the Arc-backed wallet out first, like `core_wallet_broadcast_*`. Tests: a transaction-only signer whose `sign_ecdsa` panics, proving the refusal happens before the backend is reached; `""` signed end-to-end and verified against `signed_msg_hash("")`, which nothing covered; and the FFI boundary contract that a null `message_ptr` at length 0 is accepted (verified to fail if an unconditional `check_ptr!(message_ptr)` returns). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…length-prefix docs Review findings 2 and 5 on dashpay#4259. The Kotlin and Swift docs described the digest's length prefix in each language's own string units — `message.length` (UTF-16 code units) and `message.count` (grapheme clusters). The format prefixes the UTF-8 BYTE count; the three agree only for ASCII. Corrected to `message.toByteArray(Charsets.UTF_8).size` and `message.utf8.count`. That doc bug has a real counterpart: a Kotlin `String` is an unvalidated UTF-16 sequence and may hold an unpaired surrogate, which has no UTF-8 encoding. Every conversion below is lenient and they do not even agree — the JNI bridge's string read substitutes U+FFFD, while `toByteArray(Charsets.UTF_8)` substitutes '?' (verified on JDK 17). So the wallet would sign bytes the caller never wrote and return a signature that verifies for a different message, silently, and "verbatim" in the docs would be false. Rejected at the Kotlin entry point, the last layer that still holds the exact UTF-16 and can explain why. Marshalling a UTF-8 ByteArray across the JNI instead — the other option raised in review — would NOT fix this: Kotlin's own encoder is equally lossy, so it would relocate the silent substitution and change it from U+FFFD to '?' while widening the FFI surface. Well-formed strings are unaffected. The lossy `get_string` read is the shared `read_cstring_required` behaviour on every JNI string parameter in the crate, so it is a codebase-wide convention rather than something this path introduced; flagged rather than changed here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4270d82 to
9336bdb
Compare
![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 (3)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt (1)
175-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCite the Swift source in the KDoc.
This method ports
ManagedCoreWallet.signMessagefrompackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift. The KDoc does not name it. Neighbouring methods in this file do cite their Swift counterparts, for examplesendToAddressescitesSendViewModel.swift:515-533andtokenscitesTokenActions.swift.As per coding guidelines: "When porting behavior from iOS, cite the corresponding Swift source file in KDoc."
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt` around lines 175 - 181, Update the KDoc for the signMessage method in ManagedPlatformWallet to cite its Swift counterpart, ManagedCoreWallet.signMessage in CoreWallet/ManagedCoreWallet.swift. Preserve the existing signing compatibility documentation.Source: Coding guidelines
packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs (1)
234-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFree the returned
PlatformWalletFFIResultin the test.The error path allocates the owned message
"invalid core wallet handle". The test drops the result without callingplatform_wallet_ffi_result_free, so the string leaks. The leak is harmless for a single test run, but it models the wrong ownership discipline for anyone copying this test, and it can trip a leak-checking CI job.♻️ Free the result after the assertions
assert!(out_signature.is_null()); + unsafe { crate::error::platform_wallet_ffi_result_free(&mut result) }; unsafe { dash_sdk_mnemonic_resolver_destroy(resolver) };
resultmust be bound aslet mut resultfor this.🤖 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 234 - 243, Update the test’s result binding to let mut result and call platform_wallet_ffi_result_free on it after the assertions, before destroying resolver, so the owned error message is released.packages/rs-platform-wallet/src/wallet/core/sign_message.rs (1)
468-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-P2PKH rejection test.
The tests cover malformed-network, unknown-address, and capability refusal. The non-P2PKH branch at lines 162-171 has no test. Add a case that passes a testnet P2SH address and asserts
MessageSigningAddressInvalidwith a reason naming the address type. This pins the rule that only P2PKH addresses can sign.🤖 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/src/wallet/core/sign_message.rs` around lines 468 - 491, Add a test alongside the existing message-signing validation tests that calls core.sign_message with a valid testnet P2SH address, then assert it returns MessageSigningAddressInvalid and that the reason identifies the rejected address type as P2SH. Use the existing wallet setup and MESSAGE_SIGNING_TEST_MNEMONIC patterns, preserving coverage of the rule that only P2PKH addresses may sign.
🤖 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-platform-wallet-ffi/src/core_wallet/sign_message.rs`:
- Around line 135-142: Update the read_utf8 error mapping in the message-signing
function to use MessageSigningAddressInvalid with the known address and a
message-specific UTF-8 reason, so malformed message input maps to
ErrorInvalidParameter instead of MessageSigningFailed. Also document this
message-encoding failure code in the function’s # Safety/parameter
documentation.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 175-181: Update the KDoc for the signMessage method in
ManagedPlatformWallet to cite its Swift counterpart,
ManagedCoreWallet.signMessage in CoreWallet/ManagedCoreWallet.swift. Preserve
the existing signing compatibility documentation.
In `@packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs`:
- Around line 234-243: Update the test’s result binding to let mut result and
call platform_wallet_ffi_result_free on it after the assertions, before
destroying resolver, so the owned error message is released.
In `@packages/rs-platform-wallet/src/wallet/core/sign_message.rs`:
- Around line 468-491: Add a test alongside the existing message-signing
validation tests that calls core.sign_message with a valid testnet P2SH address,
then assert it returns MessageSigningAddressInvalid and that the reason
identifies the rejected address type as P2SH. Use the existing wallet setup and
MESSAGE_SIGNING_TEST_MNEMONIC patterns, preserving coverage of the rule that
only P2PKH addresses may sign.
🪄 Autofix (Beta)
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: 79e1fef3-54dc-4b60-be06-33432478adbb
📒 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
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt
- packages/rs-platform-wallet/src/wallet/core/mod.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
- packages/rs-platform-wallet/src/test_support.rs
- packages/rs-platform-wallet/Cargo.toml
- packages/rs-unified-sdk-jni/src/wallet_manager.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/rs-platform-wallet/src/error.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
| // A non-UTF-8 message is reported against the (now known) address, so the | ||
| // error names the signing target the caller asked about. | ||
| let message = unwrap_result_or_return!(read_utf8(message_ptr, message_len, |e| { | ||
| PlatformWalletError::MessageSigningFailed { | ||
| address: address.clone(), | ||
| reason: format!("message is not valid UTF-8: {e}"), | ||
| } | ||
| })); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Map a non-UTF-8 message to an invalid-parameter error, not to a signing failure.
MessageSigningFailed maps to ErrorUnknown, as pinned by message_signing_failed_falls_through_to_unknown in packages/rs-platform-wallet-ffi/src/error.rs. A non-UTF-8 message_ptr is caller input, exactly like a non-UTF-8 address_ptr, which correctly yields ErrorInvalidParameter (2). A C caller therefore cannot tell "the message bytes were malformed" apart from an internal signing failure. The shipped JNI and Swift hosts always pass valid UTF-8, so this affects third-party callers only.
Use MessageSigningAddressInvalid with the known address and a message-specific reason, or add a dedicated variant that maps to ErrorInvalidParameter. Also state the message-encoding failure code in the # Safety / parameter documentation at lines 71-76.
🤖 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
135 - 142, Update the read_utf8 error mapping in the message-signing function to
use MessageSigningAddressInvalid with the known address and a message-specific
UTF-8 reason, so malformed message input maps to ErrorInvalidParameter instead
of MessageSigningFailed. Also document this message-encoding failure code in the
function’s # Safety/parameter documentation.
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with this PR's `ErrorStaleReservationToken = 27`. Renumber the deferred build/broadcast trio to the contiguous block 34-36, which sits above every code currently claimed by a merged commit or an open PR: 27 ErrorShutdownIncomplete MERGED, dashpay#4268 29 ErrorAssetLockInsufficientFunds dashpay#4184 31 ErrorSigningKeyUnavailable dashpay#4183, dashpay#4259 32 ErrorTransactionBuild dashpay#4247, dashpay#4256 33 ErrorTransactionSigning dashpay#4256 28 and 30 are vacated and return to the free pool. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (which has no compile-time cross-ABI check, so it was verified by grep). Also addresses three review suggestions: * `PlatformWalletInfo::generation` is now `pub(crate)`. It was publicly assignable through `state_mut()` / `state_mut_blocking()`, so downstream safe code could swap the `Arc` while `PlatformWallet` and `CoreWallet` kept the original — splitting the generation identity `Arc::ptr_eq` compares, which would make `is_current_generation()` reject a live wallet, turn generation-bound reservation cleanup into a no-op, and let teardown exclude through a different lifecycle gate than the payments it must fence. All construction and mutation sites are already inside the crate. * `buildSignedPayment` now runs under `opWithCleanupOnCancellation`. Native finalization mints the token before the blocking JNI call returns, so `withContext`'s prompt-cancellation handoff could discard the completed `SignedCoreTransaction` and leave the reservation to the GC Cleaner or the TTL. The discarded result is now closed deterministically. * Native code 26 (`ErrorTransactionBroadcastRejected`) no longer falls through to `PlatformWallet.Generic`. It maps to a dedicated `TransactionBroadcastRejected` subtype so callers can tell a definitively rejected, consumed-and-released payment (rebuild it) from an unrelated generic wallet failure, with its non-retry-in-place semantics pinned in `DashSdkErrorTest`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with the `ErrorStaleReservationToken = 27` this branch carries alongside dashpay#4185. Renumber the deferred build/broadcast trio to the contiguous block 34-36, matching dashpay#4185: ErrorStaleReservationToken 27 -> 34 ErrorReservationTokenConsumed 28 -> 35 ErrorReservationWalletMismatch 30 -> 36 34-36 sits above every code claimed by a merged commit or an open PR (27 dashpay#4268 merged, 29 dashpay#4184, 31 dashpay#4183/dashpay#4259, 32 dashpay#4247/dashpay#4256, 33 dashpay#4256), so it ends the renumbering churn. 28 and 30 are vacated and return to the free pool. This branch's own `ErrorTransactionBuild` (32) and `ErrorTransactionSigning` (33) are unaffected; their numbering-rationale rustdoc is updated to name dashpay#4268 as the owner of 27 and to record where the trio went. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (no compile-time cross-ABI check — verified by grep). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
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). This means everySignerbackend (soft seed, future hardware/HSM) gets signed-messagesupport without needing a recoverable-signing method. Cost: at most four
recovery attempts on one 32-byte digest, off the hot path.
core_wallet_sign_messagetakes thecaller's existing
MnemonicResolverHandle, exactly like the send paths.refused with a typed error; BIP44/BIP32/CoinJoin and DashPay receiving
accounts sign. The predicate is a local copy of the QA integration branch's
funding_privacy::is_signable_funding_account(identical body), so whenthat module lands the port converges by pure deletion.
(
ErrorSigningKeyUnavailable); malformed address →ErrorInvalidParameter;both mirrored in Kotlin (
DashSdkError.PlatformWallet.SigningKeyUnavailable)and Swift (
signingKeyUnavailable).Why error code 31, and why the 27–30 gap
Code 31 is what the keystore rework (#4183) assigns to
ErrorSigningKeyUnavailableon the QA integration line. This PR carries thevariant at the same number so the enums converge instead of colliding; the
27–30 gap is intentional and reserved. Until #4183's guarded catch-all lands,
MessageSigningFailed(signer-internal failure) falls through toErrorUnknown— noted in a comment at the mapping site.One contract fix included
Swift's empty-string marshalling (
Array("".utf8).withUnsafeBufferPointer)yields a nil base address, which the FFI's null-check rejected — despite an
empty message being legitimately signable (matches dashj and Core).
read_utf8now short-circuits at
len == 0without dereferencing;# Safetydocs statethe contract precisely.
address_ptrremains required non-null.Verification
ECKey.signMessageand via this Rust produce the identical base64(
HzW1qnS7pYhopcbz1ZbiQY+axMMDQ2cMXBjey37IQS15OOluF6l/rfEre7Bl8AzUOwYdANkLYRelpKJmIH4kfZM=).platform-walletlib suite 498/498;platform-wallet-ffi207/207(including the new mapping tests); goldens unchanged across the v4.2-dev port.
:sdk:compileDebugKotlinclean;DashSdkErrorTestpasses.build_ios.sh --target simsucceeds — cbindgen header carries..._ERROR_SIGNING_KEY_UNAVAILABLE = 31,ManagedCoreWallet.swiftcompiles,and SwiftExampleApp installs and runs on the iOS 26.4 simulator.
has one today); end-to-end CrowdNode use happens in the wallet integrations.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes