fix(swift-sdk): drop legacy headers pre-processing in build_ios.sh#3853
Open
ZocoLini wants to merge 1 commit into
Open
fix(swift-sdk): drop legacy headers pre-processing in build_ios.sh#3853ZocoLini wants to merge 1 commit into
ZocoLini wants to merge 1 commit into
Conversation
ZocoLini
requested review from
QuantumExplorer,
llbartekll and
shumkov
as code owners
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR migrates the Swift SDK's FFI layer from typed pointer casts ( ChangesFFI Opaque Pointer Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Collaborator
|
✅ Review complete (commit 848ebbd) |
ZocoLini
force-pushed
the
fix/opaque-types-usage
branch
from
324c337 to
1019c5c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swift (1)
426-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
orderByClauseand pagination in the no-filter path.When
whereClause == nil, this branch always sendsorder_by_json = nilandstart_at = 0, so callers cannot sort or page unless they also provide a filter. That makesdocumentList(...)return the wrong slice for a common case.🛠️ Minimal fix
} else { - var searchParams = DashSDKDocumentSearchParams() - searchParams.data_contract_handle = OpaquePointer(contractHandle) - searchParams.document_type = documentTypePtr.baseAddress - searchParams.where_json = nil - searchParams.order_by_json = nil - searchParams.limit = limit ?? 100 - searchParams.start_at = 0 - - return dash_sdk_document_search(handle, &searchParams) + let runSearch: (UnsafePointer<CChar>?) -> DashSDKResult = { orderByPtr in + var searchParams = DashSDKDocumentSearchParams() + searchParams.data_contract_handle = OpaquePointer(contractHandle) + searchParams.document_type = documentTypePtr.baseAddress + searchParams.where_json = nil + searchParams.order_by_json = orderByPtr + searchParams.limit = limit ?? 100 + if let startAt = startAt { + searchParams.start_at = UInt32(startAt) ?? 0 + } else if let startAfter = startAfter { + searchParams.start_at = (UInt32(startAfter) ?? 0) + 1 + } else { + searchParams.start_at = 0 + } + return dash_sdk_document_search(handle, &searchParams) + } + + if let orderByClause = orderByClauseCString { + return orderByClause.withUnsafeBufferPointer { runSearch($0.baseAddress) } + } else { + return runSearch(nil) + } }🤖 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/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swift` around lines 426 - 434, The no-filter branch currently forces searchParams.order_by_json = nil and searchParams.start_at = 0, dropping caller-provided sorting and pagination; instead preserve and assign the existing orderByClause and startAt values into searchParams (e.g., set searchParams.order_by_json from orderByClause's pointer and searchParams.start_at from the startAt variable) so dash_sdk_document_search receives the requested order and page when whereClause == nil; update the code around searchParams in PlatformQueryExtensions.swift (symbols: whereClause, orderByClause, startAt, limit, searchParams, dash_sdk_document_search) accordingly.packages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift (2)
1212-1368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
documentPurchaseoff the main actor before the blocking FFI calls.Unlike the surrounding transition methods, this one never hops to
DispatchQueue.global(). Because the enclosing extension is@MainActor, the contract fetch, document fetch, anddash_sdk_document_purchase_and_waitcall all run on the main actor and can freeze SwiftUI until the network returns.Suggested fix
- return try await withCheckedThrowingContinuation { continuation in - // Convert strings to C strings + return try await withCheckedThrowingContinuation { continuation in + DispatchQueue.global().async { [weak self] in + guard let self = self else { + continuation.resume(throwing: SDKError.invalidState("SDK not initialized")) + return + } + + // Convert strings to C strings guard let contractIdCString = contractId.cString(using: .utf8), let documentTypeCString = documentType.cString(using: .utf8), let documentIdCString = documentId.cString(using: .utf8), let purchaserIdCString = purchaserIdentity.id.toBase58String().cString(using: .utf8) else { continuation.resume(throwing: SDKError.serializationError("Failed to convert strings to C strings")) return } @@ - } + } + } }🤖 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/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift` around lines 1212 - 1368, The documentPurchase implementation is running blocking FFI calls on the `@MainActor` and can freeze the UI; move the blocking work (calls to dash_sdk_data_contract_fetch, dash_sdk_document_fetch, dash_sdk_document_purchase_and_wait and related heavy processing) off the main actor by executing them on a background thread (e.g., use Task.detached { ... } or DispatchQueue.global().async) and keep only the minimal UI-safe resume/return logic on the main actor (use DispatchQueue.main.async to resume the continuation or return the final purchasedDocInfo). Ensure you preserve the existing continuation.resume(returning:) / resume(throwing:) calls semantics and still destroy handles (dash_sdk_data_contract_destroy, dash_sdk_document_destroy, dash_sdk_identity_public_key_destroy, dash_sdk_error_free, dash_sdk_document_info_free) while off the main thread as needed.
1609-1619:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor the caller-supplied
keyIdin the token admin methods.These four methods accept
keyIdbut still pickownerIdentity.publicKeys.values.firstand then fetch that key’s handle. That makes signing depend on dictionary iteration order and ignores the key the caller explicitly selected, so freeze/unfreeze/burn/destroy-frozen-funds can run with the wrong authorization key.Suggested fix
- // TODO: We need to get the freezing key from the owner identity - // For now, we'll assume the first key is the freezing key - guard let freezingKey = ownerIdentity.publicKeys.values.first else { - continuation.resume(throwing: SDKError.invalidParameter("No public keys found in owner identity")) + guard ownerIdentity.publicKeys[keyId] != nil else { + continuation.resume(throwing: SDKError.invalidParameter("Public key \(keyId) not found in owner identity")) return } // Get the public key handle for the freezing key let keyHandleResult = dash_sdk_identity_get_public_key_by_id( ownerIdentityHandle, - UInt8(freezingKey.id) + UInt8(keyId) )Apply the same change in
tokenUnfreeze,tokenBurn, andtokenDestroyFrozenFunds.Also applies to: 1741-1751, 1863-1873, 1993-2003
🤖 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/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift` around lines 1609 - 1619, The token admin methods (tokenFreeze, tokenUnfreeze, tokenBurn, tokenDestroyFrozenFunds) currently ignore the caller-supplied keyId and use ownerIdentity.publicKeys.values.first; change each method to look up the public key by the provided keyId in ownerIdentity.publicKeys (instead of taking .values.first) and use that key's id when calling dash_sdk_identity_get_public_key_by_id (pass UInt8(keyId) / the selected key's id), returning the SDKError.invalidParameter if the requested keyId is not present; apply the same lookup fix in the methods referenced (also around the other occurrences at the ranges noted).
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift (1)
8-10: ⚡ Quick winUpdate the file comment to describe the current opaque-handle contract.
This wording still describes the legacy minimal-body / typed-pointer workaround, but this migration is explicitly moving the wrapper to
OpaquePointerand away from header preprocessing. Leaving the old explanation here makes the FFI boundary harder to reason about for future changes. As per coding guidelines,packages/swift-sdk/**/*.swiftshould use pointers for opaque FFI types in Swift C header imports.🤖 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/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift` around lines 8 - 10, Update the top-of-file comment in ManagedPlatformAccount.swift to describe the current opaque-handle contract: replace the legacy "minimal body / typed pointer" explanation with a clear statement that FFIManagedPlatformAccount is an opaque C type accessed from Swift via OpaquePointer, and document expectations for ownership/retention and that Swift uses OpaquePointer for FFI types per the packages/swift-sdk guideline; reference the FFIManagedPlatformAccount symbol and OpaquePointer usage so future readers know the intended boundary and pointer semantics.Source: Coding guidelines
🤖 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/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift`:
- Line 709: Replace erroneous calls to dash_sdk_document_destroy with
dash_sdk_document_free for any DocumentHandle returned from
dash_sdk_document_fetch, dash_sdk_document_replace_on_platform_and_wait, and
dash_sdk_document_purchase_and_wait; these APIs return a heap-allocated
Box<Document> and must be freed with dash_sdk_document_free (which does
Box::from_raw) rather than the borrow-only dash_sdk_document_destroy. Update the
cleanup sites in StateTransitionExtensions whereDocumentHandle cleanup currently
uses dash_sdk_document_destroy to call dash_sdk_document_free(...) instead, and
make the same change for the dash_sdk_document_destroy usage in
PlatformQueryExtensions (ensure you only change the cleanup for returned
handles, leaving actual “delete document” semantics separate).
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swift`:
- Around line 426-434: The no-filter branch currently forces
searchParams.order_by_json = nil and searchParams.start_at = 0, dropping
caller-provided sorting and pagination; instead preserve and assign the existing
orderByClause and startAt values into searchParams (e.g., set
searchParams.order_by_json from orderByClause's pointer and
searchParams.start_at from the startAt variable) so dash_sdk_document_search
receives the requested order and page when whereClause == nil; update the code
around searchParams in PlatformQueryExtensions.swift (symbols: whereClause,
orderByClause, startAt, limit, searchParams, dash_sdk_document_search)
accordingly.
In `@packages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift`:
- Around line 1212-1368: The documentPurchase implementation is running blocking
FFI calls on the `@MainActor` and can freeze the UI; move the blocking work (calls
to dash_sdk_data_contract_fetch, dash_sdk_document_fetch,
dash_sdk_document_purchase_and_wait and related heavy processing) off the main
actor by executing them on a background thread (e.g., use Task.detached { ... }
or DispatchQueue.global().async) and keep only the minimal UI-safe resume/return
logic on the main actor (use DispatchQueue.main.async to resume the continuation
or return the final purchasedDocInfo). Ensure you preserve the existing
continuation.resume(returning:) / resume(throwing:) calls semantics and still
destroy handles (dash_sdk_data_contract_destroy, dash_sdk_document_destroy,
dash_sdk_identity_public_key_destroy, dash_sdk_error_free,
dash_sdk_document_info_free) while off the main thread as needed.
- Around line 1609-1619: The token admin methods (tokenFreeze, tokenUnfreeze,
tokenBurn, tokenDestroyFrozenFunds) currently ignore the caller-supplied keyId
and use ownerIdentity.publicKeys.values.first; change each method to look up the
public key by the provided keyId in ownerIdentity.publicKeys (instead of taking
.values.first) and use that key's id when calling
dash_sdk_identity_get_public_key_by_id (pass UInt8(keyId) / the selected key's
id), returning the SDKError.invalidParameter if the requested keyId is not
present; apply the same lookup fix in the methods referenced (also around the
other occurrences at the ranges noted).
---
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift`:
- Around line 8-10: Update the top-of-file comment in
ManagedPlatformAccount.swift to describe the current opaque-handle contract:
replace the legacy "minimal body / typed pointer" explanation with a clear
statement that FFIManagedPlatformAccount is an opaque C type accessed from Swift
via OpaquePointer, and document expectations for ownership/retention and that
Swift uses OpaquePointer for FFI types per the packages/swift-sdk guideline;
reference the FFIManagedPlatformAccount symbol and OpaquePointer usage so future
readers know the intended boundary and pointer semantics.
🪄 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: 608b22e5-2a70-464d-bc53-5b620b38f27f
📒 Files selected for processing (25)
packages/swift-sdk/Sources/SwiftDashSDK/Address/AddressSyncService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Address/Addresses.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Account.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/AccountCollection.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/AddressPool.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/BLSAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/EdDSAAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccountCollection.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SimpleTransitionTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (1)
- packages/swift-sdk/build_ios.sh
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Pure build/FFI cleanup: removes the perl post-processing that gave cbindgen opaque struct typedefs fake bodies, then mechanically migrates Swift call sites from UnsafeMutablePointer to OpaquePointer. Verified the migration is ownership- and ABI-preserving at every changed call site. Only minor nits remain: one stale comment + redundant pointer round-trip, one dead force-unwrap, and a note that the public Swift handle types changed shape (source-breaking for any external Swift SDK consumers).
🟡 1 suggestion(s) | 💬 2 nitpick(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/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift:97: Public Swift handle type changes shape — call out in release notes
`KeychainSigner.handle` is `public` and its type changes from `UnsafeMutablePointer<SignerHandle>` to `OpaquePointer`; `SDK.handle` changes similarly. The previous typed pointer only existed because the build-script hack gave an opaque C typedef an artificial body, so external consumers couldn't have been dereferencing it — but anything that stored or annotated the handle by its old typed-pointer type now fails to compile. The PR title says `fix(swift-sdk):` and the checklist marks this as non-breaking; please call this surface change out in the release notes (or in the PR description) so downstream Swift SDK users aren't surprised at the next bump.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental delta from 324c337 to 1019c5c is a SwiftUI text refactor in CreateIdentityView.swift; no new FFI/consensus/security concerns. The PR's substantive change remains the OpaquePointer migration plus removal of the perl-based header rewrite in build_ios.sh. All three prior findings (one docs suggestion, two style nitpicks) are STILL VALID at the current head and are carried forward; no new latest-delta findings.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
ZocoLini
force-pushed
the
fix/opaque-types-usage
branch
from
1019c5c to
9c65521
Compare
ZocoLini
force-pushed
the
fix/opaque-types-usage
branch
3 times, most recently
from
7f7ad93 to
f89c490
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift (1)
8-9: ⚡ Quick winClarify the comment about opaque types and pointer usage.
The comment states "minimal body so Swift can use typed pointers (OpaquePointer)" which is contradictory. If the type is truly opaque, Swift doesn't see the body. The phrase "typed pointers (OpaquePointer)" is also confusing since
OpaquePointeris specifically an untyped pointer in Swift.Consider rephrasing to clearly explain the FFI contract, such as: "FFIManagedPlatformAccount is an opaque C type imported as OpaquePointer in Swift."
📝 Suggested comment revision
-// FFIManagedPlatformAccount is an opaque C type with a minimal body so Swift -// can use typed pointers (OpaquePointer). +// FFIManagedPlatformAccount is an opaque C type imported as OpaquePointer. +// The C header defines a minimal struct body to satisfy the compiler, but +// Swift treats the handle as opaque and never dereferences it.🤖 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/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift` around lines 8 - 9, Update the comment for FFIManagedPlatformAccount to accurately describe the FFI contract: state that FFIManagedPlatformAccount is an opaque C type (its definition is not visible to Swift) and is imported/used in Swift as an OpaquePointer rather than a typed pointer; reference FFIManagedPlatformAccount and usage sites that convert to or from OpaquePointer in ManagedPlatformAccount.swift so readers understand the pointer semantics.
🤖 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 @.github/workflows/swift-sdk-build.yml:
- Around line 89-100: The CI step named "One-time - force-regenerate FFI headers
(clear stale opaque typedefs)" must be made idempotent by adding a tracking
mechanism: modify that step to run only if a marker file (e.g.,
.github/ffi_headers_regenerated) does not exist, and on successful execution
create that marker file (commit or persist to runner workspace) so subsequent
jobs skip the rm -rf and cargo clean commands (rm -rf
packages/swift-sdk/DashSDKFFI.xcframework and cargo clean -p rs-unified-sdk-ffi
-p rs-sdk-ffi -p platform-wallet-ffi -p key-wallet-ffi -p dash-network); also
add a brief TODO/comment in the workflow with a link to a GitHub issue tracking
removal and an owner/target date.
---
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift`:
- Around line 8-9: Update the comment for FFIManagedPlatformAccount to
accurately describe the FFI contract: state that FFIManagedPlatformAccount is an
opaque C type (its definition is not visible to Swift) and is imported/used in
Swift as an OpaquePointer rather than a typed pointer; reference
FFIManagedPlatformAccount and usage sites that convert to or from OpaquePointer
in ManagedPlatformAccount.swift so readers understand the pointer semantics.
🪄 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: f13266e2-bd46-48db-9346-7dfb93f747a0
📒 Files selected for processing (26)
.github/workflows/swift-sdk-build.ymlpackages/swift-sdk/Sources/SwiftDashSDK/Address/AddressSyncService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Address/Addresses.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Account.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/AccountCollection.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/AddressPool.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/BLSAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/EdDSAAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccountCollection.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SimpleTransitionTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (1)
- packages/swift-sdk/build_ios.sh
✅ Files skipped from review due to trivial changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SimpleTransitionTests.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/BLSAccount.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/AddressPool.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Address/AddressSyncService.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccountCollection.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/WalletManagerStore.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/AccountCollection.swift
- packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Account.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Address/Addresses.swift
- packages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swift
- packages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Mechanical and consistent migration of Swift FFI wrappers from typed handle pointers to OpaquePointer, paired with removal of the build_ios.sh perl header-mangling hack. The only in-scope concerns are a CI step explicitly labeled 'one-time' that has no removal enforcement, and one stale comment left over from the migration.
🟡 1 suggestion(s) | 💬 1 nitpick(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 `.github/workflows/swift-sdk-build.yml`:
- [SUGGESTION] .github/workflows/swift-sdk-build.yml:89-100: "One-time" FFI-header regeneration step has no removal mechanism
The new step `One-time - force-regenerate FFI headers (clear stale opaque typedefs)` runs `rm -rf DashSDKFFI.xcframework` plus `cargo clean -p` on five FFI crates on every workflow invocation. The inline comment says "Remove this step after one green run," but nothing enforces that — once merged it will execute on every PR/push until someone remembers to delete it, permanently defeating cbindgen's incremental rebuild on the self-hosted runner that this workflow was designed around. That has two costs: material CI time added per run, and ironically, masking exactly the kind of header-staleness regression this PR was created to surface (developers will assume CI catches it because the cache is wiped). Either land the cleanup as a separate revert-after-merge PR, or gate the step (e.g., `if:` keyed on a one-shot flag or a single run number) so it cannot silently outlive its purpose.
f89c490 to
2c4dd2a
Compare
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "bbd493a6dd2d76bd1382f5cc25d5300e6b1ce263cbda38c11d7c5306396f2dce"
)Xcode manual integration:
|
ZocoLini
force-pushed
the
fix/opaque-types-usage
branch
from
2c4dd2a to
848ebbd
Compare
thepastaclaw
approved these changes
Jun 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
PR #3853 removes the legacy perl-based header post-processing in build_ios.sh and updates Swift call sites to consume cbindgen's regenerated opaque-pointer signatures directly. The latest delta is a small Swift cleanup that drops a redundant OpaquePointer rewrap and a stale comment. The prior nitpick about the // Convert OpaquePointer to OpaquePointer tautology in Addresses.swift is FIXED at head 848ebbd (lines deleted). No new in-scope defects found across general, security, and FFI agents; CodeRabbit supplied no inline findings.
QuantumExplorer
approved these changes
Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The build_ios.sh script was modifying the headers in place for compatibility after the rewrite I made months ago, with this PR I remove that logic to use the generated headers the way they were generated
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit