fix(platform-wallet): local-ledger ownership guard (V27-007)#3648
fix(platform-wallet): local-ledger ownership guard (V27-007)#3648lklimek wants to merge 3 commits into
Conversation
The SDK returns post-transition state for every address touched by a
transfer transition — both inputs and outputs. When the source wallet
sends credits to a foreign address (e.g. bank's primary receive address),
the response includes the bank's post-credit balance. Without an ownership
check, `transfer` wrote that foreign balance into the source wallet's local
ledger via `set_address_credit_balance`, corrupting `total_credits()`.
Symptom chain (PA-004b / PA-009c teardown, QA-010):
1. Test wallet trims residual credits to bank's primary address.
2. `transfer` writes bank's balance into the source wallet's ledger.
3. `total_credits()` is inflated; dust-gate passes; sweep begins.
4. Sweep selects bank's address (now in the source ledger), tries to
sign, fails ("No private key for address P2pkh(...)").
5. Failure cascade in teardown.
Fix: guard `set_address_credit_balance` with `contains_platform_address`
in `transfer`, mirroring the identical guard already present in
`fund_from_asset_lock`. The recipient wallet's syncer observes inbound
credits on its own addresses; the source wallet must not.
Also applied the same guard defensively to `withdrawal` (no foreign
platform output addresses appear there today, but consistency with the
local-ledger ownership invariant is cleaner than a latent risk).
Carved out of #3549 (source commit 16636f0) as a
standalone production fix on v3.1-dev. No e2e tests, no dependency
bumps included.
References: V27-007, QA-010
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPlatform wallet transfer and withdrawal methods now enforce an ownership invariant: only P2PKH addresses owned by the wallet account are written to the local ledger. Foreign addresses are silently skipped during balance updates and ledger mutations to prevent accounting corruption. ChangesPlatform Address Ownership Invariant
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Review complete (commit a9eae10) |
QuantumExplorer
left a comment
There was a problem hiding this comment.
We haven't been using the platform_payment_managed_account, and have plans to get rid of it entirely. Instead we use the thing in platform wallet.
|
This PR fixes a real production ledger-corruption bug (V27-007 / Found-024) on the platform-wallet spend path. What goes wrong without it: After a platform-address transfer/withdrawal is broadcast, The fix: a one-line ownership guard — Why production needs it / what calls it: this is not test-only code. 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, surgical correctness fix that adds an ownership guard before writing SDK-returned post-transition balances into the local ledger. Logic is correct and consistent with the existing precondition in fund_from_asset_lock. No blocking issues. Main gap is the absence of a crate-local regression test; secondary nits around a now-suspicious unwrap_or(0) fallback, growing duplication across three sibling files, and the silent skip having no observability.
Reviewed commit: a9eae10
🟡 1 suggestion(s) | 💬 3 nitpick(s)
2 additional findings
💬 nitpick: `unwrap_or(0)` for `address_index` is now a true bug indicator, not a safe default
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 139-149)
With the new guard at line 125, this branch is only reachable for addresses the account already considers owned. The subsequent find_map over account.addresses.addresses performs a PlatformP2PKHAddress::from_address round-trip on each stored info.address. If contains_platform_address and that round-trip disagree on what "owned" means (e.g. one consults a hashed-address set, the other reconstructs from an address string), the loop falls back to address_index = 0 and pushes a PlatformAddressBalanceEntry that overwrites the persister's row for derivation index 0 with another address's balance — a worse outcome than the bug being fixed. Replace the silent fallback with either an explicit skip + tracing::warn!, or a returned error. The identical pattern at withdrawal.rs:159 needs the same treatment. Pre-existing, but the new guard turns this branch from "expected when address is foreign" into "impossible unless an invariant is broken," so it should be treated as such.
💬 nitpick: Post-broadcast ledger-update loop now duplicated across three files
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 114-157)
After this PR, fund_from_asset_lock.rs (lines 114-148), transfer.rs (lines 114-157), and withdrawal.rs (lines 126-167) each carry a near-identical loop: filter to PlatformAddress::P2pkh, guard via contains_platform_address, map maybe_info to AddressFunds, call set_address_credit_balance, reverse-lookup the derivation index, and push a PlatformAddressBalanceEntry. Any future invariant (nonce write-back, sequence-number guards, additional ownership checks) must be applied in all three places or the consistency this PR establishes will erode. Consider extracting an &mut ManagedPlatformAccount-borrowing helper (e.g. apply_post_transition_funds(account, address_infos, key_source.as_ref(), &mut cs, wallet_id, account_index)) so the ownership invariant lives in exactly one place. Follow-up, not 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/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 119-127: No crate-local regression test pins the foreign-address skip
The entire correctness fix for QA-010 is enforced solely by control flow in `transfer()` and `withdrawal()`, yet `rs-platform-wallet` adds no unit or integration test exercising a response that mixes owned input addresses with a foreign output address. The PR description defers validation to the parent #3549 e2e suite, but this protection should live with the code it guards: a future refactor of the post-broadcast ledger-update loop (moving the guard, factoring the body out, replacing `contains_platform_address`) could silently reintroduce the exact ledger-corruption bug while compiling cleanly and passing unrelated coverage. Add a focused test that feeds an `address_infos` map containing one owned and one foreign `PlatformAddress::P2pkh`, then asserts (a) `account.address_credit_balance` only reflects the owned entry, (b) the returned `PlatformAddressChangeSet` excludes the foreign entry, and (c) `total_credits()` does not absorb the foreign balance. The same fixture covers the symmetric guard in `withdrawal.rs`, which has no e2e coverage today.
|
✅ 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: "27752a33f43106c5b66fb991df02d515fe289f677909fc2ce8a418c6707778de"
)Xcode manual integration:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a contains_platform_address ownership guard around the post-broadcast ledger update in transfer() and withdraw(), mirroring the implicit guard already provided by the pre-broadcast check in fund_from_asset_lock(). The fix is correct, minimal, and well-documented via a method-level doc block on transfer. Verification of the current head (7228d87) confirms all four prior findings are STILL VALID: no crate-local regression test was added, the unwrap_or(0) address-index fallback is unchanged and now more consequential under the new invariant, the post-broadcast loop is now near-identically duplicated across fund_from_asset_lock.rs, transfer.rs, and withdrawal.rs, and foreign-address skips remain silent. No new defects introduced in the latest delta beyond these carried-forward concerns.
Note: Inline posting failed (command failed (1): gh api /repos/dashpay/platform/pulls/3648/reviews --method POST --input -
STDOUT:
{"message":"Unprocessable Entity","errors":["Variable $threads of type [DraftPullRequestReviewThread] was provided invalid value for 0.commitId (Field is not defined on DraftPullRequestReviewThread)), so I posted the same verified findings as a top-level review body.
🟡 2 suggestion(s) | 💬 2 nitpick(s)
4 additional finding(s)
suggestion: Ownership-guard regression is not pinned by a crate-local test
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (line 119)
The V27-007 / QA-010 correctness fix is enforced solely by the new if !account.contains_platform_address(&p2pkh) { continue; } branch in transfer.rs:125 and the mirrored guard in withdrawal.rs:135. packages/rs-platform-wallet ships no unit or integration test that exercises an address_infos payload mixing an owned input with a foreign output and asserts (a) the foreign address is not written into account.address_balances, (b) the returned PlatformAddressChangeSet contains no entry for the foreign address, and (c) the persister is not called with foreign rows. The PR defers validation to the parent #3549 e2e suite, but because this PR is carved out as a standalone backport on v3.1-dev, the invariant has no in-repo guard — a future refactor of the post-broadcast loop (see the duplication finding) could silently delete or invert the branch with no CI signal here. A small focused test constructing a ManagedPlatformAccount with one owned address and one foreign PlatformP2PKHAddress in address_infos would pin the invariant where it lives.
suggestion: `unwrap_or(0)` can silently mis-key a persisted row for an owned address
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (line 139)
After the new guard, this lookup only runs when account.contains_platform_address(&p2pkh) is true. That predicate (in key-wallet/.../managed_platform_account.rs) returns true if the address appears in EITHER address_balances OR addresses. The subsequent find_map only iterates account.addresses.addresses (the indexed pool), so an address present in address_balances but missing from the pool's indexed map passes the guard and falls back to address_index = 0. That fabricated index is then propagated through PlatformAddressBalanceEntry and self.persister.store(...), overwriting the persisted row at derivation index 0 with another address's balance/nonce. The identical pattern is present in withdrawal.rs:149-159 and fund_from_asset_lock.rs:130-140. Before the guard this was masked by broader corruption; under the new invariant it is a concrete failure mode. The correct response is to surface an error (this is now an invariant violation, not a normal case) rather than coerce to 0.
let address_index = account
.addresses
.addresses
.iter()
.find_map(|(&idx, info)| {
PlatformP2PKHAddress::from_address(&info.address)
.ok()
.filter(|found| *found == p2pkh)
.map(|_| idx)
})
.ok_or_else(|| {
PlatformWalletError::AddressSync(format!(
"Owned platform address {} missing derivation index in account {}",
p2pkh, account_index
))
})?;
nitpick: Post-broadcast ledger-update loop is duplicated across three files
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (line 114)
fund_from_asset_lock.rs:114-148, transfer.rs:114-157, and withdrawal.rs:126-167 now carry near-identical loops: destructure PlatformAddress::P2pkh, build PlatformP2PKHAddress, apply the ownership guard, call set_address_credit_balance, reverse-scan the pool for an address_index, and push a PlatformAddressBalanceEntry. The bug this PR fixes was caused by exactly this kind of drift — the guard existed in fund_from_asset_lock and was missing from transfer/withdrawal. The PR description notes the withdrawal guard is added 'defensively for consistency', which is itself a symptom of the drift risk. Extracting a single helper on ManagedPlatformAccount (or in platform_addresses) that encapsulates the guard, index resolution, and changeset construction would make the next iteration of the invariant (e.g., observability, index-resolution error handling, nonce persistence) impossible to forget at any one site.
nitpick: Foreign-address skip has no observability
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (line 119)
Both guards (transfer.rs:125 and withdrawal.rs:135) drop foreign addresses with a bare continue and no log line. The original QA-010 bug surfaced as No private key for address P2pkh(...) deep in the sweep path during teardown — diagnosis took hours because the actual misbehavior was silent in the ledger-update layer. If a future regression elsewhere in the stack (e.g., gap-limit / pool-derivation race that hasn't yet recorded a derived address in account.addresses) misclassifies an owned address as foreign, the symptom is again balance just doesn't update with zero signal here. A single tracing::debug!(wallet_id, account_index, %p2pkh, "skipping post-transition balance update for non-owned platform address") inside each guard costs nothing at production log levels and makes the next analogous bug observable from logs alone. This is especially valuable in withdrawal.rs where the guard is documented as 'never expected to fire' — if it ever does, that's a signal worth surfacing.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:119-127: Ownership-guard regression is not pinned by a crate-local test
The V27-007 / QA-010 correctness fix is enforced solely by the new `if !account.contains_platform_address(&p2pkh) { continue; }` branch in `transfer.rs:125` and the mirrored guard in `withdrawal.rs:135`. `packages/rs-platform-wallet` ships no unit or integration test that exercises an `address_infos` payload mixing an owned input with a foreign output and asserts (a) the foreign address is not written into `account.address_balances`, (b) the returned `PlatformAddressChangeSet` contains no entry for the foreign address, and (c) the persister is not called with foreign rows. The PR defers validation to the parent #3549 e2e suite, but because this PR is carved out as a standalone backport on `v3.1-dev`, the invariant has no in-repo guard — a future refactor of the post-broadcast loop (see the duplication finding) could silently delete or invert the branch with no CI signal here. A small focused test constructing a `ManagedPlatformAccount` with one owned address and one foreign `PlatformP2PKHAddress` in `address_infos` would pin the invariant where it lives.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:139-149: `unwrap_or(0)` can silently mis-key a persisted row for an owned address
After the new guard, this lookup only runs when `account.contains_platform_address(&p2pkh)` is true. That predicate (in `key-wallet/.../managed_platform_account.rs`) returns true if the address appears in EITHER `address_balances` OR `addresses`. The subsequent `find_map` only iterates `account.addresses.addresses` (the indexed pool), so an address present in `address_balances` but missing from the pool's indexed map passes the guard and falls back to `address_index = 0`. That fabricated index is then propagated through `PlatformAddressBalanceEntry` and `self.persister.store(...)`, overwriting the persisted row at derivation index 0 with another address's balance/nonce. The identical pattern is present in `withdrawal.rs:149-159` and `fund_from_asset_lock.rs:130-140`. Before the guard this was masked by broader corruption; under the new invariant it is a concrete failure mode. The correct response is to surface an error (this is now an invariant violation, not a normal case) rather than coerce to 0.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:114-157: Post-broadcast ledger-update loop is duplicated across three files
`fund_from_asset_lock.rs:114-148`, `transfer.rs:114-157`, and `withdrawal.rs:126-167` now carry near-identical loops: destructure `PlatformAddress::P2pkh`, build `PlatformP2PKHAddress`, apply the ownership guard, call `set_address_credit_balance`, reverse-scan the pool for an `address_index`, and push a `PlatformAddressBalanceEntry`. The bug this PR fixes was caused by exactly this kind of drift — the guard existed in `fund_from_asset_lock` and was missing from `transfer`/`withdrawal`. The PR description notes the `withdrawal` guard is added 'defensively for consistency', which is itself a symptom of the drift risk. Extracting a single helper on `ManagedPlatformAccount` (or in `platform_addresses`) that encapsulates the guard, index resolution, and changeset construction would make the next iteration of the invariant (e.g., observability, index-resolution error handling, nonce persistence) impossible to forget at any one site.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:119-127: Foreign-address skip has no observability
Both guards (`transfer.rs:125` and `withdrawal.rs:135`) drop foreign addresses with a bare `continue` and no log line. The original QA-010 bug surfaced as `No private key for address P2pkh(...)` deep in the sweep path during teardown — diagnosis took hours because the actual misbehavior was silent in the ledger-update layer. If a future regression elsewhere in the stack (e.g., gap-limit / pool-derivation race that hasn't yet recorded a derived address in `account.addresses`) misclassifies an owned address as foreign, the symptom is again `balance just doesn't update` with zero signal here. A single `tracing::debug!(wallet_id, account_index, %p2pkh, "skipping post-transition balance update for non-owned platform address")` inside each guard costs nothing at production log levels and makes the next analogous bug observable from logs alone. This is especially valuable in `withdrawal.rs` where the guard is documented as 'never expected to fire' — if it ever does, that's a signal worth surfacing.
Issue being fixed or feature implemented
The SDK returns post-transition state for every address touched by a transfer transition — both inputs and outputs. When the source wallet sends credits to a foreign address (e.g. a bank's primary receive address), the response includes that foreign address's post-credit balance. Without an ownership check,
transferwrote the foreign balance into the source wallet's local ledger viaset_address_credit_balance, corruptingtotal_credits().Symptom chain (PA-004b / PA-009c teardown, QA-010):
transferwrites the bank's balance into the source wallet's ledger.total_credits()is inflated; the dust-gate passes; a sweep begins.No private key for address P2pkh(...)).This is a standalone correctness fix carved out of #3549 (source commit
16636f01c0). It is not present in the #3554 stack base — it is introduced by #3549 only and is self-contained.What was done?
transfer: guardset_address_credit_balancewithaccount.contains_platform_address(&p2pkh), mirroring the identical guard already present infund_from_asset_lock. Foreign output addresses are silently skipped; the recipient wallet's own syncer is responsible for observing its inbound credits.withdrawal: applied the same guard defensively. Withdrawals carry no foreign platform output addresses today, so the guard is never expected to fire, but it keeps the local-ledger ownership invariant consistent withtransfer.# Local-ledger ownership invariant (V27-007 / QA-010)doc section ontransferdocumenting the invariant.Net change: +27 LOC across 2 files, no e2e tests, no
rust-dashcorebump, noCargo.lockchurn.How Has This Been Tested?
cargo build -p platform-walletonorigin/v3.1-dev— clean (rust-dashcore stays at base rev53130869;contains_platform_addressis available onkey-wallet@53130869, no dependency on the excluded cluster-A bump).cargo clippy -p platform-wallet— clean, zero warnings on the changed crate.Breaking Changes
None. Internal post-broadcast ledger-update behavior only; public API is unchanged.
Checklist:
For repository code-owners and collaborators only
Source: extracted from #3549 (cluster F — local-ledger ownership guard, source commit
16636f01c0).🤖 Generated with Claude Code
Summary by CodeRabbit