fix(platform-wallet): preserve reported-consumed asset-lock recovery - #4357
Conversation
|
🕓 Ready for review — next in queue (commit e6241a5) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe wallet detects exact consumed asset-lock reports, records matching locks as ChangesAsset-lock reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FundingFlow
participant AssetLockManager
participant AssetLockTracker
participant PersistenceBackend
participant SDKExample
FundingFlow->>AssetLockManager: submit asset-lock funding result and effective proof
AssetLockManager->>AssetLockTracker: reconcile matching consumed report
AssetLockTracker->>PersistenceBackend: store and flush RecoveredFromChain state
PersistenceBackend-->>AssetLockTracker: persistence result
AssetLockTracker-->>FundingFlow: typed error and recovery result
FundingFlow-->>SDKExample: AssetLockAlreadyConsumed
SDKExample-->>SDKExample: classify as consumption unknown and retain reclaim state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The structured error matching and FFI code preservation are correct, but the reconciliation path trusts an unverified DAPI rejection and can permanently tombstone an asset lock that remains unspent. Persistence callback failures are also hidden from this new path, and the state-changing branch lacks direct orchestration coverage, so changes are required before merge.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (rust-quality), gpt-5.6-sol (ffi-engineer); final verifier backend: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus 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— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
🤖 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/shielded/fund_from_asset_lock.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:439-447: Do not terminally consume a lock from an unauthenticated rejection
The SDK handles a wait-stream error before verifying the response proof or quorum signature, and `submit_with_cl_height_retry` explicitly documents that consensus-error responses have no client-side proof or DAPI-quorum check. A malicious or malfunctioning endpoint can therefore inspect the submitted transition's outpoint and fabricate the matching already-consumed error; comparing the error to that outpoint does not authenticate the verdict. This branch then marks the lock `Consumed`, clears its proof, persists the terminal tombstone, and causes future resumes to reject it locally, potentially stranding an asset lock that Platform never consumed. Preserve the typed error if needed, but do not create terminal wallet state from this response alone; terminal reconciliation requires authenticated state evidence, or the wallet must retain a nonterminal/retryable reported-consumed state.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:445-447: Do not acknowledge reconciliation after host persistence fails
`consume_asset_lock` mutates the in-memory entry and calls `queue_asset_lock_changeset`, but that method logs and discards every `WalletPersister::store` error. The FFI persistence backend returns an error when an asset-lock callback or changeset commit fails, so the `.await?` here cannot observe a host rollback and this branch still reports the typed already-consumed result. After restart, the host can rehydrate the stale ChainLocked/Broadcast row even though the operation claimed to have reconciled it. Propagate the persistence failure before returning `AssetLockAlreadyConsumed`, and restore the previous in-memory lock state if the durable update fails.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:437-457: Test the consumed-error reconciliation branch directly
The added tests separately exercise the pure error matcher, `consume_asset_lock`, and FFI mapping, but none drives an `Err(dash_sdk::Error)` through this match and verifies the combined behavior. An integration mistake here—using the wrong outpoint, omitting consumption, or wrapping the result back into `PlatformWalletError::Sdk`—would leave every added test passing. Add a focused orchestration test, or extract this branch into a testable helper, and assert that a matching error updates the submitted lock and returns the typed wallet error while unrelated or mismatched-outpoint errors do not mutate persisted status.
70a9297 to
8ac6e36
Compare
llbartekll
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs (1)
399-451: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the promoted ChainLock proof in reconciliation.
The IS→CL arm promotes the proof to
chain_proof, but line 446 passes the originalprooftoreconcile_asset_lock_submit_error. In that pathproofis stillAssetLockProof::Instant, so the helper callsupgrade_to_chain_lock_proofa second time. Withcl_wait == Nonethat call waits without a bound again, even though the promotion already succeeded. Track the effective proof and pass it to reconciliation.♻️ Proposed refactor
- let submit_result = match submit_with_cl_height_retry(settings, |s| { + let mut effective_proof = proof.clone(); + let submit_result = match submit_with_cl_height_retry(settings, |s| {self.asset_locks.queue_asset_lock_changeset(cs); + effective_proof = chain_proof.clone(); submit_with_cl_height_retry(settings, |s| {return reconcile_asset_lock_submit_error( &self.asset_locks, e, &proof_out_point, - &proof, + &effective_proof, cl_wait, ) .await🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs` around lines 399 - 451, Track the effective asset-lock proof across the submit flow, updating it to the promoted chain_proof in the is_instant_lock_proof_invalid arm. Pass this effective proof instead of the original proof to reconcile_asset_lock_submit_error, while preserving the existing proof for non-promotion paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs`:
- Around line 196-231: After successfully storing the recovery changeset in the
recovery flow around queue_asset_lock_changeset, flush the per-wallet changeset
before returning so RecoveredFromChain is durable. Handle flush failures using
the existing PersistenceErrorKind conventions, including rollback behavior where
required, and propagate the resulting persistence error consistently with the
current store-error path.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- Around line 399-451: Track the effective asset-lock proof across the submit
flow, updating it to the promoted chain_proof in the
is_instant_lock_proof_invalid arm. Pass this effective proof instead of the
original proof to reconcile_asset_lock_submit_error, while preserving the
existing proof for non-promotion paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f69af23-c448-415c-8d47-d0b429bae91a
📒 Files selected for processing (5)
packages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-platform-wallet-ffi/src/shielded_send.rs
- packages/rs-platform-wallet/src/error.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The previous unauthenticated-consumption blocker and both prior suggestions are fixed: matching reports now retain an authenticated ChainLock proof in nonterminal RecoveredFromChain state, immediate store failures roll back the in-memory candidate, and the extracted reconciliation helper has direct coverage. Two in-scope suggestions remain: the helper does not cross the persistence trait's flush durability boundary, and the InstantLock-to-ChainLock retry discards the effective ChainLock proof before reconciliation.
Source: reviewers gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
🤖 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/asset_lock/sync/tracking.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:214-217: Flush the recovery marker before acknowledging reconciliation
`PlatformWalletPersistence::store` is explicitly a buffering operation, while `flush` is the durability boundary. `SqlitePersister` in `FlushMode::Manual`, for example, returns `Ok(())` here after only merging `RecoveredFromChain` into its in-memory accumulator. The caller consequently returns the typed `AssetLockAlreadyConsumed` result even though a process exit can lose the marker or a later flush can fail. Persist this update through `flush()` before acknowledging reconciliation, and route both store and flush failures through rollback handling. The failure path must also account for transient flush failures retaining the candidate in the backend buffer; otherwise a later flush could persist the candidate after the wallet's in-memory entry was rolled back.
In `packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:439-447: Carry the effective ChainLock proof into error reconciliation
When the initial InstantLock submission is rejected, the fallback obtains a valid `chain_proof`, records it, and uses it for the second submission. If that submission reports the outpoint as already consumed, this branch nevertheless passes the original InstantLock `proof` to reconciliation. The helper then repeats `upgrade_to_chain_lock_proof`, duplicating SPV and persistence work and potentially replacing the intended typed result with `AssetLockProofWait` if the transaction record becomes unavailable between lookups, even though this flow already owns a valid ChainLock proof. Track the effective proof across the retry, update it after promotion, and pass it to `reconcile_asset_lock_submit_error`; add coverage for an already-consumed result from the ChainLock retry.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The previous durability-boundary and effective-proof findings are fixed, and the recovery path now retains a ChainLock proof in nonterminal state. Two in-scope suggestions remain: a fatal FFI flush callback can make the live wallet disagree with the already-committed host store, and the fresh BIP44 funding entry point still flattens the newly reachable typed already-consumed result.
Source: reviewer backend model gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/asset_lock/sync/tracking.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:214-236: Do not roll back after the FFI store has already committed the marker
A non-transient `flush()` error does not always mean the candidate failed to reach durable storage. `FFIPersister::store` invokes the per-kind callbacks and then `on_changeset_end_fn`; that callback's documented contract commits the host transaction before `store` returns `Ok(())`. The subsequent `FFIPersister::flush` only invokes `on_flush_fn`, and a nonzero result is reported as a fatal error while the already-committed host row cannot be undone. This branch then restores the previous in-memory lock even though the host may already contain `RecoveredFromChain`, leaving the live wallet inconsistent with restart state. Separate store and flush outcomes and align `FFIPersister`'s flush disposition with its actual commit boundary so rollback occurs only when the candidate is known not to be durable; add coverage for a fatal post-store flush failure.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1041-1047: Preserve the typed report for fresh asset-lock funding
The wallet method now reconciles matching already-consumed reports for every funding mode, including `AssetLockFunding::FromWalletBalance`, so this fresh-funding entry point can return `PlatformWalletError::AssetLockAlreadyConsumed`. It still maps every failure to `ErrorWalletOperation`, unlike the resume entry point and CoinJoin entry point. Swift and Kotlin therefore receive the generic wallet-operation error instead of stable code 24 for an initial BIP44-funded submission, losing the consumption-unknown signal that this PR introduced. Preserve the typed variant here while retaining the existing generic mapping for unrelated errors.
|
Also addressed the review-body-only FFI suggestion in 4b68180: the fresh BIP44 shielded funding entry point now preserves |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The PR now preserves authenticated Core finality in a nonterminal recovery state and correctly returns the typed already-consumed result from both fresh and resumed FFI funding. One persistence-phase bug remains around a post-commit FFI store notification, while the public wording and fresh-funding regression coverage need follow-up; no blocking defects were confirmed.
Source: reviewer backend model gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:262-266: Do not describe the unauthenticated report as confirmed consumption
The documentation correctly states that this variant can represent an unauthenticated Platform report, but its rendered message still says the asset lock "has already been consumed." The public FFI documentation for code 24 similarly describes the one-shot output as already consumed. This PR now returns that message and code while deliberately storing `RecoveredFromChain` because Platform-side consumption remains unknown, so logs or host UI guidance can misclassify a retryable, nonterminal recovery state as authenticated completion. Keep the stable variant and FFI discriminant, but describe the result as a Platform-reported consumption conflict whose Platform completion is unconfirmed.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1041-1048: Exercise the fresh-funding FFI mapping through the tested helper
The fresh BIP44 funding entry point now correctly preserves `AssetLockAlreadyConsumed`, but it implements a separate match from the tested resume mapper. The new unit test only calls `map_asset_lock_resume_result`, so it would still pass if this fresh path regressed to the generic `ErrorWalletOperation` mapping—the exact defect fixed by the final commit. Extract a shared asset-lock funding result mapper that accepts the operation-specific context, use it from both entry points, and test that shared mapping for the typed consumed report, unrelated errors, and success.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:216-242: Do not roll back after the FFI store has already committed the marker
(existing thread: https://github.com/dashpay/platform/pull/4357#discussion_r3752217867)
The fatal-flush case from the prior review is fixed, but the same durable-store rollback invariant still fails during `store()` itself. `FFIPersister::store` invokes `on_changeset_end_fn`, whose documented contract commits the host transaction, before invoking `on_store_fn`; if that later notification returns nonzero, `store()` returns a fatal error even though `RecoveredFromChain` is already durable. This branch treats every `store()` error as rollback-safe and restores the previous in-memory lock, leaving the live wallet behind the state that will be loaded after restart. Move the fallible store notification before the commit and incorporate it into `round_success`, or expose a persistence outcome that distinguishes pre-commit failures from post-commit notification failures. Add coverage for a successful end callback followed by a failing `on_store_fn`.
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
This doesn't really make sense to me, why are we only fixing asset lock submission errors when they are to sheilded pool, shouldn't we do it in all cases?
|
Also addressed the carried-forward post-commit |
|
@QuantumExplorer Agreed — the earlier scope explanation is no longer current. Addressed in b803e5f by moving reconciliation into the shared asset-lock orchestration layer and routing the final submit result through it for identity registration, identity top-up, platform-address funding, and shielded funding (including the InstantLock → ChainLock retry path). The handling remains conservative: only the structured exact-outpoint consumed report is reconciled, the lock is persisted as |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The three prior findings are fixed: bracketed FFI rounds now include the store notification in their rollback verdict, the public error wording states that Platform completion is unconfirmed, and fresh and resumed funding share the tested typed-result mapper. One persistence callback-contract gap remains: the fix changes the phase of a legacy callback while still being unable to guarantee rollback for supported callback configurations without a changeset-end boundary.
Source: reviewer backend model gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:2224-2244: Do not use the legacy store notification as the rollback boundary
Moving `on_store_fn` before `on_changeset_end_fn` makes failures rollback-safe only for hosts that provide an atomic end callback, while changing the observable phase of an established field in the unversioned public `PersistenceCallbacks` vtable. Existing external hosts previously received `on_store_fn` after the end callback had committed the round; they can now emit an irreversible notification while writes are still staged and may subsequently be rolled back. The new ordering also remains unsafe for a supported partially wired vtable with no `on_changeset_end_fn`: a per-kind callback may write immediately, then `on_store_fn` may fail, causing `store()` to claim that the changeset was rolled back and `mark_asset_lock_consumption_unknown` to restore the old in-memory entry even though the host already contains `RecoveredFromChain`. Preserve the legacy post-commit notification phase and add a distinct size/version-negotiated pre-commit callback or persistence outcome. The synchronous recovery path must require an atomic end boundary or otherwise distinguish committed, uncommitted, and indeterminate store failures before rolling back memory.
|
Addressed in 06265f5. I restored |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The PR correctly retains authenticated ChainLock evidence in nonterminal recovery state, flushes the recovery marker, and preserves the typed FFI result. One in-scope persistence-contract issue remains: supported legacy or partially wired FFI callback tables cannot reliably distinguish durable, rolled-back, and indeterminate asset-lock writes, so reconciliation can still leave live state inconsistent with restart state.
Source: reviewer backend model gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 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/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:2253-2277: Do not use the legacy store notification as the rollback boundary
Restoring `on_store_fn` to its legacy post-commit phase fixes the callback-ordering regression, but the synchronous recovery path still assumes `store()` can classify every result as either durable success or rollback-safe failure. That is not true for supported legacy and partially wired callback tables. Without `on_changeset_end_fn`, an asset-lock callback can write directly and then return an error; `store()` reports that the changeset was rolled back, `store_commits_inline()` returns false, and `mark_asset_lock_consumption_unknown` restores only the in-memory entry even though the host may already contain `RecoveredFromChain`. Conversely, an existing legacy host may use `on_store_fn` as its durable-write boundary; its previously propagated failure is now ignored, allowing reconciliation to succeed when that write failed. A table without `on_persist_asset_locks_fn` also silently skips the recovery row while returning success. Require an attested capability covering atomic asset-lock upsert and restart restoration before this reconciliation, or return an outcome that distinguishes committed, rolled-back, and indeterminate writes. Preserve the legacy `on_store_fn` failure contract for callback sets without a separate atomic commit boundary.
|
Addressed the new persistence-contract review in b81e364. The reconciliation path now requires an attested combination of atomic changesets, tracked asset-lock persistence, and wallet restoration before mutating the in-memory status. FFI derives the tracked-lock leg from the concrete callback, Swift/Kotlin declare it because they wire all required callbacks, and SQLite declares only tracked-lock persistence (it still cannot satisfy the full reconciliation contract without wallet restore). Legacy hosts without an atomic end callback also keep the original nonzero Validation:
The focused Swift test target is unavailable locally because the generated |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4357 +/- ##
=========================================
Coverage 87.63% 87.63%
=========================================
Files 2670 2670
Lines 339434 339478 +44
=========================================
+ Hits 297451 297495 +44
Misses 41983 41983
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact head fixes the prior persistence-contract issue: shielded asset-lock reconciliation now fails before mutating memory unless the backend attests atomic changesets, tracked asset-lock persistence, and wallet restoration, and the FFI backend intersects that declaration with the required concrete callbacks. The recovery marker is stored and flushed before acknowledgment, while legacy store-notification behavior is preserved; no in-scope findings remain. Source: reviewer backend model gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
|
@coderabbitai review |
✅ Action performedReview finished.
|
7d08996 to
e6241a5
Compare
3 participants
Summary
RecoveredFromChainwhile returning stable code 24ASSET_LOCK_RECONCILIATIONwithout changing capability bits or FFI/API typesTests
cargo test -p platform-wallet --features shielded --lib wallet::shielded::fund_from_asset_lock::testscargo test -p platform-wallet --lib wallet::asset_lock::orchestration::testscargo test -p platform-wallet-ffi --features shielded --lib persistence::testscargo test -p platform-wallet-ffi --features shielded --lib error::testscargo check -p platform-wallet --libcargo check -p platform-wallet --features shielded --libcargo check -p platform-wallet-storagepython3 scripts/check_sdk_parity_manifest.pypython3 -m unittest discover -s scripts/tests -p 'test_*.py'cargo fmt --all -- --checkcargo clippy -p platform-wallet -p platform-wallet-ffi --features shielded --lib --tests -- -D warningsFocused Swift source parsing passed. The local Swift unit target could not import the generated
DashSDKFFImodule, and the local Kotlin Gradle test could not configure without an Android SDK; CI covers the repository integration environment.Summary by CodeRabbit
Bug Fixes
Documentation