feat(swift-sdk): contracts tab, identity tokens, owner relationship#3544
Conversation
SwiftExampleApp:
- Tab 4 is now Contracts. Unified search bar accepts a contract id, a
token id (each in hex/base58/base64), or a keyword. Token-id paths
resolve via dash_sdk_token_get_contract_info; keyword paths query
the platform's keyword-search-contract for contractKeywords docs.
- Identity detail: new DashPay link to the friends screen scoped to
this identity (Friends is no longer a tab). New Tokens section under
DPNS Names showing every token the identity holds + refresh button.
- Contract detail: new Documents and Groups sections; owner row
becomes a NavigationLink into the owner's IdentityDetailView when
the owner identity is held locally.
- New flat "All Tokens" section on the Contracts tab listing every
PersistentToken across saved contracts.
SwiftDashSDK:
- New PersistentDataContract.ownerIdentity <-> PersistentIdentity
.ownedDataContracts SwiftData relationship (.nullify). The raw
ownerId scalar stays canonical; the relationship is back-filled
by ContractIdentityLinker at every model insertion site.
- ContractDownloader centralises fetch + parse + persist so the
search bar's tap-to-add and the manual paste sheet share one path.
- DataContractParser now writes groupsData (was previously never
populated, leaving the new Groups UI empty even for contracts that
defined groups).
- Swift wrapper SDK.calculateTokenId(contractId:position:).
rs-sdk-ffi:
- New dash_sdk_calculate_token_id(contract_id, position) -> base58
pure protocol-formula bridge over dpp::tokens::calculate_token_id
so Swift doesn't have to mirror the
double_sha256("dash_token" || cid || u16_be(pos)) derivation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds FFI APIs and Rust wallet methods to create data contracts with external signers, derive single-slot ECDSA identity keys (resolver variant), and top up identities from signed address inputs; expands derivation helpers, updates FFI/Swift bindings, and implements Swift persistence, UI, and contract linking. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Swift App
participant FFI as PlatformWallet FFI
participant Worker as Platform Wallet Worker Runtime
participant Wallet as IdentityWallet
participant Platform as Platform Network
App->>FFI: platform_wallet_create_data_contract_with_signer(owner_id, docs_json, ..., signer_handle, out_contract_id)
FFI->>FFI: validate pointers, decode C strings and identifier
FFI->>Worker: block_on_worker(async move { IdentityWallet::create_data_contract_with_signer(...) })
Worker->>Wallet: invoke create_data_contract_with_signer(owner, jsons, &VTableSigner)
Wallet->>Wallet: select owner key, assemble JSON, compute id, sign transition
Wallet->>Platform: put_to_platform_and_wait_for_response(serialized_transition)
Platform-->>Wallet: confirmation/result
Wallet-->>Worker: return DataContract / error
Worker-->>FFI: return result (contract id bytes or error)
FFI->>App: write out_contract_id, return Success or error code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Review GateCommit:
|
|
✅ 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: "2dd3390db3d90becb958f45c1db87d3109945b1f3275138d92be325cebad2190"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all findings against source. One blocking issue confirmed: the new FriendsView NavigationLink is unconditionally exposed in IdentityDetailView for identities without a wallet, where FriendsView hard-fails and shows a misleading empty state. Two converging suggestions about network-scope leakage (Contracts tab + token-balance refresh) are real and should be fixed. One real Rust→Swift heap-buffer leak in the new ContractDownloader path. One nitpick about a misleading 'indexed' doc comment, plus two lower-confidence suggestions retained.
Reviewed commit: d2ba9f4
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
💬 nitpick: parseDataContract mutates ownerId without invoking ContractIdentityLinker
packages/swift-sdk/Sources/SwiftDashSDK/Core/Utils/DataContractParser.swift (lines 35-38)
parseDataContract writes existingContract.ownerId = ownerIdData on an already-persisted row but does not call ContractIdentityLinker.linkContractToOwner afterward. The linker's docstring (Persistence/ContractIdentityLinker.swift:21-22) explicitly says it must be called 'right after a PersistentDataContract row is inserted or its ownerId is updated.' The only current caller of this update path (ContractDownloader.downloadAndPersistContract) invokes the linker manually after parseDataContract returns, so this is not an active bug — but the guarantee is now caller-enforced rather than parser-enforced, and any future call site that re-parses an existing row whose owner has changed will silently break the back-fill. Either move the linker call inside parseDataContract after the ownerId mutation, or tighten the linker's docstring to scope the contract to insertion sites only.
🤖 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/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- [BLOCKING] lines 131-135: Friends drill-in is exposed for identities with no wallet backing
`IdentityDetailView` unconditionally renders a `NavigationLink` into `FriendsView(identity: identity)` inside the new `Section("DashPay")`. `FriendsView.loadFriends()` immediately calls `requireWallet(for:)`, which throws when `identity.wallet?.walletId` is nil — the normal case for any identity that was fetched from the network rather than loaded from one of the user's local wallets (`isLocal == false`, or local-but-unassociated). On that path the failure is stored into `@State errorMessage` but the view body never renders it: the user lands on the empty `"No Friends Yet"` placeholder, the `Add Friend` button opens `AddFriendView` which will fail the same way, and the action buttons are inert. Either gate the NavigationLink (`if identity.wallet != nil`), surface the `errorMessage` in `FriendsView`'s body, or give `FriendsView` a real read-only mode for unowned identities.
- [SUGGESTION] lines 848-871: reloadTokenBalances iterates every PersistentToken regardless of network
`reloadTokenBalances` issues a bare `FetchDescriptor<PersistentToken>()` with no predicate, then derives canonical token ids and submits them to `getIdentityTokenBalances` on the currently-bound network. `PersistentToken` has no own network field; network is stored on its parent `PersistentDataContract.networkRaw` (and `PersistentDataContract` already exposes `predicate(network:)` / `contractsWithTokensPredicate(network:)` for this exact purpose). After a network switch — or any time the user has cached contracts on more than one network — this code computes token ids that don't exist on the current network, inflates the request payload, and surfaces confusing 'no tokens' UX (off-network ids will return zero balances and be filtered out). Filter through the contract relationship (`token.dataContract?.networkRaw == appState.currentNetwork.rawValue`) or fetch contracts via the network-scoped predicate first and walk their tokens.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift`:
- [SUGGESTION] lines 36-45: Contracts tab mixes rows from every saved network
Both `@Query` declarations on `ContractsTabView` — `dataContracts` over `PersistentDataContract` and `allTokens` over `PersistentToken` — run with no network predicate, despite `PersistentDataContract.networkRaw` existing precisely so the UI can scope by network and `PersistentDataContract.predicate(network:)` / `contractsWithTokensPredicate(network:)` being defined for this. As soon as the user switches networks (testnet ↔ mainnet ↔ regtest) this tab continues showing contracts and tokens persisted under the previous network. Since this is now the primary contract browser, that's a visible regression. Switch to `@Query(filter: PersistentDataContract.predicate(network: appState.currentNetwork), sort: ...)` (or equivalent) and scope the token query through the contract relationship.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/ContractDownloader.swift`:
- [SUGGESTION] lines 127-173: ContractDownloader leaks json_string and serialized_data on every successful fetch
`dash_sdk_data_contract_fetch_with_serialization` returns a `DashSDKDataContractFetchResult` whose `json_string` is allocated via `CString::into_raw` (rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rs:117) and whose `serialized_data` is allocated via `Box::into_raw(data.into_boxed_slice())` (line 34). Both pointers are caller-owned and must be returned to Rust. `ContractDownloader.downloadAndPersistContract` only frees `result.error` and the contract handle — after copying the JSON via `String(cString:)` and the bytes via `Data(bytes:count:)`, the original Rust-side buffers are abandoned. This leak fires on every successful download (search-bar autocomplete, manual paste sheet, contract refresh), each contract's JSON+binary is multiple KB, so resident memory grows monotonically with usage. The matching free entry point is `dash_sdk_data_contract_fetch_result_free`, but it does `Box::from_raw(result)` on the outer struct (line 169) which is UB on a stack-located return value — so the safe minimal fix is to free the two inner pointers individually: `dash_sdk_string_free(result.json_string)` (the json string was created with `CString::into_raw`) and add a dedicated free for the boxed slice (the existing `dash_sdk_bytes_free` uses `libc::free` and is not interchangeable with Rust's global allocator). The right end-state is a sound free-by-pointer helper or a fix to `dash_sdk_data_contract_fetch_result_free` so it can operate on a caller-supplied address-of.
| Section("DashPay") { | ||
| NavigationLink(destination: FriendsView(identity: identity)) { | ||
| Label("Friends", systemImage: "person.2") | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Friends drill-in is exposed for identities with no wallet backing
IdentityDetailView unconditionally renders a NavigationLink into FriendsView(identity: identity) inside the new Section("DashPay"). FriendsView.loadFriends() immediately calls requireWallet(for:), which throws when identity.wallet?.walletId is nil — the normal case for any identity that was fetched from the network rather than loaded from one of the user's local wallets (isLocal == false, or local-but-unassociated). On that path the failure is stored into @State errorMessage but the view body never renders it: the user lands on the empty "No Friends Yet" placeholder, the Add Friend button opens AddFriendView which will fail the same way, and the action buttons are inert. Either gate the NavigationLink (if identity.wallet != nil), surface the errorMessage in FriendsView's body, or give FriendsView a real read-only mode for unowned identities.
source: ['codex']
🤖 Fix this 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/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- [BLOCKING] lines 131-135: Friends drill-in is exposed for identities with no wallet backing
`IdentityDetailView` unconditionally renders a `NavigationLink` into `FriendsView(identity: identity)` inside the new `Section("DashPay")`. `FriendsView.loadFriends()` immediately calls `requireWallet(for:)`, which throws when `identity.wallet?.walletId` is nil — the normal case for any identity that was fetched from the network rather than loaded from one of the user's local wallets (`isLocal == false`, or local-but-unassociated). On that path the failure is stored into `@State errorMessage` but the view body never renders it: the user lands on the empty `"No Friends Yet"` placeholder, the `Add Friend` button opens `AddFriendView` which will fail the same way, and the action buttons are inert. Either gate the NavigationLink (`if identity.wallet != nil`), surface the `errorMessage` in `FriendsView`'s body, or give `FriendsView` a real read-only mode for unowned identities.
| // Pull tokens off the live PersistentToken store — | ||
| // every saved contract's tokens are already hanging off | ||
| // it, so a flat `FetchDescriptor` is the simplest source | ||
| // of truth for "what tokens does the user know about?" | ||
| let descriptor = FetchDescriptor<PersistentToken>() | ||
| guard let allTokens = try? modelContext.fetch(descriptor), | ||
| !allTokens.isEmpty else { | ||
| tokenBalances = [] | ||
| return | ||
| } | ||
|
|
||
| // Compute token ids in one pass. Skip tokens whose | ||
| // position is out of u16 range (shouldn't happen — that | ||
| // would be a malformed row) so we don't crash on a bad | ||
| // downcast. | ||
| var idToToken: [String: PersistentToken] = [:] | ||
| for token in allTokens { | ||
| guard token.position >= 0, token.position <= Int(UInt16.max) else { continue } | ||
| let pos = UInt16(token.position) | ||
| let cidBase58 = token.contractId.toBase58String() | ||
| if let canonical = try? sdk.calculateTokenId(contractId: cidBase58, position: pos) { | ||
| idToToken[canonical] = token | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: reloadTokenBalances iterates every PersistentToken regardless of network
reloadTokenBalances issues a bare FetchDescriptor<PersistentToken>() with no predicate, then derives canonical token ids and submits them to getIdentityTokenBalances on the currently-bound network. PersistentToken has no own network field; network is stored on its parent PersistentDataContract.networkRaw (and PersistentDataContract already exposes predicate(network:) / contractsWithTokensPredicate(network:) for this exact purpose). After a network switch — or any time the user has cached contracts on more than one network — this code computes token ids that don't exist on the current network, inflates the request payload, and surfaces confusing 'no tokens' UX (off-network ids will return zero balances and be filtered out). Filter through the contract relationship (token.dataContract?.networkRaw == appState.currentNetwork.rawValue) or fetch contracts via the network-scoped predicate first and walk their tokens.
source: ['claude', 'codex']
🤖 Fix this 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/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- [SUGGESTION] lines 848-871: reloadTokenBalances iterates every PersistentToken regardless of network
`reloadTokenBalances` issues a bare `FetchDescriptor<PersistentToken>()` with no predicate, then derives canonical token ids and submits them to `getIdentityTokenBalances` on the currently-bound network. `PersistentToken` has no own network field; network is stored on its parent `PersistentDataContract.networkRaw` (and `PersistentDataContract` already exposes `predicate(network:)` / `contractsWithTokensPredicate(network:)` for this exact purpose). After a network switch — or any time the user has cached contracts on more than one network — this code computes token ids that don't exist on the current network, inflates the request payload, and surfaces confusing 'no tokens' UX (off-network ids will return zero balances and be filtered out). Filter through the contract relationship (`token.dataContract?.networkRaw == appState.currentNetwork.rawValue`) or fetch contracts via the network-scoped predicate first and walk their tokens.
| @Query(sort: \PersistentDataContract.lastAccessedAt, order: .reverse) | ||
| private var dataContracts: [PersistentDataContract] | ||
|
|
||
| /// Flat list of every token across every saved contract. Lets the | ||
| /// user discover tokens without having to remember which contract | ||
| /// defined them. Sorted by name; rows surface the parent contract | ||
| /// in the caption so two tokens with the same name in different | ||
| /// contracts stay distinguishable. | ||
| @Query(sort: \PersistentToken.name) | ||
| private var allTokens: [PersistentToken] |
There was a problem hiding this comment.
🟡 Suggestion: Contracts tab mixes rows from every saved network
Both @Query declarations on ContractsTabView — dataContracts over PersistentDataContract and allTokens over PersistentToken — run with no network predicate, despite PersistentDataContract.networkRaw existing precisely so the UI can scope by network and PersistentDataContract.predicate(network:) / contractsWithTokensPredicate(network:) being defined for this. As soon as the user switches networks (testnet ↔ mainnet ↔ regtest) this tab continues showing contracts and tokens persisted under the previous network. Since this is now the primary contract browser, that's a visible regression. Switch to @Query(filter: PersistentDataContract.predicate(network: appState.currentNetwork), sort: ...) (or equivalent) and scope the token query through the contract relationship.
source: ['codex']
🤖 Fix this 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/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift`:
- [SUGGESTION] lines 36-45: Contracts tab mixes rows from every saved network
Both `@Query` declarations on `ContractsTabView` — `dataContracts` over `PersistentDataContract` and `allTokens` over `PersistentToken` — run with no network predicate, despite `PersistentDataContract.networkRaw` existing precisely so the UI can scope by network and `PersistentDataContract.predicate(network:)` / `contractsWithTokensPredicate(network:)` being defined for this. As soon as the user switches networks (testnet ↔ mainnet ↔ regtest) this tab continues showing contracts and tokens persisted under the previous network. Since this is now the primary contract browser, that's a visible regression. Switch to `@Query(filter: PersistentDataContract.predicate(network: appState.currentNetwork), sort: ...)` (or equivalent) and scope the token query through the contract relationship.
| let result = trimmedId.withCString { idCStr in | ||
| dash_sdk_data_contract_fetch_with_serialization(handle, idCStr, true, true) | ||
| } | ||
|
|
||
| if let error = result.error { | ||
| let message = error.pointee.message != nil | ||
| ? String(cString: error.pointee.message!) | ||
| : "Unknown error" | ||
| dash_sdk_error_free(error) | ||
| // The FFI doesn't expose a structured "not found" code, so | ||
| // we have to substring-match on the message. This is the | ||
| // same pattern the modal sheet was using before this | ||
| // refactor, just centralized. | ||
| if message.contains("Data contract not found") | ||
| || message.contains("not found") { | ||
| throw ContractDownloadError.notFound(message) | ||
| } | ||
| throw ContractDownloadError.fetchFailed( | ||
| "Failed to fetch data contract: \(message)" | ||
| ) | ||
| } | ||
|
|
||
| // Pull JSON and binary out of the result before scheduling | ||
| // cleanup of the contract handle. | ||
| guard let jsonPtr = result.json_string else { | ||
| if result.contract_handle != nil { | ||
| dash_sdk_data_contract_destroy(result.contract_handle) | ||
| } | ||
| throw ContractDownloadError.fetchFailed( | ||
| "No JSON data returned from contract fetch" | ||
| ) | ||
| } | ||
| let jsonString = String(cString: jsonPtr) | ||
|
|
||
| var binaryData: Data? = nil | ||
| if result.serialized_data != nil && result.serialized_data_len > 0 { | ||
| binaryData = Data( | ||
| bytes: result.serialized_data, | ||
| count: Int(result.serialized_data_len) | ||
| ) | ||
| } | ||
|
|
||
| defer { | ||
| if result.contract_handle != nil { | ||
| dash_sdk_data_contract_destroy(result.contract_handle) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: ContractDownloader leaks json_string and serialized_data on every successful fetch
dash_sdk_data_contract_fetch_with_serialization returns a DashSDKDataContractFetchResult whose json_string is allocated via CString::into_raw (rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rs:117) and whose serialized_data is allocated via Box::into_raw(data.into_boxed_slice()) (line 34). Both pointers are caller-owned and must be returned to Rust. ContractDownloader.downloadAndPersistContract only frees result.error and the contract handle — after copying the JSON via String(cString:) and the bytes via Data(bytes:count:), the original Rust-side buffers are abandoned. This leak fires on every successful download (search-bar autocomplete, manual paste sheet, contract refresh), each contract's JSON+binary is multiple KB, so resident memory grows monotonically with usage. The matching free entry point is dash_sdk_data_contract_fetch_result_free, but it does Box::from_raw(result) on the outer struct (line 169) which is UB on a stack-located return value — so the safe minimal fix is to free the two inner pointers individually: dash_sdk_string_free(result.json_string) (the json string was created with CString::into_raw) and add a dedicated free for the boxed slice (the existing dash_sdk_bytes_free uses libc::free and is not interchangeable with Rust's global allocator). The right end-state is a sound free-by-pointer helper or a fix to dash_sdk_data_contract_fetch_result_free so it can operate on a caller-supplied address-of.
source: ['claude']
🤖 Fix this 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/SwiftExampleApp/SwiftExampleApp/Utils/ContractDownloader.swift`:
- [SUGGESTION] lines 127-173: ContractDownloader leaks json_string and serialized_data on every successful fetch
`dash_sdk_data_contract_fetch_with_serialization` returns a `DashSDKDataContractFetchResult` whose `json_string` is allocated via `CString::into_raw` (rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rs:117) and whose `serialized_data` is allocated via `Box::into_raw(data.into_boxed_slice())` (line 34). Both pointers are caller-owned and must be returned to Rust. `ContractDownloader.downloadAndPersistContract` only frees `result.error` and the contract handle — after copying the JSON via `String(cString:)` and the bytes via `Data(bytes:count:)`, the original Rust-side buffers are abandoned. This leak fires on every successful download (search-bar autocomplete, manual paste sheet, contract refresh), each contract's JSON+binary is multiple KB, so resident memory grows monotonically with usage. The matching free entry point is `dash_sdk_data_contract_fetch_result_free`, but it does `Box::from_raw(result)` on the outer struct (line 169) which is UB on a stack-located return value — so the safe minimal fix is to free the two inner pointers individually: `dash_sdk_string_free(result.json_string)` (the json string was created with `CString::into_raw`) and add a dedicated free for the boxed slice (the existing `dash_sdk_bytes_free` uses `libc::free` and is not interchangeable with Rust's global allocator). The right end-state is a sound free-by-pointer helper or a fix to `dash_sdk_data_contract_fetch_result_free` so it can operate on a caller-supplied address-of.
| /// | ||
| /// Cheap: the underlying `FetchDescriptor` predicate filters on | ||
| /// the indexed `ownerId` column and returns only the contracts | ||
| /// that actually need patching. Idempotent — the per-contract | ||
| /// `==` check below skips rows already linked to this identity. |
There was a problem hiding this comment.
💬 Nitpick: Comment claims ownerId column is 'indexed' but PersistentDataContract has no index attribute
linkIdentityToOwnedContracts's comment says 'the underlying FetchDescriptor predicate filters on the indexed ownerId column.' PersistentDataContract.ownerId is declared as public var ownerId: Data? with no @Attribute(.unique) or @Attribute(.indexed) decoration (PersistentDataContract.swift:18), and SwiftData does not auto-index optional Data columns. Performance is irrelevant at expected on-device contract counts, but the comment misrepresents the schema and could mislead anyone benchmarking the BLAST persister. Either drop the 'indexed' word or add @Attribute(.indexed) on the field.
source: ['claude']
| pub unsafe extern "C" fn dash_sdk_calculate_token_id( | ||
| contract_id: *const c_char, | ||
| position: u16, | ||
| ) -> DashSDKResult { | ||
| if contract_id.is_null() { | ||
| return DashSDKResult::error(DashSDKError::new( | ||
| DashSDKErrorCode::InvalidParameter, | ||
| "contract_id is null".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| let id_str = match CStr::from_ptr(contract_id).to_str() { | ||
| Ok(s) => s, | ||
| Err(e) => return DashSDKResult::error(FFIError::from(e).into()), | ||
| }; | ||
|
|
||
| let contract_identifier = match Identifier::from_string(id_str, Encoding::Base58) { | ||
| Ok(id) => id, | ||
| Err(e) => { | ||
| return DashSDKResult::error(DashSDKError::new( | ||
| DashSDKErrorCode::InvalidParameter, | ||
| format!("Invalid contract ID: {}", e), | ||
| )) | ||
| } | ||
| }; | ||
|
|
||
| let contract_id_bytes: [u8; 32] = contract_identifier.to_buffer(); | ||
| let token_id_bytes = dash_sdk::dpp::tokens::calculate_token_id(&contract_id_bytes, position); | ||
|
|
||
| let token_id_base58 = Identifier::new(token_id_bytes).to_string(Encoding::Base58); | ||
|
|
||
| let c_str = match CString::new(token_id_base58) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| return DashSDKResult::error( | ||
| FFIError::InternalError(format!("Failed to create CString: {}", e)).into(), | ||
| ) | ||
| } | ||
| }; | ||
| DashSDKResult::success_string(c_str.into_raw()) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: New dash_sdk_calculate_token_id has no unit test
The FFI is a thin direct call into dash_sdk::dpp::tokens::calculate_token_id, so functional drift from the protocol formula isn't really possible — the bridge IS the formula. That said, a small #[test] asserting that dash_sdk_calculate_token_id for a fixed (contract_id, position) pair produces the same bytes as dpp::tokens::calculate_token_id directly would lock the C-ABI marshaling (CString in/out, base58 round-trip) in place against future churn in the result/error wrapper types. Low priority.
source: ['claude']
…chainSigner sweep Move contract create, identity top-up, and add-identity-key off the rs-sdk-ffi runtime onto platform-wallet's 8 MB worker stack (the post-broadcast GroveDB Op::decode recursion was blowing the iOS default ~512 KB stack and crashing with EXC_BAD_ACCESS), thread every state-transition signature through the KeychainSigner trampoline so private-key bytes never cross the FFI, and fix several supporting bugs surfaced along the way: - Register Contract: new IdentityWallet::create_data_contract_with_signer + platform_wallet_create_data_contract_with_signer FFI; deleted dash_sdk_data_contract_create + SDK.dataContractCreate/Update. RegisterContractSourceView 4-option picker (Pasteboard / QR / Manual / Quick Basic Token), pasteboard JSON detector, in-memory ModelContainer preview reusing DataContractDetailsView, Save to Device. - Identity Keys: AddIdentityKeyView with purpose / security / contract bounds constraints; IdentityPubkeyFFI gained contract_bounds_* fields; dash_sdk_derive_identity_key_at_slot derives single slots; update_identity_with_external_signer now updates local state + fires the persister callback after broadcast (was silently succeeding without surfacing the new key). - Identity Top Up: platform_wallet_top_up_from_addresses_with_signer FFI + ManagedPlatformWallet.topUpFromAddresses wrapper + TopUpIdentityView mirroring CreateIdentityView; entry point under the Balance row in IdentityDetailView, gated on owning wallet being loaded. - KeychainSigner sweep: KeyManager.find*WithPrivateKey + create*Signer removed in favour of findSigningKey + the KeychainSigner trampoline; TransitionDetailView migrated all 14 call sites; KeychainManager gained hasIdentityPrivateKey existence check. - WIF: Swift WIFParser delegates to dash_sdk_private_key_to_wif (Dash bytes 0xCC/0xEF, not Bitcoin's 0x80/0xEF — keys now correctly start with X on mainnet). - Bug fixes from automated review: gate Friends drill-in on identity.wallet being loaded (FriendsView throws on nil-wallet paths and never renders the error); rewrite dash_sdk_data_contract_fetch_result_free to be sound on the stack-allocated result struct + plug the json_string + serialized_data leak in ContractDownloader; scope ContractsTabView @query and reloadTokenBalances to the active network via the parent contract's networkRaw so persisted rows from another network don't leak after a network switch. Tests: new PasteboardContractCandidateTests (12 XCTest cases); deleted dead WalletTests/TransactionTests.swift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Utils/ModelContainerHelper.swift (1)
33-60: 🛠️ Refactor suggestion | 🟠 Major
createContainer()still uses an inline schema — drops the de-duplication the newschema()helper was meant to provide.The doc on
schema()(lines 5–8) promises both container variants stay in lockstep through the shared helper, butcreateContainer()still constructs its ownSchema([...])literal. Adding a newPersistent*model will silently diverge between the on-disk and in-memory containers — exactly the regression this refactor was supposed to prevent. The PR's AI summary also describescreateContaineras already routed throughschema(), which doesn't match the code.♻️ Route `createContainer()` through the shared
schema()public static func createContainer() throws -> ModelContainer { - let schema = Schema([ - // Platform + core-wallet rows. `PersistentWallet` - // replaces the former `HDWallet` `@Model` as the - // canonical SwiftData wallet row; the wallet-level - // fields that lived on `HDWallet` (label, network, - // isWatchOnly, isImported) are all on - // `PersistentWallet` now. - PersistentIdentity.self, - PersistentPublicKey.self, - PersistentDocument.self, - PersistentTokenBalance.self, - PersistentDataContract.self, - PersistentToken.self, - PersistentDocumentType.self, - PersistentTokenHistoryEvent.self, - PersistentKeyword.self, - PersistentIndex.self, - PersistentProperty.self, - PersistentPlatformAddress.self, - PersistentSyncState.self, - PersistentWallet.self, - PersistentAccount.self, - PersistentCoreAddress.self, - PersistentTransaction.self, - PersistentUtxo.self, - PersistentWalletManagerMetadata.self, - ]) - + let schema = schema() let modelConfiguration = ModelConfiguration( schema: schema, isStoredInMemoryOnly: false, allowsSave: true )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Utils/ModelContainerHelper.swift` around lines 33 - 60, createContainer() builds an inline Schema literal which breaks the intended de-duplication with the shared schema() helper; change createContainer() to call the existing schema() helper (e.g., use the schema returned by schema() when constructing the ModelContainer) so both on-disk and in-memory containers use the same Schema instance and any new Persistent* model added to schema() will automatically be included; update references inside createContainer() to use the returned Schema from schema() instead of the hard-coded Schema([...]) and ensure ModelContainer(...) is constructed with that shared schema.packages/swift-sdk/Sources/SwiftDashSDK/Helpers/WIFParser.swift (1)
21-34:⚠️ Potential issue | 🟡 MinorStale prefix/version-byte comments in
parseWIFnow contradict the new file header.The new file-level docs (lines 6–14) call out the canonical Dash WIF version bytes as
0xCCmainnet /0xEFtestnet, but the inline comments inparseWIFstill describe testnet as0xCCand mainnet as0xD2, and list compressed/uncompressed prefixes that don't match Dash mainnet (X…is the compressed mainnet prefix;7…is uncompressed mainnet — testnet usesc…/9…). The runtime code doesn't actually consult the version byte, so behavior is unaffected, but the contradictory comments will mislead the next reader who checks them.📝 Suggested comment fix
public static func parseWIF(_ wif: String) -> Data? { - // WIF format: - // - Mainnet: starts with '7' (uncompressed) or 'X' (compressed) - // - Testnet: starts with 'c' (uncompressed) or 'c' (compressed) + // Dash WIF prefixes (single-character base58): + // - Mainnet: '7' (uncompressed) / 'X' (compressed) + // - Testnet: '9' (uncompressed) / 'c' (compressed) guard !wif.isEmpty else { return nil } // Decode from Base58 guard let decoded = decodeBase58(wif) else { return nil } // WIF structure: - // - 1 byte: version (0xCC for testnet, 0xD2 for mainnet) + // - 1 byte: version (0xCC mainnet, 0xEF testnet — Dash, NOT Bitcoin) // - 32 bytes: private key // - (optional) 1 byte: 0x01 for compressed public key // - 4 bytes: checksum🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Helpers/WIFParser.swift` around lines 21 - 34, Update the stale inline comments in parseWIF to match the file-level header: replace the incorrect version-byte mapping and prefixes with the canonical Dash values (mainnet version 0xCC, testnet version 0xEF) and correct the human-readable WIF prefixes (mainnet: '7' uncompressed and 'X' compressed; testnet: 'c' and '9' as appropriate), and add a short note in the parseWIF comment that the runtime code currently does not validate the version byte so behavior is unchanged; locate these changes around the parseWIF / WIFParser.swift comment block that currently lists "WIF format" and "WIF structure".packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swift (1)
322-332:⚠️ Potential issue | 🟡 MinorOrphaned doc comment above
destroySigner.Lines 324-326 (
/// Create a signer for a specific key in an identity / - Parameters: / - identity: The identity) are leftover header lines from a deleted method (per the AI summary, severalcreateSigner...extension/helpers were removed). They are now glued ontodestroySigner's docs and contradict its actual purpose. Trim them so the doc accurately documentsdestroySigner.📝 Proposed cleanup
- /// Create a signer for a specific key in an identity - /// - Parameters: - /// - identity: The identity - /// Destroy a signer handle + /// Destroy a signer handle. /// - Parameter signer: The signer handle to destroy public func destroySigner(_ signer: OpaquePointer) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swift` around lines 322 - 332, The doc comment above destroySigner is incorrect: remove the orphaned lines "Create a signer..." and its parameter notes, and replace them with a brief accurate doc for destroySigner indicating it destroys a signer handle; reference the function name destroySigner and types SignerHandle/OpaquePointer and the underlying call dash_sdk_signer_destroy so maintainers know this method releases the signer via dash_sdk_signer_destroy.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LoadIdentityView.swift (1)
414-453:⚠️ Potential issue | 🟡 MinorAlso link contracts on the existing-row branch.
ContractIdentityLinker.linkIdentityToOwnedContractsruns only when a brand-newPersistentIdentityis inserted (Lines 449-452). If the user reloads an identity that already exists locally, any contract rows added since the original insert (e.g. via the new Contracts tab /RegisterContractSourceView) will keepownerIdentity == nilbecause the linker isn't re-run. The linker is idempotent onidentityId, so calling it on both branches is safe and keeps the relationship consistent.♻️ Suggested fix
let row: PersistentIdentity if let existing = existing { existing.balance = Int64(bitPattern: fetchedBalance) existing.alias = trimmedAlias existing.isLocal = false existing.identityType = identityType.rawValue existing.network = network existing.lastUpdated = Date() // Replace public keys wholesale with the // freshly fetched set. existing.publicKeys.removeAll() row = existing } else { row = PersistentIdentity( ... ) modelContext.insert(row) - // Back-fill any locally-cached contracts that - // name this identity as their owner. The - // existing `try? modelContext.save()` later - // in this block persists the link. - ContractIdentityLinker.linkIdentityToOwnedContracts( - identity: row, - modelContext: modelContext - ) } + // Back-fill any locally-cached contracts that + // name this identity as their owner. Idempotent, + // so it's safe to run on both insert and update + // paths — picks up contracts added since the + // identity was first seen. + ContractIdentityLinker.linkIdentityToOwnedContracts( + identity: row, + modelContext: modelContext + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LoadIdentityView.swift` around lines 414 - 453, When updating/inserting PersistentIdentity in LoadIdentityView (inside the MainActor.run block), ensure ContractIdentityLinker.linkIdentityToOwnedContracts(identity:modelContext:) is called for both the existing-row branch and the new-row branch: after you finish mutating the existing PersistentIdentity (the `existing` variable) call ContractIdentityLinker.linkIdentityToOwnedContracts(identity: row, modelContext: modelContext) so any contracts added since the original insert get their ownerIdentity populated; keep the existing call for the insert branch as well (the row created via PersistentIdentity(...)) since the linker is idempotent on identityId.packages/rs-platform-wallet/src/wallet/identity/network/update.rs (1)
192-202:⚠️ Potential issue | 🟡 MinorDoc comment contradicts the new post-broadcast apply.
The function-level doc (Lines 192-202) still asserts:
CACHE INVARIANT: this function does NOT refresh the in-process
IdentityManagerafter a successful broadcast. The local cachedIdentitykeeps the pre-update revision and key set until the caller invokesSelf::refresh_identity(or the next sync round). A subsequent call to this function for the same identity without an intervening refresh will reuse the stale revision and Platform will reject the duplicate.But the new block at Lines 311-367 does take a write lock, bump the cached revision, and apply each new key via
managed.add_key(...). The "duplicate-revision-rejection" failure mode the doc warns about is now actively prevented for the add path. Only the disable path remains stale (and you've correctly logged a warning for it).Suggest updating the doc so callers don't preemptively call
refresh_identity(and so the disable-side caveat is what actually survives, rather than getting buried under outdated copy):📝 Proposed doc update
- /// CACHE INVARIANT: this function does NOT refresh the in-process - /// `IdentityManager` after a successful broadcast. The local - /// cached `Identity` keeps the pre-update revision and key set - /// until the caller invokes [`Self::refresh_identity`] (or the - /// next sync round). A subsequent call to this function for the - /// same identity without an intervening refresh will reuse the - /// stale revision and Platform will reject the duplicate. This - /// matches the behaviour of the legacy [`Self::update_identity`] - /// path; it is documented here rather than fixed because the - /// refresh requires a wallet-manager write lock that may already - /// be held higher in the call stack. + /// CACHE BEHAVIOUR: after a successful broadcast this function + /// takes a write lock on the wallet manager and applies a + /// partial local refresh: + /// - the cached `Identity.revision` is incremented so a + /// subsequent update doesn't reuse the pre-broadcast value + /// and get rejected as a duplicate, + /// - each newly-added key is applied via + /// `ManagedIdentity::add_key`, which fires the + /// `IdentityKeysChangeSet` persister callback (so + /// SwiftData / Keychain rows downstream are consistent). + /// + /// The DISABLE side is NOT yet applied locally — see the + /// `tracing::warn!` at the bottom of this function and the + /// `TODO(disable-keys)` for the planned + /// `ManagedIdentity::disable_keys` counterpart. Callers that + /// disable keys should follow up with [`Self::refresh_identity`] + /// (or the next sync round) to land the disabled-at flags + /// in the cache.Also applies to: 311-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/update.rs` around lines 192 - 202, Update the function-level doc comment to remove the blanket claim that this function never refreshes the in-process IdentityManager after broadcast and instead document the current behavior: state that after a successful broadcast the "add" path takes a wallet-manager write lock and updates the cached revision and key set by calling managed.add_key(...) (so callers do not need to call Self::refresh_identity for added keys), but that the "disable" path still does not update the cache (emit a warning) and therefore callers should call Self::refresh_identity if they rely on disables being immediately reflected; reference the IdentityManager/in-process cache and the managed.add_key(...) update behavior in the doc so the distinction is clear.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift (1)
1142-1157:⚠️ Potential issue | 🟠 MajorHardcoded 8-decimal token amount conversion can over/undershoot by orders of magnitude.
Token mint / burn / transfer all parse decimal user input as
UInt64(doubleValue * 100_000_000), but token decimals are a per-token property —PersistentTokenand the contract's token schema both carry the actualdecimalsvalue. A user entering1.5for a 6-decimal token will mint/burn/send150_000_000base units instead of1_500_000, i.e. 100× the intended amount. Resolve the token's declareddecimalsfromselectedContractId+position(thetokenSelectionalready encodes both) and scale accordingly, or refuse decimal input until the form can show the user-facing precision. As-is, this is a silent value-corruption hazard on any token whose decimals != 8.Also note that
Double->UInt64rounds toward zero, so0.1 * 100_000_000 == 9_999_999.999...becomes9_999_999, dropping a base unit on the boundary. Prefer integer-only parsing once the decimals are known.Also applies to: 1237-1252, 1528-1543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift` around lines 1142 - 1157, The amount parsing currently multiplies Double by a hardcoded 100_000_000 and truncates to UInt64; instead resolve the token's actual decimals from the selected token (use the tokenSelection value to look up PersistentToken/contract token schema via selectedContractId and position) and scale user input by 10^decimals rather than 8. Replace Double-based conversion with integer-safe parsing: split amountString on the decimal point, validate fractional length <= decimals, compute baseUnits = integerPart * 10^decimals + fractionalPartPadded, and throw SDKError.invalidParameter if the fractional length exceeds decimals or parsing fails. Apply the same change to the other amount parsing sites in TransitionDetailView (the other mint/burn/transfer blocks) or reject decimal input until UI displays token precision.
🧹 Nitpick comments (17)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swift (1)
143-155: LGTM across all three call sites.The
nonisolated(unsafe)locals are introduced consistently foridentityTransferCredits,transferCredits, andwithdrawFromIdentity, with the helpful comment block on lines 143–147 explaining the Swift 6.2+ rationale. Pointer lifetimes are properly bounded by the surroundingdeferblocks that destroy the underlying handles, so safety of the unsafe-Sendable surfacing holds.Optional nit (non-blocking): the same 2-line pattern is repeated across many tests in this suite — if it grows further, a tiny
@inline(__always)test helper likefunc unsafeOpaque(_ p: UnsafeMutableRawPointer) -> OpaquePointerreturning anonisolated(unsafe)value could cut duplication. Fine to defer.Also applies to: 293-298, 386-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swift` around lines 143 - 155, The repeated two-line pattern creating nonisolated(unsafe) locals for OpaquePointer (used before calls like identityTransferCredits, transferCredits, withdrawFromIdentity) should be consolidated into a small test-only helper; add an `@inline`(__always) helper (suggested name: unsafeOpaque(_:)) that accepts the raw handle/pointer and returns a nonisolated(unsafe) OpaquePointer, then replace the duplicated nonisolated(unsafe) let identityPtr / signerPtr lines with calls to unsafeOpaque(...) in each test while preserving existing defer-based lifetime/destroy behavior.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swift (1)
205-255: Stale doc references to deletedfindKeyWithPrivateKey.Per the AI summary,
findKeyWithPrivateKey(...) -> (key:, privateKey:)?was removed and replaced byfindSigningKey. The doc-comment forfindSigningKeystill describes itself as the "companion tofindKeyWithPrivateKey" (Line 207), and the doc forrankKeysstill says it's "Used byfindKeyWithPrivateKey" (Line 253). Both references are now dangling and should point atfindSigningKeyinstead.Also worth noting: the
findSigningKeyheader claims selection happens "WITHOUT extracting the private-key bytes" (Line 206), but the first probe on Line 239 callsgetPrivateKey(...)which actually returns the 32-byte material into a SwiftData. Only the second probe (Line 243) is non-extracting. The TODO on Lines 220-224 acknowledges this for the second call but the doc preamble overstates the current guarantee — consider tightening the wording until both probes are non-extracting.📝 Suggested doc tweak
- /// Pick a public key on `identity` that has signable private - /// material on the device, WITHOUT extracting the private-key - /// bytes. Companion to `findKeyWithPrivateKey` for callers that - /// only need the `keyId` (because the actual signing happens via - /// `KeychainSigner`'s callback, not via raw bytes pulled out - /// here). Same candidate ranking + same dual-scheme keychain - /// presence check, just discarding the bytes once they're - /// confirmed to exist. + /// Pick a public key on `identity` that has signable private + /// material on the device. The actual signing happens via + /// `KeychainSigner`'s callback, so callers only need the + /// `keyId` returned here. Today this still extracts bytes for + /// the first probe (see TODO below); once both probes go + /// through non-extracting checks the bytes will never enter + /// Swift memory during selection. @@ - /// Return every key on `identity` matching the same filters as - /// `findKey`, ordered the way `findKey` would prefer one — critical - /// keys first when `preferCritical`, then everything else, with - /// disabled keys filtered out. Used by `findKeyWithPrivateKey` so - /// it can iterate through candidates instead of bailing on the - /// first one whose private material isn't on the device. + /// Return every key on `identity` matching the same filters as + /// `findKey`, ordered the way `findKey` would prefer one — critical + /// keys first when `preferCritical`, then everything else, with + /// disabled keys filtered out. Used by `findSigningKey` so it + /// can iterate through candidates instead of bailing on the + /// first one whose private material isn't on the device.packages/rs-platform-wallet/src/wallet/identity/network/update.rs (1)
279-280: Optional: skip thedisableclone.
disabled_ids_for_local_applyis cloned at Line 280 but only used inis_empty()checks (Lines 331 and 349) and atracing::warn!count. You can avoid theVec<u32>clone entirely by capturingdisable_public_keys.is_empty()anddisable_public_keys.len()as locals before the move, and dropping the clone.♻️ Diff
- let added_keys_for_local_apply = add_public_keys.clone(); - let disabled_ids_for_local_apply = disable_public_keys.clone(); + let added_keys_for_local_apply = add_public_keys.clone(); + let disabled_count = disable_public_keys.len(); let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( &identity, &master_key_id, add_public_keys, disable_public_keys, ... @@ - if !added_keys_for_local_apply.is_empty() || !disabled_ids_for_local_apply.is_empty() { + if !added_keys_for_local_apply.is_empty() || disabled_count > 0 { ... - if !disabled_ids_for_local_apply.is_empty() { + if disabled_count > 0 { tracing::warn!( identity = %identity_id, - disabled_count = disabled_ids_for_local_apply.len(), + disabled_count, "Disable-keys post-broadcast apply not yet implemented; ..." ); }Negligible if the
Vecis small, but it costs nothing to avoid.Also applies to: 331-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/update.rs` around lines 279 - 280, The code unnecessarily clones disable_public_keys into disabled_ids_for_local_apply; instead capture disable_is_empty = disable_public_keys.is_empty() and disable_len = disable_public_keys.len() before any moves and use these locals for the is_empty() checks and the tracing::warn! count; remove the disabled_ids_for_local_apply variable and replace its usages with disable_is_empty and disable_len (leave added_keys_for_local_apply clone as-is).packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift (1)
880-950: Network-scoped Tokens fetch is correctly addressed.The
FetchDescriptor<PersistentToken>now filters viatoken.dataContract?.networkRaw == target, so post-network-switch (or multi-network local caches) the off-network token ids are no longer pushed throughgetIdentityTokenBalances. Theposition >= 0 && <= UInt16.maxguard before the downcast also handles malformed rows safely.One small optional follow-up:
try? sdk.calculateTokenId(...)on Line 921 swallows per-token derivation errors silently; if a contract row is malformed, the user sees "No tokens" with no diagnostic in the log. Aprint("⚠️ calculateTokenId failed for \(cidBase58):pos=\(pos): \(error)")in ado/catchwould help future debugging, no behavior change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift` around lines 880 - 950, The loop in reloadTokenBalances silently swallows errors from try? sdk.calculateTokenId(contractId:pos:), making malformed contract rows unobservable; wrap the call to sdk.calculateTokenId in a do/catch inside the for token in allTokens loop (where idToToken is populated), and on catch log a concise diagnostic (e.g., include cidBase58 and pos and the error) then continue so behavior is unchanged but failures are visible; keep the existing guards for position and skip entries on error so idToToken only contains successfully-derived token ids.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swift (1)
405-446: Token-id flow looks correct; minor robustness suggestion fortoken_contract_positionparsing.The Int / NSNumber dual-decode handles JSONSerialization's number bridging quirks correctly, but defaulting silently to
0for any unrecognized shape (e.g., a numeric string, or the field genuinely missing) could mislead the user into believing the position is0. Consider distinguishing "missing" from "unparseable" to surface a clearer error rather than potentially showing a wrong slot in the resolved-token banner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swift` around lines 405 - 446, The parsing for token_contract_position in the getTokenContractInfo flow currently defaults silently to 0; instead detect a missing or unparseable value and surface an error: when info["token_contract_position"] is neither Int nor NSNumber (and not present as a valid numeric string), set errorMessage, showError and isLoading on the MainActor and return rather than assigning 0; update references to resolvedTokenPosition/resolvedContractId only after a successful parse, and keep the hand-off to loadContract(resolvedId) unchanged.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/QuickBasicTokenView.swift (2)
102-112:isValiddoesn't mirror themax ≥ baserule, so the Continue button is enabled in the invalid case.
validate()rejectsmax < base(line 133), butisValid(which gates the button at line 76) doesn't, so users can tap Continue, the validation error appears, and the navigation is silently blocked. Either drop the extra rule fromvalidate()or add it toisValidto keep the gating consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/QuickBasicTokenView.swift` around lines 102 - 112, The isValid computed property currently checks numeric formats but misses the "max ≥ base" rule enforced in validate(), so add the same comparison there: parse baseSupply and maxSupply as UInt64 (after trimming), and if maxSupply is non-empty ensure UInt64(maxSupply) >= UInt64(baseSupply); return false on parse failure or if max < base. Update isValid (the computed var) to perform this additional check so it mirrors validate() and consistently gates the Continue button.
137-140: Stray_ = daftervalidationError = nil.
dis bound in the guard on line 120 and used immediately for the bounds check; the_ = don line 138 reads it again to no effect. Looks like leftover debris from an earlier revision and can be dropped.validationError = nil - _ = d return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/QuickBasicTokenView.swift` around lines 137 - 140, Remove the redundant statement `_ = d` that follows `validationError = nil`—`d` is already bound in the guard (used for the bounds check) so the `_ = d` is a no-op; delete that line in the function/method containing `validationError = nil` (the block where `d` is bound) to clean up the leftover debris.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DataContractDetailsView.swift (1)
351-358: Sort comment is slightly misleading.The "Bad position keys land at the bottom by construction (Int.max via
parseGroupsskip-on-fail above filters those out…)" wording referencesInt.maxeven though the loop above justcontinues on un-parseable position keys (they're dropped, not pushed to the bottom). Consider tightening the comment so future maintainers don't go looking for anInt.maxfallback that isn't there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DataContractDetailsView.swift` around lines 351 - 358, The comment above the out.sort call is misleading: update the comment to accurately describe that parseGroups will skip (continue) on unparseable position keys so those entries are dropped rather than assigned Int.max, and that the sort(by: { $0.position < $1.position }) simply ensures ascending order for the remaining entries; reference parseGroups, the loop that continues on parse failure, and the out.sort(by:) line when making this clarification.packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs (1)
117-130: Confirm naming consistency withderive_identity_auth_keypair.
DerivedIdentityAuthKeyexposesprivate_key: Zeroizing<[u8;32]>andpublic_key: [u8;33], while the existing siblingderive_identity_auth_keypairreturns(DerivationPath, ExtendedPrivKey, secp256k1::PublicKey). Two helpers in the same module now produce conceptually the same thing in different shapes (extended xpriv vs. raw 32 bytes; secp256k1::PublicKey vs. serialized 33 bytes). That's fine for the FFI use case, but if there's any thought of consolidating the discovery scan onto this struct down the line, consider havingDerivedIdentityAuthKeyexpose convenience accessors (secp256k1::SecretKey/PublicKey) so callers don't reach for the raw byte arrays unnecessarily.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs` around lines 117 - 130, The DerivedIdentityAuthKey struct currently exposes raw byte arrays (private_key: Zeroizing<[u8;32]>, public_key: [u8;33]) while derive_identity_auth_keypair returns ExtendedPrivKey and secp256k1::PublicKey; add convenience accessors on DerivedIdentityAuthKey (e.g., fn secret_key(&self) -> secp256k1::SecretKey and fn public_key_obj(&self) -> secp256k1::PublicKey or similar) that construct and return the appropriate secp256k1 types (handling potential parse errors) so callers can use the typed keys instead of re-parsing raw bytes and to keep shape consistent with derive_identity_auth_keypair and future consolidation.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift (3)
427-427: Idiomatic style: prefer!= niloverif let _ =.
if let _ = isLikelyContractIdBytes(trimmed)discards the bound value —if isLikelyContractIdBytes(trimmed) != nilis the more conventional spelling and triggers no SwiftLintunused_optional_bindingwarning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift` at line 427, Replace the unused optional binding with an explicit nil-check: in ContractsTabView (the conditional that currently reads "if let _ = isLikelyContractIdBytes(trimmed)"), change it to test the returned optional directly using "isLikelyContractIdBytes(trimmed) != nil" so the bound value isn't discarded and the SwiftLint unused_optional_binding warning is avoided.
451-594: ID-submit persists immediately; keyword-tap previews first — UX is asymmetric.
runIdLookupcallsContractDownloader.downloadAndPersistContractdirectly on Submit, while the keyword path produces aSearchResultrow that the user must tap, which then runspreviewContractInMemoryand lets them inspect before saving. A user who pastes a contract id and hits Return ends up with the contract permanently in their on-disk store with no preview/cancel step, even when they may have only wanted to inspect it. Routing the id path throughpreviewContractInMemoryfirst (then opening the sameDataContractDetailsViewsheet with a "Save to Device" button) would unify the two flows and avoid surprise persistence on a single Return keystroke.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift` around lines 451 - 594, The current runIdLookup function persists contracts immediately via ContractDownloader.downloadAndPersistContract on both the contract-id path and the token-id fallback; change both code paths in runIdLookup to call previewContractInMemory (instead of downloadAndPersistContract) to load the contract into memory without saving, then populate searchResults (or directly open the same DataContractDetailsView sheet) with a Preview entry that launches the existing details sheet which shows the contract and exposes the existing "Save to Device" action; ensure searchInFlight/searchError handling stays the same, do not clear searchQuery until after an actual save, and keep existing logic for alreadyExisted vs preview state so persistence only happens when the user taps Save from the preview view.
538-542:token_contract_positionfromNSNumberis silently lost when value is 0.
token_contract_positionfalls through to0if neither cast succeeds, but the message at line 555/557 then renders "Token at position 0", which is indistinguishable from an actual position-0 token. Consider returning anInt?and printing "(position unknown)" when the field is missing, so a malformed/missing position field doesn't masquerade as a legitimate slot 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift` around lines 538 - 542, The current computed let position: Int always returns 0 when the `token_contract_position` cast fails, which hides missing/malformed values as a legitimate position-0; change `position` to an optional Int (let position: Int?) that returns nil when neither `as? Int` nor `as? NSNumber` succeed, and update the UI text that currently prints "Token at position \(position)" to handle nil by printing "(position unknown)" (e.g. use optional mapping or a ternary to render either the numeric position or the "(position unknown)" string). Reference the `position` variable and the `token_contract_position` key in ContractsTabView.swift when making these edits.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/ContractDownloader.swift (2)
272-307: Duplicate name-derivation block between persist and preview paths.Both
downloadAndPersistContractandpreviewContractInMemorycarry an identical "derive a display name" closure (token-only →<TokenName> Token Contract, documents →Contract with <firstDocType>, fallback → truncated id). Pull this into a single private helper (e.g.private static func resolvedContractName(contractData:trimmedId:suggestedName:) -> String) so a future label change touches one place. Same applies to the JSON/binary unpack-and-id-resolve preamble — the two methods diverge only at the trusted-context registration / persist vs. in-memory insert point, so the head can be a shared helper returning(jsonData, contractData, contractIdData, binaryData).Also applies to: 440-469
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/ContractDownloader.swift` around lines 272 - 307, Both downloadAndPersistContract and previewContractInMemory duplicate the display-name derivation; extract that closure into a single private helper such as private static func resolvedContractName(contractData: [String: Any], trimmedId: String, suggestedName: String?) -> String and replace the inline closure in both functions with calls to it (preserving the same token-only → "<TokenName> Token Contract", documents → "Contract with <firstDocType>", fallback → truncated id behavior). Likewise factor out the unpack-and-id-resolve preamble shared by both functions into a private helper that returns (jsonData, contractData, contractIdData, binaryData) so downloadAndPersistContract can continue to register/persist and previewContractInMemory can insert in-memory while reusing the same head logic.
230-239:Data(hexString:)fallback afterfromBase58:may match by accident.The id from the JSON is canonical base58, but
Data.identifier(fromBase58: idString) ?? Data(hexString: idString)would also accept a string that happens to be valid hex. Most base58 strings are not valid hex, but a 64-character ASCII hex string is also valid base58 and would decode through the first branch — it's the reverse case that's risky: ifidStringis somehow an unusual non-canonical form that base58 rejects but hex accepts, the resulting bytes won't match the contract's actual id and the duplicate-check / linker logic will run against the wrong key. Worth tightening to base58-only here, or at least assertingidData.count == 32like the input-id helpers do elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/ContractDownloader.swift` around lines 230 - 239, The current extraction in ContractDownloader.swift accepts hex as a fallback which can silently produce wrong IDs; update the extraction logic in the block that sets contractIdData so it only accepts canonical base58 IDs (use Data.identifier(fromBase58: idString) exclusively) or, if you must keep hex fallback, validate the decoded bytes exactly match the expected length (e.g., assert idData.count == 32) before assigning contractIdData; apply the same strict length check when using the trimmedId branch (Data.identifier(fromBase58: trimmedId)) so only valid 32-byte IDs are accepted and throw ContractDownloadError.fetchFailed if validation fails.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift (2)
631-655:transferKeyis computed but never threaded intotransferCredits.
findSigningKeyruns onMainActorto validate that a transfer key exists, then the only consumer is thesdk.transferCreditsjust receivessigner.handle. If the goal is a guard ("fail fast if no transfer key"), the comment should say so; if the goal is to passkeyId:, it's missing the wiring. Suggest either dropping the lookup (the trampoline will fail at sign time anyway with a clear error) or passingtransferKey.idthrough the SDK if/when the API is extended. Same observation applies to the other flows that select a key purely for logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift` around lines 631 - 655, The code computes transferKey via KeyManager.findSigningKey on the MainActor but never passes its id into sdk.transferCredits (only signer.handle is used), so either remove the redundant lookup or thread transferKey.id into the transfer call / API; specifically, either delete the MainActor.findSigningKey block and its print if you only need a sign-time trampoline failure, or update the call site that invokes sdk.transferCredits (and related flows that mirror this pattern) to accept a keyId parameter and pass transferKey.id through; if you keep the pre-check as a guard, change the comment to state it is a fail-fast existence check rather than implying the key is used.
1792-1856:executeDataContractUpdatestill parses every input before throwing.The function now unconditionally throws
SDKError.notImplemented, but it parsesnewDocumentSchemas,newTokenSchemas,newGroupsand validates "at least one update" first. None of that work is observable and any of it can throw a more confusing serialization error than the actual "not yet wired" message. Move thethrow .notImplemented(...)to the top of the function so users get the real reason immediately.♻️ Proposed fix
private func executeDataContractUpdate(sdk: SDK) async throws -> Any { + throw SDKError.notImplemented( + "Data contract update is not yet wired through the platform-wallet path. " + + "Create a fresh contract for now." + ) + + /* TODO: re-enable once wallet.updateDataContract(...) lands. guard let contractId = formInputs["dataContractId"], !contractId.isEmpty else { ... } ... - _ = (contractId, dppIdentity, newDocumentSchemas, newTokenSchemas, newGroups) - throw SDKError.notImplemented( - "Data contract update is not yet wired through the platform-wallet path. " + - "Create a fresh contract for now." - ) + */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift` around lines 1792 - 1856, The function executeDataContractUpdate currently parses newDocumentSchemas, newTokenSchemas and newGroups and performs validation before unconditionally throwing SDKError.notImplemented, which can surface confusing serialization errors; move the throw SDKError.notImplemented(...) to the very beginning of executeDataContractUpdate (immediately after the signature and parameter checks like contractId/ownerIdentity if you still need those) so the function short-circuits before any JSON parsing or "at least one update" validation, and remove or comment out the unreachable parsing/validation blocks (newDocumentSchemas, newTokenSchemas, newGroups and the final "at least one update" check) to keep the code clear.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterContractSourceView.swift (1)
326-351: Classification ordering can mis-route token-schema-like payloads.
looksLikeDocumentSchemasruns first and accepts any dict whose every value has either apropertieskey ortype == "object". A token-schemas payload — keys are integer slot positions, values describe a token — could theoretically match if any token entry happens to carry atype: "object"field at its root, and would then be wrapped viawrap(documentSchemasDict:), losing the token semantics. Tightening the document-schemas check (e.g. require non-integer-only keys, or require bothtype: "object"ANDproperties) would make the two cases mutually exclusive without changing the happy path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterContractSourceView.swift` around lines 326 - 351, The document-schema detection (looksLikeDocumentSchemas) is too permissive and can misclassify token-schema payloads; modify the predicate in the looksLikeDocumentSchemas check used before wrap(documentSchemasDict:) so it cannot match token-like maps — for example require that each inner object has both a non-nil "properties" AND a "type" equal to "object", or require that at least one key is non-integer (i.e., not all keys parse as Int) before treating the dict as document-schemas; ensure the updated predicate makes looksLikeDocumentSchemas and looksLikeTokenSchemas mutually exclusive while preserving existing successful document-schema cases and still calling wrap(documentSchemasDict:) only when truly document-shaped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38da0e64-92c7-4451-adb1-7f8fdcb73bc4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
packages/rs-platform-wallet-ffi/src/data_contract.rspackages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rspackages/rs-platform-wallet-ffi/src/identity_key_preview.rspackages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_top_up.rspackages/rs-platform-wallet-ffi/src/identity_update.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/wallet/identity/network/contract.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/update.rspackages/rs-sdk-ffi/src/data_contract/mod.rspackages/rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rspackages/rs-sdk-ffi/src/token/queries/calculate_token_id.rspackages/rs-sdk-ffi/src/token/queries/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/Utils/DataContractParser.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Utils/ModelContainerHelper.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Helpers/WIFParser.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/ContractIdentityLinker.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDataContract.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityRegistrationFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/ContractDownloader.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DataContractDetailsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/GroupDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeysListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LoadIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/QuickBasicTokenView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterContractSourceView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PasteboardContractCandidateTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SDKMethodTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/SimpleTransitionTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/StateTransitionTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletTests/TransactionTests.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletTests/TransactionTests.swift
| pub fn derive_ecdsa_identity_auth_keypair_from_master( | ||
| master: &ExtendedPrivKey, | ||
| network: key_wallet::Network, | ||
| identity_index: u32, | ||
| key_index: u32, | ||
| ) -> Result<DerivedIdentityAuthKey, PlatformWalletError> { | ||
| use dashcore::secp256k1::Secp256k1; | ||
| use key_wallet::bip32::ExtendedPubKey; | ||
|
|
||
| let path = identity_auth_derivation_path_for_type( | ||
| network, | ||
| KeyDerivationType::ECDSA, | ||
| identity_index, | ||
| key_index, | ||
| )?; | ||
| let secp = Secp256k1::new(); | ||
| let derived = master.derive_priv(&secp, &path).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Failed to derive private key at (identity={identity_index}, key={key_index}): {e}" | ||
| )) | ||
| })?; | ||
| let extended_pub = ExtendedPubKey::from_priv(&secp, &derived); | ||
|
|
||
| Ok(DerivedIdentityAuthKey { | ||
| derivation_path: path, | ||
| private_key: Zeroizing::new(derived.private_key.secret_bytes()), | ||
| public_key: extended_pub.public_key.serialize(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether ExtendedPrivKey implements Zeroize/ZeroizeOnDrop in the version
# this workspace uses (key_wallet -> dashcore bip32 module).
ast-grep --pattern $'impl Zeroize for ExtendedPrivKey { $$$ }'
ast-grep --pattern $'impl ZeroizeOnDrop for ExtendedPrivKey { $$$ }'
ast-grep --pattern $'impl Drop for ExtendedPrivKey { $$$ }'
rg -nP --type=rust -C2 'struct\s+ExtendedPrivKey\b'Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Read the actual file to understand context
cat -n packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs | head -180 | tail -40Repository: dashpay/platform
Length of output: 1894
🏁 Script executed:
# Find and examine Cargo.toml to understand dependencies
fd Cargo.toml | head -5Repository: dashpay/platform
Length of output: 220
🏁 Script executed:
# Search for ExtendedPrivKey usage across the codebase
rg 'ExtendedPrivKey' -t rust -B2 -A2Repository: dashpay/platform
Length of output: 21016
🏁 Script executed:
# Search for key_wallet and dashcore import/use statements
rg 'use (key_wallet|dashcore)' -t rustRepository: dashpay/platform
Length of output: 43657
derived is implicitly zeroized via inner secp256k1::SecretKey, but consider explicit Zeroizing wrapper for defense-in-depth.
The local derived: ExtendedPrivKey does not explicitly implement ZeroizeOnDrop. However, the codebase comments confirm that the underlying secp256k1::SecretKey inside ExtendedPrivKey implements zeroization on drop, so the secret bytes are cleared when derived falls out of scope—just implicitly. If you want to make zeroization explicit and defense-in-depth, wrap the derived key:
let derived = Zeroizing::new(master.derive_priv(&secp, &path)?);
let extended_pub = ExtendedPubKey::from_priv(&secp, &*derived);Alternatively, if relying on the inner secp256k1::SecretKey drop handler is acceptable (as similar code in the FFI layer does), no change is needed; just clarify the doc comment to say "the inner secret scalar is zeroized on drop via secp256k1::SecretKey's drop handler."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs`
around lines 144 - 172, In derive_ecdsa_identity_auth_keypair_from_master, make
zeroization explicit by wrapping the derived ExtendedPrivKey in Zeroizing (i.e.,
wrap the result of master.derive_priv(...) in a Zeroizing) so the stack-held
derived key is zeroed on drop; update uses of derived (ExtendedPubKey::from_priv
and extracting private_key.secret_bytes()) to dereference the Zeroizing wrapper
accordingly. Alternatively, if you choose not to change code, update the
surrounding comment to state that the inner secp256k1::SecretKey is zeroized on
drop via its own drop handler.
…c round-trip) CI fix: - create_data_contract_with_signer: silence clippy::too_many_arguments (10 args is the right shape — every JSON section has its own slot on the FFI; bundling into a struct just shifts the lint somewhere else). Memory-safety / FFI: - derive_identity_key_at_slot.rs: replace Vec::from_raw_parts on the pubkey buffer with Box::from_raw(slice_from_raw_parts_mut(...)). The buffer is created via into_boxed_slice() so Vec::from_raw_parts is UB whenever the source vector had cap > len. Three deallocation sites updated (rollback after WIF or SecretKey failure + the paired _free). - register_identity_with_signer: contract_bounds_kind/id/document_type fields were declared on the FFI but silently dropped — every registered Encryption / Decryption key landed unbounded on Drive. Extract decode_contract_bounds() helper and call it from both register and update paths. - identity_top_up + register_identity_with_signer: BTreeMap::insert on duplicate funding rows for the same address overwrote the earlier credit value, under-funding the resulting transition. Sum saturating_add per-entry instead. - ContractDownloader call sites + dash_sdk_data_contract_fetch_with_serialization: the existing _free did Box::from_raw(result) on a stack-located result struct (UB by design), and ContractDownloader leaked json_string + serialized_data on every successful fetch. Rewrote the freer to operate on the inner pointers only and wired both Swift call sites to call it via withUnsafeMutablePointer + defer. Architecture (swift-sdk/CLAUDE.md "no mnemonic round-tripping"): - Add resolver-based dash_sdk_derive_identity_key_at_slot_with_resolver FFI mirroring the dash_sdk_derive_and_persist_identity_keys pattern. Switch ManagedPlatformWallet.deriveIdentityAuthKeyAtSlot to it; the mnemonic now never lives in a Swift String outside the resolver trampoline's stack frame. Inner derivation factored into a private helper shared by both entry points. Defense-in-depth: - identity_handle.derive_ecdsa_identity_auth_keypair_from_master: document the implicit zeroize chain (secp256k1::SecretKey owns its drop) since ExtendedPrivKey doesn't impl Zeroize and can't wrap directly. UI / behavior: - IdentityDetailView: gate Friends drill-in on identity.wallet being loaded into PlatformWalletManager; FriendsView throws on every action when the wallet is missing and never renders the resulting errorMessage. (Carried into this commit because FriendsView's body changed too.) - IdentityDetailView.reloadTokenBalances: filter PersistentToken fetch by parent contract networkRaw so off-network token ids never enter getIdentityTokenBalances. - ContractsTabView: scope @query by AppNetwork passed in from ContentView; clear keyword search results when the user pastes an id. - FriendsView: rebuild as a single List so the Incoming Requests Section actually gets sectioned styling. Qualify SwiftUI.Group to avoid shadowing by SwiftDashSDK.Group (Codable struct). - AddIdentityKeyView: validate derived scalar against returned pubkey before broadcasting; refuse to broadcast when the Keychain write fails; compute HASH160 of the compressed pubkey and ship that as pubkeyBytes when keyType == ecdsaHash160 (with the matching metadata.publicKey shape so the trampoline can look up the privkey at sign time). - ManagedPlatformWallet.pinContractBounds: precondition that the contract id is exactly 32 bytes before pinning to the FFI. - RegisterContractSourceView: gate UIPasteboard.string read behind hasStrings to avoid the iOS 14+ "Pasted from <App>" privacy banner on every foreground; preserve non-integer group keys as string ids in the dict→array conversion (was silently dropping them); sort entries by integer key when possible so the array order matches on-chain group-position semantics. - LocalDataContractsView: bounce the SDK-nil error mutations through MainActor.run for consistency with the empty-input path. - ContractIdentityLinker: drop the misleading "indexed" claim in the linkIdentityToOwnedContracts comment — `ownerId` isn't decorated with @Attribute(.indexed). Plus the new TokenActionPermissionsView (per-token actions filtered by token configuration + identity role), with TokenDetailsView exposing it via a "View Actions" entry point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift (1)
21-21:⚠️ Potential issue | 🟡 Minor
errorMessageis set but never surfaced in the UI.After the rewrite,
bodyno longer renderserrorMessageanywhere — only the empty/loading/list states. The four assignments on lines 174, 189, 239, 256, 259, 272, 275 silently no-op from the user's perspective: a contact-request sync failure, a local-state read failure, or an accept/reject error will all leave the list looking fine while the user gets no feedback.Either render the error in the existing list (e.g., a top section with the message) or surface it via an
.alertbound to a non-nilerrorMessage. The sameerrorMessagestate is still used inAddFriendView/SendDashPayPaymentSheetand is rendered there, so the pattern is just missing here.Also applies to: 174-174, 189-189, 239-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift` at line 21, The view's `@State` errorMessage in FriendsView is being assigned but never shown in the UI; update FriendsView's body to surface that state (either add a top section in the List showing errorMessage when non-nil or bind an .alert to errorMessage) so assignments to errorMessage (from contact sync, local-state reads, accept/reject flows) produce visible feedback; specifically, modify FriendsView.body to conditionally display errorMessage or attach .alert(isPresented: Binding(get: { errorMessage != nil }, set: { if !$0 { errorMessage = nil } })) and construct an Alert using errorMessage (clearing it when dismissed) so existing error setters in FriendsView actually surface to the user.
♻️ Duplicate comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swift (1)
462-470:⚠️ Potential issue | 🟡 MinorReset
isLoadingon this early return.In the token-id flow,
loadFromTokenId()has already flippedisLoadingtotruebefore awaitingloadContract(...). IfplatformState.sdkbecomesnilbetween those awaits, this branch shows the alert but leaves the sheet stuck disabled/spinning.Suggested fix
await MainActor.run { errorMessage = "SDK not initialized" showError = true + isLoading = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swift` around lines 462 - 470, In loadFromTokenId(), when the guard for platformState.sdk fails, reset isLoading to false on the MainActor before setting errorMessage/showError so the UI stops spinning; specifically, inside the existing await MainActor.run { ... } block update isLoading = false alongside errorMessage = "SDK not initialized" and showError = true (mirroring the empty-input branch) so that loadContract(...) early-return doesn't leave the sheet stuck.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
626-652:⚠️ Potential issue | 🟠 MajorReturn a recoverable error instead of aborting on bad contract-bound IDs.
These IDs come from public SDK inputs.
preconditionturns a bad 31-byte/emptyDatainto a process kill instead of a normalPlatformWalletError, which is rough for a library boundary. Please validate and throw before entering the FFI rather than crashing the host app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 626 - 652, Replace the aborting precondition checks in pinContractBounds with recoverable validation: change pinContractBounds to be throwing (private static func pinContractBounds<R>(_ bounds: ContractBounds?, _ body: (UInt8, UnsafePointer<UInt8>?, UnsafePointer<CChar>?) throws -> R) throws -> R) and validate id.count == 32 with a guard that throws a PlatformWalletError (e.g., .invalidContractBoundID) for both ContractBounds.singleContract and .singleContractDocumentType cases before calling withUnsafeBytes; update all callers of pinContractBounds to propagate/handle the thrown PlatformWalletError so bad public inputs produce a recoverable error instead of aborting the process.
🧹 Nitpick comments (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift (2)
17-17:sentRequestsis populated but never read.
sentRequestsis loaded insideloadFriends()(lines 209–215) but isn't referenced anywhere inbodyor elsewhere in the file. Either render a "Sent Requests" section (parallel to "Incoming Requests") or drop the state + thegetSentContactRequestIds()call to avoid the wasted round-trip and the dead state.Also applies to: 209-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift` at line 17, The sentRequests state is populated in loadFriends() but never used; either remove the unused state and the getSentContactRequestIds() call to avoid the unnecessary network round-trip, or render a "Sent Requests" UI section similar to the existing "Incoming Requests" and bind it to sentRequests; locate sentRequests (the `@State` var), the loadFriends() method and the getSentContactRequestIds() call and apply one of these fixes so the state and its fetch are either consumed by the UI (e.g., a Sent Requests list in body) or removed along with its fetch.
376-377:AddFriendView.selectedIdentitycan now be non-optional.The only call site (line 121–126) now always passes the injected
identity, so theOptionalwrapping plus theselectedIdentity == nildisable check (line 434) and theguard let identity = selectedIdentity(line 465) are effectively dead. Tightening the type tolet selectedIdentity: PersistentIdentityremoves a class of "what if it's nil here" branches and matches the new per-identity entry point described in the doc comment on line 6–9.♻️ Proposed change
struct AddFriendView: View { - let selectedIdentity: PersistentIdentity? + let selectedIdentity: PersistentIdentity @@ .disabled( searchText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty || isSending - || selectedIdentity == nil ) @@ private func sendRequest() { - guard let identity = selectedIdentity, - let walletId = identity.wallet?.walletId, + let identity = selectedIdentity + guard let walletId = identity.wallet?.walletId, let wallet = walletManager.wallet(for: walletId) else { errorMessage = "No wallet available for this identity" return }Also applies to: 431-435, 464-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift` around lines 376 - 377, Change AddFriendView.selectedIdentity from Optional to non-optional (let selectedIdentity: PersistentIdentity) and remove the now-dead nil handling: delete the `selectedIdentity == nil` disable check in the view (the button/field disabling logic) and remove the `guard let identity = selectedIdentity` unwrapping; instead use selectedIdentity directly wherever `identity` was derived. Update any references inside AddFriendView (e.g., the view body, submit/save handlers) to use the non-optional selectedIdentity and adjust signatures/initializers that pass it so they expect a PersistentIdentity rather than an Optional.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift (1)
866-876: Duplicated truncate-middle helper — consolidate.
truncateMiddlehere (head=8, tail=6) andSearchResultRow.truncatedId(lines 959–963, head=10, tail=6) implement the same shape with different prefixes. Plus there's a third inline copy inTokenListRow.contractCaption(lines 984–987, head=8, tail=6). Extract a single helper (e.g.String.truncatedMiddle(head:tail:)) so the visual style stays consistent across rows and any future tweak is one edit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift` around lines 866 - 876, Multiple copies of the "truncate middle" logic exist (private func truncateMiddle in ContractsTabView, SearchResultRow.truncatedId, and inline code in TokenListRow.contractCaption) with slightly different head/tail values; extract a single shared helper (e.g. add String.truncatedMiddle(head: Int = 8, tail: Int = 6) as an extension) and replace calls to truncateMiddle, SearchResultRow.truncatedId, and the inline truncation in TokenListRow.contractCaption to use that extension so all rows share the same implementation and default sizes; keep the same guard logic (return original when too short) and allow callers to override head/tail when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs`:
- Around line 473-482: The code zeroes the WIF bytes before calling
CString::from_raw, which makes from_raw recompute a shorter length and causes
UB; instead reconstruct the owned CString first and then zero its owned buffer
before deallocation. Concretely, call CString::from_raw(row.private_key_wif) to
obtain an owned CString (or convert it to a Vec via into_bytes_with_nul()),
overwrite the bytes in that owned buffer with zeros, drop the owned buffer to
free the allocation, and only then set row.private_key_wif =
std::ptr::null_mut(); also audit other _free paths (the loop variant and any
function handling private_key_wif) and apply the same reorder.
- Around line 286-300: The stack-local secret copy private_key_bytes is left on
the stack after you populate IdentityKeyPreviewFFI; to fix, avoid leaving an
unzeroized stack buffer by either (a) wrapping the buffer in zeroize::Zeroizing
(e.g., let private_key_bytes = Zeroizing::new([0u8;32]) and then copy from
derived.private_key.as_ref() before moving the inner array into the struct) or
(b) explicitly zeroize the array (private_key_bytes.zeroize() / fill with 0)
immediately after assigning *out_row so no secret remains on the stack; update
the code around private_key_bytes, the copy_from_slice call, and the *out_row
assignment in this function accordingly.
In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs`:
- Around line 148-235: The helper decode_contract_bounds currently returns
Ok(None) when row.contract_bounds_kind == 0 which allows unscoped
encryption/decryption keys; modify decode_contract_bounds to take an additional
parameter representing the parsed KeyPurpose (or pass purpose in) and if kind ==
0 and the purpose is an encryption/decryption purpose, set out_error to
PlatformWalletFFIError::new(PlatformWalletFFIResult::ErrorInvalidParameter, ...)
and return Err(PlatformWalletFFIResult::ErrorInvalidParameter) instead of
Ok(None); update all callers (e.g., where IdentityPubkeyFFI rows are parsed
during registration/update) to pass the key purpose into decode_contract_bounds
so under-scoped keys fail fast.
In `@packages/rs-platform-wallet/src/wallet/identity/network/contract.rs`:
- Around line 268-290: The code creates a CreatedDataContract via
CreatedDataContract::from_contract_and_identity_nonce (assigned to _created) but
never uses it while data_contract.put_to_platform_and_wait_for_response fetches
a fresh nonce, causing a mismatch and a misleading comment; either remove the
dead wrapper and its comment (delete the
CreatedDataContract::from_contract_and_identity_nonce call and update the
surrounding comment to reflect that put_to_platform_and_wait_for_response will
fetch its own nonce), or change the broadcast path so
put_to_platform_and_wait_for_response accepts/consumes the precomputed
CreatedDataContract/identity_nonce (i.e., add an API to accept the wrapper and
use identity_nonce consistently) — pick the simpler option (remove wrapper and
fix comment) unless you intend to make the broadcast consume the precomputed
nonce.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 865-883: In deriveIdentityAuthKeyAtSlot validate walletId.count ==
32 before calling walletId.withUnsafeBytes and avoid force-unwrapping
baseAddress; if the length is not 32 throw a clear error (e.g., an
InvalidWalletId/invalid length error) so callers get a recoverable failure, then
inside the withUnsafeBytes closure you can safely bindMemory and use
baseAddress! knowing the size check passed; update the function
(deriveIdentityAuthKeyAtSlot) to perform this guard and return/throw the chosen
error when the length is incorrect.
- Around line 512-525: The code building ffiInputs in ManagedPlatformWallet
(IdentityFundingInputFFI creation) must validate that each input.hash has
exactly 20 bytes instead of silently using .prefix(20); before mapping inputs to
ffi rows (the ffiInputs creation), check input.hash.count == 20 for each
IdentityFundingInput and reject or throw a descriptive error (or return early)
for any that do not match 20 bytes—mirroring the validation approach used in
transferCreditsToAddresses—so malformed hashes are not truncated/zero-padded and
the FFI always receives exactly 20-byte hashes.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift`:
- Around line 497-507: The success branch sets searchResults =
[SearchResult(...)] then immediately sets searchQuery = "" which triggers
onChange -> handleQueryChange and clears searchResults; to fix, preserve the
success confirmation by either (A) removing the searchQuery = "" assignment in
the MainActor.run success branches (the id-path and the token-id fallback in the
same file) so the confirmation row remains until the user clears it, or (B)
implement a short-lived guard/flag that handleQueryChange checks (e.g., suppress
one empty-query transition) so when MainActor.run in the save success constructs
SearchResult and clears searchQuery, handleQueryChange ignores that single empty
transition; update the code paths that construct SearchResult (the success block
and the token-id fallback) and the handleQueryChange/onChange logic accordingly
to avoid the immediate self-clear.
- Around line 417-421: The doc comment above handleSubmit incorrectly references
"400ms" while the debounce constant keywordDebounceMs is set to 700ms; update
the comment to reference the actual value (700ms) or reference the constant
(keywordDebounceMs) via a doc-link so the comment can’t drift—locate the
keywordDebounceMs definition and change the comment text in the handleSubmit
docblock to either "700ms" or a doc-link to keywordDebounceMs.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift`:
- Around line 526-555: resolveClaim currently allows the designated recipient
(token.newTokensDestinationIdentity) even when both token.perpetualDistribution
and token.preProgrammedDistribution are nil; change the logic so a designated
identity is only returned .allowed when at least one distribution exists.
Specifically, in resolveClaim (and using token.newTokensDestinationIdentity,
token.perpetualDistribution, token.preProgrammedDistribution,
mintingAllowChoosingDestination), either move the "no distribution at all" check
to apply before the isDesignated branch or add an extra condition to the
isDesignated branch to require hasPerpetual || hasPreProgrammed; if no
distributions exist return .hidden for the designated identity.
---
Outside diff comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift`:
- Line 21: The view's `@State` errorMessage in FriendsView is being assigned but
never shown in the UI; update FriendsView's body to surface that state (either
add a top section in the List showing errorMessage when non-nil or bind an
.alert to errorMessage) so assignments to errorMessage (from contact sync,
local-state reads, accept/reject flows) produce visible feedback; specifically,
modify FriendsView.body to conditionally display errorMessage or attach
.alert(isPresented: Binding(get: { errorMessage != nil }, set: { if !$0 {
errorMessage = nil } })) and construct an Alert using errorMessage (clearing it
when dismissed) so existing error setters in FriendsView actually surface to the
user.
---
Duplicate comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 626-652: Replace the aborting precondition checks in
pinContractBounds with recoverable validation: change pinContractBounds to be
throwing (private static func pinContractBounds<R>(_ bounds: ContractBounds?, _
body: (UInt8, UnsafePointer<UInt8>?, UnsafePointer<CChar>?) throws -> R) throws
-> R) and validate id.count == 32 with a guard that throws a PlatformWalletError
(e.g., .invalidContractBoundID) for both ContractBounds.singleContract and
.singleContractDocumentType cases before calling withUnsafeBytes; update all
callers of pinContractBounds to propagate/handle the thrown PlatformWalletError
so bad public inputs produce a recoverable error instead of aborting the
process.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swift`:
- Around line 462-470: In loadFromTokenId(), when the guard for
platformState.sdk fails, reset isLoading to false on the MainActor before
setting errorMessage/showError so the UI stops spinning; specifically, inside
the existing await MainActor.run { ... } block update isLoading = false
alongside errorMessage = "SDK not initialized" and showError = true (mirroring
the empty-input branch) so that loadContract(...) early-return doesn't leave the
sheet stuck.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift`:
- Around line 866-876: Multiple copies of the "truncate middle" logic exist
(private func truncateMiddle in ContractsTabView, SearchResultRow.truncatedId,
and inline code in TokenListRow.contractCaption) with slightly different
head/tail values; extract a single shared helper (e.g. add
String.truncatedMiddle(head: Int = 8, tail: Int = 6) as an extension) and
replace calls to truncateMiddle, SearchResultRow.truncatedId, and the inline
truncation in TokenListRow.contractCaption to use that extension so all rows
share the same implementation and default sizes; keep the same guard logic
(return original when too short) and allow callers to override head/tail when
needed.
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift`:
- Line 17: The sentRequests state is populated in loadFriends() but never used;
either remove the unused state and the getSentContactRequestIds() call to avoid
the unnecessary network round-trip, or render a "Sent Requests" UI section
similar to the existing "Incoming Requests" and bind it to sentRequests; locate
sentRequests (the `@State` var), the loadFriends() method and the
getSentContactRequestIds() call and apply one of these fixes so the state and
its fetch are either consumed by the UI (e.g., a Sent Requests list in body) or
removed along with its fetch.
- Around line 376-377: Change AddFriendView.selectedIdentity from Optional to
non-optional (let selectedIdentity: PersistentIdentity) and remove the now-dead
nil handling: delete the `selectedIdentity == nil` disable check in the view
(the button/field disabling logic) and remove the `guard let identity =
selectedIdentity` unwrapping; instead use selectedIdentity directly wherever
`identity` was derived. Update any references inside AddFriendView (e.g., the
view body, submit/save handlers) to use the non-optional selectedIdentity and
adjust signatures/initializers that pass it so they expect a PersistentIdentity
rather than an Optional.
🪄 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: 330eeff7-4627-4145-ba67-5aca0789b48f
📒 Files selected for processing (17)
packages/rs-platform-wallet-ffi/src/data_contract.rspackages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rspackages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_top_up.rspackages/rs-platform-wallet-ffi/src/identity_update.rspackages/rs-platform-wallet/src/wallet/identity/network/contract.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/ContractIdentityLinker.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterContractSourceView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenDetailsView.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegisterContractSourceView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swift
- packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
…_id tests - TokenActionPermissionsView: the bespoke `#Predicate` on `localIdentities` captured a local `networkRaw: Int` whose name shadowed the model's `identity.networkRaw` property. SwiftData's predicate translator can't resolve the ambiguity and aborts mid- fetch with `_swift_runtime_on_report` / `_assertionFailure` from inside `ModelContext.fetch` — surfaces as a crash from the `@Query` getter the moment the view body evaluates. Route through `PersistentIdentity.localIdentitiesPredicate(network:)` instead; the helper captures the raw Int as `target` so the translator stays unambiguous. - rs-sdk-ffi/calculate_token_id: add two unit tests pinning the FFI marshalling shape against `dpp::tokens::calculate_token_id` for a fixed `(contract_id, position)`, plus a null-input round-trip through `InvalidParameter`. Locks the C-ABI (CString-in, base58-round-trip, success-string-out) against future churn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift (2)
262-265: Duplicateshorthelper.Identical 4-line truncation helper lives in both
TokenActionEvaluator(262-265) andTokenActionPermissionsView(779-782). Lift to a singlefileprivatefree function (or aStringextension) to keep the two in sync if the abbreviation format ever changes.Also applies to: 779-782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift` around lines 262 - 265, Duplicate helper short(_:) exists in TokenActionEvaluator and TokenActionPermissionsView; replace both implementations with a single shared helper by extracting short(_:) into a fileprivate free function or a fileprivate String extension (e.g., fileprivate func short(_ s: String) -> String or fileprivate extension String { func abbreviated() -> String }) and update both TokenActionEvaluator and TokenActionPermissionsView to call that shared helper instead of their local copies so the truncation format stays consistent.
493-506: Distribution-change fallback mixes unrelated sub-rules.
mintingAllowChoosingDestinationRulesgoverns whether the minting destination choice can be reconfigured, andchangeDirectPurchasePricingRulesgoverns direct-purchase pricing changes — neither describes who can change the perpetual/pre-programmed distribution. Falling back through them for the "Change Distribution Rules" row can produce a row labelled "Distribution change: ..." whose denial reason is sourced from an entirely different sub-rule (and, conversely, may grant access to the distribution-edit flow based on permissions for an unrelated edit).Consider limiting the fallback to
perpetualDistributionRules ?? newTokensDestinationIdentityRules, or splitting into separate rows once the per-sub-rule edit views land (the in-file TODO at lines 490-492 already nods at this).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift` around lines 493 - 506, The "Change Distribution" row currently computes distributionRule by falling back through unrelated sub-rules (mintingAllowChoosingDestinationRules and changeDirectPurchasePricingRules), which can surface incorrect allow/deny reasons; update the distributionRule selection in TokenActionPermissionsView (the let distributionRule and subsequent ResolvedTokenAction for kind: .changeDistribution) to only fallback between distributionChangeRules?.perpetualDistributionRules and distributionChangeRules?.newTokensDestinationIdentityRules, removing mintingAllowChoosingDestinationRules and changeDirectPurchasePricingRules from the chain; optionally leave a TODO/note to split into separate rows when per-sub-rule edit views are implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift`:
- Around line 262-265: Duplicate helper short(_:) exists in TokenActionEvaluator
and TokenActionPermissionsView; replace both implementations with a single
shared helper by extracting short(_:) into a fileprivate free function or a
fileprivate String extension (e.g., fileprivate func short(_ s: String) ->
String or fileprivate extension String { func abbreviated() -> String }) and
update both TokenActionEvaluator and TokenActionPermissionsView to call that
shared helper instead of their local copies so the truncation format stays
consistent.
- Around line 493-506: The "Change Distribution" row currently computes
distributionRule by falling back through unrelated sub-rules
(mintingAllowChoosingDestinationRules and changeDirectPurchasePricingRules),
which can surface incorrect allow/deny reasons; update the distributionRule
selection in TokenActionPermissionsView (the let distributionRule and subsequent
ResolvedTokenAction for kind: .changeDistribution) to only fallback between
distributionChangeRules?.perpetualDistributionRules and
distributionChangeRules?.newTokensDestinationIdentityRules, removing
mintingAllowChoosingDestinationRules and changeDirectPurchasePricingRules from
the chain; optionally leave a TODO/note to split into separate rows when
per-sub-rule edit views are implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1eefbbd3-7149-4d3a-8428-ace45dd74a5a
📒 Files selected for processing (2)
packages/rs-sdk-ffi/src/token/queries/calculate_token_id.rspackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift
…oken actions Critical: - derive_identity_key_at_slot._free: zeroing the WIF via `write_bytes(ptr, 0, strlen)` BEFORE `CString::from_raw` is UB — `from_raw` recomputes length with strlen on the now-NUL-prefixed buffer, so it frees a 1-byte allocation against the original (len + 1)-byte allocation (rust-lang/rust#68456). Reconstruct the owned `CString` first, then scrub through its owned buffer via the original `as_bytes().len()`. Audited the other _free paths: identity_registration_with_signer.rs's loop-variant scrub already does this in the safe order (`from_raw` → `into_bytes_with_nul()` → `zeroize()`). Memory-safety: - derive_identity_key_at_slot: stage the inline 32-byte secret through `Zeroizing<[u8; 32]>` so the stack-local copy gets scrubbed when the function returns. The previous bare `[u8; 32]` lingered on the stack with the live secret until the frame was reused. Source `Zeroizing<...>` already cleared on its end; the in-flight copy did not. Validation: - `decode_contract_bounds` now takes the parsed `Purpose` and returns ErrorInvalidParameter for `kind == 0` whenever the purpose is Encryption / Decryption. Drive scopes those purposes to a contract, so an unbounded encryption/decryption key is a key Drive silently can't use. Both the registration and update call sites pass `purpose` through. - `topUpFromAddresses` and `registerIdentityFromAddresses` now reject `input.hash.count != 20` (and the optional output) up front. Earlier `.prefix(20)` would silently truncate / zero-pad malformed input and point the FFI at a different address. - `deriveIdentityAuthKeyAtSlot` rejects `walletId.count != 32` before the `bindMemory(...).baseAddress!` force-unwrap. Correctness: - contract.rs: drop the unused `_created` wrapper, the redundant pre-fetch of `identity_nonce`, and the pre-computed contract id. The SDK's `DataContractCreateTransition::new_from_data_contract` fetches a fresh nonce internally and overwrites the contract id with `generate_data_contract_id_v0(owner, nonce)` — earlier revisions called `get_identity_nonce(.., bump = true, ..)` once here and the SDK called it a second time, double-bumping the network nonce per call. Build the V1 format with a placeholder `Identifier::default()` id; the SDK regenerates the canonical one on broadcast. UX: - IdentityDetailView's Tokens section now wraps each token row in a NavigationLink to TokenActionPermissionsView with the identity pinned. (User-requested: tapping the "Coins" / token row from Identity Details should open the per-token actions screen.) - TokenActionPermissionsView.resolveClaim hides the row when neither perpetual nor pre-programmed distribution is configured, even for an identity that happens to be the designated recipient. Earlier behaviour advertised an action the token could not perform. - ContractsTabView: stop self-clearing `searchQuery` after a successful id-path lookup. The trailing `.onChange` fired `handleQueryChange("")` and wiped the success row before SwiftUI rendered it. Fix the docstring on `handleSubmit` to reference `keywordDebounceMs` (was hard-coded "400ms" while the constant is 700). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sible Claim
- Drop the explicit `chevron.right` from the row content. Allowed
rows are wrapped in a `NavigationLink`, which already draws its
own trailing chevron — the explicit one stacked on top produced
the doubled `> >` users were seeing. Lock icon stays for denied
rows (those don't get a system chevron).
- Switch `resolveClaim`'s `.hidden` branches to `.denied(reason:)`
so the row stays visible and explains why the action isn't
available — matches the rest of the screen's pattern (freeze /
unfreeze / destroy-frozen-funds all stay visible with a
"no one is authorized" reason). Hiding Claim outright made it
look like the screen was missing an action entirely. Concrete
reasons:
- "Token has no distribution schedule" when both perpetual /
pre-programmed configs are nil.
- "Not the designated distribution recipient" when the contract
doesn't allow choosing destination and the identity isn't the
pinned recipient.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`assertPersistKeyArgsLayout` was comparing both `MemoryLayout<PersistKeyArgs>.size` AND `.stride` to 72. Swift's `size` excludes trailing padding (this struct ends in three `UInt8`s at offset 64–66 with 5 bytes of trailing pad to 8-align the next array element), so size = 67 and stride = 72. Rust's `size_of` includes trailing padding, so the Rust compile-time assertion correctly pins it to 72 — but the Swift assertion required size == 72 too, which never held. Net effect: every `IdentityKeyPersister.init(keychain:)` aborted in `precondition` at construction. The persister-callback hot path is in `prePersistIdentityKeysForRegistration`, so any wallet-funded identity registration crashed the moment the user tapped Submit. The Swift struct's field offsets actually match Rust exactly (verified via `MemoryLayout.offset(of:)`); only the assertion was wrong. Switch to comparing stride only (which is the value that matches Rust's `size_of`) and add belt-and-braces field- offset checks so a future reorder on either side surfaces as a clean assertion failure rather than a misread field through `assumingMemoryBound`. Earlier review surfaced the same shape of bug in `IdentityKeyEntryFFI`'s assertion; left in place because that struct's stride happens to equal its size (last field is a `UInt32` at offset 132 → 136, already 8-aligned), so the bogus check coincidentally passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet-ffi/src/identity_update.rs (1)
89-95:⚠️ Potential issue | 🟠 MajorReject
(null, count > 0)FFI arrays instead of treating them as empty.Line 89 and Line 182 currently collapse an invalid pointer/count pair into
Vec::new(). That turns a caller bug into a successful no-op, so requested key adds/disables can be silently dropped.Suggested fix
- let add_keys: Vec<IdentityPublicKey> = if add_public_keys.is_null() - || add_public_keys_count == 0 - { + let add_keys: Vec<IdentityPublicKey> = if add_public_keys_count == 0 { Vec::new() } else { + if add_public_keys.is_null() { + if !out_error.is_null() { + *out_error = PlatformWalletFFIError::new( + PlatformWalletFFIResult::ErrorNullPointer, + "add_public_keys is null but add_public_keys_count > 0", + ); + } + return PlatformWalletFFIResult::ErrorNullPointer; + } let rows: &[IdentityPubkeyFFI] = slice::from_raw_parts(add_public_keys, add_public_keys_count); let mut keys: Vec<IdentityPublicKey> = Vec::with_capacity(rows.len()); @@ - let disable_ids: Vec<u32> = - if disable_public_key_ids.is_null() || disable_public_key_ids_count == 0 { + let disable_ids: Vec<u32> = + if disable_public_key_ids_count == 0 { Vec::new() } else { + if disable_public_key_ids.is_null() { + if !out_error.is_null() { + *out_error = PlatformWalletFFIError::new( + PlatformWalletFFIResult::ErrorNullPointer, + "disable_public_key_ids is null but disable_public_key_ids_count > 0", + ); + } + return PlatformWalletFFIResult::ErrorNullPointer; + } slice::from_raw_parts(disable_public_key_ids, disable_public_key_ids_count).to_vec() };Also applies to: 181-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_update.rs` around lines 89 - 95, The current logic treats a null pointer with a positive count (add_public_keys == null && add_public_keys_count > 0) as an empty slice, which silently drops requested keys; change the validation for add_public_keys/add_public_keys_count (and the analogous disable_public_keys/disable_public_keys_count) to treat (null, count>0) as an error: if pointer is null and count > 0 return an Err (or appropriate FFI error code) instead of constructing Vec::new(); otherwise, when count == 0 accept a null pointer as empty; when pointer is non-null use slice::from_raw_parts(add_public_keys, add_public_keys_count) and proceed to build Vec<IdentityPublicKey> from IdentityPubkeyFFI. Ensure the same check is applied to both add_public_keys/add_public_keys_count and disable_public_keys/disable_public_keys_count.
♻️ Duplicate comments (2)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
663-675:⚠️ Potential issue | 🟠 MajorReturn a recoverable error here instead of crashing the process.
ContractBoundsis public caller input on the register/update paths. Usingpreconditionhere turns a malformed 32-byte id into an app abort instead of the same recoverable validation error these APIs otherwise return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 663 - 675, The precondition checks inside the ContractBounds enum case handling (specifically the precondition in the .singleContractDocumentType and the earlier .singleContract branch) should not abort the process; replace them with recoverable validation handling by validating id.count == 32 with a guard and returning or throwing the same validation error used by the register/update paths instead of calling precondition; update the handler that invokes body(…) (the closure that produces R) to return an error/invalid-input result consistent with existing API error types so malformed ids produce a recoverable validation error rather than crashing the app.packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs (1)
376-417:⚠️ Potential issue | 🔴 CriticalBounds-check
mnemonic_lenbefore slicing the resolver buffer.After a
SUCCESScallback,mnemonic_lenis used directly in&mnemonic_buf[..mnemonic_len]. If the resolver reports a length larger thanMNEMONIC_RESOLVER_BUFFER_CAPACITY, thisextern "C"path panics instead of returning an FFI error.Suggested fix
match rc { x if x == mnemonic_resolver_result::SUCCESS => {} @@ } } + + if mnemonic_len > MNEMONIC_RESOLVER_BUFFER_CAPACITY { + if !out_error.is_null() { + *out_error = PlatformWalletFFIError::new( + PlatformWalletFFIResult::ErrorWalletOperation, + format!( + "mnemonic resolver: reported length {} exceeds buffer capacity {}", + mnemonic_len, MNEMONIC_RESOLVER_BUFFER_CAPACITY + ), + ); + } + return PlatformWalletFFIResult::ErrorWalletOperation; + } let mnemonic_str = match std::str::from_utf8(&mnemonic_buf[..mnemonic_len]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs` around lines 376 - 417, After the resolver returns SUCCESS, validate that mnemonic_len does not exceed MNEMONIC_RESOLVER_BUFFER_CAPACITY (and is not negative/zero if relevant) before slicing mnemonic_buf; if it is out of bounds, set *out_error (if not null) to a PlatformWalletFFIError with PlatformWalletFFIResult::ErrorWalletOperation and return that result instead of slicing. Update the code around mnemonic_len / mnemonic_buf in derive_identity_key_at_slot.rs (the block that creates mnemonic_str from &mnemonic_buf[..mnemonic_len]) to perform this bounds check and early-return on failure.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift (1)
493-506: Distribution change permission only consults the first non-nil sub-rule.
distributionChangeRulesbundles four independent sub-rules (perpetualDistributionRules,newTokensDestinationIdentityRules,mintingAllowChoosingDestinationRules,changeDirectPurchasePricingRules), each potentially configured with different authorized takers. The??chain only ever evaluates the first one that is non-nil — so an identity authorized on, say,newTokensDestinationIdentityRulesbut denied on a non-nilperpetualDistributionRuleswill be locked out of the entire "Change Distribution Rules" entry point even though they are entitled to edit one of the bundled rules.The inline comment already flags the intent to split this into per-sub-rule rows; until then, consider OR-ing across the four rules so any allowed sub-rule keeps the row enabled (and surfacing the specific allowed sub-rule when the placeholder lands on a real edit flow).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift` around lines 493 - 506, The current code selects only the first non-nil sub-rule from token.distributionChangeRules using the ?? chain, which wrongly ignores other independent sub-rules; instead, call TokenActionEvaluator.evaluate for each of the four sub-rules (perpetualDistributionRules, newTokensDestinationIdentityRules, mintingAllowChoosingDestinationRules, changeDirectPurchasePricingRules) and combine the results so the ResolvedTokenAction(kind: .changeDistribution) is allowed if any sub-rule allows the identity (logical OR across evaluations); keep the same feature/context string(s) and ensure you can record which sub-rule(s) granted permission so the UI can later surface the specific allowed sub-rule when the edit flow is implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 875-918: The method deriveIdentityAuthKeyAtSlot currently accepts
an arbitrary walletId which can differ from the instance's wallet and allow
deriving keys from the wrong mnemonic; fix by using the instance wallet id
instead of the parameter (or enforce equality): update
ManagedPlatformWallet.deriveIdentityAuthKeyAtSlot to remove the walletId
parameter and use self.walletId (or at minimum assert/guard walletId ==
self.walletId and throw PlatformWalletError.invalidParameter if mismatched), and
update all call sites to pass no walletId (or rely on the instance) so the FFI
call uses self.walletId.withUnsafeBytes(...) when invoking
dash_sdk_derive_identity_key_at_slot_with_resolver.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- Around line 914-923: The fetch for PersistentToken in reloadTokenBalances
currently uses try? which conflates a thrown SwiftData error with a legitimate
empty result and unconditionally clears tokenBalances; replace the try?
modelContext.fetch(descriptor) with an explicit do-catch so you can distinguish
success vs error: call let allTokens = try modelContext.fetch(descriptor) inside
do, set tokensError = nil and only set tokenBalances = [] when the fetch
succeeds and returns empty, and in catch set tokensError to the caught error and
do not clobber the existing tokenBalances (preserve prior balances on fetch
failure); refer to reloadTokenBalances, modelContext.fetch(descriptor),
tokenBalances, tokensError, and PersistentToken when making the change.
---
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/identity_update.rs`:
- Around line 89-95: The current logic treats a null pointer with a positive
count (add_public_keys == null && add_public_keys_count > 0) as an empty slice,
which silently drops requested keys; change the validation for
add_public_keys/add_public_keys_count (and the analogous
disable_public_keys/disable_public_keys_count) to treat (null, count>0) as an
error: if pointer is null and count > 0 return an Err (or appropriate FFI error
code) instead of constructing Vec::new(); otherwise, when count == 0 accept a
null pointer as empty; when pointer is non-null use
slice::from_raw_parts(add_public_keys, add_public_keys_count) and proceed to
build Vec<IdentityPublicKey> from IdentityPubkeyFFI. Ensure the same check is
applied to both add_public_keys/add_public_keys_count and
disable_public_keys/disable_public_keys_count.
---
Duplicate comments:
In `@packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs`:
- Around line 376-417: After the resolver returns SUCCESS, validate that
mnemonic_len does not exceed MNEMONIC_RESOLVER_BUFFER_CAPACITY (and is not
negative/zero if relevant) before slicing mnemonic_buf; if it is out of bounds,
set *out_error (if not null) to a PlatformWalletFFIError with
PlatformWalletFFIResult::ErrorWalletOperation and return that result instead of
slicing. Update the code around mnemonic_len / mnemonic_buf in
derive_identity_key_at_slot.rs (the block that creates mnemonic_str from
&mnemonic_buf[..mnemonic_len]) to perform this bounds check and early-return on
failure.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 663-675: The precondition checks inside the ContractBounds enum
case handling (specifically the precondition in the .singleContractDocumentType
and the earlier .singleContract branch) should not abort the process; replace
them with recoverable validation handling by validating id.count == 32 with a
guard and returning or throwing the same validation error used by the
register/update paths instead of calling precondition; update the handler that
invokes body(…) (the closure that produces R) to return an error/invalid-input
result consistent with existing API error types so malformed ids produce a
recoverable validation error rather than crashing the app.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift`:
- Around line 493-506: The current code selects only the first non-nil sub-rule
from token.distributionChangeRules using the ?? chain, which wrongly ignores
other independent sub-rules; instead, call TokenActionEvaluator.evaluate for
each of the four sub-rules (perpetualDistributionRules,
newTokensDestinationIdentityRules, mintingAllowChoosingDestinationRules,
changeDirectPurchasePricingRules) and combine the results so the
ResolvedTokenAction(kind: .changeDistribution) is allowed if any sub-rule allows
the identity (logical OR across evaluations); keep the same feature/context
string(s) and ensure you can record which sub-rule(s) granted permission so the
UI can later surface the specific allowed sub-rule when the edit flow is
implemented.
🪄 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: 32dfd337-1788-44a0-a98a-d913fe44bd74
📒 Files selected for processing (9)
packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rspackages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_update.rspackages/rs-platform-wallet/src/wallet/identity/network/contract.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TokenActionPermissionsView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swift
…rface token-fetch errors - `ManagedPlatformWallet.deriveIdentityAuthKeyAtSlot` no longer takes a `walletId: Data` parameter; uses `self.walletId` instead. The method is already scoped to a specific `ManagedPlatformWallet`, so accepting an arbitrary `walletId` let a caller derive a key from one wallet's mnemonic while attributing it to a different `ManagedPlatformWallet` instance. Defensive `self.walletId.count == 32` guard kept in place for the (unexpected) case of a malformed instance. AddIdentityKeyView call site updated. - `IdentityDetailView.reloadTokenBalances` now uses an explicit do-catch instead of `try?` for the `PersistentToken` fetch. `try?` collapsed thrown SwiftData errors into the same branch as a legitimately empty result — the user saw "No tokens" instead of the red error label the section already renders, and any previously-loaded balances got clobbered. The new shape surfaces the throw on `tokensError` and preserves the existing `tokenBalances` so a flaky reload doesn't blank out the section. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Self Reviewed |
2 participants
Issue being fixed or feature implemented
Adds first-class contracts and tokens management to the Swift example app, plus the supporting SDK plumbing — and pushes a wave of follow-on work through the same architectural rule: Swift / FFI persist, load, and bridge; high-level orchestration lives in
platform-wallet(perswift-sdk/CLAUDE.md). Identity-key add, contract create, and identity top-up all moved to that path; theKeychainSignertrampoline now owns every signing call so private-key bytes never cross the FFI; and the legacyKeyManager.find*WithPrivateKeyraw-bytes API is gone.What was done?
Contracts tab + identity tokens (initial scope)
SwiftExampleApp
dash_sdk_token_get_contract_infothen download the owning contract. Keyword paths query thekeyword-search-contractsystem contract forcontractKeywordsdocuments. Search is debounced 700 ms and sets the requiredkeyword ascorderBy on thestartsWithrange query. Contract + token@Querys are scoped to the active network so persisted rows from another network never leak into the picker after a network switch.getIdentityTokenBalancesagainst every locally-knownPersistentTokenon the active network — the parent contract'snetworkRawis the network filter).NavigationLinkinto the owner'sIdentityDetailViewwhen held locally.PersistentTokenacross saved contracts on the active network.SwiftDashSDK
PersistentDataContract.ownerIdentity↔PersistentIdentity.ownedDataContractsrelationship (.nullify). Lazy back-fill viaContractIdentityLinkerat every model insertion site (download, identity load, registration, persister-handler new-identity inserts).ContractDownloadercentralises fetch + parse + persist for tap-to-add and the manual paste sheet.SDK.calculateTokenId(contractId:position:)Swift wrapper.DataContractParsernow writesgroupsData(was previously never populated).dash_sdk_data_contract_fetch_result_freerewritten to be sound on the stack-allocated result struct thatdash_sdk_data_contract_fetch_with_serializationreturns by value (the previous version didBox::from_raw(result)on a stack address — UB whenever called as designed). The fetch now properly freesjson_string+serialized_data+ handle + error after every call; previously these leaked on every successful contract download.rs-sdk-ffi
dash_sdk_calculate_token_id(contract_id, position) -> base58. Pure protocol-formula bridge overdpp::tokens::calculate_token_id.Register Contract flow — moved to
platform-walletThe previous
dash_sdk_data_contract_createpath crashed withEXC_BAD_ACCESSinsidegrovedb_query::proofs::encoding::Op::decodeduring post-broadcast proof verification. Root cause: the rs-sdk-ffi tokio runtime defaulted to iOS's ~512 KB thread stack, while GroveDB proof recursion needs the 8 MB worker stack the platform-wallet runtime already configures.IdentityWallet::create_data_contract_with_signerinpackages/rs-platform-wallet/src/wallet/identity/network/contract.rs. Picks CRITICAL+AUTHENTICATION+ECDSA, generates the contract id from(owner, nonce), assemblesDataContractInSerializationFormatV1, validates and broadcasts viaput_to_platform_and_wait_for_response. Handles serde's tagged-enum quirk throughfrom_valueby round-tripping throughto_string+from_str, and injects$formatVersion: "0"defaults at every level (DataContractConfig,TokenConfiguration,TokenConfigurationConvention,TokenConfigurationLocalization,Group) to match the Rust serde tag camelCase shape.platform_wallet_create_data_contract_with_signerinrs-platform-wallet-ffi/src/data_contract.rs. Routed throughblock_on_worker(8 MB stack).dash_sdk_data_contract_create+assemble_v1_contract+ helpers + 7 unit tests frompackages/rs-sdk-ffi/src/data_contract/mod.rs. Module now only exposesDashSDKDataContractInfo,dash_sdk_data_contract_destroy, and the query re-exports.SDK.dataContractCreateandSDK.dataContractUpdateextension methods (~200 lines).Register Contract UI — new
RegisterContractSourceView4-option picker (Pasteboard / QR / Manual / Quick Basic Token):PasteboardContractCandidate.classify3-state: empty / invalid (with reason) / valid. Renders "JSON detected, not a valid contract — tap for details" when JSON is present but not a contract.ContractDownloader.previewContractInMemoryreturns aContractPreviewStatewith an in-memoryModelContainerso the preview reusesDataContractDetailsViewwithout writing to the on-disk store.Save to Devicebutton persists the previewed contract.QuickBasicTokenViewform that materialises the three-level$formatVersion: "0"token config tags so a hand-built token contract round-trips cleanly.Identity Keys — Add / view
AddIdentityKeyView— purpose limited to Auth / Encryption / Decryption / Transfer; security level limited to Critical / High / Medium (Master excluded); Transfer forced Critical; Encryption / Decryption require contract bounds (single contract, optional document type). Auto-assignskeyId = max(existing) + 1. ECDSA-only (BLS toggle present but disabled).IdentityPubkeyFFIextension — newcontract_bounds_kind / contract_bounds_id / contract_bounds_document_typefields on the FFI row; the SwiftpinNextrecursion gained apinContractBoundsstep that pins both the 32-byte id buffer and the optional doc-type CString.update_identity_with_external_signer— fixed the post-broadcast local state. The library was broadcasting key adds successfully but never updating the in-memoryManagedIdentity, so the new key never appeared in UI / Keychain / SwiftData. Fix adds a post-broadcast write lock that callsmanaged.add_key(...)+ bumps revision so the persister callback fires and writes the row.dash_sdk_derive_identity_key_at_slot— new single-slot ECDSA keypair derivation FFI that takes(identity_index, key_index)and returns the derived public key + path. Used byAddIdentityKeyViewto pick the next available slot without iterating in Swift.PrivateKeyView— added a Public Key display section.WIF encoding — delegated to Rust
The Swift
WIFParserwas hand-rolling Bitcoin's WIF version bytes (0x80 mainnet / 0xEF testnet), so encoded keys started with5instead ofXfor Dash mainnet. WIFParser now delegates todash_sdk_private_key_to_wif(Dash bytes 0xCC / 0xEF). DeadencodeBase58helper deleted. Rationale: crypto serialization isdpp/dashcore's job, not a Swift mirror.Identity Top Up — new
IdentityWallet::top_up_from_addressesalready existed; this PR exposes it.platform_wallet_top_up_from_addresses_with_signerinrs-platform-wallet-ffi/src/identity_top_up.rs. Takes(wallet_handle, identity_id, [IdentityFundingInputFFI], signer_handle), returns the post-transition balance throughout_new_balance. Only one signer is needed: top-ups are signed by the input addresses' private keys, not by an identity authentication key.ManagedPlatformWallet.topUpFromAddresses(identityId:inputs:addressSigner:)mirrors the registration wrapper's pinning / keepalive shape.TopUpIdentityViewmirrorsCreateIdentityView's funding-source picker but locks the wallet to the identity's owning wallet and skips the identity-pubkey + index sections. Entry point is a "Top Up Balance" button under the Balance row inIdentityDetailView, hidden for purely-local rows and for identities whose owning wallet isn't currently loaded.KeychainSignertrampoline migrationEvery state-transition signature now crosses the FFI through a
KeychainSignerinstance (the trampoline derives or reads the appropriate private key on demand and zeroes its buffers; nothing leaks across the boundary). Concretely:KeyManagerlostfindKeyWithPrivateKey,createSignerForKey,createContractSigner,createTransferSigner,createAuthenticationSigner,createDocumentSigner,findContractSigningKey,findDocumentSigningKey. New helpers (findSigningKey,rankKeys) pick the rightIdentityPublicKeywithout ever extracting bytes.KeychainManagergainedhasIdentityPrivateKey(publicKeyHex:)(existence check) and a module-levelIdentityPrivateKeyMetadatatypealias.TransitionDetailViewmigrated all 14 call sites to the trampoline pattern.executeDataContractCreatenow callswallet.createDataContract(...).executeDataContractUpdateis stubbednotImplementedpending its own platform-wallet path.Tests
PasteboardContractCandidateTests(12 XCTest cases) for the classifier.WalletTests/TransactionTests.swift(referenced removedSDKTransactionBuilder).How Has This Been Tested?
SwiftExampleAppfor iPhone 16 simulator (iOS 18.6) — clean build, only the pre-existing__eh_framelinker warning. No new warnings or errors.rs-unified-sdk-ffiforaarch64-apple-ios-simviapackages/swift-sdk/build_ios.sh --target sim --profile dev— clean.cargo check -p platform-wallet-ffi -p rs-sdk-fficlean.cargo fmt --all -- --checkpasses.Breaking Changes
None — no consensus-breaking changes. The Swift / FFI surface evolved (a few
publicsymbols on the iOS SDK and one rs-sdk-ffiextern "C"were removed in favour of theplatform-wallet-routed replacements), but the Platform protocol and on-chain state-transition shapes are untouched.Checklist
Summary by CodeRabbit
New Features
Updates