Skip to content

fix(platform-wallet): local-ledger ownership guard (V27-007) #3648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lklimek wants to merge 2 commits into
base: v3.1-dev
Choose a base branch
from
+27 −0
Open

fix(platform-wallet): local-ledger ownership guard (V27-007) #3648

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cac51fa
fix(rs-platform-wallet): local-ledger ownership guard (V27-007)
lklimek May 15, 2026
a9eae10
Merge branch 'v3.1-dev' into fix/platform-wallet-ledger-ownership-gua…
lklimek May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ impl PlatformAddressWallet {
/// material — callers supply a seed-backed, hardware, or
/// FFI-trampoline signer per their environment (iOS routes through
/// `KeychainSigner` via `VTableSigner`).
///
/// # Local-ledger ownership invariant (V27-007 / QA-010)
///
/// The SDK returns post-transition states for **all** addresses touched by
/// the transition, including foreign output addresses the caller does not
/// own. Only addresses that belong to this wallet's account are written into
/// the local ledger; foreign addresses are silently skipped. The recipient
/// wallet's syncer is responsible for observing inbound credits on its own
/// addresses. Violating this invariant causes the source wallet's
/// `total_credits()` to absorb the recipient's balance, which corrupts
/// dust-gate checks and sweep address selection in teardown.
pub async fn transfer<S: Signer<PlatformAddress> + Send + Sync>(
&self,
account_index: u32,
Expand Down Expand Up @@ -105,6 +116,15 @@ impl PlatformAddressWallet {
continue;
};
let p2pkh = PlatformP2PKHAddress::new(*hash);
// V27-007 / QA-010: skip foreign output addresses.
// The SDK returns post-transition state for every address in
// the transition (inputs + outputs). Output addresses may
// belong to a different wallet; writing their balances here
// would pollute this wallet's local ledger and corrupt
// `total_credits()`. See the method-level doc comment.
if !account.contains_platform_address(&p2pkh) {
continue;
}
Comment on lines +119 to +127
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw May 19, 2026

Choose a reason for hiding this comment

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

🟡 Suggestion: 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.

source: ['claude', 'codex']

🤖 Fix this 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.

Copy link
Copy Markdown
Contributor Author

@lklimek lklimek May 19, 2026

Choose a reason for hiding this comment

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

Verified against current head a9eae10. Finding is accurate: the QA-010/V27-007 ownership guard (transfer.rs:125-127, withdrawal.rs:135-136) is enforced solely by control flow with no crate-local regression test. Not resolving: per architect adjudication this PR lands as-is — the one-line guard is the production fix (reached via C-FFI platform_address_wallet_transfer/_withdraw). The focused mixed owned/foreign address_infos regression test you describe is valid and tracked as deferred follow-up under the separate #3549 platform_addresses module-debt workstream (memcan-tracked), not in scope for this surgical fix. No code change here.

Comment on lines 116 to +127
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw May 19, 2026

Choose a reason for hiding this comment

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

💬 Nitpick: Silent skip of foreign addresses has no observability

Foreign addresses are dropped with a bare continue and no log line in either transfer.rs or withdrawal.rs. The bug being fixed (a transfer test trimming credits to a bank's primary address) would have been obvious from logs alone if the wallet emitted a tracing::debug! or trace! entry like "skipping post-transition state for non-owned address {p2pkh}". Since this guard is specifically intended to absorb SDK-vs-wallet impedance mismatches, a low-level log is cheap insurance and aids future debugging without adding noise at higher log levels.

source: ['claude']

Copy link
Copy Markdown
Contributor Author

@lklimek lklimek May 19, 2026

Choose a reason for hiding this comment

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

Verified against current head a9eae10. Finding is accurate: foreign addresses are dropped with a bare continue (transfer.rs:126, withdrawal.rs:136) and no tracing::debug!/trace! line; the only tracing in transfer.rs is the unrelated error! at :177. Not resolving: per architect adjudication this PR lands as-is as a minimal surgical correctness fix. Adding low-level observability on the skip is a reasonable enhancement but is deferred follow-up under the separate #3549 platform_addresses module-debt workstream, not in scope here. No code change in this PR.

let funds = match maybe_info {
Some(ai) => dash_sdk::platform::address_sync::AddressFunds {
balance: ai.balance,
Expand Down
7 changes: 7 additions & 0 deletions packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ impl PlatformAddressWallet {
continue;
};
let p2pkh = PlatformP2PKHAddress::new(*hash);
// Defense-in-depth (V27-007 / QA-010): only update owned
// addresses. Withdrawals send no platform output addresses,
// so this guard is never expected to fire, but keeps the
// local-ledger ownership invariant consistent with `transfer`.
if !account.contains_platform_address(&p2pkh) {
continue;
}
let funds = match maybe_info {
Some(ai) => dash_sdk::platform::address_sync::AddressFunds {
balance: ai.balance,
Expand Down
Loading