diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47KotlinSerializationNullTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47KotlinSerializationNullTest.kt index 60696efab3..6e25c9bc33 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47KotlinSerializationNullTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47KotlinSerializationNullTest.kt @@ -34,9 +34,13 @@ import kotlin.test.assertNull /** * Exercises the **kotlinx** (native/iOS) NWC serializers directly — not through * `OptimizedJsonMapper`, whose JVM actual is Jackson — to cover the cross-backend - * asymmetry: Jackson (JVM/Android) writes explicit `null` for every null field, and a - * native peer parsing that output must read those as real nulls, not the string "null", - * and must not crash on a null `metadata` object. + * asymmetry on the DECODE side: a peer may write an explicit `null` for a field it + * has nothing to say about, and a native client parsing that must read it as a real + * null, not the string "null", and must not crash on a null `metadata` object. + * + * Our own Jackson backend no longer emits those on request params — see + * [com.vitorpamplona.quartz.nip01Core.jackson.OmitNullsMixin] — but a third-party + * wallet still may, so tolerating them on the way in remains required. */ class Nip47KotlinSerializationNullTest { @Test diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt new file mode 100644 index 0000000000..a773123437 --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt @@ -0,0 +1,144 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.nip47WalletConnect + +import com.vitorpamplona.quartz.nip01Core.core.OptimizedJsonMapper +import com.vitorpamplona.quartz.nip47WalletConnect.kotlinSerialization.Nip47RequestKSerializer +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.CancelHoldInvoiceMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.CreateConnectionMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.ListTransactionsMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.LookupInvoiceMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.MakeHoldInvoiceMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.MakeInvoiceMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayInvoiceMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayKeysendMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.ReceiveMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.Request +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.SettleHoldInvoiceMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.SignMessageMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.TlvRecord +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonArray +import kotlinx.serialization.json.JsonElement +import kotlinx.serialization.json.JsonNull +import kotlinx.serialization.json.JsonObject +import kotlinx.serialization.json.jsonObject +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +/** + * NIP-47 marks request parameters optional, and a wallet may type one strictly. + * Sending an absent parameter as an explicit `null` earned + * `Invalid list_transactions params: from must be an integer` from a real wallet + * and failed the request outright. + * + * EVERY params-bearing method is listed here on purpose. The Jackson mixin + * registrations that fix this are a hand-maintained list, and unlike the two + * `when` blocks over the sealed `Request` they are not compiler-checked — so a + * thirteenth method can be added, serialize correctly on native, and regress on + * JVM/Android alone. This list is the only thing that would catch that. + */ +class Nip47NullParamOmissionTest { + private val requests: List> = + listOf( + // The reported failure: five of eight fields absent. + "list_transactions" to ListTransactionsMethod.create(limit = 20, offset = 0, unpaid = false), + "pay_invoice" to PayInvoiceMethod.create("lnbc50n1abc"), + "pay" to PayMethod.create("bitcoin:?lno=lno1abc"), + "receive" to ReceiveMethod.create(amount = 21000L), + "pay_keysend" to PayKeysendMethod.create(amount = 21000L, pubkey = "0266e4"), + // Reaches the NESTED TlvRecord, with one of its two optional fields absent. + // A record is only checked when the list is non-empty, so the plain + // pay_keysend fixture above never executes this path. + "pay_keysend+tlv" to + PayKeysendMethod.create( + amount = 21000L, + pubkey = "0266e4", + tlvRecords = listOf(TlvRecord(type = 5482373484L)), + ), + "make_invoice" to MakeInvoiceMethod.create(amount = 21000L), + "lookup_invoice" to LookupInvoiceMethod.createByHash("31afdf1"), + "make_hold_invoice" to MakeHoldInvoiceMethod.create(amount = 21000L, paymentHash = "31afdf1"), + "cancel_hold_invoice" to CancelHoldInvoiceMethod.create("31afdf1"), + "settle_hold_invoice" to SettleHoldInvoiceMethod.create("0123456789abcdef"), + "sign_message" to SignMessageMethod.create("hello"), + "create_connection" to CreateConnectionMethod.create(pubkey = "abc123", name = "app"), + ) + + @Test + fun noRequestEverSendsANullParam() { + requests.forEach { (name, request) -> + val json = OptimizedJsonMapper.toJson(request) + val params = Json.parseToJsonElement(json).jsonObject["params"] as? JsonObject + + // RECURSIVE: `tlv_records` holds objects with optional fields of their own, + // so a null can hide a level below the params object. + params?.let { assertNoNulls(it, name, json) } + } + } + + private fun assertNoNulls( + element: JsonElement, + name: String, + json: String, + ) { + when (element) { + is JsonObject -> + element.forEach { (key, value) -> + assertTrue(value !is JsonNull, "$name sent \"$key\": null - omit it instead. Full: $json") + assertNoNulls(value, name, json) + } + + is JsonArray -> element.forEach { assertNoNulls(it, name, json) } + else -> Unit + } + } + + /** + * The document the failing wallet rejected, now minimal. Asserted as a KEY SET + * rather than a literal string: which keys travel is the property under test, + * while their order is each backend's own business. + */ + @Test + fun listTransactionsCarriesOnlyWhatWasAsked() { + val json = OptimizedJsonMapper.toJson(ListTransactionsMethod.create(limit = 20, offset = 0, unpaid = false)) + val params = assertNotNull(Json.parseToJsonElement(json).jsonObject["params"], "no params in $json").jsonObject + + assertEquals(setOf("limit", "offset", "unpaid"), params.keys, "unexpected keys in $json") + } + + /** + * The invariant the bug broke: one wire format, two backends, same document. + * This is the actual fix — the mixin is only the mechanism that restores it. + */ + @Test + fun bothBackendsProduceTheSameDocument() { + requests.forEach { (name, request) -> + val viaJackson = Json.parseToJsonElement(OptimizedJsonMapper.toJson(request)).jsonObject + val viaKotlinx = Json.parseToJsonElement(Json.encodeToString(Nip47RequestKSerializer, request)).jsonObject + + assertEquals(viaKotlinx, viaJackson, "$name differs between backends") + } + } +} diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/RequestTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/RequestTest.kt index 49d3f8d5fc..5af87858bf 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/RequestTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/RequestTest.kt @@ -285,6 +285,9 @@ class RequestTest { assertEquals(1000L, request.params?.from) assertEquals(2000L, request.params?.until) assertEquals(10, request.params?.limit) + // Omitted is how an absent optional param arrives, and it must read as null + // rather than as a default — this is the shape we now send. + assertNull(request.params?.offset) } // --- GetBalance --- diff --git a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/JacksonMapper.kt b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/JacksonMapper.kt index d926ad77a4..0c4365031e 100644 --- a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/JacksonMapper.kt +++ b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/JacksonMapper.kt @@ -59,9 +59,22 @@ import com.vitorpamplona.quartz.nip47WalletConnect.jackson.RequestDeserializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.RequestSerializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.ResponseDeserializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.ResponseSerializer +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.CancelHoldInvoiceParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.CreateConnectionParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.ListTransactionsParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.LookupInvoiceParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.MakeHoldInvoiceParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.MakeInvoiceParams import com.vitorpamplona.quartz.nip47WalletConnect.rpc.Notification +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayInvoiceParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayKeysendParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.ReceiveParams import com.vitorpamplona.quartz.nip47WalletConnect.rpc.Request import com.vitorpamplona.quartz.nip47WalletConnect.rpc.Response +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.SettleHoldInvoiceParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.SignMessageParams +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.TlvRecord import com.vitorpamplona.quartz.nip59Giftwrap.rumors.Rumor import com.vitorpamplona.quartz.nip59Giftwrap.rumors.jackson.RumorDeserializer import com.vitorpamplona.quartz.nip59Giftwrap.rumors.jackson.RumorSerializer @@ -108,6 +121,24 @@ class JacksonMapper { .addDeserializer(Request::class.java, RequestDeserializer()) .addSerializer(Notification::class.java, NotificationSerializer()) .addDeserializer(Notification::class.java, NotificationDeserializer()) + // NIP-47's optional params are OMITTED when null — see OmitNullsMixin. + // Matches what the kotlinx backend has always done. + .setMixInAnnotation(PayInvoiceParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(PayParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(ReceiveParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(PayKeysendParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(MakeInvoiceParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(LookupInvoiceParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(ListTransactionsParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(MakeHoldInvoiceParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(CancelHoldInvoiceParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(SettleHoldInvoiceParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(SignMessageParams::class.java, OmitNullsMixin::class.java) + .setMixInAnnotation(CreateConnectionParams::class.java, OmitNullsMixin::class.java) + // NESTED, and the only params field that is not a primitive or an + // already-registered type: a TlvRecord inside pay_keysend's + // `tlv_records` has two independently optional fields of its own. + .setMixInAnnotation(TlvRecord::class.java, OmitNullsMixin::class.java) // nip 46 .addDeserializer(BunkerMessage::class.java, BunkerMessageDeserializer()) .addSerializer(BunkerRequest::class.java, BunkerRequestSerializer()) diff --git a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/OmitNullsMixin.kt b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/OmitNullsMixin.kt new file mode 100644 index 0000000000..17f4b976aa --- /dev/null +++ b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/OmitNullsMixin.kt @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.nip01Core.jackson + +import com.fasterxml.jackson.annotation.JsonInclude + +/** + * Applied to a reflectively-serialized DTO so Jackson OMITS a null field instead + * of writing it. + * + * Optional protocol fields are absent, not null. A peer is free to type one + * strictly: sending `"from": null` for an absent `from` earned + * `Invalid list_transactions params: from must be an integer` from a NIP-47 + * wallet, and the request failed. + * + * A MIXIN rather than an annotation on the class, because these DTOs live in + * `commonMain` and Jackson annotations are JVM-only. Class-level rather than the + * mapper-wide `setSerializationInclusion`, which in Jackson 2.x also suppresses + * null MAP ENTRIES — and [com.vitorpamplona.quartz.nip01Core.kotlinSerialization.anyToJsonElement] + * deliberately keeps those, so a global setting would close one backend + * divergence by opening another. + */ +@JsonInclude(JsonInclude.Include.NON_NULL) +abstract class OmitNullsMixin