mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-05 19:28:25 +00:00
fix: address PR review — narrow search-relay seam, Local providers on Android, bug + test fixes
Review findings from @davotoula:
1. Behaviour drift (real): the per-note event finder reused Account.searchRelays()
(trusted + own search list), widening every missing-event REQ to trusted relays.
Add a narrow UserFinderAccount.searchOnlyRelays() (= the pre-extraction
account.searchRelayList read) and use it in FilterMissingEvents. Implemented on
Account and DesktopIAccount.
2. Runtime trap: LocalUserFinder/LocalUserFinderAccount/LocalEventFinder error() when
unprovided and were only provided on Desktop. Provide them on Android too, at the
logged-in root (AppNavigation) from accountViewModel.dataSources() + .account, so any
shared composable using the no-arg observeUser*/EventFinderFilterAssemblerSubscription
overloads is safe on Android (the :napplet process never renders these).
3. Removed the redundant `.ifEmpty { DefaultSearchRelayList }` in Account.searchRelays()
(SearchRelayListState.flow already applies that fallback) + its now-unused import.
4. Fixed a pre-existing shadowing bug on lines this PR touches: FilterByEvent's
`note.replyTo?.forEach { parentNote -> }` used `note` in the body, so parent notes were
never fetched — now uses `parentNote`.
5. Added a test at the layer where #1 lived: filterMissingEvents(cache, keys) fans a
missing event to searchOnlyRelays + the follow/mine/search default, NOT the trusted
relays that searchRelays would add.
6. Replaced an inline fully-qualified name in the test with an import (CLAUDE.md style).
Green: commons jvmTest + verifyKmpPurity, :amethyst compilePlayDebugKotlin, :desktopApp compile, spotless.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
fbe163e8f2
commit
eaba40f2a6
@@ -32,7 +32,6 @@ import com.vitorpamplona.amethyst.commons.connectedApps.signers.NostrSignerPermi
|
||||
import com.vitorpamplona.amethyst.commons.connectedApps.signers.NostrSignerPermissionStore
|
||||
import com.vitorpamplona.amethyst.commons.defaults.Constants
|
||||
import com.vitorpamplona.amethyst.commons.defaults.DefaultIndexerRelayList
|
||||
import com.vitorpamplona.amethyst.commons.defaults.DefaultSearchRelayList
|
||||
import com.vitorpamplona.amethyst.commons.marmot.MarmotManager
|
||||
import com.vitorpamplona.amethyst.commons.model.IAccount
|
||||
import com.vitorpamplona.amethyst.commons.model.buzz.BuzzRelayDialect
|
||||
@@ -382,7 +381,11 @@ class Account(
|
||||
|
||||
override fun outboxHomeRelays(): Set<NormalizedRelayUrl> = nip65RelayList.allFlowNoDefaults.value + privateStorageRelayList.flow.value + localRelayList.flow.value
|
||||
|
||||
override fun searchRelays(): Set<NormalizedRelayUrl> = (trustedRelayList.flow.value + searchRelayList.flow.value.ifEmpty { DefaultSearchRelayList }).toSet()
|
||||
// searchRelayList.flow already applies the DefaultSearchRelayList fallback internally
|
||||
// (SearchRelayListState.normalizeSearchRelayListWithBackup), so no ifEmpty needed here.
|
||||
override fun searchRelays(): Set<NormalizedRelayUrl> = (trustedRelayList.flow.value + searchRelayList.flow.value).toSet()
|
||||
|
||||
override fun searchOnlyRelays(): Set<NormalizedRelayUrl> = searchRelayList.flow.value
|
||||
|
||||
override fun followPlusAllMineWithSearchRelays(): Set<NormalizedRelayUrl> = followPlusAllMineWithSearch.flow.value
|
||||
|
||||
|
||||
+3
-3
@@ -45,9 +45,9 @@ fun filterByEvent(
|
||||
|
||||
// loads threading that is event-based
|
||||
note.replyTo?.forEach { parentNote ->
|
||||
if (parentNote !is AddressableNote && note.event == null) {
|
||||
potentialRelaysToFindEvent(LocalCache, note).ifEmpty { default }.forEach { relayUrl ->
|
||||
add(relayUrl, note.idHex)
|
||||
if (parentNote !is AddressableNote && parentNote.event == null) {
|
||||
potentialRelaysToFindEvent(LocalCache, parentNote).ifEmpty { default }.forEach { relayUrl ->
|
||||
add(relayUrl, parentNote.idHex)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,6 +50,9 @@ import androidx.navigation.compose.composable
|
||||
import com.vitorpamplona.amethyst.Amethyst
|
||||
import com.vitorpamplona.amethyst.R
|
||||
import com.vitorpamplona.amethyst.commons.nipACWebRtcCalls.CallState
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.event.LocalEventFinder
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.user.LocalUserFinder
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.user.LocalUserFinderAccount
|
||||
import com.vitorpamplona.amethyst.service.crashreports.DisplayCrashMessages
|
||||
import com.vitorpamplona.amethyst.service.relayClient.notifyCommand.compose.DisplayNotifyMessages
|
||||
import com.vitorpamplona.amethyst.service.resourceusage.DisplayResourceUsageAlert
|
||||
@@ -341,6 +344,15 @@ fun AppNavigation(
|
||||
CompositionLocalProvider(
|
||||
LocalScreenLayout provides screenLayout,
|
||||
LocalTabReselectCoordinator provides tabReselectCoordinator,
|
||||
// Provide the shared finder CompositionLocals so any commons composable that
|
||||
// uses the no-arg observeUser*/EventFinderFilterAssemblerSubscription(note)
|
||||
// overloads works when rendered on Android (they error() if unprovided). Android's
|
||||
// own UI uses the AccountViewModel overloads and doesn't strictly need these, but
|
||||
// providing them removes the runtime trap for shared composables reaching the
|
||||
// logged-in tree. (The :napplet process never renders these composables.)
|
||||
LocalUserFinder provides accountViewModel.dataSources().userFinder,
|
||||
LocalUserFinderAccount provides accountViewModel.account,
|
||||
LocalEventFinder provides accountViewModel.dataSources().eventFinder,
|
||||
) {
|
||||
AccountSwitcherAndLeftDrawerLayout(accountViewModel, accountSessionManager, nav) {
|
||||
Box(Modifier.fillMaxSize()) {
|
||||
|
||||
+1
-1
@@ -115,7 +115,7 @@ fun filterMissingEvents(
|
||||
add(relayUrl, key.note.idHex)
|
||||
}
|
||||
|
||||
key.account.searchRelays().forEach { relayUrl ->
|
||||
key.account.searchOnlyRelays().forEach { relayUrl ->
|
||||
add(relayUrl, key.note.idHex)
|
||||
}
|
||||
}
|
||||
|
||||
+10
-1
@@ -56,9 +56,18 @@ interface UserFinderAccount {
|
||||
/** Home/write relays used for outbox discovery (nip65 + private storage + local). */
|
||||
fun outboxHomeRelays(): Set<NormalizedRelayUrl>
|
||||
|
||||
/** Search relays (trusted + search), with the default fallback applied. */
|
||||
/** Search relays (trusted + own search list), for the user-finder's search-tier fallback. */
|
||||
fun searchRelays(): Set<NormalizedRelayUrl>
|
||||
|
||||
/**
|
||||
* Just this account's own NIP-51 search relay list — WITHOUT the trusted-relay
|
||||
* union that [searchRelays] adds. This is the narrow set the per-note event
|
||||
* finder fans "missing event" REQs to, matching the pre-extraction
|
||||
* `account.searchRelayList` read (reusing [searchRelays] there would have
|
||||
* unintentionally widened the fan-out to trusted relays).
|
||||
*/
|
||||
fun searchOnlyRelays(): Set<NormalizedRelayUrl>
|
||||
|
||||
/**
|
||||
* Follow + all-mine + search relays, used by the per-note event-finder to
|
||||
* place "missing event" / "missing addressable" REQs (reactions, zaps,
|
||||
|
||||
+67
-1
@@ -20,18 +20,22 @@
|
||||
*/
|
||||
package com.vitorpamplona.amethyst.commons.relayClient.event
|
||||
|
||||
import com.vitorpamplona.amethyst.commons.model.AddressableNote
|
||||
import com.vitorpamplona.amethyst.commons.model.Channel
|
||||
import com.vitorpamplona.amethyst.commons.model.Note
|
||||
import com.vitorpamplona.amethyst.commons.model.User
|
||||
import com.vitorpamplona.amethyst.commons.model.cache.ICacheEventStream
|
||||
import com.vitorpamplona.amethyst.commons.model.cache.ICacheProvider
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.event.loaders.filterMissingEvents
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.user.UserFinderAccount
|
||||
import com.vitorpamplona.quartz.nip01Core.core.Address
|
||||
import com.vitorpamplona.quartz.nip01Core.core.Event
|
||||
import com.vitorpamplona.quartz.nip01Core.core.HexKey
|
||||
import com.vitorpamplona.quartz.nip01Core.hints.HintIndexer
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
|
||||
import com.vitorpamplona.quartz.nip85TrustedAssertions.list.tags.ServiceProviderTag
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
@@ -73,6 +77,43 @@ class EventFinderFilterAssemblyTest {
|
||||
assertTrue(filterMissingEvents(mapOf(relay1 to emptySet())).isEmpty())
|
||||
}
|
||||
|
||||
/**
|
||||
* The per-note event finder fans a missing event out to the account's own
|
||||
* search relays ([UserFinderAccount.searchOnlyRelays]) plus the
|
||||
* follow/mine/search default — NOT the trusted-relay union that
|
||||
* [UserFinderAccount.searchRelays] adds. Regression guard for reusing the
|
||||
* wrong getter here (which would silently widen every missing-event REQ to
|
||||
* trusted relays).
|
||||
*/
|
||||
@Test
|
||||
fun `filterMissingEvents(keys) uses searchOnlyRelays, not the trusted searchRelays union`() {
|
||||
val searchOnly = NormalizedRelayUrl("wss://search.test/")
|
||||
val trustedExtra = NormalizedRelayUrl("wss://trusted.test/") // only in searchRelays()
|
||||
val default = NormalizedRelayUrl("wss://default.test/") // followPlusAllMineWithSearchRelays()
|
||||
|
||||
val account =
|
||||
object : StubAccount() {
|
||||
override fun searchOnlyRelays() = setOf(searchOnly)
|
||||
|
||||
override fun searchRelays() = setOf(searchOnly, trustedExtra)
|
||||
|
||||
override fun followPlusAllMineWithSearchRelays() = setOf(default)
|
||||
}
|
||||
|
||||
// event == null and not addressable → a "missing event"; no author/replies,
|
||||
// and the stub cache has empty relay hints, so potentialRelaysToFindEvent is
|
||||
// empty and the code falls back to the default relays + searchOnlyRelays.
|
||||
val note = Note("a".repeat(64))
|
||||
|
||||
val filters = filterMissingEvents(StubCache(), listOf(EventFinderQueryState(note, account)))
|
||||
val relays = filters.map { it.relay }.toSet()
|
||||
|
||||
assertTrue("search-only relay carries the missing-event REQ", searchOnly in relays)
|
||||
assertTrue("follow/mine/search default carries it", default in relays)
|
||||
assertFalse("trusted relay (only in searchRelays) must NOT be fanned out to", trustedExtra in relays)
|
||||
filters.forEach { assertEquals(listOf(note.idHex), it.filter.ids) }
|
||||
}
|
||||
|
||||
/**
|
||||
* The new default [ICacheProvider.checkGetOrCreateUser] must swallow a
|
||||
* malformed-key throw and return null (the event-finder follows pubkey hints
|
||||
@@ -106,7 +147,7 @@ class EventFinderFilterAssemblyTest {
|
||||
|
||||
override fun checkGetOrCreateNote(hexKey: HexKey): Note? = null
|
||||
|
||||
override fun getOrCreateAddressableNote(key: Address): com.vitorpamplona.amethyst.commons.model.AddressableNote = error("unused")
|
||||
override fun getOrCreateAddressableNote(key: Address): AddressableNote = error("unused")
|
||||
|
||||
override fun getEventStream(): ICacheEventStream = error("unused")
|
||||
|
||||
@@ -116,4 +157,29 @@ class EventFinderFilterAssemblyTest {
|
||||
|
||||
override fun justConsumeMyOwnEvent(event: Event): Boolean = false
|
||||
}
|
||||
|
||||
/** Minimal [UserFinderAccount] returning nothing; override the relevant getters per test. */
|
||||
private open class StubAccount : UserFinderAccount {
|
||||
override val userFinderPubkeyHex: HexKey = "00".repeat(32)
|
||||
|
||||
override fun indexRelays(): Set<NormalizedRelayUrl> = emptySet()
|
||||
|
||||
override fun outboxHomeRelays(): Set<NormalizedRelayUrl> = emptySet()
|
||||
|
||||
override fun searchRelays(): Set<NormalizedRelayUrl> = emptySet()
|
||||
|
||||
override fun searchOnlyRelays(): Set<NormalizedRelayUrl> = emptySet()
|
||||
|
||||
override fun followPlusAllMineWithSearchRelays(): Set<NormalizedRelayUrl> = emptySet()
|
||||
|
||||
override fun commonRelays(): Set<NormalizedRelayUrl> = emptySet()
|
||||
|
||||
override fun cardHomeRelays(): Set<NormalizedRelayUrl> = emptySet()
|
||||
|
||||
override fun trustProvider(): ServiceProviderTag? = null
|
||||
|
||||
override fun followerCountProvider(): ServiceProviderTag? = null
|
||||
|
||||
override fun declaredFollowsByOutboxRelay(): Map<NormalizedRelayUrl, Set<HexKey>> = emptyMap()
|
||||
}
|
||||
}
|
||||
|
||||
+3
@@ -114,6 +114,9 @@ class DesktopIAccount(
|
||||
|
||||
override fun searchRelays(): Set<NormalizedRelayUrl> = relayManager.connectedRelays.value
|
||||
|
||||
// Desktop has no separate NIP-51 search relay list; degrade to connected relays.
|
||||
override fun searchOnlyRelays(): Set<NormalizedRelayUrl> = relayManager.connectedRelays.value
|
||||
|
||||
// Desktop has no merged follow/mine/search relay-list subsystem; route
|
||||
// missing-event discovery through the connected relays (same degrade path
|
||||
// as the other hints above).
|
||||
|
||||
Reference in New Issue
Block a user