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: NWC connector 🤙 #2898

Merged
merged 23 commits into from
Dec 24, 2023
Merged

feat: NWC connector 🤙 #2898

merged 23 commits into from
Dec 24, 2023

Conversation

reneaaron
Copy link
Contributor

@reneaaron reneaaron commented Nov 30, 2023

Describe the changes you have made in this PR

Fixes #2891

TODO

  • Implement missing connector methods
  • Store secret of NWC in account config
  • Instructions @ Connector setup
  • Draft specs for missing methods (lookupInvoice, listTransactions)
  • Implement new specs in the extension

Still quite some stuff work on, but I figured I'd share my excitement for this early on. 🔥

Copy link

socket-security bot commented Dec 19, 2023

New and updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@webbtc/webln-types 3.0.0 None +0 12 kB rolznz
@getalby/sdk 3.1.0...3.2.2 None +0/-0 131 kB

}

async getBalance(): Promise<GetBalanceResponse> {
const balance = await this.nwc.getBalance();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this methods might raise an error if the call fails (like for example if the method is not implemented).
so I think we need to catch them here and return a proper internal error

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should also be able to know if the method is supported here, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not here unless we call getInfo first somewhere in the connector, like in the init function. Is that fine?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking to do that. would be something against it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bumi I tested it with mutiny but mutiny doesn't even support get_info yet so the TestConnection doesn't even work. I will let them know.

Copy link
Contributor

@rolznz rolznz Dec 19, 2023

Choose a reason for hiding this comment

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

EDIT: if getInfo fails it should just fallback to ["pay_invoice"] as the supported methods. Something must be wrong here in the JS SDK, I'll check again.

Copy link
Contributor

Choose a reason for hiding this comment

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

fix here: getAlby/js-sdk#171

But it does mean that currently we have to wait for the timeout (60 seconds) before the fallback is returned. This is a bit unusable. I think maybe we should be able to provide a shorter timeout to Alby JS SDK for some NWC methods, what do you think?

@reneaaron reneaaron added the next-release To be included in the next release label Dec 20, 2023
@reneaaron reneaaron marked this pull request as ready for review December 22, 2023 09:52
@reneaaron reneaaron changed the title [WIP] feat: NWC connector 🤙 feat: NWC connector 🤙 Dec 22, 2023
@bumi bumi enabled auto-merge (squash) December 24, 2023 14:39
@bumi bumi disabled auto-merge December 24, 2023 14:39
@bumi bumi merged commit ea8cc43 into master Dec 24, 2023
7 of 8 checks passed
@bumi bumi deleted the feat/nwc branch December 24, 2023 14:39
bumi added a commit that referenced this pull request Dec 24, 2023
* master:
  feat: NWC connector 🤙 (#2898)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release To be included in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Nostr Wallet Connect
4 participants