Skip to content

chore: bump rust-dashcore to v0.42-dev (428b60d)#3617

Merged
QuantumExplorer merged 2 commits intov3.1-devfrom
claude/serene-mestorf-be5aff
May 7, 2026
Merged

chore: bump rust-dashcore to v0.42-dev (428b60d)#3617
QuantumExplorer merged 2 commits intov3.1-devfrom
claude/serene-mestorf-be5aff

Conversation

@QuantumExplorer
Copy link
Copy Markdown
Member

@QuantumExplorer QuantumExplorer commented May 7, 2026

Issue being fixed or feature implemented

Switches the workspace's rust-dashcore git deps from the diverged v0.42-platform-nightly (fe247661) to v0.42-dev HEAD (428b60d). The nightly branch had accumulated parallel work that's been superseded by clean upstream merges; v0.42-dev is now the canonical line.

What was done?

Dep bump

All 9 rust-dashcore [workspace.dependencies] entries point to 428b60d0c93b7842876e568ebb40c2e5455d4277. Cargo.lock regenerated; the entire rust-dashcore subgraph now resolves to a single rev.

Upstream PRs adopted

  • dashpay/rust-dashcore#742 — wire ManagedCoreKeysAccount into the collection. ManagedAccountCollection now stores funds-bearing and keys-only accounts in separate fields and spanning accessors return ManagedAccountRef<'a> (a borrowed enum). Funds-only operations require an explicit as_funds() unwrap.
  • dashpay/rust-dashcore#727 — tx building / signing logic moved into key-wallet and key-wallet-manager. next_receive_address / next_change_address now return Option<Address> instead of Result, and AccountTypePreference is a 2-variant enum.
  • dashpay/rust-dashcore#710wallet_create_managed_wallet FFI removed. The proper construction path is via WalletManager.
  • dashpay/rust-dashcore#733keep-finalized-transactions Cargo feature (replaces the nightly's keep_txs_in_memory toggle).
  • dashpay/rust-dashcore#729 / #708 — serde tolerance for ContentDeserializer's HR-quirk on OutPoint and Hash types.
  • #738 / #739 / #737 / #736 / #721 / #724 / #697 — internal dash-spv refactors / docs / tests / seed refresh; no platform-side changes required.

Platform-side adaptations (22 errors closed)

  • transactions field on ManagedCoreFundsAccount is now the trait method transactions() returning &BTreeMap<Txid, TransactionRecord>. Migrated 5 sites in rs-platform-wallet/src/wallet/asset_lock to use the accessor (added ManagedAccountTrait imports where needed).
  • managed_account_type is now an accessor method on the trait. Updated the dashpay_receival_accounts walk in contacts.rs.
  • insert_funds renamed to insert_funds_bearing_account on ManagedAccountCollection — 2 sites in contacts.rs.
  • address_derivation_path lives on ManagedAccountTrait, not on the ref-enum. broadcast.rs and payments.rs now go through account.keys_account().address_derivation_path(...) since address pools live on the keys variant after the split.
  • next_address resolves through the trait — added imports in payments.rs and asset_lock/build.rs.
  • The two transactions_iter() callers in accessors.rs switch to transactions().values() (clippy-preferred).
  • Comments referencing the nightly's keep_txs_in_memory toggle renamed to upstream's keep-finalized-transactions.
  • Removed the orphaned Swift ManagedWallet class and KeyWalletManagedWallet typealias. Its only public init called the upstream-removed wallet_create_managed_wallet, and no production code constructed it. Per the swift-sdk CLAUDE.md ownership rules, managed-wallet construction is reached via WalletManager rather than a state-empty Swift-side wrapper.

New keep-finalized-transactions feature on platform-wallet

Off by default. When enabled, forwards to key-wallet/key-wallet-manager's same feature so the in-memory transactions map retains chainlocked records (useful for debug surfaces that page through history). Default off matches the production model where tx history is delivered through the event channel.

keep-finalized-transactions = [
    "key-wallet/keep-finalized-transactions",
    "key-wallet-manager/keep-finalized-transactions",
]

What's lost from the nightly branch (deferred — not blocking)

  • feat(dash-spv-ffi): expose addresses_derived on Wallet event callbacks — confirmed unused in this repo (platform consumes addresses_derived through key-wallet-manager WalletEvent in core_bridge.rs, not via dash-spv-ffi callbacks). Can be re-PR'd upstream when an FFI consumer needs it.
  • review(key-wallet-manager): per-record derivations, observability, real dedup test and drop redundant derivation_path from DerivedAddress — refinements that didn't make it into v0.42-dev's chore: use core 18.2.0 on testnet #725 merge; can be re-PR'd as follow-ups.

How Has This Been Tested?

  • cargo check --workspace --all-targets: clean
  • cargo check -p platform-wallet --features keep-finalized-transactions: clean (forwarded feature compiles)
  • cargo clippy --workspace --all-targets: clean (3 pre-existing warnings unrelated to this change)
  • cargo test -p platform-wallet --lib: 115/115 pass
  • iOS xcframework build (sim, dev profile): running at PR-creation time; will follow up with any fixes if the C-header regen surfaces FFI-side breakage.

Breaking Changes

None for platform consumers. The Swift ManagedWallet class is removed but had no production callers and was orphaned by upstream's PR #710.

Summary by CodeRabbit

  • New Features

    • Added asset lock transaction building and funded proof creation capabilities.
  • Breaking Changes

    • Removed KeyWalletManagedWallet type from Swift SDK public API.
  • Chores

    • Updated core wallet dependency revisions to latest versions.

Switches from the diverged v0.42-platform-nightly branch
(fe247661) to v0.42-dev HEAD (428b60d). The nightly branch had
accumulated parallel work that's been superseded by clean
upstream merges; v0.42-dev is now the canonical line.

Adopts these upstream PRs in addition to the dep bump:

- dashpay/rust-dashcore#742 — wire ManagedCoreKeysAccount into the
  collection: ManagedAccountCollection now stores funds-bearing
  and keys-only accounts in separate fields and spanning accessors
  return ManagedAccountRef<'a> (a borrowed enum). Funds-only
  operations require an explicit as_funds() unwrap.
- dashpay/rust-dashcore#727 — tx building / signing logic moved
  into key-wallet and key-wallet-manager. next_receive_address /
  next_change_address now return Option<Address> instead of
  Result, and AccountTypePreference is a 2-variant enum.
- dashpay/rust-dashcore#710 — wallet_create_managed_wallet FFI
  removed. The proper construction path is via WalletManager.
