feat(platform-wallet): wallet masternode list with DML-derived status in iOS#4116
Conversation
Aggregates the wallet's masternodes in Rust from the provider special transactions that rust-dashcore #876 retains past finalization (grouped by proTxHash, latest-update-wins service address, evonode from provider_type, per-type numbering), statuses each against the SPV deterministic masternode list (Active / Inactive when PoSe-banned / Retired when absent; Unknown preserved rather than persisted), and stages provider special txs during wallet restore so the aggregation survives app restarts. Swift persists PersistentMasternode rows and the Identities tab gains an Identities/Masternodes toggle (only when masternodes exist) with a default-off Show-retired filter; provider key account address rows show "used N times on Evonode/Masternode N · ip"; the key-pool "Absent" jargon label is renamed Additional. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
✨ 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 |
|
⛔ Blockers found — Sonnet deferred (commit d93dda1) |
…ippy type alias Adds the Storage Explorer list/detail/index views the coverage check requires for the new model, and factors the provider-masternode accessor's return tuple into the documented ProviderMasternodeTxs alias to satisfy clippy::type_complexity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…imable balance
Tapping a masternode/evonode opens a detail page showing: full key
material (owner/voting addresses + hashes, operator BLS key, evonode
platform node id, payout address — extracted in Rust with
latest-update-wins), key ownership resolved by joining the Rust-encoded
base58 key addresses against the wallet's persisted address rows
("ProviderOwnerKeys #14" / "not in this wallet"), Core payout history
(persisted TXOs paying the payout address), and for evonodes the
claimable balance — the credit balance of the Platform identity whose
id is the display-order proTxHash — with refresh.
Also fixes a pre-existing SDK wrapper bug: identity fetch-balance
returns a decimal C string that getBalance misread as a binary u64.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…igner gated) Adds the full claim path for a masternode identity's credits: a Claim button gated on claimable balance and owner-key-in-wallet, a confirmation sheet (amount with fee headroom, registered payout address as the fixed destination, signing key shown), a Swift wrapper, and the platform_wallet_manager_masternode_withdraw FFI with a unit-tested guard that selects the identity's matching OWNER ECDSA_HASH160 key and refuses otherwise. The actual sign+broadcast is deliberately gated behind a distinct error until the owner-key signer is verified against testnet — the signature encoding must not be guessed on a money path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rship Operator keys and platform node ids never appear as on-chain addresses, so ownership is resolved transitively: derive the wallet's operator pubkeys (xpub-only) and platform node ids (resident seed) over the pre-derive window and compare against each masternode's payload values. Account address rows for those key kinds also gain the "used on Evonode N" subtitle via the persisted pseudo-address join. Watch-only platform-node derivation (needs the mnemonic resolver) is a documented follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Verified exact HEAD d93dda1 against the full ad657c8..d93dda1 range. All eight Codex findings reproduce in the new masternode list, detail, ownership, and claim flows; they cause incorrect ownership, stale or invalid status, misclassification, orphaned persistence, and malformed IPv6 endpoints. These in-scope defects should be fixed before merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 8 blocking
🤖 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/manager/accessors.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:907-919: Platform-node ownership can never resolve for managed wallets
This passes no resolved master while deriving hardened Ed25519 platform-node keys and stops on the first error. `register_wallet` unconditionally downgrades the wallet to external-signable before constructing its `PlatformWallet`, and restored wallets are also external-signable, so neither path has the resident private root required by `derive_provider_key_at_index`. Registration persists a pre-derived platform-node key batch, but this accessor does not consume it or invoke the mnemonic resolver; consequently `platform_index` remains empty and every platform-node key is reported as outside the wallet.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/MasternodeSync.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/MasternodeSync.swift:135-148: Key ownership lookup is not scoped to the masternode wallet
The helper fetches the globally matching address without checking `row.account.wallet.walletId`. Provider roles can be split across wallets, allowing the same ProRegTx aggregate to appear for wallet A through its owner key and wallet B through its voting key. Wallet B then resolves wallet A's owner row and records its account type and index as B's ownership; the detail view trusts that flag to enable Claim and passes wallet B together with wallet A's index. The current Rust owner-key guard fails closed, but the displayed ownership and claim metadata are still wrong. Pass the aggregate's wallet ID into this helper and require the fetched account to belong to it.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:319-331: An intermediate DML snapshot is treated as fully synchronized
`latest_masternode_list()` only returns the last list stored in the engine; it does not indicate synchronization readiness. The pinned dash-spv implementation restores that list during startup while explicitly setting masternode progress to `WaitingForConnections`, so this accessor can expose a stale checkpoint or intermediate list despite promising `None` until synchronization completes. The FFI treats every missing proTxHash in that snapshot as absent and therefore Retired, producing incorrect persisted status for nodes registered or retired after the snapshot. Gate the result on masternode sync state and list height relative to the current header tip.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift:233-245: Masternodes are never refreshed when Core or DML synchronization completes
The only repository call to `MasternodeSync.refresh` is this `.onAppear` handler. Pull-to-refresh runs only Platform balance synchronization, and the view has no completion observer for provider-record restoration, Core rescan, or DML progress. If the tab appears before those operations finish, its masternode rows remain empty, Unknown, or stale after synchronization because ordinary progress updates do not retrigger `.onAppear`; the user must leave and re-enter the screen. Refresh on the relevant sync-completion changes and include masternodes in the explicit refresh action.
In `packages/rs-platform-wallet-ffi/src/core_wallet_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:1247-1248: Unconfirmed registrations are deterministically labeled Retired
The accessor includes unconfirmed provider records with height zero, but this loop assigns status solely from current DML membership. A newly broadcast ProRegTx cannot yet appear in the confirmed DML, so an available and fully current list still returns `Absent`, which maps directly to Retired. The Swift view hides Retired rows by default, making a pending registration disappear immediately. Treat a registration with `registration_height == 0` as Unknown or Pending until it confirms instead of applying the retirement rule.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:1185-1196: Update-only evonodes are classified as regular masternodes
Update-only aggregation is a supported case when the wallet retained a ProUpServTx without its ProRegTx. Such an aggregate starts with `is_evonode == false`, and this branch updates its service and platform-node fields while ignoring `ProviderUpdateServicePayload.mn_type`. BasicBLS ProUpServ payloads explicitly identify HighPerformance nodes, so an update-only evonode carries sufficient type information but is rendered and numbered as a regular masternode. Set the aggregate's type from `mn_type` when processing the update.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:924-930: IPv6 service endpoints are emitted without port brackets
Formatting an IPv6 address and port independently emits an ambiguous value such as `2001:db8::1:9999` instead of `[2001:db8::1]:9999`. ProReg endpoints already use `SocketAddr::to_string()`, so the same native IPv6 endpoint also changes representation when a ProUpServTx supersedes it. Constructing a `SocketAddr` preserves valid and consistent IPv4 and IPv6 syntax.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentMasternode.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentMasternode.swift:20-24: Wallet deletion leaves persistent masternode metadata behind
This model stores only a raw wallet ID and has no relationship to `PersistentWallet`, so deleting the wallet cannot cascade these rows. `PlatformWalletPersistenceHandler.deleteWalletData` explicitly removes other raw-wallet-ID tables but never fetches or deletes `PersistentMasternode`. Deleting and reimporting the same deterministic wallet ID can therefore resurrect stale ownership and status metadata, especially because `MasternodeSync` intentionally does not prune persisted rows when the fresh aggregation is empty. Delete these rows explicitly during the wallet wipe or add an appropriate cascade relationship.
| match platform_wallet.derive_provider_key_at_index( | ||
| ProviderKeyKind::PlatformNode, | ||
| index, | ||
| None, | ||
| false, | ||
| ) { | ||
| Ok(key) => { | ||
| if let Some(node_id) = key.node_id { | ||
| platform_index.insert(node_id, index); | ||
| } | ||
| } | ||
| // No platform account, or watch-only (needs the seed). Stop. | ||
| Err(_) => break, |
There was a problem hiding this comment.
🔴 Blocking: Platform-node ownership can never resolve for managed wallets
This passes no resolved master while deriving hardened Ed25519 platform-node keys and stops on the first error. register_wallet unconditionally downgrades the wallet to external-signable before constructing its PlatformWallet, and restored wallets are also external-signable, so neither path has the resident private root required by derive_provider_key_at_index. Registration persists a pre-derived platform-node key batch, but this accessor does not consume it or invoke the mnemonic resolver; consequently platform_index remains empty and every platform-node key is reported as outside the wallet.
source: ['codex']
| private static func ownership( | ||
| for address: String?, | ||
| modelContext: ModelContext | ||
| ) -> (inWallet: Bool, accountType: UInt8, index: UInt32) { | ||
| guard let address, !address.isEmpty else { return (false, 0, 0) } | ||
| let descriptor = FetchDescriptor<PersistentCoreAddress>( | ||
| predicate: #Predicate { $0.address == address } | ||
| ) | ||
| guard let row = try? modelContext.fetch(descriptor).first, | ||
| let account = row.account | ||
| else { | ||
| return (false, 0, 0) | ||
| } | ||
| return (true, UInt8(truncatingIfNeeded: account.accountType), row.addressIndex) |
There was a problem hiding this comment.
🔴 Blocking: Key ownership lookup is not scoped to the masternode wallet
The helper fetches the globally matching address without checking row.account.wallet.walletId. Provider roles can be split across wallets, allowing the same ProRegTx aggregate to appear for wallet A through its owner key and wallet B through its voting key. Wallet B then resolves wallet A's owner row and records its account type and index as B's ownership; the detail view trusts that flag to enable Claim and passes wallet B together with wallet A's index. The current Rust owner-key guard fails closed, but the displayed ownership and claim metadata are still wrong. Pass the aggregate's wallet ID into this helper and require the fetched account to belong to it.
source: ['codex']
| let engine_guard = engine.blocking_read(); | ||
| let list = engine_guard.latest_masternode_list()?; | ||
|
|
||
| let mut map = std::collections::HashMap::with_capacity(list.masternodes.len()); | ||
| for qualified in list.masternodes.values() { | ||
| let entry = &qualified.masternode_list_entry; | ||
| // `pro_reg_tx_hash` is internal order (the DML map itself keys | ||
| // by the reversed/display form, so read it off the entry). | ||
| let mut pro_tx = [0u8; 32]; | ||
| pro_tx.copy_from_slice(entry.pro_reg_tx_hash.as_ref()); | ||
| map.insert(pro_tx, entry.is_valid); | ||
| } | ||
| Some(map) |
There was a problem hiding this comment.
🔴 Blocking: An intermediate DML snapshot is treated as fully synchronized
latest_masternode_list() only returns the last list stored in the engine; it does not indicate synchronization readiness. The pinned dash-spv implementation restores that list during startup while explicitly setting masternode progress to WaitingForConnections, so this accessor can expose a stale checkpoint or intermediate list despite promising None until synchronization completes. The FFI treats every missing proTxHash in that snapshot as absent and therefore Retired, producing incorrect persisted status for nodes registered or retired after the snapshot. Gate the result on masternode sync state and list height relative to the current header tip.
source: ['codex']
| .refreshable { | ||
| await platformBalanceSyncService.performSync() | ||
| } | ||
| // Refresh the persisted masternode aggregation from Rust when the | ||
| // tab appears. Pure persist step — Rust owns the aggregation | ||
| // (`MasternodeSync.refresh`). | ||
| .onAppear { | ||
| MasternodeSync.refresh( | ||
| walletManager: walletManager, | ||
| walletIds: walletIdsOnNetwork, | ||
| modelContext: modelContext | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Masternodes are never refreshed when Core or DML synchronization completes
The only repository call to MasternodeSync.refresh is this .onAppear handler. Pull-to-refresh runs only Platform balance synchronization, and the view has no completion observer for provider-record restoration, Core rescan, or DML progress. If the tab appears before those operations finish, its masternode rows remain empty, Unknown, or stale after synchronization because ordinary progress updates do not retrigger .onAppear; the user must leave and re-enter the screen. Refresh on the relevant sync-completion changes and include masternodes in the explicit refresh action.
source: ['codex']
| for agg in result.iter_mut() { | ||
| agg.status = MasternodeStatus::from_membership(list_lookup(&agg.pro_tx_hash)); |
There was a problem hiding this comment.
🔴 Blocking: Unconfirmed registrations are deterministically labeled Retired
The accessor includes unconfirmed provider records with height zero, but this loop assigns status solely from current DML membership. A newly broadcast ProRegTx cannot yet appear in the confirmed DML, so an available and fully current list still returns Absent, which maps directly to Retired. The Swift view hides Retired rows by default, making a pending registration disappear immediately. Treat a registration with registration_height == 0 as Unknown or Pending until it confirms instead of applying the retirement rule.
source: ['codex']
| Some(TransactionPayload::ProviderUpdateServicePayloadType(p)) => { | ||
| if agg.service_address.is_none() || height >= agg.service_height { | ||
| agg.service_address = Some(provider_ip_port(p.ip_address, p.port)); | ||
| agg.service_height = height; | ||
| } | ||
| // ProUpServ's `platform_node_id` is a raw `Option<[u8; 20]>`. | ||
| if let Some(node_id) = p.platform_node_id { | ||
| if agg.platform_node_id.is_none() || height >= agg.platform_node_height { | ||
| agg.platform_node_id = Some(node_id); | ||
| agg.platform_node_height = height; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Update-only evonodes are classified as regular masternodes
Update-only aggregation is a supported case when the wallet retained a ProUpServTx without its ProRegTx. Such an aggregate starts with is_evonode == false, and this branch updates its service and platform-node fields while ignoring ProviderUpdateServicePayload.mn_type. BasicBLS ProUpServ payloads explicitly identify HighPerformance nodes, so an update-only evonode carries sufficient type information but is rendered and numbered as a regular masternode. Set the aggregate's type from mn_type when processing the update.
source: ['codex']
| public final class PersistentMasternode { | ||
| #Unique<PersistentMasternode>([\.walletId, \.proTxHash]) | ||
|
|
||
| /// Owning wallet id (32 bytes). Network is derived via the wallet. | ||
| public var walletId: Data |
There was a problem hiding this comment.
🔴 Blocking: Wallet deletion leaves persistent masternode metadata behind
This model stores only a raw wallet ID and has no relationship to PersistentWallet, so deleting the wallet cannot cascade these rows. PlatformWalletPersistenceHandler.deleteWalletData explicitly removes other raw-wallet-ID tables but never fetches or deletes PersistentMasternode. Deleting and reimporting the same deterministic wallet ID can therefore resurrect stale ownership and status metadata, especially because MasternodeSync intentionally does not prune persisted rows when the fresh aggregation is empty. Delete these rows explicitly during the wallet wipe or add an appropriate cascade relationship.
source: ['codex']
| fn provider_ip_port(ip_address: u128, port: u16) -> String { | ||
| let v6 = std::net::Ipv6Addr::from(ip_address.to_le_bytes()); | ||
| let ip = v6 | ||
| .to_ipv4_mapped() | ||
| .map(std::net::IpAddr::V4) | ||
| .unwrap_or(std::net::IpAddr::V6(v6)); | ||
| format!("{}:{}", ip, port) |
There was a problem hiding this comment.
🔴 Blocking: IPv6 service endpoints are emitted without port brackets
Formatting an IPv6 address and port independently emits an ambiguous value such as 2001:db8::1:9999 instead of [2001:db8::1]:9999. ProReg endpoints already use SocketAddr::to_string(), so the same native IPv6 endpoint also changes representation when a ProUpServTx supersedes it. Constructing a SocketAddr preserves valid and consistent IPv4 and IPv6 syntax.
| fn provider_ip_port(ip_address: u128, port: u16) -> String { | |
| let v6 = std::net::Ipv6Addr::from(ip_address.to_le_bytes()); | |
| let ip = v6 | |
| .to_ipv4_mapped() | |
| .map(std::net::IpAddr::V4) | |
| .unwrap_or(std::net::IpAddr::V6(v6)); | |
| format!("{}:{}", ip, port) | |
| std::net::SocketAddr::new(ip, port).to_string() |
source: ['codex']
…nd-example-app Conflict: Cargo.lock (base bumped grovedb 5.0.1 + rust-dashcore for the masternode-list work) — took base's resolution and re-resolved for the branch-only workspace members. Merge adaptation: WalletRestoreEntryFFI gained provider_special_txs(+count) (#4112/#4116); the JNI load path initializes them null/0 — the documented "wallet has no provider special txs" value — since Android has no provider-special-tx persistence yet (masternode-list parity tracked as a follow-up). Also lands the Sdk teardown behavioral test the review asked for: SdkLifecycleTest (4 tests over a native-free Sdk.forLifecycleTest seam — handle 0 skips the Cleaner's native destroy) pins close-awaits-lease, post-close fail-fast, cancellation-cannot-strand-cleanup, and the AutoCloseable.close() fence. The nested-C-export panic-containment finding is intentionally NOT addressed here: shumkov confirmed the mechanism and scoped the fix into follow-up #4121 (cfg-gated panic-catch inert under the iOS panic=abort profiles + PlatformWalletError surfacing); an ABI-wide change from this PR would cut across that. Verified: cargo fmt, platform-wallet-ffi lib tests (155), JNI check (shielded), workspace clippy --all-features -D warnings, gradle :sdk+:app tests (SdkLifecycleTest 4/4). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
A wallet holding provider keys had no way to see its masternodes: the Provider Owner Keys account screen showed a jargon "Absent Addresses" header, address rows said only "#0 · used" with no hint of what used them, and there was no masternode list anywhere. Builds on #4112's payload surfacing and rust-dashcore #876's provider-record retention.
What was done?
Rust — masternode aggregation + DML status (
rs-platform-wallet,rs-platform-wallet-ffi)aggregate_masternodesgroups the wallet's retained provider special txs (ProRegTx/ProUpServTx/ProUpRegTx/ProUpRevTx) by proTxHash: latest-update-wins service address, evonode flag from ProRegTxmasternode_type, owner/voting hashes + collateral, revocation data, stable registration-order sort, and per-type numbering (type_index) so evonodes and masternodes number independently.SpvRuntime::masternode_validity_snapshot_blocking: Active (in list, valid), Inactive (in list, PoSe-banned), Retired (absent — revoked/spent/expired), Unknown (DML not synced yet; the persist layer keeps the prior value instead of clobbering).platform_wallet_manager_list_masternodes/free(mirrorsget_account_balances), with Rust-side base58 owner/voting address encoding so Swift never hashes keys.ProviderSpecialTxRestoreEntryFFI, mirroring the asset-lock staging) onto every provider-key account, where build: operations in configure.sh script were in a wrong order #876's retention keeps them resident — the masternode list survives app restarts without a rescan.Swift SDK (pure persist/bridge): new
PersistentMasternodemodel ((walletId, proTxHash)unique) +PlatformWalletManager.masternodes(for:)wrapper.SwiftExampleApp
MasternodeSyncupserts/deletes rows from the aggregation; an empty aggregation never prunes (indistinguishable from "not rehydrated yet").Masternode detail page (second commit block): tapping a row opens a detail view with full key material (owner/voting addresses+hashes, operator BLS key, evonode platform node id, payout address), key ownership resolved via the persisted-address join ("ProviderOwnerKeys #14" / "not in this wallet"), Core payout history (TXOs paying the payout address), and for evonodes the claimable balance — the credit balance of the Platform identity whose id is the display-order proTxHash — with a Refresh affordance. Also fixes a pre-existing
SDK.swiftgetBalancebug (the FFI returns a decimal string; the wrapper misread it as binary u64).Claim flow (owner key, signer gated): a Claim button (enabled when claimable > 0 and the owner key is in this wallet) opens a confirmation sheet — amount with ~0.005 DASH fee headroom and live credits echo, the registered payout address as the immutable destination, and the signing key ("ProviderOwnerKeys #14"). The
platform_wallet_manager_masternode_withdrawFFI enforces the owner-key contract and carries a unit-tested guard selecting the identity's matching OWNERECDSA_HASH160key. The final sign+broadcast is deliberately gated behind a distinct error ("pending verified signer") until the owner-key signer's DPP signature encoding is verified against a testnet masternode — the whole button→sheet→FFI→guard chain is exercised without risking a malformed money transition.How Has This Been Tested?
cargo test -p platform-wallet-ffi(152 tests) green, including new suites: aggregation (registration / update-only / grouping / revocation via a synthetic ProUpRevTx), DML status matrix (all fourListMembershipoutcomes), and per-type numbering (evonode + regular each getting index 1).cargo clippyclean.build_ios.sh --target sim(warnings-as-errors) green.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code