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

including nostr specialized types #409

Merged
merged 13 commits into from
Sep 9, 2024

Conversation

antonioconselheiro
Copy link
Contributor

@antonioconselheiro antonioconselheiro commented May 30, 2024

exporting nip19 nostr specific types, implements: #408

@antonioconselheiro antonioconselheiro changed the title including nostr types including nostr specialized types May 30, 2024
@fiatjaf
Copy link
Collaborator

fiatjaf commented Jun 17, 2024

Is this ready to merge?

@antonioconselheiro
Copy link
Contributor Author

antonioconselheiro commented Jun 17, 2024

In my view yes, but I'm nobody.
You think the regex in type guard are ok?

@fiatjaf
Copy link
Collaborator

fiatjaf commented Jun 17, 2024

Yes, except the types should be called "npub" not "npublic", "nsec" not "nsecret", but I can fix that after I merge.

@antonioconselheiro
Copy link
Contributor Author

so, maybe nadress should be just nadrr

@antonioconselheiro
Copy link
Contributor Author

I've changed the names in types, type guards and in the included unit tests

@fiatjaf
Copy link
Collaborator

fiatjaf commented Jul 18, 2024

I'll take a look at this soon, if I don't forget. Thank you.

@antonioconselheiro
Copy link
Contributor Author

antonioconselheiro commented Jul 18, 2024

Thank you for Nostr sir,
but talking about types exportations,
can this lib export too the { write: boolean, read: boolean } relay config interface with a name?
As you said, it doesn't hurt.
If yes, I'll include in this pull request.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Jul 18, 2024

It does hurt my ability to understand my own library.

@alexgleason
Copy link
Collaborator

Naming types is overrated. It's better to just copy the same type in multiple places, unless it's a truly universal interface.

@antonioconselheiro
Copy link
Contributor Author

ok

@antonioconselheiro
Copy link
Contributor Author

Another thing...
There is already a regex to valid nip05 and would be nice include type guard for this too,
NIP05 type is included in this PR but not the type guard, but to include this it will be in nip5.ts and not with the others in NostrTypeGuard object in core.ts.

Make sense include this in nip5.ts?

export const isNip05 = (value?: string | null): value is Nip05 => NIP05_REGEX.test(value || '')

@fiatjaf
Copy link
Collaborator

fiatjaf commented Jul 21, 2024

Yes. In nip.ts

@antonioconselheiro
Copy link
Contributor Author

NIP05_REGEX matches for domain.com and [email protected], but my type in this PR for NIP05 is ${string}@${string}, is my type right? or should have a regex with @ as mandatory to validate?

@alexgleason
Copy link
Collaborator

NIP05_REGEX is wrong, I learned after looking at NIP-05 itself. It's too lenient. I was following what the person had done before, but the @ is actually required by the spec.

Btw I think this is still a pointless type.

@antonioconselheiro
Copy link
Contributor Author

Thank you, but I can't see they as pointless types, I see a lot of value in this.

@antonioconselheiro
Copy link
Contributor Author

antonioconselheiro commented Jul 21, 2024

This unit test requires NIP05_REGEX to include @ as optional 🤔, is this wrong? Or is this a particular validation of the function queryProfile?
image

@antonioconselheiro
Copy link
Contributor Author

antonioconselheiro commented Sep 9, 2024

nrelay got deprecated, so I should remove nrelay from this pr, right?

Nostrify uses NRelay name for a symbol too, removing this will avoid package conflit.

@fiatjaf fiatjaf merged commit ee76d69 into nbd-wtf:master Sep 9, 2024
2 checks passed
@antonioconselheiro
Copy link
Contributor Author

antonioconselheiro commented Sep 9, 2024

you just merged it, but I found a problem in isNcryptsec guard, it is returning 'is Note' instead is Ncryptsec 😳😥

fiatjaf added a commit that referenced this pull request Sep 9, 2024
@antonioconselheiro antonioconselheiro deleted the including-nostr-types branch September 9, 2024 17:33
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