Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive credential storage tests for both Swift and Kotlin platforms. The changes replace simple placeholder tests with production-ready storage implementations and test suites that verify keystore encryption, blob storage, and credential management functionality.
Changes:
- Adds storage provider implementations for iOS (keychain-based) and Android (Android Keystore)
- Implements atomic blob storage for both platforms with file-based persistence
- Creates comprehensive test suites covering encryption, storage, and credential operations
- Updates build configurations to support the new test structure
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| swift/tests/WalletKitTests/TestHelpers.swift | Test utilities for creating temp directories and managing keychain cleanup |
| swift/tests/WalletKitTests/SimpleTest.swift | Removed placeholder test file |
| swift/tests/WalletKitTests/DeviceKeystoreTests.swift | Tests for keystore seal/open operations and associated data validation |
| swift/tests/WalletKitTests/CredentialStoreTests.swift | Comprehensive tests for credential storage, caching, and proof disclosure |
| swift/tests/WalletKitTests/AtomicBlobStoreTests.swift | Tests for atomic file operations (write, read, delete) |
| swift/tests/WalletKitTests/AuthenticatorTests.swift | Extensive tests for U256 wrapper and authenticator initialization |
| swift/tests/Package.swift | Updates package dependencies and removes macOS platform support |
| swift/test_swift.sh | Simplifies build paths and file copying logic |
| swift/support/IOSStorageProvider.swift | iOS storage provider implementation with default factory method |
| swift/support/IOSDeviceKeystore.swift | iOS keychain-based keystore with fallback mechanism |
| swift/support/IOSAtomicBlobStore.swift | iOS file-based atomic blob storage |
| kotlin/walletkit-tests/src/test/kotlin/org/world/walletkit/TestHelpers.kt | Test utilities with in-memory implementations for testing |
| kotlin/walletkit-tests/src/test/kotlin/org/world/walletkit/SimpleTest.kt | Removed placeholder test file |
| kotlin/walletkit-tests/src/test/kotlin/org/world/walletkit/DeviceKeystoreTests.kt | Tests for keystore encryption operations |
| kotlin/walletkit-tests/src/test/kotlin/org/world/walletkit/CredentialStoreTests.kt | Tests for credential storage and cache operations |
| kotlin/walletkit-tests/src/test/kotlin/org/world/walletkit/AtomicBlobStoreTests.kt | Tests for atomic blob storage operations |
| kotlin/walletkit-android/src/main/kotlin/org/world/walletkit/storage/AndroidStorageProvider.kt | Android storage provider with default factory |
| kotlin/walletkit-android/src/main/kotlin/org/world/walletkit/storage/AndroidDeviceKeystore.kt | Android KeyStore-based secure key storage |
| kotlin/walletkit-android/src/main/kotlin/org/world/walletkit/storage/AndroidAtomicBlobStore.kt | Android file-based atomic storage |
| kotlin/settings.gradle.kts | Updates module structure to include Android library and test modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func deleteKeychainItem(service: String, account: String) { | ||
| let query: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: service, | ||
| kSecAttrAccount as String: account | ||
| ] | ||
| SecItemDelete(query as CFDictionary) |
There was a problem hiding this comment.
The SecItemDelete return status is not checked, which could hide potential errors when cleaning up keychain items. Consider checking the return status and handling error cases, especially if the operation might fail silently in production environments.
| @Test | ||
| fun sealAndOpenRoundTrip() { | ||
| val keystore = InMemoryDeviceKeystore() | ||
| val associatedData = "ad".encodeToByteArray() | ||
| val plaintext = "hello".encodeToByteArray() | ||
|
|
||
| val ciphertext = keystore.seal(associatedData, plaintext) | ||
| val opened = keystore.openSealed(associatedData, ciphertext) | ||
|
|
||
| assertTrue(opened.contentEquals(plaintext)) | ||
| } |
There was a problem hiding this comment.
The test doesn't verify that the ciphertext is different from the plaintext. Consider adding an assertion to ensure encryption actually transforms the data.
| public final class IOSStorageProvider: StorageProvider { | ||
| private let keystoreImpl: IOSDeviceKeystore | ||
| private let blobStoreImpl: IOSAtomicBlobStore | ||
| private let pathsImpl: StoragePaths | ||
|
|
||
| public init( | ||
| rootDirectory: URL, | ||
| keystoreService: String = "walletkit.devicekeystore", | ||
| keystoreAccount: String = "default" | ||
| ) throws { | ||
| let worldidDir = rootDirectory.appendingPathComponent("worldid", isDirectory: true) | ||
| do { | ||
| try FileManager.default.createDirectory( | ||
| at: worldidDir, | ||
| withIntermediateDirectories: true | ||
| ) | ||
| } catch { | ||
| throw StorageError.BlobStore("failed to create storage directory: \(error)") | ||
| } | ||
|
|
||
| self.pathsImpl = StoragePaths.fromRoot(root: rootDirectory.path) | ||
| self.keystoreImpl = IOSDeviceKeystore( | ||
| service: keystoreService, | ||
| account: keystoreAccount | ||
| ) | ||
| self.blobStoreImpl = IOSAtomicBlobStore(baseURL: worldidDir) | ||
| } | ||
|
|
||
| public func keystore() -> DeviceKeystore { | ||
| keystoreImpl | ||
| } | ||
|
|
||
| public func blobStore() -> AtomicBlobStore { | ||
| blobStoreImpl | ||
| } | ||
|
|
||
| public func paths() -> StoragePaths { | ||
| pathsImpl | ||
| } | ||
| } |
There was a problem hiding this comment.
Public classes and methods in the storage provider lack documentation. Consider adding KDoc comments to explain the purpose, parameters, and any important behavior such as thread-safety guarantees and error conditions for the public API.
| class AndroidDeviceKeystore( | ||
| private val alias: String = "walletkit_device_key" | ||
| ) : DeviceKeystore { | ||
| private val lock = Any() |
There was a problem hiding this comment.
Public classes and methods lack documentation. Consider adding KDoc comments to explain the purpose, thread-safety guarantees (synchronized block usage), and error conditions for the public API.
| override fun writeAtomic(path: String, bytes: ByteArray) { | ||
| val file = File(baseDir, path) | ||
| val parent = file.parentFile | ||
| if (parent != null && !parent.exists()) { | ||
| parent.mkdirs() | ||
| } | ||
| val temp = File( | ||
| parent ?: baseDir, | ||
| "${file.name}.tmp-${UUID.randomUUID()}" | ||
| ) | ||
| try { | ||
| temp.writeBytes(bytes) | ||
| if (file.exists() && !file.delete()) { | ||
| throw StorageException.BlobStore("failed to remove existing file") | ||
| } | ||
| if (!temp.renameTo(file)) { | ||
| temp.copyTo(file, overwrite = true) | ||
| temp.delete() | ||
| } | ||
| } catch (error: Exception) { | ||
| throw StorageException.BlobStore("write failed: ${error.message}") | ||
| } |
There was a problem hiding this comment.
The iOS implementation uses .atomic option for writes which is good, but the Android implementation manually manages temp files and rename operations. There's a potential race condition where multiple threads could create temp files with the same UUID (extremely unlikely but theoretically possible). Consider adding synchronized blocks or using the lock object for thread safety in the Android implementation.
| if (file.exists() && !file.delete()) { | ||
| throw StorageException.BlobStore("failed to remove existing file") | ||
| } | ||
| if (!temp.renameTo(file)) { | ||
| temp.copyTo(file, overwrite = true) | ||
| temp.delete() | ||
| } |
There was a problem hiding this comment.
The writeAtomic implementation doesn't handle the case where file.delete() succeeds but temp.renameTo(file) fails. In this scenario, the original file is deleted but the new file isn't in place, potentially causing data loss. Consider checking the rename operation status and attempting recovery or throwing a more specific error.
| if (file.exists()) { | ||
| file.delete() | ||
| } | ||
| if (!temp.renameTo(file)) { | ||
| temp.copyTo(file, overwrite = true) | ||
| temp.delete() | ||
| } |
There was a problem hiding this comment.
The test helper writeAtomic has the same issue as the Android implementation. After deleting the existing file, if temp.renameTo(file) fails, the fallback is temp.copyTo(file, overwrite: true), but at this point the original file is already deleted. If the copy also fails, data could be lost. This pattern could mask issues in production code.
| temp.writeBytes(bytes) | ||
| if (file.exists() && !file.delete()) { | ||
| throw StorageException.BlobStore("failed to remove existing file") | ||
| } | ||
| if (!temp.renameTo(file)) { | ||
| temp.copyTo(file, overwrite = true) | ||
| temp.delete() | ||
| } |
There was a problem hiding this comment.
The temp file is not cleaned up if the writeBytes operation throws an exception before the rename/copy operations. This could leave orphaned temp files in the directory. Consider wrapping in a try-finally block to ensure cleanup.
| temp.writeBytes(bytes) | |
| if (file.exists() && !file.delete()) { | |
| throw StorageException.BlobStore("failed to remove existing file") | |
| } | |
| if (!temp.renameTo(file)) { | |
| temp.copyTo(file, overwrite = true) | |
| temp.delete() | |
| } | |
| try { | |
| temp.writeBytes(bytes) | |
| if (file.exists() && !file.delete()) { | |
| throw StorageException.BlobStore("failed to remove existing file") | |
| } | |
| if (!temp.renameTo(file)) { | |
| temp.copyTo(file, overwrite = true) | |
| } | |
| } finally { | |
| if (temp.exists()) { | |
| temp.delete() | |
| } | |
| } |
| func testSealAndOpenRoundTrip() throws { | ||
| let service = uniqueKeystoreService() | ||
| defer { deleteKeychainItem(service: service, account: account) } | ||
|
|
||
| let keystore = IOSDeviceKeystore(service: service, account: account) | ||
| let associatedData = Data("ad".utf8) | ||
| let plaintext = Data("hello".utf8) | ||
|
|
||
| let ciphertext = try keystore.seal( | ||
| associatedData: associatedData, | ||
| plaintext: plaintext | ||
| ) | ||
| let opened = try keystore.openSealed( | ||
| associatedData: associatedData, | ||
| ciphertext: ciphertext | ||
| ) | ||
|
|
||
| XCTAssertEqual(opened, plaintext) | ||
| } |
There was a problem hiding this comment.
The test doesn't verify that the ciphertext is actually different from the plaintext, which is an important property of encryption. Consider adding an assertion to ensure that encryption actually transforms the data.
| class AndroidStorageProvider( | ||
| private val rootDir: File, | ||
| private val keystoreImpl: AndroidDeviceKeystore = AndroidDeviceKeystore(), | ||
| private val blobStoreImpl: AndroidAtomicBlobStore = | ||
| AndroidAtomicBlobStore(File(rootDir, "worldid")) | ||
| ) : StorageProvider { | ||
| private val pathsImpl: StoragePaths = StoragePaths.fromRoot(rootDir.absolutePath) | ||
|
|
||
| init { | ||
| val worldidDir = File(rootDir, "worldid") | ||
| if (!worldidDir.exists() && !worldidDir.mkdirs()) { | ||
| throw StorageException.BlobStore("failed to create storage directory") | ||
| } | ||
| } | ||
|
|
||
| override fun keystore(): DeviceKeystore = keystoreImpl | ||
|
|
||
| override fun blobStore(): AtomicBlobStore = blobStoreImpl | ||
|
|
||
| override fun paths(): StoragePaths = pathsImpl | ||
| } |
There was a problem hiding this comment.
Public classes and methods lack documentation. Consider adding KDoc comments to explain the purpose, parameters, thread-safety guarantees, and error conditions for the public API.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| - name: Compress XCFramework binary | ||
| run: | | ||
| zip -r WalletKit.xcframework.zip WalletKit.xcframework | ||
| zip -r WalletKit.xcframework.zip swift/WalletKit.xcframework |
There was a problem hiding this comment.
Zip embeds XCFramework under nested path breaking SPM
High Severity
The zip command zip -r WalletKit.xcframework.zip swift/WalletKit.xcframework stores the framework inside the archive under the path swift/WalletKit.xcframework/.... Swift Package Manager binary targets require the .xcframework bundle to be at the root of the zip archive. Since it's now nested under swift/, SPM won't locate the framework when consumers resolve the package, breaking the entire Swift release distribution.
| exit 1 | ||
| fi | ||
|
|
||
| cargo metadata --no-deps --format-version 1 | jq -r '.workspace_members' |
There was a problem hiding this comment.
Debug cargo metadata print left in release workflow
Low Severity
The line cargo metadata --no-deps --format-version 1 | jq -r '.workspace_members' prints workspace members to stdout without storing or using the result. This looks like a leftover debug statement that was not removed before committing.
|
closed in favor of #271 |
Note
Medium Risk
Changes restructure CI and release automation (new Kotlin job, runner changes, feature-flagged test/build commands, and updated publish paths), which could break builds or artifact publication if assumptions are wrong. No runtime product code is modified.
Overview
CI now cancels superseded PR runs, upgrades
actions/checkouttov6, and tightens Rust linting by running Clippy across all features, default features, and no default features with-D warnings.Adds Swift test linting (
swiftlint) and a new Kotlin build/test job that runs./kotlin/test_kotlin.shplusktlint. The Rust test job switches runners and stops running--all-features, explicitly excluding expensivecompress-zkeyswhile still testing selected non-default features.Release workflows are updated to align Swift/Kotlin artifact paths and publishing (Swift assets under
swift/, Kotlin JNI libs and binding generation underkotlin/walletkit), build Android artifacts withcompress-zkeys, and finalize the Swift repo release viagh release edit. Version discovery for initiating releases is changed to usecargo metadatainstead of greppingCargo.toml, and.gitignore/docs add corresponding housekeeping and guidance.Written by Cursor Bugbot for commit c3ed2a4. This will update automatically on new commits. Configure here.