- dashpay/rust-dashcore#733 — keep-finalized-transactions Cargo
  feature (replaces the nightly's keep_txs_in_memory toggle).
- dashpay/rust-dashcore#729 / #708 — serde tolerance for
  ContentDeserializer's HR-quirk on OutPoint and Hash types.
- dashpay/rust-dashcore#738 / #739 / #737 / #736 / #721 / #724 /
  #697 — internal dash-spv refactors / docs / tests / seed
  refresh; no platform-side changes required.

Platform-side adaptations:

* `transactions` field on ManagedCoreFundsAccount is now the trait
  method `transactions()` returning `&BTreeMap<Txid, TransactionRecord>`.
  Migrated 5 sites in rs-platform-wallet/src/wallet/asset_lock to
  use the accessor (added `ManagedAccountTrait` imports where needed).
* `managed_account_type` is now an accessor method on the trait.
  Updated the dashpay_receival_accounts walk in
  wallet/identity/network/contacts.rs.
* `insert_funds` renamed to `insert_funds_bearing_account` on
  ManagedAccountCollection — 2 sites in contacts.rs.
* `address_derivation_path` lives on ManagedAccountTrait, not on
  the ref-enum. broadcast.rs and payments.rs now go through
  `account.keys_account().address_derivation_path(...)` since
  address pools live on the keys variant after the split.
* `next_address` resolves through the trait — added imports in
  payments.rs and asset_lock/build.rs.
* The two `transactions_iter()` callers in manager/accessors.rs
  switch to `transactions().values()` (clippy-preferred).
* Comments referencing the nightly's `keep_txs_in_memory` toggle
  renamed to upstream's `keep-finalized-transactions`.
* Removed the orphaned Swift `ManagedWallet` class
  (KeyWallet/ManagedWallet.swift) and the
  KeyWalletManagedWallet typealias. Its only public init
  called the upstream-removed `wallet_create_managed_wallet`,
  and no production code constructed it. Per the swift-sdk
  CLAUDE.md ownership rules, managed-wallet construction is
  reached via WalletManager rather than a state-empty
  Swift-side wrapper.

Adds a forwarded `keep-finalized-transactions` feature on
platform-wallet (off by default). When enabled, it forwards to
key-wallet/key-wallet-manager's same feature so the in-memory
transactions map retains chainlocked records (useful for debug
surfaces). Default off matches the production model where tx
history is delivered through the event channel.

What's lost from the nightly branch (deferred, not blocking):

- `feat(dash-spv-ffi): expose addresses_derived on Wallet event
  callbacks` — confirmed unused in this repo (platform consumes
  addresses_derived through key-wallet-manager WalletEvent in
  changeset/core_bridge.rs, not via dash-spv-ffi callbacks). Can
  be re-PR'd upstream when an FFI consumer needs it.
- `review(key-wallet-manager): per-record derivations,
  observability, real dedup test` and `drop redundant
  derivation_path from DerivedAddress` — refinements that didn't
  make it into v0.42-dev's #725 merge; can be re-PR'd as
  follow-ups.

Verification:

- cargo check --workspace --all-targets: clean
- cargo check -p platform-wallet --features keep-finalized-transactions: clean
- cargo clippy --workspace --all-targets: clean (3 pre-existing
  warnings unrelated to this change)
- cargo test -p platform-wallet --lib: 115/115 pass
- iOS xcframework build (sim, dev profile): in progress at
  commit time

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@QuantumExplorer QuantumExplorer requested a review from shumkov as a code owner May 7, 2026 15:48
@github-actions github-actions Bot added this to the v3.1.0 milestone May 7, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Review Change Stack

Warning

Rate limit exceeded

@QuantumExplorer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 46 minutes and 13 seconds before requesting another review.

To continue reviewing without waiting, purchase usage credits in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 45190d85-0362-42f6-ab30-c726317efa6a

📥 Commits

Reviewing files that changed from the base of the PR and between aac3aa9 and d4aff16.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • Cargo.toml
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
📝 Walkthrough

Walkthrough

This PR upgrades rust-dashcore dependencies and adapts platform-wallet code to the new transaction and account access APIs. Transaction lookups now use method-based access; account type queries and key derivation paths are obtained via trait methods. New asset-lock implementation includes transaction building and unified proof orchestration. Swift SDK removes a deprecated typealias.

Changes

Asset Lock and Transaction API Upgrade

Layer / File(s) Summary
Dependencies and Feature Configuration
Cargo.toml, packages/rs-platform-wallet/Cargo.toml
rust-dashcore dependencies updated to new revision. keep-finalized-transactions feature flag added to enable in-memory transaction retention.
Transaction Access API Migration
packages/rs-platform-wallet/src/changeset/core_bridge.rs, packages/rs-platform-wallet/src/manager/accessors.rs, packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs, packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
All transaction record lookups migrate from field access (transactions.get()) to method calls (transactions().get()); iterator counting replaced with transactions().len() and transactions().values(). Empty map behavior when feature disabled documented.
Account Keys Trait and Method Integration
packages/rs-platform-wallet/src/wallet/core/broadcast.rs, packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs, packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
ManagedAccountTrait imported across files. Address derivation paths now accessed via account.keys_account().address_derivation_path() instead of direct account method. Account type obtained via managed_account_type() method.
Contact and Account Registration Updates
packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs
DashPay contact account registration switched from insert_funds to insert_funds_bearing_account API in both mutual contact and external account paths. Account type matching uses managed_account_type() method.
Asset Lock Transaction Building and Proof Flow
packages/rs-platform-wallet/src/wallet/asset_lock/build.rs
build_asset_lock_transaction validates amount and selects funding address; peek_next_funding_address derives address without marking used; create_funded_asset_lock_proof orchestrates full flow: build, broadcast, wait for proof (300s timeout), validate/upgrade, advance status, queue changesets, and return proof with private key and OutPoint.
Swift SDK Public API Cleanup
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWallet.swift, packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift
Removes KeyWalletManagedWallet typealias from public re-exports while preserving other convenience typealiases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • dashpay/platform#3225: Updates rust-dashcore workspace dependencies and adapts wallet code to upstream API changes from dependency revision bumps.

Suggested labels

ready for final review

Poem

🐰 Through method gates the transactions flow,
Keys unlocked by traits so new,
Asset locks and proofs now grow,
Swift SDK deprecated—adieu!
APIs guide the cleaner path we go.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: bumping rust-dashcore to v0.42-dev with commit hash 428b60d, which aligns with the changeset's primary objective of updating workspace dependencies.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/serene-mestorf-be5aff

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@QuantumExplorer QuantumExplorer changed the title chore(deps): bump rust-dashcore to v0.42-dev (428b60d) chore: bump rust-dashcore to v0.42-dev (428b60d) May 7, 2026
@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented May 7, 2026

Review Gate

Commit: d4aff16c

  • Debounce: 13m ago (need 30m)

  • CI checks: builds passed, 0/0 tests passed

  • CodeRabbit review: comment found

  • Off-peak hours: peak window (5am-11am PT) — currently 09:15 AM PT Thursday

  • Run review now (check to override)

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (1)

32-37: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

try_write should be try_read — unnecessary exclusive lock on a read-only critical section.

The lock guard wm is used only for reads (get_wallet_info + match_in_collection) before it is dropped. Holding a write lock here blocks every other concurrent reader and writer for no reason. Use try_read() instead:

🔧 Proposed fix
-        let wm = self.wallet_manager.try_write().map_err(|_| ())?;
+        let wm = self.wallet_manager.try_read().map_err(|_| ())?;
         let Some(info) = wm.get_wallet_info(&self.wallet_id) else {
             return Ok(None);
         };
         let m = Self::match_in_collection(info, address);
         drop(wm);
🤖 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/identity/network/payments.rs` around
lines 32 - 37, The code is taking an exclusive write lock with
self.wallet_manager.try_write() but only performs reads (get_wallet_info and
Self::match_in_collection), so replace the write lock with a shared read lock by
using try_read() instead of try_write(), keep the error mapping behavior and
variable name (wm) consistent, call get_wallet_info(&self.wallet_id) and then
Self::match_in_collection(info, address) while dropping the read guard after use
to avoid holding the lock longer than necessary.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (2)

131-135: 💤 Low value

DashpayAccountKey constructed twice with identical fields.

The same key is built at lines 131-135 (inside the first inner block) and again at lines 151-155. Both blocks operate within the same outer let (payment_address, tx) = { ... } scope. Since the first block ends before the second begins (the ? on the first block exits the scope), the key cannot be reused directly, but the duplication could be avoided by hoisting the key construction to the outer scope:

♻️ Proposed refactor
+        let dashpay_key = DashpayAccountKey {
+            index: account_index,
+            user_identity_id: from_identity_id.to_buffer(),
+            friend_identity_id: to_contact_id.to_buffer(),
+        };
         let (payment_address, tx) = {
             let mut wm = self.wallet_manager.write().await;
             let contact_xpub = {
                 let wallet = wm.get_wallet(&self.wallet_id).ok_or_else(|| { ... })?;
                 wallet
                     .accounts
                     .dashpay_external_accounts
-                    .get(&DashpayAccountKey {
-                        index: account_index,
-                        user_identity_id: from_identity_id.to_buffer(),
-                        friend_identity_id: to_contact_id.to_buffer(),
-                    })
+                    .get(&dashpay_key)
                     ...
             };
             ...
-            let key = DashpayAccountKey {
-                index: account_index,
-                user_identity_id: from_identity_id.to_buffer(),
-                friend_identity_id: to_contact_id.to_buffer(),
-            };
-            let external_account = info.core_wallet.accounts.dashpay_external_accounts.get_mut(&key)
+            let external_account = info.core_wallet.accounts.dashpay_external_accounts.get_mut(&dashpay_key)

Also applies to: 151-155

🤖 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/identity/network/payments.rs` around
lines 131 - 135, DashpayAccountKey is constructed twice with identical fields
inside the same let (payment_address, tx) = { ... } flow; hoist creating a
single DashpayAccountKey before the inner blocks (using account_index,
from_identity_id.to_buffer(), to_contact_id.to_buffer()) and reuse that variable
in both places where .get(&DashpayAccountKey { ... }) is currently called
(instead call .get(&dashpay_account_key) or similar), ensuring the key is
immutable and in scope for both lookups to remove duplication while preserving
existing error propagation via ?.

43-56: ⚡ Quick win

Spawned task errors are silently swallowed — consider logging failures.

The JoinHandle returned by tokio::spawn is immediately dropped, so a panic or unwind in the task is never observed. Inside the task, record_dashpay_payment's return value is also discarded, so a persistence failure goes unnoticed. At minimum, log on failure:

🔧 Proposed fix
         tokio::spawn(async move {
             let mut wm = this.wallet_manager.write().await;
             if let Some(info) = wm.get_wallet_info_mut(&this.wallet_id) {
                 if let Some(managed) = info.identity_manager.managed_identity_mut(&owner_id) {
-                    managed.record_dashpay_payment(
+                    if let Err(e) = managed.record_dashpay_payment(
                         txid,
                         crate::wallet::identity::types::dashpay::payment::PaymentEntry::new_received(
                             contact_id, value, None,
                         ),
                         &this.persister,
-                    );
+                    ) {
+                        tracing::warn!(error = %e, "Failed to record incoming DashPay payment");
+                    }
                 }
             }
         });

(Adjust if record_dashpay_payment is infallible and returns ().)

🤖 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/identity/network/payments.rs` around
lines 43 - 56, The spawned tokio task currently drops its JoinHandle and ignores
record_dashpay_payment's return, so panics or persistence errors are swallowed;
capture the JoinHandle returned by tokio::spawn and await it (or explicitly
spawn and then tokio::spawn a wrapper that logs the result) so any panic is
observed and logged, and check the Result/return value of
managed.record_dashpay_payment (the call inside the tokio::spawn closure) and
log an error when it returns Err (or log if it returns () only on success),
referencing the existing tokio::spawn block, this.wallet_manager.write().await,
wm.get_wallet_info_mut(&this.wallet_id), and managed.record_dashpay_payment to
locate where to add the await and logging.
🤖 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/wallet/asset_lock/sync/proof.rs`:
- Around line 49-50: The lookups using .and_then(|a|
a.transactions().get(&out_point.txid)).ok_or_else(...) fail when finalized
transactions are evicted; update the proof lookup paths (used by wait_for_proof,
wait_for_chain_lock, upgrade_to_chain_lock_proof, validate_or_upgrade_proof) to
fall back to checking the account's finalized_txids for out_point.txid when
transactions().get(...) returns None, and treat a found finalized_txid as a
valid chain-locked presence (either by creating a minimal placeholder/flag or by
returning a specific ChainLockedFound result) so the proof lifecycle continues
when keep-finalized-transactions is disabled.

---

Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 32-37: The code is taking an exclusive write lock with
self.wallet_manager.try_write() but only performs reads (get_wallet_info and
Self::match_in_collection), so replace the write lock with a shared read lock by
using try_read() instead of try_write(), keep the error mapping behavior and
variable name (wm) consistent, call get_wallet_info(&self.wallet_id) and then
Self::match_in_collection(info, address) while dropping the read guard after use
to avoid holding the lock longer than necessary.

---

Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 131-135: DashpayAccountKey is constructed twice with identical
fields inside the same let (payment_address, tx) = { ... } flow; hoist creating
a single DashpayAccountKey before the inner blocks (using account_index,
from_identity_id.to_buffer(), to_contact_id.to_buffer()) and reuse that variable
in both places where .get(&DashpayAccountKey { ... }) is currently called
(instead call .get(&dashpay_account_key) or similar), ensuring the key is
immutable and in scope for both lookups to remove duplication while preserving
existing error propagation via ?.
- Around line 43-56: The spawned tokio task currently drops its JoinHandle and
ignores record_dashpay_payment's return, so panics or persistence errors are
swallowed; capture the JoinHandle returned by tokio::spawn and await it (or
explicitly spawn and then tokio::spawn a wrapper that logs the result) so any
panic is observed and logged, and check the Result/return value of
managed.record_dashpay_payment (the call inside the tokio::spawn closure) and
log an error when it returns Err (or log if it returns () only on success),
referencing the existing tokio::spawn block, this.wallet_manager.write().await,
wm.get_wallet_info_mut(&this.wallet_id), and managed.record_dashpay_payment to
locate where to add the await and logging.
🪄 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: 061915fc-9e76-4ad1-9a6c-0e89399acc85

📥 Commits

Reviewing files that changed from the base of the PR and between 0fd99fc and aac3aa9.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (12)
  • Cargo.toml
  • packages/rs-platform-wallet/Cargo.toml
  • packages/rs-platform-wallet/src/changeset/core_bridge.rs
  • packages/rs-platform-wallet/src/manager/accessors.rs
  • packages/rs-platform-wallet/src/wallet/asset_lock/build.rs
  • packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs
  • packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
  • packages/rs-platform-wallet/src/wallet/core/broadcast.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWallet.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift
💤 Files with no reviewable changes (2)
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWallet.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift

Comment on lines +49 to 50
.and_then(|a| a.transactions().get(&out_point.txid))
.ok_or_else(|| {
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that proof/recovery logic relies on in-memory transactions() lookups.
rg -n --type=rust -C2 'transactions\(\)\.get\(' \
  packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs \
  packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs

# Check whether these paths have a fallback for finalized/evicted records.
rg -n --type=rust -C2 'finalized_txids|finalized|chain_locked_height|tracked_asset_locks.*proof' \
  packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs \
  packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs

Repository: dashpay/platform

Length of output: 8245


🌐 Web query:

In dashpay/rust-dashcore at commit 428b60d0c93b7842876e568ebb40c2e5455d4277, when keep-finalized-transactionsis disabled, doesManagedAccountTrait::transactions() retain finalized/chainlocked transaction records or evict them?

💡 Result:

Despite extensive searches across GitHub, documentation, issues, changelogs, and related sources for dashpay/rust-dashcore, no explicit information was found about the behavior of keep-finalized-transactions option (when disabled) on ManagedAccountTrait::transactions() at commit 428b60d0c93b7842876e568ebb40c2e5455d4277 regarding retaining or evicting finalized/chainlocked transaction records. The repository provides wallet management via the key-wallet crate [1], with traits like ManagedWalletInfo mentioned in changelogs [2], but no source code snippets, docs, or issues detail this specific configuration or trait method behavior at that commit. The commit itself was not located in search results, and no matching code or descriptions appeared for "keep-finalized-transactions", "ManagedAccountTrait", or related terms in context of transaction eviction/retention. Insufficient evidence to determine if it retains or evicts them.

Citations:


🏁 Script executed:

# Find ManagedAccountTrait definition in the repository
rg -n "trait ManagedAccountTrait|impl.*ManagedAccountTrait" --type=rust -A 10

# Search for keep-finalized-transactions feature usage
rg -n "keep-finalized-transactions|keep_finalized_transactions" --type=rust

# Check if there are any fallback mechanisms or alternative lookups for finalized transactions
rg -n "finalized.*transaction|transaction.*finalized" --type=rust -A 3 -B 3 packages/rs-platform-wallet/src/wallet/asset_lock/sync/

Repository: dashpay/platform

Length of output: 569


🏁 Script executed:

# Read the specific files to understand the keep-finalized-transactions behavior
cat -n packages/rs-platform-wallet/src/manager/accessors.rs | sed -n '545,570p'
cat -n packages/rs-platform-wallet/src/manager/accessors.rs | sed -n '655,670p'
cat -n packages/rs-platform-wallet/src/changeset/core_bridge.rs | sed -n '210,225p'

Repository: dashpay/platform

Length of output: 3245


🏁 Script executed:

# Look for test cases or usage examples for asset-lock proof handling
rg -n "asset.*lock.*proof|wait_for_proof|wait_for_chain_lock" --type=rust packages/rs-platform-wallet/tests packages/rs-platform-wallet/src/bin -A 5 -B 2

# Check if there are any comments about feature-gated behavior in proof.rs itself
cat -n packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs | head -50

Repository: dashpay/platform

Length of output: 2540


🏁 Script executed:

# Check if keep-finalized-transactions is in Cargo.toml and what default state is
rg -n "keep-finalized-transactions|keep_finalized_transactions" --type=toml

# Look for any documentation about feature requirements
fd -e md | xargs rg -l "keep-finalized|asset-lock|proof" | head -5

# Check the actual error message when transactions not found
cat -n packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs | sed -n '49,57p'

Repository: dashpay/platform

Length of output: 1121


🏁 Script executed:

# Read the Cargo.toml comment about keep-finalized-transactions
cat -n packages/rs-platform-wallet/Cargo.toml | sed -n '65,85p'

Repository: dashpay/platform

Length of output: 1002


Asset-lock proof lookup fails in default configuration: finalized transactions are evicted from in-memory store.

When keep-finalized-transactions is disabled (the default), chainlocked transactions are actively evicted from the transactions() map and only their txids are retained in finalized_txids for dedup. Lines 49, 143, 227, and 298 in proof.rs call transactions().get() without a fallback, causing these lookups to fail with "Transaction not found in account" errors. This breaks the proof lifecycle (wait_for_proof, wait_for_chain_lock, upgrade_to_chain_lock_proof, validate_or_upgrade_proof) in production builds unless keep-finalized-transactions is explicitly enabled.

The proof paths need either: (1) a fallback to finalized_txids for chainlocked transactions, (2) integration with the event-driven tx history delivery, or (3) documentation requiring the feature to be enabled for asset-lock operations.

Also applies to: 143-144, 227-228, 298-299

🤖 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/sync/proof.rs` around lines
49 - 50, The lookups using .and_then(|a|
a.transactions().get(&out_point.txid)).ok_or_else(...) fail when finalized
transactions are evicted; update the proof lookup paths (used by wait_for_proof,
wait_for_chain_lock, upgrade_to_chain_lock_proof, validate_or_upgrade_proof) to
fall back to checking the account's finalized_txids for out_point.txid when
transactions().get(...) returns None, and treat a found finalized_txid as a
valid chain-locked presence (either by creating a minimal placeholder/flag or by
returning a specific ChainLockedFound result) so the proof lifecycle continues
when keep-finalized-transactions is disabled.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a real architectural concern but it's pre-existing, not introduced by this PR — out of scope for the dep bump.

The same lookup pattern existed before my change as a.transactions.get(...) (field access). It worked in the nightly because nightly's keep_txs_in_memory toggle behaved identically to upstream's renamed keep-finalized-transactions feature: chainlocked records are evicted from the in-memory map in both. My diff is a mechanical field → accessor rename + comment update; semantics on the eviction path are unchanged.

There's also a softer mitigation than CodeRabbit's report suggests: when an asset-lock tx first arrives (mempool / InBlock), it IS in transactions(). Eviction only happens at the chainlock transition. wait_for_proof polls; it would normally observe the InstantSend or InBlock state while the record is still present and exit before the eviction window. The narrow failure mode is "first lookup happens after a chainlock has already arrived for a tx the caller hasn't seen yet" — possible, but not the common path.

That said, the broader fix (fallback to finalized_txids for "tx is chainlocked, height needed" recovery, or sourcing through the event channel) is worth doing in a dedicated PR with its own test coverage. I'll file a follow-up issue rather than expanding scope here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QuantumExplorer, that's a clear and accurate breakdown — thank you. The pre-existing nature of the pattern, the identical eviction semantics between the old keep_txs_in_memory toggle and the renamed keep-finalized-transactions feature, and the mitigation window (IS/InBlock state observed before the chainlock eviction) all confirm this is a scoping issue rather than a regression introduced here. The narrow race condition is real but not something this dep-bump PR should expand to fix.

Would you like me to open a GitHub follow-up issue to track the finalized_txids fallback work (or event-channel sourcing) with the context from this discussion as a backlink?


✏️ Learnings added
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 3617
File: packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:49-50
Timestamp: 2026-05-07T16:03:14.353Z
Learning: In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs`, the `transactions().get(txid)` lookups in `wait_for_proof`, `wait_for_chain_lock`, `upgrade_to_chain_lock_proof`, and `validate_or_upgrade_proof` can return `None` for chainlocked transactions when `keep-finalized-transactions` is disabled (the default), because chainlocked records are evicted from the in-memory map and only their txids are retained in `finalized_txids`. This is a pre-existing architectural concern (was `a.transactions.get(...)` field access before; renamed to accessor in PR `#3617`). The narrow failure mode is "first lookup happens after chainlock eviction" — not the common path since `wait_for_proof` polls and normally observes InstantSend/InBlock state before eviction. A dedicated follow-up is needed to add a fallback to `finalized_txids` or event-channel sourcing.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Picks up dashpay/rust-dashcore#735 (`feat(key-wallet):
build_and_sign_transaction_with_signer`) — purely additive on the
key-wallet API; no platform-side adaptation required.

iOS-side cleanup that surfaced under the bumped key-wallet-ffi:

* Remove `transactionCount` and `getTransactions()` from Swift
  `ManagedAccount`. The corresponding C symbols
  (`managed_core_account_get_transaction_count` /
  `managed_core_account_get_transactions`) are gated upstream
  behind `#[cfg(feature = "keep-finalized-transactions")]` (off by
  default — production model delivers tx history through the
  platform-wallet event channel). With the feature off the static
  archive doesn't export those symbols, so the SwiftDashSDK link
  step fails. No Swift callers in this repo, so the right move per
  the swift-sdk CLAUDE.md ownership rules is to drop the dead
  bridges rather than force the feature on (which would change
  the production-build memory model).

Verification:
- cargo check --workspace --all-targets: clean
- packages/swift-sdk/build_ios.sh --target sim --profile dev:
  BUILD SUCCEEDED (Rust + Swift, including SwiftExampleApp link)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

✅ DashSDKFFI.xcframework built for this PR.

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "8d49baadd3a74a6889e08b5916dece621468a4a3cae319b50206491ff8d485aa"
)

Xcode manual integration:

  • Download 'DashSDKFFI.xcframework' artifact from the run link above.
  • Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
  • If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

@QuantumExplorer
Copy link
Copy Markdown
Member Author

Reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants