ndk icon indicating copy to clipboard operation
ndk copied to clipboard

NIP-66/115 support

Open dskvr opened this issue 1 year ago • 1 comments

help needed

  • Where is the correct location for the geotags stuff? Intuitively this would be a mixin or similar, but see no existing patterns. Presently extending a new class that extends NDKEvent inside kinds/nip66 but this is clearly not right.

todo

  • [x] squash initial history
  • [x] accidentally committed lock file, revert before squash
  • [x] accidentally committed changes to content-tagger.ts, revert before squash
  • [x] relay-monitor.test.ts
  • [x] relay-discovery.test.ts
  • [x] relay-meta.test.ts
  • [x] add mocks for async methods in relay-monitor.test.ts
  • [x] Monitor discovery ... it's not a kind, don't know appropriate location, static methods inside RelayMonitor.ts? An NDKRelayMonitor class that implements NDKUser, NDKRelayLists and NDKProfile?
  • [x] RelayMonitor fetches
    • [x] add tests for RelayMonitor fetches
  • [ ] fix GeoCodedEvents (if needed)
    • [x] add GeoCodedEvents.test.ts
  • [x] squash

depends on https://github.com/nostr-protocol/nips/pull/230 depends on https://github.com/nostr-protocol/nips/pull/952

dskvr avatar Mar 20 '24 08:03 dskvr

Leaning towards extracting fetchers from RelayMonitor (relay discovery and meta fetchers) and the entire NDKRelayMonitors (monitor discovery) from this PR.

Rationale:

  • To obtain a full dataset of relays would most likely require multiple subscriptions due to the dataset being larger than the average max_events value of most relays. (like nostr-fetch does)
  • The coverage over NIP-66 in this PR is not necessary to be included in NDK, as this coverage bloats the package far too much for an auxiliary kind that may take some time to be incorporated into clients and internal functionalities such as outbox

Certain methods, such as the NIP-66 filter generation and a flexible fetcher would remain in this PR. The helper methods are what I am targeting to externalize to another library that implements NDK, likely via nostr-fetch

dskvr avatar Mar 24 '24 11:03 dskvr

Is this PR still active @dskvr ? There is one task left in the list above so I'm not sure if I should review or not?

erskingardner avatar May 27 '24 07:05 erskingardner

@erskingardner Never received any feedback on that item so couldn't finish it.

I'm probably going to move most functionality into an NDK extension, because NDK does not natively support fetching of a full dataset and full coverage over NIP-66 would needlessly bloat NDK.

Edit What would remain in this PR is the NIP-66 event kinds that extend NDKEvent (less the fetcher methods they contain!)

I can update this PR to reflect the updated, previously expressed scope of NIP-66 in NDK. Changing PR to draft.

dskvr avatar May 27 '24 09:05 dskvr

@erskingardner reduced the scope, still need feedback on geocoded.ts, as this should really be a mix-in. Locally I use mix-ins but was not sure I wanted to introduce that pattern to NDK.

jest --verbose ./kinds/nip66/*.test.ts ./geocoded.test.ts

Didn't check coverage, but the important bits are tested.

EDIT: Need to update the geotags in this PR, as NIP-115 has changed. Missed that.

dskvr avatar Jun 24 '24 13:06 dskvr

@dskvr when you say mixins do you mean extensions? LIke, you'd add the functionality via an external repo/package to NDK?

erskingardner avatar Jun 26 '24 08:06 erskingardner

I mean a mixin pattern, instead of extending or implementing, which is inherently linear, you can "mix in" properties/methods from many classes in an ad-hoc fashion. It's a holdover from vanilla JS that doesn't translate well to TS for many reasons. IMO Mixins are probably not a good idea in a library like NDK, they are fragile and complex with many caveats.

And with regards to extensions, I have been working on a theoretical extension and NDK would require quite a number of changes to facilitate anything other than extremely simple extensions. I've retreated from the idea for now and instead just have a library that depends on NDK with the option to accept an NDK instance.

Edit: Sorry for closing, I truly disdain the github hotkeys for closing issues. ~Need to find a way to disable those~ Disabled. Never again

dskvr avatar Jun 26 '24 10:06 dskvr

@dskvr can you include some code in a comment on how an application would use this?

I now have a docs/tutorial directory -- maybe even start a stab at a docs/tutorial/relay-discoverability.md (just a general WIP overview, not like writing an entire finished doc) -- but I think that would help understand and clear the interface

pablof7z avatar Jul 13 '24 09:07 pablof7z

@pablof7z doc (updated)

Only did one edit pass, and didn't test the tutorial yet, but that's the general idea. Should be noted that the tests I submitted are testing most (if not all) of these methods, and so usage can be inferred from the tests.

dskvr avatar Jul 15 '24 11:07 dskvr