build: update rust-dashcore to v0.44.0#3973
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesDash workspace dependency bump
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit c624074) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Cargo.toml`:
- Around line 53-60: The git dependencies in Cargo.toml are pinned by tag, which
can drift if the tag is moved; update each of the listed dependency entries
(dashcore, dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi,
key-wallet-manager, dash-network, and dashcore-rpc) to use the fixed commit SHA
with rev = "3d0d5dcd4ad64e2199a726651bca7f8ffac123e6" instead of tag =
"v0.43.0".
In `@packages/rs-platform-wallet-ffi/src/mnemonic_words.rs`:
- Around line 157-189: The CJK wrapping logic in the mnemonic normalization path
should avoid using a global string replacement for each matched candidate,
because `s.replace` in this loop can rewrite earlier or unrelated occurrences
and prevent longer matches from being inserted correctly. Update the candidate
insertion logic in the normalization routine that iterates over
`normalized.split(' ')` and scans `wchars` so replacements are applied at the
matched position only, preserving longer matches and the intended ordering of
inserted `IDEO_SP` separators.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 149c6b9f-6a68-410b-a6e1-e18c94e73866
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlpackages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/src/mnemonic_words.rs
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR cleanly moves the workspace to rust-dashcore v0.43.0 and reintroduces three removed mnemonic recover-flow helpers (word_list, normalize_phrase, cleanup_phrase) inside platform-wallet-ffi. No consensus, FFI ABI, or memory-safety regressions. Real concerns center on (a) tag vs. rev pinning across all rust-dashcore git deps — also requested by a human reviewer — and (b) the new cleanup_phrase auto-split using global String::replace plus an asymmetric trim, in code paths the new tests do not actually validate against key_wallet's checker.
🟡 6 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 `Cargo.toml`:
- [SUGGESTION] Cargo.toml:53-60: Pin rust-dashcore workspace deps to a commit SHA, not a moving tag
All eight rust-dashcore workspace deps use `tag = "v0.43.0"`. Git tags are mutable — if upstream ever re-tags v0.43.0 (force-push, retag during a backport), every future `cargo update` silently pulls a different tree, with no Cargo.lock churn that names the new commit obviously. The prior pin in this workspace was a `rev = "…"`, and a human reviewer (Lukasz) and CodeRabbit independently requested keeping that convention by pinning to `rev = "3d0d5dcd4ad64e2199a726651bca7f8ffac123e6"` (what v0.43.0 currently resolves to). Switch all eight entries (`dashcore`, `dash-network-seeds`, `dash-spv`, `key-wallet`, `key-wallet-ffi`, `key-wallet-manager`, `dash-network`, `dashcore-rpc`) to that rev. The tag can stay as documentation in a comment.
- [SUGGESTION] Cargo.toml:53-60: v0.43.0 tag is older than the previously pinned post-release rev — audit dropped commits
The previous pin was rev `5c0113e7901551450f6063023eec4be95beeb6b9`, described as a post-release commit on top of v0.43.0; this PR moves back to the tag (commit `3d0d5dcd…`). Any rust-dashcore changes that landed between v0.43.0 and `5c0113e7` — including potential fixes in `dashcore`, `dash-spv`, `dashcore-rpc`, or `key-wallet` — are silently reverted. Before merging, enumerate the dropped commits (`git log 3d0d5dcd..5c0113e7` in the rust-dashcore tree) and confirm none are security-relevant, and consider noting the audit in the PR description so reviewers do not have to re-derive it.
In `packages/rs-platform-wallet-ffi/src/mnemonic_words.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:160-189: cleanup_phrase CJK auto-split uses global String::replace, corrupting unrelated occurrences
Line 177's `s = s.replace(&candidate, &wrapped)` is an unbounded global replacement called from inside a per-word, per-window scanner. Any other position in `s` that happens to contain `candidate` — a different word, a different window within the same word, or a substring of a longer valid match later in the scan — is wrapped in ideographic spaces as well, fragmenting previously-valid words. The `i += j - 1` / `i += 1` cursor advancement assumes only the current window mutated, which the global replace violates; the trailing `while s.contains(&dbl_ideo) { … IDEO_SP }` collapse only fixes adjacent ideographic spaces, it cannot rejoin a word that was split mid-way. Replace with a position-aware splitter: walk the original word left-to-right with a longest-match table-lookup against `word_in_any_list`, and write the result into a fresh buffer. The current Japanese test (line 347) only asserts that *some* ideographic space appears, so this bug is not exercised — strengthening that test (see separate finding) will surface it.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:84-113: word_list (bip39 crate) and phrase validator (key_wallet) are independent sources of truth
`word_list` / `word_in_any_list` source words from the new `bip39 = 2.2` dependency, while `phrase_is_valid_any` still routes through `key_wallet::Mnemonic::validate`, which uses key-wallet's embedded wordlists. BIP-39 lists are standardized today so both tables agree, but there is no compile-time or test-time guarantee they stay byte-identical across upgrades of either crate. If they ever drift (typo fix, locale variant, NFC/NFKD form), the recover UI will accept a word that the real validator rejects (or vice versa), and `cleanup_phrase`'s auto-split will build phrases that pass membership but fail seed derivation — a confusing UX failure that only appears on real recover input. Add a test that asserts `word_list(lang)` equals the wordlist key-wallet uses for each `Language` (or that every word in `word_list(lang)` makes `key_wallet::Mnemonic::validate` succeed in a phrase composed only of that wordlist). Long-term, request a `word_list()` accessor on `key_wallet::mnemonic::Language` upstream and drop the `bip39` direct dep.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:150-155: cleanup_phrase fast path leaks trailing whitespace
Line 150 does `s = s.trim_start().to_string()`. If the phrase already validates after normalization (lines 152–155), `s` is returned without ever trimming the end, so input like `"abandon … about \n"` is returned as `"abandon … about "`. The autosplit branch explicitly uses `s.trim()` at line 181, and the doc on `normalize_phrase` (line 117) promises "ends trimmed" — the asymmetry is almost certainly unintentional. Change line 150 to `s.trim().to_string()`.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:327-350: Japanese no-space autosplit test does not check that the result is a valid phrase
`mnemonic_words_japanese_spaced_phrase_validates_and_no_space_phrase_autosplits` only asserts `cleanup_phrase(&nospace).contains(IDEO_SP)` — a splitter that wraps arbitrary CJK substrings still passes. The contract of `cleanup_phrase` for a no-space CJK paste is that it reconstructs a *valid* phrase, so the test should assert `phrase_is_valid_any(&normalize_phrase(&cleanup_phrase(&nospace)))`, or assert it round-trips to the original normalized phrase. This will also exercise the global-replace concern above against a realistic 12-word input.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (642b9dd) is a clean cargo-only change: converts all eight rust-dashcore workspace deps from tag = "v0.43.0" to rev = "3d0d5dcd..." plus matching Cargo.lock churn. Prior-1 is fixed. No new findings from the latest delta. Four prior findings against packages/rs-platform-wallet-ffi/src/mnemonic_words.rs (file unchanged) and the dropped-commits audit on Cargo.toml are carried forward. Prior-3 (wordlist source divergence) is dropped as outdated — key_wallet v0.43.0 itself sources wordlists from the bip39 crate, so there is no real divergence.
🟡 4 suggestion(s) | 💬 2 nitpick(s)
Findings not posted inline (2)
These findings could not be anchored to the current diff, but they are still part of this review.
- [NITPICK]
(no file): Untitled finding - [NITPICK]
(no file): Untitled finding
🤖 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-ffi/src/mnemonic_words.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:160-189: cleanup_phrase CJK auto-split uses global String::replace, corrupting unrelated occurrences
Carried forward (prior-2; STILL VALID — file unchanged in this delta). The auto-split loop scans one CJK word/window at a time, but line 177's `s = s.replace(&candidate, &wrapped)` is an unbounded global replacement over the entire phrase. Any other position in `s` that contains `candidate` — a substring of a different word, an earlier window in the same word, or a substring of a later valid match — also gets wrapped in ideographic spaces, fragmenting previously-valid words. The cursor advancement (`i += j - 1` / `i += 1`) assumes only the current window mutated, which the global replace violates; the trailing `while s.contains(&dbl_ideo) { … IDEO_SP }` collapse only fixes adjacent ideographic spaces and cannot rejoin a word split mid-way. Reachable across the FFI: Swift `Mnemonic.cleanupPhrase` returns the corrupted string into the recover flow. Concrete failure mode: a valid Japanese phrase containing both `かいせん` and `いせん` would have `かいせん` split as `か` / `いせん`, producing 13 tokens after Swift sees the cleaned string. Replace with a position-aware splitter that walks the original word left-to-right with a longest-match table lookup against `word_in_any_list` and writes into a fresh buffer.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:150-155: cleanup_phrase fast path leaks trailing whitespace across the FFI boundary
Carried forward (prior-4; STILL VALID). Line 150 uses `s.trim_start().to_string()`. If the normalized phrase validates at lines 152–155, `s` is returned without trailing whitespace ever being stripped, so input like `"abandon … about \n"` is returned as `"abandon … about "` and crosses the C boundary unchanged. The autosplit branch uses `s.trim()` at line 181, and `normalize_phrase`'s doc (line 117) promises "ends trimmed". The asymmetry is almost certainly unintentional and means Swift callers see inconsistent cleanup results depending on which branch ran. Trim both ends.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:327-350: Japanese no-space autosplit test does not verify the cleaned phrase is valid
Carried forward (prior-5; STILL VALID). `mnemonic_words_japanese_spaced_phrase_validates_and_no_space_phrase_autosplits` only asserts `cleanup_phrase(&nospace).contains(IDEO_SP)`. A splitter that wraps arbitrary CJK substrings — the precise failure mode in the global-replace finding above — still passes this test. The contract `cleanupPhrase` advertises to Swift for a no-space CJK paste is reconstruction of a *valid* phrase, so the test should also assert validity. Strengthening this assertion exercises the global-replace bug on a realistic 12-word input.
In `Cargo.toml`:
- [SUGGESTION] Cargo.toml:53-60: Audit rust-dashcore commits dropped by moving back to the v0.43.0 release commit
Carried forward (prior-6; STILL VALID). The latest delta correctly pins by `rev` (fixing prior-1) but the new pin `3d0d5dcd4ad64e2199a726651bca7f8ffac123e6` is the v0.43.0 *release* commit, while the previous pin `5c0113e7901551450f6063023eec4be95beeb6b9` was a post-v0.43.0 commit on `v4.0-dev`. Commits in the range `3d0d5dcd..5c0113e7` are silently reverted across `dashcore`, `dash-spv`, `dashcore-rpc`, `key-wallet`, `key-wallet-ffi`, and `key-wallet-manager`. A reviewer flagged at least one concrete concern: post-release commit `7ff6b246 fix(key-wallet): correct CoinJoin discovery` is reportedly in the dropped range — if accurate, it changes CoinJoin derivation paths and discovery logic, which affects what a restored wallet sees. Before merging, please enumerate the dropped commits (`git log 3d0d5dcd..5c0113e7` in rust-dashcore), confirm none touch the FFI/header surface or fix consensus-/wallet-relevant bugs that Platform relies on, and summarize the audit in the PR description so reviewers don't have to re-derive it.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta fixes three of the four prior findings (CJK global replace, fast-path trim, Japanese no-space test assertion). One real-but-narrow new edge case remains: the 8-char NFKD candidate window in cleanup_phrase cannot match the single 9-NFKD-char Japanese wordlist entry だいじょうぶ, so any no-space recover input containing that word will fail to round-trip. The Cargo.toml downgrade-audit suggestion is carried forward only as a low-confidence nit since the PR description now enumerates the dropped commits.
🟡 1 suggestion(s) | 💬 1 nitpick(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-ffi/src/mnemonic_words.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:180-197: 8-char NFKD window misses the only Japanese wordlist entry longer than 8 NFKD chars (だいじょうぶ)
`let mut j = core::cmp::min(8, wlen - i);` caps the longest candidate the splitter will try at 8 NFKD scalars. The bip39 Japanese wordlist (which key-wallet uses verbatim and which this PR matches against via `word_in_any_list`) is stored in NFKD form; every entry except one fits in 8 NFKD scalars, but だいじょうぶ decomposes to た+◌゙ い し+◌゙ ょ う ふ+◌゙ — 9 scalars — because each voiced kana expands to base + U+3099. Trace: at the position of だ the loop's largest candidate is the 8-scalar prefix `だいじょうふ` (the trailing U+3099 dropped), which is not in any wordlist. j then decrements; no shorter prefix is in any wordlist either, so the matcher either skips this position or later wraps shorter substrings such as `いじょう`, leaving a cleaned phrase that no longer round-trips through `phrase_is_valid_any(normalize_phrase(...))`. The current `mnemonic_words_japanese_spaced_phrase_validates_and_no_space_phrase_autosplits` fixture happens to use indices [145, 1301, 1265, 427, 1647, 960, 483, 1581, 482, 726, 1234, 1924], none of which contain voiced marks, so the regression is invisible today. Replace the hardcoded `8` with the maximum NFKD scalar length across the supported wordlists (or iterate from `wlen - i` downward), and add a fixture whose 12 words include だいじょうぶ so the regression is locked in.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the latest delta (3aa07c5..1568e5d) against source. Prior-1 is FIXED: packages/rs-platform-wallet-ffi/src/mnemonic_words.rs:182 now uses let mut j = wlen - i; with an explanatory comment at lines 180-181, and the new test at lines 371-389 covers the だいじょうぶ NFKD-long-word case. Prior-2 is resolved at the documentation layer the agents converged on: the PR body's Release Commit Audit / Impact note now spells out that the dropped post-release rust-dashcore commits do not change the preserved Platform wallet FFI mnemonic surface; the missing inline Cargo.toml comment is below the noise threshold for a request. No new findings in the latest delta.
Note: GitHub does not allow me to approve my own PR, so this clean self-review is posted as a COMMENT while preserving the verifier result.
1568e5d to
e40e084
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Cargo.toml (1)
53-60: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the
rust-dashcoreworkspace deps to the release commit, not the tag.Lines 53-60 reintroduce mutable git refs.
Cargo.lockonly freezes the current resolution; the next lockfile refresh or any consumer building without that lockfile can resolve different code ifv0.44.0is ever retargeted. Since the PR already identifies the peeled release commit, keep the release intent but switch these entries back torev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409".Suggested patch
-dashcore = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } -dash-network-seeds = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } -dash-spv = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } -key-wallet = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } -key-wallet-ffi = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } -key-wallet-manager = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } -dash-network = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } -dashcore-rpc = { git = "https://github.com/dashpay/rust-dashcore", tag = "v0.44.0" } +dashcore = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" } +dash-network-seeds = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" } +dash-spv = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" } +key-wallet = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" } +key-wallet-ffi = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" } +key-wallet-manager = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" } +dash-network = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" } +dashcore-rpc = { git = "https://github.com/dashpay/rust-dashcore", rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" }#!/bin/bash set -euo pipefail echo "Resolve the published tag to its peeled commit:" git ls-remote https://github.com/dashpay/rust-dashcore 'refs/tags/v0.44.0^{}' echo echo "Current workspace dependency declarations:" sed -n '53,60p' Cargo.toml echo echo "Current lockfile source entries for the affected crates:" rg -n -A1 -B1 '^(name = "(dashcore|dash-network-seeds|dash-spv|key-wallet|key-wallet-ffi|key-wallet-manager|dash-network|dashcore-rpc)"|source = "git\+https://github.com/dashpay/rust-dashcore)' Cargo.lock || true🤖 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 `@Cargo.toml` around lines 53 - 60, The workspace dependency entries for rust-dashcore are still pinned to the mutable v0.44.0 tag, which should be replaced with the fixed release commit. Update the dependency declarations for dashcore, dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager, dash-network, and dashcore-rpc in Cargo.toml to use the peeled release rev instead of tag, preserving the same git source. Use the existing release commit identifier referenced in the PR so future lockfile refreshes and builds without Cargo.lock stay on the intended code.
🤖 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.
Duplicate comments:
In `@Cargo.toml`:
- Around line 53-60: The workspace dependency entries for rust-dashcore are
still pinned to the mutable v0.44.0 tag, which should be replaced with the fixed
release commit. Update the dependency declarations for dashcore,
dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager,
dash-network, and dashcore-rpc in Cargo.toml to use the peeled release rev
instead of tag, preserving the same git source. Use the existing release commit
identifier referenced in the PR so future lockfile refreshes and builds without
Cargo.lock stay on the intended code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e1a07af-0ce0-4b5b-9e98-37fa27ec1598
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings (prior-1 NFKD windowing, prior-2 Cargo.toml audit comment) are both OUTDATED at HEAD e40e084: the latest delta dropped the local packages/rs-platform-wallet-ffi/src/mnemonic_words.rs workaround entirely, leaving the cumulative PR as a clean retarget of all eight rust-dashcore workspace deps from pinned rev 5c0113e7 to tag v0.44.0 (Cargo.lock pins resolved commit 991c6ebe). No new findings emerged from the latest delta — five independent agents (claude/codex general, security, two FFI) all returned empty in-scope finding sets, and CodeRabbit has no actionable inline comments.
Note: GitHub does not allow me to submit this review event on my own PR, so this is posted as a COMMENT while preserving the verified findings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (e40e084..c624074) only swaps tag = "v0.44.0" for rev = "991c6ebe24d7ea8ba7d900a052b25be8c5498409" across the eight rust-dashcore workspace deps in Cargo.toml and refreshes Cargo.lock to record the same resolved source — a strictly stronger supply-chain pin since git tags are mutable but commit hashes are not. No source code changes, no FFI/ABI surface changes, and the prior review carried zero active findings. All historical prior items (mnemonic NFKD windowing, FFI-shim rationale comment) remain OUTDATED because the local Platform-side wallet FFI workaround they targeted is no longer part of this PR.
Note: GitHub does not allow me to submit this review event on my own PR, so this is posted as a COMMENT while preserving the verified findings.
Issue being fixed or feature implemented
Updates Platform's rust-dashcore workspace dependencies from pinned rev
5c0113e7901551450f6063023eec4be95beeb6b9to the publishedv0.44.0release tag.What was done?
Cargo.tomltotag = "v0.44.0":dashcoredash-network-seedsdash-spvkey-walletkey-wallet-ffikey-wallet-managerdash-networkdashcore-rpcCargo.lockso rust-dashcore crates resolve tov0.44.0, peeled to commit991c6ebe24d7ea8ba7d900a052b25be8c5498409.How Has This Been Tested?
Local validation on macOS arm64:
Pre-push code review gate:
Result:
Recommendation: ship.Breaking Changes
None intended in Platform code. This PR only moves the rust-dashcore dependency source to the
v0.44.0release tag.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit