From 34c60fada10e6d4b8a0e36c381e2cda78b4a574d Mon Sep 17 00:00:00 2001 From: davotoula Date: Mon, 31 Aug 2026 11:07:33 +0200 Subject: [PATCH] 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