-
Notifications
You must be signed in to change notification settings - Fork 6
fix: event source tracking #470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import '../entities/relay_connectivity.dart'; | |
| import '../entities/relay_info.dart'; | ||
| import '../entities/request_state.dart'; | ||
| import '../entities/tuple.dart'; | ||
| import '../repositories/cache_manager.dart'; | ||
| import '../repositories/nostr_transport.dart'; | ||
| import 'accounts/accounts.dart'; | ||
| import 'engines/network_engine.dart'; | ||
|
|
@@ -74,6 +75,9 @@ class RelayManager<T> { | |
| /// AUTH strategy: eager (on challenge) or lazy (on auth-required) | ||
| final bool eagerAuth; | ||
|
|
||
| /// cache manager for updating event sources | ||
| final CacheManager? cacheManager; | ||
|
|
||
| /// Creates a new relay manager. | ||
| RelayManager({ | ||
| required this.globalState, | ||
|
|
@@ -84,6 +88,7 @@ class RelayManager<T> { | |
| allowReconnect = true, | ||
| this.eagerAuth = false, | ||
| this.authCallbackTimeout = RequestDefaults.DEFAULT_AUTH_CALLBACK_TIMEOUT, | ||
| this.cacheManager, | ||
| }) : _accounts = accounts { | ||
| allowReconnectRelays = allowReconnect; | ||
| _connectSeedRelays(urls: bootstrapRelays ?? DEFAULT_BOOTSTRAP_RELAYS); | ||
|
|
@@ -501,6 +506,29 @@ class RelayManager<T> { | |
| if (globalState.inFlightBroadcasts[eventId] != null && | ||
| !globalState | ||
| .inFlightBroadcasts[eventId]!.networkController.isClosed) { | ||
| // Update cache with source if broadcast was successful | ||
| if (success && cacheManager != null) { | ||
| final broadcastState = globalState.inFlightBroadcasts[eventId]; | ||
| final event = broadcastState?.event; | ||
| if (event != null) { | ||
| // Only update cache if event was already saved (saveToCache was true) | ||
| // Check if event exists in cache before updating sources | ||
| cacheManager!.loadEvent(eventId).then((cachedEvent) { | ||
| if (cachedEvent != null) { | ||
| // Merge existing sources with new relay URL, avoiding duplicates | ||
| final updatedSources = { | ||
| ...event.sources, | ||
| relayConnectivity.url | ||
| }.toList(); | ||
| final updatedEvent = event.copyWith(sources: updatedSources); | ||
| cacheManager!.saveEvent(updatedEvent); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should saveEvent be awaited? |
||
| // Update the event in broadcast state | ||
| broadcastState!.event = updatedEvent; | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| globalState.inFlightBroadcasts[eventId]?.networkController.add( | ||
| RelayBroadcastResponse( | ||
| relayUrl: relayConnectivity.url, | ||
|
|
@@ -659,7 +687,7 @@ class RelayManager<T> { | |
| } | ||
|
|
||
| final eventWithSources = | ||
| event.copyWith(sources: [...event.sources, connectivity.url]); | ||
| event.copyWith(sources: {...event.sources, connectivity.url}.toList()); | ||
|
|
||
| if (state.networkController.isClosed) { | ||
| // this might happen because relays even after we send a CLOSE subscription.id, they'll still send more events | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ import '../../entities/nip_01_event.dart'; | |
| /// given a stream with Nip01 events it tracks the id and adds the one to the provided stream controller \ | ||
| /// tracking of the happens in the tracking list | ||
| class StreamResponseCleaner { | ||
| final Set<String> _trackingSet; | ||
| final Map<String, Set<String>> _trackingMap; // event_id -> set of sources | ||
| final List<Stream<Nip01Event>> _inputStreams; | ||
| final StreamController<Nip01Event> _outController; | ||
| final List<EventFilter> _eventOutFilters; | ||
|
|
@@ -24,7 +24,7 @@ class StreamResponseCleaner { | |
| required List<Stream<Nip01Event>> inputStreams, | ||
| required StreamController<Nip01Event> outController, | ||
| required List<EventFilter> eventOutFilters, | ||
| }) : _trackingSet = trackingSet, | ||
| }) : _trackingMap = {for (var id in trackingSet) id: {}}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. final instead of var |
||
| _outController = outController, | ||
| _inputStreams = inputStreams, | ||
| _eventOutFilters = eventOutFilters; | ||
|
|
@@ -37,16 +37,28 @@ class StreamResponseCleaner { | |
|
|
||
| void _addStreamListener(Stream<Nip01Event> stream) { | ||
| stream.listen((event) { | ||
| // check if event id is in the set | ||
| if (_trackingSet.contains(event.id)) { | ||
| if (_outController.isClosed) { | ||
| return; | ||
| } | ||
|
|
||
| if (_outController.isClosed) { | ||
| // check if event id is already seen | ||
| final existingSources = _trackingMap[event.id]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when i understand correctly the sources map is per ndk session, no db integration for now, right?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't that depend wether the cache impl does persist it or not? |
||
| if (existingSources != null) { | ||
| // Event already seen - merge sources if this event has new sources | ||
| if (event.sources.isNotEmpty) { | ||
| final newSources = Set<String>.from(existingSources)..addAll(event.sources); | ||
| // Only emit if we have new sources to add | ||
| if (newSources.length > existingSources.length) { | ||
| _trackingMap[event.id] = newSources; | ||
| final mergedEvent = event.copyWith(sources: newSources.toList()); | ||
| _outController.add(mergedEvent); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| _trackingSet.add(event.id); | ||
| // First time seeing this event | ||
| _trackingMap[event.id] = event.sources.toSet(); | ||
|
|
||
| // check against filters | ||
| for (final filter in _eventOutFilters) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,44 @@ | ||||||
| import 'package:ndk/ndk.dart'; | ||||||
| import 'package:ndk/shared/nips/nip01/bip340.dart'; | ||||||
| import 'package:test/test.dart'; | ||||||
|
|
||||||
| import '../mocks/mock_event_verifier.dart'; | ||||||
| import '../mocks/mock_relay.dart'; | ||||||
|
|
||||||
| void main() async { | ||||||
| test("braodcast should update source", () async { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in test name: "braodcast" should be "broadcast". Proposed fix- test("braodcast should update source", () async {
+ test("broadcast should update source", () async {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| final relay = MockRelay(name: "relay"); | ||||||
|
|
||||||
| await relay.startServer(); | ||||||
|
|
||||||
| final ndk = Ndk(NdkConfig( | ||||||
| eventVerifier: MockEventVerifier(), | ||||||
| cache: MemCacheManager(), | ||||||
| bootstrapRelays: [relay.url], | ||||||
| )); | ||||||
|
|
||||||
| final keypair = Bip340.generatePrivateKey(); | ||||||
| final signer = Bip340EventSigner( | ||||||
| privateKey: keypair.privateKey, | ||||||
| publicKey: keypair.publicKey, | ||||||
| ); | ||||||
| ndk.accounts.loginExternalSigner(signer: signer); | ||||||
|
|
||||||
| final event = Nip01Event( | ||||||
| pubKey: keypair.publicKey, | ||||||
| kind: 1, | ||||||
| tags: [], | ||||||
| content: "content", | ||||||
| ); | ||||||
|
|
||||||
| await ndk.broadcast.broadcast(nostrEvent: event).broadcastDoneFuture; | ||||||
|
|
||||||
| final localEvent = await ndk.config.cache.loadEvent(event.id); | ||||||
|
|
||||||
| expect(localEvent, isNotNull); | ||||||
| expect(localEvent!.sources, isNotEmpty); | ||||||
|
Comment on lines
+34
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check if broadcast updates cache with sources after relay responses
echo "=== Checking broadcast.dart for post-response cache updates ==="
cat packages/ndk/lib/domain_layer/usecases/broadcast/broadcast.dart
echo ""
echo "=== Looking for saveEvent calls with sources ==="
rg -n -C5 'saveEvent.*sources|sources.*saveEvent' --type dart
echo ""
echo "=== Checking if OK responses trigger cache updates ==="
rg -n -C5 '"OK".*sources|sources.*"OK"' --type dartRepository: relaystr/ndk Length of output: 8951 🏁 Script executed: #!/bin/bash
# Find where sources are assigned to events
echo "=== Looking for where sources are assigned/populated ==="
rg -n 'sources\s*=' --type dart | head -20
echo ""
echo "=== Checking relay_manager.dart for relay response handling ==="
fd -t f 'relay_manager.dart' -x cat {}
echo ""
echo "=== Checking for saveEvent calls and their context ==="
rg -n 'saveEvent' --type dart -B2 -A2Repository: relaystr/ndk Length of output: 50368 🏁 Script executed: #!/bin/bash
echo "=== Checking network_engine.dart for broadcast handling ==="
fd -t f 'network_engine.dart' -x cat {} | head -200
echo ""
echo "=== Looking at the test file ==="
cat packages/ndk/test/usecases/broadcast_sources_test.dartRepository: relaystr/ndk Length of output: 1919 The test documents unimplemented functionality: broadcast responses must update the cached event with sources. The event is saved to cache before broadcasting (line 81 in 🤖 Prompt for AI Agents |
||||||
|
|
||||||
| await ndk.destroy(); | ||||||
| await relay.stopServer(); | ||||||
| }); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||
| import 'package:ndk/shared/nips/nip01/bip340.dart'; | ||||||
| import 'package:test/test.dart'; | ||||||
| import 'package:ndk/ndk.dart'; | ||||||
|
|
||||||
| import '../../mocks/mock_event_verifier.dart'; | ||||||
| import '../../mocks/mock_relay.dart'; | ||||||
|
|
||||||
| void main() async { | ||||||
| test("requests should update sources", () async { | ||||||
| final bannedWord = "cow"; | ||||||
|
|
||||||
| final relay1 = MockRelay(name: "relay 1"); | ||||||
| final relay2 = MockRelay(name: "relay 2"); | ||||||
| final relay3 = MockRelay(name: "relay 2", bannedWord: bannedWord); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copy-paste error in relay name.
Proposed fix- final relay3 = MockRelay(name: "relay 2", bannedWord: bannedWord);
+ final relay3 = MockRelay(name: "relay 3", bannedWord: bannedWord);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
final relay3 = MockRelay(name: "relay 3", bannedWord: bannedWord);This ensures each relay has a unique name, making test output and debugging much clearer. 🧠 Learnings used |
||||||
|
|
||||||
| await relay1.startServer(); | ||||||
| await relay2.startServer(); | ||||||
| await relay3.startServer(); | ||||||
|
|
||||||
| final ndk = Ndk(NdkConfig( | ||||||
| eventVerifier: MockEventVerifier(), | ||||||
| cache: MemCacheManager(), | ||||||
| bootstrapRelays: [relay1.url, relay2.url, relay3.url], | ||||||
| )); | ||||||
|
|
||||||
| final keypair = Bip340.generatePrivateKey(); | ||||||
| final signer = Bip340EventSigner( | ||||||
| privateKey: keypair.privateKey, | ||||||
| publicKey: keypair.publicKey, | ||||||
| ); | ||||||
| ndk.accounts.loginExternalSigner(signer: signer); | ||||||
|
|
||||||
| final event = Nip01Event( | ||||||
| pubKey: keypair.publicKey, | ||||||
| kind: 1, | ||||||
| tags: [], | ||||||
| content: bannedWord, | ||||||
| ); | ||||||
| await ndk.broadcast.broadcast(nostrEvent: event).broadcastDoneFuture; | ||||||
|
|
||||||
| await ndk.config.cache.clearAll(); | ||||||
|
|
||||||
| final query = ndk.requests.query(filter: Filter(ids: [event.id])); | ||||||
| final events = await query.future; | ||||||
|
|
||||||
| // The last event should have all merged sources | ||||||
| expect(events.last.sources.length, equals(2)); | ||||||
|
|
||||||
| await ndk.destroy(); | ||||||
| await relay1.stopServer(); | ||||||
| await relay2.stopServer(); | ||||||
| await relay3.stopServer(); | ||||||
| }); | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to keep the cacheManager out of relay manager. We are already dealing with cache outside, so I would want to keep it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@1-leo wdyt?