Add Nostr verify identity component - #85
Open
alokdangre wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds a Changesnostr-verify-identity Web Component
Relay Directory Crawler Script
Sequence DiagramsequenceDiagram
actor User
participant NostrVerifyIdentity as nostr-verify-identity
participant NIP07 as NIP-07 Signer
participant XoEmbed as publish.x.com/oembed (JSONP)
participant NDK as NDK/Relay
rect rgba(100, 149, 237, 0.5)
Note over User,NIP07: Connect step
User->>NostrVerifyIdentity: click "Connect Nostr"
NostrVerifyIdentity->>NIP07: getUserPubkey()
NIP07-->>NostrVerifyIdentity: pubkey → npub
NostrVerifyIdentity-->>User: show proof text + intent URL (proof step)
end
rect rgba(144, 238, 144, 0.5)
Note over User,XoEmbed: Verify step
User->>NostrVerifyIdentity: paste tweet URL, click "Verify & publish"
NostrVerifyIdentity->>XoEmbed: fetchProofTweet (JSONP)
XoEmbed-->>NostrVerifyIdentity: OEmbedResult (author handle + tweet html)
NostrVerifyIdentity->>NostrVerifyIdentity: verifyTwitterProof (handle match + npub in html)
end
rect rgba(255, 165, 0, 0.5)
Note over NostrVerifyIdentity,NDK: Publish step
NostrVerifyIdentity->>NIP07: sign kind:10011 event
NIP07-->>NostrVerifyIdentity: signed event
NostrVerifyIdentity->>NDK: publish kind:10011
NostrVerifyIdentity->>NDK: publish kind:0 (if existing profile)
NDK-->>NostrVerifyIdentity: published kinds[]
NostrVerifyIdentity-->>User: dispatch nc:verified event (done step)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
scripts/relay-directory-crawler.mjs (1)
566-573: ⚖️ Poor tradeoffSequential tweet verification is a performance bottleneck.
With
--max-proofs 250(default) and--timeout-ms 12000(default), worst-case verification time is ~50 minutes. EachverifyCandidateawaits sequentially.Consider adding concurrency control for parallel verification:
Proposed approach using a concurrency limiter
// Helper for bounded concurrency async function mapWithConcurrency(items, fn, concurrency = 10) { const results = []; const executing = new Set(); for (const item of items) { const promise = fn(item).then((result) => { executing.delete(promise); return result; }); executing.add(promise); results.push(promise); if (executing.size >= concurrency) { await Promise.race(executing); } } return Promise.all(results); } // Usage in runCrawler: const verifiedOrRejected = await mapWithConcurrency( candidates.slice(0, proofLimit), (candidate) => verifyCandidate(candidate, args.timeoutMs), 10 // concurrent requests );The same pattern could be applied to
checkZapSupportat lines 597-599.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/relay-directory-crawler.mjs` around lines 566 - 573, The sequential awaiting of verifyCandidate calls in the args.verifyTweets block creates a significant performance bottleneck, potentially taking ~50 minutes with default settings. Implement a helper function that manages bounded concurrency (accepting an array of items, an async function to apply, and a concurrency limit parameter), then replace the sequential for loop that pushes to verifiedOrRejected with a call to this concurrency-controlled function, passing the candidates slice, verifyCandidate, and an appropriate concurrency limit. Additionally, apply the same concurrency pattern to the checkZapSupport verification logic to parallelize those operations as well.scripts/relay-directory-crawler.test.mjs (4)
15-27: ⚡ Quick winConsider adding edge-case tests for robustness.
The current tests cover the happy path and common invalid cases. Consider adding tests for:
null,undefined, and empty string inputs- Handles with leading/trailing whitespace
- Exactly 15 and 16 character handles (boundary testing)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/relay-directory-crawler.test.mjs` around lines 15 - 27, The test suite for the normalizeTwitterHandle function lacks edge-case coverage that would help ensure robustness. Add additional test cases within the describe block to cover: null, undefined, and empty string inputs; handles with leading and trailing whitespace; and boundary cases for exactly 15 and 16 character handles. These tests should verify that normalizeTwitterHandle handles these edge cases appropriately by either normalizing them correctly or returning null as expected.
29-37: ⚡ Quick winConsider adding edge-case tests for input validation.
Add tests for:
null,undefined, and empty string inputs- Malformed URLs without status path
- Very long numeric strings (to verify regex doesn't match arbitrary numbers)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/relay-directory-crawler.test.mjs` around lines 29 - 37, The extractTweetId test suite is missing edge-case tests that are important for ensuring robust input validation. Add additional test cases to the existing it block or create new test cases within the describe('extractTweetId') block to cover: null, undefined, and empty string inputs (these should likely return null), malformed URLs that lack a status path component (to ensure the regex properly validates URL structure), and very long numeric strings that don't match the actual tweet ID pattern (to verify the function doesn't incorrectly match arbitrary long numbers). Each edge case should have an expect statement validating the expected behavior of extractTweetId when given these problematic inputs.
91-116: ⚡ Quick winConsider expanding WoT scoring test coverage.
The current test validates invariant preservation with a single follow event. Consider adding tests for:
- Multiple WoT signals (follows, reports, assertions)
- Different
identityStatusvalues (verified,claimed,failed)- Score computation edge cases (no signals, conflicting signals)
- Different
autoZapAllowedstarting valuesThis would increase confidence in the scoring algorithm's correctness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/relay-directory-crawler.test.mjs` around lines 91 - 116, The test for computeWotScores only covers a single scenario with a follow event and claimed identity status. Add additional test cases to the same describe block to cover: (1) multiple WoT signals such as follows, reports, and assertions in a single call to computeWotScores, (2) different identity status values (verified, claimed, and failed) to ensure scoring works correctly for each, (3) edge cases like calling computeWotScores with empty signals array and with conflicting signals, and (4) different starting values for autoZapAllowed (both true and false). Each test should verify that computeWotScores correctly computes wot scores while handling these various scenarios appropriately.
83-89: ⚡ Quick winAdd tests for invalid lightning address formats.
The test suite only covers the happy path. Consider adding tests for error cases:
- Invalid formats:
'no-at-sign','@missing-user','missing-domain@'- Edge cases:
null,undefined, empty string- Multiple
@symbols🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/relay-directory-crawler.test.mjs` around lines 83 - 89, The test suite for the lightningAddressToLnurlp function currently only covers the happy path with a valid email address. Add additional test cases within the same describe block to cover error cases and edge cases including invalid formats (no-at-sign, `@missing-user`, missing-domain@), null and undefined inputs, empty strings, and addresses with multiple @ symbols. Each test case should verify that the function handles these inputs appropriately, either by throwing an error or returning a specific error value as documented by the function's specification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/relay-directory-crawler.mjs`:
- Around line 288-294: The syndicationToken function loses precision when
converting the tweetId parameter to Number, since Tweet IDs (Snowflake IDs) are
19-digit numbers that exceed JavaScript's Number.MAX_SAFE_INTEGER limit. Replace
the Number conversion with BigInt to preserve precision during the calculation.
Convert the tweetId to BigInt, perform the division operation using BigInt
arithmetic, then convert the result to a string for the subsequent replace
operation to generate the syndication token correctly for all tweet IDs.
- Around line 110-115: The native WebSocket global used at line 111 in the
constructor attempt is only available in Node.js 22.0.0+ by default (or Node.js
21+ with the experimental flag), but there is no documented version requirement,
which may cause ReferenceError on unsupported versions. To fix this, either add
"engines": { "node": ">=22.0.0" } to package.json to enforce the minimum version
requirement, document the Node.js version requirement in the help text or README
for the script, or add a fallback mechanism that imports the `ws` package when
the native WebSocket is not available to support broader Node.js version
compatibility.
In `@scripts/relay-directory-crawler.test.mjs`:
- Line 79: The test assertion on the expect statement does not guard against the
potential case where metadataByPubkey.get(PUBKEY) returns undefined, which would
cause a TypeError instead of a clear test failure. Add a guard assertion before
accessing the lud16 property to first verify that metadataByPubkey.get(PUBKEY)
returns a defined value, then check that the lud16 property equals the expected
value. This ensures that if the entry is missing from metadataByPubkey, the test
will fail with a clear assertion message rather than a cryptic TypeError.
In `@src/nostr-verify-identity/nostr-verify.ts`:
- Around line 115-123: The proof URL input is cleared when render() is called
during verification failures in the handleVerify method (around line 115-123)
and other locations (lines 132-138 and 166-168), forcing users to re-paste the
URL on retry. Add a class property to store the proof URL draft in component
state, capture and persist the input value to this state property whenever it's
read in handleVerify, and then restore the stored value back into the input
field's value attribute during render or template binding to rehydrate the
input. Apply this state persistence pattern consistently across all three
affected locations where verification or error handling triggers a re-render.
In `@src/nostr-verify-identity/render.ts`:
- Around line 86-89: The input field with name="proof-url" lacks proper
accessibility labeling and currently relies only on placeholder and context
text. Add an explicit `<label>` element with a `for` attribute that references a
corresponding `id` on the input element. Assign a unique id to the proof-url
input and create a label element that references this id with appropriate
descriptive text for users completing the form.
In `@src/nostr-verify-identity/verify-utils.ts`:
- Around line 151-169: The verification logic does not validate that the author
handle was successfully resolved when no specific handle is declared. Add a
check after the npub validation and before the final success return to reject
cases where oembed.handle is empty or falsy, ensuring that unresolved author
handles fail verification before returning ok: true.
- Around line 231-237: The kind:10011 event is published unconditionally via
signAndPublish without verifying that the fetchEvent call succeeded, which means
if the fetch fails or times out, the empty tags fallback from mergeIdentityTag
will overwrite existing identity tags. Add a safety check before the
signAndPublish call to verify that existing10011 was successfully retrieved
(similar to the pattern used for kind:0 with `if (existing0)`), or alternatively
refactor to use NDK's publishReplaceable() method with appropriate error
handling to safely handle replaceable event updates and prevent data loss from
failed or uncertain reads.
---
Nitpick comments:
In `@scripts/relay-directory-crawler.mjs`:
- Around line 566-573: The sequential awaiting of verifyCandidate calls in the
args.verifyTweets block creates a significant performance bottleneck,
potentially taking ~50 minutes with default settings. Implement a helper
function that manages bounded concurrency (accepting an array of items, an async
function to apply, and a concurrency limit parameter), then replace the
sequential for loop that pushes to verifiedOrRejected with a call to this
concurrency-controlled function, passing the candidates slice, verifyCandidate,
and an appropriate concurrency limit. Additionally, apply the same concurrency
pattern to the checkZapSupport verification logic to parallelize those
operations as well.
In `@scripts/relay-directory-crawler.test.mjs`:
- Around line 15-27: The test suite for the normalizeTwitterHandle function
lacks edge-case coverage that would help ensure robustness. Add additional test
cases within the describe block to cover: null, undefined, and empty string
inputs; handles with leading and trailing whitespace; and boundary cases for
exactly 15 and 16 character handles. These tests should verify that
normalizeTwitterHandle handles these edge cases appropriately by either
normalizing them correctly or returning null as expected.
- Around line 29-37: The extractTweetId test suite is missing edge-case tests
that are important for ensuring robust input validation. Add additional test
cases to the existing it block or create new test cases within the
describe('extractTweetId') block to cover: null, undefined, and empty string
inputs (these should likely return null), malformed URLs that lack a status path
component (to ensure the regex properly validates URL structure), and very long
numeric strings that don't match the actual tweet ID pattern (to verify the
function doesn't incorrectly match arbitrary long numbers). Each edge case
should have an expect statement validating the expected behavior of
extractTweetId when given these problematic inputs.
- Around line 91-116: The test for computeWotScores only covers a single
scenario with a follow event and claimed identity status. Add additional test
cases to the same describe block to cover: (1) multiple WoT signals such as
follows, reports, and assertions in a single call to computeWotScores, (2)
different identity status values (verified, claimed, and failed) to ensure
scoring works correctly for each, (3) edge cases like calling computeWotScores
with empty signals array and with conflicting signals, and (4) different
starting values for autoZapAllowed (both true and false). Each test should
verify that computeWotScores correctly computes wot scores while handling these
various scenarios appropriately.
- Around line 83-89: The test suite for the lightningAddressToLnurlp function
currently only covers the happy path with a valid email address. Add additional
test cases within the same describe block to cover error cases and edge cases
including invalid formats (no-at-sign, `@missing-user`, missing-domain@), null and
undefined inputs, empty strings, and addresses with multiple @ symbols. Each
test case should verify that the function handles these inputs appropriately,
either by throwing an error or returning a specific error value as documented by
the function's specification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7bce19a-493e-41dc-923c-ab0cd626a1e8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
package.jsonscripts/relay-directory-crawler.mjsscripts/relay-directory-crawler.test.mjssrc/index.tssrc/nostr-verify-identity/__tests__/verify-utils.test.tssrc/nostr-verify-identity/nostr-verify.tssrc/nostr-verify-identity/render.tssrc/nostr-verify-identity/spec/spec.mdsrc/nostr-verify-identity/style.tssrc/nostr-verify-identity/verify-utils.tsstories/nostr-verify-identity/NostrVerifyIdentity.stories.tsxvite.config.esm.tsvite.config.umd.ts
Owner
|
Hi @alokdangre, 1 major feedback. The component and crawler should go in 2 separate PRs, so it's easy to discuss and improve each in isolation. Here, I will post my feedback on the identity component.
|
Contributor
Author
|
the identity component is primarily end-user facing, More precisely:
|
Contributor
Author
|
Because kind:0 is still the event most Nostr clients read as a user’s public profile, while kind:10011 is the canonical NIP-39 home. Improves compatibility during the transition The important guardrail is what the component already does: it only mirrors into kind:0 if a profile event already exists. That avoids creating a blank profile and accidentally wiping name, about, picture, or lud16 |
alokdangre
changed the title
Owner
|
Thanks for the comments Alok. DMed in Signal |
Owner
We can add a helper message somewhere in the component or the page, instructing the user to update their bio with their social links. And once you made the component, dev only, as with 'testing' folders. I will give do a 2nd round of review. Thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a new
<nostr-verify-identity>web component for NIP-39 identity verification, plus a relay directory crawler for collecting and scoring identity claims from Nostr relays.The new component guides a user through:
itag tokind:10011kind:0when a profile already existsIt also adds Storybook coverage, unit tests, and package/export wiring so the component can be consumed as part of the library.
What changed
src/nostr-verify-identity/component implementationValidation
Notes
kind:10011is treated as the canonical NIP-39 home, whilekind:0is mirrored only when a profile event already exists.Summary by CodeRabbit