mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 03:38:23 +00:00
Merge pull request #4022 from davotoula/fix/nwc-omit-null-request-params
Fix/nwc omit null request params
This commit is contained in:
Vitor Pamplona
+7
-3
@@ -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
|
||||
|
||||
+144
@@ -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<Pair<String, Request>> =
|
||||
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")
|
||||
}
|
||||
}
|
||||
}
|
||||
+3
@@ -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 ---
|
||||
|
||||
+31
@@ -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())
|
||||
|
||||
+42
@@ -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
|
||||
Reference in New Issue
Block a user