getAlby / js-sdk

JavaScript SDK for the Alby OAuth2 Wallet API and the Nostr Wallet Connect API.
https://npmjs.com/package/@getalby/sdk
62 stars 15 forks source link

Alby NWC implementation doesn't return get_balance as a supported method #211

Closed verbiricha closed 2 months ago

verbiricha commented 6 months ago

I'm experimenting with Alby NWC with nwc.getalby.com. I created a NWC connection that supports reading balance, sending payments and creating invoices. However, when connecting to it with the SDK only sendPayment is listed in the methods array of the info response, is this a bug in Alby? Thanks in advance!

rolznz commented 6 months ago

Hi @verbiricha

What version of JS SDK are you using? I created a new connection string and executed the following and it seems to work ok.

WebLN interface:

node examples/nwc/get-info.js 
{
  methods: [
    'sendPayment',
    'makeInvoice',
    'lookupInvoice',
    'listTransactions',
    'getBalance',
    'getInfo'
  ],
  node: { alias: 'getalby.com', pubkey: '', color: '' },
  supports: [ 'lightning', 'nostr' ],
  version: 'Alby JS SDK'
}

or with the client directly:

node examples/nwc/client/get-info.js 
{
  alias: 'getalby.com',
  color: '',
  pubkey: '',
  network: 'mainnet',
  block_height: 0,
  block_hash: '',
  methods: [
    'pay_invoice',
    'make_invoice',
    'lookup_invoice',
    'list_transactions',
    'get_balance',
    'get_info'
  ]
}

Could you possibly share some code so I can look into the issue further?

rolznz commented 6 months ago

@verbiricha ah, I see - you need the get_info permission in order to request the info which includes the supported methods. Without it, the info response will fall back to only supporting send_payment.

rolznz commented 6 months ago

Note: I think this is a problem - returning supported methods should not require the same permissions as getting info about the node (like the node pubkey). It's not really an issue with nwc.getalby.com since users share a node, but this won't be the case in self-custodial NWC. I will create an issue for that here: https://github.com/getAlby/hub/issues/188 CC @bumi

bumi commented 6 months ago

hmm, the info event (kind 13194) can be used for this? I think we have two levels here: the general info event (kind 13194) which has the information of what capabilities the wallet has. and then the get_info level which is on the connection level - what permissions the user gives to the app.

So apps can check before connecting if the wallet has the required capabilities and I guess you're right once an app connection exists the get_info should always be included and no specific permission for this should be required? Even though this has the downside that a user might not want to share the information returned by the get_info call (e.g. I only want to allow an app to send a payment and not to know my pubkey. does this make sense?)

rolznz commented 6 months ago

@bumi I think we should keep the permission for get_info to restrict access to the node pubkey and other node info. Most apps do not need this. (We are also making this change in the Alby Extension)

What about we return partial information in the get_info call if the permission is not provided? or we propose another NIP-47 call to get the list of allowed methods which is not linked to get_info (I think I was wrong to suggest the list of supported methods should be returned in get_info)

bumi commented 6 months ago

This sounds like very hard to communicate to the user to understand what permission is given there, doesn't it? and a bit annoying to implement because it works differently than the other permissions. But this is up to the wallet and we should not care about this here in the SDK as client.

not sure if throwing more functions at NIP47 helps. I'd be afraid that this just leads to confusion and not all implementations supporting the functions?

hmmm, I actually now think the SDK here should assume it always gets the methods from the info call - ideally apps before check the general capabilities.

then it's a wallet issue.

verbiricha commented 6 months ago

you need the get_info permission in order to request the info which includes the supported methods. Without it, the info response will fall back to only supporting send_payment.

👍 I see, I could use the 13194 event for figuring out which methods the wallet provider supports (using getWalletServiceSupportedMethods) but that doesn't give me accurate information about what methods the connection supports.

My issue arose because when giving these permissions to a connection:

Screenshot 2024-03-10 at 12 28 59

the get_info call is not allowed for my connection so calling getInfo was defaulting to only return sendPayment permission. I was puzzled by this since I could call getBalance and get a result. Now I see why this happens.

I think I was wrong to suggest the list of supported methods should be returned in get_info

I agree, without giving get_info permission to the connection there is no way to figure out which methods are allowed on the connection if not by trial and error. Could be every method from the 13194 or a subset of them like in my example.

I was calling get_info only because I was interested in the methods list. get_info is conflating connection-level method permissions with node info so perhaps querying the connection capabilities should be a call that is available for every connection: something like get_methods.

Sorry for my confusion, I should have read the spec more carefully 😅

bumi commented 6 months ago

@verbiricha this is an important discussion.

and actually I only think that the Alby implementation of the get_info call is not ideal. as mentioned here the get_info cal should always work and the app should get information about the connection with this call. so the permission config is not ideal in Alby NWC.

rolznz commented 3 months ago

@bumi so should we just make the NWC getinfo call always public and remove the permission from NWC? I'll create an issue there and close this one.

rolznz commented 2 months ago

In Alby Hub the get_info call will now return the permitted methods even if get_info is not in the list of permissions (But node info such as the pubkey will not be accessible)

See https://github.com/getAlby/nostr-wallet-connect-next/pull/441

This is not fixed in the other Alby NWC implementations yet.

rolznz commented 2 months ago

Closing as we are no longer planning to maintain nwc.getalby.com, this is fixed in Alby Hub.