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

feat: send account switched messages to website #2504

Merged
merged 10 commits into from
Aug 2, 2023

Conversation

rolznz
Copy link
Contributor

@rolznz rolznz commented Jun 22, 2023

Describe the changes you have made in this PR

Send a message to the website when user switches account.

To listen to the event the website would add an event listener like:

window.addEventListener(
  "message",
  (event) => {
      if (event.data === "accountSwitched") {
         //... do something here
      }
  },
);

Link this PR to an issue [optional]

Fixes #2503

Type of change

  • feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

Add screenshots to make your changes easier to understand. You can also add a video here.

How has this been tested?

Manually

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)
  • Review security implications (we do not give any info about the account)
  • Review the action name. Should it have some sort of prefix?
  • How to document / promote this feature?

@github-actions
Copy link

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.

Don't forget: keep earning sats!

@bumi
Copy link
Collaborator

bumi commented Jun 26, 2023

best first also write a spec for window.nostr and window.webln

@MoritzKa
Copy link
Contributor

Thanks for initiating this. With the possibility of having different Accounts in the extension, users have from time to time the wrong account selected without knowing it.
If we follow the NIP standardization process, we would need a working implementation first. But we should propose it maybe here and let people know? https://github.com/nostr-protocol/nips/blob/master/07.md
After that I can reach out to coracle, iris. I know the developers of Primal and snort but did not have direct contact yet.

@rolznz
Copy link
Contributor Author

rolznz commented Jun 29, 2023

@MoritzKa I think the only way to successfully propose it is to create a PR. If we already have a working implementation then the PR may be reviewed more closely so maybe it is best to wait for that?

@rolznz rolznz self-assigned this Jul 3, 2023
@rolznz
Copy link
Contributor Author

rolznz commented Jul 10, 2023

I have assigned @pavanjoshi914 to help updating this PR to use the new .on() method for window.webln and window.nostr

@pavanjoshi914
Copy link
Contributor

pavanjoshi914 commented Jul 14, 2023

to handle events in the best way we can use EventEmitters node environments to provide such functionality. in our case, we can use an events package to handle events such as accountSelected.

here is the use case:

  1. webln.on("accountSelected", callback) : let users subscribe to the event and add to the list of events
    image

the package provides a complete set of methods required to handle events under EventEmitter class such as removing listener, emitting events, etc

other supported events: https://nodejs.org/dist/v11.13.0/docs/api/events.html#events_class_eventemitter

same eventEmitters are used by metaMask as well

@pavanjoshi914
Copy link
Contributor

2023-07-20.19-59-41.mp4

website registering event

webln.on("accountSelected", callback);

website remove the event, insure to pass same function to remove listener successfully

webln.removeListener("accountSelected", callback)

@pavanjoshi914 pavanjoshi914 requested a review from bumi July 21, 2023 06:57
@rolznz
Copy link
Contributor Author

rolznz commented Jul 21, 2023

@pavanjoshi914 after this is merged and deployed, it would be great to do these follow up tasks:

  • Create PR for updating NIP-07
  • Update https://github.com/WebBTC/webln-types
  • Update webln.guide with new .on and .off methods and the supported event types (only accountChanged for now)
  • Possibly we can submit PRs to snort.social and other open source nostr clients to add this feature

@@ -109,6 +113,27 @@ export default class WebLNProvider {
});
}

//webln events
on(...args: Parameters<EventEmitter["on"]>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this new code is duplicated in the two providers. I wonder if we can extend a common provider class or something. We could also move the common stuff like enabling and executing there.

I would do this as a follow up PR I guess.

rolznz and others added 5 commits July 27, 2023 16:08
signed-off-by: pavan joshi <pavanj914@gmail.com>
add explicit methods on, off and emit to be used in provider instead of extending whole class
cleaning up
correct nostr enable function problem which doesn't updates to latest value
mechanism to catch and emit events fired webln.emit and nostr.emit
signed-off-by: pavan joshi <pavanj914@gmail.com>
@pavanjoshi914 pavanjoshi914 force-pushed the feature/account-switched-message branch from c70ae39 to dde01a3 Compare July 27, 2023 11:06
don't break application if webln is enabled, instead make events call always after enabling webln

signed-off-by: pavan joshi <pavanj914@gmail.com>
@rolznz
Copy link
Contributor Author

rolznz commented Jul 28, 2023

Looks good to me. I cannot approve it as I originally made the PR.

CC @bumi @reneaaron

@rolznz rolznz merged commit 047276d into master Aug 2, 2023
4 of 6 checks passed
@rolznz rolznz deleted the feature/account-switched-message branch August 2, 2023 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants