From ad3304a6653a3259301aef2e3ad4d473fecf2079 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sat, 5 Sep 2026 23:22:06 +0200 Subject: [PATCH] refactor: build the DM inbox filter once, where it can be asserted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The filter fix a commit ago changed a value inline in a ViewModel, which is not a place a test can reach: ChatMessageListViewModel needs a repository and a coroutine scope to construct, and NostrDao needs Room. So the filter that had just been wrong in three call sites went back to having no coverage at all. Nip17Filters.inbox is that filter with one definition. ChatMessageListViewModel and ChatRoomListViewModel now both call it — they had been building it separately and identically, which is also what made their negentropy requests collapse into one under computeId, a coincidence better expressed as shared code than left to hold by luck. Nip17FiltersTest asserts every clause that was got wrong in production: - the p tag names us, not a peer - there is no authors clause, because a wrap is signed by the throwaway key GiftWrapEvent.create mints and discards, so authors=[anything knowable] matches nothing on any relay - there is no since cursor, because NIP-59 back-dates a wrap by up to two days and a high-water mark taken from the newest wrap we hold skips mail stamped behind it — the trap waiting for whoever acts on the TODO in NegentropySynchronizeRequest.toSynchronizeNostrEventRequest - the wire JSON is pinned, so an added default cannot quietly split the two callers back into separate requests - the SQL NostrEventFilterQuery builds from it bounds no author either, since negentropy is only as good as the agreement between the set we build locally and the set the relay builds from the same filter Neither of the two failure modes this covers was visible from reading the filter. The authors clause failed silently for as long as it existed, and the peer p-tag failed loudly but somewhere else entirely — in a Room transaction, three files away, as a MAC error out of Nip44. Co-Authored-By: Claude Opus 5 --- .../mantra/compose/nostr/Nip17Filters.kt | 37 +++++++++ .../ui/view/model/ChatMessageListViewModel.kt | 21 ++--- .../ui/view/model/ChatRoomListViewModel.kt | 12 +-- .../mantra/compose/nostr/Nip17FiltersTest.kt | 80 +++++++++++++++++++ 4 files changed, 125 insertions(+), 25 deletions(-) create mode 100644 composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/Nip17Filters.kt create mode 100644 composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/Nip17FiltersTest.kt diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/Nip17Filters.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/Nip17Filters.kt new file mode 100644 index 00000000..a1069e6b --- /dev/null +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/nostr/Nip17Filters.kt @@ -0,0 +1,37 @@ +package press.mantra.compose.nostr + +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent +import press.mantra.compose.database.model.types.SynchronizationFilter + +/** + * The one filter shape that can return a NIP-17 message we are able to read. + * + * A gift wrap hides everything except who it is for. The author is the throwaway + * key [GiftWrapEvent.create] mints and discards, the content is sealed to the + * recipient, and `created_at` is randomised up to two days into the past. That + * leaves the `p` tag as the only clause worth writing, and it has to name us: + * naming a peer subscribes to mail no key of ours can open, and adding `authors` + * matches nothing on any relay, ever. Both mistakes were live in three separate + * call sites, so the filter is built in one place now and asserted in one place. + */ +object Nip17Filters { + + /** + * Everything gift-wrapped to [publicKey], capped at [limit] events. + * + * Deliberately carries no `since`. NIP-59 back-dates a wrap by up to two days, + * so a cursor built from the newest wrap we hold silently skips mail that was + * sent later but stamped earlier. + */ + fun inbox( + publicKey: HexKey, + limit: Int = DEFAULT_LIMIT, + ) = SynchronizationFilter( + kinds = arrayOf(GiftWrapEvent.KIND), + tags = mapOf("p" to listOf(publicKey)), + limit = limit, + ) + + const val DEFAULT_LIMIT = 50 +} diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt index 1a5d0357..50334f6a 100755 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatMessageListViewModel.kt @@ -66,6 +66,7 @@ import press.mantra.compose.database.model.intermdiate.LocalChatRoom import press.mantra.compose.database.model.types.SynchronizationFilter import press.mantra.compose.extensions.shortened import press.mantra.compose.extensions.toFormattedTimeAndDateString +import press.mantra.compose.nostr.Nip17Filters import press.mantra.compose.nostr.Relays import press.mantra.compose.repository.ChatRepository import press.mantra.compose.repository.NostrRepository @@ -74,7 +75,6 @@ import press.mantra.compose.ui.view.state.ChatMessageListUIState import co.touchlab.kermit.Logger import com.vitorpamplona.quartz.marmot.mip03GroupMessages.GroupEvent import com.vitorpamplona.quartz.nip17Dm.settings.ChatMessageRelayListEvent -import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.IO import kotlinx.coroutines.flow.distinctUntilChanged @@ -123,22 +123,13 @@ class ChatMessageListViewModel( val chatMessageRelayListEvent = chatRepository.getChatMessageRelayForPublicKey(recipients.participant.participantPublicKey) val relayAndSynchronizationFilter = if (chatMessageRelayListEvent != null) { - // Refresh our own inbox. A wrap names only its recipient, so - // "the messages in this conversation" is not something a filter - // can ask for, and the recipient's relays hold their mail, not - // ours. p-tagging the peer here fetched other people's wraps, - // which no key of ours can open. + // Opening a conversation refreshes our own inbox: this used to + // p-tag the peer and read their relays, which is where their mail + // is kept, not ours. See Nip17Filters for why a per-conversation + // filter is not a thing that can be written. Pair( Relays.DefaultDMRelayList, - SynchronizationFilter( - kinds = arrayOf( - GiftWrapEvent.KIND, - ), - tags = mapOf( - Pair("p", listOf(localChatRoom.chatRoom.userPublicKey)) - ), - limit = 50 - ) + Nip17Filters.inbox(localChatRoom.chatRoom.userPublicKey), ) } else { diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt index 994f76bb..4a709874 100755 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt @@ -27,13 +27,13 @@ import androidx.lifecycle.viewmodel.initializer import androidx.lifecycle.viewmodel.viewModelFactory import press.mantra.compose.database.model.NegentropySynchronizeRequest import press.mantra.compose.database.model.types.SynchronizationFilter +import press.mantra.compose.nostr.Nip17Filters import press.mantra.compose.nostr.Relays import press.mantra.compose.repository.ChatRepository import press.mantra.compose.repository.NostrRepository import press.mantra.compose.ui.view.state.ChatRoomListUIState import co.touchlab.kermit.Logger import com.vitorpamplona.quartz.marmot.mip03GroupMessages.GroupEvent -import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.IO import kotlinx.coroutines.launch @@ -72,15 +72,7 @@ class ChatRoomListViewModel( logger.d("scheduleSynchronization") viewModelScope.launch(Dispatchers.IO) { // Sync Notifications... might want to also run this in the background - val chatRequestFilter = SynchronizationFilter( - kinds = arrayOf( - GiftWrapEvent.KIND, - ), - tags = mapOf( - Pair("p", listOf(publicKey)) - ), - limit = 50 - ) + val chatRequestFilter = Nip17Filters.inbox(publicKey) nostrRepository.queueNegentropySynchronizeRequest( Relays.DefaultDMRelayList.shuffled().map { normalizedRelayUrl -> NegentropySynchronizeRequest( diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/Nip17FiltersTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/Nip17FiltersTest.kt new file mode 100644 index 00000000..8ecf0c8f --- /dev/null +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/nostr/Nip17FiltersTest.kt @@ -0,0 +1,80 @@ +package press.mantra.compose.nostr + +import press.mantra.compose.database.query.NostrEventFilterQuery +import press.mantra.compose.network.serialization.encodeToJsonString +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * Pins the only gift wrap filter that can come back with something we can read. + * + * Every clause here is one that was got wrong in production. Two call sites asked + * for `authors = [our pubkey]`, which cannot match a wrap signed by a throwaway + * key and so returned nothing at all, silently, for as long as it existed. A third + * asked for `p = [the peer]`, which returned other people's mail and crashed the + * save that tried to unseal it. Neither failure was visible from reading the + * filter, so the shape is asserted instead of trusted. + */ +class Nip17FiltersTest { + + private val us = "a".repeat(64) + + @Test + fun `it asks for wraps addressed to us`() { + assertEquals(mapOf("p" to listOf(us)), Nip17Filters.inbox(us).tags) + } + + @Test + fun `it constrains no authors`() { + // GiftWrapEvent.create signs with a key it generates and drops, so the author + // of a wrap is a value nobody can predict -- least of all the sender's own + // pubkey. Any authors clause here silently matches zero events on every relay. + assertNull(Nip17Filters.inbox(us).authors) + } + + @Test + fun `it carries no since cursor`() { + // A wrap is stamped up to two days earlier than it was sent, so a high-water + // mark taken from the newest wrap we hold skips mail that arrives behind it. + // Anything reintroducing `since` has to back-date by at least two days first. + assertNull(Nip17Filters.inbox(us).since) + assertNull(Nip17Filters.inbox(us).until) + } + + @Test + fun `it asks for gift wraps and nothing else`() { + assertEquals(listOf(1059), Nip17Filters.inbox(us).kinds?.toList()) + } + + @Test + fun `two callers asking for the same inbox make one request`() { + // computeId hashes the encoded filter, so the chat room list and the chat + // message screen collapse into a single negentropy request only while both + // encode identically. Building the filter once is what holds that true; the + // wire shape is asserted so an added default cannot quietly split them. + assertEquals(Nip17Filters.inbox(us), Nip17Filters.inbox(us)) + assertEquals( + """{"kinds":[1059],"tags":{"p":["$us"]},"limit":50}""", + Nip17Filters.inbox(us).encodeToJsonString(), + ) + } + + @Test + fun `the local set it builds is the same set the relay is asked for`() { + // Negentropy reconciles our local set against the relay's: this filter goes out + // in NEG-OPEN, and the local side is built by running the same filter through + // NostrEventFilterQuery. A clause that survives one trip and not the other + // reports differences that are not real -- events re-downloaded forever, or + // pushed at a relay that excluded them on purpose. What matters here is that + // the local query reads the p tag and, like the wire filter, bounds no author: + // an authors clause would show up as `pubKey IN (?)`. + val query = NostrEventFilterQuery.build(Nip17Filters.inbox(us)) + + assertEquals( + "SELECT * FROM NostrEvent WHERE kind IN (?) AND (tags LIKE ? ESCAPE '\\') " + + "ORDER BY createdAt DESC, id DESC", + query.sql, + ) + } +}