fix: restore four Swift-parity guards in the Kotlin SDK, plus two DPNS marketplace defects - #4423
fix: restore four Swift-parity guards in the Kotlin SDK, plus two DPNS marketplace defects#4423bfoss765 wants to merge 4 commits into
Conversation
…er got Each of these landed in the Swift SDK through review and was never ported to Kotlin, so the Android persister diverged from the reference implementation. 1. Asset-lock Consumed (4) is terminal again. `onPersistAssetLockUpsert` did a plain Room upsert and `onPersistAssetLockRemoval` an unconditional delete, both last-write-wins. The wallet-event adapter's batched drain can deliver a stale reconstruction snapshot AFTER the live flow's consumption write, which silently regressed a consumed lock. Now a stored consumed row is never overwritten by a non-consumed write and never deleted by a non-consumed removal — mirroring PlatformWalletPersistenceHandler.swift:270 and :310. Non-terminal statuses stay last-write-wins in both directions. 2. Split isOwned from the marketplace columns. The identity-snapshot sweep deleted departed DPNS rows outright, and its canonical branch wrote saleStatusRaw = 0 / counterpartyIdentityId = null over live marketplace state. A name that departed in a round the marketplace pass could not classify lost its sale history permanently — Android-only; Swift never did this. The sweep now marks isOwned = false and deletes only when documentId is null (a pure label-cache row); the canonical branch refreshes only acquiredAt/label and carries every marketplace field through. Mirrors upsertDPNSNames (swift:1912-1941). 3. isLocal is promoted, not hardcoded. Every persister-created row got a constant false, so a wallet's own identities — always local — were mis-marked. The flag is now promoted whenever the row carries a wallet link (one-way: nothing writes false over true), plus a promote-only, idempotent load-path heal for rows already written by the old code. Mirrors swift:1827 and healIdentityIsLocalFlags (swift:4688, called from loadWalletList :4719). Safe on Android precisely because the Kotlin persister never mislinked walletId — "has a wallet link" is exactly "is wallet-owned". The example app's manual-add is corrected to isLocal = true to match owner semantics. 4. Watch-only clears a stale seedMismatch. `unlockWalletFromKeystore` returned on the no-mnemonic check before any status update, so a wallet whose seed failed to bind and whose Keystore entry was then removed kept publishing the unlock banner for a seed that is no longer there. Mirrors PlatformWalletManager.swift:764-770. Tests: 16 added, :sdk:testDebugUnitTest green at 325. Verified as real regression coverage — reverting the four fixes fails exactly the 10 tests that assert changed behavior, while the 6 that pin unchanged behavior (guard narrowness, cache-row deletion, observed identities) keep passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t zero-price listings Two defects in the DPNS marketplace lane, both surfaced by the same audit. **F1 — orphaned marketplace row after a restart.** `resolve_departed_name` derived a departed name's `previous_document_id` solely from the in-memory `dpns_name_states` map. That map is session-scoped: the load path builds it EMPTY on every process start and nothing rehydrates it. So a departure observed in the first sync pass after a restart resolved no document id, emitted no removal delta — and still deleted the label. Since the label is what triggers departure detection, no later pass ever revisited it: the host mirror (Swift PersistentDPNSName, the Android Room dpns_names table) kept a stale owned/listed row for a name the wallet no longer holds, permanently. The lookup now falls back to the durable mirror via a new defaulted `PlatformWalletPersistence::get_dpns_name_state`, following the same `Ok(None)`-default shape as `get_core_tx_record` / `list_wallet_core_txids`, so no existing backend breaks. `SqlitePersister` overrides it with a real reader scoped on all three of wallet, identity and normalized label — `Sold`/`Transferred` rows are retained here, so dropping the identity predicate could remove another identity's document. A persistence read failure degrades to today's behaviour rather than aborting the departure. Both defective arms are fixed at once: the `Ok(None)` arm and the summary in the `Err` arm shared the same resolution. **Zero-price listings.** `set_dpns_name_price` never validated `price != 0`. Consensus would accept the listing and anyone could then take the name for free. Rejected now with a typed `InvalidParameter` as the first statement — ahead of the operation gate and any network round-trip. `purchase_dpns_name` likewise refuses a `Some(0)` listing, checked before the `expected_price` comparison so the caller is told the listing is invalid rather than that the price moved. Every `> 0` path is unchanged. Also in this commit, from review of the above: - `read_all` moved its SQL into a `format!` as part of sharing the row projection with the new reader, which silently escaped the TC-P1-003 `prepare_cached` lint (its allow-list probe only inspects the three lines starting at the `.prepare(` call). Switched to `prepare_cached` and dropped the now-dead allow-list exemption rather than widen it — a stale exemption would later mask a genuinely uncached writer. - Added the missing coverage for the SQLite reader itself; the marketplace tests exercise it through a test double, which proves the wiring but not the SQL. No migration for a dedicated index: SQLite serves this from the `(wallet_id, document_id)` primary key, scanning one wallet's rows, at most once per departed name per pass. Adding one would bump max_supported_version and trip the forward-version gate. Tests: platform-wallet 855 passed / 0 failed; platform-wallet-storage 336 passed / 0 failed (doctests included). fmt and clippy (-D warnings) clean on both crates. Non-vacuity checked by disabling the persister branch: exactly the 4 fallback-dependent tests fail, including the end-to-end one through `resolve_departed_name`, while the steady-state and unwired-backend tests correctly keep passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 14 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Opus deferred (commit 0311298) |
…the unit under test The item-4 fix had no regression test with teeth: `isGenuineWatchOnly` took a pre-computed Boolean and a bare clear lambda, so WatchOnlySeedMismatchTest exercised the helper against itself while the real guard sequence — storage read, status-key derivation, the seedMismatch clear, the early return — lived untested at the call site. Reverting the call site to the pre-fix `if (!walletStorage.hasMnemonic(walletId)) return false` kept the whole suite green. The seam now IS the sequence: `isGenuineWatchOnly` takes the wallet id, the storage existence probe, and the manager's status-map updater, and performs the read, the hex-key derivation, the clearing transform, and the verdict itself. The call site in `unlockWalletFromKeystore` is pure delegation. The rewritten tests drive the seam through a fake probe and a manager-shaped status map and pin the ordering (clear BEFORE the watch-only return; stored mnemonic touches nothing). Mutation-verified: transplanting the pre-fix call-site shape into the seam (watch-only verdict with no clear) fails 2 of the 3 tests; the stored-mnemonic test, which pins unchanged behavior, keeps passing. Full suite 325/0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…departed-name fallback yet The item-5 narration overstated its reach. `get_dpns_name_state` is overridden only by `SqlitePersister`; `FFIPersister` keeps the `Ok(None)` default and the persistence vtable has NO read slot for the lookup — there is no callback a host could set. So the restart orphan the fallback exists to close is still live on both mobile hosts (the Android Room and iOS SwiftData mirrors) until a `get_dpns_name_state` read callback lands with the batched vtable/ABI additions. Corrects the docs on `previous_document_id_for`, the trait method, and `resolve_departed_name` to state that scope instead of implying the durable fallback reaches every host mirror. Docs only — no code change; `cargo check -p platform-wallet` and `cargo fmt --check` clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Two corrections on my own claims in this PR, both pushed (0311298). Item 4's test evidence was circular. The watch-only test exercised the extracted helper against itself: reverting the production call site to the pre-fix Item 5's description overstated its reach. The |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Kotlin parity changes and zero-price guards are generally well scoped, but the new SQLite departed-name recovery can still orphan the current row when multiple historical documents match, and it permanently loses recovery after a transient persistence-read failure. The production SQLite decoder also needs checked timestamp conversions, and the purchase-side zero-price ordering lacks direct regression coverage.
Source: reviewers codex general/security-auditor/rust-quality (backend model gpt-5.6-sol); final verifier codex (backend model 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— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 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-storage/src/sqlite/schema/dpns_name_states.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/dpns_name_states.rs:249-252: Select the current retained row deterministically
The schema does not enforce uniqueness for `(wallet_id, identity_id, normalized_label)`; its primary key is `(wallet_id, document_id)`. DPNS domain documents can be deleted and re-registered with a new document ID, while this table deliberately retains earlier `Sold` and `Transferred` rows. A wallet identity can therefore have an old historical row and a newer owned row for the same normalized label. This unordered `LIMIT 1` may select the old row according to primary-key scan order. If the current document is then deleted and the fallback runs after restart, the sync removes the historical row and the identity label while leaving the current persisted row permanently orphaned. Prefer an `Owned` row, then the most recently synchronized row, and update the trait documentation that currently says any matching row is acceptable.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/dpns_name_states.rs:211-216: Use checked conversions for persisted timestamps
Only `price` has a non-negative schema constraint; the four timestamp columns permit negative SQLite integers. These `as u64` casts silently turn a malformed or externally modified `-1` into `u64::MAX`, despite the storage crate's explicit rule that durable-boundary casts use `safe_cast`. This decoder is now used by a production persistence lookup rather than only the test-gated whole-table helper. Decode every signed database value through `i64_to_u64` so malformed rows return the existing typed `IntegerOverflow` error.
In `packages/rs-platform-wallet/src/wallet/identity/network/dpns_marketplace.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/dpns_marketplace.rs:437-446: Retry departed-name recovery after persistence read failures
`Ok(None)` means that a backend does not support this lookup or has no row, while `Err` means an attempted persistence read failed; this branch collapses those distinct outcomes. If SQLite returns a transient read error and the subsequent Platform lookup confirms that the domain document is absent, `resolve_departed_name` receives no previous document ID and sets `retry` to false. The caller then removes the label without emitting a row-removal delta. Because that label is the trigger for future departure detection, the durable row is permanently orphaned. `PersistenceError` already preserves transient classification, and this flow already retains pending departures for retryable network errors. Propagate the persistence lookup result into `resolve_departed_name` and retain the pending departure on a transient error instead of treating it as an unsupported lookup.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/dpns_marketplace.rs:1168-1179: Add direct coverage for purchase-side zero-price rejection
The added tests exercise `set_dpns_name_price(0)` and its non-zero path, but no test reaches this separate guard in `purchase_dpns_name`. Add a regression case whose fetched domain state has `price = Some(0)` and whose `expected_price` is non-zero. It should assert `InvalidParameter`, rather than `DocumentPriceChanged`, and verify that signing and broadcast are not reached. This pins the ordering that the new code and API documentation explicitly promise.
| let sql = format!( | ||
| "SELECT {ROW_PROJECTION} FROM dpns_name_states \ | ||
| WHERE wallet_id = ?1 AND identity_id = ?2 AND normalized_label = ?3 LIMIT 1" | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Select the current retained row deterministically
The schema does not enforce uniqueness for (wallet_id, identity_id, normalized_label); its primary key is (wallet_id, document_id). DPNS domain documents can be deleted and re-registered with a new document ID, while this table deliberately retains earlier Sold and Transferred rows. A wallet identity can therefore have an old historical row and a newer owned row for the same normalized label. This unordered LIMIT 1 may select the old row according to primary-key scan order. If the current document is then deleted and the fallback runs after restart, the sync removes the historical row and the identity label while leaving the current persisted row permanently orphaned. Prefer an Owned row, then the most recently synchronized row, and update the trait documentation that currently says any matching row is acceptable.
| let sql = format!( | |
| "SELECT {ROW_PROJECTION} FROM dpns_name_states \ | |
| WHERE wallet_id = ?1 AND identity_id = ?2 AND normalized_label = ?3 LIMIT 1" | |
| ); | |
| let sql = format!( | |
| "SELECT {ROW_PROJECTION} FROM dpns_name_states \ | |
| WHERE wallet_id = ?1 AND identity_id = ?2 AND normalized_label = ?3 \ | |
| ORDER BY CASE status WHEN 'owned' THEN 0 ELSE 1 END, \ | |
| last_synced_at_ms DESC, document_id DESC \ | |
| LIMIT 1" | |
| ); |
source: ['codex']
| match persister.get_dpns_name_state(identity_id, &normalized_label) { | ||
| Ok(row) => row.map(|entry| entry.document_id), | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| identity = %identity_id, | ||
| name = label, | ||
| "persisted DPNS row lookup failed for a departed name; the host mirror may \ | ||
| keep a stale row for it until the name is re-acquired: {error}" | ||
| ); | ||
| None |
There was a problem hiding this comment.
🔴 Blocking: Retry departed-name recovery after persistence read failures
Ok(None) means that a backend does not support this lookup or has no row, while Err means an attempted persistence read failed; this branch collapses those distinct outcomes. If SQLite returns a transient read error and the subsequent Platform lookup confirms that the domain document is absent, resolve_departed_name receives no previous document ID and sets retry to false. The caller then removes the label without emitting a row-removal delta. Because that label is the trigger for future departure detection, the durable row is permanently orphaned. PersistenceError already preserves transient classification, and this flow already retains pending departures for retryable network errors. Propagate the persistence lookup result into resolve_departed_name and retain the pending departure on a transient error instead of treating it as an unsupported lookup.
source: ['codex']
| price: price.map(|p| p as u64), | ||
| status: status_from_columns(&status, counterparty)?, | ||
| created_at_ms: created_at.map(|v| v as u64), | ||
| updated_at_ms: updated_at.map(|v| v as u64), | ||
| transferred_at_ms: transferred_at.map(|v| v as u64), | ||
| last_synced_at_ms: last_synced as u64, |
There was a problem hiding this comment.
🟡 Suggestion: Use checked conversions for persisted timestamps
Only price has a non-negative schema constraint; the four timestamp columns permit negative SQLite integers. These as u64 casts silently turn a malformed or externally modified -1 into u64::MAX, despite the storage crate's explicit rule that durable-boundary casts use safe_cast. This decoder is now used by a production persistence lookup rather than only the test-gated whole-table helper. Decode every signed database value through i64_to_u64 so malformed rows return the existing typed IntegerOverflow error.
| price: price.map(|p| p as u64), | |
| status: status_from_columns(&status, counterparty)?, | |
| created_at_ms: created_at.map(|v| v as u64), | |
| updated_at_ms: updated_at.map(|v| v as u64), | |
| transferred_at_ms: transferred_at.map(|v| v as u64), | |
| last_synced_at_ms: last_synced as u64, | |
| price: price | |
| .map(|value| { | |
| crate::sqlite::util::safe_cast::i64_to_u64("dpns_name_states.price", value) | |
| }) | |
| .transpose()?, | |
| status: status_from_columns(&status, counterparty)?, | |
| created_at_ms: created_at | |
| .map(|value| { | |
| crate::sqlite::util::safe_cast::i64_to_u64( | |
| "dpns_name_states.created_at_ms", | |
| value, | |
| ) | |
| }) | |
| .transpose()?, | |
| updated_at_ms: updated_at | |
| .map(|value| { | |
| crate::sqlite::util::safe_cast::i64_to_u64( | |
| "dpns_name_states.updated_at_ms", | |
| value, | |
| ) | |
| }) | |
| .transpose()?, | |
| transferred_at_ms: transferred_at | |
| .map(|value| { | |
| crate::sqlite::util::safe_cast::i64_to_u64( | |
| "dpns_name_states.transferred_at_ms", | |
| value, | |
| ) | |
| }) | |
| .transpose()?, | |
| last_synced_at_ms: crate::sqlite::util::safe_cast::i64_to_u64( | |
| "dpns_name_states.last_synced_at_ms", | |
| last_synced, | |
| )?, |
source: ['codex']
| // A `$price` of 0 is not a listing this wallet will act on — see | ||
| // `set_dpns_name_price`, which refuses to create one. Rejected | ||
| // ahead of the `expected_price` comparison so the caller is told | ||
| // the listing itself is not purchasable rather than that the | ||
| // price moved. Only `== 0` is affected; every `> 0` listing | ||
| // follows the unchanged price-match path below. | ||
| if listed_price == 0 { | ||
| return Err(PlatformWalletError::InvalidParameter(format!( | ||
| "DPNS name {name:?} carries a listed price of 0 credits, which is not a \ | ||
| valid sale listing and will not be purchased by this wallet" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add direct coverage for purchase-side zero-price rejection
The added tests exercise set_dpns_name_price(0) and its non-zero path, but no test reaches this separate guard in purchase_dpns_name. Add a regression case whose fetched domain state has price = Some(0) and whose expected_price is non-zero. It should assert InvalidParameter, rather than DocumentPriceChanged, and verify that signing and broadcast are not reached. This pins the ordering that the new code and API documentation explicitly promise.
source: ['codex']
2 participants
Six audit-verified fixes in one batch.
Items 1-4 are Swift-parity restorations: each landed in the Swift SDK through review and was never ported to Kotlin, so the Android persister has been running without a guard its own reference implementation has. Every one cites the Swift code it mirrors.
Items 5-6 are two adjacent defects in the marketplace lane (Rust) surfaced by the same audit. Item 6 is fully fixed; item 5 lands the Rust plumbing and the SQLite backend but does not yet reach the mobile hosts — see its section.
1. Consumed asset locks are terminal again
Swift reference:
PlatformWalletPersistenceHandler.swift:270(upsert),:310(removal)onPersistAssetLockUpsertdid a plain Room@UpsertandonPersistAssetLockRemovalan unconditional delete — both last-write-wins. The writers race: the wallet-event adapter's batched drain can deliver a stale reconstruction/enrichment snapshot after the live flow's synchronous consumption write, silently regressing a consumed lock. Swift guards both sides; Kotlin did not.A stored
Consumed(4) row is now never overwritten by a non-consumed write and never deleted by a non-consumed removal. The guard is deliberately narrow — non-terminal statuses legitimately move both ways and stay last-write-wins.2.
isOwned/ marketplace-column authority splitSwift reference:
upsertDPNSNames,PlatformWalletPersistenceHandler.swift:1912-1941Two halves, both restored:
isOwned = falseand deletes only whendocumentId == null(a pure label-cache row). This is the audit's Android-only permanent-destruction case: a name that departed during a round the marketplace pass could not classify lost the only record of where it went. Swift never did this.saleStatusRaw = 0andcounterpartyIdentityId = nullover live marketplace state on every identity flush. Swift refreshes onlyacquiredAt/label(plusisOwned = true).The identity snapshot's authority stops at
isOwned; the marketplace columns belong to the reconciliation lane.3.
isLocalpromotion + startup healSwift reference:
row.isLocal = trueon wallet linkage (swift:1827);healIdentityIsLocalFlags()(swift:4688, called fromloadWalletList:4719)The persister hardcoded
isLocal = falseon every row it created, so a wallet's own identities — which are always local — were mis-marked, with no promotion on wallet linkage and no heal for rows already written.falseover atrue.UPDATE identities SET isLocal = 1 WHERE walletId IS NOT NULL AND isLocal = 0) runs from the load path — the one guaranteed per-launch pass over the store — and is skipped while a changeset round is open.The heal is safe on Android precisely because the Kotlin persister never mislinked
walletId. It only ever writes the link the FFI entry declared, so "has a wallet link" is exactly "is wallet-owned" — the condition cannot over-promote.Also corrects the example app's
LoadIdentityScreenmanual-add toisLocal = true: a manual add is an identity the owner deliberately tracks, which is what the flag means. It carries nowalletId, so neither the promotion nor the heal can reach it — that write is the only thing that can set it.4. Watch-only clears a stale seed-mismatch flag
Swift reference:
PlatformWalletManager.swift:764-770unlockWalletFromKeystorereturned on thehasMnemoniccheck before any status update. A wallet whose seed failed to bind and whose Keystore entry was then removed kept publishing the unlock banner forever for a seed that is no longer there — nothing downstream of the early return could clear it. No mnemonic is not a mismatch.Ordering is the whole contract, so the ENTIRE guard sequence — the
hasMnemonicstorage read, the hex status-key derivation, theseedMismatch = falsetransform, and the early-return verdict — lives in aninternalseam (isGenuineWatchOnly; the file's established pattern for native-free unit tests — cf.initializePlatformWalletNativeManager,decodeShieldedCreatePayload), and the call site inunlockWalletFromKeystoreis pure delegation. The tests drive the seam through a fake storage probe and a manager-shaped status map and assert the clear provably runs before the watch-only return. An earlier cut of this fix extracted only a Boolean-taking helper, which left the real call-site sequence untested — see the corrected non-vacuity note below.5. Orphaned marketplace row after a restart (audit F1) — Rust-side plumbing + SQLite backend; mobile-host wiring pending the persistence-vtable batch — the mobile orphan is NOT yet fixed
resolve_departed_namederived a departed name'sprevious_document_idsolely from the in-memorydpns_name_statesmap. That map is session-scoped: the load path builds it EMPTY on every process start and nothing rehydrates it. So a departure observed in the first sync pass after a restart resolved no document id, emitted no removal delta — and still deleted the label. Since the label is what triggers departure detection, no later pass ever revisited it, and the host mirror kept a stale owned/listed row for a name the wallet no longer holds, permanently.The lookup now falls back to the durable mirror through a new defaulted
PlatformWalletPersistence::get_dpns_name_state, following the sameOk(None)-default shape as the existingget_core_tx_record/list_wallet_core_txids, so no existing backend breaks.SqlitePersisteroverrides it with a real reader scoped on all three of wallet, identity and normalized label —Sold/Transferredrows are retained in that table, so dropping the identity predicate could remove another identity's document. A persistence read failure degrades to today's behaviour rather than aborting the departure.Scope — what this does and does not fix. The fallback reaches only backends that implement the read, and today that is
SqlitePersisteralone.FFIPersisterkeeps theOk(None)default, and the persistence vtable has no read slot for this lookup — there is no callback a host could set. So the Android Room and iOS SwiftData mirrors — the two hosts this section's orphan narrative describes — still resolve nothing after a restart, and their orphan is still live. On Android, combined with item 2's keep-as-history sweep, it now renders as a permanent "Owned · not listed" card in the marketplace list (see item 2's note). Closing it requires aget_dpns_name_stateread callback in the persistence vtable, which is deliberately deferred to the batched vtable/ABI additions rather than shipped piecemeal here; the in-code docs onprevious_document_id_forand the trait method state this scope plainly.Both defective arms are fixed at once on implementing backends: the
Ok(None)arm and the summary in theErrarm shared the same resolution.No migration for a dedicated index — SQLite serves this from the
(wallet_id, document_id)primary key, scanning one wallet's rows, at most once per departed name per pass. Adding one would bumpmax_supported_versionand trip the forward-version gate.6. Zero-price listing guard
set_dpns_name_pricenever validatedprice != 0. Consensus would accept the listing and anyone could then take the name for free. It is now rejected with a typedInvalidParameteras the first statement — ahead of the operation gate and any network round-trip.purchase_dpns_namelikewise refuses aSome(0)listing, checked before theexpected_pricecomparison so the caller is told the listing is invalid rather than that the price moved. Every> 0path is unchanged.There is no legitimate zero-price caller: a deliberate free handover is
transfer_dpns_name, which names the recipient.Test evidence
:sdk:testDebugUnitTest(Kotlin)cargo test -p platform-wallet --features shieldedcargo test -p platform-wallet-storagecargo fmt --check(both crates)cargo clippy --all-targets -- -D warnings(both crates)Non-vacuity was verified rather than assumed — a green suite proves nothing if the tests don't gate the fix:
if (!walletStorage.hasMnemonic(walletId)) return false— the actual regression — kept the whole suite green (verified, 325/0). That gap is closed: the guard sequence now lives whole in theisGenuineWatchOnlyseam, the call site is pure delegation, and the mutation was re-run against the seam — transplanting the pre-fix call-site shape into it fails 2 of the 3 watch-only tests, while the stored-mnemonic test (pinning unchanged behavior) keeps passing. The one line JVM tests still cannot see is the delegation call itself, which now carries no logic.resolve_departed_name_recovers_the_document_id_from_the_persister, which runs the real async fn on a realIdentityWalletand so proves the fallback is wired into production rather than merely reachable as a helper. The steady-state test (..._without_reading_the_persister) and the unwired-backend test correctly keep passing. These exercise the SQLite-backed path; there is no FFI-host equivalent to test because no FFI read slot exists yet (see item 5's scope).Also in scope, from reviewing the above
read_allmoved its SQL into aformat!as part of sharing the row projection with the new reader, which silently escaped the TC-P1-003prepare_cachedlint — its allow-list probe only inspects the three lines starting at the.prepare(call. Switched toprepare_cachedand dropped the now-dead allow-list exemption rather than widening it; a stale exemption would later mask a genuinely uncached writer.Reviewer notes
documentId, a shape the old Kotlin condition skipped entirely, leaving them to leak afteronRemoveDpnsNameStatenulls the marketplace columns. This is Swift's exact behavior, so it is parity, but it is worth a look.get_dpns_name_stateread callback.@QueryUPDATE, andIdentityEntityis untouched.