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-46: restore original types #1155

Closed
wants to merge 1 commit into from

Conversation

alexgleason
Copy link
Member

In #1047 we agreed to leave the protocol the same, and just clarify its usage. However a change slipped through, which I think wasn't intentional, because nobody building existing NIP-46 apps has updated AFAIK, and I don't think they knew they were supposed to.

Fixes #1154

@alexgleason
Copy link
Member Author

This NIP is such a shit show. Now it's possible this revision could break an implementation developed in the past 40 days by restoring the behavior from 1 year ago.

@alexgleason
Copy link
Member Author

cc @erskingardner @nostrband

Are yall stringifying these params or not?

@erskingardner
Copy link
Contributor

There weren't any changes, I only made things in the NIP more explicit. You can't put anything into an event that isn't stringified if you want it to be valid JSON. If you weren't doing it explicitly, then whatever language/framework/library you're using was probably doing it for you.

@alexgleason
Copy link
Member Author

@erskingardner Okay but if you stringify a property inside an object it's not the same as stringifying the object

> JSON.stringify({ a: { b: 1 } });
'{"a":{"b":1}}'
> JSON.stringify({ a: JSON.stringify({ b: 1 }) });
'{"a":"{\\"b\\":1}"}'

I assume it was changed so the message type can fit cleanly into C structs or whatever. But it's a breaking change.

@erskingardner
Copy link
Contributor

ahhh, ok. I see what you mean now. yeah, my interpretation of how I'd written it was not as two steps but you're right, that is confusing and doing it twice does change things.

@nostrband
Copy link
Collaborator

All params are strings, putting an event to signEvent without stringify is a breaking change. Result is a string, removing stringify from there is a breaking change too. I don't like it, but that's how it's implemented everywhere.

@fiatjaf fiatjaf closed this Apr 2, 2024
@alexgleason
Copy link
Member Author

@fiatjaf Why did you close this? I'm confused.

Is it true people have been stringifying against the spec all along? I don't believe it.

@fiatjaf
Copy link
Member

fiatjaf commented Apr 2, 2024

Unfortunately it is. I assumed it wasn't because the spec didn't say so, but when I went to look in order to implement it -- much before the NIP-46 rewrite -- it was implemented like that in ndk, nsecbunker and snort, and that's how I implemented it in nostr-tools and go-nostr and these things all interoperated correctly together with the implementations that came afterwards on coracle and nsec.app.

@alexgleason
Copy link
Member Author

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.

Type of NIP-46 message has changed
4 participants