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

Add accountsChanged event for NIP-07 #1552

Open
Anderson-Juhasc opened this issue Oct 29, 2024 · 17 comments
Open

Add accountsChanged event for NIP-07 #1552

Anderson-Juhasc opened this issue Oct 29, 2024 · 17 comments

Comments

@Anderson-Juhasc
Copy link

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
Copy link
Collaborator

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

@staab
Copy link
Member

staab commented Oct 29, 2024

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
Copy link

Kinda need this too.

@alexgleason
Copy link
Member

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

@Anderson-Juhasc
Copy link
Author

Anderson-Juhasc commented Oct 29, 2024

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

@Anderson-Juhasc
Copy link
Author

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

@reneaaron
Copy link

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 😅

@alexgleason
Copy link
Member

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:

  • window already has an addEventListener method on it, so users can call it directly without any sort of prior checks, and it will work regardless of the browser extension used. If the browser extension doesn't support it (or if there's no browser extension at all), the callback will just never be called.
  • The name nostr.change is the same tense as Web Standard events, eg change. Using a dot prefix to namespace it is kind of arbitrary, but is a common practice in libraries.

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
Copy link

For reference:

#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
Copy link

1l0 commented Nov 3, 2024

@neilck the suggestion of @alexgleason LGTM.

@neilck
Copy link
Contributor

neilck commented Nov 4, 2024

@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
Copy link

bumi commented Nov 4, 2024

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
Copy link
Collaborator

vitorpamplona commented Nov 4, 2024

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
Copy link

1l0 commented Nov 4, 2024

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

@neilck
Copy link
Contributor

neilck commented Nov 4, 2024

@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
Copy link

studioTeaTwo commented Nov 6, 2024

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
Copy link
Contributor

neilck commented Nov 9, 2024

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).

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

No branches or pull requests

10 participants