MutinyWallet / mutiny-node

SDK behind Mutiny Wallet
https://mutinywallet.com
MIT License
187 stars 34 forks source link

Improve NWC support (return response from send payment, get_balance, make_invoice/lookup_invoice) #895

Closed rolznz closed 5 months ago

rolznz commented 9 months ago

Hi, we added a mutiny wallet connector to Bitcoin Connect so you can connect to your wallet in any lightning webapp and seamlessly execute lightning functionality, as long as the wallet is open on a second device (e.g. keep the Mutiny wallet open on your PC and then use Bitcoin Connect to connect to it on your phone).

Unfortunately there are some issues:

I created an issue here: https://github.com/getAlby/bitcoin-connect/issues/150

benthecarman commented 9 months ago

The response event is a problem with our relay.

What is the filter you're using to try and get it?

rolznz commented 9 months ago

@benthecarman we use this code:

const event = await this.signEvent(unsignedEvent);
const sub = this.relay.sub([
  {
    kinds: [23195],
    authors: [this.walletServicePubkey], 
    "#e": [event.id],
  },
]);
benthecarman commented 9 months ago

Ah okay, looks like we were only allowing filters that had the authors and #p defined. Have a change ready to fix that: https://github.com/MutinyWallet/blastr/pull/31

benthecarman commented 9 months ago

Get Balance and Make invoice just don't seem to be supported at all?

We haven't done get_balance because of the privacy concerns. I think we can add make_invoice without any issues though, probably only need to worry about rate limiting

TonyGiorgio commented 9 months ago

Is there any info or capabilities feature of NWC? Now that the new NWC extensions are going to add a ton more API calls, some we may or may not implement. I wonder what's the best way to indicate who supports what.

rolznz commented 9 months ago

@TonyGiorgio good point, it would be nice to add a methods property in the get_info RPC call at https://github.com/nostr-protocol/nips/pull/685/files

That is currently how WebLN does it: https://www.webln.guide/building-lightning-apps/webln-reference/webln.getinfo - I think this makes sense because it can be per-connection (not per-wallet-service as per the NIP-47 info event)

I will add a suggestion there.

benthecarman commented 9 months ago

There is the info event that you create when you create the NWC, but yeah adding to the get_info event makes sense

rolznz commented 8 months ago

We added a NWC connector to the Alby extension and it will be shipped soon. It uses the get_info event which unfortunately fails with Mutiny.

If you make any progress here please let us know. I think it's cool to use your Mutiny wallet and pay on desktop using Alby with WebLN :rocket:

TonyGiorgio commented 8 months ago

We added a NWC connector to the Alby extension and it will be shipped soon. It uses the get_info event which unfortunately fails with Mutiny.

If you make any progress here please let us know. I think it's cool to use your Mutiny wallet and pay on desktop using Alby with WebLN 🚀

What is it that you need out of the get_info call? So far we haven't needed to support it so I'm curious what is needed out of it besides firing off an invoice.

rolznz commented 8 months ago

@TonyGiorgio if the get_info call was supported and returned a list of methods we could then know what Mutiny does and does not support (get_balance for example which Ben mentioned is not possible right now).

Currently we use get_info to test the connection. However, if the call times out we can fallback to just assuming pay_invoice is the only supported method. Unfortunately that means we cannot actually test the NWC URL is valid or not.

Adding extra support to Mutiny for the extension methods is not necessary, it'll just improve the UX in the Alby extension (and Bitcoin Connect).

benthecarman commented 8 months ago

We definitely want to support most of the functions. I don't think get_info for that purpose will work well with mutiny however. It can only send responses when the wallet is open so it won't be able to register what it supports. The info event is really meant to signal what the connection supports

rolznz commented 8 months ago

@benthecarman I think it's ok to require the user to have their wallet open while using the Alby extension. We can add instructions there.

Currently the WebLN spec assumes the web application connected to a wallet can get a response directly from that wallet (e.g. within 60 seconds).

benthecarman commented 8 months ago

Okay if that is fine with you guys I guess that's okay.

rolznz commented 8 months ago

By the way, was https://github.com/MutinyWallet/blastr/pull/31 deployed? I still didn't seem to get a response event when I tested paying again today.

benthecarman commented 8 months ago

It is. I tested with the filter you gave me and was able to get a response

rolznz commented 8 months ago

@benthecarman thanks! it seems to be working now. The first payment I did failed with this error but then the next two succeeded. Can you check the error code?

{
  error: 'Failed to pay invoice: Payment timed out.',
  code: 'INSUFFICIENT_BALANCE'
}
benthecarman commented 8 months ago

It says payment timed out. That normally means the payment will fail. We send this if the payment hasn't completed after 30 seconds.

The insufficient balance code isn't the best but not sure of a better error code that'd fit.

rolznz commented 8 months ago

@benthecarman would it make sense to add one in the extensions PR? to me it just feels confusing because I had sufficient balance.

rolznz commented 8 months ago

Currently doing concurrent requests cause all the requests to time out (e.g. if there is an outstanding get_balance or get_info request, I never get a response for send_payment even though the payment is sent). Are there any restrictions on your side we need to be aware of?

benthecarman commented 8 months ago

Looks like PAYMENT_FAILED was defined just not in rust-nostr will add it to my PR.

benthecarman commented 8 months ago

Are there any restrictions on your side we need to be aware of?

Sounds like a bug on our side, I think I might know the cause

benthecarman commented 8 months ago

Currently doing concurrent requests cause all the requests to time out (e.g. if there is an outstanding get_balance or get_info request, I never get a response for send_payment even though the payment is sent). Are there any restrictions on your side we need to be aware of?

Added what I think is a fix in #905. We weren't responding to events with different commands instead of sending NOT_IMPLEMENTED so we would have to continually reprocess the events. If you left the wallet open for long enough it should eventually respond I believe unless we are running into limits on the relay side.

rolznz commented 8 months ago

@benthecarman thanks for looking into it. We currently set a timeout of 60 seconds so we also time out there, but I think more than 60 seconds is too long for a user to wait in our case. I will try again when the fix is merged