From 0030432e20e2ecd1a532651397e150fb756b2c7f Mon Sep 17 00:00:00 2001 From: davotoula Date: Mon, 31 Aug 2026 07:35:44 +0200 Subject: [PATCH 1/2] fix(nwc): omit absent request params instead of sending them as null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Viewing transactions on one NWC wallet failed with Invalid list_transactions params: from must be an integer because Amethyst sent every optional parameter explicitly: {"method":"list_transactions","params":{"from":null,"until":null,"limit":20, "offset":0,"unpaid":false,"unpaid_outgoing":null,"unpaid_incoming":null,"type":null}} NIP-47 marks those optional, and a wallet is free to type `from` as an integer and refuse a null. Nothing in the request was wrong except the nulls. The two serialization backends had disagreed since they were written. Nip47RequestKSerializer builds every params object with `params.x?.let { put("x", it) }`, so kotlinx has always omitted nulls; Jackson serializes the params classes reflectively and wrote them. The same request was two different documents depending on the platform, and only JVM/Android was broken — which is why it survived: the tests that cover this shape run against the backend that was already correct. A Jackson mixin now applies NON_NULL to all twelve NIP-47 params classes. A mixin rather than an annotation because the classes live in commonMain and Jackson annotations are JVM-only. The regression test asserts the property rather than the symptom: no request type may emit a null param, and both backends must produce the same document. The second is the one that would have caught this. Not new to any recent change — the reflective serialization predates it. What changed is that 24a8540ad9 surfaces a NIP-47 refusal instead of rendering it as an empty list, so users now see the error rather than an empty transaction screen. Older builds sent the same request and were refused just as silently. --- .../quartz/nip01Core/jackson/JacksonMapper.kt | 28 +++++ .../jackson/OmitNullParamsMixin.kt | 45 +++++++ .../Nip47NullParamOmissionTest.kt | 115 ++++++++++++++++++ 3 files changed, 188 insertions(+) create mode 100644 quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/jackson/OmitNullParamsMixin.kt create mode 100644 quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt 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..37daf0aa6d 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 @@ -55,13 +55,26 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.jackson.BunkerResponseDeserial import com.vitorpamplona.quartz.nip46RemoteSigner.jackson.BunkerResponseSerializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.NotificationDeserializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.NotificationSerializer +import com.vitorpamplona.quartz.nip47WalletConnect.jackson.OmitNullParamsMixin 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.nip59Giftwrap.rumors.Rumor import com.vitorpamplona.quartz.nip59Giftwrap.rumors.jackson.RumorDeserializer import com.vitorpamplona.quartz.nip59Giftwrap.rumors.jackson.RumorSerializer @@ -108,6 +121,21 @@ 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, never written as + // `null` — see OmitNullParamsMixin. Matches what the kotlinx backend + // has always done, and what a strictly-typed wallet accepts. + .setMixInAnnotation(PayInvoiceParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(PayParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(ReceiveParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(PayKeysendParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(MakeInvoiceParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(LookupInvoiceParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(ListTransactionsParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(MakeHoldInvoiceParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(CancelHoldInvoiceParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(SettleHoldInvoiceParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(SignMessageParams::class.java, OmitNullParamsMixin::class.java) + .setMixInAnnotation(CreateConnectionParams::class.java, OmitNullParamsMixin::class.java) // nip 46 .addDeserializer(BunkerMessage::class.java, BunkerMessageDeserializer()) .addSerializer(BunkerRequest::class.java, BunkerRequestSerializer()) diff --git a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/jackson/OmitNullParamsMixin.kt b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/jackson/OmitNullParamsMixin.kt new file mode 100644 index 0000000000..786835d3c5 --- /dev/null +++ b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/jackson/OmitNullParamsMixin.kt @@ -0,0 +1,45 @@ +/* + * 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.jackson + +import com.fasterxml.jackson.annotation.JsonInclude + +/** + * Applied to every NIP-47 request `params` class so Jackson OMITS a null field + * instead of writing it. + * + * NIP-47 marks these parameters optional, and a wallet is free to type one + * strictly: sending `"from": null` for an absent `from` earns + * `Invalid list_transactions params: from must be an integer` from a wallet that + * expects an integer or nothing, and the request fails. + * + * THE TWO BACKENDS DISAGREED, which is the reason this is a mixin rather than a + * fix at one call site. [com.vitorpamplona.quartz.nip47WalletConnect.kotlinSerialization.Nip47RequestKSerializer] + * builds every params object with `params.x?.let { put("x", it) }`, so the + * kotlinx (native) backend has always omitted nulls; Jackson serializes the + * params classes reflectively and wrote them. The same request was two different + * documents depending on the platform. + * + * A MIXIN rather than an annotation on the class, because the params classes live + * in `commonMain` and Jackson annotations are JVM-only. + */ +@JsonInclude(JsonInclude.Include.NON_NULL) +abstract class OmitNullParamsMixin diff --git a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt new file mode 100644 index 0000000000..70e6c273ae --- /dev/null +++ b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt @@ -0,0 +1,115 @@ +/* + * 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.CreateConnectionMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.GetBalanceMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.ListTransactionsMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.LookupInvoiceMethod +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.Request +import kotlinx.serialization.json.Json +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.assertIs +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. + * + * The two serialization backends had disagreed since they were written: kotlinx + * omits nulls (`params.x?.let { put(...) }`), Jackson wrote them reflectively. The + * same request was two different documents depending on the platform, and only the + * JVM/Android one was broken. + */ +class Nip47NullParamOmissionTest { + private val requests: List> = + listOf( + // The reported failure: every field but three is absent. + "list_transactions" to ListTransactionsMethod.create(limit = 20, offset = 0, unpaid = false), + "pay_invoice" to PayInvoiceMethod.create("lnbc50n1abc"), + "make_invoice" to MakeInvoiceMethod.create(amount = 21000L), + "lookup_invoice" to LookupInvoiceMethod.createByHash("31afdf1"), + "pay_keysend" to PayKeysendMethod.create(amount = 21000L, pubkey = "0266e4"), + "create_connection" to CreateConnectionMethod.create(pubkey = "abc123", name = "app"), + "get_balance" to GetBalanceMethod.create(), + ) + + @Test + fun noRequestEverSendsANullParam() { + requests.forEach { (name, request) -> + val json = OptimizedJsonMapper.toJson(request) + val params = Json.parseToJsonElement(json).jsonObject["params"] as? JsonObject + + params?.forEach { (key, value) -> + assertTrue(value !is JsonNull, "$name sent \"$key\": null - omit it instead. Full: $json") + } + } + } + + /** The exact document the failing wallet rejected, now minimal. */ + @Test + fun listTransactionsCarriesOnlyWhatWasAsked() { + val json = OptimizedJsonMapper.toJson(ListTransactionsMethod.create(limit = 20, offset = 0, unpaid = false)) + + assertEquals( + """{"method":"list_transactions","params":{"limit":20,"offset":0,"unpaid":false}}""", + json, + ) + } + + /** + * The invariant the bug broke: both backends must produce the same document. + * Compared as parsed objects, since key ORDER is each backend's own business. + */ + @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") + } + } + + /** Omitting a field must not change how the request reads back. */ + @Test + fun anOmittedParamStillParsesBack() { + val json = OptimizedJsonMapper.toJson(ListTransactionsMethod.create(limit = 20, offset = 0, unpaid = false)) + val back = OptimizedJsonMapper.fromJsonTo(json) + + assertIs(back) + assertEquals(20, back.params?.limit) + assertEquals(0, back.params?.offset) + assertEquals(false, back.params?.unpaid) + assertEquals(null, back.params?.from) + } +} From 34c60fada10e6d4b8a0e36c381e2cda78b4a574d Mon Sep 17 00:00:00 2001 From: davotoula Date: Mon, 31 Aug 2026 11:07:33 +0200 Subject: [PATCH 2/2] Code review: omit nulls one level down too + make the null-omission guard cover every method fix(nwc): omit nulls one level down too, inside pay_keysend's TLV records refactor(nwc): make the null-omission guard cover every method, on every target --- .../Nip47KotlinSerializationNullTest.kt | 10 +- .../Nip47NullParamOmissionTest.kt | 99 ++++++++++++------- .../quartz/nip47WalletConnect/RequestTest.kt | 3 + .../quartz/nip01Core/jackson/JacksonMapper.kt | 35 ++++--- .../jackson/OmitNullsMixin.kt} | 31 +++--- 5 files changed, 107 insertions(+), 71 deletions(-) rename quartz/src/{jvmTest => commonTest}/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt (53%) rename quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/{nip47WalletConnect/jackson/OmitNullParamsMixin.kt => nip01Core/jackson/OmitNullsMixin.kt} (55%) 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/jvmTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt similarity index 53% rename from quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt rename to quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt index 70e6c273ae..a773123437 100644 --- a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/Nip47NullParamOmissionTest.kt @@ -22,21 +22,29 @@ 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.GetBalanceMethod 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.assertIs +import kotlin.test.assertNotNull import kotlin.test.assertTrue /** @@ -45,22 +53,37 @@ import kotlin.test.assertTrue * `Invalid list_transactions params: from must be an integer` from a real wallet * and failed the request outright. * - * The two serialization backends had disagreed since they were written: kotlinx - * omits nulls (`params.x?.let { put(...) }`), Jackson wrote them reflectively. The - * same request was two different documents depending on the platform, and only the - * JVM/Android one was broken. + * 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: every field but three is absent. + // 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"), - "pay_keysend" to PayKeysendMethod.create(amount = 21000L, pubkey = "0266e4"), + "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"), - "get_balance" to GetBalanceMethod.create(), ) @Test @@ -69,26 +92,45 @@ class Nip47NullParamOmissionTest { val json = OptimizedJsonMapper.toJson(request) val params = Json.parseToJsonElement(json).jsonObject["params"] as? JsonObject - params?.forEach { (key, value) -> - assertTrue(value !is JsonNull, "$name sent \"$key\": null - omit it instead. Full: $json") - } + // 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) } } } - /** The exact document the failing wallet rejected, now minimal. */ - @Test - fun listTransactionsCarriesOnlyWhatWasAsked() { - val json = OptimizedJsonMapper.toJson(ListTransactionsMethod.create(limit = 20, offset = 0, unpaid = false)) + 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) + } - assertEquals( - """{"method":"list_transactions","params":{"limit":20,"offset":0,"unpaid":false}}""", - json, - ) + is JsonArray -> element.forEach { assertNoNulls(it, name, json) } + else -> Unit + } } /** - * The invariant the bug broke: both backends must produce the same document. - * Compared as parsed objects, since key ORDER is each backend's own business. + * 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() { @@ -99,17 +141,4 @@ class Nip47NullParamOmissionTest { assertEquals(viaKotlinx, viaJackson, "$name differs between backends") } } - - /** Omitting a field must not change how the request reads back. */ - @Test - fun anOmittedParamStillParsesBack() { - val json = OptimizedJsonMapper.toJson(ListTransactionsMethod.create(limit = 20, offset = 0, unpaid = false)) - val back = OptimizedJsonMapper.fromJsonTo(json) - - assertIs(back) - assertEquals(20, back.params?.limit) - assertEquals(0, back.params?.offset) - assertEquals(false, back.params?.unpaid) - assertEquals(null, back.params?.from) - } } 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 37daf0aa6d..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 @@ -55,7 +55,6 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.jackson.BunkerResponseDeserial import com.vitorpamplona.quartz.nip46RemoteSigner.jackson.BunkerResponseSerializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.NotificationDeserializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.NotificationSerializer -import com.vitorpamplona.quartz.nip47WalletConnect.jackson.OmitNullParamsMixin import com.vitorpamplona.quartz.nip47WalletConnect.jackson.RequestDeserializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.RequestSerializer import com.vitorpamplona.quartz.nip47WalletConnect.jackson.ResponseDeserializer @@ -75,6 +74,7 @@ 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 @@ -121,21 +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, never written as - // `null` — see OmitNullParamsMixin. Matches what the kotlinx backend - // has always done, and what a strictly-typed wallet accepts. - .setMixInAnnotation(PayInvoiceParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(PayParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(ReceiveParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(PayKeysendParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(MakeInvoiceParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(LookupInvoiceParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(ListTransactionsParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(MakeHoldInvoiceParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(CancelHoldInvoiceParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(SettleHoldInvoiceParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(SignMessageParams::class.java, OmitNullParamsMixin::class.java) - .setMixInAnnotation(CreateConnectionParams::class.java, OmitNullParamsMixin::class.java) + // 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/nip47WalletConnect/jackson/OmitNullParamsMixin.kt b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/OmitNullsMixin.kt similarity index 55% rename from quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/jackson/OmitNullParamsMixin.kt rename to quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/OmitNullsMixin.kt index 786835d3c5..17f4b976aa 100644 --- a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip47WalletConnect/jackson/OmitNullParamsMixin.kt +++ b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/nip01Core/jackson/OmitNullsMixin.kt @@ -18,28 +18,25 @@ * 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.jackson +package com.vitorpamplona.quartz.nip01Core.jackson import com.fasterxml.jackson.annotation.JsonInclude /** - * Applied to every NIP-47 request `params` class so Jackson OMITS a null field - * instead of writing it. + * Applied to a reflectively-serialized DTO so Jackson OMITS a null field instead + * of writing it. * - * NIP-47 marks these parameters optional, and a wallet is free to type one - * strictly: sending `"from": null` for an absent `from` earns - * `Invalid list_transactions params: from must be an integer` from a wallet that - * expects an integer or nothing, and the request fails. + * 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. * - * THE TWO BACKENDS DISAGREED, which is the reason this is a mixin rather than a - * fix at one call site. [com.vitorpamplona.quartz.nip47WalletConnect.kotlinSerialization.Nip47RequestKSerializer] - * builds every params object with `params.x?.let { put("x", it) }`, so the - * kotlinx (native) backend has always omitted nulls; Jackson serializes the - * params classes reflectively and wrote them. The same request was two different - * documents depending on the platform. - * - * A MIXIN rather than an annotation on the class, because the params classes live - * in `commonMain` and Jackson annotations are JVM-only. + * 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 OmitNullParamsMixin +abstract class OmitNullsMixin