nostr-protocol / nips

Nostr Implementation Possibilities
2.39k stars 582 forks source link

Add accountsChanged event for NIP-07 #1552

Open Anderson-Juhasc opened 3 weeks ago

Anderson-Juhasc commented 3 weeks ago

I think many clients that use NIP-07 will need something to watch when the user change of account, something like occurs on metamask.

async window.nostr.onAccountChanged(newPublicKey): { } //  Triggered when the user's account changes (e.g., switching accounts).

Usage:

window.nostr.onAccountChanged((newPublicKey) => {
    console.log("Nostr account changed:", newPublicKey);
    // Additional actions, such as refreshing data or updating the UI, can go here
  });
vitorpamplona commented 3 weeks ago

Yes please... Using multiple accounts with extensions sucks.

staab commented 3 weeks ago

This is probably necessary, but it will also break clients. Extensions should have some way to detect this feature, maybe by disabling it in the UI if the client doesn't add a handler.

cesardeazevedo commented 3 weeks ago

Kinda need this too.

alexgleason commented 3 weeks ago

Doesn't Alby already emit some kind of event when the user switches accounts? @bumi

Anderson-Juhasc commented 3 weeks ago

@alexgleason I think Alby doesn't have multi accounts yet, try Nostrame: https://chromewebstore.google.com/detail/nostrame/phfdiknibomfgpefcicfckkklimoniej

Anderson-Juhasc commented 3 weeks ago

I did something like that: https://gist.github.com/Anderson-Juhasc/e78e1ff349eb4299328038eedd3fe3b2 What do you think?

reneaaron commented 3 weeks ago

I've put together a little demo of how it's currently implemented in Alby:

https://codepen.io/reneaaron/pen/RwXMBZO?editors=1111

It's basically offering these events through an EventEmitter interface where you can subscribe / unsubscribe to these events with on() and off() methods:

window.nostr.on("accountChanged", async (e) => {      
  console.log("⚡️ window.nostr.on('accountChanged') fired!");
  const pubkey = await window.nostr.getPublicKey();
  console.log("🔑 new public key", pubkey);
});

@alexgleason I think Alby don't have multi accounts yet, try Nostrame: https://chromewebstore.google.com/detail/nostrame/phfdiknibomfgpefcicfckkklimoniej

Alby does have multi-account since basically ever. We really need to work on the UX of that :sweat_smile:

alexgleason commented 3 weeks ago

I thought so. I remembered seeing it in the Alby code while digging around.

But I think extending window.nostr with an extra property (.on) isn't the best way to do it. People will have to check something like if ('on' in window.nostr), and there is no proper TypeScript support for it. Plus it's based on some random event emitter library instead of Web Standard APIs.

How about this?

window.addEventListener("nostr.change", callback);

eg:

window.addEventListener("nostr.change", async (e) => {
  console.log("⚡️ nostr.change fired!");
  const pubkey = await window.nostr.getPublicKey();
  console.log("🔑 new public key", pubkey);
});

Reasons:

Event object:

As for the Event object returned in the callback, it would probably be an instance of CustomEvent. Since the pubkey or relays can both change, it could probably look like this:

new CustomEvent('nostr.change', { detail: ['pubkey'] }); // pubkey changed
new CustomEvent('nostr.change', { detail: ['relays'] }); // relay list changed
new CustomEvent('nostr.change', { detail: ['pubkey', 'relays'] }); // pubkey & relay list changed

I think this would help with adoption of the feature.

reneaaron commented 3 weeks ago

For reference:

https://github.com/nostr-protocol/nips/pull/701

Ultimately I think it's a personal preference thing:

Each implementation (EventEmitter vs. window events vs. typed methods like fiatjaf suggested in the linked issue) has it's own strengths and weaknesses. I wonder if any method is so much better to justify this breaking change?

1l0 commented 2 weeks ago

@neilck the suggestion of @alexgleason LGTM.

neilck commented 2 weeks ago

@1l0 I implemented window.nostr.on('accountChanged', accountChangedHandler); as per draft NIP-07: switching accounts in aka-extension simply because someone else had already implemented it this way, and needed a second implementation to get the NIP-07 change merged (which it never was).

@alexgleason 's point of making it easier for clients to integrate is good (window.addEventLister vs windows.nostr.on), as there are more clients than extensions, and we want wider adoption of this feature.

I think a better solution for clients is not having to listen and react to an account changed notification, but rather to modify NIP-07 function signEvent to accept an optional pubkey parameter. If the pubkey exists in the extension and the extension has the user's permission to signing events for that client with that pubkey, then it doesn't matter what the current pubkey is returned by getPublicKey.

bumi commented 2 weeks ago

I think all the options have some pros and cons and we can find arguments for each of them. Imo it's ultimately boils down to personal taste. I personally don't see how such a different syntax is making it easier for devs.

In the current implementation I like that it does not leak anything outside of the nip07 object. The same API works also when no window object is available.

vitorpamplona commented 2 weeks ago

modify NIP-07 function signEvent to accept an optional pubkey parameter

Not only sign, but all NIP-07 functions. This would allow clients to build interfaces that are merging multiple accounts at once, like how Gmail's App has an "All Inboxes" feed, and allowing users to reply, like, zap with any of the logged-in accounts (linkedin style), regardless of the current setting.

This should be done regardless of how the main account's onChange event is defined.

1l0 commented 2 weeks ago

@neilck That's a beautiful solution. In that case I believe clients want GetPublicKeys() instead of GetPublicKey().

neilck commented 2 weeks ago

@1l0 @vitorpamplona Yes, I think its better to have the client determine the account / accounts, and make the signer extension responsible for signing and permissions.

I wouldn't return all public keys from getPublicKeys() since it's leaky. I'd prefer to have getSharedPublicKeys(). My permission model is based off nos2x, where the extension remembers permissions per domain.

image

getSharedPublicKeys() would only return public keys that the user has given the client permission to access.

studioTeaTwo commented 2 weeks ago

I prefer the web standard style to the node.js style. Because it is based on event bubbing, it allows for open-and-wide ecosystem use (in browser-side javascript though).

window.nostr is not persisted but deletable, so Interaction of events between signers over but related to window.nostr is difficult. Although accountChanged seems to be no problem only to emit to window.nostr, thinking other event, for example, providerChanged, multisigCreated and so on, it would be helpful for being able to bubble to window.

neilck commented 1 week ago

I've updated akaprofiles extension based on this conversation.

Anyone have a client willing to integrate these changes?

  1. Added getSharedPublicKeys() which returns string array of all public keys where the user has granted a client (by domain) permission to view. Usually happens when user first logs into a client using extension.
  2. Updated signEvent so that if pubkey is specified in the event to sign, uses that pubkey's private key to sign the event, even if not the selected pubkey in the extension. All permissions still apply.

It didn't make sense to add a new signEventWithPubkey function, since a client can specify the pubkey they want right in the event.

I also created a Next.js NIP-07 tester that can test any signing extension by calling it like a client (including the current version of onAccountChanged).