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: Add window.alby methods #1916

Merged
merged 16 commits into from
Jun 7, 2023
Merged

Conversation

AaronDewes
Copy link
Contributor

@AaronDewes AaronDewes commented Dec 23, 2022

Describe the changes you have made in this PR

This adds a window.alby API and adds an initial method to it: window.alby.addAccount allows websites to register an account in Alby.

Link this PR to an issue [optional]

Fixes #ISSUE-NUMBER

Type of change

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

Screenshots of the changes [optional]

grafik

How has this been tested?

Manually calling the function from the browser dev tools. I'll create a demo app later.

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)

Things that are still missing

  • Accounts should be better validated before adding them
  • Better type definitions for window.alby (Maybe with a custom library similar to the existing webln lib on npm?)
  • Documentation

I opened this PR to gather feedback on this API. I already suggested it in the Alby telegram channel.

@github-actions
Copy link

github-actions bot commented Dec 23, 2022

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


This build is brought to you by: Moritz (who recently dropped 1000 sats):

Merry Christmas to the Alby team

Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@AaronDewes AaronDewes mentioned this pull request Dec 23, 2022
3 tasks
@bumi
Copy link
Collaborator

bumi commented Dec 28, 2022

thanks for this PR and tackling this! looks pretty good, I will review this asap

@AaronDewes AaronDewes marked this pull request as ready for review January 3, 2023 18:56
Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

For more clear separation I am thinking about also splitting up the content script code into a specil alby script.
then we would have one for webln, nostr, and alby.
Eventhough this means some code duplication (which we can also refactor into some helpers likely) I think this separation makes it a bit clearer and easier to follow to flow.

src/extension/content-script/onend.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

After chatting with @reneaaron we think it's best to move the content script for this in its own file - to have this separate. Then every provider has it's own corresponding content script and we are fully flexible there and those are independent of each other.

but we don't do an enable call there as we always prompt the user anyway.

wdyt?
If ok, then we get this in the next release. :shipit:

@bumi
Copy link
Collaborator

bumi commented Jan 24, 2023

@AaronDewes do you still have time for this? otherwise maybe @im-adithya could you also help here?

@bumi bumi requested a review from rolznz February 23, 2023 23:26
@rolznz
Copy link
Contributor

rolznz commented Feb 28, 2023

After chatting with @reneaaron we think it's best to move the content script for this in its own file - to have this separate. Then every provider has it's own corresponding content script and we are fully flexible there and those are independent of each other.

but we don't do an enable call there as we always prompt the user anyway.

wdyt? If ok, then we get this in the next release. :shipit:

Without an enable call the user will not be able to reject further requests. Is this ok?

@rolznz
Copy link
Contributor

rolznz commented Feb 28, 2023

I agree it would be good to split this into a separate file. With 3 files and much duplication I think it would be best to refactor this, but I guess could be done in a follow up MR. Maybe we can also migrate these files to Typescript.

Why are these files called "onend"?

@reneaaron
Copy link
Contributor

I agree it would be good to split this into a separate file. With 3 files and much duplication I think it would be best to refactor this, but I guess could be done in a follow up MR. Maybe we can also migrate these files to Typescript.

👍

Why are these files called "onend"?

They are added via at document_end in the manifest.json:

image

@im-adithya
Copy link
Member

im-adithya commented Apr 21, 2023

To add an account, do

await alby.enable();
await alby.addAccount(
  "name", "lndhub", 
  {  
      lnAddress: "name@getalby.com", 
      login: "xxxxx",
      password: "xxxx", 
      url: "https://ln.getalby.com"
  });

src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/app/screens/ConfirmAddAccount/index.tsx Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
@reneaaron
Copy link
Contributor

@im-adithya Can you please address the review feedback in this PR. Let's get this merged! 🚀

@im-adithya
Copy link
Member

@reneaaron Done!

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Reading the code it seems there should be a success message but there is none.

I think that adding the success message (and delaying the popup from closing) could be tricky, so I would maybe just remove it for now.

There are efforts to introduce success messages in #2171 that we could reuse here.

src/app/screens/ConfirmAddAccount/index.tsx Outdated Show resolved Hide resolved
src/extension/ln/alby/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Pushed some last improvements and did some final testing. Published some docs here:

https://guides.getalby.com/alby-extension-apis/

tACK

@im-adithya im-adithya merged commit 0369aeb into getAlby:master Jun 7, 2023
5 of 6 checks passed
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.

None yet

6 participants