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

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

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<Pair<String, Request>> =
         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<Request>(json)
-
-        assertIs<ListTransactionsMethod>(back)
-        assertEquals(20, back.params?.limit)
-        assertEquals(0, back.params?.offset)
-        assertEquals(false, back.params?.unpaid)
-        assertEquals(null, back.params?.from)
-    }
 }

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 ---

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())

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