feat(platform-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the asset-lock path - #4350
Merged
QuantumExplorer merged 7 commits intoAug 10, 2026
Conversation
…n the asset-lock path dashpay#4329 pooled the send path and left asset locks single-account. An invitation, identity registration or top-up could only be funded from ONE BIP44 account, so a wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them together first and lock out of the sweep — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege. `ASSET_LOCK_FUNDING_SOURCES` (the same set as `SEND_FUNDING_SOURCES`) ends that shape: coin selection draws from the union, change returns to BIP44, and sources the wallet has nothing for are skipped. `build_asset_lock_transaction_with_funding` and `broadcast_funded_asset_lock_with_funding` now take the source list plus a `source_index` instead of a single `AssetLockFundingAccount`, and the historical entry points — `build_asset_lock_transaction`, `broadcast_funded_asset_lock`, `create_funded_asset_lock_proof` — keep their signatures and simply pass the pooled set, so every call site above them (invitation `create_invitation`, identity registration, top-up, platform-address and shielded funding, and the `asset_lock_manager_build_transaction` FFI export) becomes pooled without new plumbing. Nothing gained an elective funding selector it does not need. Reservation reconciliation is the part that had to change shape. `release_reservation_after_rejected_broadcast` now takes the contributing account LIST rather than one `ReservedFundingAccount`: a pooled build reserves in each contributing account's own set under one owner token, so a release reaching only the first account would strand the rest of the inputs until the 24-block TTL backstop and make an immediate retry fail with spurious insufficient funds. The build returns those accounts (upstream's `AssetLockResult::funding_accounts`) and both rejection paths — the undersized-drain abandon and the rejected broadcast — release across all of them, still owner-guarded (dashpay#4185). `ReservedFundingAccount` is gone; `AccountType` already names every family, including the DashPay accounts the old enum could not. `funding_tx_record` had to widen with it. It resolved the funding transaction in BIP44 and CoinJoin at the tracked index only, but key-wallet files a transaction under every account its inputs touch, and a pooled lock may take nothing from BIP44 — funded entirely out of BIP32 or a contact account. The lookup missed it, which burns the proof wait and is outright fatal under `NoPlatformPersistence`, whose persister fallback always returns `None`. It now covers the standard pair and CoinJoin at the index, then the DashPay receiving accounts by txid (they span their own indices) — the same class of gap dashpay#4336's CoinJoin fix closed. CoinJoin funding is untouched: still drain-only, still a single account, now routed through `create_funded_asset_lock_proof_with_funding`, which converts it to a one-element source list. Upstream additionally rejects a CoinJoin source pooled with any other, since spending mixed outputs alongside transparent ones links them and undoes the mixing. Depends on dashpay/rust-dashcore#935; Cargo.toml is pinned to that branch's rev so CI is honest, and MUST be re-pointed to dashpay/rust-dashcore `dev` on merge.
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 9, 2026 19:48
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe wallet now pools asset-lock funding across BIP44, BIP32, and DashPay receiving accounts. Build, broadcast, tracking, transaction lookup, persistence, and reservation cleanup support multiple contributing accounts. Workspace Dash dependencies use a new pinned revision. Asset-lock funding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AssetLockManager
participant KeyWalletBuilder
participant ReservationCleanup
participant FundingLookup
AssetLockManager->>KeyWalletBuilder: build with pooled funding sources
KeyWalletBuilder-->>AssetLockManager: return transaction and contributors
AssetLockManager->>ReservationCleanup: release reservations for contributors
AssetLockManager->>FundingLookup: search funding records across account families
FundingLookup-->>AssetLockManager: return matching funding transaction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
|
🕓 Ready for review — 2 ahead in queue (commit 5387517) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Cargo.toml (1)
55-62: 🔒 Security & Privacy | 🔵 TrivialTrack the temporary fork migration before release.
The release workflow runs
yarn buildwithCARGO_BUILD_PROFILE=release, so it can publish artifacts built frombfoss765/rust-dashcore. Whendashpay/rust-dashcore#935merges, replace all eight sources withdashpay/rust-dashcoreand refreshCargo.lock.🤖 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 55 - 62, Before release, update all eight rust-dashcore dependency entries—dashcore, dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager, dash-network, and dashcore-rpc—to use the dashpay/rust-dashcore repository after PR `#935` merges, then regenerate Cargo.lock to reflect the new sources and revisions.packages/rs-platform-wallet/src/wallet/asset_lock/build.rs (1)
200-222: 🗄️ Data Integrity & Integration | 🔵 TrivialKeep the current dependency pin until pull request 935 merges. Revision
1a1263a27fa96f7dbf9b283f6fc14101149ab5fdcontains the required API and types. Thedevbranch still has the old API, and pull request 935 remains open. Move the workspace pin todevafter the merge.🤖 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/asset_lock/build.rs` around lines 200 - 222, Keep the workspace dependency pinned to revision 1a1263a27fa96f7dbf9b283f6fc14101149ab5fd for now, since build_asset_lock_with_signer depends on its API and types. Do not switch the pin to dev until pull request 935 has merged.
🤖 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.
Nitpick comments:
In `@Cargo.toml`:
- Around line 55-62: Before release, update all eight rust-dashcore dependency
entries—dashcore, dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi,
key-wallet-manager, dash-network, and dashcore-rpc—to use the
dashpay/rust-dashcore repository after PR `#935` merges, then regenerate
Cargo.lock to reflect the new sources and revisions.
In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- Around line 200-222: Keep the workspace dependency pinned to revision
1a1263a27fa96f7dbf9b283f6fc14101149ab5fd for now, since
build_asset_lock_with_signer depends on its API and types. Do not switch the pin
to dev until pull request 935 has merged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c5b6ea35-6408-4ba0-a332-31eee9cd0e35
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlpackages/rs-platform-wallet-ffi/src/asset_lock/build.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/identity/network/invitation.rspackages/rs-platform-wallet/src/wallet/identity/network/top_up.rspackages/rs-platform-wallet/src/wallet/reservations.rs
thepastaclaw
suggested changes
Aug 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The pooled asset-lock implementation has three merge blockers: the workspace still depends on a contributor-owned rust-dashcore fork, invitation persistence failures leak pooled input reservations, and the FFI restart bridge can discard valid BIP32- or DashPay-only funding records. The family-aware lookup is otherwise coherent, but its new branches need direct regression tests and all public foreign-language funding documentation must describe the new pooled semantics.
Source: reviewers gpt-5.6-sol (Codex general, security-auditor, rust-quality, and ffi-engineer); final verifier gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus 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)
🔴 3 blocking | 🟡 2 suggestion(s)
3 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Do not merge with workspace dependencies pinned to a personal fork
All eight rust-dashcore workspace dependencies resolve from `bfoss765/rust-dashcore`, including foundational crates such as `dashcore`, `dash-spv`, and `key-wallet`. The exact required revision is currently exposed as `dashpay/rust-dashcore` pull request 935's head, while the project-owned `dev` branch remains at `b056d07c61f8618f05082552bbb88072290d57c1`; the PR description also explicitly says not to merge with this fork pin. Merging this revision would make Platform builds and release artifacts depend on a contributor-owned repository. After rust-dashcore#935 merges, repoint every entry to the project-owned repository at the merged revision and regenerate `Cargo.lock`.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:891-900: Release pooled reservations when invitation persistence aborts
After `build_asset_lock_transaction_with_funding` succeeds, each account in `funding_accounts` can hold selected inputs under `reservation_token`. If persisting or flushing the invitation funding index fails, this branch returns before tracking or broadcasting the transaction but never reconciles those reservations. The old single-account branch already retained its reservation here, and this PR worsens that behavior by allowing the abandoned build to reserve inputs across BIP44, BIP32, and DashPay accounts simultaneously. An immediate retry therefore sees those inputs as unavailable until the 24-block TTL sweep. Drop the serialization guard, release the transaction's reservations across every contributing account, and then return the persistence error; extend the existing flush-failure test with an immediate rebuild so it verifies the inputs were released.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:5745-5755: Restore bridge drops pooled locks when the indexed BIP44 account is absent
The pooled source list is lenient: a valid asset lock can be funded entirely by the BIP32 account or DashPay receiving accounts even when `standard_bip44_accounts[account_index]` is absent. The persisted `TrackedAssetLock.account_index` remains only the standard source index, but this restore helper still inserts exclusively into that BIP44 slot and drops the record when it does not exist. After restart, the funding transaction is then absent from every in-memory account map, so the widened `funding_tx_record` lookup cannot recover it and the chain-lock cascade can leave an already-broadcast lock stuck at `Broadcast`. Restore the transaction into an eligible present account family—or extend the restore protocol with sufficient routing information—and add a restart test covering a BIP32- or DashPay-only lock without a BIP44 account at the source index.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:71-80: Add direct coverage for the newly searched BIP32 and DashPay families
The shared lookup now adds behaviorally important BIP32 and DashPay receiving-account branches, but its unit tests still insert records only into BIP44 and CoinJoin accounts. The pooled build tests stop after broadcast and therefore do not exercise proof lookup or the `NoPlatformPersistence` path this widening is intended to support. Add one record stored exclusively in `standard_bip32_accounts` and another stored exclusively in a DashPay receiving account. The DashPay test should use an account whose own index differs from `account_index`, confirming that DashPay accounts are intentionally searched by txid rather than the tracked source index.
In `packages/rs-platform-wallet-ffi/src/identity_top_up.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_top_up.rs:172-175: Foreign API documentation still promises BIP44-only funding
This public C API documentation says `account_index` selects the sole BIP44 funding account, but the call now pools the BIP44 and BIP32 accounts at that index with every DashPay receiving account. The same stale contract remains on identity registration, platform-address funding, shielded asset-lock and seed-pool exports, and their Swift wrappers. Foreign callers following those docs may present an account-specific funding choice while Rust actually spends and links UTXOs from other families. Update every affected C and Swift surface to state that the index addresses the standard families and does not constrain the DashPay contributors included in the pool.
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 9, 2026
`cargo clippy --lib --tests -- -D warnings` has not been green on this branch; the failures predate dashpay#4350 and the classifier fix and would have blocked their gate. All mechanical, none behavioural: platform-wallet * `changeset/core_bridge.rs` — `AssetLockFundingAccount` imported but unused in `asset_locks_only_batch_reaches_store`, left behind by the dashpay#4342 prereq merge (618ce41). * `wallet/core/send.rs` — `MAX_FEE_PER_KB > 1_000_000` compares two constants, so `assertions_on_constants` fires. Moved into a `const` block, which is what it always meant: a compile-time check. * `wallet/identity/network/withdrawal.rs` (x2) — `let keys = vec![...]` for a fixture only ever passed as `keys.iter()`. Plain array. * `wallet/asset_lock/sync/recovery.rs` — `await_holding_lock`. Checked before changing anything: the guard is a `std::sync::MutexGuard` on the recording broadcaster's captured transactions, and the test ALREADY released it with an explicit `drop(broadcast)` before the `.await`s below — clippy simply does not track `drop`. Scoping the guard in a block releases it at the identical point, so this is a lint fix with no locking-semantics change; it is NOT an `#[allow]` hiding a live guard-across-await. platform-wallet-ffi * `core_wallet_types.rs`, `persistence.rs` — `map_or(false, ..)` -> `is_some_and`, `get(&k).is_none()` -> `!contains_key(&k)`. * `persistence.rs` (x8) — `PersistenceCallbacks::default()` followed by field assignment -> struct-update syntax. Same fields, same values. rustfmt * `rs-sdk-ffi/src/signer.rs` — one call the committed formatting had on a single line and the pinned rustfmt wraps. `cargo fmt --all -- --check` is now clean. platform-wallet 637 tests, platform-wallet-ffi 254, rs-unified-sdk-jni 38 — all still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses the review finding that the 8 rust-dashcore workspace dependencies pinned a personal fork (bfoss765/rust-dashcore). The feature branch has been pushed to dashpay/rust-dashcore, so the deps now resolve from the project-owned repository at the same rev (1a1263a27fa96f7dbf9b283f6fc14101149ab5fd). The final repoint to the post-merge dev rev still happens once rust-dashcore#935 merges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…_record Addresses the review finding that the widened shared proof lookup had unit coverage only for the BIP44 and CoinJoin families. Adds: - funding_tx_record_finds_bip32_only_record: a record filed only under standard_bip32_accounts is visible to the lookup. - funding_tx_record_finds_dashpay_receival_record_across_indices: a record filed only under a DashPay contact-receiving account whose own index (7) differs from the tracked source account_index (0) is still found — DashPay receiving accounts are searched by txid, not by the tracked source index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 55-62: Update the revisions for all eight rust-dashcore
dependencies in Cargo.toml to the merged dev branch revision, replacing the
temporary pin consistently. Regenerate Cargo.lock so its git dependency entries
reference the same revision, then run the workspace cargo test, cargo check,
cargo clippy, and cargo fmt checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8668a0d-0f08-4674-b8c1-9fabc54303cd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlpackages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs
thepastaclaw
suggested changes
Aug 10, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The project-owned dependency URL and direct BIP32/DashPay lookup tests resolve two prior findings, and the pooled funding implementation is otherwise coherent. Three blockers remain: the upstream implementation is still absent from rust-dashcore's dev branch, invitation durability failures strand every pooled input reservation, and restart restoration drops valid pooled locks when the indexed BIP44 account is absent; several foreign API comments also retain the obsolete BIP44-only contract.
Source: reviewers gpt-5.6-sol (Codex general, security-auditor, rust-quality, and ffi-engineer); final verifier gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus 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)
🔴 3 blocking | 🟡 1 suggestion(s)
3 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Update all eight pins to the merged `dev` revision before merge
The manifest and lockfile now use the project-owned repository, but they still pin the unmerged `rust-dashcore#935` head (`1a1263a27fa96f7dbf9b283f6fc14101149ab5fd`). That PR was closed without merging on 2026-08-10 and superseded by open PR #944 (`f07f8d5a7b7caa427df01b088434ebb8fa5a813d`); the current `dev` head is `d91ad055c58eec241048226d4fa0586d35d148bc`, and an ancestry check confirms it does not contain the pinned pooled asset-lock implementation. Repointing to `dev` immediately would therefore remove APIs this PR calls, but merging Platform against the stale, unmerged #935 revision would bypass the upstream integration this PR declares as a prerequisite. Wait for #944 to merge, then update all eight entries and `Cargo.lock` to the resulting `dev` revision.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:891-899: Release pooled reservations when invitation persistence aborts
A successful pooled build reserves each selected input in its contributing account under `reservation_token`. If persisting or flushing the invitation funding index fails, this branch returns before tracking or broadcasting the transaction but never releases those reservations. An immediate retry consequently sees the selected BIP44, BIP32, and/or DashPay inputs as unavailable until the 24-block TTL sweep, even though no transaction reached Core and no tracked row can resume it. Drop the build/persist guard, owner-release the reservation from every account in `funding_accounts`, and then return the durability error. Extend the flush-failure test with an immediate pooled rebuild to verify that all contributors were reconciled.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:5738-5755: Restore bridge drops pooled locks when the indexed BIP44 account is absent
The new multi-source builder is deliberately lenient: it skips absent source families, so a valid asset lock can be funded entirely by BIP32 or a DashPay receiving account even when `standard_bip44_accounts[account_index]` is absent. The persisted `TrackedAssetLock.account_index` is only the family-independent source index, but this restore helper still inserts exclusively into the BIP44 slot and drops the transaction otherwise. After restart, the transaction is absent from every in-memory account map, so the family-aware `funding_tx_record` lookup and the chain-lock promotion cascade cannot recover it; an already-broadcast lock can remain stuck at `Broadcast`. Route the synthetic record into an eligible present family, or extend the restore record with contributor routing information, and add restart coverage for BIP32-only and DashPay-only locks without the indexed BIP44 account.
In `packages/rs-platform-wallet-ffi/src/identity_top_up.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_top_up.rs:173-175: Foreign API documentation still promises BIP44-only funding
This public C API says `account_index` selects the sole BIP44 funding account, but `AssetLockFunding::FromWalletBalance` now pools the BIP44 and BIP32 accounts at that index with every DashPay receiving account. The same stale contract remains on identity registration, platform-address funding, shielded asset-lock and seed-pool APIs, and their Swift wrappers. Callers following these comments may expose an account-specific funding choice while Rust actually co-spends and links UTXOs from additional account families. Update all affected C and Swift surfaces to state that the index addresses the standard families and does not constrain which DashPay receiving accounts can contribute.
rust-dashcore dashpay#944 (supersedes dashpay#935 with the same pooled-funding content, authorship preserved) merged to dev; all eight workspace deps now pin the project-owned repo at the merged revision 5a80bd71f6, answering the review blocker. No API adaptation was needed: this PR already passes its own explicit source set, which is exactly the caller-chosen shape dashpay#944 standardized. 615 platform-wallet tests green against the new pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 10, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The rust-dashcore prerequisite is now satisfied, and the new family-aware funding lookup has direct BIP32 and DashPay coverage. Three in-scope blockers remain: invitation durability failures strand pooled reservations, FFI restoration can discard locks when the indexed BIP44 account is absent, and reconstruction still records the wrong source index for BIP32-only funding. Public C and Swift documentation also continues to describe obsolete BIP44-only semantics.
Source: reviewers gpt-5.6-sol (Codex general, security-auditor, ffi-engineer, and rust-quality); final verifier gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus 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— ffi-engineer (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
4 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:891-899: Release pooled reservations when invitation persistence aborts
The pooled build has already reserved every selected input in its contributing account under `reservation_token`. If storing or flushing the invitation funding index fails, this branch returns before tracking or broadcasting the transaction but does not release those reservations. Dropping the local transaction and contributor list has no effect on the accounts' reservation sets, so an immediate retry cannot select the BIP44, BIP32, or DashPay inputs until the 24-block TTL sweep. Drop the serialization guard, owner-release the reservation across every entry in `funding_accounts`, and then return the durability error. Extend the flush-failure test with an immediate pooled rebuild to prove all contributors were reconciled.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:5738-5755: Restore bridge drops pooled locks when the indexed BIP44 account is absent
The pooled builder intentionally skips absent source families, so a valid asset lock can be funded entirely by BIP32 or a DashPay receiving account when `standard_bip44_accounts[account_index]` does not exist. `TrackedAssetLock.account_index` remains a family-independent source index, but this restore helper still inserts exclusively into that BIP44 slot and discards the record otherwise. After restart, the transaction is therefore absent from every in-memory account map; the widened `funding_tx_record` lookup and chain-lock promotion path cannot recover a record that restoration dropped, leaving an already-broadcast lock potentially stuck at `Broadcast`. Restore the synthetic record into a present family searched by `funding_tx_record`, or persist enough contributor information to route it accurately, and add BIP32-only and DashPay-only restart tests without the indexed BIP44 account.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs:165-170: Recover the source index from BIP32-funded asset locks
`funding_account_index` still checks only BIP44 and CoinJoin records, although this PR allows a lock to be funded exclusively from a BIP32 account at a nonzero `source_index`. The purpose-specific funding-account record triggers reconstruction, and the BIP32 sibling identifies the actual source index, but this function ignores it and stores the fallback index 0. A pre-final reconstructed lock then passes index 0 to `wait_for_proof`; `funding_tx_record` cannot find the record filed under the actual nonzero BIP32 index, and `NoPlatformPersistence` provides no fallback. IS-lock and chain-lock notifications only repeat that miss, so an unbounded proof wait can remain parked even after finality. Include `standard_bip32_accounts` in index inference and add a nonzero-index reconstruction/proof regression.
In `packages/rs-platform-wallet-ffi/src/identity_top_up.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_top_up.rs:173-175: Foreign API documentation still promises BIP44-only funding
This public C API says `account_index` selects the sole BIP44 funding account, but `AssetLockFunding::FromWalletBalance` now pools the BIP44 and BIP32 accounts at that index with every DashPay receiving account. The same obsolete contract remains on identity registration, platform-address funding, shielded asset-lock and seed-pool APIs, persisted asset-lock routing fields, and the corresponding Swift wrappers. A caller following these comments may expose an account-scoped funding or privacy choice while Rust actually co-spends and links UTXOs from additional account families. Update all affected C and Swift surfaces to explain that the index addresses the standard families and does not restrict which DashPay receiving accounts can contribute.
1. Invitation-durability abort no longer strands pooled reservations: the pre-broadcast durability error now drops the serialization guard and owner-releases across every contributing account before returning, exactly like the drain-floor abort above it. The flush-failure test now proves an immediate rebuild reselects the released inputs. 2. The FFI restore bridge no longer drops pooled locks: a record whose indexed BIP44 account is absent (a lock funded entirely from BIP32 or a DashPay receiving account — the pooled builder skips absent families) now restores into the first present family the proof lookup searches (BIP44 -> BIP32 -> CoinJoin by index, then any receival account, which the lookup scans by txid). New BIP32-only and DashPay-only restore tests. 3. Reconstruction recovers the true source index for BIP32-funded locks: funding_account_index now searches standard_bip32_accounts (same family order as funding_tx_record), so a nonzero-index BIP32 lock no longer stores fallback 0 and parks its proof wait forever under NoPlatformPersistence. New nonzero-index regression. Also updates every C and Swift surface that still promised BIP44-only funding (top-up, registration, platform-address, shielded asset-lock, restore routing field, Swift wrappers) to the pooled contract: the index addresses the standard families and does not restrict which DashPay receiving accounts contribute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
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 `@packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- Around line 484-490: Update the documentation near
restore_unresolved_asset_lock_tx_records to describe the complete routing order,
including the coinjoin_accounts[account_index] check before the DashPay
receiving-account fallback; alternatively, explicitly state that CoinJoin
records are excluded from this type.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 3866-3872: Update the documentation for topUpIdentityWithFunding
so accountIndex describes pooled BIP44, BIP32, and all DashPay contact-receiving
accounts, with change returning to BIP44; clarify that it does not scope funding
or privacy and that CoinJoin remains drain-only and unreachable. Keep the
implementation unchanged and synchronize the contract with identity_top_up.rs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88e6996e-0d73-4c83-a7c7-d55cacfaae28
📒 Files selected for processing (10)
packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_top_up.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet-ffi/src/wallet_restore_types.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current head fixes all three prior pooled-funding blockers: invitation aborts release every contributing reservation, unresolved records restore without requiring BIP44, and BIP32-funded reconstruction preserves nonzero source indices. Three in-scope suggestions remain: several public foreign-language contracts still advertise obsolete BIP44-only semantics, the Swift example-app preflights prevent valid pooled balances from reaching Rust, and restore documentation omits CoinJoin from the implemented routing order.
Source: reviewers gpt-5.6-sol (Codex general, security-auditor, rust-quality, and ffi-engineer); final verifier gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 3 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:661-664: Foreign API documentation still promises BIP44-only funding
This public Swift parameter still says the selected BIP44 account alone funds the asset lock and explicitly claims BIP32 is unsupported. The method delegates to the pooled `AssetLockFunding::FromWalletBalance` path, which combines the BIP44 and BIP32 accounts at this source index with every DashPay receiving account. The same obsolete contract remains on `ManagedPlatformWallet.topUpIdentityWithFunding` at lines 4094-4096, the public shielded seed-pool export in `rs-platform-wallet-ffi/src/shielded_send.rs` at lines 1326-1328, and persisted/tracked fields such as `AssetLockEntryFFI.account_index` and `TrackedAssetLockFFI.account_index`. These comments can cause callers to present an account-scoped or privacy-preserving funding choice even though Rust may co-spend other families. Update every remaining foreign-facing surface to describe the family-independent source index, unrestricted DashPay contributors, BIP44 change routing, and CoinJoin's separate drain-only behavior.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift:2188-2207: Swift preflights prevent pooled balances from reaching Rust
The example app still exposes only BIP44 rows for Core identity funding and says BIP32 is intentionally unsupported, while `registerIdentityWithFunding` now invokes Rust's pooled asset-lock builder. Its `canSubmit` check at lines 1037-1041 also compares the requested amount only with the selected BIP44 balance. A BIP32-only or DashPay-only wallet therefore has no selectable Core source, and a wallet whose required balance is split across eligible families is rejected before the FFI call. The platform-address and shielded views repeat this mismatch by filtering to BIP44 and checking only that account's balance in `FundFromAssetLockPlatformAddressView.swift:403-429,472-486` and `ShieldedFundFromAssetLockView.swift:795-810,839-844`. Model the picker as a source index and calculate availability from the same pooled families as Rust, or defer the sufficiency decision to the Rust result.
In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:484-490: Document CoinJoin in unresolved asset-lock restore routing
The field documentation says restoration tries the standard families and then DashPay, but the implementation also checks `coinjoin_accounts[account_index]` between BIP32 and the DashPay fallback. CoinJoin drain-funded locks use this record type, so omitting that branch leaves the documented routing order incomplete. The function-level comment in `persistence.rs:5659-5662` is also stale in the opposite direction because it still says records are projected only into BIP44 slots. Update both comments to state the actual BIP44, BIP32, CoinJoin, then DashPay routing order.
The restore-routing doc now states the full family order including the CoinJoin step, and the Swift top-up wrapper carries the same pooled contract as its registration sibling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ManagedPlatformAddressWallet funding param, the shielded seed-pool export, the restore projection function doc (full BIP44 -> BIP32 -> CoinJoin -> DashPay routing order), and the persisted/tracked account_index fields (AssetLockEntryFFI, TrackedAssetLockFFI) no longer promise BIP44-only funding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
#4329 pooled the send path and left asset locks single-account. An invitation, identity registration or top-up could only be funded from ONE BIP44 account, so a wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them together first and lock out of the sweep — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege. On Android that sweep-then-lock shape is what users actually hit.
This is the approved follow-up: the same pooling, for asset locks.
Depends on dashpay/rust-dashcore#935.
Cargo.tomlis pinned to that branch's rev so CI here is honest about what it is testing; the pin must be re-pointed todashpay/rust-dashcoredevwhen #935 merges — please don't merge this with the fork pin in place.What was done?
ASSET_LOCK_FUNDING_SOURCES(deliberately the same set asSEND_FUNDING_SOURCES: BIP44, BIP32,AllDashpayReceivingFunds) is now the default for asset-lock funding. Coin selection draws from the union, the first source supplies the change address so change returns to BIP44, and sources this wallet has nothing for — no BIP32 account, no contacts — are skipped rather than fatal.build_asset_lock_transaction_with_fundingandbroadcast_funded_asset_lock_with_fundingtake the source list plus asource_indexin place of a singleAssetLockFundingAccount. The historical entry points —build_asset_lock_transaction,broadcast_funded_asset_lock,create_funded_asset_lock_proof— keep their exact signatures and just pass the pooled set, so every call site above them becomes pooled with no new plumbing:create_invitation, identity registration, top-up, platform-address and shielded funding, and theasset_lock_manager_build_transactionFFI export. Nothing gained an elective funding selector it does not need, and #4337'sbuild_asset_lock_transaction(…, account_index, …)recovery helper call is unaffected.Reservation reconciliation — the part to review hardest
release_reservation_after_rejected_broadcastnow takes the contributing account list instead of oneReservedFundingAccount. A pooled build reserves in each contributing account's ownReservationSetunder the one owner token, so a release reaching only the first account would strand the rest of the inputs until the 24-block TTL backstop, and an immediate retry would fail with spurious insufficient funds. The build returns those accounts (upstream'sAssetLockResult::funding_accounts, which is the contributor list — selection routinely takes nothing from most offered accounts, and a list naming every contact would make this scale with the address book), and both rejection paths — the undersized-drain abandon and the rejected broadcast — release across all of them, still owner-guarded (#4185).ReservedFundingAccountis deleted:AccountTypealready names every family, including the DashPay accounts the old enum could not express at all.A lookup that had to widen with it
funding_tx_recordresolved the funding transaction in BIP44 and CoinJoin at the tracked index only. key-wallet files a transaction under every account its inputs touch, and a pooled lock may take nothing from BIP44 — funded entirely out of BIP32 or a contact account. The lookup would miss it, which burns the proof wait and is outright fatal underNoPlatformPersistence, whose persister fallback always returnsNone. It now covers the standard pair and CoinJoin at the index, then the DashPay receiving accounts by txid (they span their own indices). This is the same class of gap #4336's CoinJoin fix closed, arriving here because pooling opens it.TrackedAssetLock.account_indexkeeps its meaning as the source index and its persisted shape — no schema change — but its doc now says plainly that it is not a record of which accounts funded the lock.CoinJoin is untouched
Still drain-only, still a single account, now routed through
create_funded_asset_lock_proof_with_funding, which converts it to a one-element source list. Upstream additionally rejects a CoinJoin source pooled with any other, since spending mixed outputs alongside transparent ones links them and undoes the mixing. #4327's flow and itsundersized_drain_abandoned_before_broadcasttest are unchanged.Docs that asserted the old invariant
AssetLockFunding::FromWalletBalance("This exact-amount form is BIP44-only … BIP32 funding remains unsupported"),top_up_identity'saccount_index("Only BIP44 standard accounts are supported today"), and the invitation build comment all described the single-account world. They now describe the pooled one rather than silently contradicting the code.How Has This Been Tested?
cargo test -p platform-wallet— 613 lib tests + integration suites, 0 failures.cargo check --workspace --all-targetsclean;cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warningsclean;cargo fmtapplied.New tests, on the fixtures #4329 added for the pooled send:
pooled_asset_lock_spans_the_standard_families— a 1,000,000-duff lock against two 700,000-duff accounts. Before pooling this wasCoreInsufficientFunds.pooled_asset_lock_spends_dashpay_contact_funds— the same against a realDashpayReceivingFundscontact account, so the contact path cannot silently degrade to BIP44 + BIP32.rejected_pooled_broadcast_releases_every_contributing_account— the reservation hazard. A rejected pooled broadcast, then an identical rebuild that can only succeed if both families' inputs came back.I checked that last one is not vacuous: with the release loop truncated to the first account it fails with
Insufficient funds: available 700000, required 1000000, which is exactly the stranding it is meant to catch. The upstream siblings in #935 were mutation-checked the same way.Breaking Changes
Internal to the crate — no FFI signature changes, no persistence format change:
build_asset_lock_transaction_with_funding/broadcast_funded_asset_lock_with_fundingtakefunding_sources: &[AccountTypePreference], source_index: u32; the former also returns the contributing accounts.create_funded_asset_lock_proof_with_fundingkeepsAssetLockFundingAccountand is now explicitly the whole-balance drain form.pub(crate) enum ReservedFundingAccountremoved in favour ofAccountType.platform_wallet::ASSET_LOCK_FUNDING_SOURCES.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation