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

spark-wallet able to detect whether xpay-handle-pay is true or false #7953

Open
hMsats opened this issue Dec 18, 2024 · 10 comments
Open

spark-wallet able to detect whether xpay-handle-pay is true or false #7953

hMsats opened this issue Dec 18, 2024 · 10 comments

Comments

@hMsats
Copy link
Contributor

hMsats commented Dec 18, 2024

"version": "v24.11-18-g2c8d9d0" (master)

Main spark-wallet wallet offer: lno1pgp79x4pzcssxxtelekwpe57eekenf9uffanpmfz4r9cwq7a0hfdltmpncr3ylst
Test spark-wallet tries to pay some msats to main wallet

When I use (my version of) spark-wallet to pay, something still doesn't work well:
If xpay-handle-pay=false, the payment goes through but after I set xpay-handle-pay=true I get "Invalid parameter amount_msat (should be a millisatoshi amount): token 'null'".

I don't understand how this can happen. I know one could say it's a spark-wallet thingy but spark-wallet does the same thing, it doesn't know that pay is being redirected to xpay.

If I do it all on the command line everything works just fine.

How is it possible that spark-wallet is able to detect whether xpay-handle-pay is false or true?

Maybe someone has an idea?

Older version example (what happens on the test (paying) lightning node) but it's still that way (starts with xpay-handle-pay=true):

< 2024-12-17T18:29:13.499Z INFO    plugin-cln-xpay: Redirecting pay->xpay
< 2024-12-17T18:29:13.499Z INFO    plugin-cln-xpay: JSON COMMAND xpay-as-pay: Invalid parameter amount_msat (should be a millisatoshi amount): token 'null'
< 2024-12-17T18:31:53.030Z INFO    lightningd: setconfig: xpay-handle-pay false (updated /media/ssd/.lightning_test/bitcoin/config:2)
< 2024-12-17T18:32:08.702Z INFO    plugin-pay: cmd 17211/cln:pay#34952 partid 0: Paying invoice bolt11=lni1qqgrww60g0yacd7x3uektfrrnlnuwzqz4lyq5gtsd3shjetjtacxjvtrdvurgun8dd3xuutp8p4rq6rfxd3kcwfsx343vggrr9ulum8qu60vumve5j7y57esa5323ju8q0whm5kl4aseupcj0c94sggzurdgxt4q8vqvzpmnr6e0q4wd60ty5u6t380scasapj78lz4qvcy9j0rvdehnz7nrwdenjvnhd43njetev9h8wer2wdkhj6n80pcnjwrc894kvum3wvuryvmxwf68smn3v4nkcunh8qu8vctvx4erwe9qnqp3j707dnswd8kwdkv6f0z20vcw6g4gewrs8hta6t067cv7quf8uzcrfyhzzh9y0uqx0lkd9228rvy2ctnc6v9vkec7eghqe777fthnt45qzqk2gduwdae0kcka920wnft23a2y37zcpmunfxayff6j30kkqn6pjsqr9majkpa5tsgs7x5pd498p5pmw9ryrag7c3k49396wd62llmerrc7swy2x0amgtj9swyyx9933sl9cyymlgsuqqqqqqqqqqqqqqqjqqqqqqqqqqqqq8fykt06c5sqqqqqpfqyvasu829gyzgh5knnc827x8nwly72nxupd6ugn5xxcdhssp8qfwll4xqydqfhp2sz4lytqggrr9ulum8qu60vumve5j7y57esa5323ju8q0whm5kl4aseupcj0c9lqsxy20nalu8mayatrs4jwgtrpl8lk032wk9lts5h9xfns882ute927k5fyd5cl94vuhpwq6umvptvaqk4za2vvpv76z3h79xu23ydvpky
< 2024-12-17T18:32:08.760Z INFO    plugin-pay: cmd 17211/cln:pay#34952 partid 0: Payment fee constraint 225msat is below exemption threshold, allowing a maximum fee of 5000msat
< 2024-12-17T18:32:08.800Z INFO    lightningd: Sending 45000msat in onion to deliver 45000msat

Fun fact: I bought a Royal Club tonic at Bitcoin Amsterdam with renepay (from the command line): photo

@hMsats
Copy link
Contributor Author

hMsats commented Dec 18, 2024

Solved it in spark-wallet:

src/cmd.js:

, async _pay(paystr, ...args) {
    this.emit('paying', paystr)
    // doesn't work when xpay-handle-pay=true
    //const pay_result = await this.pay(paystr, ...args)
    const pay_result = await this.pay(paystr)

as far as I understand it, there are extra parameters that can't be handled by pay when xpay-handle-pay=true. I don't want to spam the Issues, so closing.

@hMsats hMsats closed this as completed Dec 18, 2024
@Lagrang3
Copy link
Collaborator

As of 24.11.1 xpay as pay will handle amount_msat, retry_for, maxfee, partial_msat, maxfeepercent and exemptfee,
besides the payment invoice bolt11. Legacy pay has more parameters.
To completely take over pay we would have to eat all of those, possibly ignoring their values as they are not useful
to xpay.
The current design choice is "xpay as pay: I will try to handle this subset of parameters, but if you ask for more
I better redirect to pay."

There is another detail, RPC parameters can arrive either as an array or values or a dictionary kind object.
When getting an array with more than one entry we are again falling back to pay.

In your case I guess xpay is receiving a dictionary of parameters, but with null values on those parameters that
are not specified. Probably something like this: {"arg": null}

@Lagrang3 Lagrang3 reopened this Dec 19, 2024
@Lagrang3
Copy link
Collaborator

Solved it in spark-wallet:

src/cmd.js:

, async _pay(paystr, ...args) {
    this.emit('paying', paystr)
    // doesn't work when xpay-handle-pay=true
    //const pay_result = await this.pay(paystr, ...args)
    const pay_result = await this.pay(paystr)

as far as I understand it, there are extra parameters that can't be handled by pay when xpay-as-pay=true. I don't want to spam the Issues, so closing.

I think this could break your wallet for the case in which the invoice has no amount and the user specifies a custom amount.

@Lagrang3
Copy link
Collaborator

It's our fault.
We are getting an array of arguments, some of them could be null.
Then we are accepting it if we have one or two, otherwise fallback to pay.

Consider the case we accept the array of params, but one of them is null. eg: ["lnbcrt1u1pn...", null]
because spark does not specify the next parameter amount_msat. This is legal (https://docs.corelightning.org/reference/lightningd-rpc#json-commands). But we wrongly assume we have been given amount_msat
and build a param dictionary with a null value.

@hMsats
Copy link
Contributor Author

hMsats commented Dec 19, 2024

You are right! I I thought I've tested it also for an invoice without an amount and thought it worked but now I see it doesn't (amount_msat required). It does work for bolt12 payments without an amount. Thanks a lot! When it's corrected in CLN, I will put everything back in the old state (in spark-wallet).

@hMsats
Copy link
Contributor Author

hMsats commented Dec 19, 2024

@Lagrang3 You can change the title if you want

@hMsats
Copy link
Contributor Author

hMsats commented Dec 19, 2024

#7958 solved it for me! Reversed the above changes in spark-wallet and paying invoices without an amount is (of course) no problem anymore. This issue has been resolved (spark-wallet behaves the same with xpay-handle-pay true or false) 🥇

@hMsats
Copy link
Contributor Author

hMsats commented Dec 20, 2024

@Lagrang3 I see both xpay-as-pay and xpay-handle-pay is this correct? The as only in plugin/xpay/xpay.c: sorry, is (probably) correct. I totally forgot about the setconfig

@hMsats hMsats changed the title spark-wallet able to detect whether xpay-as-pay is true or false spark-wallet able to detect whether xpay-handle-pay is true or false Dec 21, 2024
@satslawyer
Copy link

@Lagrang3 Between 24.11 and 24.11.1 the feature metnioned above when xpay falls back to pay seems to have broken Zeus wallet:

2024-12-27T21:53:22+00:00 2024-12-27T21:53:22.800Z INFO plugin-cln-xpay: Not redirecting pay (only handle 1 or 2 args): {"rpc_command":{"jsonrpc":"2.0","method":"pay","params":["LNBC52990N1PNK7GVMPP5LEQRW496LLWQWUF8JLQELVDAPP4Z0GQKPSM9E48PU4ZDPC35MC8SDQQCQZZSXQYZ5VQSP5XEATTJHXRWH454NQN6QSMHSXQMENN3PTDD7K92X0H875TZV3CD2Q9P4GQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQPQYSGQPMYF509JQKTCT46VG75UFNR24H5E97FX3Q8VQGGGA3A5PSEMV998VMKAZRQ9RKJ5YZ4DEJU0U6KGCKWJ8AS8VQ0QQ7N9GZT5S8TYSRGPY49VQC",null,null,null,"5.0",null,null,null,null,null,null,null],"id":"39"}}

On 24.11 this just worked. On 24.11.1 notice the message that xpay is not handling pay because of all the extra arguments that Zeus passes in.

(Using the new Start9 CLN package version 24.11.1~1)
cc @Dominion5254

@satslawyer
Copy link

@chrisguida and I tested it further and we discovered the problem was actually C-Lightning-Rest, updating to CLNRest fixed the issue on both 24.11 and 24.11.1. Apologies for the confusion, everything works now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@Lagrang3 @hMsats @satslawyer and others