fiatjaf / nocomment

See a demo at https://fiatjaf.com/nostr.html
https://nocomment.pages.dev/embed.js
136 stars 17 forks source link

window.nostr.signEvent() has returned an error: Invalid event. #9

Closed gzuuus closed 1 year ago

gzuuus commented 1 year ago

Tested with nos2x, working perfectly, but when testing with alby it doesn't work, the error message is window.nostr.signEvent() has returned an error: Invalid event.

fiatjaf commented 1 year ago

@bumi can you offer more insight? Alby seems to be doing some validation that nos2x isn't. Who is wrong?

bumi commented 1 year ago

I think in nostr-tools the id validation was recently removed which is not in Alby, yet. (https://github.com/nbd-wtf/nostr-tools/commit/4b36848b2d1d377e4832d9f66ecaa489240e2ba2#diff-86e685055ffe2bbb0133c9d084d6d409934450d1adf8139413e7cf3d77b32196L60) I had seen another report where the ID was missing. @gzuuus do you have an example of the event.

Is the ID required or not? We should update this in the next release.

gzuuus commented 1 year ago

yes, here is a sample event generated through nocomment

{
  "event": {
    "pubkey": "b42f038a31e4dba02f4286a22bc19a98be8426ad28148941f61ea58b2e9ccd66",
    "created_at": 1673796819,
    "kind": 1,
    "tags": [
      [
        "e",
        "90676d7a26adaea0fd13c367dbbedf643215fa80b659173cb772ae54c621c5ae",
        "wss://nostr-pub.wellorder.net",
        "root"
      ]
    ],
    "content": "hi!"
  }
}
bumi commented 1 year ago

yup, this seems to miss the ID.

we're updating the validation to be in sync again with nostr-tools. I guess @fiatjaf ID should not be validated at this stage? can you try this development build: https://github.com/getAlby/lightning-browser-extension/pull/1979

it should be released tomorrow.

fiatjaf commented 1 year ago

ID should be validated. This is a bug on my stuff.

EDIT: oh, I see, actually I made it so nos2x injects the correct ID as it is signing the event.

I think this is better. Relieves the web app of the burden of having to implement sha256. It can just make an event in the most naïve manner possible and give it to the extension to perform cryptographic stuff and fill in the event. What do you think, @bumi @sondreb?

If you're ok with that I'll modify NIP-07 to say the extension should accept an event without ID and fill in the ID as well as the pubkey and sig.

bumi commented 1 year ago

+1 dealing with sha256 in the web app is annoying. this just would put a bit more logic on the signing apps.

thinking out load: is signEvent then still correct or is maybe finalizeEvent or something else a better name?

fiatjaf commented 1 year ago

finalizeEvent would have been better, but as always I think it's too late to change now.

bumi commented 1 year ago

right now web apps can not rely on signers adding the ID. we would change the implementation now, so maybe that's a thing to consider.

nos2x is already implementing it in the way that it adds the ID? - I will then also adjust Alby.

fiatjaf commented 1 year ago

nos2x is already adding the ID. I think most apps are adding the ID themselves, so this error had never happened.

We just need Blockcore extension to also add the ID.

bumi commented 1 year ago

ok, will update Alby acordingly

fiatjaf commented 1 year ago

OK, it looks like Blockcore already fills in the .id and .pubkey.

sondreb commented 1 year ago

Yes, Blockcore already does it so I think extensions should just append the .id. I'm not 100% sure if Blockcore Wallet will replace the .pubkey if provided, I think maybe it won't so required that user pick the suggested pubkey from nostr client.

bumi commented 1 year ago

upadted Alby acordingly and this will be in the next release