Repository navigation
feat(platform-wallet)!: report a freshly built asset lock's outpoint to the caller - #5476
romchornyi wants to merge 13 commits into
Conversation
…to the caller A host that watches persisted asset-lock rows while a funding call runs cannot tell its own new lock from rows the wallet re-inserts during recovery (`recovered_status` brings historical locks back at Broadcast/InstantSendLocked with a fresh timestamp). Guessing by wallet, funding type and creation time picks the wrong row. Add an optional `on_asset_lock_built` observer to the fresh-build funding paths: `shielded_fund_from_asset_lock`, `fund_from_asset_lock` and `fund_from_asset_lock_external`. The asset-lock builder calls it with the outpoint once the lock is tracked in memory, before its row is persisted or the lock is broadcast, so no persisted row exists before the host knows the outpoint. It does not run on resume or for a build abandoned before tracking. It does run for a lock whose broadcast then fails; the row stays unless the rejection is definite. Expose it over the C ABI as an optional `AssetLockBuiltFn` plus context on the BIP44 and CoinJoin-drain shielded fund functions and the two platform-address fund functions, and in Swift as an `onAssetLockBuilt` handler (default `nil`) that receives the outpoint in `PersistentAssetLock.outPointHex` form. The JNI bindings pass no callback. The BIP44 shielded fund export now reports a broadcast rejection and an unconfirmed broadcast with their typed codes (26 and 20), as the CoinJoin-drain export already does, so a host can tell a removed lock from a kept one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…SDK reports A fresh Core → Shielded or Core → Platform transfer found its new asset lock by polling for the newest row of its wallet and funding type created since the attempt started. The SDK re-inserts historical locks during recovery with a fresh timestamp, so the guess could pick another lock, and a lock could look stuck before the poll claimed it. The funding calls now pass `onAssetLockBuilt` (dashpay/platform#5476). The handler claims the lock in `ShieldedAssetLocksInFlight` on the FFI worker thread before the lock's row is persisted, the poll reads that exact row, and the attempt takes over the claim when the call returns. "Try again" is armed with the reported outpoint unless the broadcast was definitely rejected. This removes the per-wallet new-lock slot, the `mayBeUnclaimedNewLock` resume gate and the "Another transfer is still in progress" refusal. The registry still counts fresh transfers from before authorization, so removals stay refused while one runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Observer contract and build timing packages/rs-platform-wallet/src/wallet/asset_lock/build.rs, packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs, packages/rs-platform-wallet/src/wallet/asset_lock/mod.rs, packages/rs-platform-wallet/src/lib.rs, packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs, packages/rs-platform-wallet/src/wallet/identity/network/registration.rs, packages/rs-platform-wallet/src/wallet/shielded/seed_pool.rs |
The optional observer is threaded through asset-lock construction and funding resolution. The build pipeline reports qualifying outpoints before persistence or broadcast. Resume paths and existing callers without an observer use None. The seed-pool call no longer supplies a chain-lock fallback timeout. |
Wallet funding entry points and tests packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs, packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs, packages/rs-platform-wallet/src/wallet/shielded/wallet_drain_tests.rs, packages/rs-platform-wallet/src/test_support.rs, packages/rs-platform-wallet-ffi/src/shielded_wallet_drain.rs |
Platform-address and shielded funding APIs forward the optional observer. Tests check outpoint reporting, rejected broadcasts, tracking, and wallet-drain operation state. Test support adds chain-locked funding helpers and a signed wallet fixture. The FFI wallet-drain call passes no observer. |
C callback adapter and FFI funding exports packages/rs-platform-wallet-ffi/src/asset_lock/*, packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs, packages/rs-platform-wallet-ffi/src/shielded_send.rs, packages/rs-unified-sdk-jni/src/* |
The FFI adapter stores the opaque context, serializes outpoints, and invokes the C callback. Fresh funding exports construct and forward the observer; resume calls pass none. Shielded funding preserves typed broadcast errors. JNI calls pass no callback and a null context. |
Swift callback API and forwarding packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLockBuiltCallback.swift, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift, packages/swift-sdk/SwiftTests/SwiftDashSDKTests/AssetLockBuiltCallbackTests.swift |
Swift adds an outpoint type, a handler type, and a C trampoline. The bridge decodes the 36-byte outpoint and keeps the handler box alive for the FFI call. Platform-address and shielded funding APIs accept and forward optional handlers. Tests cover decoding, pointer handling, byte ownership, and handler lifetime. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant FundingResolver
participant AssetLockBuild
participant FfiCallbackAdapter
participant SwiftTrampoline
participant AssetLockBuiltHandler
FundingResolver->>AssetLockBuild: pass optional observer
AssetLockBuild->>FfiCallbackAdapter: report built outpoint
FfiCallbackAdapter->>SwiftTrampoline: pass serialized outpoint and context
SwiftTrampoline->>AssetLockBuiltHandler: deliver decoded outpoint
Possibly related issues
- platform-wallet: one notification model for hosts — asset-lock lifecycle events keyed by operation id through the event bus #5539: This change adds per-call asset-lock notifications, but does not introduce the issue’s broader event-bus notification model.
Suggested reviewers: thepastaclaw
Merge Risk: 
a063b
-
A narrow wallet-removal race can leave a caller without the outpoint of a broadcast lock. The PR is mergeable with that known risk explicitly accepted or addressed.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "feat(platform-wallet)!: report a freshly built asset lock's outpoint to the caller" matches the main change in the changeset. The raw summary shows a new optional AssetLockBuiltObserver/Ass… Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "feat(platform-wallet)!: report a freshly built asset lock's outpoint to the caller" matches the main change in the changeset. The raw summary shows a new optional AssetLockBuiltObserver/AssetLockBuiltFn callback that reports the outpoint of a newly built asset lock, threaded through Rust, FFI, and Swift funding entry points. The breaking-change marker (!) fits the summary, which notes that Rust and C ABI signatures gained required arguments. The title is specific and concise, and a teammate scanning history would understand the primary change.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
⛔ Final review complete — 1 blocking finding(s) (commit a063b37) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against exact head 13509f3. No blocking defects were confirmed; one non-blocking Swift callback test-coverage suggestion remains. Validation was static only: the supplied CI snapshot reports successful Rust workspace, Swift SDK, and Kotlin SDK checks, while the dedicated Rust wallet tests were skipped and PR Hygiene remained pending.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The cross-language change adds asset-lock observation callbacks and typed error propagation without changing funds movement, coin selection, signing, or persistence rules themselves. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLockBuiltCallback.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLockBuiltCallback.swift:44-51: Add Swift tests for the optional callback bridge and outpoint decoding
The new Swift bridge has no direct coverage in SwiftTests. The Rust callback tests verify context forwarding and raw outpoint bytes, but they do not exercise Swift's box recovery, handler invocation, or conversion into the persisted outpoint key used to identify an in-flight transfer. Add tests through `withAssetLockBuiltCallback`: assert that a nil handler supplies two nil arguments, then invoke the supplied function pointer with its context and a synthetic 36-byte buffer and verify the handler receives the expected display-order txid and vout. Use nonuniform txid bytes and a multibyte vout so the test detects byte-order regressions. The private trampoline can remain private because the helper exposes its function pointer to the test.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Extend asset lock built observer to identity registration and top-up flows — The PR explicitly states that identity registration and top-up callers pass None because no host currently needs these notifications. Both registration and top-up retain the existing funding behavior through the shared resolver; extending their public APIs is outside the stated goal and is not required for the new funding callbacks to work.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
`AssetLockBuiltHandler` now receives an `AssetLockBuiltOutPoint` with the wire-order txid, the vout and the `PersistentAssetLock.outPointHex` key, decoded once from the FFI's 36 bytes. A host no longer has to parse the hex string back into bytes to claim or resume the lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arsing it `onAssetLockBuilt` now hands over an `AssetLockBuiltOutPoint` with the wire-order txid and vout (dashpay/platform#5476), so the coordinator no longer parses the hex string. That drops `parseOutPoint`, whose delegation to `DWIdentityRegistrationCoordinator` did not compile in the dashwallet target, and the unreadable-outpoint branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 3fab1ac found no blocking issues; the remaining suggestion is targeted Swift callback-bridge coverage. The observer ordering, borrowed-buffer copying, and blocking-call context lifetime are consistent across the Rust, C, and Swift layers. The supplied CI snapshot reports successful Rust workspace, Swift, and Kotlin checks; the dedicated Rust wallet job was skipped and PR Hygiene remains pending.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The cross-language callback plumbing and error mapping require careful review, but the diff adds notification around existing asset-lock operations rather than changing funds movement, coin selection, signing, or consensus rules. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLockBuiltCallback.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLockBuiltCallback.swift:70-77: Add Swift tests for the optional callback bridge and outpoint decoding
(existing thread: https://github.com/dashpay/platform/pull/5476#discussion_r4296059400)
No tests under SwiftTests exercise this helper or AssetLockBuiltOutPoint. The Rust tests cover observer ordering and delivery to a C callback, but cannot detect regressions in Swift's optional callback/context pairing, Unmanaged context recovery, or typed decoding. Add offline tests that verify a nil handler supplies two nil arguments and synchronously invoke the supplied callback/context with a nonuniform txid and a multi-byte little-endian vout. Assert the wire-order txid, numeric vout, and independently expected display-order outPointHex, since those values identify the persisted row and provide the resume arguments. Retain the delivered value and overwrite the source buffer to verify ownership; also cover captured-state release after normal and throwing body completion. The existing @testable import permits these tests without expanding the public API. No current decoding or lifetime defect is asserted.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
…coding Six offline tests through `withAssetLockBuiltCallback`: - a nil handler gets no callback and no context; - a nonuniform txid and a multi-byte vout decode in wire order, and the `outPointHex` row key matches a hand-built display-order string, after the borrowed buffer is overwritten in place; - a NULL context or outpoint is ignored; - a callback at the end of the body still reaches the handler; - the handler is released after the body returns and after it throws, and the body's own error is rethrown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at 4b0f4b4, including notification ordering, Rust/C/Swift callback lifetimes, outpoint encoding, broadcast-error propagation, and updated callers; no actionable in-scope defects were found. The six added Swift tests resolve the prior callback-bridge coverage finding. Validation was static only: current CI reports Kotlin build/tests and the Swift frozen-schema check passing, while Rust workspace tests remain pending; the author's reported Swift test runs were not independently executed.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The cross-language change adds callback plumbing and typed broadcast-error reporting without changing transaction construction, coin selection, signing, or funds-movement rules, so it does not meet the critical bar. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
github-actions
Bot
added
the
waiting-self-review
github-actions
Bot
removed
the
waiting-self-review
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Re-approving at e3e4f33. The new commit adds the wiring tests through every funding entry point and narrows the observer, FFI and Swift docs, which addresses both points from my earlier comment. It changes only tests and docs. LGTM.
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
I'm approving the rs-unified-sdk-jni part (the files with no dedicated owner). Passing None, std::ptr::null_mut() matches the new C ABI, and the Kotlin build is green. Nothing here touches dpp, drive or consensus.
Merge order: this PR conflicts with #5460 in rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs and wallet/shielded/fund_from_asset_lock.rs. Since this one is the follow-up, please rebase it once #5460 lands, and recheck the observer-wiring tests against #5460's InstantSend → ChainLock resubmit path.
Nit: the "yet" in "No Kotlin caller observes the built lock yet" will go stale. Something like "Kotlin hosts do not take the callback" ages better.
|
Policy satisfied — this can merge. |
github-actions
Bot
removed
ready-for-human
|
Heads-up from #5645 (DIP-11 identity creation keys), which overlaps this PR and #5460 in
Nothing in #5645 depends on the 🤖 Co-authored by Claudius the Magnificent AI Agent |
…uilt-observer Conflicts (imports only): both fund_from_asset_lock.rs keep AssetLockBuiltObserver and drop submit_with_cl_height_retry, which #5460 replaced with submit_with_chain_lock_fallback. The two FromExistingAssetLock tests #5460 added in sync/proof.rs pass no on_built observer: an existing lock is not built. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…llback Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0aac302
|
@shumkov #5460 landed, so I merged
CI here will still fail |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
@thepastaclaw review |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The callback wiring and lifetime handling are correct, and all three prior findings are fixed. However, the PR changes four existing exported C symbols by appending arguments; an already-built host using the old prototypes can cause the new function to read arbitrary ABI argument locations and potentially call an invalid callback pointer. The Rust workspace test failure appears unrelated and comes from pre-existing rs-sdk call sites, but the C ABI issue requires changes.
🔴 1 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large cross-cutting behavioral change spanning Rust wallet funding orchestration, Swift FFI-facing wrappers, JNI plumbing, callbacks, and broadcast error handling, but it does not itself alter consensus, cryptography, key handling, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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-ffi/src/platform_addresses/fund_from_asset_lock.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs:76-90: Do not change existing C export signatures in place
The existing exported symbol `platform_address_wallet_fund_from_asset_lock_signer` now requires `AssetLockBuiltFn` and its context, and the same in-place ABI change is made to `platform_address_wallet_fund_from_asset_lock_external_signer` and the two shielded funding exports. A host compiled against the previous C prototype will still call the unchanged symbol with the old argument count. The new callee then reads the callback and context from registers or stack locations that the old caller did not provide; if the callback value is non-null, `AssetLockBuiltCallback::notify` can call an arbitrary address, causing undefined behavior or a process crash. Marking the PR as breaking and telling source callers to pass `NULL, NULL` does not make an already-built binary compatible. Preserve the old symbols with their old signatures and add versioned/new symbols for the callback-enabled ABI, or introduce an explicit ABI version boundary and update rebuilt Swift/JNI callers to use it.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:33: Keep the built-lock observer with the asset-lock build layer
`build.rs` imports `AssetLockBuiltObserver` from `asset_lock::orchestration`, while `orchestration.rs` imports `AssetLockBuildAmount` from `build`. The observer describes the lifecycle boundary owned by the asset-lock builder, so placing it in `build` or `asset_lock::mod.rs` would avoid making the lower-level transaction-building module depend on the higher-level funding-choice orchestration module and would give both layers a shared downward dependency.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Coordinate asset-lock tracking, persistence, and broadcast with wallet teardown — The asset-lock flow can still race wallet teardown between tracking, persistence, and broadcast, potentially leaving a funded transaction without durable local tracking or recovery state. The same tracking-to-broadcast sequence exists on the base revision, so this callback change does not introduce or worsen it; the issue is nevertheless a concrete funds-recovery risk.
- Follow-up: Continue the tracked teardown-coordination fix in #5516, covering both ordinary and wallet-drain asset-lock builds with the wallet generation payment guard.
| @@ -79,6 +85,8 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_signer( | |||
| signer_address_handle: *mut SignerHandle, | |||
| core_signer_handle: *mut MnemonicResolverHandle, | |||
| out_changeset: *mut PlatformAddressChangeSetFFI, | |||
| on_asset_lock_built: AssetLockBuiltFn, | |||
| on_asset_lock_built_context: *mut c_void, | |||
| ) -> PlatformWalletFFIResult { | |||
There was a problem hiding this comment.
🔴 Blocking: Do not change existing C export signatures in place
The existing exported symbol platform_address_wallet_fund_from_asset_lock_signer now requires AssetLockBuiltFn and its context, and the same in-place ABI change is made to platform_address_wallet_fund_from_asset_lock_external_signer and the two shielded funding exports. A host compiled against the previous C prototype will still call the unchanged symbol with the old argument count. The new callee then reads the callback and context from registers or stack locations that the old caller did not provide; if the callback value is non-null, AssetLockBuiltCallback::notify can call an arbitrary address, causing undefined behavior or a process crash. Marking the PR as breaking and telling source callers to pass NULL, NULL does not make an already-built binary compatible. Preserve the old symbols with their old signatures and add versioned/new symbols for the callback-enabled ABI, or introduce an explicit ABI version boundary and update rebuilt Swift/JNI callers to use it.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
There was a problem hiding this comment.
Not changing this one: these exports have no separately built consumer, so the old-binary-on-new-library case does not occur.
- Swift links
librs_unified_sdk_ffi.afromDashSDKFFI.xcframework, whichbuild_ios.shbuilds from one tree together with the cbindgen header that Swift compiles against. The released xcframework zip ships both, and dashwallet-ios builds it at its.platform-ref. A mismatched prototype is a compile error there. - Kotlin reaches these functions through
rs-unified-sdk-jni, which calls them as Rust functions over the rlib (already updated in this PR). TheJava_*entry points Kotlin calls keep their signatures, andbuild_android.shdrops the standalonelibplatform_wallet_ffi.so. - Nothing in the repository loads the cdylib dynamically.
The repository does not keep these C prototypes stable across builds. The coding conventions treat even removing an extern "C" function as ordinary work described in the PR body, and there is precedent on these very exports: #3793 added surplus_output_ptr/surplus_output_len to platform_wallet_manager_shielded_fund_from_asset_lock in place, and #5649 replaced dash_sdk_derive_connect_key_with_resolver without keeping the old symbol. Versioned duplicates here would add four exports that no matching build calls. Binary stability for the FFI would be a crate-wide policy, which is worth an issue of its own if maintainers want it. This change is listed under "C ABI" in the description.
There was a problem hiding this comment.
The bundled Swift static library and the JNI path are rebuilt together, so those in-tree consumers are not the counterexample. However, platform-wallet-ffi still declares staticlib, cdylib, and rlib, and its README explicitly documents consuming the generated .so/.dylib from C or C++; those consumers can load a newer library with an older caller and are not protected by the synchronized build. The prior in-place changes do not establish ABI compatibility, and marking this release as breaking does not prevent the old symbol from being called with the old argument list, so the existing exports still need to retain their signatures or be supplemented with new symbols.
There was a problem hiding this comment.
The README's C/C++ section builds the library from source, and the same build runs cbindgen (build.rs) to regenerate platform_wallet_ffi.h, so a consumer that follows it gets the new prototype along with the new library. No release publishes libplatform_wallet_ffi.so/.dylib: release-swift-sdk.yml ships the xcframework (static library plus header) and release-kotlin-sdk.yml ships the Android AAR, whose libdash_sdk_jni.so is called only by the Kotlin classes packaged with it. Neither artifact lets an older caller resolve a newer library.
The repository also does not treat these exports as a stable ABI: the coding conventions (book/src/contributing/coding-conventions.md, "Commits, pull requests") class removing an extern "C" function as ordinary feat:/refactor: work described in the PR body. A changed signature is no stronger a break than a removed symbol. The error-code registry is a different case: those integers are mirrored by hand in Swift and Kotlin, so a renumbering compiles cleanly and is misread. A changed prototype fails to compile against the regenerated header.
A versioned C ABI for platform-wallet-ffi (old symbols kept as wrappers, suffixed callback-capable ones added) would be a policy for the whole crate and should be settled with the maintainers in its own issue, not introduced for four functions here. I'm leaving this thread open for them.
There was a problem hiding this comment.
The synchronized Swift/JNI builds protect only callers rebuilt from this checkout; they do not protect a previously compiled C/C++ object or an application that links an older object against a newly built static or dynamic library. This crate explicitly produces staticlib and cdylib, and its README documents C/C++ consumption, so regenerating the header makes a source rebuild fail or succeed correctly but does not make the existing symbol ABI-safe for already-compiled callers. Prior in-place breaks are precedent for the same compatibility problem, not evidence that this change is safe; these exports still need old-signature wrappers or distinct callback-enabled symbols.
There was a problem hiding this comment.
Weighing in as an rs-platform-wallet-ffi owner: I agree with Roman and don't consider this blocking.
- The repository does not treat the C prototypes as a stable ABI.
book/src/contributing/coding-conventions.mdclasses removing anextern "C"function as ordinaryfeat:/refactor:work, and feat(drive): shielded fees for Shield/ShieldFromAssetLock + shield credit conservation #3793 already appendedsurplus_output_ptr/surplus_output_lentoplatform_wallet_manager_shielded_fund_from_asset_lockin place. - Nothing ships the cdylib on its own. The Swift release carries the xcframework and the cbindgen header built from one tree, and Kotlin reaches these functions through the JNI crate over the rlib. A caller built against the old header fails to compile against the new one; it cannot resolve the new symbol with the old arity.
ERROR_CODE_REGISTRY.md's "shipped host binary" rule is about the integer codes, which Swift and Kotlin mirror by hand and which renumber silently. That is a different failure mode from a changed prototype.
If maintainers want versioned C exports for this crate, that's a crate-wide policy and belongs in its own issue, not in this PR.
There was a problem hiding this comment.
I still consider this blocking. The repository itself documents the opposite compatibility assumption in core_wallet_signed_payment_finalize: it explicitly preserves an old C signature because a host's compiled code and DashSDKFFI.xcframework can meet at different versions, and warns that adding an argument in place can make the callee treat an absent argument as an address. That applies to these exports as well; the README documents both staticlib and independently consumable cdylib outputs, so rebuilding the generated header only protects source rebuilds, not an already-compiled C/C++ caller or separately packaged binary. The prior in-place change is precedent for the same ABI hazard, not evidence that this four-export change is safe; please retain wrappers with the old signatures or add distinct callback-enabled symbols.
| use crate::ASSET_LOCK_FUNDING_SOURCES; | ||
|
|
||
| use super::manager::{AssetLockManager, DEFAULT_FEE_PER_KB}; | ||
| use super::orchestration::AssetLockBuiltObserver; |
There was a problem hiding this comment.
✅ Resolved at
a063b376; see the replies below.
🟡 Suggestion: Keep the built-lock observer with the asset-lock build layer
build.rs imports AssetLockBuiltObserver from asset_lock::orchestration, while orchestration.rs imports AssetLockBuildAmount from build. The observer describes the lifecycle boundary owned by the asset-lock builder, so placing it in build or asset_lock::mod.rs would avoid making the lower-level transaction-building module depend on the higher-level funding-choice orchestration module and would give both layers a shared downward dependency.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Moved in a063b37: AssetLockBuiltObserver now lives in build.rs and is re-exported from asset_lock; orchestration imports it from build, so build no longer imports from orchestration outside tests.
There was a problem hiding this comment.
Resolved (re-reviewed at a063b376): This is fixed in a063b37: AssetLockBuiltObserver now lives in build.rs, is re-exported from asset_lock, and orchestration.rs imports it from the builder layer, removing the upward dependency.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
…t-lock builder AssetLockBuiltObserver moves from orchestration.rs to build.rs, so the builder no longer imports from the funding orchestration above it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The observer is now correctly owned by the asset-lock build layer and is wired through the Rust, FFI, JNI, and Swift paths with focused lifecycle coverage. One blocking compatibility defect remains: four existing public C ABI symbols were changed in place by appending callback parameters, so previously compiled native callers can supply arbitrary values that may be invoked as a callback. The changed-package and runner-selection CI checks were still queued at the captured head; no related failure was reported.
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate cross-language change affecting asset-lock construction, tracking, broadcasting, persistence ordering, wallet draining, and funds-movement error handling in platform-wallet. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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-ffi/src/platform_addresses/fund_from_asset_lock.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs:76-90: Do not change existing C export signatures in place
(existing thread: https://github.com/dashpay/platform/pull/5476#discussion_r4414066754)
`platform_address_wallet_fund_from_asset_lock_signer` and its external-signer sibling now append `AssetLockBuiltFn` and a context pointer to existing `#[no_mangle] extern "C"` symbols. The shielded funding and CoinJoin-drain exports in `shielded_send.rs` have the same change. A caller compiled against the previous header still resolves the old symbol and calls it with the old arity; the new callee can read unspecified register or stack contents as the callback and context. If the accidental callback value is non-null, `AssetLockBuiltCallback::notify` invokes it as a function pointer, potentially crashing or corrupting the host. Marking the PR as breaking and documenting `NULL, NULL` only helps callers rebuilt against the new declaration; it does not make existing binaries safe. Preserve the old symbols and signatures as wrappers that pass no observer, and add suffixed or versioned callback-capable exports for rebuilt Swift and other native consumers. The repository's FFI registry explicitly treats shipped ABI values as public and notes that shipped host binaries retain the ABI they were built against.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Coordinate wallet teardown with in-flight asset-lock broadcasts — The non-operation asset-lock path can still reach broadcast after wallet teardown removes the wallet, potentially leaving a broadcast lock without reliable wallet-owned tracking or reservation state. This behavior predates this PR and is not fixed by the callback change; it has been tracked separately as #5516.
- Follow-up: #5516
|
On the open blocking finding about the C ABI (thread): this PR appends I'm not doing that here, because no consumer can be in that position:
If maintainers want a stable C ABI for |
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Re-approving at a063b37 for rs-platform-wallet and rs-platform-wallet-ffi.
Since my approval at e3e4f33 the PR content changed in two places, and I checked both against an interdiff of the PR against its base before and after the v5.1-dev merges:
- a063b37 moves
AssetLockBuiltObserverfromorchestration.rstobuild.rsand re-exports it fromasset_lock. The doc text and the type are unchanged;orchestrationnow imports frombuild, and thelib.rsexport still resolves. The builder no longer depends upward. - 0aac302 rewords three JNI comments.
The merges from v5.1-dev (incl. #5460) did not disturb the wiring: on_built still reaches broadcast_funded_asset_lock_with_operation from every fresh-build arm of resolve_funding_with_is_timeout_fallback, both platform-address entry points and shielded_fund_from_asset_lock; resume paths and the identity/seed-pool callers still pass None; the four C exports and the Swift handler are as reviewed. The entry-point wiring tests from e3e4f33 are in place, and CI at this head is green (Rust workspace tests, Swift SDK build + tests, Kotlin SDK).
On the open thepastaclaw thread about changing the C export signatures in place: I replied there. I don't consider it blocking for this crate given the repo's stated conventions and the #3793 precedent, and no artifact ships the cdylib for an older caller to load.
Basic explanation
What this does: When a wallet app moves Dash from its Core balance to the shielded pool or to a Platform address, the SDK builds a new asset lock inside one long-running call. This lets the app pass a callback that receives the new lock's outpoint as soon as the lock is signed and tracked, before the SDK saves its row or broadcasts it.
Value: The app can tell its own new lock apart from every other lock row while the call runs. Today it has to guess by wallet, funding type and creation time. That guess picks the wrong row whenever the SDK re-inserts an old lock during recovery:
recovered_statusbrings historical locks back at Broadcast/InstantSendLocked with a fresh timestamp. With the exact outpoint the app shows a running transfer as running and never offers "Finish now" on a lock that a running call is still consuming (dashwallet-ios#1212, the follow-up to dashwallet-ios#1042 and #5460).Risks: Low. No consensus or storage change. The callback is optional and passing none behaves as before. It runs at most once per fresh build, synchronously on the funding worker thread, before the lock's row is persisted. One error-code change: the BIP44 shielded fund and resume exports now report a broadcast rejection as
ErrorTransactionBroadcastRejected(26) and an unconfirmed broadcast asErrorTransactionBroadcastUnconfirmed(20), as the CoinJoin-drain export already did, instead ofErrorWalletOperation(6).Issue being fixed or feature implemented
dashwallet-ios#1212 marks an asset lock as "in flight" while the funding call that consumes it is running, so the transaction details and the Home recovery surfaces neither offer a second attempt nor a removal. For a fresh transfer the app learns the lock only by polling
PersistentAssetLockrows and taking the newest row of the wallet and funding type created after the call started. Review found that a reconstructed historical lock can match that filter, and that a stuck-lock resume can race the new lock before the poll claims it. Neither can be closed from the app without knowing the lock's outpoint.What was done?
AssetLockBuiltObserver(dyn Fn(OutPoint) + Send + Sync).broadcast_funded_asset_lock_with_fundingcalls it right aftertrack_asset_lockand before the synchronousqueue_asset_lock_changeset, with nothing awaited in between, so a dropped future cannot hand out an untracked outpoint. It is skipped when the build fails, the drain floor abandons it, or the wallet is gone when tracking. The observer reaches the builder through the newcreate_funded_asset_lock_proof_observed/…_with_funding_observed(the public wrappers passNone) andresolve_funding_with_is_timeout_fallback(theFromWalletBalance,DrainAccountBalanceandDrainWalletBalancearms; resume never builds a lock). On the pooled wallet-drain path (DrainWalletBalance) the timing differs: the observer fires afteroperation.set(Pending, Some(out_point))and before the durable store, and a lock whose store fails is recovered by the operation id. The FFI wallet-drain export passesNone, so hosts see the callback only on the non-drain paths. Wiring tests drive every hop from a public entry point to the builder with a refused broadcast and assert one report:resolve_funding_with_is_timeout_fallback(FromWalletBalance,DrainAccountBalance),fund_from_asset_lock,fund_from_asset_lock_external, andshielded_fund_from_asset_lockfor a fresh lock and for the pooled wallet drain. The Kotlin SDK does not take the callback yet: the JNI exports passNone, and adding it there is left to a follow-up (see platform-wallet: one notification model for hosts — asset-lock lifecycle events keyed by operation id through the event bus #5539, which proposes one notification model for all hosts).shielded_fund_from_asset_lock,fund_from_asset_lockandfund_from_asset_lock_externalgain anon_asset_lock_built: Option<&AssetLockBuiltObserver>parameter. Identity registration/top-up, invitations and the seed pool passNone, because no host needs them yet.AssetLockBuiltFn, a nullablevoid (*)(void *context, const uint8_t *out_point_36)typedef, whereout_point_36uses theAssetLockEntryFFI.out_pointlayout. It is added with a context pointer toplatform_wallet_manager_shielded_fund_from_asset_lock,…_coinjoin_drain,platform_address_wallet_fund_from_asset_lock_signerand…_external_signer.map_asset_lock_funding_resultkeepsTransactionBroadcast/TransactionBroadcastUnconfirmedtyped, so a host that holds the reported outpoint can tell a removed lock (26) from a kept one.AssetLockBuiltHandler(@Sendable (String) -> Void, receiving the outpoint asPersistentAssetLock.outPointHex) andonAssetLockBuilt: AssetLockBuiltHandler? = nilonshieldedFundFromAssetLock,shieldedFundFromCoinJoinDrain,fundFromAssetLockandfundFromAssetLockExternal. It uses the same box/trampoline pattern as the seed-pool progress callback, with the importedAssetLockBuiltFntype.How Has This Been Tested?
platform-walletasset_lock/build.rs:should_report_the_built_outpoint_before_persisting_or_broadcast: the observer runs once, with the outpoint the build returns, while the lock is tracked but not stored and the broadcaster has not been called;should_not_report_an_outpoint_for_an_abandoned_build: an undersized drain never reaches the observer;should_report_a_lock_whose_broadcast_is_then_rejected: reported once, then the row is removed;should_keep_a_reported_lock_whose_broadcast_may_have_been_sent: reported once, and the row stays tracked and stored.platform-wallet-ffi:asset_lock/built_callback.rs: an absent function yields no callback, and the observer passes the caller's context and the entry-layout outpoint;map_asset_lock_funding_resulttests: codes 26 and 20 for fresh and resume.cargo test -p platform-wallet --lib: 1430 with--all-features, 1203 default;-p platform-wallet-ffi --all-features: 432;-p rs-unified-sdk-jni: 43.--all-features --tests) andcargo fmt --checkare clean.scripts/check_sdk_parity_manifest.pyis OK.build_ios.sh --target simand a dashwallet-iosdashpaybuild against this SDK.Breaking Changes
PlatformWallet::shielded_fund_from_asset_lock,PlatformAddressWallet::fund_from_asset_lockandfund_from_asset_lock_externaltake a trailingon_asset_lock_builtargument. PassNoneto keep the old behaviour.on_asset_lock_builtandon_asset_lock_built_context. PassNULL, NULLto keep the old behaviour.nil.platform_wallet_manager_shielded_fund_from_asset_lockandplatform_wallet_manager_shielded_resume_fund_from_asset_lockreturn 26 / 20 instead of 6 for a rejected / unconfirmed broadcast. The message no longer carries theshielded fund-from-asset-lock failed:prefix.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
a063b37/self-reviewedrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/asset_lock/built_callback.rs,packages/rs-platform-wallet-ffi/src/asset_lock/mod.rs,packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rsand 2 more) — approved by llbartekllrs-platform-wallet(packages/rs-platform-wallet/src/lib.rs,packages/rs-platform-wallet/src/test_support.rs,packages/rs-platform-wallet/src/wallet/asset_lock/build.rsand 8 more) — approved by llbartekllpackages/rs-unified-sdk-jni/src/funding.rs,packages/rs-unified-sdk-jni/src/wallet_manager.rs) — QuantumExplorer or shumkovswift-sdk— you own itWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit