feat(platform-wallet): classic Dash message signing (signMessage) over FFI, JNI, Kotlin, and Swift - #4259
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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 signing errors to native result codes. ChangesClassic Dash message signing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedCoreWallet
participant WalletManagerNative
participant core_wallet_sign_message
participant CoreWallet
participant Signer
ManagedCoreWallet->>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-->>ManagedCoreWallet: 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 |
|
✅ Final review complete — no blockers (commit f597866) |
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.
There was a problem hiding this comment.
Resolved in this update — Honor the signer's digest capability before signing no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| * **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.
There was a problem hiding this comment.
Resolved in this update — Document the signed message's UTF-8 byte length no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| 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.
There was a problem hiding this comment.
Resolved in this update — Release the global Core-wallet storage lock before signing no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // 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.
There was a problem hiding this comment.
Resolved in this update — Cover the null-pointer empty-message ABI contract no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| 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.
There was a problem hiding this comment.
Resolved in this update — Do not silently alter malformed Java UTF-16 messages no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
… 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.
… 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
…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>
…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>
…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>
…ot an internal failure Review finding on dashpay#4259. `core_wallet_sign_message` takes the message as raw bytes, so it can be handed something that is not valid UTF-8. That mapped through `MessageSigningFailed` to `ErrorUnknown` — telling the host "internal failure" about an argument it could simply fix, while the *address* argument in the same function already reported malformed bytes as `ErrorInvalidParameter`. Two equivalent caller-input errors, two different answers. Adds a dedicated `MessageSigningMessageInvalid` and maps it to ErrorInvalidParameter, matching the address arm. A dedicated variant rather than reusing `MessageSigningAddressInvalid`: that variant renders "message-signing address {address} is unusable", so reusing it would blame a perfectly valid address for a message problem. These variants exist precisely to name which argument the caller must fix — the crate already splits AddressInvalid from KeyUnavailable on the same principle — and it maps to the already-mirrored ErrorInvalidParameter, so no new numeric code and no Kotlin/Swift mirror churn. Reachable only across the FFI; a Rust or Kotlin caller cannot build an ill-formed string. `read_utf8`'s contract, the `message_ptr` parameter doc and the `# Safety` section now state the code, and Safety notes that encoding is checked rather than assumed. The `MessageSigningFailed` fall-through test still pins ErrorUnknown for what genuinely remains internal (no recovering recovery id), with a note that malformed bytes have moved off it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9336bdb to
5b77dfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/error.rs (1)
35-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this PR scoped to message signing.
TxMetadataPayloadTooLargehas no stated dependency on the message-signing flow. It expands the public error contract outside the stated objective. Move it to a linked PR, or document the dependency.As per coding guidelines, “Keep pull requests focused.”
🤖 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/error.rs` around lines 35 - 48, Remove the TxMetadataPayloadTooLarge variant and its associated documentation from the error contract in error.rs, unless the message-signing implementation demonstrably requires it; keep this PR scoped to message signing and defer unrelated txMetadata validation changes to a separate PR.Source: Coding guidelines
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt (1)
176-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse EditorConfig indentation in the non-Rust additions.
The changed Kotlin and Swift blocks use four-space nesting. The repository requires two-space indentation outside Rust.
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt#L176-L290: Reformat the signing API and UTF-16 validation block with two-space indentation.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt#L666-L673: Reformat the identity-not-found translation block with two-space indentation.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L79-L92: Reformat the result-code addition with two-space indentation.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L154-L155: Reformat the FFI-code switch arm with two-space indentation.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L282-L291: Reformat the Swift error-case addition with two-space indentation.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L315-L315: Reformat the grouped error-description switch case.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L359-L360: Reformat the native-result conversion arm.As per coding guidelines, “Follow repository EditorConfig settings: 2-space indentation, 4 spaces for Rust files.”
🤖 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 176 - 290, Reformat the additions to use the repository’s two-space indentation (four spaces only for Rust): in ManagedPlatformWallet.kt lines 176-290 and 666-673, and in PlatformWalletResult.swift lines 79-92, 154-155, 282-291, 315, and 359-360. Apply the indentation consistently to the signing API, validation helper, identity translation, result-code additions, switch arms, error case, and native-result conversion.Source: Coding guidelines
🤖 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/src/error.rs`:
- Around line 155-184: Update message-signing error handling around sign_ecdsa
in packages/rs-platform-wallet/src/error.rs: detect and return the existing
structured signer-key-unavailable error before wrapping failures as
MessageSigningFailed, while preserving the existing wrapper for capability
refusals and other invariant failures. In
packages/rs-platform-wallet-ffi/src/error.rs ranges 466-491 and 1146-1170,
retain the existing typed key-unavailable conversion paths; no direct change is
required there, but they must receive the structured error after the root fix.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 176-290: Reformat the additions to use the repository’s two-space
indentation (four spaces only for Rust): in ManagedPlatformWallet.kt lines
176-290 and 666-673, and in PlatformWalletResult.swift lines 79-92, 154-155,
282-291, 315, and 359-360. Apply the indentation consistently to the signing
API, validation helper, identity translation, result-code additions, switch
arms, error case, and native-result conversion.
In `@packages/rs-platform-wallet/src/error.rs`:
- Around line 35-48: Remove the TxMetadataPayloadTooLarge variant and its
associated documentation from the error contract in error.rs, unless the
message-signing implementation demonstrably requires it; keep this PR scoped to
message signing and defer unrelated txMetadata validation changes to a separate
PR.
🪄 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: 2083313d-2e4f-4fdc-8974-aaabc1bd0dd2
📒 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 (10)
- packages/rs-unified-sdk-jni/src/wallet_manager.rs
- packages/rs-platform-wallet/src/wallet/core/mod.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
- packages/rs-platform-wallet/Cargo.toml
- packages/rs-platform-wallet/src/wallet/core/sign_message.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
- packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs
…code 31 from message signing Review finding on dashpay#4259 (CodeRabbit, Major). Investigated and found NOT fixable at the producer as described; recording the evidence in-code and pinning it with a test so it does not have to be re-derived. The finding asks `sign_message` to route signer key-unavailable failures through `preserve_signer_key_unavailable_or` before building `MessageSigningFailed`. That helper does exist (dashpay#4183 shipped it in this crate) and is the right tool — for the STATE-TRANSITION signing paths, whose failures are `dash_sdk::Error`, which is where document replace, DPNS and token transfer already use it. Message signing is a different surface. It calls key-wallet's `Signer::sign_ecdsa`, whose error is the associated type `S::Error`, bounded only by `Display + Send + Sync + 'static`. There is no enum to match, and passing it to the helper does not compile: error[E0308]: mismatched types expected enum `dash_sdk::Error` Nor does a producer exist. The only production impl, `rs_sdk_ffi::MnemonicResolverCoreSigner`, has `Error = MnemonicResolverSignerError` — a typed enum that never stamps the reserved marker; its `NotFound` ("mnemonic not found in keychain") IS the key-unavailable case but is indistinguishable once `Display`ed. The marker is produced only in rs-sdk-ffi's state-transition completion callback (`SignResult = Result<Vec<u8>, ProtocolError>`), never on a `sign_ecdsa` path. So a position-0 check would match nothing today, and a `contains` check is the substring sniff dashpay#4183's review rejected. Corrects an earlier note of mine that blamed the marker constant's visibility: it IS visible here now, mirrored as `SIGNER_KEY_UNAVAILABLE_PREFIX` and pinned byte-identical by a compile-time assertion. The blocker is the error type. Closing it needs an upstream change — the signer rendering its key-unavailable variants with the marker at position 0, or key-wallet tightening `Signer::Error` to something matchable. Both touch shared, externally-consumed surfaces and want their own review. `signer_key_unavailable_is_not_preserved_during_message_signing` pins the current behaviour and shows the mechanism: the marker survives but mid-string, which is precisely why a position-0 match cannot recover it. It is written to FAIL if an upstream fix ever makes code 31 reachable, with instructions to flip it. The existing fall-through test continues to pin genuinely-internal failures, and capability refusals still take the `MessageSigningFailed` path unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
At head f597866, all five prior findings from the previous review round are confirmed FIXED against the actual source: the digest-capability guard runs before sign_ecdsa, the FFI clones the wallet out of HandleStorage before block_on/the host callback, a direct null-pointer/zero-length regression test exists at the FFI boundary, the Kotlin/Swift docs correctly describe UTF-8 byte length, and Kotlin's public signMessage rejects unpaired UTF-16 surrogates before any native call. All 8 platform-wallet sign_message tests and 2 platform-wallet-ffi sign_message tests pass locally, and clippy shows no new warnings in files touched by this PR. One new latest-delta finding from this round: the MessageSigningFailed doc comment in platform-wallet/src/error.rs still claims the FFI catch-all promotes signer key-unavailable completions to code 31, directly contradicted by the FFI conversion's own explicit comment (which says MessageSigningFailed is deliberately left unmapped and always falls to ErrorUnknown) and by sign_message.rs's own inline note added in this same PR. This is a self-contradictory public doc, not a functional defect, so it does not block the PR.
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 - Sonnet 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 (completed)
🟡 1 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/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:173-178: MessageSigningFailed doc claims a code-31 promotion the FFI layer explicitly does not perform
This doc comment says a signer's typed key-unavailable completion "carries the stable machine prefix in its Display, which the FFI conversion's catch-all promotes to ErrorSigningKeyUnavailable." That is the opposite of the current, carefully-documented behavior: the FFI conversion at packages/rs-platform-wallet-ffi/src/error.rs:466-528 explicitly states 'MessageSigningFailed is deliberately NOT matched, so it falls to the ErrorUnknown catch-all below' and spends ~40 lines explaining exactly why a signer's key-unavailable failure cannot currently reach code 31 during message signing. The sign_message.rs implementation in this same crate (lines 216-220) says the same thing: 'Every signer failure becomes MessageSigningFailed (→ ErrorUnknown), including a key-unavailable one. That is a known limitation, not an oversight.' The new regression test signer_key_unavailable_is_not_preserved_during_message_signing pins this exact behavior. Since hosts branch on the numeric FFI code to decide between a key-repair UX flow and a generic-error flow, this stale claim on the public Rust type is misleading to anyone reading the type docs without also reading the FFI conversion source.
| /// Deliberately NOT given a dedicated FFI code: a signer that reports its | ||
| /// typed key-unavailable completion carries the stable machine prefix in | ||
| /// its `Display`, which the FFI conversion's catch-all promotes to | ||
| /// `ErrorSigningKeyUnavailable`. The remaining causes are genuine internal | ||
| /// invariant breaks and should surface as unknown rather than as a | ||
| /// key-repair prompt. |
There was a problem hiding this comment.
🟡 Suggestion: MessageSigningFailed doc claims a code-31 promotion the FFI layer explicitly does not perform
This doc comment says a signer's typed key-unavailable completion "carries the stable machine prefix in its Display, which the FFI conversion's catch-all promotes to ErrorSigningKeyUnavailable." That is the opposite of the current, carefully-documented behavior: the FFI conversion at packages/rs-platform-wallet-ffi/src/error.rs:466-528 explicitly states 'MessageSigningFailed is deliberately NOT matched, so it falls to the ErrorUnknown catch-all below' and spends ~40 lines explaining exactly why a signer's key-unavailable failure cannot currently reach code 31 during message signing. The sign_message.rs implementation in this same crate (lines 216-220) says the same thing: 'Every signer failure becomes MessageSigningFailed (→ ErrorUnknown), including a key-unavailable one. That is a known limitation, not an oversight.' The new regression test signer_key_unavailable_is_not_preserved_during_message_signing pins this exact behavior. Since hosts branch on the numeric FFI code to decide between a key-repair UX flow and a generic-error flow, this stale claim on the public Rust type is misleading to anyone reading the type docs without also reading the FFI conversion source.
| /// Deliberately NOT given a dedicated FFI code: a signer that reports its | |
| /// typed key-unavailable completion carries the stable machine prefix in | |
| /// its `Display`, which the FFI conversion's catch-all promotes to | |
| /// `ErrorSigningKeyUnavailable`. The remaining causes are genuine internal | |
| /// invariant breaks and should surface as unknown rather than as a | |
| /// key-repair prompt. | |
| /// Deliberately NOT given a dedicated FFI code: `Signer::Error` is generic | |
| /// and bounded only by `Display`, so it cannot be classified structurally | |
| /// here. Signer failures — including a signer-reported key-unavailable | |
| /// completion — remain `MessageSigningFailed` and fall through to | |
| /// `ErrorUnknown`. Only `MessageSigningKeyUnavailable` (address resolution | |
| /// failing before a signer is ever invoked) reaches FFI code 31. See the | |
| /// `MessageSigningFailed` arm's NOTE in `platform-wallet-ffi`'s error | |
| /// conversion for the full chain and the upstream change that would be | |
| /// needed to close this gap. |
source: ['codex']
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