feat: add nostr wallet integration - #23
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Nostr wallet integration, including utilities for key management, event signing, and specialized support for NIP-57 zaps and NIP-58 badges. The review identifies critical issues in the publishEvent and subscribeToZaps functions, specifically regarding resource leaks from repeated SimplePool instantiation and runtime errors due to incorrect API usage of the nostr-tools library. Additionally, it is recommended to use the canonicalize helper when generating attestation content to ensure deterministic event IDs that align with the calculated data hashes.
| const publisher = options.publisher ?? new SimplePool(); | ||
| const acknowledgements = await Promise.all(publisher.publish(relays, event)); |
There was a problem hiding this comment.
The default implementation using new SimplePool() has two significant issues:
- Resource Leak: Creating a new
SimplePoolon every call topublishEventwill leak WebSocket connections, asSimplePoolmaintains an internal cache of relay connections that are never closed. You should use a shared, module-level pool instance instead. - Runtime Error:
SimplePool.publishfromnostr-toolsreturns a singlePromise<void>(in v2) or aPubobject (in v1). It does not return an array of promises. Consequently,Promise.all(publisher.publish(...))will throw aTypeErrorbecause it expects an iterable. This bug was likely masked in tests by theFakePublisherimplementation.
| const publisher = options.publisher ?? new SimplePool(); | ||
|
|
||
| return publisher.subscribe( |
There was a problem hiding this comment.
| { | ||
| kind: KIND_APP_DATA, | ||
| created_at: now(), | ||
| content: JSON.stringify(attestation.data), |
There was a problem hiding this comment.
To ensure the attestation has a stable event ID regardless of the object key order in memory, you should use the canonicalize helper for the content field. This ensures consistency between the event content and the hash used in the d tag, making the attestation truly deterministic.
| content: JSON.stringify(attestation.data), | |
| content: canonicalize(attestation.data), |
|
Addressed the review notes in b50b6e9:\n\n- Destroy internally-created SimplePool instances after publish and when owned subscriptions are closed, while leaving injected test/app publishers untouched.\n- Keep subscribeToZaps on the nostr-tools SimplePool subscribe path and wrap close cleanup.\n- Use canonicalized attestation content so event IDs and hash tags are derived from the same deterministic payload.\n- Treat undefined values as null in canonicalization to avoid runtime hashing failures.\n\nRe-verified:\n- npx vitest run tests/wallet/nostr.test.ts -> 8 passed\n- npx tsc --noEmit --skipLibCheck --target ES2022 --module NodeNext --moduleResolution NodeNext src/wallet/nostr.ts tests/wallet/nostr.test.ts -> passed |
Summary
src/wallet/nostr.tswith Nostr keypair generation, event signing/verification, relay publishing, zap request publishing, zap receipt subscriptions, immutable attestations, and NIP-58 badge helpersNOSTR_RELAY_URL,NOSTR_RELAY_LIST, andNOSTR_PRIVATE_KEYwhile still allowing injected relays/private keys for tests and servicesVerification
npx vitest run tests/wallet/nostr.test.ts(6 passed)npx tsc --noEmit --skipLibCheck --target ES2022 --module NodeNext --moduleResolution NodeNext src/wallet/nostr.ts tests/wallet/nostr.test.tsNote: tests use an injected relay publisher so they do not hit live relays or attempt Lightning settlement. The repo-wide
npm run buildremains blocked by pre-existing unrelated project issues noted on the other DS bounty PRs (database/**/*outsiderootDir, missing@as-integrations/fastify, and existing Drizzle/resolver typing errors).Closes #2