Replace shared(apiKey:) with configure() and setJWT#91
Replace shared(apiKey:) with configure() and setJWT#91tomas-martins-crossmint wants to merge 12 commits into
Conversation
…ogout on CrossmintSDK
… crossmintTEE in body
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
Sources/CrossmintClient/SwiftUI/CrossmintSDK.swift:44-46
Missing indentation on `private let sdk`. The property declaration is flush against the left margin instead of being indented to match the rest of the class body, which appears to be a diff artifact from the refactor.
```suggestion
private let sdk: ClientSDK
public let crossmintWallets: CrossmintWallets
```
### Issue 2 of 2
Sources/CrossmintClient/SwiftUI/View+NonCustodialSigner.swift:37
`CrossmintSDK.shared` is accessed on every SwiftUI render pass inside `body(content:)`. Because `crossmintTEE` is a `let` constant on the singleton this always returns the same object and is functionally correct, but accessing the singleton each render is inconsistent with how the old code captured the reference at init time. If `configure()` has not been called before the first render (e.g. if the modifier is somehow applied before `init()` runs), this will `fatalError` with a less obvious callsite than a misconfiguration at startup.
Reviews (1): Last reviewed commit: "Remove Configuration struct — add back i..." | Re-trigger Greptile |
| do { | ||
| _ = try await authManager.logout() | ||
| } catch { | ||
| Logger.sdk.warn("Logout request failed: \(error) — clearing local state anyway") |
There was a problem hiding this comment.
what can happen here if we're not logged out but clear local state? What's the difference?
There was a problem hiding this comment.
the intent was to avoid locking users out if the logout endpoint request fails. But on the current implementation, if that request throws, we'd also never clear the user's JWT and refresh tokens from keychain, which could lead to the user getting automatically logged in on the next app launch, if the client doesn't have any safeguards to that
the only part that can throw is the logout network call, so we could clear the credentials before making the request instead, so even if that fails, the client would still lose access to the JWT and the user wouldn't end up accidentally being signed in again, regardless of the request result
| let components = Self.makeComponents(apiKey: apiKey) | ||
| sdk = components.sdk | ||
| crossmintWallets = components.wallets | ||
| authManager = components.authManager | ||
| crossmintService = components.service | ||
| crossmintTEE = components.tee |
There was a problem hiding this comment.
this is what I mean, I think it stems also from the try/catch maybe? Which step can actually throw? If we want to condense the code, i'd be thinking more:
do {
let sdk = try CrossmintClient.sdk(key: apiKey)
} catch {
...
}
initialize(with: sdk)
with initialize being
self.crossmintWallets = sdk.crossmintWallets()
self.authManager = sdl.authManager
self.crossmintService = sdk.crossmintService
self.crossmintTEE = CrossmintTEE.start(
...
There was a problem hiding this comment.
yeah, on a second look the whole Components was just some unnecessary ceremony. I've inlined everything into the initializer
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
Sources/CrossmintClient/SwiftUI/CrossmintSDK.swift:17-25
**DEBUG env-var escape hatch removed**
The previous `private convenience init()` supported a `#if DEBUG` branch that would read `CROSSMINT_API_KEY` from `ProcessInfo.processInfo.environment`, allowing Xcode Previews and simulator runs to boot the SDK without an explicit `configure()` call. That path is gone. Any `#Preview` block that renders a view whose `.task { }` or `init` accesses `CrossmintSDK.shared` (e.g. `SplashScreen`, whose `authenticate()` is invoked via `.task`) will now unconditionally hit the `fatalError`. This is a DX regression for teams that relied on the env-var injection for quick previewing or CI simulator tests.
Reviews (2): Last reviewed commit: "Remove Components struct, inline SDK ini..." | Re-trigger Greptile |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
Sources/CrossmintClient/SwiftUI/CrossmintSDK.swift:29-36
**`DEBUG` env-var escape hatch removed — SwiftUI previews will now crash**
The previous no-arg `init()` checked `ProcessInfo.processInfo.environment["CROSSMINT_API_KEY"]` in `#if DEBUG` and used it to self-configure, giving previews a zero-config path. That fallback is gone from `configure()`. Any `#Preview { SplashScreen() }` (or similar) will now hit the `fatalError` in `shared` because `configure()` was never called in that process. Developers who relied on the env-var shortcut for local iteration will need an alternative (e.g. a no-op stub or a preview-specific `configure()` call in the preview block).
Reviews (3): Last reviewed commit: "Capture crossmintTEE reference at modifi..." | Re-trigger Greptile |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
Sources/CrossmintClient/SwiftUI/CrossmintSDK.swift:40-42
`crossmintService` is exposed as `public let`, which makes the internal service layer part of the SDK's public API surface. Previously only `isProductionEnvironment: Bool` was public (and it still is, as a computed property on line 56). Any consumer that stores a reference to `crossmintService` today will become a dependant of that internal type, making it harder to refactor or change the service layer later without a breaking change.
```suggestion
public let crossmintWallets: CrossmintWallets
public let authManager: CrossmintAuthManager
let crossmintService: CrossmintService
```
Reviews (4): Last reviewed commit: "Merge main, resolve conflicts" | Re-trigger Greptile |
The old
CrossmintSDK.shared(apiKey:authManager:logLevel:)was a factory method pretending to be a property. You had to pass aCrossmintAuthManagerto it, keep that instance around separately for JWT auth and logout, and call two different methods just to sign out.This pull request replaces it with a configure-then-access pattern and moves auth management fully onto the singleton.
Changes
shared(apiKey:authManager:logLevel:)withconfigure(apiKey:logLevel:).sharedis now read-only and crashes with a clear message if accessed before configuration.setJWT(_ jwt: String) asyncdirectly onCrossmintSDK. No more keeping a separateCrossmintAuthManagerreference just to set a JWT.logout()now handles both auth cleanup and TEE reset in one call and dropsthrows, since errors were always swallowed internally.crossmintNonCustodialSigner()is now no-arg. Passing the SDK instance was redundant since there's only one.authManageris typed asCrossmintAuthManagerthroughout, removing the internal force cast.API changes
This pull request introduces breaking changes, requiring small changes from end users of the SDK:
SDK initialization
JWT authentication
Logout
Non-custodial signer modifier