Skip to content

feat: add nostr wallet integration - #23

Open
saim256 wants to merge 2 commits into
AuthorPrime:mainfrom
saim256:nostr-wallet-issue-2
Open

feat: add nostr wallet integration#23
saim256 wants to merge 2 commits into
AuthorPrime:mainfrom
saim256:nostr-wallet-issue-2

Conversation

@saim256

@saim256 saim256 commented May 11, 2026

Copy link
Copy Markdown

Summary

  • adds src/wallet/nostr.ts with Nostr keypair generation, event signing/verification, relay publishing, zap request publishing, zap receipt subscriptions, immutable attestations, and NIP-58 badge helpers
  • supports NOSTR_RELAY_URL, NOSTR_RELAY_LIST, and NOSTR_PRIVATE_KEY while still allowing injected relays/private keys for tests and services
  • adds offline Vitest coverage for NIP-01 signing, relay publishing, NIP-57 zap requests, zap subscriptions, attestations, badge events, and relay parsing

Verification

  • 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.ts

Note: tests use an injected relay publisher so they do not hit live relays or attempt Lightning settlement. The repo-wide npm run build remains blocked by pre-existing unrelated project issues noted on the other DS bounty PRs (database/**/* outside rootDir, missing @as-integrations/fastify, and existing Drizzle/resolver typing errors).

Closes #2

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/wallet/nostr.ts Outdated
Comment on lines +86 to +87
const publisher = options.publisher ?? new SimplePool();
const acknowledgements = await Promise.all(publisher.publish(relays, event));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The default implementation using new SimplePool() has two significant issues:

  1. Resource Leak: Creating a new SimplePool on every call to publishEvent will leak WebSocket connections, as SimplePool maintains an internal cache of relay connections that are never closed. You should use a shared, module-level pool instance instead.
  2. Runtime Error: SimplePool.publish from nostr-tools returns a single Promise<void> (in v2) or a Pub object (in v1). It does not return an array of promises. Consequently, Promise.all(publisher.publish(...)) will throw a TypeError because it expects an iterable. This bug was likely masked in tests by the FakePublisher implementation.

Comment thread src/wallet/nostr.ts Outdated
Comment on lines +127 to +129
const publisher = options.publisher ?? new SimplePool();

return publisher.subscribe(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to publishEvent, creating a new SimplePool here leads to a resource leak. Additionally, SimplePool does not have a subscribe method with this signature (it uses subscribeMany in v2 or sub in v1). This will result in a runtime error when the default publisher is used.

Comment thread src/wallet/nostr.ts Outdated
{
kind: KIND_APP_DATA,
created_at: now(),
content: JSON.stringify(attestation.data),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
content: JSON.stringify(attestation.data),
content: canonicalize(attestation.data),

@saim256

saim256 commented May 11, 2026

Copy link
Copy Markdown
Author

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

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.

Implement Nostr Wallet & Zap Integration

1 participant