Add backwards-compatibility for UMA v0 from v1 SDK #31
Conversation
| * The minimum and maximum amounts that can be sent in this currency and converted from SATs by | ||
| * the receiver. | ||
| */ | ||
| val convertible: CurrencyConvertible, |
There was a problem hiding this comment.
It seems problematic to me that the caller needs to downcast to either V1 or V0 to get the min and max sendable values. These should be mapped up to interface values instead so that callers don't need to care which they're using.
There was a problem hiding this comment.
I think this is okay and the caller doesnt need to downcast based on lightsparkdev/kotlin-sdk@d417b30#r140038947, lmk what you think!
There was a problem hiding this comment.
Well wouldn't they still need to when parsing a currency object from their counterparty? The demo vasp doesn't really do this right now because it doesn't check min/max boundaries, but it probably should. Try to handle these cases in the demo vasp and you'll see what I mean:
- As the sending vasp, I need to tell my user what the min and max sendable amounts are in the receiving currency, and should error out if my user tries to send outside those bounds in the payreq.
- As the receiving vasp, I should reject a payreq outside of my currency's min/max range.
| } | ||
|
|
||
| @Serializable | ||
| data class CurrencyV1( |
There was a problem hiding this comment.
I don't love that callers need to know anything about whether to construct a CurrencyV1 or CurrencyV0 object. If you look at how the other SDKs behave, the versioning for this object is totally abstracted away from consumers of the SDK. They just construct a Currency object which gets serialized and deserialized as needed via an internal uma version field.
We can definitely still have separate V1 and V0 objects for each class, but those should really just be internal to the SDK and not even exposed publicly if possible.
| @OptIn(ExperimentalSerializationApi::class) | ||
| @Serializable | ||
| data class PayReqResponse( | ||
| data class PayReqResponseV1( |
There was a problem hiding this comment.
We should consider making this an internal constructor and ideally even have the whole version-specific objects be internal if possible. Users shouldn't be directly constructing these.
There was a problem hiding this comment.
updated all of the version specific objects to be internal!
| @Serializable(with = ByteArrayAsHexSerializer::class) | ||
| private val signingPubKey: ByteArray?, | ||
| @Serializable(with = ByteArrayAsHexSerializer::class) |
There was a problem hiding this comment.
Do these also need a default value for future-proofing?
| * @return the [Currency] to be sent to the sender VASP. | ||
| */ | ||
| @JvmOverloads | ||
| fun getCurrency( |
There was a problem hiding this comment.
nit (although I know I'm bad about this in a few places):
| fun getCurrency( | |
| fun createCurrency( |
| * @return the [Currency] to be sent to the sender VASP. | ||
| */ | ||
| @JvmOverloads | ||
| fun getCurrency( |
There was a problem hiding this comment.
Lets add a @file:JvmName annotation so that Java callers don't get the gross CurrencyKt name here.
There was a problem hiding this comment.
I think the interface name clashes with @file:JvmName("Currency") - updated to be CurrencyUtils
| * The minimum and maximum amounts that can be sent in this currency and converted from SATs by | ||
| * the receiver. | ||
| */ | ||
| val convertible: CurrencyConvertible, |
There was a problem hiding this comment.
Well wouldn't they still need to when parsing a currency object from their counterparty? The demo vasp doesn't really do this right now because it doesn't check min/max boundaries, but it probably should. Try to handle these cases in the demo vasp and you'll see what I mean:
- As the sending vasp, I need to tell my user what the min and max sendable amounts are in the receiving currency, and should error out if my user tries to send outside those bounds in the payreq.
- As the receiving vasp, I should reject a payreq outside of my currency's min/max range.
…ess/uma-kotlin-sdk into bwd
No description provided.