fix(swift-sdk): remove EstablishedContact's clone-mutating setters#4140
Conversation
managed_identity_get_established_contact hands out a .cloned() copy in the FFI handle table, so setAlias/clearAlias/setNote/clearNote/hide/ unhide mutate a detached clone: the change never reaches the wallet manager's contact state, is never persisted, and is silently lost when the handle is freed. dashwallet wired its contact-settings UI to these and shipped a "Saved" toast over a no-op. The real write path already exists (ManagedPlatformWallet .setDashPayContactInfo → platform_wallet_set_dashpay_contact_info_with_ signer → set_contact_info_with_external_signer: local state + persist + encrypted contactInfo document publish), and the example app uses it. Deprecate the Swift setters with a message pointing there, and document the handle-local semantics on the FFI functions. Getters stay — reading the clone is fine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesEstablished contact mutation guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 2aaef1e) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-ffi/src/established_contact.rs`:
- Around line 280-287: Remove the duplicated Handle-local only documentation
block in the unhide documentation, retaining exactly one copy of the four-line
comment and leaving the surrounding documentation unchanged.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f494a76-0443-4b69-807e-bc4ae860bf80
📒 Files selected for processing (2)
packages/rs-platform-wallet-ffi/src/established_contact.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/EstablishedContact.swift
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tright Nothing has shipped, so no deprecation window is owed: delete the six Swift setters (setAlias/clearAlias/setNote/clearNote/hide/unhide) and their FFI functions instead of deprecating them. They mutated the detached clone in the handle table — never wallet state, never disk, never Platform. Getters stay (reading the clone snapshot is fine); the real write path is ManagedPlatformWallet.setDashPayContactInfo, which dashwallet now uses (dashwallet-ios #834) and the example app always did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR cleanly removes the six clone-mutating EstablishedContact setters/wrappers (setAlias/clearAlias/setNote/clearNote/hide/unhide) from both the Rust FFI and Swift layers in matched pairs, with no orphaned declarations or callers on either side of the boundary. Two checked-in API documents (Swift PlatformWallet README and rs-platform-wallet-ffi IMPLEMENTATION_SUMMARY.md) still advertise the removed methods/exports and should be updated to stay in sync with the actual surface.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— general (completed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— ffi-engineer (failed),claude-sonnet-5— general (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/README.md`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/README.md:334-335: Swift API documentation still advertises the removed contact mutators
This PR deletes `setAlias`, `clearAlias`, `setNote`, `clearNote`, `hide`, and `unhide` from `EstablishedContact`, but this public API reference still declares them (lines 334-335, 356-357, 378, 382) and demonstrates them in runnable examples (lines 341, 349, 362, 364, 510-514). Verified directly: none of these six methods exist anywhere in the current `EstablishedContact.swift`. These examples no longer compile and continue steering consumers toward the detached-snapshot write model this PR removes. Update the doc to show only the remaining getters (`getAlias`, `getNote`, `isHidden`) and demonstrate persistent writes through `ManagedPlatformWallet.setDashPayContactInfo(identityId:contactId:alias:note:hidden:signer:)`.
Summary
managed_identity_get_established_contactinserts a.cloned()copy of the contact into the FFI handle table, soEstablishedContact.setAlias / clearAlias / setNote / clearNote / hide / unhidemutated a detached clone: the change never reached the wallet manager's contact state, was never persisted, and was silently lost when the handle was freed.Found via dashwallet QA: the contact-settings sheet was wired to these setters and showed a "Saved" toast over a complete no-op (empty
contactAlias/contactNotemirror columns, nothing published).Since nothing has shipped, the setters are removed outright (Swift wrappers + the six FFI functions) rather than deprecated — no consumer exists: dashwallet is rewired to the correct API in dashwallet-ios#834 and SwiftExampleApp always used it.
The correct write path is untouched and already complete:
ManagedPlatformWallet.setDashPayContactInfo→platform_wallet_set_dashpay_contact_info_with_signer→set_contact_info_with_external_signer(local state first + persist + encrypted DIP-15contactInfodocument publish, with published / deferred / skipped outcome).Getters (
getAlias,getNote,isHidden, …) stay — reading the clone snapshot is fine.Test plan
cargo check -p platform-wallet-fficleandashpayscheme builds against this branch (compiles swift-sdk from source; no remaining references)🤖 Generated with Claude Code