CryptoOnramp SDK: API Error Refactor - #13156
Conversation
jeanregisser
left a comment
There was a problem hiding this comment.
This is looking very close to the direction I had in mind, and it lines up well with the iOS shape now: concrete rich errors, API-specific context, safe message / userMessage, richer developerMessage, and a shared renderer for the final diagnostic format.
One thing I’d like to get your take on: should the common rich-error contract be source-agnostic and named similarly to iOS, e.g. StripeCryptoOnrampError?
Right now CryptoOnrampException is the base rich-error type, but because it extends StripeException, the common error surface is effectively tied to API-backed failures. That works well for AppAttestationException / UncategorizedApiErrorException, but I think we’ll want future non-API rich errors to participate in the same rendering / DevX model without needing to pretend they are Stripe API exceptions.
Maybe the split could be:
interface StripeCryptoOnrampError {
val code: String
val userMessage: String
val developerMessage: String
val sdkVersion: String
val docUrl: String?
val underlyingError: Throwable?
}Then API-backed errors can still extend StripeException and implement this interface.
The main downside I see is catch ergonomics: callers can’t catch (e: StripeCryptoOnrampError) if it’s only an interface, so they’d need to catch Throwable / Exception and check if (e is StripeCryptoOnrampError). That may or may not be worth it, but I think it keeps the rich-error contract cleaner and better aligned with the longer-term direction.
Curious what you think. I don’t feel strongly that this PR needs to solve every future case, but I’d like us to avoid making the shared rich-error shape API-specific if we can.
I went back and forth on this, especially around the conformance to StripeException in general. I've switched to doing what you suggested here, as it's probably the most correct. |
jeanregisser
left a comment
There was a problem hiding this comment.
This is looking very aligned with the iOS direction now. I like the split between the source-agnostic StripeCryptoOnrampError interface and API-backed CryptoOnrampApiException; that feels like the right foundation for future non-API rich errors too.
A couple small alignment points I’d consider:
-
Can
StripeCryptoOnrampError.codebe non-null? I think every rich error should have a stable SDK-owned code. For API errors, we can still prefer the backend code when present, but concrete errors should provide a fallback like iOS does. -
Can we keep
userMessageSDK-owned for now, and preserve backenduser_messageonly as API context? Right now Android usesapiUserMessage ?: fallbackUserMessage, while iOS always uses SDK-owned copy forAppAttestationAPIError/UncategorizedAPIErrorand exposesapiUserMessageseparately. We may later decide API v2user_messageshould take precedence, but I’d rather make that decision intentionally across both SDKs.
Overall this feels like a strong foundation and very close to where we want this to land.
Summary
Refactors API errors to adopt what's being proposed in Proposal: Errors are a UI for attestation errors.
We added a new abstract error type
CryptoOnrampExceptionwith two concrete cases so far:AppAttestationException: This uses app attestation failures as a first step to adopt the new API surface for errors according to the linked doc. While there's no attestation-specific error properties on AppAttestationException, they could be added to this concrete associated value, keying off type to know to parse them.UncategorizedApiErrorException: Captures all other API errors that we're not specifically targeting yet so that they share a common API surface to other API errors.CryptoOnrampExceptionexposesuserMessageanddeveloperMessagefor all types.Motivation
Proposal: Errors are a UI
Testing
Changelog
Updated