Skip to content
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

NIP-52/66 support #192

Closed
wants to merge 45 commits into from
Closed

NIP-52/66 support #192

wants to merge 45 commits into from

Conversation

dskvr
Copy link

@dskvr dskvr commented Mar 20, 2024

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

  • squash initial history
  • accidentally committed lock file, revert before squash
  • accidentally committed changes to content-tagger.ts, revert before squash
  • relay-monitor.test.ts
  • relay-discovery.test.ts
  • relay-meta.test.ts
  • add mocks for async methods in relay-monitor.test.ts
  • 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?
  • RelayMonitor fetches
    • add tests for RelayMonitor fetches
  • fix GeoCodedEvents (if needed)
    • add GeoCodedEvents.test.ts
  • squash

depends on nostr-protocol/nips#230

@dskvr dskvr force-pushed the kind/nip66 branch 2 times, most recently from 9455064 to bd252ab Compare March 20, 2024 08:20
@dskvr dskvr changed the title nip66 NIP-66 support Mar 20, 2024
@dskvr dskvr marked this pull request as ready for review March 21, 2024 16:00
@dskvr dskvr marked this pull request as draft March 21, 2024 19:03
@dskvr
Copy link
Author

dskvr commented Mar 24, 2024

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 dskvr marked this pull request as ready for review March 24, 2024 22:27
@dskvr dskvr changed the title NIP-66 support NIP-66/115 support Mar 25, 2024
@erskingardner
Copy link
Collaborator

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?

@dskvr
Copy link
Author

dskvr commented May 27, 2024

@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 dskvr marked this pull request as ready for review June 24, 2024 13:11
@dskvr
Copy link
Author

dskvr commented Jun 24, 2024

@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 dskvr marked this pull request as draft June 24, 2024 13:32
@dskvr
Copy link
Author

dskvr commented Jul 15, 2024

@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 dskvr changed the title NIP-66/115 support NIP-32/66/115 support Jul 19, 2024
@dskvr dskvr changed the title NIP-32/66/115 support NIP-52/66/115 support Jul 23, 2024
@dskvr dskvr changed the title NIP-52/66/115 support NIP-52/66 support Jul 27, 2024
@dskvr dskvr requested a review from pablof7z August 15, 2024 08:17
@dskvr dskvr closed this Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants